Ability to delete a team #877

slorek · 2020-09-11T10:53:12Z

https://trello.com/c/FRRtOhuq/668-5-absorb-opss-product-safety-enforcement-into-opss-enforcement

Description

This allows us to delete a team from service.

When a team is deleted:

all users on that team are transferred to another designated team. Any users that own a case retain ownership. All user roles are transferred to the new team
all case collaborations for that team are transferred to another designated team. If the team owns any cases, the designated team will inherit ownership.
audit activity is generated as normal - except when a user on the deleted team is the owner of a case. In this event we add an extra activity showing the team ownership has changed
no emails are sent
the team will no longer be in the filter cases list
the team will no longer appear in the list to transfer case ownership
the team will no longer appear in the list to add a team to the case

In our initial use case this process would ordinarily generate many hundreds of emails to the affected users. This implementation suppresses those emails. This could be made optional in a future iteration.

Checklist:

Have you documented your changes in the pull request description?
Does the change present any security considerations?
Is any gem functionality overloaded? Eg: Devise controller methods being overloaded.
Has acceptance criteria been tested by a peer?
Has the CHANGELOG been updated? (If change is worth telling users about.)

General testing (author)

Test without JavaScript
Test on small screen

Accessibility testing (author)

Works keyboard only
Tested with one screen reader
Zoom page to 400% - content still visible
Disable CSS - does content make sense and appear in a logical order?

codeclimate · 2020-09-11T14:41:25Z

app/services/delete_team.rb

+
+  delegate :team, :new_team, :user, to: :context
+
+  def call


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

slorek · 2020-09-14T14:47:59Z

.rubocop.yml

@@ -36,6 +36,9 @@ Style/TrailingCommaInArguments:
 Rails/HelperInstanceVariable:
  Enabled: false

+Lint/AmbiguousBlockAssociation:


Disabled this because fixing it introduces syntax errors: rubocop/rubocop#4222

slorek · 2020-09-21T11:05:38Z

app/helpers/investigations/user_filters_helper.rb

@@ -1,6 +1,6 @@
 module Investigations::UserFiltersHelper
  def entities
-    User.get_owners(except: current_user).decorate + Team.all_with_organisation.decorate


We no longer need the team organisation. This was previously used in the display_name method but was changed a while ago to no longer require it.

app/services/delete_team.rb

frankieroberto · 2020-09-21T13:11:13Z

app/services/delete_team.rb

+class DeleteTeam
+  include Interactor
+
+  delegate :team, :new_team, :user, to: :context


Suggest rewording this to be clearer that it’s deleting a team and merging all its users & cases into another team:

Suggested change

class DeleteTeam

include Interactor

delegate :team, :new_team, :user, to: :context

class MergeTeams

include Interactor

delegate :team_to_delete, :team_to_keep, :user, to: :context

🤔 not sure about this. The merging is the thing you have to do to accomplish the team deletion.

frankieroberto · 2020-09-21T13:18:53Z

app/services/delete_team.rb

+  def change_case_owner_to_new_team(collaboration)
+    ChangeCaseOwner.call!(
+      investigation: collaboration.investigation,
+      owner: new_team,
+      user: user,
+      rationale: change_case_owner_rationale,
+      silent: true
+    )
+  end
+
+  def change_case_owner_team(collaboration)
+    collaboration.update!(collaborator_id: new_team.id)
+
+    metadata = update_owner_activity_class.build_metadata(new_team, change_case_owner_rationale)
+
+    update_owner_activity_class.create!(
+      source: UserSource.new(user: user),
+      investigation: collaboration.investigation,
+      title: nil,
+      body: nil,
+      metadata: metadata
+    )
+  end


These method names are so similar it's kinda hard to follow. I think I understand what’s going on, but it's taken a bit of head-scratching. I don't have any immediate suggestions on how to make it clearer though. 😬

Agree. I'll try to think of better names.

frankieroberto · 2020-09-21T13:26:05Z

db/migrate/20200911142816_add_deleted_at_to_teams.rb

+
+  def change
+    add_column :teams, :deleted_at, :datetime
+    add_index :teams, :deleted_at, algorithm: :concurrently


I wonder if a partial index would be helpful here, as we'd normally be querying for non-deleted teams?

Suggested change

add_index :teams, :deleted_at, algorithm: :concurrently

add_index :teams, :deleted_at, name: 'not_deleted_index', where: {deleted_at: nil), algorithm: :concurrently

(Probably marginal gains at our scale though)

frankieroberto · 2020-09-21T13:32:33Z

spec/features/add_team_to_case_spec.rb

+    # Check deleted teams are not listed
+    expect(page).to have_css("#team option[value=\"#{team.id}\"]")
+    expect(page).not_to have_css("#team option[value=\"#{deleted_team.id}\"]")


You could use the Capybara method here to check the values of the select?

Suggested change

# Check deleted teams are not listed

expect(page).to have_css("#team option[value=\"#{team.id}\"]")

expect(page).not_to have_css("#team option[value=\"#{deleted_team.id}\"]")

# Check deleted teams are not listed

expect(page).not_to have_select("Select other team name", with_options: [deleted_team.name])

