Skip to content

Add Solidus 4 support #39

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add Solidus 4 support #39

wants to merge 6 commits into from
+33 −30

Conversation

@kjriga
Copy link
Contributor

@kjriga kjriga commented Sep 2, 2025 •
edited
Loading

This adds Solidus 4 support while maintaining backwards compatibility.

  • Updating the solidus core dependency
  • Fixing email subscriber for Solidus 4s refactored mailer system (ensuring compatibility with non-solidus-4)

Specs are passing locally.

All examples were filtered out; ignoring {focus: true}

Randomized with seed 26676
Finished in 1.61 seconds (files took 2.3 seconds to load)
99 examples, 0 failures

Randomized with seed 26676

Coverage report generated for RSpec to /Users/kendrariga/Documents/work-projects/solidus_tracking/coverage.
Line Coverage: 87.58% (402 / 459)

@kjriga kjriga force-pushed the solidus-4-support branch 17 times, most recently from 1057d24 to 62461dd Compare September 5, 2025 18:15
@kjriga
Copy link
Contributor Author

kjriga commented Sep 5, 2025

Aware I still have a failure here spec/models/spree/order_spec.rb:81 but any initial feedback would be nice. Thanks

@kjriga kjriga marked this pull request as ready for review September 5, 2025 18:15
@kjriga kjriga marked this pull request as draft September 5, 2025 18:15
jarednorman
jarednorman requested changes Sep 5, 2025
View reviewed changes
@kjriga kjriga force-pushed the solidus-4-support branch 3 times, most recently from 147be7e to ecfc13a Compare September 19, 2025 19:17
@kjriga kjriga force-pushed the solidus-4-support branch 5 times, most recently from 4787dab to b833477 Compare September 23, 2025 18:40
@kjriga kjriga force-pushed the solidus-4-support branch 5 times, most recently from 6543d3f to eead68b Compare October 7, 2025 19:30
@kjriga kjriga force-pushed the solidus-4-support branch 4 times, most recently from 046fd7c to 45f59d6 Compare October 10, 2025 17:49
@kjriga kjriga marked this pull request as ready for review October 10, 2025 17:49
@kjriga kjriga force-pushed the solidus-4-support branch from 45f59d6 to 3213c0f Compare October 10, 2025 17:51
@kjriga kjriga requested a review from forkata October 10, 2025 17:51
@kjriga kjriga force-pushed the solidus-4-support branch from 3213c0f to 55f994a Compare October 10, 2025 17:54
AlistairNorman
AlistairNorman reviewed Oct 10, 2025
View reviewed changes
solidus_tracking.gemspec
spec.require_paths = ["lib"]

spec.add_dependency 'solidus_core', ['>= 2.0.0', '< 4']
spec.add_dependency 'solidus_core', ['>= 4.0.0', '< 4.6']
Copy link

Choose a reason for hiding this comment

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

Can you add something in this commit about why we don't support 4.6/main?

Gemfile
# Provides basic authentication functionality for testing parts of your engine
gem 'solidus_auth_devise'

gem 'state_machines', '0.6.0'
Copy link

Choose a reason for hiding this comment

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

Can you reference the issue so we have some more context on what this is fixing?

Copy link

Choose a reason for hiding this comment

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

FYI this is the issue I mean solidusio/solidus#6326

@AlistairNorman
Copy link

A little more context needed in a couple places but this looks good as long as tests are passing locally.

@kjriga kjriga force-pushed the solidus-4-support branch from 55f994a to 24aa010 Compare October 15, 2025 14:25
@kjriga kjriga requested a review from jarednorman October 15, 2025 14:26
@kjriga
Copy link
Contributor Author

kjriga commented Oct 15, 2025 •
edited
Loading

I am happy to pull out the final commit for moving Ci -> github actions and PR it separately if it is preferred. @jarednorman I think I'll need you to update the repo to work with github actions. I took the test workflow from here but I won't know if it's working until the repo attempts to run it.

@jarednorman
Copy link
Member

@kjriga If you pull the GitHub Action commit out into its own PR, I'll merge that and it'll get the CI to start running. You'll have to fix any issues with it in a separate PR.

@kjriga
Copy link
Contributor Author

kjriga commented Oct 15, 2025

@kjriga If you pull the GitHub Action commit out into its own PR, I'll merge that and it'll get the CI to start running. You'll have to fix any issues with it in a separate PR.

PR

@kjriga kjriga force-pushed the solidus-4-support branch from 24aa010 to 149a0dd Compare October 15, 2025 16:15
kjriga added 6 commits October 17, 2025 12:48
@kjriga
Bump supported Solidus versions
f4f837a 
We want to support Solidus 4 to 4.5.

I stopped at 4.5 because there are changes in 4.6+ with some
'disable_builtin_emails' configuration that required more work, so I
think a plan to move forward is to make the current gemspec only allow
'>= 4.0.0', '< 4.6', and add some deprecation warnings about that
feature becoming obsolete. Then have a new version that's '>= 4.6.0',
'<5.0' that removes it all.

I am doing this work specifically to bump another app that uses it to
Solidus 4, and that is my priority at the moment.
@kjriga
Update mailer subscriber for Solidus 4 compatibility
617d05c 
- Update method name from order_finalized to send_confirmation_email
- Change target class from Spree::MailerSubscriber to Spree::OrderMailerSubscriber
- Remove version conditional since gem now only supports Solidus 4+
@kjriga
Remove unneeded decorator
593d1ca 
No longer required with Solidus 4+. The method it is overriding doesn't
exist anymore.
@kjriga
Fix various failing specs
6c342c6 
- Add the requirement of open struct now that it isn't automatically
  included
- Change expectations to account for changes with how state changes are
  handled in background jobs. solidusio/solidus@2a2df71
- Replace build_stubbed with just build, some objects were missing
  fields
- Since we removed the requirement for solidus_frontend from this gem,
  we don't care about the reset password email sending so we can just
  stub it out
@kjriga
Lock state_machines gem
041e5a6 
To fix 'stack level too deep' error.
@kjriga
Add decorator for DisableCancelEmail
0b06fbd 
Similar to the confirm email, we need this to respect the
'disable_builtin_emails' value.
@kjriga kjriga force-pushed the solidus-4-support branch from 149a0dd to 0b06fbd Compare October 17, 2025 18:49
@jarednorman
Copy link
Member

jarednorman commented Oct 20, 2025 •
edited
Loading

I've just disabled the CircleCI builds, but it looks like the GitHub builds are busted.

Edit: I see you are working on that in #42.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@forkata forkata Awaiting requested review from forkata

@jarednorman jarednorman Awaiting requested review from jarednorman

1 more reviewer

@AlistairNorman AlistairNorman AlistairNorman left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kjriga @AlistairNorman @jarednorman @forkata