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

Refactor communications #2591

Closed

Conversation

sasha0
Copy link
Member

@sasha0 sasha0 commented Feb 1, 2018

Refs #319 #783

Summary:

  • Created separate app - communication.
  • Dispatcher moved to communication app's utils.
  • Dispatcher improved with methods to send each separate email.
  • Added ability to send email with attachments.
  • All event codes (for each separate email) now kept in Dispatcher.
  • Sending notifications moved to communication app as well.
  • Added ability to chose if sent email should be saved to DB (through OSCAR_SAVE_SENT_EMAILS_TO_DB, True by default).
  • All relevant models moved from customer app to communication app (with data migrations) - CommunicationEventType, Email and Notification.
  • Moved classes and models added to MOVED_CLASSES and MOVED_MODELS.
  • Tests adapted to check new functionality.
  • Added tests to check the sending of each separate email.
  • Deleted TestOrderPlacementMixin - Now we use Dispatcher to send emails and code (event code) does not used when we get message context for email that informs about a placed order.
  • All emails' templates (commtype_*) moved from customer/emails to communication/emails.
  • Templates from customer/email/*.html and customer/notification/*.html moved to communication/email/*.html and communication/notification/*.html.

@sasha0 sasha0 changed the title Introduce communication app. Refactor communications Feb 1, 2018
@codecov
Copy link

codecov bot commented Feb 1, 2018

Codecov Report

Merging #2591 into master will decrease coverage by 0.08%.
The diff coverage is 86.19%.

@@            Coverage Diff             @@
##           master    #2591      +/-   ##
==========================================
- Coverage   84.62%   84.54%   -0.09%     
==========================================
  Files         279      286       +7     
  Lines       15045    15135      +90     
==========================================
+ Hits        12732    12796      +64     
- Misses       2313     2339      +26
Impacted Files Coverage Δ
src/oscar/apps/communication/managers.py 100% <ø> (ø)
src/oscar/__init__.py 33.33% <ø> (ø) ⬆️
src/oscar/apps/customer/models.py 100% <ø> (ø) ⬆️
src/oscar/apps/customer/abstract_models.py 80.5% <ø> (-6.07%) ⬇️
src/oscar/apps/order/abstract_models.py 93.2% <ø> (ø) ⬆️
...anagement/commands/oscar_generate_email_content.py 0% <0%> (ø) ⬆️
src/oscar/management/commands/oscar_send_alerts.py 0% <0%> (ø) ⬆️
src/oscar/apps/communication/config.py 0% <0%> (ø)
src/oscar/apps/communication/app.py 0% <0%> (ø)
src/oscar/apps/checkout/mixins.py 89.85% <100%> (+0.24%) ⬆️
... and 32 more

@sasha0 sasha0 force-pushed the issues/319/communications branch 3 times, most recently from 4827fb5 to 94e3385 Compare February 20, 2018 13:20
@samitnuk samitnuk force-pushed the issues/319/communications branch 2 times, most recently from 20a9b91 to ffa9bc0 Compare February 22, 2018 19:37
@craigloftus
Copy link
Contributor

@samitnuk @sasha0 To help guide the review would you be able to provide a summary of the changes in the PR, what the intention in the PR is and whether there is ambition for a follow up PR to refactor further?

@sasha0 sasha0 added this to the 2.0 milestone Aug 26, 2018
@samitnuk samitnuk force-pushed the issues/319/communications branch 15 times, most recently from 1fad3e7 to 7b00c5a Compare August 29, 2018 18:56
@samitnuk samitnuk force-pushed the issues/319/communications branch 2 times, most recently from a7d83a9 to d8097b3 Compare December 8, 2019 10:38
@samitnuk
Copy link
Contributor

samitnuk commented Dec 8, 2019

@solarissmoke Many thanks!

This PR updated based on your comments. Please review when you will have a chance.

@solarissmoke
Copy link
Member

solarissmoke commented Dec 12, 2019

Thanks @samitnuk. I think this is ready and will rebase and merge in #3260.

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.

None yet

5 participants