Deprecate try_spree_current_user by elia · Pull Request #3923 · solidusio/solidus

elia · 2021-02-05T16:36:00Z

Description

It was introduced 9 years ago to overcome an obscure call loop in the early days of Spree (a00d703) and to support both a custom helper (spree_current_user) and the helper generated by Devise (current_spree_user).

Then, 8 years ago, solidus_auth_devise actually added support for the official custom helper (spree_current_user) making
try_spree_current_user finally unnecessary (solidusio/solidus_auth_devise@69269765).

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)

spaghetticode

I like this deprecation, @elia thank you 👍

jarednorman

Would it be preferred to fix the custom user generator? We get people in the support channel asking for help around this use case.

elia · 2021-11-18T10:26:06Z

(sorry for the hiatus)
@jarednorman I agree we should fix the custom user generator, but in the meanwhile I believe it's better not to have docs rather than misleading/broken ones. Anyway I removed that commit and will move it to a different issue/PR so it can be dealt with properly 👍

waiting-for-dev

It's always so lovely removing tech debt. Thanks! 🙂

waiting-for-dev · 2022-06-15T09:48:33Z

Hey @elia, not sure why this PR got staled. Would you mind rebasing from master? 🙏

elia · 2022-06-15T12:55:25Z

@waiting-for-dev done ✅

jarednorman

What's the deal with the final commit? It doesn't seem related to the PR title.

waiting-for-dev · 2022-06-23T11:54:08Z

What's the deal with the final commit? It doesn't seem related to the PR title.

@elia, what about moving it to a separated PR? 🙏

elia · 2022-06-24T09:09:07Z

@jarednorman @waiting-for-dev extracted the last commit to #4440, the reason behind that was to remove all ties to devise and reduce the amount of hidden coupling we have between solidus and solidus_auth_devise.

jarednorman · 2022-06-24T17:23:39Z

 class ApplicationController < ActionController::Base
  protect_from_forgery with: :exception
+
+  def spree_current_user


Why is this needed?

When using try_spree_current_user you can get away with not setting up any authentication and it will just return nil. By not using it we're relying more on the expectation on ApplicationController to implement this method. For test purposes having a no-op method is equivalent of using try_spree_current_user without an auth implementation.

It was introduced 9 years ago to overcome an obscure call loop in the early days of Spree (a00d703) and to support both a custom helper (spree_current_user) and the helper generated by Devise (current_spree_user). Then, 8 years ago, solidus_auth_devise actually added support for the official custom helper (spree_current_user) making try_spree_current_user finally unnecessary (solidusio/solidus_auth_devise@69269765).

waiting-for-dev · 2022-06-29T04:05:14Z

@elia, do we need to update solidus_auth_devise calls now?

waiting-for-dev · 2022-06-29T04:07:14Z

I wonder if we still need to support the method through solidus_support. Otherwise, newer versions of solidus_auth_devise won't be compatible with older solidus versions.

elia · 2022-06-29T16:09:04Z

I wonder if we still need to support the method through solidus_support. Otherwise, newer versions of solidus_auth_devise won't be compatible with older solidus versions.

I'm quite sure that since auth-devise will implement spree_current_user and that is also a requirement for implementing a custom auth it's not needed. Any extension could switch to be using spree_current_user even now without this PR, it's just that we kept that helper around and people went on using it because it looks safer.

@elia, do we need to update solidus_auth_devise calls now?

Good call, I'll send a pr for auth-devise 👍

The current gem is defining `spree_current_user` so we don't need to be defensive about using it directly. Related to solidusio/solidus#3923.

We deprecated `#try_spree_current_user` in favor of `#spree_current_user` in solidusio#3923 [1]. The former defaulted to `nil` when no authentication library was present, but the latter was simply not defined under those circumstances. This commit restores the old behavior, as the code base works with the assumption that current user is nil when no extension has defined it (e.g. [2] & [3]). The issue was raised in the support channel [4]. [1] - solidusio#3923 [2] - https://github.com/nebulab/solidus/blob/fee8a860c068b6ad050f9b2ff9052aadf9d56174/backend/app/controllers/spree/admin/base_controller.rb#L36 [3] - https://github.com/nebulab/solidus/blob/fee8a860c068b6ad050f9b2ff9052aadf9d56174/backend/app/controllers/spree/admin/cancellations_controller.rb#L33 [4] - https://solidusio.slack.com/archives/C0JBKDF35/p1657030519259159?thread_ts=1657019907.668419&cid=C0JBKDF35

