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

refactoring & Test case for send email to dnd contact #12654

Merged
merged 10 commits into from Aug 28, 2023

Conversation

dadarya0
Copy link
Contributor

Q A
Bug fix? (use the a.b branch) No
New feature/enhancement? (use the a.x branch) Yes
Deprecations? [ ]
BC breaks? (use the c.x branch) [ ]
Automated tests included? Yes
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #...

Description:

Refactoring & Test case for send email to dnd contact which was implemented in https://github.com/mautic/mautic/pull/11787/files

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Send an individual email, an email via API /emails/ID/contact/CONTACT_ID/send, transactional email by form and by campaign.
  3. Check the headers of the emails. There is no 'List-Unsubscribe'.
  4. Send a marketing email by form and by campaign.
  5. Check the headers of the emails. There is 'ListUnsubscribe'.

@dadarya0 dadarya0 added the unforking Used for PRs in the Acquia's unforking initiative label Aug 16, 2023
@dadarya0 dadarya0 requested review from a team, roboparker and rahuld-dev and removed request for a team August 16, 2023 07:18
@codecov
Copy link

codecov bot commented Aug 16, 2023

Codecov Report

Merging #12654 (39ef1f4) into 5.x (c86f0ee) will increase coverage by 0.00%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##                5.x   #12654   +/-   ##
=========================================
  Coverage     58.45%   58.45%           
  Complexity    33597    33597           
=========================================
  Files          2183     2183           
  Lines        101716   101716           
=========================================
+ Hits          59454    59455    +1     
+ Misses        42262    42261    -1     
Files Changed Coverage Δ
.../EmailBundle/Controller/Api/EmailApiController.php 83.33% <ø> (ø)
...ailBundle/EventListener/MessageQueueSubscriber.php 11.42% <ø> (ø)
...pp/bundles/EmailBundle/Form/Type/EmailSendType.php 80.55% <0.00%> (ø)
...s/EmailBundle/EventListener/CampaignSubscriber.php 66.36% <100.00%> (ø)
app/bundles/EmailBundle/Helper/MailHelper.php 64.25% <100.00%> (ø)
app/bundles/EmailBundle/Model/EmailModel.php 49.59% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Contributor

@roboparker roboparker left a comment

Choose a reason for hiding this comment

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

Thank you @dadarya0

A few nitpicks with the tests.

  1. Instead of having a test with a foreach to test all the emails, we should use a data provider.
  2. Please don't add phpstan ignore lines unless its the only reasonable solution. I am guessing its to hide a type error? If so use assert

app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php Outdated Show resolved Hide resolved
app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php Outdated Show resolved Hide resolved
app/bundles/EmailBundle/Tests/Helper/MailHelperTest.php Outdated Show resolved Hide resolved
Copy link
Contributor

@roboparker roboparker left a comment

Choose a reason for hiding this comment

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

Thank you @dadarya0

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged enhancement Any improvement to an existing feature or functionality email Anything related to email labels Aug 18, 2023
Copy link
Sponsor Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

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

Sending an email to the contact is blocked by #12495 and sending via form actions and API is blocked by #12663 in case others are testing this.

Copy link
Contributor

@volha-pivavarchyk volha-pivavarchyk left a comment

Choose a reason for hiding this comment

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

Works well as before

  • an individual email - no 'List-Unsubscribe',
  • an email via API - no 'List-Unsubscribe',
  • a transactional email by campaign - no 'List-Unsubscribe',
  • a marketing email by campaign - 'List-Unsubscribe'.

Code looks good 👍, thanks.

Maybe a couple of nuances.
If you agree, all hard-coded "transactional" types can be replaced with a constant, e.g. $mailer->setEmailType('transactional') > $mailer->setEmailType(MailHelper::EMAIL_TYPE_TRANSACTIONAL).
If you agree, you can add the MailHelper::EMAIL_TYPE_MARKETING constant to make the way similar.

@rahuld-dev rahuld-dev added the ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged label Aug 22, 2023
@RCheesley RCheesley added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged labels Aug 23, 2023
@RCheesley
Copy link
Sponsor Member

@dadarya0 please take a look at the suggestions from @volha-pivavarchyk in the code review above?

@dadarya0
Copy link
Contributor Author

@dadarya0 please take a look at the suggestions from @volha-pivavarchyk in the code review above?

Suggestions Implemented in 39ef1f4

@dadarya0
Copy link
Contributor Author

Works well as before

  • an individual email - no 'List-Unsubscribe',
  • an email via API - no 'List-Unsubscribe',
  • a transactional email by campaign - no 'List-Unsubscribe',
  • a marketing email by campaign - 'List-Unsubscribe'.

Code looks good 👍, thanks.

Maybe a couple of nuances. If you agree, all hard-coded "transactional" types can be replaced with a constant, e.g. $mailer->setEmailType('transactional') > $mailer->setEmailType(MailHelper::EMAIL_TYPE_TRANSACTIONAL). If you agree, you can add the MailHelper::EMAIL_TYPE_MARKETING constant to make the way similar.

Suggestions Implemented in 39ef1f4

@RCheesley RCheesley added the code-review-passed PRs which have passed code review label Aug 24, 2023
@escopecz escopecz added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-feedback PR's and issues that are awaiting feedback from the author pending-test-confirmation PR's that require one test before they can be merged labels Aug 28, 2023
@escopecz escopecz added this to the 5.0-beta milestone Aug 28, 2023
@escopecz escopecz merged commit 71e9908 into mautic:5.x Aug 28, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-passed PRs which have passed code review email Anything related to email enhancement Any improvement to an existing feature or functionality ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged unforking Used for PRs in the Acquia's unforking initiative
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants