department-of-veterans-affairs / caseflow

Caseflow is a web application that enables the tracking and processing of appealed claims at the Board of Veterans' Appeals.
Other
54 stars 18 forks source link

Write tech spec describing how to mark Caseflow users as inactive #11631

Closed lowellrex closed 5 years ago

lowellrex commented 5 years ago

Rows in Caseflow's users table should never be deleted* from the database. This allows us to keep a record of users as long as the application lives. However, this practice of persisting User records in perpetuity presents some challenges with tasks. Two salient challenges are 1) people who are no longer working on Caseflow (perhaps because they are no longer employed at the VA or at a VSO) can still be assigned tasks and 2) we have no insight into how many active tasks are assigned to people who will never act on those tasks.

These challenges were identified in a related Slack conversation and this ticket exists to explore how we can solve these problems and other problems that stem from the fact that we do not identify inactive Caseflow users.

Acceptance criteria

Estimating this ticket at a 3 because that is our practice for all tickets where the end result is a tech spec.

*On rare occasions we have deleted rows from the users table. One example that comes to mind is when we consolidated duplicate rows in that table.

pkarman commented 5 years ago

fwiw I'd vote for the "soft delete" approach we follow in HearingDay with the acts_as_paranoid gem. It has some drawbacks, but I would enjoy the consistency of it in the codebase.

msj0nes commented 5 years ago

Idea. If it ever boils down to a manual process where the Board has to notify us of staff leaving then it would be cool to have a ‘admin panel’ similar to ‘team management’ where we add the user/CSS ID.

Adding the user/css ID to the listing would:

The VLJ from there makes the decision on what happens next.

hschallhorn commented 5 years ago

Things we're trying to solve

  1. Develop a way to determine if a user is inactive
  2. Make sure tasks aren't assigned to inactive users
  3. Develop a way to mark a user inactive
  4. Handle tasks assigned to user when they become inactive (this is similar to when a user leaves an org, which we also don't handle) (may want to spin out to a different spec to keep this at a 3!)

Possible Solutions

Solution 1 - Add status column to users table

Add a status column to the user's table. When assigning tasks only show "active" users. Add a uses management page to show all users and their status (in different sections or sorted by active/inactive) with the option to mark them inactive (or active again).

What this solution would touch

  1. Create a migration to add status and status_changed_at column to the users table with the default status as "active" and add an index on status as we will frequently be requesting only users with a status of "active". Add an enum for all the available user statuses ('active' and 'inactive' to start until design or product indicate we need others?)
  2. Scope for each status is automatically created due to rails magic active record enums. Places we would want to return only active users:
    • task_action_repository/assign_to_user_data
    • any assignment that uses Judge.list_all (returns Users, so the scope should work)
    • any assignment that uses Attorney.list_all (returns Users, so the scope should work) On task creation, validate that the assigned user has an active status.
  3. Create a new endpoint to manage all users. Users who can access this endpoint should be limited to members of the BVA admin team. Add option in users_controller to fetch all users (this should include users of all statuses). Add endpoint to users_controller (not within organizations/users_controller) to update the status and status_updated_at columns for that user. Add front end page that displays all users and their status. Add a button, confirmation, and success alert that hits the update user's status endpoint. (If we ever up the number of statuses to >2, we will require either a modal or dropdown rather than an "Activate" or "Deactivate" button)

Solution 2 - acts_as_paranoid

The paranoia gem that we use to soft delete HearingDays and WorksheetIssues would allow us to "delete" a user without removing it from our database. By adding acts_as_paranoid to the user model, any calls to fetch a user would only return the non "deleted" users.

What this solution would touch

The implementation would be very similar to the one above

  1. Create migration to add deleted_at to the users table. Add acts_as_paranoid without_default_scope: true to the user model.
  2. In places we want to filter out deleted users, add the scope without_deleted. On task creation, we would still need to validate user.deleted? as the default scope is to return all users.
  3. Create a new endpoint to manage all users. Users who can access this endpoint should be limited to members of the BVA admin team. Add option in users_controller to fetch all users. Be sure to serialize user.deleted? to handle both options on the front end. Add endpoint to users_controller (not within organizations/users_controller) to soft delete (user.destroy) or restore (user.restore) that user. Add front end page that displays all users and their deleted status. Add button, confirmation, and success alert that hits the delete or restore user endpoint.

Thoughts

  1. As we were planning on doing a slow rollout of only returning active users to certain known workflows and others when the need arose, we would probably need to use acts_as_paranoid without_default_scope: true to avoid any unintended consequences to existing code
  2. Without the default scope, I think we lose almost all advantages of using act_as_paranoid other than not writing our own scope and one fewer columns in the users table.
  3. As we had plans to still use inactive/deleted users (eg. in a users management page) I think active/inactive is a better naming convention than without_deleted/with_deleted, especially if this functionality gets used for users who are expected to become active again (people going on extended vacations or maternity/paternity leave)
  4. Maybe not as important (or even at all), but paranoia relies on the deleted_at column being nil to know what users are active, so we would not be able to use this column to track when a user’s active status was reinstated.
  5. Going the first route will allow us to use an enum status column to allow us to represent other states

For these reasons we will be moving forward with Solution 1

Other actions

hschallhorn commented 5 years ago

Compiling a list of places we may want to add an active scope to or validation that a user is active:

Wow bowing out for the day once I decided to look at organization.users. Other places I still had yet to glance at:

Other finds:

lpciferri commented 5 years ago

Next step is to create engineering tickets that spawn from this tech spec cc: @hschallhorn

Proposed user flow: On Caseflow team management page, team admin searches for user and then marks them inactive.