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

Fix newsletter subscriptions between stores #12035

Merged
merged 5 commits into from
Nov 13, 2017

Conversation

sbaixauli
Copy link

@sbaixauli sbaixauli commented Nov 5, 2017

Include store_id in query result in order to ensure that the action (subscribe/unsubscribe) it’s done in the correct store.

Description

When user have different accounts in two or more stores newsletter table save the subscription status, store_id and customer_id.
The problem is that when we try to update this information the query result don't keep in mind the store of the user.

Fixed Issues (if relevant)

  1. Newsletter subscriptions status not isolated between multi stores #10014: Newsletter subscriptions status not isolated between multi stores

Manual testing scenarios

  1. Create user: 'me@domain.com' in the first store (Website 1). Subscribe it to general subscription list.
  2. Login this user 'me@domain.com' in the second store (Website 2). Subscribe it to general subscription list.
  3. Login in the second store and uncheck subscription list.
  4. Logout and login the user in the first store. The user mantain the subscription.

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)

Include store_id in query result in order to ensure that the action (subscribe/unsubscribe) it’s done in the correct store.
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Nov 5, 2017

CLA assistant check
All committers have signed the CLA.

@dmanners dmanners added the mm17es label Nov 5, 2017
@dmanners dmanners self-assigned this Nov 5, 2017
@dmanners dmanners added this to the November 2017 milestone Nov 5, 2017
@dmanners dmanners added Release Line: 2.2 2.2.x bug report Component: Customer Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 5, 2017
@dmanners
Copy link
Contributor

dmanners commented Nov 5, 2017

The updating for the data seems to be fine but I noticed during testing with 2 store views that the frontend will not load the right information.

Using the following steps.

  1. Create system with 2 store views,
  2. Create account and register for newsletter on 1 store view,
  3. Switch store view and see You are subscribed to "General Subscription". even though admin shows you are not registered.

Admin newsletter status
Newsletter frontend status

@dmanners
Copy link
Contributor

@sbaixauli I updated the unit and integration tests so hopefully it all passes now.

  1. Unit tests failed as they could not mock the new call to the store manager. 19fe0df
  2. Integration tests failed as the customer created via dev/tests/integration/testsuite/Magento/Customer/_files/two_customers.php was attached to one store but the subscriber created via dev/tests/integration/testsuite/Magento/Newsletter/_files/subscribers.php ended up being attached to a different store. dev/tests/integration/testsuite/Magento/Newsletter/_files/subscribers.php

Hopefully this passes and we can now continue with the processing of this PR.

@dmanners
Copy link
Contributor

@sbaixauli if you are interested in knowing I used http://devdocs.magento.com/guides/v2.2/test/integration/integration_test_execution.html to run the individual tests that had been failing. Then stepped through the process using xdebug. I noticed that the stores did not match, which previously did not matter but with your change now mattered, so I updated the test fixtures to match.

@okorshenko okorshenko merged commit 194ba57 into magento:2.2-develop Nov 13, 2017
@magento-engcom-team magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Nov 13, 2017
@sbaixauli
Copy link
Author

Thanks for all @dmanners!

@Nil79
Copy link

Nil79 commented Feb 7, 2018

Hello @sbaixauli , due to the issue that I reported in my pull request #13469 , I was checking the function loadByCustomerId in Magento/Newsletter/Model/Subscriber.php.

I noticed that also removing the line that you added ($customerData->setStoreId($this->_storeManager->getStore()->getId());), the newsletter subscription is working fine on frontend in a multi site scenario.

I don't know if my checks are right, can you please give me a feedback? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report bugfix Component: Customer Fixed in 2.2.x The issue has been fixed in 2.2 release line Progress: accept Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants