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

newsletter module #2754

Merged
merged 15 commits into from
May 21, 2019
Merged

newsletter module #2754

merged 15 commits into from
May 21, 2019

Conversation

mdesmet
Copy link
Contributor

@mdesmet mdesmet commented Apr 17, 2019

Related issues

closes #2558

Short description and why it's useful

Newsletter module supporting subscribe/unsubscribe (can be integrated with mailchimp module (in develop branch of vue-storefront-api) but as long as API is respected with any email provider.

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

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 requested review from filrak and patzick April 18, 2019 04:27
@pkarw
Copy link
Collaborator

pkarw commented Apr 18, 2019

Cool stuff, really like this approach. Thanks! Could you please check the failing checks and update changelog?

@mdesmet
Copy link
Contributor Author

mdesmet commented Apr 18, 2019

It seems this issue is also on the divante develop branch and not related to my commit.

It seems to be related to using following code parts:

once('__VUE_EXTEND_VUEX__', () => { Vue.use(Vuex) }) in core/store/index.ts

once('__VUE_EXTEND_I18N__', () => { Vue.use(VueI18n) }) in core/i18n/index.ts

If I remove the once wrapping, the tests work, @patzick : can you have a look?

@pkarw
Copy link
Collaborator

pkarw commented Apr 18, 2019

@mdesmet we need once() to avoid memory leaks in the ssr there must be some other solution to this failing check

@pkarw
Copy link
Collaborator

pkarw commented Apr 18, 2019

I belive we just must adjust the unit test: core/modules/cart/test/unit/store/mutations.spec.ts @lukeromanowicz

@lukeromanowicz
Copy link
Contributor

lukeromanowicz commented Apr 23, 2019

@mdesmet I've fixed the failing test from develop branch but there still were some errors coming from yours. To speed things up I've done the following changes:

  • split test suite from one integration suite into 4 separate unit test suites
  • fixed eslint in mixins and test
  • added return values to mixin methods so they can be tested.

I hope that you don't mind that.

@lukeromanowicz
Copy link
Contributor

I've also updated the Changelog and Braking Changes list because the code of this PR is not backward compatible.

@lukeromanowicz lukeromanowicz added this to the 1.10.0-rc.1 milestone Apr 23, 2019
@lukeromanowicz
Copy link
Contributor

lukeromanowicz commented Apr 23, 2019

I approve the code but I'm not really able to fully test it. Maybe @patzick could take a look if it works as expected. Please, remember that there is also API part: vuestorefront/vue-storefront-api#212

Edit:
I talked with OP and it looks like it works correctly. There is no mechanism for keeping information if user has already signed to the newsletter but I suggest doing it in another PR (so we have a base that could be further improved).

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.

Very good piece of code, thanks!

Just please resolve conflicts and optionally get rid of the file i listed below

@pkarw
Copy link
Collaborator

pkarw commented May 10, 2019

@mdesmet could You please resolve the conflicts in order to merge this feature in?

@pkarw pkarw merged commit 74596d9 into vuestorefront:develop May 21, 2019
@patzick patzick mentioned this pull request May 28, 2019
8 tasks
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

4 participants