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

set default storeId to 1 instead of 0 so default magento storeId is q… #2590

Merged
merged 4 commits into from
Apr 2, 2019
Merged

set default storeId to 1 instead of 0 so default magento storeId is q… #2590

merged 4 commits into from
Apr 2, 2019

Conversation

janmyszkier
Copy link
Contributor

…ueried instead of 0 which never has chance of existence as primary key

Related issues

none

Short description and why it's useful

cmsModule by default uses endpoint:
https://github.com/DivanteLtd/vue-storefront/blob/master/config/default.json#L405
but the storeId is set to 0. This PR is made for default magento storeId 1 to be queried instead of 0.
0 storeId doesn't exist in default magento instances (nor any instance which usually supports primary keys).

Screenshots of visual changes before/after (if there are any)

no changes

Screenshot of passed e2e tests (if you are using our standard setup as a backend)

none, but storeId is nowhere compared to fallback value 0

Upgrade Notes and Changelog

  • No upgrade steps required (100% backward compatibility)
  • I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

Contribution and currently important rules acceptance

@filrak filrak requested a review from pkarw March 17, 2019 20:32
@@ -54,7 +54,7 @@ export default {
return currentStoreView()
},
storeView () {
return (this.isMultistoreEnable && this.currentStore) ? this.currentStore.storeId : 0
return (this.isMultistoreEnable && this.currentStore) ? this.currentStore.storeId : 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

since storeID is set to '1' by default it can be false therefore the last check is redundant. Am I right?

Copy link
Contributor Author

@janmyszkier janmyszkier Mar 18, 2019

Choose a reason for hiding this comment

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

@filrak I don't think so.
what the above condition seems to do is:

  • if you have multistore and the current store has been correctly set, use this.currentStore.storeId (from multistore module)
  • else (multistore is disabled or this.currentStore is empty) use cms fallback (not taken from multistore fallback)

there might be some magic logic I cannot find but when we tried, both of those had to be set separately (depending on multistore setup we had). Based on that I think this still require to set BOTH fallbacks this way.

@filrak filrak changed the base branch from master to develop March 19, 2019 07:38
Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

@janmyszkier could you just update changelog.md?

Jan Myszkier added 2 commits March 25, 2019 13:28
…ueried instead of 0 which never has chance of existence as primary key
@janmyszkier
Copy link
Contributor Author

@patzick rebased on master and added changelog

@patzick patzick added this to the 1.10.0-rc.1 milestone Apr 2, 2019
@patzick patzick merged commit 904fc81 into vuestorefront:develop Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants