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(core): expose enabled field for product in shop api #2541

Merged

Conversation

alexisvigoureux
Copy link
Contributor

Description

Expose enabled field for product in shop api

Breaking changes

Screenshots

Capture d’écran 2023-11-23 à 13 58 18

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

⚡ Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

Copy link

netlify bot commented Nov 23, 2023

Deploy Preview for effervescent-donut-4977b2 ready!

Name Link
🔨 Latest commit 71774e2
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/657c54f5d4f8880008371dcd
😎 Deploy Preview https://deploy-preview-2541--effervescent-donut-4977b2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@alexisvigoureux alexisvigoureux marked this pull request as ready for review November 23, 2023 13:15
@michaelbromley
Copy link
Member

Hi!

Can you explain the use-case for this? The "enabled" toggle currently means that the given product is not available in the Shop API when set to true. So by definition, if a Product can be seen in the Shop API, it must be enabled.

@alexisvigoureux
Copy link
Contributor Author

alexisvigoureux commented Nov 23, 2023

Hi, here is our use case:

  • The product A is available on the shop API.
  • A user adds it to the cart but does not go as far as paying for the order
  • We deactivate the product in the back office for any reason
  • The user come back few days/hours later to finalize the order
  • We want to display a message that the product is no longer available <- the use case based on the « enabled » field of the product. We don't want to delete the product from the cart because it's not great for the UX. The user would not go through the search query but directly from the activeOrder query

Another option: We could disable all the variants when we deactivate a product but that would complicate reactivation (Were all the variants enabled?). This is an option because the productVariant enabled field is available

@michaelbromley
Copy link
Member

ah ok I see. So this change would not alter the actual ability to access disabled variants at all. It only is relevant in that case where the variant is related to an existing OrderLine.

Looking at the code, I see that currently we would return that OrderLine.productVariant regardless of the enabled setting (and even if it has been soft-deleted). So in that case yes I see that the enabled field gives useful information.

Another option: We could disable all the variants when we deactivate a product

I'm not understanding how this would solve it - the disabled variants would still be returned with orderLine.productVariant even if disabled right?

@alexisvigoureux
Copy link
Contributor Author

ah ok I see. So this change would not alter the actual ability to access disabled variants at all. It only is relevant in that case where the variant is related to an existing OrderLine.

Exactly, it wouldn't change the way things work now. It's fine to return the variant regardless of the enabled setting. We just need to be able to access the enabled setting of the product as well as that of the variant, it's a feature rather than a fix.

I'm not understanding how this would solve it - the disabled variants would still be returned with orderLine.productVariant even if disabled right?

I was proposing another solution, which is to listen to the change of state of the enabled setting of the product to deactivate all the variants, but this is more complex and can lead to side effects.

@alexisvigoureux
Copy link
Contributor Author

Did you need any other information?
The enabled field was already exposed in the admin-api, so we can expose it in the shop-api to access it from the orderLine

@michaelbromley
Copy link
Member

Hi,

No I think we're good to include this. Can you just make the PR against the minor branch though because it's a new feature & API change. I've started publishing pre-releases for the next minor so you'll be able to use it before the final v2.2 release.

@alexisvigoureux alexisvigoureux changed the base branch from master to minor December 15, 2023 13:30
@alexisvigoureux
Copy link
Contributor Author

Hi, perfect, it's moved against the minor branch

@michaelbromley michaelbromley merged commit f6f2975 into vendure-ecommerce:minor Dec 15, 2023
16 of 18 checks passed
@alexisvigoureux alexisvigoureux deleted the expose-prodcut-enabled branch December 15, 2023 16:13
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