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

Refactoring user vuex module #3247

Conversation

andrzejewsky
Copy link
Contributor

@andrzejewsky andrzejewsky commented Jul 17, 2019

Related issues

closes #3095

Acceptance criteria

  • network calls refactoed to data-resolvers
  • independent logic moved to the helpers
  • try to remove spawnNotification move the notifications one level higher (to the UI)
  • replace all rootStore calls with the context instead
  • remove the calls to history and me from login
  • replace all fetch calls with Task api calls

Which environment this relates to

  • 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

Contribution and currently important rules acceptance

@andrzejewsky
Copy link
Contributor Author

Not ready yet. I'm still working on it. There are few criteria left.

@andrzejewsky andrzejewsky added the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Jul 17, 2019
@andrzejewsky andrzejewsky self-assigned this Jul 17, 2019
@andrzejewsky andrzejewsky changed the title Feature/3095 refactoring user vuex module Refactoring user vuex module Jul 17, 2019
@filrak filrak changed the title Refactoring user vuex module [WIP] Feature/3095 refactoring user vuex module Jul 17, 2019
@filrak
Copy link
Collaborator

filrak commented Jul 17, 2019

in that cases please add WIP flag at the beginning suggesting Work in progress ;)

Copy link
Collaborator

@filrak filrak left a comment

Choose a reason for hiding this comment

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

Just letting you know that name of the user cache collection will change (LS key stays the same). PR with naming change will probably be merged tomorrow. Looks good btw ;)

core/modules/user/store/actions.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

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

It's on a good way - please continue and let's merge this one in shortly :)

@pkarw
Copy link
Collaborator

pkarw commented Jul 18, 2019

@andrzejewsky please do change the references to Vue.prototype.$db.* to StorageManager.get('*') and it should be OK to merge this one in

@pkarw pkarw requested a review from patzick July 18, 2019 14:45
@andrzejewsky andrzejewsky force-pushed the feature/3095-refactoring-user-vuex-module branch 2 times, most recently from 743cc42 to 76cdcb2 Compare July 18, 2019 19:10
@andrzejewsky andrzejewsky changed the title [WIP] Feature/3095 refactoring user vuex module Refactoring user vuex module Jul 18, 2019
@andrzejewsky
Copy link
Contributor Author

OK it's ready. I didn't remove the spawnNotification calls, Because I've noticed there is dedicated issue (https://github.com/DivanteLtd/vue-storefront/issues/3141) for that.

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.

Nice work!:)
Few notes from me. Also i don't exactly get using Promise.resolve inside async methods, i asked in one of them; i'll be grateful for explanation :)

core/data-resolver/UserService.ts Outdated Show resolved Hide resolved
core/data-resolver/UserService.ts Outdated Show resolved Hide resolved
core/modules/user/store/actions.ts Outdated Show resolved Hide resolved
core/modules/user/store/actions.ts Outdated Show resolved Hide resolved
core/modules/user/store/actions.ts Outdated Show resolved Hide resolved
core/store/getters.ts Outdated Show resolved Hide resolved
core/store/getters.ts Outdated Show resolved Hide resolved
core/store/getters.ts Outdated Show resolved Hide resolved
core/store/actions.ts Outdated Show resolved Hide resolved
core/modules/user/store/actions.ts Outdated Show resolved Hide resolved
@andrzejewsky andrzejewsky force-pushed the feature/3095-refactoring-user-vuex-module branch 3 times, most recently from 323ece9 to b0a7e39 Compare July 19, 2019 14:01
@andrzejewsky
Copy link
Contributor Author

@patzick Done.

@filrak
Copy link
Collaborator

filrak commented Jul 19, 2019

It would be nice if we could also document stuff that we write in VS docs (at least briefly). One @degi is not enough to write it, maintain it and keep up with new stuff. @andrzejewsky could you please add a Data Resolvers section to docs with very brief documentation of this one?

@andrzejewsky
Copy link
Contributor Author

@filrak just added a new section about data resolvers in the docs, with the short example of how to create a new one

@filrak
Copy link
Collaborator

filrak commented Jul 19, 2019

I meant you should document the user data resolver that you made in a new section about data resolvers in docs. Sorry if I wasn't precise. How I see this is that "data resolvers" should be a separate category in the docs with agenda more or less like:

Data resolvers

  • Introduction (the doc you made)
  • User data resolver
  • ...

What do you think about this? ;)

@andrzejewsky
Copy link
Contributor Author

@filrak sounds good, I'll do that.

@andrzejewsky andrzejewsky force-pushed the feature/3095-refactoring-user-vuex-module branch from e40542f to a83d8b3 Compare July 20, 2019 15:31
@andrzejewsky
Copy link
Contributor Author

@filrak Updated. Also I added docs about CategoryService. It's just a simple few words about each service. Let me know, if you have noticed there either is something missing or I should add something else.

@pkarw
Copy link
Collaborator

pkarw commented Jul 22, 2019

Hi @andrzejewsky is this one ready to be merged in? Have all @patzick hints from the CR been applied?

@andrzejewsky
Copy link
Contributor Author

@pkarw yes, it's ready.

@pkarw pkarw requested a review from patzick July 22, 2019 06:53
@pkarw
Copy link
Collaborator

pkarw commented Jul 22, 2019

OK, @andrzejewsky next time, please "Resolve" all the comments or comment about the changnes/actions taken. We're reviewing the PRs in a group of 3-4 ppl (me, @lukeromanowicz, @filrak, @patzick) and this helps us a lot :)

@pkarw pkarw requested a review from filrak July 22, 2019 13:29
@patzick
Copy link
Collaborator

patzick commented Jul 23, 2019

Looks good, could you just resolve conflict?

@patzick patzick added the QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. label Jul 23, 2019
@andrzejewsky andrzejewsky force-pushed the feature/3095-refactoring-user-vuex-module branch 2 times, most recently from 80d545a to 9aa5496 Compare July 23, 2019 08:34
@andrzejewsky andrzejewsky force-pushed the feature/3095-refactoring-user-vuex-module branch from c7b623b to 32a23e2 Compare July 23, 2019 08:54
@patzick patzick merged commit 76211f7 into vuestorefront:develop Jul 23, 2019
@patzick patzick removed QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. not ready for merge PR is holded. Needs some clarifications or things that need to be finished. labels Jul 23, 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.

4 participants