-
Notifications
You must be signed in to change notification settings - Fork 9.4k
added config for newsletter in frontend #18407
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
added config for newsletter in frontend #18407
Conversation
Hi @torhoehn, best regards, Lars |
@larsroettig The functional test could not start correctly I think, but the rest looks fine for me now. |
@torhoehnI restarted the test if it is done I will check again |
Hi @larsroettig, thank you for the review. |
Hi @torhoehn , we found one more scenario that should be covered in scope of the provided changes: Steps:
Expected: Actual: |
@sidolov I‘ll implement this later this month. |
Hi @torhoehn , will you continue progress with PR? |
@sidolov Yes, I'm just back from vacation. |
app/code/Magento/Newsletter/Observer/PredispatchNewsletterObserver.php
Outdated
Show resolved
Hide resolved
Hi @torhoehn , are you going to continue the progress with this PR? |
@sivaschenko Yes, I'm just a little stuck with the unit test. |
@torhoehn great! Feel free to contact me in slack if you need any help with the unit tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for collaboration! Can you please fix the code styles to comply with PSRs and be consistent with the codebase
app/code/Magento/Customer/Test/Unit/Block/Form/RegisterTest.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Newsletter/Test/Unit/Observer/PredispatchNewsletterObserverTest.php
Show resolved
Hide resolved
app/code/Magento/Newsletter/Test/Unit/Observer/PredispatchNewsletterObserverTest.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Newsletter/Test/Unit/Observer/PredispatchNewsletterObserverTest.php
Outdated
Show resolved
Hide resolved
Hi @sivaschenko, thank you for the review. |
@magento-engcom-team What kind of response is needed here? |
Hi @torhoehn sorry, looks like I forgot to remove the "needs update" label. |
Hi @torhoehn, thank you for your contribution! |
Hi @torhoehn. Thank you for your contribution. |
Description
Until now there wasn't a config field to hide newsletter elements from frontend. I orientated at the wishlist module, because for wishlist it was already possible.
Manual testing scenarios
Contribution checklist