Move to Spree::SoftDeletable on users model

[jtapia]

Summary

• Based on the latest gemspec of auth-devise, move to Spree::SoftDeletable

[gsmendoza]

Hello @jtapia !

• Can you reproduce the issue on the latest patch version of Solidus, v3.1.7?
• What Rails version are you using?

[jarednorman]

Hey @jtapia! The commit in this PR appears to two things:

1. It removes support for Solidus versions prior to the introduction of Spree::SoftDeletable in order to fix an autoloading issue
2. Fixes keyword usage in order to fix Ruby 3.x support in the specs.

We really like atomic commits on the Solidus projects, and expect some level of explanation in the commit messages. This article has a good example of what we expect.

If you split the commit in two and PR the Ruby 3.x compat change, we can get that merged right away. That's an easy rubber stamp.

As for the first thing, the soft deletion stuff, can you open an issue explaining the issue you're seeing? We need to understand what the issue is and confirm that this is the correct solution before we move forward with that change.

[jakepenn]

I'm seeing this autoloader issue on 3.1.7 -- App boots well but after modifying hot reload crashes:

undefined local variable or method acts_as_paranoid for Spree::User:Class

gem 'rails', '6.1.4.1'

[gsmendoza]

@jakemumu Can you tell what file you are modifying that causes hot reload to crash? How can we reproduce the error?

[jarednorman]

We probably don't want https://github.com/solidusio/solidus/blob/master/core/app/models/concerns/spree/soft_deletable.rb to be code-reloaded. It sounds like there's an issue with code reload order with it.

[jtapia]

@jarednorman ok, I moved it into 2 commits and created this issue #225
is anything else that do you need?

[jarednorman]

If you put the keyword commit in a new PR we can merge it right away. I don't think the other commit will be the solution we want for the autoload issue though.

[jtapia]

@jakemumu ok, here is the new PR #226

[jtapia]

@jarednorman if you removed all the references for acts_as_paranoid here https://github.com/solidusio/solidus_auth_devise/blob/master/app/models/spree/user.rb#L11=, why we continue using here https://github.com/solidusio/solidus_auth_devise/blob/master/app/models/spree/user.rb#L14=?
for older Solidus versions is ok, but since 3.0 removed Paranoia, I think we don't need that condition anymore or if we want to continue supporting older versions, we gonna need to bring back the paranoia reference

[jtapia]

@gsmendoza I'm using rails', '6.1.4.1' and didn't test it yet with Solidus v3.1.7, on the issue I added more information

[jarednorman]

@jarednorman if you removed all the references for acts_as_paranoid here https://github.com/solidusio/solidus_auth_devise/blob/master/app/models/spree/user.rb#L11=, why we continue using here https://github.com/solidusio/solidus_auth_devise/blob/master/app/models/spree/user.rb#L14=?
for older Solidus versions is ok, but since 3.0 removed Paranoia, I think we don't need that condition anymore or if we want to continue supporting older versions, we gonna need to bring back the paranoia reference

I'm not sure I understand what you're asking. The conditional above allows stores that are significantly behind (pre-v2.10.4 I think) on their Solidus upgrades to still use the latest version of solidus_auth_devise.

[jakepenn]

I get what you're saying @jarednorman -- I think that sounds correct. During hot reloading that check is failing & it's trying to load paranoid -- on initial app launch it's all good. It must be the user loading first for some reason.

@gsmendoza -- It's not any single file but we use view_component gem in our app and I've noticed it occur whenever I edit a x_component.rb

[jarednorman]

Yeah, it probably isn't significant what file you edit. Zeitwerk will reload everything if anything has changed since the last request.

[elia]

I'm not sure I understand what you're asking. The conditional above allows stores that are significantly behind (pre-v2.10.4 I think) on their Solidus upgrades to still use the latest version of solidus_auth_devise.

@jarednorman the latest gemspec of auth-devise requires solidus 3.x, so I think it's ok to rely on Spree::SoftDeletable now. Any objections to merging other than updating the commit message?

[jarednorman]

Ah, no, that's fine then.

[elia]

@jtapia can you take care of updating the commit message and PR title?

[jtapia]

@elia I updated it, but not sure if that is the title and description you want to, please let me know if you want to change it and I can do it

[elia]

@jtapia I mentioned the PR title, but the most important thing is the commit message, for future lookups, as the current one could be confusing 🙏

[sislr]

@jarednorman would you mind looking into this again?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (5a8d695) to head (cd13355).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff     @@
##   main   #223   +/-   ##
===========================
===========================

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.