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

ICMSLST-2135 - Gov Notify #1149

Merged
merged 1 commit into from
Aug 8, 2023
Merged

ICMSLST-2135 - Gov Notify #1149

merged 1 commit into from
Aug 8, 2023

Conversation

marcuspp
Copy link
Contributor

@marcuspp marcuspp commented Aug 2, 2023

Initial implementation of Gov Notify

  • Gov notify backend added
  • Access requested email updated to use gov notify
  • Backend not set by default to GOV Notify this will be done once Gov Notify is 'Live'
  • Support added to override recipients of emails on dev/local to aid with manual testing
  • Logging handled by django-celery-results
  • Removed django-ses

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.09% 🎉

Comparison is base (144cd1c) 75.43% compared to head (fd4e575) 75.53%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1149      +/-   ##
==========================================
+ Coverage   75.43%   75.53%   +0.09%     
==========================================
  Files         256      261       +5     
  Lines       18219    18289      +70     
  Branches     2928     2936       +8     
==========================================
+ Hits        13744    13814      +70     
  Misses       4021     4021              
  Partials      454      454              
Files Changed Coverage Δ
config/settings/base.py 95.53% <100.00%> (-0.12%) ⬇️
config/settings/non_prod_base.py 100.00% <100.00%> (ø)
web/domains/case/access/views.py 94.63% <100.00%> (+0.03%) ⬆️
web/mail/api.py 100.00% <100.00%> (ø)
web/mail/backends.py 100.00% <100.00%> (ø)
web/mail/decorators.py 100.00% <100.00%> (ø)
web/mail/emails.py 100.00% <100.00%> (ø)
web/mail/messages.py 100.00% <100.00%> (ø)
web/mail/recipients.py 100.00% <100.00%> (ø)
web/notify/notify.py 93.54% <100.00%> (-0.16%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1,5 +1,5 @@
boto3==1.21.29
celery[redis]==5.2.7
celery[redis]==5.3.1
Copy link
Contributor Author

@marcuspp marcuspp Aug 8, 2023

Choose a reason for hiding this comment

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

Came across this issue during testing celery/billiard#377 Upgrading celery fixed the problem

@@ -26,6 +25,7 @@ notifications-python-client==8.0.1
openpyxl==3.0.7
oracledb==1.2.0
phonenumbers==8.12.12
pytz==2023.3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was a dependancy of celery 5.2.7 and not 5.3.1. Is used by add_commodity_data script

@@ -228,6 +223,7 @@
CELERY_ACCEPT_CONTENT = ["application/json"]
CELERY_RESULT_SERIALIZER = "json"
CELERY_TASK_SERIALIZER = "json"
CELERY_RESULT_EXTENDED = True
Copy link
Contributor Author

@marcuspp marcuspp Aug 8, 2023

Choose a reason for hiding this comment

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

Adding this takes care of logging (Ticket ICMSLST-2031). All tasks are logged in TaskResult object and this setting adds the task args and traceback information on failure. Is all viewable in django admin

Makefile Outdated Show resolved Hide resolved
web/mail/backends.py Outdated Show resolved Hide resolved
web/mail/backends.py Outdated Show resolved Hide resolved
web/mail/backends.py Outdated Show resolved Hide resolved
web/mail/messages.py Outdated Show resolved Hide resolved
web/mail/messages.py Outdated Show resolved Hide resolved
} | self.get_context()

def get_template_id(self) -> UUID:
return EmailTemplate.objects.get(name=self.name).gov_notify_template_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think it will be a problem but this will hit the db everytime any derived class is initialised.

@marcuspp marcuspp merged commit a1b2714 into main Aug 8, 2023
@marcuspp marcuspp deleted the ICMSLST-2135 branch August 8, 2023 15:33
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.

4 participants