Refactor update owner audit activity #700

slorek · 2020-06-09T18:50:43Z

Description

This is a continuation of the previous refactoring work to:

store metadata about an audit activity in a JSON column rather than an HTML blob in order to render content dynamically
move business logic from callbacks from the Investigation model to a service class
remove the dependency on User.current being set

Additionally, this PR introduces a simple example of a proposed pattern to improve the wizard controllers with the use of form objects for input validation. We still have to persist the previous page's inputs in the session, but the new implementation makes it possible to have multiple tabs open for different cases at the same time.

This owner updated activity was particularly crazy because the new owner ID was stored in the title column (presumably because it was the only place to put it). It made no sense, but with a metadata object we can store this sensibly.

I fixed an apparent issue when the app sent notifications of the case owner being updated. Previously if a team had a email address set it was ignored and each user was emailed individually. Now it will just email the team's address if set.

I also fixed a number of issues with user names being displayed without passing in the current_user, which meant it fell back to User.current. This necessitated the activity title, and subtitle needing the user to be supplied as an argument. This is only needed on a couple of activity types, but unfortunately because of the inheritance there is a need to have a consistent interface. This is mostly a short-term solution which can be improved on later.

The current form also shows inactive users as potential case owners for selection, and also emails inactive users when the case is updated. I've fixed these bugs.

Checklist:

Have you documented your changes in the pull request description?
Has acceptance criteria been tested by a peer?

codeclimate · 2020-06-09T20:04:54Z

psd-web/app/services/change_case_owner.rb

+
+  delegate :investigation, :owner, :rationale, :user, :old_owner, to: :context
+
+  def call


Method call has a Cognitive Complexity of 8 (exceeds 5 allowed). Consider refactoring.

I don't really see the complexity - probably moving the required params checking out will reduce this score but I think it will be more difficult to follow, and it's not a pattern I want to establish.

slorek · 2020-06-10T13:04:16Z

psd-web/app/controllers/investigations/ownership_controller.rb

@@ -46,7 +41,12 @@ def authorize_user
    authorize @investigation, :change_owner_or_status?
  end

-  def owner_params
+  def session_store_key
+    "update_case_owner_#{@investigation.pretty_id}_params"


Previously the key was :owner_id which is too generic. We now use the form name and case ID in the key so that the user could have multiple forms open without them conflicting.

slorek · 2020-06-10T13:07:27Z

psd-web/app/models/activity.rb

+  end
+
+  def subtitle(user)
+    "#{subtitle_slug} by #{source&.show(user)}, #{pretty_date_stamp}"


Passing user in here eliminates a dependency on User.current.

Suggestion: rename this viewer or similar, to make it a bit clearer who/what the user is?

slorek · 2020-06-10T13:08:17Z

psd-web/app/models/audit_activity/investigation/automatically_update_owner.rb

    type = investigation.case_type.capitalize
-    new_owner = (User.find_by(id: owner_id) || Team.find_by(id: owner_id))&.decorate&.display_name
+    new_owner = owner.decorate.display_name(viewer: user)


Passing user in here eliminates a dependency on User.current.

slorek · 2020-06-10T13:12:45Z

psd-web/app/models/investigation.rb

@@ -96,8 +97,7 @@ def important_owner_people

  def past_owners


This now returns both teams and users; it is only actually used in one helper method.

slorek · 2020-06-10T13:14:26Z

psd-web/app/services/change_case_owner.rb