expect(page).to have_select("Select other team name", with_options: [team.name])

nasirkhanbeis

looks good as expected!

StupidCodeFactory · 2020-09-22T09:59:58Z

app/services/delete_team.rb

+    context.fail!(error: "No team supplied") unless team.is_a?(Team)
+    context.fail!(error: "No new team supplied to absorb cases and users") unless new_team.is_a?(Team)
+    context.fail!(error: "No user supplied") unless user.is_a?(User)
+    context.fail!(error: "Team cannot already be deleted") if team.deleted?


Suggested change

context.fail!(error: "Team cannot already be deleted") if team.deleted?

context.fail!(error: "Team already deleted") if team.deleted?

StupidCodeFactory · 2020-09-22T10:00:25Z

app/services/delete_team.rb

+    context.fail!(error: "No new team supplied to absorb cases and users") unless new_team.is_a?(Team)
+    context.fail!(error: "No user supplied") unless user.is_a?(User)
+    context.fail!(error: "Team cannot already be deleted") if team.deleted?
+    context.fail!(error: "New team cannot already be deleted") if new_team.deleted?


Suggested change

context.fail!(error: "New team cannot already be deleted") if new_team.deleted?

context.fail!(error: "New team already be deleted") if new_team.deleted?

StupidCodeFactory · 2020-09-22T10:03:33Z

app/services/delete_team.rb

+  end
+
+  def new_team_already_collaborating?(investigation)
+    investigation.collaboration_accesses.find_by(collaborator_id: new_team.id).present?


Suggested change

investigation.collaboration_accesses.find_by(collaborator_id: new_team.id).present?

investigation.collaboration_accesses.where(collaborator: new_team).exists?

StupidCodeFactory · 2020-09-22T10:05:57Z

lib/tasks/delete_team.rake

+namespace :team do
+  desc "Marks the given team as deleted, assigning their cases and users to another team"
+  task delete: :environment do
+    team = Team.find(ENV.fetch("ID", nil))


Suggested change

team = Team.find(ENV.fetch("ID", nil))

team = Team.find(ENV["ID"])

StupidCodeFactory · 2020-09-22T10:06:29Z

doc/tasks.md

