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

#4004: Newsletter Subscriber create-date not set, and change_status_at broken #11879

Merged
merged 1 commit into from
Nov 7, 2017
Merged

#4004: Newsletter Subscriber create-date not set, and change_status_at broken #11879

merged 1 commit into from
Nov 7, 2017

Conversation

nemesis-back
Copy link

@nemesis-back nemesis-back commented Oct 30, 2017 •
edited
Loading

fix for #4004

Description

This PR adds new method beforeSave for Subscriber abstract model if data has changed for subsriber_status

Fixed Issues

  1. Newsletter Subscriber create-date not set, and change_status_at broken #4004:

Manual testing scenarios

  1. Guest customer
  • Register new subscriber from footer
  • Go to Admin -> Marketing -> Newsletter Subscribers -> select customer and choose "Unsubscribe" action
  1. Registered customer
  • login as existing customer
  • go to Account Dashboard -> Newsletters -> "check" or "uncheck" checkbox
  • save

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)

osrecio
osrecio reviewed Oct 31, 2017
View reviewed changes
app/code/Magento/Newsletter/Model/Subscriber.php Outdated
@@ -95,6 +95,11 @@ class Subscriber extends \Magento\Framework\Model\AbstractModel
protected $_customerSession;

/**
* @var \Magento\Framework\Stdlib\DateTime\DateTime
*/
protected $_date;
Copy link
Member

Choose a reason for hiding this comment

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

This attribute should be private Explanation and the name of variable should be without underscore PSR-2#Properties

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, PR updated

@nemesis-back nemesis-back force-pushed the bugfix/4004-newsletter-subscriber-change-status branch from 61877cb to 3afed3d Compare October 31, 2017 10:48
@okorshenko okorshenko self-assigned this Nov 3, 2017
@okorshenko okorshenko added this to the November 2017 milestone Nov 3, 2017
@okorshenko okorshenko 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 3, 2017
@okorshenko
Copy link
Contributor

Hi @nemesis-back could you please share your email address so that we can send you SQUASHTOBERFEST contributor T-Shirt?
Email us engcom @ magento.com

@okorshenko okorshenko merged commit 3afed3d into magento:2.2-develop Nov 7, 2017
okorshenko pushed a commit that referenced this pull request Nov 7, 2017
MAGETWO-83257: #4004: Newsletter Subscriber create-date not set, and …
60a698d 
…change_status_at broken #11879

 - fixed backward compatibility issue
okorshenko pushed a commit that referenced this pull request Nov 7, 2017
MAGETWO-83257: #4004: Newsletter Subscriber create-date not set, and …
a12e731 
…change_status_at broken #11879
@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 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@osrecio osrecio osrecio left review comments

Assignees

@okorshenko okorshenko

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
Milestone
November 2017
Development

Successfully merging this pull request may close these issues.
5 participants
@nemesis-back @okorshenko @osrecio @magento-team @magento-engcom-team