Fix getReservedOrderId() to use current store instead of default store

[tdgroot]

Description

Fix getReservedOrderId() to use current store instead of default store. When placing orders from a new store view under the same website, it would use the reserved order id from the default store view.

In the image below, you'll the first 2 created orders without the fix. The last order(2000000001) is after the fix and you'll see the increment id corresponds to the store view.

I also saw a typo in Quote::substractProductFromQuotes() and refactored the method. I created a placeholder for the old method and deprecated it.

So this problem is fixed this way, but I'm wondering why it was implemented like this in the first place. Does this have implications for Single Store Mode? I'd say it wouldn't, but I think it's good to be sure. Please let me know :)

Fixed Issues (if relevant)

1. Default Store is always used when retrieving sequence value's for sales entity's. #9055

Manual testing scenarios

1. Setup a clean install of Magento
2. Add a second store view to the default website
3. Place an order on the second store view

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)

[ihor-sviziev]

@tdgroot I think we we need to add unit test for reserved order id, check that it now actually use current store. It will prevent do regression in this place

[tdgroot]

Fair point, I'm on it.

[tdgroot]

@ihor-sviziev Unit test added!

[ihor-sviziev]

I see that $quote has method getStoreId(). Isn't it better to use it?

[tdgroot]

Thank you for the review.

I'd say that's better indeed, but it shouldn't matter nonetheless. If you want it to use $quote->getStoreId() I can change that.

[tdgroot]

@ihor-sviziev I changed it to $quote->getStoreId() and the concerning Test. I also improved the test to make sure the method returns the same value as the sequence value.

[fooman]

@tdgroot Thanks for this pull request and working on adding tests. We'll wait to see what Travis reports back.

I generally agree with the proposed fix.

One thing I am concerned about is, when we roll out this change it changes the behaviour of existing stores. So people that already have stores running with order numbers 00000002 would all over a sudden start getting order numbers like 200000003 which would be unexpected.

Any thoughts on this and what could be done to mitigate it?

Also for the deprecated notice 101.0.0 is the currently already released version - the next one where this change would be included in the develop-2.2 branch would currently be 101.0.1.

[tdgroot]

@fooman Well we could write a data upgrade to set the prefix to NULL for every additional store view in a website so the increment ids stay consistent. But that doesn't feel right, because their order numbers are bugged now, and after applying this fix, that problem is solved. It'd be better to make sure this change is mentioned in the release notes.

As for the deprecation: the version number now is 101.0.1 :).

[fooman]

@tdgroot thanks for the updates and thoughts. Let me get some wider feedback on how best to deal with existing stores.

[dverkade]

@fooman We have several clients complaining about this issue, because the behaviour of M2 right now is different then the expected behaviour of the user and the behaviour of Magento 1. So from a user perspective it's already broken and needs to be fixed. Therefor I do not see an issue when the prefixes will be changed.

[okorshenko]

Update: we are waiting for approval from Product Owner of this feature

[okorshenko]

Update: Product Owner approved this change. Thank you everyone for collaboration

[tdgroot]

@okorshenko Thanks for finalizing this PR! Will this change get merged in 2.3 branch by Magento engineering or does it need another backport PR?

[ihor-sviziev]

@tdgroot accepting PRs are usually much faster. Could you please create PRs with the same changes to 2.1-develop and 2.3-develop branches?

[mimou78]

Hello,

I think this is not a good solution.
I think the best solution is to make this configurable.
because in my case I use the storeview to have two languages in my site. I do not have to have a different incremetation per language.

Thanks

[ihor-sviziev]

Hi @mimou78,
It looks like a feature request. PR is welcomed 👍