Add facility field changes to contributor notification emails #649
Conversation
9ec1543
to
1231cbb
Compare
This PR includes changing one instance of |
1231cbb
to
6e179ce
Compare
Heads up that there is a merge conflict in one of the email template files. |
6e179ce
to
6d1860f
Compare
Rebased on develop and resolved the conflict. |
src/django/api/models.py
Outdated
help_text='A description of the facility') | ||
verification_method = models.TextField( | ||
null=False, | ||
blank=True, | ||
verbose_name='Verification method', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very trivial but I just read that the Django doc recommend not capitalizing the first letter of these verbose name fields since Django will capitalize them as it needs: https://docs.djangoproject.com/en/2.2/topics/db/models/#verbose-field-names
However, if it doesn't matter feel free to ignore this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update these to be consistent with the Django docs and your new fields in #650. I will also make the required update to make sure that we do the capitalization when preparing the email context object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'office_country_code', | ||
'office_phone_number', | ||
'parent_company', | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#650 will have a few new fields to add to this list, but it should be easy to adjust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This is working well. If it's possible to wait on merging this, we could consider waiting to do so until #650 is in, which would save a lot of having to rebase migrations.
I agree, and will hold on this until #650 is done so I can incorporate the additional fields. |
227fd9a
to
e8344a9
Compare
We are adding these names to support sending emails that summarize the changes made to a facility. We will only be reporting changes to some of these files, but we are adding `verbose_name` to all of the fields for consistency.
When notifying contributors of changes to a facility profile made by an approved claimant, we want to include a list of the values that were changed. This method provides the raw data for creating that list and has several important features. - Fields to be reported on are whitelisted to prevent the future addition of a private field from accidentally leaking to the public. - There are field values conditionally restricted from public view and this method checks those conditions before reporting changes. - One value is foreign key to `Contributor` and this method handles serializing it to a single string value by using the name of the contributor.
e8344a9
to
73da0a6
Compare
When notifying contributors when facility claimants change the profile of a facility we want to include the specific public details that were changed. The non-standard position of the `endfor` in the plain text email template is intentional and ensures that extra blank lines are not included. When capitalizing the verbose name, we are manually uppercasing the first letter because Python's built-in `capitalize` method changes all characters but the first to lower case, which is not what we want in the case of a verbose name like "facility name in English."
73da0a6
to
d62ba88
Compare
I retested this after rebasing and adding support for the new fields added in #650. I changed all the fields on a claim but did not enable any switches. I got this expected list in the email:
I switched on the public fields and changed their values and got this expected list in the email:
|
Thanks for the review. Will merge after CI passes. |
Overview
Add a list of updated field values to contributor email notifications.
Connects #635
Testing Instructions
Facility Name
to "Changed Name"Phone Number
to "Changed Phone Number"Publicly Visible
switch next toPhone Number
Website
to "change.org"./scripts/server
to ensure that emails are displayed in the console.Checklist
fixup!
commits have been squashed