Skip to content

Conversation

@herzog31
Copy link
Member

@herzog31 herzog31 commented Mar 27, 2020

Description

  • Add support for virtual products in Product and ProductTeaser components.
    • Use addVirtualProductsToCart mutation to add virtual products to cart.
  • Update cart to hide shipping address and method for fully virtual carts (carts that only contain virtual products).
  • Fix bug where the street of the billing address was not properly stored.

How Has This Been Tested?

  • Updated unit tests
  • Manual test with registered and guest user

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:

  • I have signed the Adobe Open Source CLA.
  • 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 and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

Add the customer cart and the merge carts related queries
Update the cart initialized to read the cart data.
The hook that triggered the cart merging had some unwanted dependencies (e.g. `cartId` ) that we don't really need so I removed those.
Remove console statements.
- get rid of all the `useEffect` hooks that were making the code more readable (and less error-prone)
- add a `useAwaitQuery` hook so that we can invoke graphql queries using asyn/await syntax
- save the customer's cart id in the state (will be merged down in the CartContext if needed)
… context.

Update unit tests
Remove an old "cart initializer" script
…tching the same cart in a sign-out / sign-in again scenario.
- don't pass the `Authorizatio` header if the token is empty
- `useReceipt`: call the `continueShopping` function directly instead of asynchronously
- `useOverview`: update the state in a `finally` block.
Copy link
Contributor

@cjelger cjelger left a comment

Choose a reason for hiding this comment

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

Looks good: the only open question is whether we want to support adding multiple products of different types at the same time, say for example a grouped product composed of one virtual product and one "normal" product. WDYT?

@@ -1,5 +1,6 @@
query cartDetails($cartId: String!) {
cart(cart_id: $cartId) {
is_virtual
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this true only when all the cart products are virtual?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. As soon as one physical item is in the cart, this property is false.

dispatch({ type: 'beginLoading' });
return addItem({

let addItemFunc = ev.detail.virtual ? addVirtualItem : addItem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, shouldn't we have that virtual attribute per item added to the cart? What if we have a grouped product composed of two virtual products and one normal product? Not sure if this is realistic, but it would be better to support that, WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will try if a grouped product can be composed of virtual and physical products.

We would need to split the query then however into addSimpleProductsToCart and addVirtualProductsToCart mutations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this works OOTB in Magento, but I've seen countless of "Guitar, amp and 1yr subscription to online course" offers, so it's a valid business case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created a follow up JIRA for this and will work on it after the release :)

dispatch({ type: 'beginLoading' });
return addItem({

let addItemFunc = ev.detail.virtual ? addVirtualItem : addItem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this works OOTB in Magento, but I've seen countless of "Guitar, amp and 1yr subscription to online course" offers, so it's a valid business case.

@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #241 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #241   +/-   ##
=======================================
  Coverage   48.02%   48.02%           
=======================================
  Files         105      105           
  Lines        3096     3096           
  Branches      595      595           
=======================================
  Hits         1487     1487           
  Misses       1609     1609           
Flag Coverage Δ
#jest 39.75% <0.00%> (ø)
#karma 94.83% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a15813b...a15813b. Read the comment docs.

@dplaton dplaton merged commit 616697c into master Apr 1, 2020
@delete-merged-branch delete-merged-branch bot deleted the issue/CIF-1317 branch April 1, 2020 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request To Verify

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants