Email list contributors about facility claims #611

kellyi · 2019-06-17T18:49:53Z

Overview

email list contributors when a facility claim is approved
email list contributors when an approved facility claim's profile
details have been updated
update tests to check for facility claim emails to contributors

Connects #578
Connects #577

Testing Instructions

serve this branch with the default set of fixtures
sign in as c2@example.com and claim a facility
sign in as a superuser and approve the facility claim
verify that you see email/s to the facility list contributors for a facility that has been claimed
sign back in as c2@example.com
update the claimed facility profile and save, then verify that you see email/s to the facility list contributors for the facility that has had its claim updated

Checklist

fixup! commits have been squashed
CI passes after rebase
CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

jwalgran · 2019-06-17T22:26:37Z

Looking at this now.

jwalgran

I manually tested both notification and verified that they worked correctly. I made a few suggestions and I opened a draft issue to cover opting out of these notifications (#614)

jwalgran · 2019-06-17T23:05:01Z

src/django/api/views.py

@@ -1641,7 +1646,8 @@ def get_claimed_details(self, request, pk=None):
            )

            updated_claim = FacilityClaim.objects.get(pk=pk)
-
+            send_claim_update_notice_to_list_contributors(request,


We should consider wrapping this notification in a try block and posting the exception to Rollbar without raising. It may be confusing for the claimant to get a failure message after we successfully committed their change to the database.

Sounds good. This is in 8406351

I don't know the full range of exceptions that can get thrown when sending an email, so I just used except Exception for the try/except blog. In order to be parsimonious about what data get sent to Rollbar I only logged the facility claim id, although I don't know what is included in sys.exc_info() so it may be that that needs to go too.

jwalgran · 2019-06-17T23:05:22Z

src/django/api/views.py

@@ -1473,6 +1475,9 @@ def approve_claim(self, request, pk=None):

            send_claim_facility_approval_email(request, claim)

+            send_approved_claim_notice_to_list_contributors(request,


We should consider wrapping this notification in a try block and posting the exception to Rollbar without raising. It may be confusing for the superuser to get a failure message after we successfully committed their approval to the database.

Sounds good. This is in 8406351

I don't know the full range of exceptions that can get thrown when sending an email, so I just used except Exception for the try/except blog. In order to be parsimonious about what data get sent to Rollbar I only logged the facility claim id, although I don't know what is included in sys.exc_info() so it may be that that needs to go too.

This screenshot shows the arguments and local vars that are visible in rollbar when we submit the stack trace. These values are at the item level, not the occurrence level, so they are not deletable.

jwalgran · 2019-06-17T23:07:52Z

src/django/api/tests.py

@@ -1686,6 +1710,33 @@ def test_approved_facility_claim_info_is_in_details_response(self):

        self.assertEqual(response_data['description'], 'new_description')

+    @override_switch('claim_a_facility', active=True)
+    def test_updating_claim_profile_sends_email_to_contributor(self):


Did we include a test for update notification but exclude a test for approval notification for a specific reason?

Oh, the approval test is there-is but it's really subtle and probably could be denotated more clearly. It is essentially this line, which checks that there are now 2 emails in the outbox on approval rather than just 1:

https://github.com/open-apparel-registry/open-apparel-registry/pull/611/files#diff-a5f8ed705a64b1fe29a3eeb6439265d3R1406

Rather than creating a new test I revised the test here to adjust its name and to add a comment about len(mail.outbox) == 2: 436301d

jwalgran · 2019-06-17T23:08:50Z

src/django/api/templates/mail/approved_facility_claim_contributor_notice_body.txt

+
+The facility is:
+
+  - Facility: {{ facility_name }}, {{ facility_address }}


Consider including the full facility country name in these notifications.

Done in 0d3d6a2

jwalgran · 2019-06-17T23:08:56Z

src/django/api/templates/mail/facility_claim_profile_update_contributor_notice_body.html

+        </p>
+        <ul>
+            <li>
+                Facility: {{ facility_name }}, {{ facility_address }}


Consider including the full facility country name in these notifications.

Added this in 0d3d6a2 and also added the full country name to the previous set of facility claim emails.

jwalgran · 2019-06-17T23:09:07Z

src/django/api/templates/mail/approved_facility_claim_contributor_notice_body.html

+        </p>
+        <ul>
+            <li>
+                Facility: {{ facility_name }}, {{ facility_address }}


Consider including the full facility country name in these notifications.

Added this in 0d3d6a2 and also added the full country name to the previous set of facility claim emails.

jwalgran · 2019-06-17T23:09:24Z

src/django/api/templates/mail/facility_claim_profile_update_contributor_notice_body.txt

+
+The facility is:
+
+  - Facility: {{ facility_name }}, {{ facility_address }}


Consider including the full facility country name in these notifications.

Sounds good. I added this in 0d3d6a2 and also added the full country name to the previous set of facility claim emails.

jwalgran · 2019-06-18T21:15:17Z

Looking at this now.

jwalgran · 2019-06-18T21:28:19Z

src/django/api/views.py

+
+    if ROLLBAR:
+        import rollbar
+        rollbar.report.exc_info(


Typo. report.exc_info -> report_exc_info

jwalgran · 2019-06-18T21:45:10Z

src/django/api/views.py

@@ -1473,6 +1475,9 @@ def approve_claim(self, request, pk=None):

            send_claim_facility_approval_email(request, claim)

+            send_approved_claim_notice_to_list_contributors(request,


This screenshot shows the arguments and local vars that are visible in rollbar when we submit the stack trace. These values are at the item level, not the occurrence level, so they are not deletable.

jwalgran

There is a typo in the Rollbar exception reporting but the other changes look good.

kellyi · 2019-06-18T22:05:38Z

There is a typo in the Rollbar exception reporting

Oops, thanks for catching that! I fixed this in af700d9

kellyi · 2019-06-19T00:34:49Z

Thanks for all your help with this! I'm going to squash and then merge.

- email list contributors when a facility claim is approved - email list contributors when an approved facility claim's profile details have been updated - add tests for contributor emails - catch exceptions thrown when trying to email contributors about facility claim approvals and profile changes, sending only the Facility Claim ID to Rollbar. Afterward, continue sending a response as usual.

kellyi force-pushed the ki/claim-a-facility-email branch from eb5a93b to eb42bc4 Compare June 17, 2019 20:16

kellyi changed the title ~~WIP Email list contributors about facility claims~~ Email list contributors about facility claims Jun 17, 2019

kellyi marked this pull request as ready for review June 17, 2019 20:21

kellyi requested a review from jwalgran June 17, 2019 20:21

kellyi assigned jwalgran Jun 17, 2019

jwalgran reviewed Jun 17, 2019

View reviewed changes

jwalgran assigned kellyi and jwalgran and unassigned jwalgran Jun 17, 2019

kellyi force-pushed the ki/claim-a-facility-email branch from eb42bc4 to 8406351 Compare June 18, 2019 14:14

jwalgran reviewed Jun 18, 2019

View reviewed changes

jwalgran approved these changes Jun 18, 2019

View reviewed changes

jwalgran removed their assignment Jun 18, 2019

kellyi force-pushed the ki/claim-a-facility-email branch from af700d9 to 72ce805 Compare June 19, 2019 00:35

kellyi merged commit 9259935 into develop Jun 19, 2019

kellyi deleted the ki/claim-a-facility-email branch June 19, 2019 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Email list contributors about facility claims #611

Email list contributors about facility claims #611

kellyi commented Jun 17, 2019 •

edited

jwalgran commented Jun 17, 2019

jwalgran left a comment

jwalgran Jun 17, 2019

kellyi Jun 18, 2019

jwalgran Jun 17, 2019

kellyi Jun 18, 2019

jwalgran Jun 18, 2019

jwalgran Jun 17, 2019

kellyi Jun 17, 2019

kellyi Jun 18, 2019

jwalgran Jun 17, 2019

kellyi Jun 18, 2019

jwalgran Jun 17, 2019

kellyi Jun 18, 2019

jwalgran Jun 17, 2019

kellyi Jun 18, 2019

jwalgran Jun 17, 2019

kellyi Jun 18, 2019

jwalgran commented Jun 18, 2019

jwalgran Jun 18, 2019

jwalgran Jun 18, 2019

jwalgran left a comment

kellyi commented Jun 18, 2019

kellyi commented Jun 19, 2019

		@@ -1473,6 +1475,9 @@ def approve_claim(self, request, pk=None):

		send_claim_facility_approval_email(request, claim)

		send_approved_claim_notice_to_list_contributors(request,


		The facility is:

		- Facility: {{ facility_name }}, {{ facility_address }}

Email list contributors about facility claims #611

Email list contributors about facility claims #611

Conversation

kellyi commented Jun 17, 2019 • edited

Overview

Testing Instructions

Checklist

jwalgran commented Jun 17, 2019

jwalgran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwalgran commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwalgran left a comment

Choose a reason for hiding this comment

kellyi commented Jun 18, 2019

kellyi commented Jun 19, 2019

kellyi commented Jun 17, 2019 •

edited