feat: #49/shipping billing #51

Baroshem · 2021-08-20T19:29:25Z

Closes #49

Description

Saving and loading shipping and billing data works, now I have to also fetch the shipping methods.

EDIT: Fetching Shipping Methods work but now I also have to figure out how to update the cart object.

EDIT: Everything works

Screen.Recording.2021-08-22.at.13.41.31.mov

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

Baroshem · 2021-08-22T11:56:52Z

@michaelbromley If you could take a look at GraphQL queries, mutations and fragments and @Mateuszpietrusinski if you could do a general check of this feature that would be great. I can offer a cute cat picture in return:

Mateuszpietrusinski

A lot of work has been done 👍 .

Mateuszpietrusinski · 2021-08-23T06:22:20Z

packages/api-client/__tests__/api/getShippingMethods.spec.ts

+import eligibleShippingMethodsQuery from '../../src/api/getShippingMethods/eligibleShippingMethodsQuery';
+import { Context } from '../../src/types';
+
+describe('[vendure-api-client] getShippingMethods', () => {


think about different scenarios, like apollo client function should have been called with passed params

For now, all unit tests are pretty basic stuff. I will refactor them later so that they will serve more purpose.

Mateuszpietrusinski · 2021-08-23T06:26:52Z

packages/api-client/__tests__/api/updateAddressDetails.spec.ts

+
+    const { data } = await updateAddressDetails(context, { input: givenVariables.input });
+
+    expect(data).toBe('update shipping details response');


you can extract string into a variable and place it into a given section with the name expectedAddressDetails this will contribute to readability.

Mateuszpietrusinski · 2021-08-23T06:28:46Z

packages/api-client/src/api/updateAddressDetails/index.ts

+  const request = await context.client.mutate({
+    mutation: gql`${updateAddressDetails.query}`,
+    variables: updateAddressDetails.variables,
+    fetchPolicy: 'no-cache'


Maybe its good to move that const into global variable

Yes, it is used in many places. Thanks for noticing that!

Mateuszpietrusinski · 2021-08-23T06:35:39Z

packages/composables/__tests__/mocks/index.ts

@@ -1,3 +1,4 @@
 export * from './mockedCollectionList';


comment related to folder name :
Jest recommends to name mocks folder with underscores https://jestjs.io/docs/manual-mocks#mocking-user-modules.
So instead of mocks you will have __mocks__.
Here you can find examples https://jestjs.io/docs/manual-mocks#examples

Excelent, I will correct that :)

Mateuszpietrusinski · 2021-08-23T06:39:14Z

packages/composables/__tests__/useBilling/useBilling.spec.ts

+    const response = await load(context, {});
+
+    expect(response).toBe(loadedBillingAddress);
+    expect(context.cart.load).not.toHaveBeenCalled();


I'm just wondering why are you used .not in this place?

copy/paste from CT project. Should probably remove it

Mateuszpietrusinski · 2021-08-23T06:42:15Z

packages/composables/src/composables/useBilling/index.ts

+const params: UseBillingParams<OrderAddress, AddParams> = {
+  provide() {
+    return {
+      cart: useCart()


IMO you can have some issues when you use other composable outside setup function, but let me know if error occur sometimes?

There is no error and we are using similar functionality in order integrations as well. Is it because in some cases we are fetching the data that we already have so better approach would be to reuse the data.

Mateuszpietrusinski · 2021-08-23T06:44:58Z

packages/composables/src/composables/useBilling/index.ts

@@ -3,23 +3,34 @@ import {
  useBillingFactory,


If the logic for billing is related to cart at the backend maybe think about extending cart composable instead of covering useBilling. This approach will give you the ability to have cart value available out of the box.

Shipping, Billing and Shipping provided is all related to cart but I thought that it would be better to have it separate as it requires different mutations/queries to be performed.

Mateuszpietrusinski · 2021-08-23T06:47:54Z

packages/composables/src/composables/useShipping/index.ts


-const params: UseShippingParams<ShippingAddress, AddParams> = {
+const params: UseShippingParams<OrderAddress, AddParams> = {
+  provide() {


AFAIK UseShippingParams by default don't have provide function, are everything works well for TS?

Yes, AFAIK everything is working correctly. No errors what so ever.

Ok, FactoryParams shipps provide fucntion

Mateuszpietrusinski · 2021-08-23T06:52:32Z

packages/theme/components/Checkout/VsfShippingProvider.vue

-        </div>
-        <div v-else-if="errorShippingProvider.save">
+      <SfLoader :loading="loadingShippingProvider">
+        <div v-if="errorShippingProvider.save">


IMO it's better to place errorShippingProvider.save into a variable and maybe I'm wrong but if errorShippingProvider is computed you need to access the value in this way errorShippingProvider.value.save

In the template we do not have to use .value

michaelbromley

The queries & fragments look fine to me.

One thing I was wondering about: I could not see whether you are handling the ErrorResult cases for the various mutations. Very possibly I just was unable to locate this code given my unfamiliarity with the patterns.

Baroshem · 2021-08-24T13:43:59Z

The queries & fragments look fine to me.

One thing I was wondering about: I could not see whether you are handling the ErrorResult cases for the various mutations. Very possibly I just was unable to locate this code given my unfamiliarity with the patterns.

For now I was just passing the basic ErrorResult fragment from the query/mutation. The error will be available in the error property of certain factory (like useShippingFactory) and can be accessible in the theme (frontend of the frontend ;)) but I also do not wanted to add to much complexity into one PR. I will create an issue to add other errors as framgents and error handling

Baroshem added 4 commits August 20, 2021 08:54

feat: #49 (api) create api-client function

312f896

feat: #49(composable) add useShipping & useBilling

cca6719

feat: #49 (composable) add unit tests

529a451

feat: #49 (docs) add docs

61bfd68

Baroshem self-assigned this Aug 20, 2021

Baroshem added 10 commits August 21, 2021 09:21

feat: #49 (theme) update vue components

7187b4c

feat: #49 (api) getShippingMethods function

1cd437b

feat: #49 (api) setShippingMethod function

088f0d4

feat: #49 (*) adjust billing shipping

c03bf85

feat: #49 (*) update totals in cartPreview

70d9e29

feat: #49 (composable) write unit tests

c019eb1

feat: #49 (docs) useShippingProvider

5f52511

feat: #49 (*) run lint

423c6e6

feat: #49 (theme) remove unnecessary code

9a9b46a

feat: #49 (*) fix price helper

036a964

Baroshem changed the title ~~feat: #49/shipping billing [WIP]~~ feat: #49/shipping billing Aug 22, 2021

Baroshem requested review from Mateuszpietrusinski and michaelbromley August 22, 2021 11:53

Mateuszpietrusinski reviewed Aug 23, 2021

View reviewed changes

Baroshem added 2 commits August 24, 2021 15:25

feat: #49 (test) store string in variable

f912fd1

feat: #49 (api) use constant for fetch policy

c7cbdb8

michaelbromley reviewed Aug 24, 2021

View reviewed changes

Baroshem added 2 commits August 24, 2021 15:33

feat: #49 (composable) rename to __mocks__

22c1bbf

feat: #49 (composable) fix useBilling test

d8e02ce

Mateuszpietrusinski approved these changes Aug 25, 2021

View reviewed changes

Baroshem merged commit 59ccdf3 into main Aug 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: #49/shipping billing #51

feat: #49/shipping billing #51

Baroshem commented Aug 20, 2021 •

edited

Loading

Baroshem commented Aug 22, 2021

Mateuszpietrusinski left a comment

Mateuszpietrusinski Aug 23, 2021

Baroshem Aug 24, 2021

Mateuszpietrusinski Aug 23, 2021

Baroshem Aug 24, 2021

Mateuszpietrusinski Aug 23, 2021

Baroshem Aug 24, 2021

Mateuszpietrusinski Aug 23, 2021

Baroshem Aug 24, 2021

Mateuszpietrusinski Aug 23, 2021

Baroshem Aug 24, 2021

Mateuszpietrusinski Aug 23, 2021 •

edited

Loading

Baroshem Aug 24, 2021

Mateuszpietrusinski Aug 23, 2021

Baroshem Aug 24, 2021

Mateuszpietrusinski Aug 23, 2021

Baroshem Aug 24, 2021

Mateuszpietrusinski Aug 25, 2021

Mateuszpietrusinski Aug 23, 2021

Baroshem Aug 24, 2021

michaelbromley left a comment

Baroshem commented Aug 24, 2021


		const { data } = await updateAddressDetails(context, { input: givenVariables.input });

		expect(data).toBe('update shipping details response');

feat: #49/shipping billing #51

feat: #49/shipping billing #51

Conversation

Baroshem commented Aug 20, 2021 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Baroshem commented Aug 22, 2021

Mateuszpietrusinski left a 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

Choose a reason for hiding this comment

Mateuszpietrusinski Aug 23, 2021 • edited Loading

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

michaelbromley left a comment

Choose a reason for hiding this comment

Baroshem commented Aug 24, 2021

Baroshem commented Aug 20, 2021 •

edited

Loading

Mateuszpietrusinski Aug 23, 2021 •

edited

Loading