Use `UpdateUser` class when updating users #1067

jessieay · 2017-02-07T23:39:20Z

Why: Makes our app less tied to Rails

jessieay · 2017-02-08T00:11:17Z

spec/controllers/sign_up/passwords_controller_spec.rb

-      user.update(
-        confirmation_token: token, confirmation_sent_at: Time.current
-      )
+      token = Devise.token_generator.generate(User, :confirmation_token)


woah I removed the comma and this still works! is that bad?

I don't think that's necessarily bad. This Devise generator returns an array whose first element is the raw token, and the second element is the encrypted token to be stored in the DB. The generator is meant to be used such that each part of the array is assigned to a different variable.

In this case, if you remove the comma, the token variable will be an Array, but it will get saved to the DB as a String. Then, when you make the POST request, Devise will compare the passed in token to the value in the DB. In this case, they match, so all is good.

Looking at Devise's confirmable module, I see that the confirmation_token doesn't get encrypted. It gets generated via Devise.friendly_token, so perhaps we should do the same in this test.

Thinking about this, I don't think we even need to invoke Devise at all. We just need to make sure the passed in token matches what's in the DB, so we can set it to any old String.

so right! will probably make tests faster to do that, too. updated at c299d10

monfresh · 2017-02-08T20:47:36Z

app/controllers/concerns/two_factor_authenticatable.rb

+
+    UpdateUser.new(
+      user: current_user,
+      attributes: set_time_lock_if_max_attempts_reached


I think you missed the second_factor_attempts_count attribute. Here, we definitely want to increment second_factor_attempts_count by 1, and we want to use UpdateUser to do that. Then, optionally, we might also want to update second_factor_locked_at. So, I think perhaps this will do it:

attributes = { second_factor_attempts_count: current_user.second_factor_attemps_count + 1 } attributes[:second_factor_locked_at] = Time.zone.now if current_user.max_login_attempts? UpdateUser.new( user: current_user, attributes: attributes )

we are still updating second_factor_attempts_count above. If I remove Line 83, a bunch of specs fail (so it is well tested, yay!)

The code you have above works the same way as the current code. do you think it is more readable? In general I prefer longer conditional statements like we have in set_time_lock_if_max_attempts_reached , find them easier to parse than trailing conditionals

I think it is. I read it like this:

Attributes are being set. Some are set conditionally.

Update the user with the final attributes.

This way, I can see all the attributes in one place, and the update is performed using one method.

In your version, we are mixing and matching 2 ways to update the user. First, we use the setter, then we use update. I don't think I've seen that combination before. I've seen either setter + #save, or update by itself. I think it has a higher cognitive load. I read it like this:

Set some attributes on the user via setter

Call UpdateUser with some other attributes

Scroll down to see what those attributes are. Notice that they can be empty, which first made me think, why are we making a DB call with empty attributes? Then I realized there were attributes being set previously via the setter call.

Makes sense. updated at fbd37d4

eek ok this caused many spec failures because we need to increment user second factor attempts before checking current_user.max_login_attempts? (can't just set attr in a hash)

refactored / fixed at e9b5d9a

monfresh · 2017-02-08T20:50:14Z

app/controllers/users/reset_passwords_controller.rb

-      resource.update(confirmed_at: Time.current) unless resource.confirmed?
-      resource.update(password: user_params[:password])
+      resource.confirmed_at = Time.current unless resource.confirmed?
+      UpdateUser.new(user: resource, attributes: { password: user_params[:password] }).call
    end


Same comment here about using UpdateUser consistently.

which comment are you referring to?

The comment about using one method to update the user, as opposed to mixing and matching setter + UpdateUser.

got it! updated at fbd37d4

monfresh · 2017-02-08T20:52:35Z

app/services/idv/attempter.rb

+      UpdateUser.new(
+        user: current_user,
+        attributes: { idv_attempts: current_user.idv_attempts + 1, idv_attempted_at: Time.current }
+      ).call


Why the change from Time.zone.now to Time.current? I think we should be consistent and stick with one. Looks like we use the former more than the latter.

honestly: line length. I saw that we are using them both so thought I'd take the short cut. Seems like usage is kind of 50/50?

monfresh

Looks great % comments. Thanks for doing this!

**Why**: Makes our app less tied to Rails

**Why**: Need to increment user second factor attempts before checking `current_user.max_login_attempts?`

jessieay · 2017-02-09T18:05:36Z

app/services/key_rotator/hmac_fingerprinter.rb

@@ -14,7 +14,7 @@ def rotate(user:, pii_attributes: nil, profile: nil)

    def rotate_email_fingerprint(user)
      ee = EncryptedAttribute.new_from_decrypted(user.email)
-      user.update_columns(email_fingerprint: ee.fingerprint)
+      UpdateUser.new(user: user, attributes: { email_fingerprint: ee.fingerprint }).call


is it important for this to be update_columns instead of update! ?

Behavior is slightly different but changing this breaks no tests. If we want to revert, I can create a ticket to write tests for this so we know not to change it.

cc @monfresh @pkarman

I didn't want the updated_at timestamp to change, since no user data is changing. No hard biz requirement around that. I just prefer maintenance tasks like this to be as transparent as possible. ref http://www.davidverhasselt.com/set-attributes-in-activerecord/

Is your suggestion that we revert and write some tests later, or keep as is? Happy to do either.

update_columns is faster, but it doesn't perform any validations or callbacks. Given that we don't have any in the User model (except for set_default_role), I think it should be fine to use update_columns. I would vote to pick one for consistency.

Hey @monfresh when you say "pick one for consistency", what exactly are you referring to? Totally down with using update_columns in the key rotator classes, but assume we'd want to stick with update! elsewhere?

Given that my decision to use update_columns was both for performance reasons (rotating millions of records can take time), and to avoid the updated_at timestamp, I would prefer to see it stay in this instance. ActiveRecord has multiple ways to do things, because a single method does not fit every need. I would rather keep it here. Adding more indirection doesn't help here IMO.

Cool, will revert change here and add tests separately so we don't make this change again! https://github.com/18F/identity-private/issues/1450

Reverted at 6ac6a7b

What I'm saying is that I think we can use update_columns everywhere because we don't have any validations in the User model. update! will never raise an error.

oh huh that's interesting, sorry I merged before I saw this comment.

I feel like using an update method that does not run validations or callbacks makes sense in some situtations, like this one, but perhaps not all?

In general in any rails app I would expect records to be updated with update or update!, but I could be convinced otherwise, I supposed.

**Why**: Makes our app less tied to Rails

jessieay self-assigned this Feb 8, 2017

jessieay added the status - ready for review label Feb 8, 2017

jessieay commented Feb 8, 2017

View reviewed changes

jessieay force-pushed the jy-update-user branch from f5d14de to bebbe3a Compare February 8, 2017 17:25

monfresh reviewed Feb 8, 2017

View reviewed changes

monfresh approved these changes Feb 8, 2017

View reviewed changes

jessieay force-pushed the jy-update-user branch 3 times, most recently from 52bb9ca to 3fae0e3 Compare February 9, 2017 00:51

jessieay added 2 commits February 9, 2017 10:03

Use UpdateUser class when updating users

db78ba0

**Why**: Makes our app less tied to Rails

Refactor to make logic work

b6e043f

**Why**: Need to increment user second factor attempts before checking `current_user.max_login_attempts?`

jessieay force-pushed the jy-update-user branch from e9b5d9a to b6e043f Compare February 9, 2017 18:03

jessieay commented Feb 9, 2017

View reviewed changes

Revert changes to update_columns

6ac6a7b

jessieay merged commit 176ac5f into master Feb 9, 2017

jessieay deleted the jy-update-user branch February 9, 2017 18:51

amoose pushed a commit that referenced this pull request Mar 7, 2017

Use UpdateUser class when updating users (#1067)

2100b70

**Why**: Makes our app less tied to Rails

amoose pushed a commit that referenced this pull request Mar 8, 2017

Use UpdateUser class when updating users (#1067)

6803421

**Why**: Makes our app less tied to Rails

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `UpdateUser` class when updating users #1067

Use `UpdateUser` class when updating users #1067

jessieay commented Feb 7, 2017

jessieay Feb 8, 2017

monfresh Feb 8, 2017

monfresh Feb 8, 2017 •

edited

jessieay Feb 8, 2017

monfresh Feb 8, 2017

jessieay Feb 8, 2017

monfresh Feb 8, 2017

jessieay Feb 9, 2017

jessieay Feb 9, 2017

monfresh Feb 8, 2017

jessieay Feb 8, 2017

monfresh Feb 8, 2017

jessieay Feb 9, 2017

monfresh Feb 8, 2017

jessieay Feb 8, 2017

monfresh Feb 8, 2017

monfresh left a comment

jessieay Feb 9, 2017

pkarman Feb 9, 2017

jessieay Feb 9, 2017

monfresh Feb 9, 2017

jessieay Feb 9, 2017

pkarman Feb 9, 2017

jessieay Feb 9, 2017

jessieay Feb 9, 2017

monfresh Feb 9, 2017

jessieay Feb 9, 2017

Use UpdateUser class when updating users #1067

Use UpdateUser class when updating users #1067

Conversation

jessieay commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfresh Feb 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfresh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `UpdateUser` class when updating users #1067

Use `UpdateUser` class when updating users #1067

monfresh Feb 8, 2017 •

edited