Fix "Confirmation request" email is sent on customer's newsletter unsubscribe action #15464

nuzil · 2018-05-23T15:51:28Z

Description

This fix related to #15218
Skip update customer subscribe status from save subscribe action from my account when nothing is changed.

Fixed Issues (if relevant)

Skip update customer subscribe status from save subscribe action from my account when nothing is changed.

Manual testing scenarios

Activate "Need to Confirm" for the newsletter subscription
Register a new customer
Subscribe to the newsletter
Confirm the newsletter subscription.
Login as created customer
Unsubscribe from the newsletter from the customer area

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 on Travis CI are green)

…ubscription (issues/15218). Skip update customer subscribe status from save subscribe action from my account when nothing is changed.

…stomer account page

dmanners

Thanks for this PR. I have added some comments with regards to language in the messages and also ease of understanding of the code.

dmanners · 2018-06-07T10:11:49Z

app/code/Magento/Newsletter/Controller/Manage/Save.php

+                            ->subscribeCustomerById($customerId);
+                        $subscribeStatus = $subscribeModel->getStatus();
+                        if ($subscribeStatus == Subscriber::STATUS_SUBSCRIBED) {
+                            $this->messageManager->addSuccess(__('We saved the subscription.'));


I would be tempted to update these messaged if they are to be shown to a user. Something like "We have saved your subscription." to make it a bit more personal to the user.

dmanners · 2018-06-07T10:12:27Z

app/code/Magento/Newsletter/Controller/Manage/Save.php

+                        if ($subscribeStatus == Subscriber::STATUS_SUBSCRIBED) {
+                            $this->messageManager->addSuccess(__('We saved the subscription.'));
+                        } else {
+                            $this->messageManager->addSuccess(__('The confirmation request has been sent.'));


I would suggest that this should be "A confirmation request has been sent." here.

dmanners · 2018-06-07T10:13:33Z

app/code/Magento/Newsletter/Controller/Manage/Save.php

+                    } else {
+                        $this->subscriberFactory->create()
+                            ->unsubscribeCustomerById($customerId);
+                        $this->messageManager->addSuccess(__('We removed the subscription.'));


Again to make this a bit more personal I would suggest "We have removed your newsletter subscription". You could maybe drop the newsletter part from the text and in which case. "We have removed your subscription" would be fine.

dmanners · 2018-06-07T10:13:59Z

app/code/Magento/Newsletter/Controller/Manage/Save.php

                } else {
-                    $this->subscriberFactory->create()->unsubscribeCustomerById($customerId);
-                    $this->messageManager->addSuccess(__('We removed the subscription.'));
+                    $this->messageManager->addSuccess(__('We updated the subscription.'));


Again to make it more personal to the user "We have updated your subscription" would be good here.

dmanners · 2018-06-07T10:14:34Z

dev/tests/integration/testsuite/Magento/Newsletter/Controller/ManageTest.php

@@ -84,7 +85,7 @@ public function testSaveActionRemoveSubscription()
         * Check that success message
         */
        $this->assertSessionMessages(
-            $this->equalTo(['We removed the subscription.']),
+            $this->equalTo(['We updated the subscription.']),


We would need to update this message to related to the changes to the messages I have suggested.

dmanners · 2018-06-07T10:15:34Z

app/code/Magento/Newsletter/Controller/Manage/Save.php

+                    ->getIsSubscribed();
+                $isSubscribedParam = (boolean)$this->getRequest()
+                    ->getParam('is_subscribed', false);
+                if ($isSubscribedParam != $isSubscribedState) {


Would it not work to do a !== comparison here?

dmanners · 2018-06-07T10:16:57Z

app/code/Magento/Newsletter/Controller/Manage/Save.php

-                if ((boolean)$this->getRequest()->getParam('is_subscribed', false)) {
-                    $this->subscriberFactory->create()->subscribeCustomerById($customerId);
-                    $this->messageManager->addSuccess(__('We saved the subscription.'));
+                $isSubscribedState = $customer->getExtensionAttributes()


This whole section is still a bit hard to read/understand do you think that there would be a way to make it a bit easier to read? Maybe moving some of the logic to new private methods and splitting out some of the if/else so it is not so nested. What do you think?

Hi @dmanners
Thats a reasonable proposition, cause I agree that this part of newsletter module is not the best, but for now I will leave it as it is. If I will find enough time I will come back to it later.

…ewsletter unsubscribe action #15464

magento-engcom-team · 2018-07-03T21:46:09Z

Hi @nuzil. Thank you for your contribution.
We will aim to release these changes as part of 2.2.6.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

Fix "Confirmation request" email is sent on customer's newsletter uns…

d790198

…ubscription (issues/15218). Skip update customer subscribe status from save subscribe action from my account when nothing is changed.

vkublytskyi mentioned this pull request May 26, 2018

"Confirmation request" email is sent on customer's newsletter unsubscription #15218

Closed

nuzil added 3 commits May 26, 2018 16:01

Remove redirect to the same page. Left default Magento redirect to Cu…

1f66421

…stomer account page

Updat messages related to a new logic

f80eb03

Fix static tests and remove return before redirect

3227832

dmanners suggested changes Jun 7, 2018

View reviewed changes

dmanners self-assigned this Jun 7, 2018

dmanners added the Component: Newsletter label Jun 7, 2018

Change Success messages to appropriated

69eb20f

magento-engcom-team added this to the Release: 2.2.6 milestone Jun 27, 2018

magento-engcom-team added Release Line: 2.2 Partner: Comwrap partners-contribution Pull Request is created by Magento Partner Progress: accept labels Jun 27, 2018

magento-engcom-team merged commit 69eb20f into magento:2.2-develop Jul 3, 2018

magento-engcom-team pushed a commit that referenced this pull request Jul 3, 2018

ENGCOM-2144: Fix "Confirmation request" email is sent on customer's n…

ee87089

…ewsletter unsubscribe action #15464

magento-engcom-team added the Award: bug fix label Jul 3, 2018

nuzil deleted the issues/15218 branch July 5, 2018 08:53

ihor-sviziev mentioned this pull request Jul 22, 2018

[Forwardport] Fix "Confirmation request" email is sent on customer's newsletter unsubscribe action #16995

Merged

4 tasks

gelanivishal mentioned this pull request Aug 3, 2018

[Backport] Fix "Confirmation request" email is sent on customer's newsletter unsubscribe action #17347

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix "Confirmation request" email is sent on customer's newsletter unsubscribe action #15464

Fix "Confirmation request" email is sent on customer's newsletter unsubscribe action #15464

Uh oh!

nuzil commented May 23, 2018

Uh oh!

dmanners left a comment

Uh oh!

dmanners Jun 7, 2018

Uh oh!

dmanners Jun 7, 2018

Uh oh!

dmanners Jun 7, 2018

Uh oh!

dmanners Jun 7, 2018

Uh oh!

dmanners Jun 7, 2018

Uh oh!

dmanners Jun 7, 2018

Uh oh!

dmanners Jun 7, 2018

Uh oh!

nuzil Jun 26, 2018

Uh oh!

magento-engcom-team commented Jul 3, 2018

Uh oh!

Uh oh!

Fix "Confirmation request" email is sent on customer's newsletter unsubscribe action #15464

Fix "Confirmation request" email is sent on customer's newsletter unsubscribe action #15464

Uh oh!

Conversation

nuzil commented May 23, 2018

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

Uh oh!

dmanners left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magento-engcom-team commented Jul 3, 2018

Uh oh!

Uh oh!