Users can CRUD deputy assignments #363

rschenk · 2021-11-11T19:47:47Z

A few things to note:

I modified the database schema of the deputy_assignments table, added another column which allows me to write a database level uniqueness constraint that all active DeputyAssignments must have a unique pairing of (primary, deputy) but inactive DeputyAssignments can have multiples. There's now a big comment on DeputyAssignment class explaining how this works.
There's a big form object that accepts a user's webaccess id, then does all the legwork of creating and validating a DeputyAssignment for them. The logic for this is broken down in Allow an RMD user to assign another RMD user as their delegate #269. It also puts a lot of effort into making the validation errors look reasonable to the user on the screen.
There's also a much smaller service object used to delete/deactivate DeputyAssignments
Based on the fact that we renamed Delegates/Deputies/Proxies three times in a week, I put everything in a translation file. While I was at it, I just put the entire user profile management navigation bar into the translations as well.
I added ViewComponent for making the Proxies UI

There is one thing I noticed when coding this up, it seems like the Rails-UJS javascript is being loaded/initialized twice. We should look into that, because any button with data: { confirm: ... } will prompt you twice.

Closes #269

rschenk · 2021-11-11T20:30:31Z

app/forms/new_deputy_assignment_form.rb

+      if deputy.persisted?
+        existing_deputy_assignment = DeputyAssignment.active.find_by(
+          primary: primary,
+          deputy: deputy
+        )
+        raise AlreadyAssigned if existing_deputy_assignment.present?
+      end


FYI if you are reviewing this commit-by-commit, CodeClimate flagged this and I agree. I moved this check out of this method and into its own in a future commit

awead

I must admit, I had great difficulty getting through this. I understand there's a lot to get done here, so it's going to be a complicated PR. My main concerns are with the form object which doesn't seem to be sustainably written. In other words, it's going to require a lot of cognitive load when we have to revisit it in the future. Relying on raising and catching exceptions is thornier than using errors directly. Also, there might some other concerns regarding the PsuIdentity end that could be shoved off to the gem and then it's be one less "branch" condition to deal with here.

I'm happy to pair up or give more detailed feedback, so I'll leave at this for now.

app/models/deputy_assignment.rb

awead · 2021-11-11T20:03:14Z

app/models/deputy_assignment.rb

+      return if active?
+      return if active_uniqueness_key == id
+
+      update_column(:active_uniqueness_key, id)


return if active? || active_uniqueness_key == id ?

awead · 2021-11-11T21:09:35Z

app/models/deputy_assignment.rb

+
+      # See note at the top of this class for what this does and how.
+      self.active_uniqueness_key ||= if is_active?
+                                       0


This line is coming up uncovered.

I noticed that rcov reported this too, even though it is covered

awead · 2021-11-11T21:09:44Z

app/models/deputy_assignment.rb

+                                       # If we are _creating_ an inactive record,
+                                       # init with temp value of current time in millis.
+                                       # Will update it to the row's id once we know it
+                                       (Time.zone.now.to_f * 1000).to_i


This is also uncovered too.

I noticed that too, except it totally has tests so I don't know what rcov's deal is

Maybe it's weirded out by some combo of the format/indentation/comments here?

app/components/deputy_assignment_component.html.erb

awead · 2021-11-11T21:48:00Z

app/forms/new_deputy_assignment_form.rb

+        new_user.last_name = psu_identity.family_name
+        new_user.psu_identity = psu_identity
+
+        raise ErrorCreatingUser unless new_user.valid?


Why not just add the errors here directly instead of raise/catch?

I could add errors here, but I still need to raise because I need a way to short-circuit the #save method if a User cannot be created, i.e. the calling method should not proceed to try to build a DeputyAssignment if the deputy User is invalid and can't be saved.

Now Avdi Grimm claims that you should never use exceptions for control flow and that you should use try / catch for this purpose instead. The thing is, though, I've never seen anyone actually ever do that. 🤣

awead · 2021-11-11T21:48:26Z

app/forms/new_deputy_assignment_form.rb

+      errors.add(:base, :identity_service_error)
+    rescue IdentityNotFound
+      errors.add(:deputy_webaccess_id, :not_found)
+    rescue AlreadyAssigned


this line isn't covered.

spec/component/forms/new_deputy_assignment_form_spec.rb

app/forms/new_deputy_assignment_form.rb

awead · 2021-11-11T22:00:17Z

app/forms/new_deputy_assignment_form.rb

+      errors.add(:base, :error_creating_user)
+    rescue StandardError
+      errors.add(:base, :unknown_error)
+    end


Rescuing all the errors strikes me as a bit of a code smell. Could we do the same thing with nil checks, which are faster, and then add the errors as we go?

This line exists to catch network errors coming from the PsuIdentity gem. Network timeouts, 4xx responses, etc, which could not be handled by nil checks. As best I could tell, that gem does not have a base class for its errors, and right now uses Faraday (which does have a base class). However, that gem could easily not use Faraday in future versions, so it would be foolish to catch Faraday error classes here. If PsuIdentity had its own base class for errors, that's what I'd use on this line

psu-libraries/psu_identity#12

No longer uses exceptions for control flow

rschenk · 2021-11-12T19:19:20Z

Relying on raising and catching exceptions is thornier than using errors directly

Adam, I refactored that class to not use (as many) catches, give the new version a perusal

ajkiessl

Good 2 go.

rschenk requested review from awead, EricDurante and ajkiessl November 11, 2021 19:47

rschenk commented Nov 11, 2021

View reviewed changes

awead suggested changes Nov 11, 2021

View reviewed changes

rschenk added 11 commits November 12, 2021 14:14

DeputyAssignments are unique only when is_active: true

aa4eda8

Install ViewComponent gem

5a4e3d8

niftany allows BEM css syntax

f8c47e1

ViewComponent for DeputyAssignments

543fa3b

Form object to create a new DeputyAssignment

4f306b0

Service to delete/deactivate a DeputyAssignment

dd78875

Controller/views for managing DeputyAssignments

0fb5a29

Refactor NewDeputyAssignmentForm to be less complex

67d1586

Update some language around proxies per feedback

4a1343f

Add a missing condition to NewDeputyAssignmentForm spec

33323cd

Refactor NewDeputyAssignmentForm

4aae43f

No longer uses exceptions for control flow

rschenk force-pushed the 269-user-delegate-ui branch from 70d057f to 4aae43f Compare November 12, 2021 19:16

rschenk requested a review from awead November 12, 2021 19:19

awead approved these changes Nov 15, 2021

View reviewed changes

ajkiessl approved these changes Nov 15, 2021

View reviewed changes

rschenk merged commit cd41550 into main Nov 15, 2021

rschenk deleted the 269-user-delegate-ui branch November 15, 2021 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Users can CRUD deputy assignments #363

Users can CRUD deputy assignments #363

rschenk commented Nov 11, 2021 •

edited

Loading

rschenk Nov 11, 2021

awead left a comment •

edited

Loading

awead Nov 11, 2021

awead Nov 11, 2021

rschenk Nov 12, 2021

awead Nov 11, 2021

rschenk Nov 12, 2021

EricDurante Nov 12, 2021

awead Nov 11, 2021

rschenk Nov 12, 2021

awead Nov 11, 2021

awead Nov 11, 2021

rschenk Nov 12, 2021

awead Nov 15, 2021

rschenk commented Nov 12, 2021

ajkiessl left a comment

Users can CRUD deputy assignments #363

Users can CRUD deputy assignments #363

Conversation

rschenk commented Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

awead left a comment • edited Loading

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

rschenk commented Nov 12, 2021

ajkiessl left a comment

Choose a reason for hiding this comment

rschenk commented Nov 11, 2021 •

edited

Loading

awead left a comment •

edited

Loading