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

LTD-4652-send-email-task #230

Closed
wants to merge 11 commits into from
Closed

LTD-4652-send-email-task #230

wants to merge 11 commits into from

Conversation

seijihg
Copy link
Contributor

@seijihg seijihg commented Feb 6, 2024

https://uktrade.atlassian.net/browse/LTD-4652

I have replaced smtp_send with celery task send_smtp_task which is being globally locked. I think this can be expanded in the future quite easily in memcache_lock if we add counter into the cache and just lock True when is counter == n

@stuaxo
Copy link

stuaxo commented Feb 6, 2024

I know we called it celery_tasks, but tasks.py is the standard - probably too late to change this throughout, but there we go.

@seijihg seijihg force-pushed the LTD-4652-send-email-task branch 4 times, most recently from 3b3de86 to a60e8c7 Compare February 6, 2024 21:47
@seijihg seijihg marked this pull request as ready for review February 6, 2024 21:51
@seijihg seijihg force-pushed the LTD-4652-send-email-task branch 3 times, most recently from aab46cb to 9304133 Compare February 6, 2024 22:51
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e6a3be4) 85.76% compared to head (f2ca1f7) 85.89%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   85.76%   85.89%   +0.13%     
==========================================
  Files          46       46              
  Lines        2929     2957      +28     
  Branches      413      415       +2     
==========================================
+ Hits         2512     2540      +28     
  Misses        328      328              
  Partials       89       89              

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

@seijihg seijihg force-pushed the LTD-4652-send-email-task branch 2 times, most recently from 6c0a394 to 6d265e2 Compare February 6, 2024 23:04
@seijihg seijihg force-pushed the LTD-4652-send-email-task branch from 6d265e2 to 5315e1d Compare February 6, 2024 23:29
Copy link
Contributor

@saruniitr saruniitr left a comment

Choose a reason for hiding this comment

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

I have not looked at the tests as they will be very different once it is updated.
I can take a look again when ready.

We need to have tests that ensure child task is spawned and calls email sending methods.

@seijihg seijihg requested a review from saruniitr February 8, 2024 09:39
@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e6a3be4) 85.76% compared to head (c3b2e67) 85.82%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
+ Coverage   85.76%   85.82%   +0.06%     
==========================================
  Files          46       46              
  Lines        2929     2963      +34     
  Branches      413      417       +4     
==========================================
+ Hits         2512     2543      +31     
- Misses        328      331       +3     
  Partials       89       89              

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

@seijihg seijihg force-pushed the LTD-4652-send-email-task branch from 7194cc9 to c3b2e67 Compare February 11, 2024 23:47
@seijihg seijihg closed this Feb 13, 2024
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.

6 participants