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: Fixes to product module and improving tests #6898

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

sradevski
Copy link
Member

No description provided.

Copy link

changeset-bot bot commented Apr 1, 2024

⚠️ No Changeset found

Latest commit: a148ad8

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 Apr 1, 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 Apr 2, 2024 6:59am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
api-reference ⬜️ Ignored (Inspect) Apr 2, 2024 6:59am
docs-ui ⬜️ Ignored (Inspect) Visit Preview Apr 2, 2024 6:59am
medusa-docs ⬜️ Ignored (Inspect) Visit Preview Apr 2, 2024 6:59am

@@ -2585,102 +2546,6 @@ medusaIntegrationTestRunner({
expect(variantPost).not.toBeTruthy()
})

// TODO: This one is a bit more complex, leaving for later
it.skip("successfully deletes a product variant and its associated option values", async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is testing implementation detail of the module, and as such I moved the tests to the module (there already was a test for the product, I just added one for the variant)

@@ -2008,7 +2006,8 @@ medusaIntegrationTestRunner({
})
})

it("successfully updates a variant's price by changing an existing price (given a region_id)", async () => {
// TODO: Do we want to add support for region prices through the product APIs?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think we should keep this. On the product details page, we allow you to update region and currency prices for variants in the bulk editor.

That said, I would be fine starting with the currency prices, so this doesn't block the end-to-end flows.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I'll tackle it once we have a full end-to-end setup in place 👍

@@ -15,6 +15,7 @@ export const updateProductsWorkflow = createWorkflow(
(
input: WorkflowData<WorkflowInput>
): WorkflowData<ProductTypes.ProductDTO[]> => {
// TODO: Delete price sets for removed variants
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Wouldn't the cascade on the link definition take care of this? At least I would expect it to.

Copy link
Member

@adrien2p adrien2p Apr 1, 2024

Choose a reason for hiding this comment

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

Theoretically, when calling remoteLink.delete, it will call the module service softDelete method, to cascade the deletion, the soft delete will return the deleted entries deeply mapped with the linkable keys and allow to perform the cascade soft delete to all the module services.

The same should apply for restoring.

That way, if soft deleting a variant is cascading to other entities that have a linkable key, we can also perform the cascade for those entities that was link to something else.

To see more, you can look at the executeCascade on the remote link.

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 think the only issue is that we currently don't do a soft-delete in the upsertWithReplace, and at this point we don't handle the deleted variants in any way. In the deleteVariants workflow this is already handled. Since this doesn't block end-to-end flow for now I decided not to do it for now.

/**
* When the product variant option was deleted.
*/
deleted_at?: string | Date
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
deleted_at?: string | Date
deleted_at: string | Date | null

@kodiakhq kodiakhq bot merged commit b3ce13d into develop Apr 2, 2024
24 checks passed
@kodiakhq kodiakhq bot deleted the fix/small-fixes-test-improvements-product branch April 2, 2024 07:13
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.

None yet

3 participants