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

feat(medusa): Add metadata to StorePostCartsCartLineItemsItemReq #4230

Conversation

erikengervall
Copy link
Contributor

@erikengervall erikengervall commented Jun 1, 2023

This is my first contribution.

I tried my best to follow the available documentation on how to contribute -- LMK if there's anything missing 🙏🏼

What - what changes are in this PR

This PR adds the Line Item metadata field to the StorePostCartsCartLineItemsItemReq request.

If omitted, no changes are applied.

Why - why are these changes relevant

I created a feature request very recently and figured I'd take a stab at creating a PR myself. Here's a quote of the particular use case:

We've got a use case where we'd like to enrich line items' metadata field just before entering the checkout flow. Without the addition of metadata to StorePostCartsCartLineItemsItemReq, the options I see are

  • Creating a custom endpoint that's almost identical to StorePostCartsCartLineItemsItemReq but that accepts metadata
    • Although this would work just fine in the short term, we would effectively opt out of future updates to this endpoint, which I would prefer not to do.
  • Call deleteItem followed by addItem as addItem accepts metadata.
    • This is quite hacky and it also causes the UI to render the Empty Cart page for a brief moment.

How - how have the changes been implemented

Not sure what this refers to.

The changes were developed locally and pushed to a new branch on my fork.

Testing - how has the changes been tested or how can the reviewer test the feature

Two additional unit tests have been created to cover this particular functionality.

@erikengervall erikengervall requested a review from a team as a code owner June 1, 2023 09:57
@vercel
Copy link

vercel bot commented Jun 1, 2023

Someone is attempting to deploy a commit to the medusajs Team on Vercel.

A member of the Team first needs to authorize it.

@changeset-bot
Copy link

changeset-bot bot commented Jun 1, 2023

🦋 Changeset detected

Latest commit: bf56127

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@medusajs/medusa Patch
@medusajs/client-types Patch

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented Jun 28, 2023

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

Name Status Preview Comments Updated (UTC)
medusa-docs 🔄 Building (Inspect) Jun 28, 2023 6:21am

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @erikengervall. I have a minor suggestion. Let me know what you think :)

@@ -89,7 +89,7 @@ export default async (req, res) => {
variant_id: existing.variant.id,
region_id: cart.region_id,
quantity: validated.quantity,
metadata: existing.metadata || {},
metadata: validated.metadata || existing.metadata || {},
Copy link
Contributor

Choose a reason for hiding this comment

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

todo: I believe we can do validated.metadata || {} and let our dedicated update metadata util take care of this. The new and existing metadata will be merged correctly in the LineItemService.update method.

Would be good with a test to verify

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merging sounds even better 👍🏼

Feel free to make the change, if not I’ll have a look tomorrow (it’s late here) 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

All good - I'll push the changes so we can include it in today's release :)

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

But otherwise LGTM, feel free to merge as you please :)

@olivermrbl olivermrbl requested a review from a team as a code owner July 19, 2023 13:00
@olivermrbl olivermrbl force-pushed the feat/add-metadata-to-update-line-item-endpoint branch from 5a6c2fd to bf56127 Compare July 19, 2023 13:23
@olivermrbl olivermrbl merged commit 2f28399 into medusajs:develop Jul 19, 2023
@github-actions github-actions bot mentioned this pull request Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants