Opt-in to Current fully #297

2024-02-05T05:10:46+08:00

dwightwatson commented

2024-02-05 05:10:46 +08:00

(Migrated from github.com)

As I floated in #289, this moves towards using Current more fully, and replacing the helper methods like current_user and current_family.

To me, Current feels like the more Rails-y way of doing things, and current_* style helpers feel more like the Devise way of doing things. Right now we're sitting half-in half-out of both (especially considering we use a before_action called authenticate_user!).

I delegate Current.family to the current user's family
I take inspiration from the Current docs to set the user or redirect away in the one method

It might even be worth considering dropping authenticate_user to just authenticate but that's neither here nor there. I do think this PR is important because it makes access to the current user/family consistent.

As I floated in #289, this moves towards using `Current` more fully, and replacing the helper methods like `current_user` and `current_family`. To me, `Current` feels like the more Rails-y way of doing things, and `current_*` style helpers feel more like the Devise way of doing things. Right now we're sitting half-in half-out of both (especially considering we use a before_action called `authenticate_user!`). * I delegate `Current.family` to the current user's family * I take inspiration from the [`Current` docs](https://api.rubyonrails.org/classes/ActiveSupport/CurrentAttributes.html) to set the user or redirect away in the one method It might even be worth considering dropping `authenticate_user` to just `authenticate` but that's neither here nor there. I do think this PR is important because it makes access to the current user/family **consistent**.

👍 3

robzolkos (Migrated from github.com) approved these changes 2024-02-05 05:28:58 +08:00

robzolkos (Migrated from github.com) left a comment

Love this.

ricsdeol (Migrated from github.com) approved these changes 2024-02-05 05:55:55 +08:00

jclusso (Migrated from github.com) reviewed 2024-02-05 05:58:46 +08:00

app/controllers/accounts_controller.rb

						
				@@ -19,3 +19,3 @@

				  def create

				    @account = Account.new(account_params.merge(family: current_family))

				    @account = Account.new(account_params.merge(family: Current.family))

				    @account.accountable = account_params[:accountable_type].constantize.new

jclusso (Migrated from github.com) commented

2024-02-05 05:58:46 +08:00

In the Current docs they show how you can make this even cleaner by setting the default on the model using Current instead of having to merge it in with params.

belongs_to :creator, default: -> { Current.user }

In the Current [docs](https://api.rubyonrails.org/classes/ActiveSupport/CurrentAttributes.html) they show how you can make this even cleaner by setting the default on the model using `Current` instead of having to merge it in with params. ```ruby belongs_to :creator, default: -> { Current.user } ```

dwightwatson (Migrated from github.com) reviewed 2024-02-05 06:02:51 +08:00

app/controllers/accounts_controller.rb

						
				@@ -19,3 +19,3 @@

				  def create

				    @account = Account.new(account_params.merge(family: current_family))

				    @account = Account.new(account_params.merge(family: Current.family))

				    @account.accountable = account_params[:accountable_type].constantize.new

dwightwatson (Migrated from github.com) commented

2024-02-05 06:02:51 +08:00

I was about to follow up with another PR that adds test for this controller and then uses the association fully...

@account = Current.family.accounts.new(account_params)

I think that's the clearest option. I personally quite like the default: option though.

I was about to follow up with another PR that adds test for this controller and then uses the association fully... ```rb @account = Current.family.accounts.new(account_params) ``` I think that's the clearest option. I personally quite like the `default:` option though.

Shpigford commented

2024-02-05 07:36:44 +08:00

(Migrated from github.com)

Great job on this, @dwightwatson!

❤️ 1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#297