Bug #5537/newsletter functionality #5601

Baroshem · 2021-03-02T09:38:11Z

Related Issues

Short Description and Why It's Useful

This pull request implements functionality for updating newsletter sections. For now it's just a mock (similarly to user billing), so after page reload checked checkboxes won't be checked. However, if the user navigates the pages/sections, the checkboxes will remain checked.

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 Vue Storefront sites with this new feature
(Next only) I've followed this instruction and I've created a .js file with information about my Pull Request

IMPORTANT NOTICE

Remember to update CHANGELOG.md with description of your change

Contribution and Currently Important Rules Acceptance

I read and followed contribution rules

- encapsulate logic, style, and template into separate form component - add local reactive object for newsletter checkbox - create updateNewsletterField method for handling checkbox status change - create submitForm method for submitting checked checkboxes

- move form template to separate component for easier reusability and readability - include useNewsletter in setup method - add onSSR lifecycle hook to load newsletter data - add method to updateNewsletter on form submit

CLAassistant · 2021-03-02T09:38:18Z

All committers have signed the CLA.

coveralls · 2021-03-02T09:45:47Z

Pull Request Test Coverage Report for Build 613559196

31 of 31 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+5.3%) to 86.36%

Totals
Change from base Build 613485104:	5.3%
Covered Lines:	846
Relevant Lines:	965

💛 - Coveralls

github-actions · 2021-03-02T09:49:14Z

💙 vsf-next-demo successfully deployed at https://90607d712e9e1aadc630117f7848c5f85287a340.vsf-next-demo.preview.storefrontcloud.io

coveralls · 2021-03-02T09:49:34Z

Pull Request Test Coverage Report for Build 613559196

0 of 42 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-15.7%) to 65.314%

Totals
Change from base Build 613485104:	-15.7%
Covered Lines:	322
Relevant Lines:	395

💛 - Coveralls

coveralls · 2021-03-02T09:49:35Z

Pull Request Test Coverage Report for Build 613559196

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

42 of 42 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.3%) to 81.328%

Totals
Change from base Build 613485104:	0.3%
Covered Lines:	1548
Relevant Lines:	1770

💛 - Coveralls

filrak · 2021-03-02T10:13:00Z

It was not intended to add a new composable with #5537. I'm closing for now - likely useNewsletter topic will come back

filipsobol · 2021-03-02T10:16:32Z

packages/core/core/src/factories/useNewsletterFactory.ts