+    entities = [owner]
+    entities << old_owner if old_owner != user
+
+    entities.map { |entity|


It might be nice to extract this logic out to the Team class in a later refactor.

psd-web/config/locales/en.yml

slorek · 2020-06-10T13:20:07Z

psd-web/db/migrate/20200610111320_move_update_owner_activity_metadata.rb

@@ -0,0 +1,33 @@
+class MoveUpdateOwnerActivityMetadata < ActiveRecord::Migration[5.2]


I've tested this locally and originally intended it to be a Rake task but that would require taking the service into maintenance mode; I don't think it's necessary and this allows for the quickest deployment. Also there is precedent in our project for using migrations with no schema changes.

frankieroberto

Nice work! Approved as I've just added comments / suggestions (a couple of which apply more widely than this PR).

frankieroberto · 2020-06-11T08:34:11Z

psd-web/app/models/activity.rb

+  end
+
+  def subtitle(user)
+    "#{subtitle_slug} by #{source&.show(user)}, #{pretty_date_stamp}"


Suggestion: rename this viewer or similar, to make it a bit clearer who/what the user is?

frankieroberto · 2020-06-11T08:41:45Z

psd-web/app/models/audit_activity/investigation/automatically_update_owner.rb

+  def owner
+    User.find_by(id: metadata["owner_id"]) || Team.find_by(id: metadata["owner_id"])
+  end


I wonder if we could avoid this by storing the owner team ID and the owner user ID under separate metadata keys? Especially as we'll soon be always storing the team ID as the "owner" on a case, with an optional user id as "assigned officer"?

I also thought storing the owner_type could avoid it.

I had the same thought initially, but then remembered that we had planned to change "owner" to always be the team, and optionally additionally store the user. Don't think we've done that on the investigation model yet though.

We would still need to store historical audit activity where it could be a user.

We could potentially backfill those (by looking up the current team for that user), which is what I think we're planning to do for cases?

Happy to leave this to a follow-up PR though.

I started looking at this but I quickly realised it doesn't make a lot of sense to do this until the change to always be a team. Currently it can be either, and we can easily migrate the data at a later date.

frankieroberto · 2020-06-11T08:47:56Z

psd-web/app/services/change_case_owner.rb

+    context.fail!(error: "No investigation supplied") unless investigation.is_a?(Investigation)
+    context.fail!(error: "No owner supplied") unless owner.is_a?(User) || owner.is_a?(Team)
+    context.fail!(error: "No user supplied") unless user.is_a?(User)


We’ve done this elsewhere, and I can see how it could be helpful, but it feels a bit un-Rubyish to do this kind of type checking and defensive programming (until Ruby gets type annotations?). I don't think we have any code which relies on these failures? We could considering dropping these lines, particularly if the method throws an exception if these are nil later on in the execution?

Happy to keep them if we decide they're helpful though.

My concern is the input parameters aren't documented anywhere (normally you'd at least have a list of arguments to the method).

Yeah, this is one think I dislike about the Interactor gem. It’d be nice if you could define required keyword arguments in the call method. It looks like there’s an open pull request to add this, but it's not been merged yet.

Let's see if we can come up with a better pattern which we can apply consistently in a follow up PR.

frankieroberto · 2020-06-11T08:50:48Z

psd-web/app/services/change_case_owner.rb

+      investigation.owner = owner
+      investigation.save!


Suggestion: combine with:

investigation.update!(owner: owner)

frankieroberto · 2020-06-11T08:53:27Z

psd-web/app/services/change_case_owner.rb

+  def email_body(viewer = nil)
+    user_name = user.decorate.display_name(viewer: viewer)
+    body = "Case owner changed on #{investigation.case_type} to #{owner.decorate.display_name(viewer: viewer)} by #{user_name}."
+    body << "\n\nMessage from #{user_name}: #{inset_text_for_notify(rationale)}" if rationale
+    body
+  end


Bit shocked that this doesn’t get built within the mailer job itself! One for a future refactor?

Yes I thought the same. Probably need a more tailored mailer method.

psd-web/config/locales/en.yml

frankieroberto · 2020-06-11T09:03:42Z

psd-web/app/models/audit_activity/investigation/update_owner.rb

+  def self.build_metadata(owner, rationale)
+    {
+      owner_id: owner.id,
+      rationale: rationale
+    }


I wonder if we should also store the owner’s name, to guard against the user or team being deleted in future, and so that we can display the user/team’s name at the time they became the new owner? I think that’s what you'd expect to see when reading through the timeline of events, although it’s hard to know for sure. @tobyhughes @edwardhorsford?

Unsure! We're disabling users, not deleting them - so this would mean name updates get reflected. On the other hand we could take the view that these audit entries are a point in time snapshot so could just store the value of the data at that time.

Users should not be deleted entirely, just marked with a deleted flag. Their names and team membership can change over time. Storing their team as well might be a good shout, but this wouldn't be used anywhere currently and we'd have to do some work refactoring the way we display activities to actually use it.

I think displaying the name at-the-time would probably be ideal. But it's also not really essential, so happy to park this for now.

I think this'll need a fair bit of re-jigging which is going to tie this up for a while, would be pleased to see a follow up PR for this though.

frankieroberto · 2020-06-11T10:04:29Z

psd-web/app/models/audit_activity/investigation/automatically_update_owner.rb

+  def self.build_metadata(owner)
+    {
+      owner_id: owner.id
+    }
  end


We have this pattern elsewhere, but I wonder whether we could avoid the class method and just pass the owner_id when initializing the new object? (ie so you can do ActivityClass.create!(owner_id: owner_id, ...) rather than having to build the metadata object first?

Some possible implementations:

class AuditActivity::Investigation::AutomaticallyUpdateOwner < AuditActivity::Investigation::Base attr_writer :owner_id def metadata read_attribute(:metadata) || { owner_id: @owner_id } end end

or overriding the initialiser:

class AuditActivity::Investigation::AutomaticallyUpdateOwner < AuditActivity::Investigation::Base attr_writer :owner_id def initialize(*args) super(args) return unless new_record? self.metadata = { owner_id: owner_id } end end

or using the activerecord after_initialize callback to avoid overriding:

class AuditActivity::Investigation::AutomaticallyUpdateOwner < AuditActivity::Investigation::Base attr_writer :owner_id after_initialize :set_metadata def set_metadata return unless new_record? self.metadata = { owner_id: @owner_id } end end

Worth exploring?

Yes that's an interesting idea. I think at the moment where we have dozens of classes inheriting from Activity there should be some consistent interface, but as we move over to services perhaps this is less important as I think it will only be accessed from the related service class. 🤔

I'm going to park this idea for this PR - we should try it separately and see how it looks.

…llbacks and dependency on User.current

* origin/master: Bump rubocop and rubocop-govuk in /psd-web (#704) Only show complainant details on case added activity if user is collaborating on the case (#696) Fix error when trying to display automatically updated case ownership change activity (#708) Remove User.current dependency from display_name and show methods (#701) Refactor update owner audit activity to store metadata, and remove callbacks and dependency on User.current (#700)

opss-bot temporarily deployed to review-app-700 June 9, 2020 18:51 Inactive

slorek force-pushed the activity_callbacks branch from 29e4c8b to 1ba2a97 Compare June 9, 2020 18:52

opss-bot temporarily deployed to review-app-700 June 9, 2020 18:53 Inactive

slorek force-pushed the activity_callbacks branch from 1ba2a97 to 3ae9fc6 Compare June 9, 2020 19:33

opss-bot temporarily deployed to review-app-700 June 9, 2020 19:33 Inactive

slorek force-pushed the activity_callbacks branch from 3ae9fc6 to 4108690 Compare June 9, 2020 19:37

opss-bot temporarily deployed to review-app-700 June 9, 2020 19:37 Inactive

slorek force-pushed the activity_callbacks branch from 4108690 to 1f9c7f3 Compare June 9, 2020 20:01

opss-bot temporarily deployed to review-app-700 June 9, 2020 20:02 Inactive

OfficeForProductSafetyAndStandards deleted a comment from codeclimate bot Jun 9, 2020

slorek force-pushed the activity_callbacks branch from 1f9c7f3 to 292d722 Compare June 9, 2020 20:04

opss-bot temporarily deployed to review-app-700 June 9, 2020 20:04 Inactive

codeclimate bot reviewed Jun 9, 2020

View reviewed changes

slorek force-pushed the activity_callbacks branch from 292d722 to 4891c92 Compare June 10, 2020 11:27

opss-bot temporarily deployed to review-app-700 June 10, 2020 11:27 Inactive

slorek commented Jun 10, 2020

View reviewed changes

psd-web/config/locales/en.yml Show resolved Hide resolved

slorek commented Jun 10, 2020

View reviewed changes

slorek self-assigned this Jun 10, 2020

slorek marked this pull request as ready for review June 10, 2020 13:24

slorek force-pushed the activity_callbacks branch from 4891c92 to 164a1bd Compare June 10, 2020 13:25

opss-bot temporarily deployed to review-app-700 June 10, 2020 13:25 Inactive

slorek mentioned this pull request Jun 10, 2020

Remove User.current from display name #701

Merged

frankieroberto approved these changes Jun 11, 2020

View reviewed changes

slorek force-pushed the activity_callbacks branch from 164a1bd to 9975282 Compare June 11, 2020 16:28

opss-bot temporarily deployed to review-app-700 June 11, 2020 16:28 Inactive

Refactor update owner audit activity to store metadata, and remove ca…

642bdf0

…llbacks and dependency on User.current

slorek force-pushed the activity_callbacks branch from 9975282 to 642bdf0 Compare June 11, 2020 16:46

opss-bot temporarily deployed to review-app-700 June 11, 2020 16:46 Inactive

slorek merged commit 847d58e into master Jun 11, 2020

slorek deleted the activity_callbacks branch June 11, 2020 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor update owner audit activity #700

Refactor update owner audit activity #700

slorek commented Jun 9, 2020 •

edited

Loading

codeclimate bot Jun 9, 2020

slorek Jun 10, 2020 •

edited

Loading

slorek Jun 10, 2020 •

edited

Loading

slorek Jun 10, 2020

frankieroberto Jun 11, 2020

slorek Jun 10, 2020

slorek Jun 10, 2020

slorek Jun 10, 2020 •

edited

Loading

slorek Jun 10, 2020

frankieroberto left a comment

frankieroberto Jun 11, 2020

frankieroberto Jun 11, 2020

slorek Jun 11, 2020

frankieroberto Jun 11, 2020

slorek Jun 11, 2020

frankieroberto Jun 11, 2020

slorek Jun 11, 2020

frankieroberto Jun 11, 2020

slorek Jun 11, 2020

frankieroberto Jun 11, 2020

slorek Jun 11, 2020

frankieroberto Jun 11, 2020

frankieroberto Jun 11, 2020

slorek Jun 11, 2020

frankieroberto Jun 11, 2020

edwardhorsford Jun 11, 2020

slorek Jun 11, 2020

frankieroberto Jun 11, 2020

slorek Jun 11, 2020

frankieroberto Jun 11, 2020

slorek Jun 11, 2020

slorek Jun 11, 2020


		delegate :investigation, :owner, :rationale, :user, :old_owner, to: :context

		def call

		@@ -0,0 +1,33 @@
		class MoveUpdateOwnerActivityMetadata < ActiveRecord::Migration[5.2]

Refactor update owner audit activity #700

Refactor update owner audit activity #700

Conversation

slorek commented Jun 9, 2020 • edited Loading

Description

Checklist:

codeclimate bot Jun 9, 2020

Choose a reason for hiding this comment

slorek Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

slorek Jun 10, 2020 • 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

slorek Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankieroberto 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

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

slorek commented Jun 9, 2020 •

edited

Loading

slorek Jun 10, 2020 •

edited

Loading

slorek Jun 10, 2020 •

edited

Loading

slorek Jun 10, 2020 •

edited

Loading