We deprecated `#try_spree_current_user` in favor of `#spree_current_user` in solidusio#3923 [1]. The former defaulted to `nil` when no authentication library was present, but the latter was simply not defined under those circumstances. This commit restores the old behavior, as the code base works with the assumption that current user is `nil` when no extension has defined it (e.g. [2] & [3]). The issue was raised in the support channel [4]. [1] - solidusio#3923 [2] - https://github.com/nebulab/solidus/blob/fee8a860c068b6ad050f9b2ff9052aadf9d56174/backend/app/controllers/spree/admin/base_controller.rb#L36 [3] - https://github.com/nebulab/solidus/blob/fee8a860c068b6ad050f9b2ff9052aadf9d56174/backend/app/controllers/spree/admin/cancellations_controller.rb#L33 [4] - https://solidusio.slack.com/archives/C0JBKDF35/p1657030519259159?thread_ts=1657019907.668419&cid=C0JBKDF35

We deprecated `#try_spree_current_user` in favor of `#spree_current_user` in solidusio#3923 [1]. The former defaulted to `nil` when no authentication library was present, but the latter was simply not defined under those circumstances. This commit restores the old behavior, as the code base works with the assumption that current user is `nil` when no extension has defined it (e.g. [2] & [3]). The issue was raised in the support channel [4]. [1] - solidusio#3923 [2] - https://github.com/solidusio/solidus/blob/fee8a860c068b6ad050f9b2ff9052aadf9d56174/backend/app/controllers/spree/admin/base_controller.rb#L36 [3] - https://github.com/solidusio/solidus/blob/fee8a860c068b6ad050f9b2ff9052aadf9d56174/backend/app/controllers/spree/admin/cancellations_controller.rb#L33 [4] - https://solidusio.slack.com/archives/C0JBKDF35/p1657030519259159?thread_ts=1657019907.668419&cid=C0JBKDF35

This fixes SolidusStarterFrontend after the changes in solidusio/solidus#3923 (Deprecate try_spree_current_user). WARNING: this change causes two examples in the `templates/spec/system/checkout_spec.rb` to reveal that they are broken. They will be fixed in the next commit.

solidusio/solidus#3923

Please, use spree_current_user instead. Ref solidusio#3923

See solidusio/solidus#3923

houndci-bot reviewed Feb 5, 2021

View reviewed changes

Comment thread frontend/app/controllers/spree/checkout_controller.rb Outdated

elia force-pushed the deprecate-try-spree-current-user branch 2 times, most recently from 96abb82 to fbe348d Compare February 5, 2021 21:53

elia marked this pull request as ready for review February 26, 2021 09:17

elia force-pushed the deprecate-try-spree-current-user branch from fbe348d to 539ec3c Compare February 26, 2021 10:00

spaghetticode approved these changes May 3, 2021

View reviewed changes

jarednorman reviewed May 3, 2021

View reviewed changes

elia force-pushed the deprecate-try-spree-current-user branch 2 times, most recently from a208edd to 6118ae2 Compare November 18, 2021 10:23

elia requested a review from jarednorman November 18, 2021 10:26

jarednorman approved these changes Nov 18, 2021

View reviewed changes

waiting-for-dev approved these changes Nov 19, 2021

View reviewed changes

elia force-pushed the deprecate-try-spree-current-user branch from 6118ae2 to 106738c Compare June 15, 2022 12:55

jarednorman requested changes Jun 15, 2022

View reviewed changes

Comment thread core/lib/spree/core/controller_helpers/auth.rb

elia mentioned this pull request Jun 24, 2022

Remove the devise-specific fields from spree_users #4440

Closed

5 tasks

elia force-pushed the deprecate-try-spree-current-user branch from 106738c to 48797e7 Compare June 24, 2022 09:00

elia requested a review from jarednorman June 24, 2022 09:09

elia force-pushed the deprecate-try-spree-current-user branch from 48797e7 to a17d30b Compare June 24, 2022 09:35

jarednorman reviewed Jun 24, 2022

View reviewed changes

elia requested a review from jarednorman June 27, 2022 12:30

elia added 2 commits June 27, 2022 14:30

Migrate all uses of try_spree_current_user to spree_current_user

201d0e8

