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(medusa): discounts service create validates against at-least 1 region #2729

Merged
merged 10 commits into from
Dec 8, 2022

Conversation

riqwan
Copy link
Contributor

@riqwan riqwan commented Dec 6, 2022

What:

  • introduces regions validation on creating a discount
    • you can only create a discount with at-least 1 valid region ID

Why:

A discount is validated against a region while being added to a cart. Any discount that is applied therefore needs to be associated with a valid region.

How:

As a first step, we introduce a validation into the discount create service. With this PR, discounts will always be associated with a region on creation of a discount.

There are some cases we need to handle:

  • When a region is deleted, what happens to the discount?
  • When a discount is updated with an empty regions array, the discount ends up in a state with no regions.
  • What do we do about existing discounts that have no region?

Testing:

Unit tests:

  • Add valid regions to all existing unit tests
  • Unsuccessful creation with invalid regions

Integration tests:

  • Add valid regions to all existing integration tests
  • Fails to create a discount if the regions contains an invalid regionId
  • Fails to create a discount if regions are not present

FIXES CORE-873

@changeset-bot
Copy link

changeset-bot bot commented Dec 6, 2022

🦋 Changeset detected

Latest commit: dbe2524

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

This PR includes changesets to release 1 package
Name Type
@medusajs/medusa 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

Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

Very good, only minor comments 🚀

integration-tests/api/__tests__/admin/discount.js Outdated Show resolved Hide resolved
integration-tests/api/__tests__/admin/discount.js Outdated Show resolved Hide resolved
integration-tests/api/__tests__/admin/discount.js Outdated Show resolved Hide resolved
integration-tests/api/__tests__/admin/discount.js Outdated Show resolved Hide resolved
integration-tests/api/__tests__/admin/discount.js Outdated Show resolved Hide resolved
integration-tests/api/__tests__/admin/discount.js Outdated Show resolved Hide resolved
@adrien2p
Copy link
Member

adrien2p commented Dec 6, 2022

@riqwan can I get you to add a description on the PR and link the linear issue the way I sent you :)

@riqwan riqwan force-pushed the fix/discount-regions-validation branch 2 times, most recently from b156e44 to aac7794 Compare December 6, 2022 16:10
@riqwan riqwan marked this pull request as ready for review December 6, 2022 16:11
@riqwan riqwan requested a review from a team as a code owner December 6, 2022 16:11
@riqwan riqwan requested a review from adrien2p December 6, 2022 16:11
Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

One last comment and one thing to look at. We should probably update the service method that creates the discounts to enforce at least one. At the moment it accepts optional regions properties but it might be wrong too. Could you check please?

@riqwan riqwan requested a review from adrien2p December 7, 2022 10:06
Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

LGTM, few minor comments left and I think we are good to go ahah @olivermrbl do you want to run a review?

integration-tests/api/__tests__/admin/discount.js Outdated Show resolved Hide resolved
integration-tests/api/__tests__/admin/discount.js Outdated Show resolved Hide resolved
integration-tests/api/__tests__/admin/discount.js Outdated Show resolved Hide resolved
packages/medusa/src/services/__tests__/discount.js Outdated Show resolved Hide resolved
packages/medusa/src/helpers/test-request.js Outdated Show resolved Hide resolved
packages/medusa/src/services/discount.ts Outdated Show resolved Hide resolved
@riqwan riqwan requested a review from adrien2p December 7, 2022 15:05
Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 one last comment ahah

A discount should always be associated to atleast 1 region - we validate it against the region in the cart service.

Adding a request parameter validation to pass in atleast one regionID in an array of regions will ensure that regionID will be validated as a part of the discount service and fail if its not a valid region ID.
@adrien2p adrien2p force-pushed the fix/discount-regions-validation branch from 4bd126d to c1cdb80 Compare December 7, 2022 15:42
@riqwan riqwan force-pushed the fix/discount-regions-validation branch from 2577562 to 5240972 Compare December 7, 2022 16:04
Copy link
Member

@adrien2p adrien2p left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 very nice boy scoot session included 💪

@adrien2p
Copy link
Member

adrien2p commented Dec 7, 2022

@riqwan I suggest to also wait the approval from @olivermrbl to have a second pair of eyes

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.

Really solid work @riqwan! Don't have anything to add 🥳

When a discount is updated with an empty regions array, the discount ends up in a state with no regions.

Ideally, we would add a check similar to this PR. Not super urgent, but feel free to create a Linear issue and tackle that too 💪

What do we do about existing discounts that have no region?

I don't think we have to do anything. Worst case, the invalid discounts throw a 400 + sends an appropriate error message when applied to carts.

@riqwan riqwan merged commit 34e405c into develop Dec 8, 2022
@riqwan riqwan deleted the fix/discount-regions-validation branch December 8, 2022 10:34
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.

3 participants