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

Bugfix/2918 #3156

Merged
merged 35 commits into from
Jul 10, 2019
Merged

Bugfix/2918 #3156

merged 35 commits into from
Jul 10, 2019

Conversation

pkarw
Copy link
Collaborator

@pkarw pkarw commented Jun 28, 2019

Related issues

closes #2918, #3050, #3099

Short description and why it's useful

I've fixed the naming convention of the prices field, got final_price to be used in price calculations + removed all the Vue.prototype.* references introducing the new object StorageManager

Which environment this relates to

Check your case. In case of any doubts please read about Release Cycle

  • Test version (https://test.storefrontcloud.io) - this is a new feature or improvement for Vue Storefront. I've created branch from develop branch and want to merge it back to develop
  • RC version (https://next.storefrontcloud.io) - this is a stabilisation fix for Release Candidate of Vue Storefront. I've created branch from release branch and want to merge it back to release
  • Stable version (https://demo.storefrontcloud.io) - this is an important fix for current stable version. I've created branch from hotfix or master branch and want to merge it back to hotfix

Upgrade Notes and Changelog

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

IMPORTANT NOTICE - Remember to update CHANGELOG.md with description of your change

Contribution and currently important rules acceptance

@pkarw pkarw added the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Jun 28, 2019
@pkarw pkarw requested a review from patzick June 28, 2019 14:01
@pkarw
Copy link
Collaborator Author

pkarw commented Jul 1, 2019

I've fixed the unit tests + merged the develop in

@pkarw
Copy link
Collaborator Author

pkarw commented Jul 1, 2019

  • TODO: Check the MP02 - Caesar pants - why the MP02-32-Gray is not setting the special_price in the main product
  • TODO: Add finalPriceIncludeTax parameter to the updateProductPrice function call - as it might be separate setting than sourcePriceIncludeTax

@pkarw
Copy link
Collaborator Author

pkarw commented Jul 3, 2019

  • TODO: To take care of the actions.spec.ts for the unit/cart/store as some of the tests generate warnings after new getters got introduced + basically needs to be refactored to keep up with the curent state of the cart module

@pkarw
Copy link
Collaborator Author

pkarw commented Jul 3, 2019

QA: In order to test this feature please do update vue-storefront-api on your local machine to the PR: vuestorefront/vue-storefront-api#289

In this PR I've changed the price fields naming + changed the way localStorage got operated. I've also added the support for the final_price field which was missing before. Features that needed to be tested out:

  • catalog promotion rules should got applied after product indexation (before hand they were' not - only product special_prices which is a different use case)
  • need to check the total prices in the cart,
  • order placing mechanism uses queues in the localStorage - we need to check it out if it still works just fine,
  • offline mode also uses localStorage for caching so let's try it out as well
  • all the other features which base on product or cart prices needed to be double-checked (for example bundle products)
  • the dynamic pricing alwaysSyncPlatformPricesOver feature that syncs the prices with Magento backedn

@pkarw pkarw added the QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. label Jul 3, 2019
@pkarw pkarw changed the title [WIP] Bugfix/2918 Bugfix/2918 Jul 3, 2019
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.

Looks good to me. Is it still WIP?

I have points only for storage-manager.ts i'd like you to address to:

  1. I'd change register method to set or add
  2. To what is currentStoreCode used for inside?
  3. We should have some kind of storage map inside which holds the keys, otherwise someone could register get collection and override out get
  4. What about second registration? I'm afraid of potential memory leaks - maybe for safety check if is already registered and delete it before or not register new one?
  5. Why is get registering new storage if not exists? Shouldn't just return null? Why we need exists function then if get always returns collection? I believe there are reasons for that so this is a question to clarify :)

@pkarw
Copy link
Collaborator Author

pkarw commented Jul 10, 2019

  1. I'd change register method to set or add

Done!

  1. To what is currentStoreCode used for inside?

It's for discovering the changes (https://github.com/DivanteLtd/vue-storefront/blob/3d1d81c0e1aebfcdeda97ea7fec58b6d3ab8ef29/core/lib/multistore.ts#L103) as UniversalStorage's are related to the currentStoreView by the prefix

  1. We should have some kind of storage map inside which holds the keys, otherwise someone could register get collection and override out get

Well, the object itself is kind of hash - this[collectionName] so i thoutght it's safe

  1. What about second registration? I'm afraid of potential memory leaks - maybe for safety check if is already registered and delete it before or not register new one?

I think it's safe - as the set (formerly register) just sets the property so when it's changed the reference counter got decreased and then removed so I'm not sure if the memory leak is a true danger in here?

  1. Why is get registering new storage if not exists? Shouldn't just return null? Why we need exists function then if get always returns collection? I believe there are reasons for that so this is a question to clarify :)

It's for those who miss the point storage needs to be registered :) There was no risk of having sucha a safe fallbsack

@patzick patzick removed the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Jul 10, 2019
@patzick patzick added this to the 1.11.0-rc.1 milestone Jul 10, 2019
@pkarw pkarw merged commit dea3bf8 into develop Jul 10, 2019
@pkarw pkarw mentioned this pull request Aug 22, 2019
8 tasks
@patzick patzick deleted the bugfix/2918 branch September 18, 2019 11:53
@alinadivante alinadivante removed the QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. label Sep 18, 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