elia force-pushed the deprecate-try-spree-current-user branch from a17d30b to 201d0e8 Compare June 27, 2022 12:30

jarednorman approved these changes Jun 27, 2022

View reviewed changes

waiting-for-dev merged commit 8c6e205 into solidusio:master Jun 29, 2022

waiting-for-dev deleted the deprecate-try-spree-current-user branch June 29, 2022 03:23

elia added a commit to nebulab/solidus_auth_devise that referenced this pull request Jun 30, 2022

Stop using try_spree_current_user

9597190

The current gem is defining `spree_current_user` so we don't need to be defensive about using it directly. Related to solidusio/solidus#3923.

elia mentioned this pull request Jun 30, 2022

Stop using try_spree_current_user solidusio/solidus_auth_devise#221

Merged

waiting-for-dev mentioned this pull request Jul 15, 2022

Fix stores with no authentication #4456

Merged

gsmendoza mentioned this pull request Jul 18, 2022

Use spree_current_user instead of try_spree_current_user solidusio/solidus_starter_frontend#231

Merged

2 tasks

cpfergus1 added a commit to cpfergus1/solidus_paypal_commerce_platform that referenced this pull request Sep 21, 2022

Remove try_spree_current_user

12c7a6d

solidusio/solidus#3923

cpfergus1 added a commit to cpfergus1/solidus_paypal_commerce_platform that referenced this pull request Sep 26, 2022

Remove try_spree_current_user

6fd8b01

solidusio/solidus#3923

cpfergus1 added a commit to cpfergus1/solidus_paypal_commerce_platform that referenced this pull request Sep 29, 2022

Remove try_spree_current_user

9d2a6f9

solidusio/solidus#3923

cpfergus1 added a commit to cpfergus1/solidus_paypal_commerce_platform that referenced this pull request Sep 29, 2022

Remove try_spree_current_user

01118ed

solidusio/solidus#3923

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 28, 2023

Remove deprecated try_spree_current_user

79fb7c7

Please, use spree_current_user instead. Ref solidusio#3923

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 28, 2023

Remove deprecated try_spree_current_user

3b585fa

Please, use spree_current_user instead. Ref solidusio#3923

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 28, 2023

Remove deprecated try_spree_current_user

808da89

Please, use spree_current_user instead. Ref solidusio#3923

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 28, 2023

Remove deprecated try_spree_current_user

a590929

Please, use spree_current_user instead. Ref solidusio#3923

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 12, 2023

Remove deprecated try_spree_current_user

d4f375a

Please, use spree_current_user instead. Ref solidusio#3923

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 14, 2023

Remove deprecated try_spree_current_user

59ce38d

Please, use spree_current_user instead. Ref solidusio#3923

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 18, 2023

Remove deprecated try_spree_current_user

ee98ec4

Please, use spree_current_user instead. Ref solidusio#3923

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 18, 2023

Remove deprecated try_spree_current_user

102881e

Please, use spree_current_user instead. Ref solidusio#3923

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 19, 2023

Remove deprecated try_spree_current_user

ca2968a

Please, use spree_current_user instead. Ref solidusio#3923

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 24, 2023

Remove deprecated try_spree_current_user

a99b82e

Please, use spree_current_user instead. Ref solidusio#3923

tvdeyen added a commit to tvdeyen/alchemy-solidus that referenced this pull request May 18, 2023

Use spree_current_user over try_spree_current_user

b4ec701

See solidusio/solidus#3923

Uh oh!

Conversation

elia commented Feb 5, 2021 • edited by spaghetticode Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

spaghetticode left a comment

Choose a reason for hiding this comment

Uh oh!

jarednorman left a comment

Choose a reason for hiding this comment

Uh oh!

elia commented Nov 18, 2021

Uh oh!

waiting-for-dev left a comment

Choose a reason for hiding this comment

Uh oh!

waiting-for-dev commented Jun 15, 2022

Uh oh!

elia commented Jun 15, 2022

Uh oh!

jarednorman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

waiting-for-dev commented Jun 23, 2022

Uh oh!

elia commented Jun 24, 2022

Uh oh!

jarednorman Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

elia Jun 27, 2022

Choose a reason for hiding this comment

Uh oh!

waiting-for-dev commented Jun 29, 2022

Uh oh!

waiting-for-dev commented Jun 29, 2022

Uh oh!

elia commented Jun 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

elia commented Feb 5, 2021 •

edited by spaghetticode

Loading