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

[Backport 2.3] #11825: Generate new FormKey and replace for oldRequestParams Wishlist #12042

Merged
merged 3 commits into from
Dec 1, 2017
Merged

[Backport 2.3] #11825: Generate new FormKey and replace for oldRequestParams Wishlist #12042

merged 3 commits into from
Dec 1, 2017

Conversation

osrecio
Copy link
Member

@osrecio osrecio commented Nov 5, 2017

Generate new FormKey afterLogin and set to the beforeRequest(Wishlist)

Description

Generate new FormKey afterLogin and set to the beforeRequest(Wishlist)

Fixed Issues (if relevant)

  1. 2.1.9 Item not added to the Wishlist if the user is not logged at the moment he click on the button to add it. #11825: 2.1.9 Item not added to the Wishlist if the user is not logged at the moment he click on the button to add it
  2. Adding to wishlist doesn't work when not logged in #11908: Adding to wishlist doesn't work when not logged in

Manual testing scenarios

  1. Create an user account.
  2. Logout from the user account
  3. Add a product to your Wishlist , you will get redirected to the login page
  4. Login

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)

@vkublytskyi vkublytskyi self-assigned this Nov 5, 2017
@vkublytskyi vkublytskyi added this to the November 2017 milestone Nov 5, 2017
@vkublytskyi vkublytskyi added Release Line: 2.3 2.2.x duplicate 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
@vkublytskyi
Copy link

@osrecio tests failed as this fix introduces dependency for PageCache on Customer module. We should avoid this for bugfixes. Also we should avoid this as Customer module already has a dependency on PageCashe module and we must avoid circular dependencies. As a solution, I may suggest make a plugin on \Magento\PageCache\Observer\FlushFormKey in a customer module

@osrecio osrecio force-pushed the PR#11825_2.3 branch 3 times, most recently from 63b9e78 to a67e08d Compare November 26, 2017 11:51
@osrecio
Copy link
Member Author

osrecio commented Nov 26, 2017

Hi @vkublytskyi I added a Plugin in Customer Module to make the same logic.

vkublytskyi
vkublytskyi reviewed Nov 27, 2017
View reviewed changes
app/code/Magento/Customer/Model/Plugin/CustomerFlushFormKey.php Outdated
/**
* @param FlushFormKey $subject
* @param callable $proceed
* @param $args
Copy link

Choose a reason for hiding this comment

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

Please add @SuppressWarnings(PHPMD.UnusedFormalParameter) to avoid static tests failure

@osrecio osrecio force-pushed the PR#11825_2.3 branch 2 times, most recently from 6a7d8a0 to 107e375 Compare November 27, 2017 20:07
@osrecio
Copy link
Member Author

osrecio commented Nov 27, 2017

@vkublytskyi @ihor-sviziev

  1. Added @SuppressWarnings(PHPMD.UnusedFormalParameter) to avoid static tests failure
  2. Deleted not related changes
  3. Added Unit Tests

@ihor-sviziev ihor-sviziev self-assigned this Nov 28, 2017
vkublytskyi
vkublytskyi suggested changes Nov 28, 2017
View reviewed changes
Copy link

@vkublytskyi vkublytskyi left a comment

Choose a reason for hiding this comment

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

Unit test should be updated to fix issue with passed invalid argument. See #12038 (comment)

MAGETWO-83289: [Backport 2.3] magento#11825: Generate new FormKey and…
cc1602f 
… replace for oldRequestParams Wishlist magento#12042

 - fixed unit test
@osrecio
Copy link
Member Author

osrecio commented Dec 1, 2017

Thanks @vkublytskyi I was going to work on the changes this weekend. Busy Week

@okorshenko okorshenko merged commit cc1602f into magento:2.3-develop Dec 1, 2017
okorshenko pushed a commit that referenced this pull request Dec 1, 2017
@ishakhsuvarov
MAGETWO-83289: [Backport 2.3] #11825: Generate new FormKey and replac…
84c1beb 
…e for oldRequestParams Wishlist #12042
okorshenko pushed a commit that referenced this pull request Dec 1, 2017
@ishakhsuvarov
Merge pull request #1796 from magento-engcom/develop-prs
bc6ea8c 
[EngCom] Public Pull Requests - develop
 - MAGETWO-83289: [Backport 2.3] #11825: Generate new FormKey and replace for oldRequestParams Wishlist #12042
 - MAGETWO-84529: [2.3] Correct flat indexing with staging and > 500 cats #12345
@magento-engcom-team magento-engcom-team added Fixed in 2.1.x The issue has been fixed in 2.1 release line Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line labels Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihor-sviziev ihor-sviziev ihor-sviziev approved these changes

@vkublytskyi vkublytskyi vkublytskyi approved these changes

Assignees

@vkublytskyi vkublytskyi

@ihor-sviziev ihor-sviziev

Labels
Award: test coverage duplicate Fixed in 2.1.x The issue has been fixed in 2.1 release line Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Progress: accept Release Line: 2.3 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
December 2017
Development

Successfully merging this pull request may close these issues.
6 participants
@osrecio @vkublytskyi @ihor-sviziev @magento-team @okorshenko @magento-engcom-team