+cf run-task psd-web "export \$(./env/get-env-from-vcap.sh) && ID=<id> NEW_TEAM_ID=<id> EMAIL=<email address> rake team:delete" --name <task name>
+```
+
+Case collaborations and users which belong to the team identified by `ID` will be migrated to another team identified by `NEW_TEAM`. The user identified by `EMAIL` will be attributed to the change on all relevant audit activity.


Suggested change

Case collaborations and users which belong to the team identified by `ID` will be migrated to another team identified by `NEW_TEAM`. The user identified by `EMAIL` will be attributed to the change on all relevant audit activity.

Case collaborations and users which belong to the team identified by `ID` will be migrated to another team identified by `NEW_TEAM_ID`. The user identified by `EMAIL` will be attributed to the change on all relevant audit activity.

StupidCodeFactory · 2020-09-22T10:10:59Z

spec/features/filter_investigations_spec.rb

    expect(page).to have_css("#created_by_someone_else_id option[value=\"#{another_active_user.id}\"]")
    expect(page).not_to have_css("#created_by_someone_else_id option[value=\"#{another_inactive_user.id}\"]")
+    expect(page).not_to have_css("#created_by_someone_else_id option[value=\"#{other_deleted_team.id}\"]")


Same as @frankieroberto comment above, you could use the have_select(team.name), that would also test that the value in the drop down is the one team name.

I refactored the other existing expectations as well, looks a lot better.

spec/services/delete_team_spec.rb

StupidCodeFactory · 2020-09-22T10:15:03Z

spec/services/delete_team_spec.rb

+
+        context "when the new team is not already a collaborator on the case" do
+          it "adds the new team to the case with the same access level as the old team" do
+            expect { delete_team }.to change { other_team_case.reload.teams_with_read_only_access }.from([team]).to([new_team])


Suggested change

expect { delete_team }.to change { other_team_case.reload.teams_with_read_only_access }.from([team]).to([new_team])

expect { delete_team }.to change(other_team_case, :teams_with_read_only_access).from([team]).to([new_team])

Doesn't work. Need reload.

opss-bot temporarily deployed to review-app-877 September 11, 2020 10:53 Inactive

opss-bot temporarily deployed to review-app-877 September 11, 2020 12:36 Inactive

opss-bot temporarily deployed to review-app-877 September 11, 2020 14:40 Inactive

codeclimate bot reviewed Sep 11, 2020

View reviewed changes

opss-bot temporarily deployed to review-app-877 September 14, 2020 14:34 Inactive

opss-bot temporarily deployed to review-app-877 September 14, 2020 14:47 Inactive

slorek commented Sep 14, 2020

View reviewed changes

opss-bot temporarily deployed to review-app-877 September 15, 2020 10:21 Inactive

slorek force-pushed the delete_team branch from b18073e to a2ca43a Compare September 15, 2020 10:24

opss-bot temporarily deployed to review-app-877 September 15, 2020 10:25 Inactive

opss-bot temporarily deployed to review-app-877 September 15, 2020 12:29 Inactive

slorek changed the title ~~WIP: Delete team~~ Ability to delete a team Sep 15, 2020

opss-bot temporarily deployed to review-app-877 September 15, 2020 15:17 Inactive

slorek added 12 commits September 21, 2020 12:01

Test behaviour skeleton for DeleteTeam service

80a4e02

Modify test scenarios

6b494db

DeleteTeam service tests implementation

f7abfe1

Implement team deletion

0a84707

Refactor

a343c6b

Rubocop fixes

6345a85

Don't show deleted team in the case filters

5f4736f

Don't show deleted teams on change case owner form

3e2b619

Don't show deleted team on add collaborator form

e536cc9

Remove test for email confirmation

11ef614

Add activity when changing case owner team but not ultimate case owner

2a67897

Delete team Rake task

74d8f7c

slorek force-pushed the delete_team branch from 023d1c9 to 74d8f7c Compare September 21, 2020 11:02

opss-bot temporarily deployed to review-app-877 September 21, 2020 11:03 Inactive

slorek commented Sep 21, 2020

View reviewed changes

New Rubocop fixes

b78c509

opss-bot temporarily deployed to review-app-877 September 21, 2020 11:14 Inactive

slorek marked this pull request as ready for review September 21, 2020 11:14

slorek requested review from frankieroberto, StupidCodeFactory and nasirkhanbeis September 21, 2020 11:15

slorek self-assigned this Sep 21, 2020

opss-bot temporarily deployed to review-app-877 September 21, 2020 11:17 Inactive

codeclimate bot reviewed Sep 21, 2020

View reviewed changes

app/services/delete_team.rb Outdated Show resolved Hide resolved

slorek force-pushed the delete_team branch from 5715512 to b78c509 Compare September 21, 2020 11:19

opss-bot temporarily deployed to review-app-877 September 21, 2020 11:19 Inactive

frankieroberto approved these changes Sep 21, 2020

View reviewed changes

nasirkhanbeis approved these changes Sep 21, 2020

View reviewed changes

StupidCodeFactory approved these changes Sep 22, 2020

View reviewed changes

Improvements following review feedback

d24959e

opss-bot temporarily deployed to review-app-877 September 22, 2020 14:15 Inactive

Merge branch 'master' into delete_team

2ac0f76

opss-bot temporarily deployed to review-app-877 September 23, 2020 09:14 Inactive

slorek merged commit 90367f7 into master Sep 23, 2020

slorek deleted the delete_team branch September 23, 2020 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to delete a team #877

Ability to delete a team #877

slorek commented Sep 11, 2020 •

edited

codeclimate bot Sep 11, 2020

slorek Sep 14, 2020

slorek Sep 21, 2020

frankieroberto Sep 21, 2020

slorek Sep 21, 2020

frankieroberto Sep 21, 2020

slorek Sep 21, 2020 •

edited

frankieroberto Sep 21, 2020

frankieroberto Sep 21, 2020

nasirkhanbeis left a comment

StupidCodeFactory Sep 22, 2020

StupidCodeFactory Sep 22, 2020

StupidCodeFactory Sep 22, 2020

StupidCodeFactory Sep 22, 2020

StupidCodeFactory Sep 22, 2020

StupidCodeFactory Sep 22, 2020

slorek Sep 22, 2020

StupidCodeFactory Sep 22, 2020

slorek Sep 22, 2020

	add_index :teams, :deleted_at, algorithm: :concurrently
	add_index :teams, :deleted_at, name: 'not_deleted_index', where: {deleted_at: nil), algorithm: :concurrently

	context.fail!(error: "Team cannot already be deleted") if team.deleted?
	context.fail!(error: "Team already deleted") if team.deleted?

	context.fail!(error: "New team cannot already be deleted") if new_team.deleted?
	context.fail!(error: "New team already be deleted") if new_team.deleted?

	investigation.collaboration_accesses.find_by(collaborator_id: new_team.id).present?
	investigation.collaboration_accesses.where(collaborator: new_team).exists?

	team = Team.find(ENV.fetch("ID", nil))
	team = Team.find(ENV["ID"])

	Case collaborations and users which belong to the team identified by `ID` will be migrated to another team identified by `NEW_TEAM`. The user identified by `EMAIL` will be attributed to the change on all relevant audit activity.
	Case collaborations and users which belong to the team identified by `ID` will be migrated to another team identified by `NEW_TEAM_ID`. The user identified by `EMAIL` will be attributed to the change on all relevant audit activity.

	expect { delete_team }.to change { other_team_case.reload.teams_with_read_only_access }.from([team]).to([new_team])
	expect { delete_team }.to change(other_team_case, :teams_with_read_only_access).from([team]).to([new_team])

Ability to delete a team #877

Ability to delete a team #877

Conversation

slorek commented Sep 11, 2020 • edited

Description

Checklist:

General testing (author)

Accessibility testing (author)

codeclimate bot Sep 11, 2020

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 Sep 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nasirkhanbeis 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

slorek commented Sep 11, 2020 •

edited

slorek Sep 21, 2020 •

edited