Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mnch 985 contentrepo whatsapp template #223

Merged
merged 6 commits into from
Jan 8, 2024

Conversation

Buhle79
Copy link
Contributor

@Buhle79 Buhle79 commented Jan 3, 2024

Purpose

Describe the problem or feature in addition to a link to the issues.
Template was store with uppercase and throw an error when they try to retrieve it as template names are stored with lower case.

Approach

How does this change address the problem?
Convert whatsapp template name to lower case before we store it.

Open Questions and Pre-Merge TODOs

  • Use github checklists. When solved, check the box and explain the answer.

Learning

Describe the research stage

Create a whatsapp template locally and modify unittests

Links to blog posts, patterns, libraries or addons used to solve this problem
None

Blog Posts

@Buhle79
Copy link
Contributor Author

Buhle79 commented Jan 3, 2024

Hi @jerith for review please

home/models.py Outdated
@@ -605,6 +605,8 @@ def latest_revision_rating(self):

@property
def whatsapp_template_prefix(self):
if self.whatsapp_title:
self.whatsapp_title = self.whatsapp_title.lower()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to set self.whatsapp_title to lowercase here, or just return a lowercase version of it?

Also, on the line below we call .replace() on it unconditionally. That means we don't need to check if it's non-empty before lowercasing it.

Copy link
Contributor

@rudigiesler rudigiesler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I think we might want a data migration, to lowercase all the existing template names that are in the database, see https://github.com/praekeltfoundation/contentrepo/blob/main/home/migrations/0030_contentpage_whatsapp_template_name.py for an example, but happy for that to happen in a separate PR.

@Buhle79 Buhle79 merged commit c91db43 into main Jan 8, 2024
4 checks passed
@Buhle79 Buhle79 deleted the MNCH-985-contentrepo-whatsapp-template branch January 8, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants