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

Fixing new promotion system readme with migration notice and fixing some typos and wording #6105

Closed
wants to merge 75 commits into from

Conversation

fthobe
Copy link
Contributor

@fthobe fthobe commented Feb 7, 2025

Summary

  • fixed some typos
  • clearly linked to migration guide
  • changed some wording to a more positive wording

Needs some work still, but this is the bare minimum.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

tvdeyen and others added 30 commits November 12, 2024 11:04
This is part of the release process for Solidus v4.4.0.

This code has been automatically generated by our 'Prepare post-release' GitHub
action.
…945831-1-1731409469

Post-creation chores after new v4.4 patch branch
This table belongs to the legacy promotion system and was,
unfortunately, missed in the original extraction.

(cherry picked from commit d339e84)
The bin/rails script is solely used for local dev purposes.
Since the default bindir from Rubygems is bin the binstub
gets installed with the gems. This can cause issues with
the rails command comming from rails itself.

Also removing the old script/rails files, which were excluded
to solve the same issue.

(cherry picked from commit dde8966)
[v4.4] Move Line Item Actions to solidus_legacy_promotions
[v4.4] Remove rails binstubs from built gems
Prior to this commit, legacy promotions would be called "Promotions",
but if `solidus_promotions` is installed, they would be called "Legacy
Promotions" in the menu. It's much easier if each of the two promotion
engines just inserts its own menu item into the menu, and the legacy
promotion system is called "Legacy Promotions" from the get-go.

Co-Authored-By: thomas@vondeyen.com
(cherry picked from commit 6a7d839)
When this was moved to the correct gem, we failed to also move it to the
right directory. This generates Zeitwerk errors.

(cherry picked from commit 8710da9)
[v4.4] Backend Menu: Fix "Promotions" items
[v4.4] Fix file location of spree/line_item_action.rb
In the `view_component` gem, the `i18n_scope` for a component depends on
the file name of the component, and removes `app_components` and
everything before that in order to construct a scope.

This fails to work with components that live in `lib/components/admin/`,
as with our extensions, as there is no `app/components` in the path to
these components.

This change uses the class name instead, which should be easier on
memory and correctly works with components from solidus extensions.

This also fixes translations in inherited components, allowing to delete
some redundant translations in the promotion gem's `order` component.

(cherry picked from commit 46e8250)
Prior to this change, the promotion admin would have a blue color
scheme, and not the red one that all the other pages have.

(cherry picked from commit 21bb837)
This commit allows the menu of the new admin to accomodate routes from
other engines than solidus backend and solidus admin. This is needed for
`solidus_promotions`, which is built as a separate Rails Engine, but it
is also convenient for `solidus_paypal_commerce_platform` or even for
integrating gems like AlchemyCMS lateron.

Co-Authored-By: thomas@vondeyen.com
(cherry picked from commit c1e4f12)
[v4.4] Use new global variables stylesheet in legacy promotions
[v4.4] Feat(Admin): Dynamic routing proxies
This makes sure that the promotion menus are always below the product
menu item, in both new admin and the backend.

It also changes the nomenclature to be "Promotions" for the legacy
promotion system and "Promotions (new)" for the new promotion system.

(cherry picked from commit 6d75515)
Prior to this commit, the new promotion system would exchange the
promotion and promotion category index page components if the gem was
loaded. This made it impossible to actually see the old promotion
configuration when using the new admin.

This commit now only changes the orders/index component if the new
promotion system is activated. In any case, it will display new
promotion and promotion category records under "Promotions (new)".

(cherry picked from commit 0a69b6a)
This code has been automatically generated by our 'Prepare release' GitHub
action.

The actual release is not part of the automation, and it still needs to be manually done by a maintainer.
…552738-1-1731929590

Prepare release for Solidus v4.4.1
This code has been automatically generated by our 'Prepare post-release' GitHub
action.

Make sure that:

- [ ] The new release has been published, along with its tag. See https://github.com/solidusio/solidus/releases/tag/v4.4.1
- [ ] The corresponding patch branch exists. See https://github.com/solidusio/solidus/tree/v4.4
- [ ] The corresponding backport-v4.4 label exists. See https://github.com/solidusio/solidus/labels
Due to the top level constant in testing.rake the release
task was also trying to release frontend. Using a local
variable instead.
…611808-1-1731929810

Post-release chores after having released Solidus v4.4.1
Not sure how we missed that when including `solidus_promotions`.

(cherry picked from commit 6c8e842)
[v4.4] Include solidus_legacy_promotions in release task
Newest version of Rubocop wants us to remove extra spacing at the top of
method bodies.

(cherry picked from commit 6d44a13)
[v4.4] [FIX] Remove spacing at top of OrderShipping#ship method
We were missing a translation here, but never noticed, because we were
not testing for an actual human-understandable string.

This is broken in the v4.0, v4.1, v4.2 and v4.3 branches with a noisier
error message, because in those branches, `I18n.t("unavailable")` is
different from `t("unavailable")`.

(cherry picked from commit 506ddfc)
[v4.4] Backend: Add missing error translation
tvdeyen and others added 19 commits December 6, 2024 14:37
[v4.4] tests: Give even more dialogs more time to open in tests
[v4.4] Only set promo configuration to legacy if no other set
The  `authorization_subject` method from `solidus_admin` assumes that
all models are in the `Spree` namespace.

(cherry picked from commit c2ba46a)
This controller still did not know how what to authorize against, and
wanted to visit a URL that doesn't exist when clicking on a promotion.

This also changes at least the name of each promotion to be a link
element that can easily be targeted with Capybara and works with all
major browsers.

(cherry picked from commit 2eccd6d)
[v4.4] Legacy Promotions: Remove unneeded decorator
[v4.4] [FIX] A few small tweaks for the new promotion admin
[v4.4] Fix(promotions): Validate benefits on save
This code has been automatically generated by our 'Prepare release' GitHub
action.

The actual release is not part of the automation, and it still needs to be manually done by a maintainer.
…877460-1-1733751717

Prepare release for Solidus v4.4.2
This code has been automatically generated by our 'Prepare post-release' GitHub
action.

Make sure that:

- [ ] The new release has been published, along with its tag. See https://github.com/solidusio/solidus/releases/tag/v4.4.2
- [ ] The corresponding patch branch exists. See https://github.com/solidusio/solidus/tree/v4.4
- [ ] The corresponding backport-v4.4 label exists. See https://github.com/solidusio/solidus/labels
…663542-1-1733754389

Post-release chores after having released Solidus v4.4.2
The mock component in the ComponentHelpers module was not a real
constant, which messes with ViewComponent's expectations about what
render_inline is given. Using `stub_const` in the helper allows us to
give it an actual name, and view_component > 3.21.0 will work for us.

The helper is only used in the base component spec. I could have spent
more time giving it an optional block, but this solution is the most
straightforward.
Linting runs parallel to testing and circle ci is
slow and our free plan has limited resources.

Also GH actions are better integrated and has better dev
feedback.

(cherry picked from commit 7275966)
This PR modifies the Persistable module in Solidus to check the Rails version using Rails.gem_version instead of inspecting the serialize method parameters. This change is necessary only when using Solidus Globalize, ensuring compatibility with Rails 7.2.

## Background
Solidus itself does not have any issues with the current implementation:

```
if method(:serialize).parameters.include?([:key, :type]) # Rails 7.1+
  serialize :preferences, type: Hash, coder: YAML
else
  serialize :preferences, Hash, coder: YAML
end
```

However, when using Solidus Globalize, which relies on globalize, an issue arises in Rails 7.2 due to globalize overriding the serialize method:

```
ArgumentError: wrong number of arguments (given 2, expected 1)
```

This occurs because the serialize method signature is modified by globalize, causing a mismatch when Solidus inspects its parameters.
For reference, see the globalize implementation here:
https://github.com/globalize/globalize/blob/main/lib/patches/active_record/rails7_2/serialization.rb

## Solution
To avoid conflicts, this PR replaces the method introspection with a Rails version check:
```
if Rails.gem_version >= Gem::Version.new('7.1')
  serialize :preferences, type: Hash, coder: YAML
else
  serialize :preferences, Hash, coder: YAML
end
```
This approach ensures compatibility while keeping Solidus independent from dependency-specific overrides.

This issue is currently blocking the CI of the following PR in the solidus_globalize repository:
solidusio-contrib/solidus_globalize#153
Fixing this in Solidus will help unblock that PR.

(cherry picked from commit 2bcf076)
[v4.4] Fix preferences serialization compatibility with Rails version check
@fthobe fthobe requested a review from a team as a code owner February 7, 2025 17:14
@github-actions github-actions bot added the changelog:repository Changes to the repository not within any gem label Feb 7, 2025
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 93.50649% with 5 lines in your changes missing coverage. Please review.

Project coverage is 89.46%. Comparing base (dd4dbc3) to head (12a721a).
Report is 166 commits behind head on main.

Files with missing lines Patch % Lines
...nd/solidus_promotions/admin/benefits_controller.rb 25.00% 3 Missing ⚠️
core/lib/spree/testing_support/extension_rake.rb 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6105      +/-   ##
==========================================
+ Coverage   87.82%   89.46%   +1.64%     
==========================================
  Files         477      782     +305     
  Lines       11662    18005    +6343     
==========================================
+ Hits        10242    16108    +5866     
- Misses       1420     1897     +477     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


> [!IMPORTANT]
> If you are upgrading from a previous version of Solidus, it is advised that you update also the promotion system to the new gem. Please consult the [Migration Guide](./MIGRATING.md).
> While the current version of Solidus still installs the legacy promotion system, we advise a migration at the ealiest convinience to avoid having to rush the migration in the future.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: earliest.

@mamhoff
Copy link
Contributor

mamhoff commented Feb 7, 2025

Can you direct the PR to the main branch and request a backport to v4.4. instead of directing it against the v4.4 branch? Otherwise these changes will only show up in future point releases to v4.4, and not in later Solidus releases.

@fthobe
Copy link
Contributor Author

fthobe commented Feb 7, 2025

absolutely, I can do it on sunday as I am away from the computer, if you want to fix it you can just take my changes and I close the PR.

@fthobe fthobe changed the base branch from v4.4 to main February 7, 2025 18:06
@fthobe
Copy link
Contributor Author

fthobe commented Feb 7, 2025

Let me se

@fthobe fthobe closed this Feb 7, 2025
@fthobe
Copy link
Contributor Author

fthobe commented Feb 7, 2025

closed in favor of #6106

@fthobe fthobe deleted the patch-5 branch February 7, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:repository Changes to the repository not within any gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants