Skip to content

Wrong store_id assigned to Customer when reading his subscription to Newsletter #13469

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

Closed
wants to merge 1 commit into from

Conversation

Nil79
Copy link

@Nil79 Nil79 commented Feb 2, 2018

Description

The issue comes in a scenario with multiple stores.

When we edit an existing customer that was previously subscribed in newsletter from backend, magento assigns to him store_id 1 also if he was created from another store (e.g store 2).

From the manage Customer page, In the Newsletter panel tab, the label "Last Date Unsubscribed" with the date of last subscription of the user, does not appear.

If we try to subscribe again the customer into newsletter (always from Customer page), Magento will create a duplicated entry in the database table "newsletter_subscriber"

Fixed Issues (if relevant)

N/A

Manual testing scenarios

  1. Create a new customer for the store view with store_id = 2 (if you have two stores, be sure to select the store view with a store_id different from 1)
  2. From the customer page, Assign the customer to newsletter.
  3. Edit again the customer, go to newsletter panel: The label "Last Date Unsubscribed" does not appear.
  4. Subscribe again the customer and check the table newsletter_subscriber into database. You will find a duplicated entry. (If you save more times, a new row will be added)

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)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Feb 2, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@rogyar rogyar self-assigned this Feb 2, 2018
@rogyar rogyar added this to the February 2018 milestone Feb 3, 2018
@magento-engcom-team
Copy link
Contributor

@Nil79 thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@rogyar
Copy link
Contributor

rogyar commented Feb 5, 2018

Hello @Nil79. Thank you for your contribution.
The store_id check has been introduced in a scope of #12035 in order to fix an issue with multistore subscriptions. We cannot just remove the store_id check. Instead of removing the check, we need to determine the correct customer store_id upon subscription from the admin panel. At the moment if we subscribe a customer via admin panel, we always have store_id = 0.
Thanks.

@@ -360,7 +360,6 @@ public function loadByCustomerId($customerId)
{
try {
$customerData = $this->customerRepository->getById($customerId);
$customerData->setStoreId($this->_storeManager->getStore()->getId());
Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot simply remove the store_id check since it brings problems with multistore subscriptions. Details are here: #10014

@Nil79
Copy link
Author

Nil79 commented Feb 7, 2018

Hello @rogyar , Thanks for the feedback, I'm testing the frontend in a multisite scenario and it seems that this commit doesn't affect the frontend newsletter subscription, because the function loadByCustomerId also loads the customer for the right storeId before the line "$customerData->setStoreId($this->_storeManager->getStore()->getId());" is called.

But to be sure that I'm doing the right tests on frontend (and before make changes to solve the issue on backend) l I've contacted the author of the pull request #12035, to get more feedbacks.

Regards,

@rogyar
Copy link
Contributor

rogyar commented Feb 28, 2018

Hello @Nil79. Any update regarding this issue? Thanks

@Nil79 Nil79 force-pushed the 2.2-develop branch 2 times, most recently from 5b461aa to 999f439 Compare February 28, 2018 15:53
@Nil79
Copy link
Author

Nil79 commented Feb 28, 2018

Hello @rogyar ,

I have contacted the author of the pull request #12035, to get more clarifications about the changes in the file Subscriber.php, but I didn't receive any reply.

I have just made some tests, verifying that in a multisite scenario, removing the following line of code doesn't affects the issue:

$customerData->setStoreId($this->_storeManager->getStore()->getId());

I don't know if to wait again for the reply or just apply a little code change to skip this line of code when the user is in the Admin area (but also removing it definitively doesn't affect the issue as I have said)

Regards.

@okorshenko okorshenko modified the milestones: February 2018, March 2018 Mar 1, 2018
@okorshenko okorshenko modified the milestones: March 2018, April 2018 Apr 16, 2018
@okorshenko okorshenko modified the milestones: April 2018, May 2018 May 18, 2018
@okorshenko okorshenko removed this from the May 2018 milestone May 31, 2018
@ihor-sviziev ihor-sviziev self-assigned this Aug 8, 2018
@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Aug 8, 2018

Hi @Nil79,
Could you clarify what actual steps to reproduce and what is actual and expected results?
As for me it not looks as a proper fix for issue.

It looks as you trying to fix the same issue as #14186

@osrecio osrecio self-assigned this Aug 28, 2018
@osrecio
Copy link
Member

osrecio commented Aug 28, 2018

Hi @Nil79 , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@osrecio osrecio closed this Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants