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

fix: allow customer_id in UpdateCart #9387

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

VariableVic
Copy link
Contributor

Allows customer_id to be present when updating a cart.

Copy link

changeset-bot bot commented Sep 30, 2024

⚠️ No Changeset found

Latest commit: e383f7a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Sep 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medusa-dashboard ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2024 11:53am
6 Skipped Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Oct 2, 2024 11:53am
api-reference-v2 ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 11:53am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 11:53am
docs-v2 ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 11:53am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 11:53am
resources-docs ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 11:53am

Copy link
Collaborator

@srindom srindom left a comment

Choose a reason for hiding this comment

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

I'd expect to see:

  1. A test
  2. Some considerations around edge cases - e.g. update has both customer_id and email. Which one dictates the customer on the cart?

assume this is for the b2b starter - is it maybe better to add a custom endpoint?

@olivermrbl
Copy link
Contributor

@VariableVic, can I get you to address the comments, so we can move forward with this?

@VariableVic
Copy link
Contributor Author

VariableVic commented Oct 2, 2024

@srindom

The issue is that carts are not properly linked to existing customers with an account because we can't pass in a customer_id for logged in customers. Instead, a new anonymous customer is created with the same email, and linked to the cart.

AFAICT the workflow expects a customer_id to be present for customers with an account. When passing in an email, the workflow will only look for customers that don't have an account (has_account: false)

update has both customer_id and email.

This is taken care of in findOrCreateCustomerStep - the workflow accounts for receiving both email and customer id.

A test

I have no experience writing these tests. Maybe @riqwan can help out?

I don't think this is a B2B starter specific issue, that's why we decided to open the PR.

@riqwan
Copy link
Contributor

riqwan commented Oct 2, 2024

Whats the expected outcome here?

If the customer_id is passed to the workflow, we always prioritize the email from the customer record and ignore any email coming into the api. Is this missing just a test?

Only case I can think of is when you run 2 requests:

  1. Update cart with customer_id
  2. Update cart with email

In this case ^ do we update the customer's email, but keep the customer_id? and vice versa, do we override the email?

Alternatively, we remove customer_id from the storefront API and then pass customer_id to all cart workflows when the customer is logged in. i.e to handle the case of when the user goes about adding items to cart and then logging in after.

@VariableVic
Copy link
Contributor Author

VariableVic commented Oct 3, 2024

The expected outcome is that we can update a cart for a logged in customer. This is currently impossible.

I think that when a customer is logged in, the customer_id and email from the customer record should always be used on the cart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants