Add to Wishlist from Cart #3130

revanth0212 · 2021-04-16T19:31:01Z

Description

This PR adds a new feature that lets the user move an item from the cart to the wishlist. This PR only targets single wishlist use cases, for instance, CE backends or EE backends with multiple wishlists turned off.

Related Issue

Closes PWA-1495

Verification Stakeholders

@schensley
@tkacheva
@dpatil-magento

Verification Steps

Go to the deployed version of the app.
Login and add an item to the cart.
Go to the cart page and click on the kabab menu.
Click the Save for later button.
The item should be removed from the cart and should be available in the favorites list.

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.
I have added translations for new strings, if necessary.
I have updated the documentation accordingly, if necessary.

PWAStudioBot · 2021-04-16T19:42:22Z

	Messages
📖	Associated JIRA tickets: PWA-1495.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against ab8c672

revanth0212 · 2021-04-19T17:21:34Z

UX and PM review passed.

revanth0212 · 2021-04-20T15:52:31Z

packages/peregrine/lib/talons/CartPage/ProductListing/useWishlist.ce.js

+                    timeout: 5000
+                });
+
+            await removeItemFromCart({


We have discussed this in the Dev time and decided that we will not be rolling back wishlist addition if remove from the cart fails.

revanth0212 · 2021-04-20T16:01:17Z

packages/venia-ui/lib/components/CartPage/ProductListing/productListingFragments.js

@@ -42,6 +42,7 @@ export const ProductListingFragment = gql`
            ... on ConfigurableCartItem {
                configurable_options {
                    id
+                    configurable_product_option_value_uid


Suggested change

configurable_product_option_value_uid

I guess this is a change I forgot to revert after reverting the rollback code.

It looks like you are still using configurable_product_option_value_uid throughout the code. Not sure what you meant by your comment but...do something? :)

Yes, my bad. Wrong call.

sirugh

Nice work :) I was a little confused why we have a .ee file in here when you're just implementing the single wishlist, but I suppose this is going to help be a jumping off point for the next ticket (add to multiple).

sirugh · 2021-04-22T16:20:44Z

packages/peregrine/lib/talons/CartPage/ProductListing/useProduct.js

+    const [displayError, setDisplayError] = useState(false);
+    const [showLoginToast, setShowLoginToast] = useState(0);


Would prefer a boolean here, but I understand why you use an integer (so the memo fires, and returns a new object, triggering the toast again). I wonder if we have any other instances of a repeated toast in the app, or if this is a new pattern.

I picked this logic from Tommy's #3105

packages/peregrine/lib/talons/CartPage/ProductListing/useWishlist.ce.js

sirugh · 2021-04-22T16:38:01Z

packages/venia-ui/lib/components/CartPage/ProductListing/product.js

@@ -91,10 +101,16 @@ const Product = props => {
              })
            : '';

+    useEffect(() => {
+        if (loginToastProps) {
+            addToast({ ...loginToastProps, icon: InfoIcon });


Is this the new style so that the props are customizable via talon export?

Yeah I kind of like this idea of sending toast props from the talon and let the component only worry about when to show the toast itself.

packages/peregrine/lib/talons/CartPage/ProductListing/useWishlist.ee.js

packages/peregrine/lib/talons/CartPage/ProductListing/useProduct.js

packages/venia-ui/lib/components/CartPage/ProductListing/product.js

sirugh · 2021-04-22T16:53:35Z

packages/venia-ui/i18n/en_US.json

@@ -376,10 +378,10 @@
    "validation.isRequired": "Is required.",
    "validation.mustBeChecked": "Must be checked.",
    "validation.validatePassword": "A password must contain at least 3 of the following: lowercase, uppercase, digits, special characters.",
+    "wishlist.emptyListText": "There are currently no items in this list",
+    "wishlistButton.addedText": "Added to Favorites",


This is nitpicky, sorry, but addedText should come after addText as add comes before added.

Alphabetically they are correct arent they?

I don't think so :P If we sorted purely by string value, then you'd be correct, but afaict we use dictionary order here, and you'd find add before added in the dictionary.

Edit: I was wrong - string value should work here:

Honestly we should just set up a lint rule to auto sort this file for us :D

Prefer standard string-value sorting, please, as it's the least arbitrary.

addError addText addedText

Yeah, the plugin I used is not considering the length of the keys, it is only considering based on the character in the position. Since addEr comes after added it sorted it that way.

sirugh · 2021-04-22T17:05:23Z

packages/peregrine/lib/talons/CartPage/ProductListing/wishlistFragment.ee.js

+    fragment WishlistFragment on Wishlist {
+        id
+        name
+        items: items_v2 {


Generally I would avoid renaming or aliasing responses from gql as it is just another layer of abstraction that could make it more difficult to debug. If something went wrong with items in the response we would have to know that it was aliased from items_v2. Obviously this is a pretty minor thing, and I won't ask you to change it, but I think it's just good to be aware.

In this case I might actually leave it as items_v2 is...rough.

Yeah dude you are right. I am not a fan of renaming usually but I felt items_v2 is not readable in the code.

After discussion in the other PR I think we should move the alias to the destructure so we don't run into a conflict with the existing items schema value.

packages/peregrine/lib/talons/CartPage/ProductListing/useWishlist.ce.js

sirugh · 2021-04-22T17:11:15Z

packages/peregrine/lib/talons/CartPage/ProductListing/useProduct.js

 import configuredVariant from '@magento/peregrine/lib/util/configuredVariant';
+
+import { useWishlist } from './useWishlist';


I feel like having this hook live as a sibling to the talon within the talons directory could be confusing. I know we spoke about this regarding @tjwiebell's use case. Did we end up with a decision on where to place these utility hooks?

Not quite completely though. I am trying to see if I can generalize wishlist logic into a talon of its own so all the components/talons that use it can just import. But unfortunately, that can be tricky because the use cases can be different and Jimmy suggested moving over it if it's getting complex to generalize.

Yea I agree. At a minimum, throw the file into a helpers directory.

If I am able to generalize this, yes I will move it into a helper's folder but if it is bound to stay like this, it's not a helper anymore so we can leave it as it is. I'll update soon about the generalization work and let's make the call then.

revanth0212 · 2021-05-11T16:58:15Z

Hey @supernova-at, thanks for doing the QA on this. I have handled the simple product's scenario. Unfortunately, I could not understand what the error scenario was. Can you expand on that?

dpatil-magento · 2021-05-11T17:03:16Z

@dpatil-magento I'm curious if any of the mftf failures are due to the recent category filters/sort work. Failures 3 and 4 seem like it but I thought MFTF passed on the #3034 PR...

@sirugh Looks like this PR branch is not updated with latest develop that why those are failing.

…o-wishlist

supernova-at · 2021-05-11T18:39:49Z

Unfortunately, I could not understand what the error scenario was. Can you expand on that?

@revanth0212 Sure, it's this one (in the JIRA ticket):

Verify error flow. One way to simulate this scenario is to have 2 tabs open for same customer > Move or remove product in tab 1 and try to move the product on second tab.

supernova-at · 2021-05-11T19:28:43Z

@revanth0212 I updated the QA comment after merging in develop. There is still one MFTF test failure.

dpatil-magento · 2021-05-11T20:06:05Z

@revanth0212 I updated the QA comment after merging in develop. There is still one MFTF test failure.

@supernova-at Those are valid failures as display position in selectors has been changed. You can try running using this branch https://github.com/magento-commerce/pwa-tests/pull/15

BTW to avoid this kind of things we will have data-ids for cypress :)

revanth0212 · 2021-05-12T15:19:49Z

Verify error flow. One way to simulate this scenario is to have 2 tabs open for same customer > Move or remove product in tab 1 and try to move the product on second tab.

Unfortunately, this is a problem with the GQL mutation. Since there is no single mutation to move an item from cart to wishlist, we are using 2 mutations in tandem.

First we add the item to the wishlist, which in this case is working fine.
Second we remove the item from the cart, which in this case is failing because the item is not part of the cart anymore.

I am not sure if we need to solve for this case, because firstly, its an edge case and secondly this wont happen if GQL can provide us with a unified GQL mutation for moving an item from cart to wishlist.

Also, this wont be a problem if the user refreshes the page because the cart will update and the item wont be visible anymore.

Alternatively, if we want to solve this, the ideal solution would be to refresh the cart before performing this action but that would slow the process down by adding a new query into the mix.

…ento/pwa-studio into revanth/add-cart-to-wishlist

supernova-at · 2021-05-13T19:33:26Z

MFTF tests are failing but not because of this PR. Running using a PR in that repo, they all pass.

sirugh

Not sure how to evaluate the recent push - could you help me understand how to repro the error state?

sirugh · 2021-05-13T20:05:54Z

packages/peregrine/lib/talons/CartPage/ProductListing/useProduct.js

+    const handleWishlistUpdateError = useCallback(() => {
+        setIsCartUpdating(true);
+
+        fetchCartDetails({ variables: { cartId } });


Threading this prop all the way down from the cart page feels odd. Could you tell me why this is necessary in the first place and how to repro or get this function to be called? If we do have to go this route, perhaps we can go with a callback like onError that is passed down.

The react state update error I showed earlier today was present even before this change. That said, you can reproduce an error that can call this function by opening the cart in 2 tabs, move an item in 1 cart to the wishlist and do the same on the other tab. Removing items from the cart will fail because the item is not in the cart anymore.

Ok thanks for providing repro.

I do think the handling here is a bit off still - the item greys out, then the success toast pops up, then you see an error on the item for a brief moment and then the item is deleted from the cart page UI.

Edit:

If this is the desired approach (refetching the cart data on error) then I think a callback such as onError defined by the cart page and passed to any children who care, is preferable to a fetchCartDetails prop.

I also think we can remove the setDisplayError(true) call for the product causing the error since it's going to be deleted anyways.

Yeah, it is doggy for sure, but that's the best at the moment till GQL comes up with a unified query. And this is a very very edge case. UX approves this as a valid solution given the circumstances.

I also think we can remove the setDisplayError(true) call for the product causing the error since it's going to be deleted anyways.

Wouldn't that hide legitimate errors as well?

Wouldn't that hide legitimate errors as well?

Would it? Normally you would call setDisplayError(true) in the catch block related to the mutation that fails. Is this what we use this function for?

Perhaps it would hide legitimate errors but I still think the UX is weird. Look at the gif!

I think I understand my confusion about this. In these files we have called setDisplayError in the catch block of the related item call, such as here. In fact, I believe that is the code that is causing the error (failure to remove) to display, not what you added here on line 118.

So please disregard my suggestion to remove the call to setDisplayError. We do need it, in the event that the wishlist mutation throws an error.

I still would like to see my other concern addressed :)

@revanth0212 had reasoning for not making my suggested change, I wanted to post it here so it was captured:

I would prefer it called fetchCartDetails because form the cart page POV we are not using that function for all errors. We are only using that when the wishlist move errors out.

This makes sense to me.

sirugh · 2021-05-13T20:57:50Z

Maybe the state error, at least the one from product listing, is coming from here: https://github.com/magento/pwa-studio/blob/revanth/add-cart-to-wishlist/packages/peregrine/lib/talons/CartPage/ProductListing/useQuantity.js#L94

The backend I'm using got so slow I can't debug this right now, but I was wondering if the problem existed before your PR, such as when you remove the last item from your cart in the cart page (just by deleting, or setting quantity to 0). If it doesn't manifest on develop then there must be some effect being triggered after your "add to wishlist" mutation that is not completing by the time the "remove from cart" mutation completes.

supernova-at · 2021-05-13T22:50:33Z

I still think the UX is weird. Look at the gif!

It is definitely weird but since we're getting a mutation that moves to wishlist from cart all at once, this is an interim weirdness that we made sure to get UX and QA's approval on.

sirugh · 2021-05-14T13:43:10Z

@supernova-at thanks, I didn't have that context! I also realized that Revanth's use of setDisplayError was not actually the cause of the weirdness in the gif - as you said it is due to the dual mutations.

When are we getting a single mutation?

tjwiebell · 2021-05-14T15:18:00Z

@revanth0212 - you mentioned during Office Hour that you were using a single operation for this, and just wanted to clarify what I meant in case it can help with future use cases. No need to re-factor if you've worked through the issues of these mutations being done is separate transactions, but there is one example of this with Configurable Product edits (which happen to be an add + removal).

revanth0212 · 2021-05-14T16:37:02Z

No need to re-factor if you've worked through the issues of these mutations being done is separate transactions, but there is one example of this with Configurable Product edits (which happen to be an add + removal).

This is a wonderful idea @tjwiebell but unfortunately, it did not also solve the problem and also this will be a huge change because the cart fragment resides in venia-ui and the talon is in peregrine. We can move the cart fragments over but I would prefer that to be done in a different PR.

revanth0212 · 2021-05-14T17:45:25Z

@sirugh can you do a code review on the recent changes and @supernova-at can you please do a QA re-run on this?

supernova-at · 2021-05-14T18:45:41Z

When are we getting a single mutation?

Likely not until 2.4.3 in August. I created PWA-1736 to track.

sirugh · 2021-05-14T18:50:35Z

@revanth0212 did you see my previous comment? It may have gotten lost in the discussion about the other issue.

If this is the desired approach (refetching the cart data on error) then I think a callback such as onError defined by the cart page and passed to any children who care, is preferable to a fetchCartDetails prop.

supernova-at · 2021-05-14T19:34:46Z

QA

⏳ In Progress ⏳

MFTF
JIRA ticket test plan
Run Verification Steps
Chrome
Firefox
Safari. I have accidentally upgraded to 14.1 and the whole app fails to load. Also fails on develop and demo instance, so not specific to this PR. May be related to https://bugs.webkit.org/show_bug.cgi?id=225344.
Mobile device

* Updated handler function to add item to wishlist and remove from cart. * Minor. * Roll back functionality added. * Created CE and EE versions. * Minor. * Added login check. * Added toasts. * Sorting lang files. * Removed the rolling back logic. * Minor. * Added loading state. * Added french dictionary items. * chore: log error from mutation (#3140) * Addressed PR comments. * Minor test updates. * Common useWishlist helper hook. (#3141) * Using useWishlist as a helper. * CE wishlist refactor. * EE wishlist refactor. * onWishlistUpdateError -> setDisplayError * Minor. * Minor. * Added initial test. * Added useWishlist.ce tests. * Added useWishlist.ee tests. * Wishlist hook files refactor. * Revert "Wishlist hook files refactor." This reverts commit 34a771c. * Revert "Common useWishlist helper hook. (#3141)" This reverts commit 063144e. * Updated failing tests. * Added more tests. * Removed unecesaary gql. * Added cart page tests. * More tests. * Removed isFavorite * Fix for simple products. * Minor. * Refreshing cart on error. * Minor prop rename. Co-authored-by: Stephen <sirugh@users.noreply.github.com> Co-authored-by: Andy Terranova <13182778+supernova-at@users.noreply.github.com>

revanth0212 added 10 commits April 7, 2021 17:24

Updated handler function to add item to wishlist and remove from cart.

4a67049

Minor.

f418995

Roll back functionality added.

a5fa171

Created CE and EE versions.

fd53eb5

Minor.

fffedb6

Added login check.

bd54449

Added toasts.

ebd89c0

Sorting lang files.

e627307

Removed the rolling back logic.

b4f36e6

Minor.

effc2f2

revanth0212 added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Apr 16, 2021

m2-community-project bot added the Progress: ready for review label Apr 16, 2021

PWAStudioBot added pkg:extensions pkg:peregrine pkg:venia-ui labels Apr 16, 2021

revanth0212 added 2 commits April 16, 2021 16:12

Added loading state.

775a8a4

Added french dictionary items.

cf19090

revanth0212 commented Apr 20, 2021

View reviewed changes

revanth0212 marked this pull request as ready for review April 20, 2021 16:01

sirugh force-pushed the revanth/add-cart-to-wishlist branch from 4786865 to cf19090 Compare April 22, 2021 17:28

chore: log error from mutation (#3140)

8850244

sirugh requested changes Apr 22, 2021

View reviewed changes

m2-community-project bot assigned sirugh Apr 22, 2021

m2-community-project bot added Progress: changes requested and removed Progress: ready for review labels Apr 22, 2021

revanth0212 and others added 2 commits May 11, 2021 12:06

Merge remote-tracking branch 'origin/develop' into revanth/add-cart-t…

a9418de

…o-wishlist

Merge branch 'develop' into revanth/add-cart-to-wishlist

86b3912

revanth0212 and others added 3 commits May 12, 2021 16:41

Refreshing cart on error.

0c77c49

Merge branch 'revanth/add-cart-to-wishlist' of https://github.com/mag…

87be4fc

…ento/pwa-studio into revanth/add-cart-to-wishlist

Merge branch 'develop' into revanth/add-cart-to-wishlist

8a41010

sirugh reviewed May 13, 2021

View reviewed changes

Minor prop rename.

ab8c672

sirugh approved these changes May 14, 2021

View reviewed changes

supernova-at merged commit cee1896 into develop May 14, 2021

m2-community-project bot added Progress: done and removed Progress: review labels May 14, 2021

michaelyu0123 deleted the revanth/add-cart-to-wishlist branch February 1, 2022 20:49

		const [displayError, setDisplayError] = useState(false);
		const [showLoginToast, setShowLoginToast] = useState(0);

		import configuredVariant from '@magento/peregrine/lib/util/configuredVariant';

		import { useWishlist } from './useWishlist';

Add to Wishlist from Cart #3130

Add to Wishlist from Cart #3130

Conversation

revanth0212 commented Apr 16, 2021

Description

Related Issue

Verification Stakeholders

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Apr 16, 2021 • edited Loading

revanth0212 commented Apr 19, 2021

revanth0212 Apr 20, 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

sirugh 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

sirugh Apr 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

revanth0212 Apr 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

revanth0212 commented May 11, 2021

dpatil-magento commented May 11, 2021 • edited Loading

supernova-at commented May 11, 2021 • edited Loading

supernova-at commented May 11, 2021

dpatil-magento commented May 11, 2021 • edited Loading

revanth0212 commented May 12, 2021 • edited Loading

supernova-at commented May 13, 2021

sirugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh May 13, 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

sirugh May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh commented May 13, 2021 • edited Loading

supernova-at commented May 13, 2021 • edited Loading

sirugh commented May 14, 2021

tjwiebell commented May 14, 2021

revanth0212 commented May 14, 2021

revanth0212 commented May 14, 2021

supernova-at commented May 14, 2021

sirugh commented May 14, 2021

supernova-at commented May 14, 2021 • edited Loading

QA

PWAStudioBot commented Apr 16, 2021 •

edited

Loading

revanth0212 Apr 20, 2021 •

edited

Loading

sirugh Apr 23, 2021 •

edited

Loading

revanth0212 Apr 23, 2021 •

edited

Loading

dpatil-magento commented May 11, 2021 •

edited

Loading

supernova-at commented May 11, 2021 •

edited

Loading

dpatil-magento commented May 11, 2021 •

edited

Loading

revanth0212 commented May 12, 2021 •

edited

Loading

sirugh May 13, 2021 •

edited

Loading

sirugh May 14, 2021 •

edited

Loading

sirugh commented May 13, 2021 •

edited

Loading

supernova-at commented May 13, 2021 •

edited

Loading

supernova-at commented May 14, 2021 •

edited

Loading