[OSDEV-1721] SLC. Customer Comms for SLC workflow#536
Conversation
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
had a problem deploying
to
Quality Environment
GitHub Actions
Failure
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
React App | Jest test suite - Code coverage reportTotal: 31.61%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Dedupe Hub App | Unittest test suite - Code coverage reportTotal: 56.14%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
Contricleaner App | Unittest test suite - Code coverage reportTotal: 98.91%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
Countries App | Unittest test suite - Code coverage reportTotal: 100%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
roman-stolar
temporarily deployed
to
Quality Environment
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
doc/release/RELEASE-NOTES.md (1)
34-43: New Email Template Announcements for SLC WorkflowThe updated "What's new" section under Release 1.31.0 now documents the addition of four new email templates for the Moderation/SLC workflow as specified in [OSDEV-1721]. This clear listing will help stakeholders and developers quickly understand the new communication channels implemented.
Static analysis suggests that the nested bullet points (lines 35–38) are indented by 4 spaces rather than the recommended 2 spaces (MD007). Adjusting this will improve the markdown formatting consistency.
Suggested diff:
- * Email #1 SLC additional contribution to existing production location - Pending moderation. - * Email #2 SLC contribution - Moderation complete. APPROVED. - * Email #3 SLC contribution - Moderation complete. REJECTED. - * Email #4 SLC new production location contribution - Pending moderation. + * Email #1 SLC additional contribution to existing production location - Pending moderation. + * Email #2 SLC contribution - Moderation complete. APPROVED. + * Email #3 SLC contribution - Moderation complete. REJECTED. + * Email #4 SLC new production location contribution - Pending moderation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
35-35: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
40-40: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
42-42: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
43-43: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
doc/release/RELEASE-NOTES.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
35-35: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-django-cov
There was a problem hiding this comment.
Could you please cover email sending with tests? At least some basic tests. I think we need to check that the emails are being sent after triggering these specific endpoints.
Here is an article on how to test email sending in Django: https://timonweb.com/django/testing-emails-in-django/
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/django/api/views/v1/moderation_events.py (1)
141-142:⚠️ Potential issueAdd conditional check for rejection status
The code unconditionally sends a rejection email when the event's source is SLC, without verifying that the event's status is actually "rejected". This could result in sending rejection emails for non-rejected events.
Apply this fix to ensure rejection emails are only sent when appropriate:
if event.source == ModerationEvent.Source.SLC: + if event.status == ModerationEvent.Status.REJECTED: send_slc_contribution_rejected_email(request, event)
🧹 Nitpick comments (2)
src/django/api/mail.py (2)
496-502: Consider separating claimed and unclaimed facility email templatesThe code checks whether a facility is claimed, but only uses this information as a boolean flag in the template. For better user experience, consider creating separate templates or significantly different content sections based on this status.
538-547: Add documentation about optional rejection reason attributesThe code uses
getattr()withNonedefault foraction_reason_text_cleanedandaction_reason_text_raw, implying these attributes may not always be available. Consider adding a comment explaining when these might be missing and how the template handles null values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/django/api/mail.py(3 hunks)src/django/api/views/v1/moderation_events.py(4 hunks)src/django/api/views/v1/production_locations.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/django/api/views/v1/production_locations.py
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-contricleaner-cov
🔇 Additional comments (1)
src/django/api/mail.py (1)
559-561:Details
❓ Verification inconclusive
Ensure emails are sent only to SLC contributors
Based on previous review comments, these emails should only be sent to contributors who created moderation events following the SLC flow. Verify that
moderation_event.contributor.admin.emailonly includes intended recipients.
🌐 Web query:
Does Django's send_mail function support sending emails to multiple recipients?💡 Result:
Yes, Django's
send_mailfunction supports sending emails to multiple recipients natively. Here's how it works:Key Implementation Details
Therecipient_listparameter accepts an array/list of email addresses:from django.core.mail import send_mail send_mail( subject='Your Subject', message='Email Content', from_email='[email protected]', recipient_list=['[email protected]', '[email protected]', '[email protected]'], # <-- Multiple addresses fail_silently=False )Important Considerations
- Visibility: All recipients will see other addresses in the "To" field[1][6].
- Alternatives for Privacy/Bulk:
- Use
send_mass_mailfor separate messages with shared SMTP connection[1][7]- Utilize
EmailMessageclass with BCC for hidden recipients[7][10]- Implement third-party packages like
django-multi-email-fieldfor advanced list management[5]Performance Tip: For sending >100 emails, consider:
# Using send_mass_mail for bulk operations from django.core.mail import send_mass_mail messages = [ ('Subject 1', 'Message 1', '[email protected]', ['[email protected]']), ('Subject 2', 'Message 2', '[email protected]', ['[email protected]']) ] send_mass_mail(messages)This maintains a single SMTP connection while sending individualized messages[1][7].
Citations:
- 1: https://mailtrap.io/blog/django-send-email/
- 2: https://mailtrap.io/blog/python-send-email/
- 3: https://www.abstractapi.com/guides/email-validation/django-send-email
- 4: https://opensource.com/article/22/12/django-send-emails-smtp
- 5: https://github.com/fle/django-multi-email-field
- 6: https://www.thedevspace.io/community/django-send-emails
- 7: https://docs.djangoproject.com/en/5.1/topics/email/
- 8: https://forum.djangoproject.com/t/send-contact-form-data-to-e-mail-address/8187
- 9: https://www.skysilk.com/blog/2020/how-to-send-emails-in-django/
- 10: https://dev.to/shivamrohilla/how-to-send-an-email-in-django-4pdd
Action: Verify SLC Contributor Email Filtering
Although Django’s
send_mailproperly supports sending emails to multiple recipients, please ensure that the value ofmoderation_event.contributor.admin.emailis indeed restricted to contributors who originated moderation events via the SLC flow. If not already handled elsewhere in the code, consider implementing a conditional check so that emails are dispatched only to SLC-approved contributors.
- Location:
src/django/api/mail.py(Lines 559-561)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/django/api/mail.py (1)
41-56: Consider simplifying return statements.Both URL generation functions use unnecessary parentheses that could be simplified.
-def make_pl_search_url(request): - return ( - "{}/contribute/single-location".format( - make_oar_url(request) - ) - ) +def make_pl_search_url(request): + return "{}/contribute/single-location".format(make_oar_url(request)) -def make_pl_claim_url(request, facility): - return ( - "{}/facilities/{}/claim".format( - make_oar_url(request), - facility.id, - ) - ) +def make_pl_claim_url(request, facility): + return "{}/facilities/{}/claim".format(make_oar_url(request), facility.id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/django/api/mail.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
🔇 Additional comments (5)
src/django/api/mail.py (5)
4-9: LGTM! Adding necessary imports for new functionality.The imports of
FacilityClaimandFacilityClaimStatusesare properly added to support the new email notification functions for the SLC workflow.
433-458: LGTM! Well-structured confirmation email for additional info.The function properly retrieves data from the moderation event, formats it, and sends the email using the appropriate templates. The use of
get()with defaults is a good practice for safely handling missing keys.
481-523: LGTM! Well-implemented approval email with claim status check.The function properly checks if the facility has any approved or pending claims before sending the email. This ensures relevant information is included in the notification.
526-562: LGTM! Robust implementation for rejection emails.The function handles potential missing attributes safely using
getattrwith default values and properly formats the email content. The inclusion of rejection reasons and relevant URLs provides good user experience.
461-478:Details
✅ Verification successful
Verify that empty context is intentional.
This function doesn't pass any context data to the templates. Ensure that the templates don't require any dynamic content.
🏁 Script executed:
#!/bin/bash # Description: Check if the templates expect any context variables # Check the text template for any variables echo "Checking text template for variables:" cat src/django/api/templates/mail/slc_new_location_confirmation_body.txt | grep -o "{{.*}}" || echo "No variables found in text template" # Check the HTML template for any variables echo "Checking HTML template for variables:" cat src/django/api/templates/mail/slc_new_location_confirmation_body.html | grep -o "{{.*}}" || echo "No variables found in HTML template"Length of output: 520
Email Template Context Verification Confirmed
The grep results show that both the text and HTML email templates contain no dynamic placeholders. This indicates that not passing any context is intentional. Please also verify that the subject template (located at
src/django/api/templates/mail/slc_new_location_confirmation_subject.txt) similarly does not require any dynamic content.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/django/api/tests/test_moderation_events_update_production_location.py (1)
171-172: Good email verification for successful location update.These assertions effectively verify that the email notification is sent with the correct subject when a production location is successfully updated. This aligns well with the PR objective of enhancing customer communications for the SLC workflow.
However, consider enhancing the test to also verify:
- The email recipient (to ensure it's sent to the correct user)
- Key elements of the email content (to verify the template is properly populated)
email = mail.outbox[0] self.assertEqual(email.subject, "Great News: your OS ID is ready!") +self.assertEqual(email.to[0], self.contributor.admin.email) +self.assertIn(self.os_id, email.body)src/django/api/tests/test_production_locations_create.py (1)
189-194: Good email verification for new location submission.The test now properly verifies that a confirmation email is sent when a new location is submitted, with the correct subject line. This ensures that users receive appropriate notifications about their submissions.
Similar to other tests in this PR, consider expanding the verification to include:
- The email recipient
- Key content verification
email = mail.outbox[0] self.assertEqual( email.subject, "Thank You for Your Submission – " "Your Data Is Now Being Reviewed" ) +self.assertEqual(email.to[0], self.user.email) +self.assertIn("Blue Horizon Facility", email.body)src/django/api/tests/test_production_locations_partial_update.py (1)
249-254: Good email verification for location update submission.The test now properly verifies that a confirmation email is sent when a location update is submitted, with the correct subject line. This ensures that users receive appropriate notifications about their update submissions.
As with the other test files, consider enhancing the verification to include:
- The email recipient
- Key content verification
email = mail.outbox[0] self.assertEqual( email.subject, "Thank You for Your Submission – " "Your Data Is Now Being Reviewed" ) +self.assertEqual(email.to[0], self.user.email) +self.assertIn("Blue Horizon Facility", email.body) +self.assertIn(self.production_location.id, email.body)src/django/api/tests/test_moderation_events_update.py (1)
115-120: Good email verification for rejected submissions.The test now verifies that when a moderation event is rejected, an appropriate email notification is sent with the correct subject line requesting additional information from the user. This ensures proper communication when submissions need more information.
To make the test more robust, consider enhancing the verification to include:
- The email recipient
- Key content verification
email = mail.outbox[0] self.assertEqual( email.subject, "ACTION REQUESTED: We need more information to process " "your submission and provide an OS ID" ) +self.assertEqual(email.to[0], self.contributor.admin.email) +self.assertIn("rejected", email.body.lower())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/django/api/tests/test_moderation_events_update.py(2 hunks)src/django/api/tests/test_moderation_events_update_production_location.py(2 hunks)src/django/api/tests/test_production_locations_create.py(2 hunks)src/django/api/tests/test_production_locations_partial_update.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/django/api/tests/test_production_locations_partial_update.py (1)
Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:21-65
Timestamp: 2024-12-12T14:59:19.694Z
Learning: In `src/django/api/tests/test_moderation_events_add_production_location.py`, the tests have been refactored, and the use of `POST` methods is intentional. Future suggestions to change HTTP methods in these tests may not be necessary.
src/django/api/tests/test_production_locations_create.py (1)
Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:21-65
Timestamp: 2024-12-12T14:59:19.694Z
Learning: In `src/django/api/tests/test_moderation_events_add_production_location.py`, the tests have been refactored, and the use of `POST` methods is intentional. Future suggestions to change HTTP methods in these tests may not be necessary.
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-django-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
🔇 Additional comments (4)
src/django/api/tests/test_moderation_events_update_production_location.py (1)
6-6: Added import for Django's mail module.Good addition of the mail module import to support the new email testing assertions. This is necessary for accessing the mail outbox in the tests.
src/django/api/tests/test_production_locations_create.py (1)
6-6: Added import for Django's mail module.The mail module import is correctly added to support the new email testing functionality.
src/django/api/tests/test_production_locations_partial_update.py (1)
6-6: Added import for Django's mail module.The mail module import is correctly added to support the new email testing assertions.
src/django/api/tests/test_moderation_events_update.py (1)
3-3: Added import for Django's mail module.The mail module import is correctly added to support the new email testing assertions.
# Conflicts: # doc/release/RELEASE-NOTES.md # src/django/api/mail.py # src/django/api/views/v1/moderation_events.py
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
doc/release/RELEASE-NOTES.md (1)
35-39: Well-documented email template additions in release notes.The release notes clearly document the addition of new email templates for the Moderation/SLC workflow, listing all four new email templates with descriptive names that explain their purpose.
Although there are minor indentation inconsistencies in the markdown (4 spaces vs 2 spaces expected), these don't affect the rendered output and can be addressed in a future cleanup.
- * Email #1 SLC additional contribution to existing production location - Pending moderation. - * Email #2 SLC contribution - Moderation complete. APPROVED. - * Email #3 SLC contribution - Moderation complete. REJECTED. - * Email #4 SLC new production location contribution - Pending moderation. + * Email #1 SLC additional contribution to existing production location - Pending moderation. + * Email #2 SLC contribution - Moderation complete. APPROVED. + * Email #3 SLC contribution - Moderation complete. REJECTED. + * Email #4 SLC new production location contribution - Pending moderation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
36-36: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
doc/release/RELEASE-NOTES.md(1 hunks)src/django/api/mail.py(3 hunks)src/django/api/views/v1/moderation_events.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/django/api/views/v1/moderation_events.py
🧰 Additional context used
🪛 Ruff (0.8.2)
src/django/api/mail.py
51-51: Undefined name make_oar_url
(F821)
59-59: Undefined name make_oar_url
(F821)
🪛 markdownlint-cli2 (0.17.2)
doc/release/RELEASE-NOTES.md
36-36: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
39-39: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-dd-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-fe-code-quality
- GitHub Check: run-django-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-fe-cov
🔇 Additional comments (4)
src/django/api/mail.py (4)
5-12: Imports are properly organized and structured.The imports are well-organized with related imports grouped together. Adding
FacilityClaimandFacilityClaimStatusesallows for proper integration with the claim functionality in the new email templates.
470-496: Email function for additional information confirmation follows best practices.This email function properly handles sending an email notification when additional information about a location is submitted. It correctly uses templates for the subject and body, and populates the necessary data from the moderation event.
518-561: Email function for contribution approval is well-implemented.This function correctly handles checking for existing claims before sending the approval email. It properly constructs URLs for the facility and claim actions, providing a good user experience.
563-599: Email function for rejected contributions is well-structured.The function properly collects information about the rejected contribution and provides appropriate context for the email templates, including the reason for rejection and search URL.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/django/api/mail.py (3)
48-53:⚠️ Potential issueFix the undefined function reference:
make_oar_urlThe function
make_oar_urlis called here but not defined anywhere in the file. This will cause a runtime error when the code is executed.Apply this fix to use the existing URL helper function:
def make_pl_search_url(request): return ( "{}/contribute/single-location".format( - make_oar_url(request) + make_oshub_url(request) ) )🧰 Tools
🪛 Ruff (0.8.2)
51-51: Undefined name
make_oar_url(F821)
56-62:⚠️ Potential issueFix the undefined function reference:
make_oar_urlThe function
make_oar_urlis called here but not defined anywhere in the file. This will cause a runtime error when the code is executed.Apply this fix to use the existing URL helper function:
def make_pl_claim_url(request, facility): return ( "{}/facilities/{}/claim".format( - make_oar_url(request), + make_oshub_url(request), facility.id, ) )🧰 Tools
🪛 Ruff (0.8.2)
59-59: Undefined name
make_oar_url(F821)
498-516: 🛠️ Refactor suggestionEmail function for new location confirmation is missing context data.
The
send_slc_new_location_confirmation_emailfunction passes empty context to the template rendering, which means the email won't have any specific information about the new location.Consider adding relevant information about the location from the moderation event to the template context, similar to the
additional_info_dictionaryin the previous function:def send_slc_new_location_confirmation_email(moderation_event): subj_template = get_template( 'mail/slc_new_location_confirmation_subject.txt' ) text_template = get_template( 'mail/slc_new_location_confirmation_body.txt' ) html_template = get_template( 'mail/slc_new_location_confirmation_body.html' ) + new_location_dictionary = { + 'pl_name': moderation_event.cleaned_data.get("name", ''), + 'pl_address': moderation_event.cleaned_data.get("address", ''), + 'pl_country': ( + moderation_event.cleaned_data.get("country_code", '') + ), + } + send_mail( subj_template.render().rstrip(), - text_template.render(), + text_template.render(new_location_dictionary), settings.DATA_FROM_EMAIL, [moderation_event.contributor.admin.email], - html_message=html_template.render() + html_message=html_template.render(new_location_dictionary) )
🧹 Nitpick comments (1)
src/django/api/mail.py (1)
563-599: Consider adding docstrings to the new email functions.While the function names are descriptive, adding docstrings (similar to the one in
send_production_location_creation_email) would improve code documentation and help future developers understand when and why these emails are sent.Example implementation:
def send_slc_additional_info_confirmation_email(moderation_event): + ''' + This function sends an email confirming receipt of additional information + for a production location contributed through the SLC workflow. + ''' subj_template = get_template( 'mail/slc_additional_info_confirmation_subject.txt' ) # ...Also applies to: 470-495, 498-516, 518-560
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/django/api/mail.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/django/api/mail.py
51-51: Undefined name make_oar_url
(F821)
59-59: Undefined name make_oar_url
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-flake8-linter
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: run-dd-code-quality
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: run-fe-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-django-code-quality
🔇 Additional comments (5)
src/django/api/mail.py (5)
470-495: Good implementation of SLC additional info confirmation email.The email template context properly includes necessary details about the location (name, address, country) from the moderation event's cleaned data.
518-560: Robust implementation for SLC contribution approval.The code properly:
- Checks if the facility is already claimed
- Includes both claim and location URLs in the email context
- Uses the appropriate email templates
This will provide users with clear next steps after their contribution is approved.
563-599: Good implementation with robust error handling for SLC rejection emails.The code effectively:
- Uses
getattrwith default values (None) to safely handle cases where reason texts might be missing- Includes facility details from the cleaned data
- Provides a search URL for the user to try again
This approach ensures the email will be sent even if some data fields are missing.
5-10: LGTM! Updated imports reflect the new functionality.The imports have been appropriately updated to include the models needed for the new SLC email functionality.
12-12: LGTM! FacilityClaimStatuses constant import.Adding the
FacilityClaimStatusesimport is appropriate for the new claim status checks in the approval email function.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/django/api/mail.py (6)
470-495: Add a docstring to explain the function's purpose.Consider adding a docstring similar to
send_production_location_creation_emailto explain when this function is called and what it's used for.def send_slc_additional_info_confirmation_email(moderation_event): + ''' + This function is used to send a confirmation email to the contributor + when they've provided additional information about a location in the SLC workflow. + ''' subj_template = get_template( 'mail/slc_additional_info_confirmation_subject.txt' )
498-515: Add a docstring and consider adding error handling.Add a docstring to explain the function's purpose and consider adding defensive checks for the moderation_event structure to prevent potential errors.
def send_slc_new_location_confirmation_email(moderation_event): + ''' + This function is used to send a confirmation email to the contributor + when they've submitted a new location in the SLC workflow. + The email template contains static content, so no context is needed. + ''' subj_template = get_template( 'mail/slc_new_location_confirmation_subject.txt' ) text_template = get_template( 'mail/slc_new_location_confirmation_body.txt' ) html_template = get_template( 'mail/slc_new_location_confirmation_body.html' ) + # Verify that we have a valid contributor with an admin email + if not moderation_event.contributor or not moderation_event.contributor.admin: + # Log error or handle the case where email can't be sent + return + send_mail( subj_template.render().rstrip(), text_template.render(), settings.DATA_FROM_EMAIL, [moderation_event.contributor.admin.email], html_message=html_template.render() )
518-532: Add a docstring explaining the function's purpose.This function should have a docstring explaining when it's called and what it does in the SLC workflow.
def send_slc_contribution_approval_email( request, moderation_event, facility_list_item ): + ''' + This function is used to send an approval email to the contributor + when their facility contribution has been approved in the SLC workflow. + It provides links to claim the facility if it's not already claimed. + ''' subj_template = get_template( 'mail/slc_contribution_approval_subject.txt' )
563-579: Add a docstring and use consistent variable naming.Add a docstring and ensure consistent variable naming throughout the function.
def send_slc_contribution_rejected_email(request, moderation_event): + ''' + This function is used to send a rejection email to the contributor + when their facility contribution has been rejected in the SLC workflow. + It includes the reason for rejection and details about the location. + ''' subj_template = get_template( 'mail/slc_contribution_rejected_subject.txt' ) text_template = get_template( 'mail/slc_contribution_rejected_body.txt' ) html_template = get_template( 'mail/slc_contribution_rejected_body.html' ) rejected_dictionary = { - 'action_reason_text_cleaned': getattr( + 'rejection_reason_cleaned': getattr( moderation_event, "action_reason_text_cleaned", None ),
470-599: Consider refactoring common email sending patterns.I notice there's significant duplication across all the email sending functions. Consider creating a helper function to reduce this duplication.
+def _send_templated_email(subject_template_name, text_template_name, html_template_name, + context, from_email, to_emails): + ''' + Helper function to send emails with templates. Reduces duplication in email sending functions. + + Args: + subject_template_name: Path to the subject template + text_template_name: Path to the plaintext body template + html_template_name: Path to the HTML body template + context: Dictionary with template context variables + from_email: Sender email address + to_emails: List of recipient email addresses + ''' + subj_template = get_template(subject_template_name) + text_template = get_template(text_template_name) + html_template = get_template(html_template_name) + + send_mail( + subj_template.render().rstrip(), + text_template.render(context), + from_email, + to_emails, + html_message=html_template.render(context) + ) + def send_slc_additional_info_confirmation_email(moderation_event): ''' This function is used to send a confirmation email to the contributor when they've provided additional information about a location in the SLC workflow. ''' - subj_template = get_template( - 'mail/slc_additional_info_confirmation_subject.txt' - ) - text_template = get_template( - 'mail/slc_additional_info_confirmation_body.txt' - ) - html_template = get_template( - 'mail/slc_additional_info_confirmation_body.html' - ) - additional_info_dictionary = { 'pl_name': moderation_event.cleaned_data.get("name", ''), 'pl_address': moderation_event.cleaned_data.get("address", ''), 'pl_country': ( moderation_event.cleaned_data.get("country_code", '') ), } - - send_mail( - subj_template.render().rstrip(), - text_template.render(additional_info_dictionary), - settings.DATA_FROM_EMAIL, - [moderation_event.contributor.admin.email], - html_message=html_template.render(additional_info_dictionary) - ) + + _send_templated_email( + 'mail/slc_additional_info_confirmation_subject.txt', + 'mail/slc_additional_info_confirmation_body.txt', + 'mail/slc_additional_info_confirmation_body.html', + additional_info_dictionary, + settings.DATA_FROM_EMAIL, + [moderation_event.contributor.admin.email] + )This refactoring could be applied to all email sending functions to make the code more maintainable.
470-599: Add type annotations to new functions.Consider adding type annotations to the new functions for consistency with other functions in the file (like
send_production_location_creation_email).-def send_slc_additional_info_confirmation_email(moderation_event): +def send_slc_additional_info_confirmation_email(moderation_event: ModerationEvent): -def send_slc_new_location_confirmation_email(moderation_event): +def send_slc_new_location_confirmation_email(moderation_event: ModerationEvent): -def send_slc_contribution_approval_email( - request, - moderation_event, - facility_list_item -): +def send_slc_contribution_approval_email( + request: Request, + moderation_event: ModerationEvent, + facility_list_item +): -def send_slc_contribution_rejected_email(request, moderation_event): +def send_slc_contribution_rejected_email(request: Request, moderation_event: ModerationEvent):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/django/api/mail.py(3 hunks)src/django/api/tests/test_moderation_events_add_production_location.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/django/api/tests/test_moderation_events_add_production_location.py (1)
Learnt from: mazursasha1990
PR: opensupplyhub/open-supply-hub#438
File: src/django/api/tests/test_moderation_events_add_production_location.py:21-65
Timestamp: 2024-12-12T14:59:19.694Z
Learning: In `src/django/api/tests/test_moderation_events_add_production_location.py`, the tests have been refactored, and the use of `POST` methods is intentional. Future suggestions to change HTTP methods in these tests may not be necessary.
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: run-integration-test-code-quality
- GitHub Check: run-flake8-linter
- GitHub Check: run-eslint-linter-and-prettier-formatter
- GitHub Check: run-countries-code-quality
- GitHub Check: run-contricleaner-code-quality
- GitHub Check: run-fe-code-quality
- GitHub Check: run-dd-code-quality
- GitHub Check: get-base-branch-django-cov
- GitHub Check: get-base-branch-dd-cov
- GitHub Check: get-base-branch-contricleaner-cov
- GitHub Check: get-base-branch-countries-cov
- GitHub Check: get-base-branch-fe-cov
- GitHub Check: run-django-code-quality
🔇 Additional comments (9)
src/django/api/tests/test_moderation_events_add_production_location.py (2)
103-104: Good addition of clarifying comments!These comments effectively explain the test setup and improve code understanding by making it explicit that the SLC source is set by default in the base class's setUp method.
114-116: Good explicit test setup!This change correctly sets up the test scenario where the source is 'API', ensuring that we properly test the condition where no email should be sent for API contributions. This is important for verifying the selective email notification behavior that's central to this PR.
src/django/api/mail.py (7)
5-12: Import organization looks good.The imports are properly organized and include all necessary modules for the new email functionality.
48-62: LGTM! URL helper functions follow established patterns.These URL helper functions follow the established patterns in the codebase, which is good for maintainability.
481-486: LGTM! Context dictionary is properly constructed.The function properly extracts data from the moderation event and handles missing fields with default values.
533-539: LGTM! Good practice checking if facility is already claimed.The query correctly checks for both APPROVED and PENDING claim statuses using the constants from FacilityClaimStatuses.
541-552: Well-structured context dictionary with clear variable names.The context dictionary is well-structured and includes all necessary information for the email template.
574-591: Good use of defensive programming with getattrUsing getattr with a default value is a good defensive programming practice to handle potential missing attributes.
5-600:Details
✅ Verification successful
Verify integration with the SLC workflow.
The new email functions need to be called from the appropriate points in the moderation workflow code.
🏁 Script executed:
#!/bin/bash # Search for where these functions are being called echo "Searching for calls to new email functions..." echo "1. send_slc_additional_info_confirmation_email:" rg "send_slc_additional_info_confirmation_email" --type py echo "2. send_slc_new_location_confirmation_email:" rg "send_slc_new_location_confirmation_email" --type py echo "3. send_slc_contribution_approval_email:" rg "send_slc_contribution_approval_email" --type py echo "4. send_slc_contribution_rejected_email:" rg "send_slc_contribution_rejected_email" --type py # Look at the moderation events view to see the integration echo "\nChecking moderation_events.py for integration:" fd "moderation_events.py" --type f | xargs catLength of output: 19093
SLC Workflow Integration Verified
The new email functions are appropriately integrated within the moderation workflow. Specifically:
Production Locations Flow:
send_slc_additional_info_confirmation_emailandsend_slc_new_location_confirmation_emailare called from the production locations views (in src/django/api/views/v1/production_locations.py).Moderation Events Flow:
send_slc_contribution_approval_emailis invoked during the update of production locations in src/django/api/views/v1/moderation_events.py.send_slc_contribution_rejected_emailis called in the partial update endpoint when the event source is SLC.No further integration gaps were found.
🧰 Tools
🪛 Ruff (0.8.2)
21-24: Use ternary operator
host = request.get_host() if request else settings.EXTERNAL_DOMAINinstead ofif-else-blockReplace
if-else-block withhost = request.get_host() if request else settings.EXTERNAL_DOMAIN(SIM108)
369-372: Use ternary operator
closure_state = 're-opened' if report.closure_state == 'OPEN' else 'closed'instead ofif-else-blockReplace
if-else-block withclosure_state = 're-opened' if report.closure_state == 'OPEN' else 'closed'(SIM108)
5 participants
OSDEV-1721 SLC. Customer Comms for SLC workflow