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

Make display_price optional on admin variants list #4213

Conversation

luca-landa
Copy link
Contributor

@luca-landa luca-landa commented Nov 25, 2021

When there are variants which respond nil as #default_price (e.g. because admin is creating them and they still don't have all prices set up, or they don't have a price matching Spree::Variant.default_price_attributes), the /admin/products/<product>/variants page returns 500 and it's not obvious why to the admin.

This small fix allows for the variants list page to be displayed anyway.

Description

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

Thanks, @luca-landa! Could we have a test for that to avoid possible future regressions?

@luca-landa luca-landa force-pushed the luca-landa/fix-variants-list-in-admin-with-not-fully-configured-prices branch 2 times, most recently from c0b54b0 to 41c9017 Compare November 29, 2021 10:05
When there are variants with not fully configured prices (e.g. because
they are being created) the product variants page breaks without the
admin really knowing why.
This allows for the variants list to be displayed anyway.
@luca-landa luca-landa force-pushed the luca-landa/fix-variants-list-in-admin-with-not-fully-configured-prices branch from 41c9017 to c2d4424 Compare November 29, 2021 10:07
@luca-landa
Copy link
Contributor Author

Thanks, @luca-landa! Could we have a test for that to avoid possible future regressions?

@waiting-for-dev added! I wasn't sure if I should have created a factory for the variant needed in this test, being a unique use case, let me know if you think I should change something 🙏

@waiting-for-dev
Copy link
Contributor

@waiting-for-dev added! I wasn't sure if I should have created a factory for the variant needed in this test, being a unique use case, let me know if you think I should change something

Thanks, @luca-landa! I think there's no need for a custom factory 🙂

@waiting-for-dev waiting-for-dev merged commit e363fe3 into solidusio:master Dec 2, 2021
@tvdeyen
Copy link
Member

tvdeyen commented Oct 13, 2022

This should be backported to v3.1 and v3.0 as well.

@tvdeyen tvdeyen self-assigned this Oct 13, 2022
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.

4 participants