+      }
+    };
+
+    const updateNewsletterData = async (newsletter): Promise<NewsletterSections> => {


updateNewsletterData name is redundant because it's already is part of useNewsletter. I suggest naming it just update to match load method.

filipsobol · 2021-03-02T10:23:32Z

packages/core/core/src/factories/useNewsletterFactory.ts

+export interface UseNewsletterFactoryParams extends FactoryParams {
+  updateNewsletterData: (
+    context: Context,
+    params: NewsletterSections) => Promise<NewsletterSections>;


) => Promise<NewsletterSections>; should go to the next line

filipsobol · 2021-03-02T10:30:50Z

packages/core/docs/changelog/5537.js

+  description: 'create update newsletter sections functionality',
+  link: 'https://github.com/vuestorefront/vue-storefront/issues/5537',
+  isBreaking: true,
+  breakingChanges: [


useNewsletter composable is not BC, because it's totally new composable. However, changes in MyNewsletter.vue that require a new component that uses new composable is BC :)

{ { module: 'core / commercetools', before: '', after: 'Added newsletter functionality', comment: 'Added \'useNewsletter\' composable and \'components/MyAccount/NewsletterForm.vue\' component, modified \'pages/MyAccount/MyNewsletter.vue\' component' } }

filipsobol · 2021-03-02T10:32:08Z

packages/core/core/src/types.ts

+export interface NewsletterSections {
+  woman: boolean,
+  man: boolean,
+  kids: boolean,
+}


We can't hardcode Newsletter options.

filipsobol · 2021-03-02T10:41:26Z

packages/core/core/src/types.ts

+export interface UseNewsletter {
+  newsletter: ComputedProperty<NewsletterSections>;
+  loading: ComputedProperty<boolean>;
+  error: ComputedProperty<UseNewsletterErrors>;
+  updateNewsletterData: (params: NewsletterSections) => Promise<NewsletterSections>;
+  load: () => Promise<NewsletterSections>;
+}


I see few issues here:

Response from the API saved in the newsletter property will be different for every integration, so we cannot assume any signature. We need TypeScript's generic here, like NEWSLETTER.

We don't return any values from the load or update methods in any other composable, so to be consistent, both should return Promise<void>.

We don't know what properties might be needed in update method. I would suggest using generic here, like NEWSLETTER_UPDATE_PARAMS.

export interface UseNewsletter<NEWSLETTER, NEWSLETTER_UPDATE_PARAMS> { newsletter: ComputedProperty<NEWSLETTER>; loading: ComputedProperty<boolean>; error: ComputedProperty<UseNewsletterErrors>; updateNewsletterData: (params: NEWSLETTER_UPDATE_PARAMS) => Promise<void>; load: () => Promise<void>; }

filipsobol · 2021-03-02T10:45:49Z

packages/core/core/src/factories/useNewsletterFactory.ts

+        newsletterRef.value = await _factoryParams.updateNewsletterData(newsletter);
+        return newsletterRef.value;
+      } catch (err) {
+        error.value = err;


error.value.update = err;

filipsobol · 2021-03-02T10:47:09Z

packages/core/nuxt-theme-module/theme/pages/MyAccount/MyNewsletter.vue

+      try {
+        const data = await updateNewsletterData(newsletter);
+        await onComplete(data);
+      } catch (error) {
+        onError(error);
+      }


Unfortunately, the code you referred to is outdated. We no longer throw errors from methods. To check if the request was successful, refer to the error object from useNewsletter.

filipsobol · 2021-03-02T10:49:29Z

packages/commercetools/composables/__tests__/useNewsletter/useNewsletter.spec.ts

+
+    const loadedNewsletter = await load({});
+
+    expect(loadedNewsletter).toStrictEqual(defaultNewsletterData);


Here you use toStrictEqual, but below you use toMatchObject. If both are used for the same purpose, I'd suggest using toMatchObject because it better describes what we are expecting.

Additionally, as load shouldn't return any data, we should be comparing defautNewsletterData and newsletter object from useNewsletter composable.

filipsobol · 2021-03-02T10:51:41Z

packages/commercetools/theme/components/MyAccount/NewsletterForm.vue

@@ -0,0 +1,103 @@
+<template>
+    <form
+      id="newsletter-form"


We are not using this anywhere, so it may be removed

filipsobol · 2021-03-02T10:53:17Z

packages/commercetools/theme/components/MyAccount/NewsletterForm.vue

+      <div class="form__checkbox-group">
+        <SfCheckbox
+          v-model="newsletter.woman"
+          @change="updateNewsletterField('woman', newsletter.woman)"


I don't think it will change anything. Should we use !newsletter.woman or $event's value?

Baroshem added 16 commits February 28, 2021 12:50

bug-#5537: remove unnecessary validation observer

94abb01

bug-#5537: create NewsletterForm template in core

1cda1bb

bug-#5537: create useNewsletter in boilerplate

491a393

bug-#5537: create useNewsletter in commercetools

ded9fe1

bug-#5537: create useNewsletterFactory in core

5c90883

bug-#5537: create types for newsletter logic

4cff613

bug-#5537: create newsletterGetters in boilerplate

e6c5f68

bug-#5537: create newsletterGetters for ct

317981e

bug-#5537: adjust NewsletterForm to use props

9b041a5

bug-#5537: extend MyNewsletter logic

30c516b

- move form template to separate component for easier reusability and readability - include useNewsletter in setup method - add onSSR lifecycle hook to load newsletter data - add method to updateNewsletter on form submit

bug-#5537: remove unused newsletterGetters

0b7d484

bug-#5537: adjust return type for updateNewsletter

80dc857

bug-#5537: add guard clause for updateNewsletter

80c5b71

bug-#5537: add changelog for update newsletter

1d5bc42

bug-#5537: write unit tests for useNewsletter

3622791

Baroshem added the backlog label Mar 2, 2021

Baroshem self-assigned this Mar 2, 2021

Baroshem mentioned this pull request Mar 2, 2021

[BUG] My Profile #5520

Closed

4 tasks

Baroshem changed the title ~~[WIP] Bug #5537/newsletter and profile~~ Bug #5537/newsletter functionality Mar 2, 2021

Baroshem requested a review from filrak March 2, 2021 10:02

filrak closed this Mar 2, 2021

filipsobol suggested changes Mar 2, 2021

View reviewed changes

filrak deleted the bug-#5537/newsletter-and-profile branch March 19, 2021 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug #5537/newsletter functionality #5601

Bug #5537/newsletter functionality #5601

Baroshem commented Mar 2, 2021 •

edited by sync-by-unito bot

Loading

CLAassistant commented Mar 2, 2021 •

edited

Loading

coveralls commented Mar 2, 2021 •

edited

Loading

github-actions bot commented Mar 2, 2021

coveralls commented Mar 2, 2021

coveralls commented Mar 2, 2021 •

edited

Loading

filrak commented Mar 2, 2021

filipsobol Mar 2, 2021

filipsobol Mar 2, 2021

filipsobol Mar 2, 2021

filipsobol Mar 2, 2021

filipsobol Mar 2, 2021

filipsobol Mar 2, 2021

filipsobol Mar 2, 2021

filipsobol Mar 2, 2021

filipsobol Mar 2, 2021

filipsobol Mar 2, 2021

filipsobol Mar 2, 2021


		const loadedNewsletter = await load({});

		expect(loadedNewsletter).toStrictEqual(defaultNewsletterData);

Bug #5537/newsletter functionality #5601

Bug #5537/newsletter functionality #5601

Conversation

Baroshem commented Mar 2, 2021 • edited by sync-by-unito bot Loading

Related Issues

Short Description and Why It's Useful

Screenshots of Visual Changes before/after (if There Are Any)

Which Environment This Relates To

Upgrade Notes and Changelog

Contribution and Currently Important Rules Acceptance

CLAassistant commented Mar 2, 2021 • edited Loading

coveralls commented Mar 2, 2021 • edited Loading

Pull Request Test Coverage Report for Build 613559196

💛 - Coveralls

github-actions bot commented Mar 2, 2021

coveralls commented Mar 2, 2021

Pull Request Test Coverage Report for Build 613559196

💛 - Coveralls

coveralls commented Mar 2, 2021 • edited Loading

Pull Request Test Coverage Report for Build 613559196

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

filrak commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baroshem commented Mar 2, 2021 •

edited by sync-by-unito bot

Loading

CLAassistant commented Mar 2, 2021 •

edited

Loading

coveralls commented Mar 2, 2021 •

edited

Loading

coveralls commented Mar 2, 2021 •

edited

Loading