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]: Recent orders are not filtered per store at the customer account page #13257

Merged
merged 4 commits into from
Apr 9, 2018
Merged

[FIX]: Recent orders are not filtered per store at the customer account page #13257

merged 4 commits into from
Apr 9, 2018

Conversation

coderimus
Copy link
Contributor

@coderimus coderimus commented Jan 18, 2018

Description

Hello,

In this PR I provide fix for filtering recent orders block where previously orders were not filtered by store. You can have different accounts with the same emails for stores belong to a website and when you logged-in to one of those stores you will see 5 recent orders but it is possible to see orders from all stores belong to a website.

Main changes:

  1. Magento\Sales\Block\Order\Recent
  • Move recent orders select to separate method
  • Filter selected collection by current store
  1. Magento\Sales\Test\Unit\Block\Order\RecentTest
  • Change test according to the class method changes
  1. app/code/Magento/Sales/view/frontend/templates/order/recent.phtml
  • Change sizeof() to count() and execute it only once instead of double call

Best regards,
Alex

Fixed Issues (if relevant)

N/A

Manual testing scenarios

  1. Create 2 stores on one website
  2. Create 2 accounts with the same email for previously added stores
  3. Place orders per store
  4. Log In to previously created account for which you created orders and check Recent Orders section. It will show orders for all stores

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)

CollectionFactory $orderCollectionFactory,
Session $customerSession,
Config $orderConfig,
StoreManagerInterface $storeManager,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @coderimus, did you have a look to retrieve the current store id other than via adding a new dependency with StoreManager?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we need this new dependency it needs to be backwards compatible, see here: http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/#adding-a-constructor-parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@avoelkl thank you so much for your response and review!
I think I can do this without including new dependency using $this->_storeManager property in the Block class and leave the unit test with the Mock object of the storeManagerInterface. As I can see they are the same objects.
I will implement this change, test it and deploy to review ASAP.
Thank you,
Alex

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @coderimus, thanks for your reply. Let me know if you need some help!

@avoelkl
Copy link
Contributor

avoelkl commented Jan 18, 2018

Hi @coderimus,
Thanks for the PR! Please have a look at my comment/review above. Thanks!

@coderimus
Copy link
Contributor Author

Hi @avoelkl,
I provided requested changes. Please review them when it will be suitable for you :)

Changes explanation
I decided to implement backward compatibility and adding a new dependency to the class construct just to avoid overhead with Unit tests and do not refactor this block class too much which is also can generate more issues than benefits for this major version of Magento2.

The main reason why I added dependency and didn't use parent class property is that during block class instance initialization (while tests are running) the _construct() method is called and, unfortunately, it doesn't have _storeManager property from the parent class. In this case, I think it will be good just to save block class as it is. This block can be widely used and if we will remove/replace setOrders() Magento integrators can face problems with templates or other places where they call $block->getOrders()

@coderimus
Copy link
Contributor Author

Hi @avoelkl sorry if I interrupt you but when you will have free time, please, review uploaded changes. :) Thank you!

@avoelkl avoelkl added this to the January 2018 milestone Jan 26, 2018
@avoelkl
Copy link
Contributor

avoelkl commented Jan 26, 2018

Hi @coderimus!
Thanks for the update! I continue with processing the PR :)

@avoelkl
Copy link
Contributor

avoelkl commented Jan 26, 2018

Btw I saw there is a failing check from Codacy/PR Quality Review. We can ignore it in this case.

@coderimus
Copy link
Contributor Author

Hi @avoelkl,
Sorry if interrupt you but just want to ask you about next: can I prepare the backport of this issue for 2.3 and 2.4 branches?
Have a good day,
Alex

@okorshenko okorshenko modified the milestones: January 2018, February 2018 Feb 7, 2018
@okorshenko okorshenko modified the milestones: February 2018, March 2018 Mar 1, 2018
@dmanners dmanners self-assigned this Mar 9, 2018
@magento-engcom-team magento-engcom-team added Partner: Vaimo Pull Request is created by partner Vaimo partners-contribution Pull Request is created by Magento Partner Progress: accept labels Apr 5, 2018
@magento-engcom-team magento-engcom-team merged commit eda5866 into magento:2.2-develop Apr 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Component: Sales Partner: Vaimo Pull Request is created by partner Vaimo partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants