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

Readd config.cache_classes on test env and remove with_model dep #4358

Merged

Conversation

waiting-for-dev
Copy link
Contributor

@waiting-for-dev waiting-for-dev commented Apr 25, 2022

Rails' config setting cache_classes was recently set to false in the
dummy app (see
ef9b5ab).
That was due to a misunderstanding, as Rails recommends it be true
except when spring is present. From
https://guides.rubyonrails.org/configuring.html#config-cache-classes:

Controls whether or not application classes and modules should be
reloaded if they change. When the cache is enabled (true), reloading
will not occur. Defaults to false in the development environment, and
true in production. In the test environment, the default is false if
Spring is installed, true otherwise.

That wasn't such a big deal until now. However, if we try to use the new
Ruby 3.1 in conjunction with cache_classes in the test environment,
many errors related to STI classes not being found pop out.

However, having the correct false value on Rails 7 raises an error
sourced on the with_model gem. See
Casecommons/with_model#35.

Because of that, we remove the with_model dependency so that our path
to support Ruby 3.1 is unblocked. We take different decisions to remove
the occurrences of with_model usage on Solidus:

  • Tests on Spree::Api::ResourceController. That controller was only
    used on Spree::Api::UsersController. Therefore, it was no longer a
    valid abstraction:

    • We removed the tests altogether.
    • We moved the missing pieces to UsersController.
    • We removed the soft deletable specific test and added one to UsersController.
    • We marked ResourceController as deprecated (not removing it just in case some store uses it).
  • Tests on Spree::Admin::ResourceController, Spree::CalculatedAdjustements,
    Spree::Validations::DbMaximumLengthValidtor &
    Spree::WalletPaymentSource: we use a custom logic for the same
    behavior provided by with_model.

  • Tests on Spree::SoftDeletable: we use Spree::Product as a fixture.

Rails' config setting `cache_classes` was recently set to `false` in the
dummy app (see
ef9b5ab).
That was due to a misunderstanding, as Rails recommends it be true
except when `spring` is present. From
https://guides.rubyonrails.org/configuring.html#config-cache-classes:

> Controls whether or not application classes and modules should be
> reloaded if they change. When the cache is enabled (true), reloading
> will not occur. Defaults to false in the development environment, and
> true in production. In the test environment, the default is false if
> Spring is installed, true otherwise.

That wasn't such a big deal until now. However, if we try to use the new
Ruby 3.1 in conjunction with `cache_classes` in the test environment,
many errors related to STI classes not being found pop out.

However, having the correct `false` value on Rails 7 raises an error
sourced on the `with_model` gem. See
Casecommons/with_model#35.

Because of that, we remove the `with_model` dependency so that our path
to support Ruby 3.1 is unblocked. We take different decisions to remove
the occurrences of `with_model` usage on Solidus:

- Tests on `Spree::Api::ResourceController`. That controller was only
used on `Spree::Api::UsersController`. Therefore, it was no longer a
valid abstraction:
  - We removed the tests altogether.
  - We moved the missing pieces to `UsersController`.
  - We removed the soft deletable specific test and added one to `UsersController`.
  - We marked `ResourceController` as deprecated (not removing it just in case some store uses it).

- Tests on `Spree::Admin::ResourceController`, `Spree::CalculatedAdjustements`,
`Spree::Validations::DbMaximumLengthValidtor` &
`Spree::WalletPaymentSource`: we use a custom logic for the same
behavior provided by `with_model`.

- Tests on `Spree::SoftDeletable`: we use `Spree::Product` as a fixture.
@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/with_model_and_cache_classes branch from c05ac60 to c268949 Compare April 25, 2022 08:15
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

I like having a dependency less, and those Widget tests were always confusing to me.

Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

I'm vaguely apprehensive that extensions or stores might be using this, but it's a pretty easy change for them to switch off. Seems good.

Comment on lines +9 to +10
Spree::Api::ResourceController is deprecated. Please, copy any logic you
need and inherit directly from Spree::Api::BaseController.
Copy link
Member

Choose a reason for hiding this comment

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

"I didn't even know there was a Spree::Api::ResourceController."–@benjaminwil

@jarednorman
Copy link
Member

There's a typo in the commit message/PR name: "Readd"

@kennyadsl kennyadsl mentioned this pull request Apr 28, 2022
@waiting-for-dev waiting-for-dev force-pushed the waiting-for-dev/with_model_and_cache_classes branch from c42ae73 to c268949 Compare April 28, 2022 14:17
@waiting-for-dev
Copy link
Contributor Author

waiting-for-dev commented Apr 28, 2022

There's a typo in the commit message/PR name: "Readd"

@jarednorman, hmm... I used readd as in re+add, i.e., add again 🙂 Does it sound too weird? Not an English native speaker, so I'll trust your judgment more than mine one thousand times 😉

@waiting-for-dev
Copy link
Contributor Author

There's a typo in the commit message/PR name: "Readd"

@jarednorman, hmm... I used readd as in re+add, i.e., add again slightly_smiling_face Does it sound too weird? Not an English native speaker, so I'll trust your judgment more than mine one thousand times wink

Going ahead with the merge to not unblock support for Ruby 2.7 🙂

@waiting-for-dev waiting-for-dev merged commit 0e410a5 into master May 3, 2022
@waiting-for-dev waiting-for-dev deleted the waiting-for-dev/with_model_and_cache_classes branch May 3, 2022 08:10
kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 27, 2023
Please, inherit from Spree::Api::BaseController directly.

Ref solidusio#4358
kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 27, 2023
Please, inherit from Spree::Api::BaseController directly.

Ref solidusio#4358
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 12, 2023
Please, inherit from Spree::Api::BaseController directly.

Ref solidusio#4358
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 14, 2023
Please, inherit from Spree::Api::BaseController directly.

Ref solidusio#4358
kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 18, 2023
Please, inherit from Spree::Api::BaseController directly.

Ref solidusio#4358
kennyadsl added a commit that referenced this pull request Apr 18, 2023
Please, inherit from Spree::Api::BaseController directly.

Ref #4358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants