Move solidus admin promotion to legacy promotions #5724
Merged
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:repository
mamhoff
force-pushed
the
move-solidus-admin-promotion-to-legacy-promotions
branch
from
068e8ce to
d147ed0
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5724 +/- ##
=======================================
Coverage 88.84% 88.84%
=======================================
Files 704 704
Lines 16757 16761 +4
=======================================
+ Hits 14887 14891 +4
Misses 1870 1870 ☔ View full report in Codecov by Sentry. |
tvdeyen
approved these changes
Apr 17, 2024
kennyadsl
reviewed
Apr 17, 2024
kennyadsl
approved these changes
Apr 17, 2024
mamhoff
force-pushed
the
move-solidus-admin-promotion-to-legacy-promotions
branch
from
dc106b3 to
92e00c4
Compare
|
Hey @mamhoff can you please check the failing specs? I think it's something related. Thanks! |
mamhoff
force-pushed
the
move-solidus-admin-promotion-to-legacy-promotions
branch
3 times, most recently
from
f042cb6 to
bb974ae
Compare
For now, this introduces a hard dependency from legacy_promotions to the new admin.
We need a way of a spec telling the app to use the new admin for tests that require it, and using the old admin to not use it. After experimenting with using a cookie, which led to a number of undesirable issues (needing a new gem to manage cookies across Capybara drivers, having to visit some URL before being able to even set the cookie etc), what we do is we just define a global module attribute accessor on the dummy app that enables or disables the routes for the `solidus_admin` gem.
mamhoff
force-pushed
the
move-solidus-admin-promotion-to-legacy-promotions
branch
from
bb974ae to
3bffe16
Compare
This drops the dependency between solidus_legacy_promotions and solidus_admin. If `solidus_legacy_promotions` is not defined, the admin will not have a promotion configuration. If `solidus_admin` is not defined, the routes and controllers necessary for the new admin won't be loaded.
This naming is better since two directories, `admin` and `solidus_admin` are really confusing, and naming them after the gems they use is more appropriate.
mamhoff
force-pushed
the
move-solidus-admin-promotion-to-legacy-promotions
branch
from
3bffe16 to
bce5114
Compare
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
We don't want any of the core gems to depend on
solidus_legacy_promotions. This PR removes the dependency fromsolidus_admintosolidus_legacy_promotions.This was pretty hard to do, because there's not a lot of support for differentiating between the two types of admin from an RSpec perspective. What we ended up doing was define a global switch for the DummyApp. If anyone is aware of something like
RSpec.current_test.metadata, that would be even nicer, but I have not found it.This is also probably the first gem that makes use of the new admin, so it's a good discussion point for how we provide testing facilities for gems consuming the new admin.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The first commit is not green, nor will it be. But the changes from the second commit tell what's wrong, so I'm opting to leave these separate for now.
The following are not always needed: