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.

Sign up for GitHub

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

Jump to bottom

Newsletter subscription emails should also use paragraph tags #27357

Merged
merged 6 commits into from
Jun 5, 2020
Merged

Newsletter subscription emails should also use paragraph tags #27357

merged 6 commits into from
Jun 5, 2020

Conversation

ptylek
Copy link
Contributor

@ptylek ptylek commented Mar 19, 2020 •
edited by ghost
Loading

Description (*)

Other emails (also subscription confirmation email in the same module) are using proper html tags for their content.

<p>{{trans "To begin receiving the newsletter, you must first confirm your subscription by clicking on the following link:"}}</p>

The same should apply for newsletter subscription and unsubscription emails.

This will help in keeping same email styling.

Related Pull Requests

Fixed Issues (if relevant)

Couldn't find issue.

Manual testing scenarios (*)

  1. As a logged in user go to User Area - Newsletter section and subscribe / unsubscribe to the newsletter.
  2. Go to your email box and check those emails

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Resolved issues:

  1. resolves [Issue] Newsletter subscription emails should also use paragraph tags #28166: Newsletter subscription emails should also use paragraph tags

@m2-assistant
Copy link

m2-assistant bot commented Mar 19, 2020

Hi @ptylek. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@rogyar rogyar self-assigned this Mar 22, 2020
rogyar
rogyar requested changes Mar 22, 2020
View reviewed changes
Copy link
Contributor

@rogyar rogyar left a comment

Choose a reason for hiding this comment

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

Hi @ptylek. Thank you for your collaboration.

Please, take a look at the failing functional tests

@magento-engcom-team magento-engcom-team added this to Changes Requested in Pull Requests Dashboard Mar 24, 2020
@ptylek
Copy link
Contributor Author

ptylek commented Mar 25, 2020

@rogyar , I don't think these failing tests are related. I will run them again.

@rogyar
Copy link
Contributor

rogyar commented Mar 27, 2020 •
edited
Loading

Hi @ptylek. Sorry, my bad.

I meant the integration tests. It does look like the integration tests use the old email body in the assertion. And the subscribe/unsubscribe tests are failing because of that.

So we need to modify the assertion expectation to match our change.

Thank you.

@rogyar rogyar mentioned this pull request Mar 28, 2020
Merged
5 tasks
lbajsarowicz added a commit to lbajsarowicz/magento2 that referenced this pull request Apr 5, 2020
@lbajsarowicz
magento#27357 Fix the test coverage for e-mail contents
766a6ab
@lbajsarowicz lbajsarowicz mentioned this pull request Apr 5, 2020
Merged
4 tasks
lbajsarowicz added a commit to lbajsarowicz/magento2 that referenced this pull request Apr 5, 2020
@lbajsarowicz
magento#27357 Add missing visibility
dcd807b
@ptylek
Copy link
Contributor Author

ptylek commented Apr 18, 2020

@magento run Database Compare, Integration Tests

@ptylek
Copy link
Contributor Author

ptylek commented Apr 21, 2020

@rogyar just wanted to confirm if this PR is also going to be merged? As tests for this PR have received Progress: accept label

@rogyar
Copy link
Contributor

rogyar commented Apr 26, 2020

Hi @ptylek. We still have the integration tests failing. So the integration tests need to be fixed before the PR merge.

Thank you

@ptylek
Copy link
Contributor Author

ptylek commented May 3, 2020

@rogyar Only database test is failing, however I think this is not related to this PR.

@rogyar rogyar added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label May 6, 2020
@ghost ghost moved this from Changes Requested to Ready for Testing in Pull Requests Dashboard May 6, 2020
@ghost ghost removed the Progress: needs update label May 6, 2020
@VladimirZaets VladimirZaets added QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) and removed Progress: accept labels May 19, 2020
VladimirZaets
VladimirZaets requested changes May 21, 2020
View reviewed changes
Copy link
Contributor

@VladimirZaets VladimirZaets left a comment

Choose a reason for hiding this comment

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

Hi @ptylek. Thanks for collaboration. I think this case can be covered by MFTF tests, can you please do it?

@ghost ghost moved this from Extended Testing (optional) to Changes Requested in Pull Requests Dashboard May 21, 2020
@ghost ghost assigned VladimirZaets May 21, 2020
@ptylek
Copy link
Contributor Author

ptylek commented May 21, 2020 •
edited
Loading

Hi @ptylek. Thanks for collaboration. I think this case can be covered by MFTF tests, can you please do it?

Hi @VladimirZaets ,
@lbajsarowicz provided integration tests for this PR -> #27609. Aren't these sufficient?

All other e-mails use proper html tags and there aren't any MFTF tests for that. This is a small fix to add html tag in e-mail template that misses them.

@VladimirZaets
Copy link
Contributor

Hi @ptylek. Integration test is ok for this case, thank you, I missed PR with coverage

@engcom-Echo
Copy link
Contributor

@magento run all tests

@ghost ghost moved this from Changes Requested to Ready for Testing in Pull Requests Dashboard May 27, 2020
@VladimirZaets VladimirZaets moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard May 27, 2020
@VladimirZaets VladimirZaets moved this from Testing in Progress to Merge in Progress in Pull Requests Dashboard May 27, 2020
@magento-engcom-team
Copy link
Contributor

Hi @VladimirZaets, thank you for the review.
ENGCOM-7522 has been created to process this Pull Request

@VladimirZaets VladimirZaets added QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope and removed QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels May 27, 2020
magento-engcom-team pushed a commit that referenced this pull request Jun 5, 2020
@slavvka
ENGCOM-7522: Newsletter subscription emails should also use paragraph…
2eb9082 
… tags #27357
@magento-engcom-team magento-engcom-team merged commit 5a9e5e5 into magento:2.4-develop Jun 5, 2020
@m2-assistant
Copy link

m2-assistant bot commented Jun 5, 2020

Hi @ptylek, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@ghost ghost moved this from Merge in Progress to Recently Merged in Pull Requests Dashboard Jun 5, 2020
magento-engcom-team added a commit that referenced this pull request Sep 2, 2020
@magento-engcom-team
ENGCOM-7461: Test coverage for PR #27357 (E-mail templates) #27609
02d6f96 
 - Merge Pull Request #27609 from lbajsarowicz/magento2:test-coverage/27357-integration-tests
 - Merged commits:
   1. a46c024
   2. 766a6ab
   3. dcd807b
   4. 5c489c7
   5. 1a6b60f
   6. ac4ba09
   7. 65940b8
   8. 8ad8df2
magento-engcom-team pushed a commit that referenced this pull request Sep 2, 2020
@sidolov
ENGCOM-7461: Test coverage for PR #27357 (E-mail templates) #27609
ced8102
magento-engcom-team added a commit that referenced this pull request Sep 2, 2020
@magento-engcom-team
🔃 [Magento Community Engineering] Community Contributions - 2.4-develop
b87f1df 
Accepted Community Pull Requests:
 - #29799: Add elasticsearch params to integration tests config (by @ihor-sviziev)
 - #29634: [MFTF] Added some ActionGroups to Analytics module (by @Usik2203)
 - #29348: [MFTF] Test scenario for "Apply shopping cart rule to a single bundle item" (#28921) (by @zhartaunik)
 - #28413: Fix Downloadable product after refund (by @ProkopovVitaliy)
 - #27609: Test coverage for PR #27357 (E-mail templates) (by @lbajsarowicz)
 - #27579: Fix #27523: throw informative errors in setup:db:generate-patch (by @korostii)


Fixed GitHub Issues:
 - #29648: [Issue] [MFTF] Added some ActionGroups to Analytics module (reported by @m2-assistant[bot]) has been fixed in #29634 by @Usik2203 in 2.4-develop branch
   Related commits:
     1. f54fd86
     2. 1492668
     3. 59d5d32
     4. 8c2291f
     5. e25fc55

 - #28921: Apply shopping cart rule to a single bundle item (reported by @MilanFrajt) has been fixed in #29348 by @zhartaunik in 2.4-develop branch
   Related commits:
     1. e6aca3b
     2. 7abc82f

 - #28388: Downloadable product is available in My Download Products tab after it has been partially refunded  (reported by @YaninaPrudnikova) has been fixed in #28413 by @ProkopovVitaliy in 2.4-develop branch
   Related commits:
     1. a6f460d
     2. 9a83830
     3. 96c9d54
     4. 59cf9f7
     5. c6c3231
     6. eafdb17
     7. 92fd06b
     8. 1f7eace
     9. db84d3a
     10. d0a0962
     11. aea9276
     12. 1be2cd7
     13. dee9b16
     14. a49709f
     15. 8023f42
     16. 9e31246
     17. d91b305
     18. 62e4866
     19. 97df424
     20. 2d58988
     21. 87d8984

 - #27523: CLI Patch Generator: Lack of information what path cannot be created (reported by @lbajsarowicz) has been fixed in #27579 by @korostii in 2.4-develop branch
   Related commits:
     1. 1e8e825
     2. 1dd5290
     3. c1d8fef
     4. a74f8fe
     5. f48cd50
     6. 2bc691a
     7. 8991733
     8. 347602f
     9. 75bb2c3
     10. 7d316d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rogyar rogyar rogyar approved these changes

@VladimirZaets VladimirZaets VladimirZaets approved these changes

Assignees

@rogyar rogyar

@VladimirZaets VladimirZaets

@engcom-Delta engcom-Delta

@engcom-Echo engcom-Echo

Labels
Area: Frontend Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Component: Newsletter Event: Global-Contribution-Day Partner: creativestyle partners-contribution Pull Request is created by Magento Partner Priority: P3 May be fixed according to the position in the backlog. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround.
Projects
Pull Requests Dashboard
  
Recently Merged
Milestone
2.4.1
Development

Successfully merging this pull request may close these issues.

[Issue] Newsletter subscription emails should also use paragraph tags
7 participants
@ptylek @rogyar @magento-engcom-team @engcom-Delta @engcom-Echo @slavvka @VladimirZaets