WIP Extract legacy promotions #5634
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
changelog:solidus_api
mamhoff
force-pushed
the
extract-legacy-promotions
branch
2 times, most recently
from
bcaff25
to
be37df1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5634 +/- ##
==========================================
- Coverage 88.87% 88.81% -0.07%
==========================================
Files 699 705 +6
Lines 16607 16660 +53
==========================================
+ Hits 14760 14797 +37
- Misses 1847 1863 +16 ☔ View full report in Codecov by Sentry. |
mamhoff
added a commit
to mamhoff/solidus
that referenced
this pull request
Jan 29, 2024
We want to be able to move all promotion-related things out of `solidus_core` in PR solidusio#5634. However, Spree::AppConfiguration also does all the promotion-specific things, and we can't move the app configuration out of core. So what this does is it introduces a new configuration object containing those attributes of the core app_configuration class and moves them into a `promotion_configuration` object. This object is now available as Spree::Promotion::Config.
|
Depends on #5635 |
|
Thanks for your work on this @mamhoff. Now to find time to actually review it properly... 😅 |
tvdeyen
reviewed
Feb 2, 2024
This was referenced Feb 3, 2024
mamhoff
added a commit
to mamhoff/solidus
that referenced
this pull request
Feb 17, 2024
…ration We want to be able to move all promotion-related things out of `solidus_core` in PR solidusio#5634. However, Spree::AppConfiguration also does all the promotion-specific things, and we can't move the app configuration out of core. In solidusio#5658, we've added a promotion configuration object as a nucleus for core's promotion system's configuration, `Spree::Core::PromotionConfiguration`. This implements all the promotion-specific configuration preferences from `Spree::AppConfiguration` there.
mamhoff
added a commit
to mamhoff/solidus
that referenced
this pull request
Feb 18, 2024
…ration We want to be able to move all promotion-related things out of `solidus_core` in PR solidusio#5634. However, Spree::AppConfiguration also does all the promotion-specific things, and we can't move the app configuration out of core. In solidusio#5658, we've added a promotion configuration object as a nucleus for core's promotion system's configuration, `Spree::Core::PromotionConfiguration`. This implements all the promotion-specific configuration preferences from `Spree::AppConfiguration` there.
spaghetticode
pushed a commit
to nebulab/solidus
that referenced
this pull request
Feb 26, 2024
…ration We want to be able to move all promotion-related things out of `solidus_core` in PR solidusio#5634. However, Spree::AppConfiguration also does all the promotion-specific things, and we can't move the app configuration out of core. In solidusio#5658, we've added a promotion configuration object as a nucleus for core's promotion system's configuration, `Spree::Core::PromotionConfiguration`. This implements all the promotion-specific configuration preferences from `Spree::AppConfiguration` there.
mamhoff
force-pushed
the
extract-legacy-promotions
branch
2 times, most recently
from
f57fc58
to
ba90efe
Compare
tvdeyen
reviewed
Feb 27, 2024
legacy_promotions/db/migrate/20231031175215_add_promotion_order_promotion_foreign_key.rb
Outdated
Show resolved
Hide resolved
legacy_promotions/spec/mailers/spree/promotion_code_batch_mailer_spec.rb
Outdated
Show resolved
Hide resolved
legacy_promotions/db/migrate/20160101010001_solidus_one_four_promotions.rb
Outdated
Show resolved
Hide resolved
legacy_promotions/db/migrate/20161017102621_create_spree_promotion_code_batch.rb
Outdated
Show resolved
Hide resolved
legacy_promotions/app/decorators/solidus_legacy_promotions/models/spree/order_decorator.rb
Show resolved
Hide resolved
mamhoff
force-pushed
the
extract-legacy-promotions
branch
2 times, most recently
from
14d959e
to
679f6b6
Compare
This was referenced Feb 28, 2024
mamhoff
force-pushed
the
extract-legacy-promotions
branch
2 times, most recently
from
8b1a5f9
to
4525f8b
Compare
Spree::OrderContents does promotion-related things and shouldn't. To keep the constant, I remove the parts that do promotion-related things and rename the class, and reintroduce it it solidus_legacy_promotions.
In legacy_promotions, we'll just keep them in `app`, where they're tested, too.
The one example here relies on the legacy promo system.
Spree::Calculator::FlatRate is gone, but luckily, we can just use the shipping equivalent.
We don't need #recalculate for anything in core, and we don´t need the association to promotion codes.
We don't actually have any promotion code in core left, so we need to be using the Null promotion configuration.
I'm keeping the spec to test what the model does, but moving the extended spec with the actual promotion to solidus_legacy_promotions.
With a properly configured promotion system, we don't need a special "shipping" promotion handler. This moves the shipping-specific promotion handling into the solidus_legacy_promotions gem.
We would rather keep `Spree::Order#ensure_promotions_eligible` in core, as what it does is re-run the promotion adjuster class and make sure the amounts are still correct. This should be fine for every promotion system. However, since the Null adjuster doesn't actually do anything, we need to stub what it does in the spec.
This should probably in the future be implemented in an Omnes hook.
This method isn't called anywhere in core, so we're not creating some kidn of configuration endpoint. It's also worthy of deprecation or fixing, as it will return true for canceled orders.
`Spree::PromotionAction` is not in core any longer, and the null promo adjuster can not actually produce any adjustments.
These make references to the old classes and cannot stay in core.
In the future, the admin views for legacy promotions will live in the legacy_promotions gem.
In the future, the admin views for legacy promotions will live in the solidus_legacy_promotions gems. This is just to see if we get a green build.
mamhoff
force-pushed
the
extract-legacy-promotions
branch
from
5bebaa5
to
ab26519
Compare
github-actions
bot
removed
the
changelog:solidus_api
This was referenced May 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This extracts the legacy promotion system into a gem that lives within the solidus codebase. I've talked about this with @kennyadsl as a way to integrate a new solidus_promotions gem later, modeled after solidus_friendly_promotions.
This is still Work in progress, and comments are welcome.
Checklist
The following are mandatory for all PRs:
The following are not always needed: