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

Solidus 3 preparation #55

Merged
merged 15 commits into from
Aug 30, 2021

Conversation

cpfergus1
Copy link

This prepares the extension to work with current supported versions of Solidus.
This PR satisfies #51

Discussion points:
Use of conditionals and if parameters to satisfy versioning requirements.
Other possible parameters to use for SoftDelete checks than Solidus Version

app/controllers/spree/admin/sale_prices_controller.rb Outdated Show resolved Hide resolved
app/models/spree/sale_price.rb Outdated Show resolved Hide resolved
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Maybe there's the opportunity of removing the conditional code for soft-deletion, I left a comment about it.

app/helpers/solidus_sale_prices/soft_delete.rb Outdated Show resolved Hide resolved
@cpfergus1 cpfergus1 force-pushed the solidus-3-preparation branch from 4b3373b to d92e20f Compare July 6, 2021 19:15
Copy link
Author

@cpfergus1 cpfergus1 left a comment

Choose a reason for hiding this comment

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

I opted for implementing soft delete functionality for 2.10. Please let me know if you agree or if you believe we should go another direction

@cpfergus1 cpfergus1 force-pushed the solidus-3-preparation branch 6 times, most recently from b584b0e to 9bc979c Compare July 7, 2021 12:33
@kennyadsl
Copy link
Member

@jarednorman time for a second pass?

Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

Overall approach seems good to me. This could use some rebase love to get rid of some churn and things committed in the wrong commits to keep the history sensible.

.byebug_history Outdated Show resolved Hide resolved
Gemfile Show resolved Hide resolved
app/models/spree/sale_price.rb Outdated Show resolved Hide resolved
lib/solidus_sale_prices/engine.rb Show resolved Hide resolved
spec/features/admin/sale_prices_spec.rb Outdated Show resolved Hide resolved
app/helpers/solidus_sale_prices/soft_delete.rb Outdated Show resolved Hide resolved
spec/models/variant_spec.rb Outdated Show resolved Hide resolved
spec/models/sale_price_spec.rb Show resolved Hide resolved
@cpfergus1 cpfergus1 force-pushed the solidus-3-preparation branch from 8d7306f to add4554 Compare July 21, 2021 12:45
SamuelMartini and others added 10 commits July 21, 2021 07:02
From Rails 5.2 is not possible to pass a raw string to order.
Once 2.10 is no longer supported, act as paranoid will be removed and
destroy will no longer work as it previously did.
…ity gap

This is required to merge the functionality of price_in and price_for_options.
The functionality of finding prices changes in solidus 2.11, 3.0 and master
so this function serves to work for all three for these scenarios
In Solidus 3.1, the test factory no longer appends the prices to the variant
without the object being reloaded. Implemented reload to assist specs to
pass.
@cpfergus1 cpfergus1 force-pushed the solidus-3-preparation branch from add4554 to 8422bfd Compare July 21, 2021 13:05
The sales_price is made from an price associated with the variant. To verify
that the sales_price recieved the properties of the original price,
you would want to test against the original price properties. In 3.1,
this test failed due to the variant having more than one price, a master
price and a test set price. The previous behavior would ask for the last
price, `variant.prices.last.currency`, assuming a new price was generated
when it was actually pulling the master variant price which was in "USD".
In tests for 3.0 and below, the first price is also the last price,
which is why it never failed in the first place. Swapped to literal to
avoid the error.
This commit reflects changes made by Solidus [PR #169]
(solidusio/solidus_dev_support#169) When testing extensions
using Solidus vesions that support the definition_file_paths,
it will load the core factory first and then the ones defined in
`lib/extension_name/testing_support/factories`.
Test was failing within a couple ten thousandths of a second.
Increased by one second to accommodate float.
@cpfergus1 cpfergus1 force-pushed the solidus-3-preparation branch from 8422bfd to d5f72fc Compare July 21, 2021 13:13
@cpfergus1
Copy link
Author

Thank you for your corrections @kennyadsl and @jarednorman. All current topics have been remedied and pushed. I also removed two older commits that were obsolete and cleaned up the history a little bit to be more explicit. Please let me know if I left anything out!

The bug that this code fixed has been patched in the the latest versions
of bundler and is no longer required.
@cpfergus1 cpfergus1 force-pushed the solidus-3-preparation branch from d5f72fc to b8033cd Compare July 21, 2021 14:42
Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

I think this looks all good to me.

@kennyadsl kennyadsl merged commit 9bf5ab9 into solidusio-contrib:master Aug 30, 2021
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