add workflow_state to account_users

fixes CNVS-38231

test plan
 - delete an admin
 - admin should be gone from every where
 - admin should not have permissions to do stuff

Change-Id: I56c90a12b2be879c5d646c1ab6980693ff161673
Reviewed-on: https://gerrit.instructure.com/119220
Reviewed-by: Tyler Pickett <tpickett@instructure.com>
Tested-by: Rob Orton <rob@instructure.com>
QA-Review: Jeremy Putnam <jeremyp@instructure.com>
Product-Review: Rob Orton <rob@instructure.com>

app/controllers/accounts_controller.rb

@@ -128,7 +128,7 @@ class AccountsController < ApplicationController
   def index
     respond_to do |format|
       format.html do
-        @accounts = @current_user ? @current_user.all_accounts : []
+        @accounts = @current_user ? @current_user.adminable_accounts : []
       end
       format.json do
         if @current_user
@@ -754,7 +754,7 @@ class AccountsController < ApplicationController
         end
       end
       load_course_right_side
-      @account_users = @account.account_users
+      @account_users = @account.account_users.active
       ActiveRecord::Associations::Preloader.new.preload(@account_users, user: :communication_channels)
       order_hash = {}
       @account.available_account_roles.each_with_index do |role, idx|
@@ -1078,12 +1078,13 @@ class AccountsController < ApplicationController
     admins = users.map do |user|
       admin = @context.account_users.where(user_id: user.id, role_id: role.id).first_or_initialize
       admin.user = user
+      admin.workflow_state = 'active'
       return unless authorized_action(admin, @current_user, :create)
       admin
     end
 
     account_users = admins.map do |admin|
-      if admin.new_record?
+      if admin.new_record? || admin.workflow_state_changed?
         admin.save!
         if admin.user.registered?
           admin.account_user_notification!

app/controllers/admins_controller.rb

@@ -37,7 +37,7 @@
 #           "description": "The user the role is assigned to. See the Users API for details.",
 #           "$ref": "User"
 #         },
-#         "status": {
+#         "workflow_state": {
 #           "description": "The status of the account role/user assignment.",
 #           "type": "string",
 #           "example": "deleted"
@@ -75,10 +75,11 @@ class AdminsController < ApplicationController
 
     require_role
     admin = @context.account_users.where(user_id: user.id, role_id: @role.id).first_or_initialize
+    admin.workflow_state = 'active'
 
     return unless authorized_action(admin, @current_user, :create)
 
-    if admin.new_record?
+    if admin.new_record? || admin.workflow_state_changed?
       if admin.save
         # if they don't provide it, or they explicitly want it
         if params[:send_confirmation].nil? ||
@@ -130,7 +131,7 @@ class AdminsController < ApplicationController
   def index
     if authorized_action(@context, @current_user, :manage_account_memberships)
       user_ids = api_find_all(User, Array(params[:user_id])).pluck(:id) if params[:user_id]
-      scope = @context.account_users
+      scope = @context.account_users.active
       scope = scope.where(user_id: user_ids) if user_ids
       route = polymorphic_url([:api_v1, @context, :admins])
       admins = Api.paginate(scope.order(:id), self, route)

app/controllers/application_controller.rb

@@ -659,7 +659,7 @@ class ApplicationController < ActionController::Base
         @context = api_find(Account, params[:account_id])
         params[:context_id] = @context.id
         params[:context_type] = "Account"
-        @context_enrollment = @context.account_users.where(user_id: @current_user.id).first if @context && @current_user
+        @context_enrollment = @context.account_users.active.where(user_id: @current_user.id).first if @context && @current_user
         @context_membership = @context_enrollment
         @account = @context
       elsif params[:group_id]

app/controllers/communication_channels_controller.rb

@@ -220,7 +220,7 @@ class CommunicationChannelsController < ApplicationController
       @root_account ||= @user.pseudonyms.first.try(:account) if @user.pre_registered?
       @root_account ||= @user.enrollments.first.try(:root_account) if @user.creation_pending?
       unless @root_account
-        account = @user.all_accounts.first
+        account = @user.adminable_accounts.first
         @root_account = account.try(:root_account)
       end
       @root_account ||= @domain_root_account

app/models/account.rb

@@ -826,7 +826,7 @@ class Account < ActiveRecord::Base
   end
 
   def membership_for_user(user)
-    self.account_users.where(user_id: user).first if user
+    self.account_users.active.where(user_id: user).first if user
   end
 
   def available_custom_account_roles(include_inactive=false)
@@ -937,7 +937,7 @@ class Account < ActiveRecord::Base
         @account_users_cache[user.global_id] ||= begin
           all_site_admin_account_users_hash = MultiCache.fetch("all_site_admin_account_users3") do
             # this is a plain ruby hash to keep the cached portion as small as possible
-            self.account_users.inject({}) { |result, au| result[au.user_id] ||= []; result[au.user_id] << [au.id, au.role_id]; result }
+            self.account_users.active.inject({}) { |result, au| result[au.user_id] ||= []; result[au.user_id] << [au.id, au.role_id]; result }
           end
           (all_site_admin_account_users_hash[user.id] || []).map do |(id, role_id)|
             au = AccountUser.new
@@ -956,7 +956,7 @@ class Account < ActiveRecord::Base
         if account_chain_ids == [Account.site_admin.id]
           Account.site_admin.account_users_for(user)
         else
-          AccountUser.where(:account_id => account_chain_ids, :user_id => user).to_a
+          AccountUser.where(:account_id => account_chain_ids, :user_id => user).active.to_a
         end
       end
     end
@@ -964,12 +964,12 @@ class Account < ActiveRecord::Base
     @account_users_cache[user.global_id]
   end
 
-  # returns all account users for this entire account tree
+  # returns all active account users for this entire account tree
   def all_account_users_for(user)
     raise "must be a root account" unless self.root_account?
     Shard.partition_by_shard(account_chain(include_site_admin: true).uniq) do |accounts|
       next unless user.associated_shards.include?(Shard.current)
-      AccountUser.eager_load(:account).where("user_id=? AND (root_account_id IN (?) OR account_id IN (?))", user, accounts, accounts)
+      AccountUser.active.eager_load(:account).where("user_id=? AND (root_account_id IN (?) OR account_id IN (?))", user, accounts, accounts)
     end
   end
 

app/models/account_authorization_config.rb

@@ -231,11 +231,13 @@ class AccountAuthorizationConfig < ActiveRecord::Base
           account.get_account_role_by_name(role_name)
         end.compact
         roles_to_add = roles - existing_account_users.map(&:role)
-        account_users_to_delete = existing_account_users.select { |au| !roles.include?(au.role) }
+        account_users_to_delete = existing_account_users.select { |au| au.active? && !roles.include?(au.role) }
+        account_users_to_activate = existing_account_users.select { |au| au.deleted? && roles.include?(au.role) } 
         roles_to_add.each do |role|
           account.account_users.create!(user: user, role: role)
         end
         account_users_to_delete.each(&:destroy)
+        account_users_to_activate.each(&:reactivate)
       when 'sis_user_id', 'integration_id'
         pseudonym[attribute] = value
       when 'display_name'

app/models/account_user.rb

@@ -36,10 +36,27 @@ class AccountUser < ActiveRecord::Base
 
   alias_method :context, :account
 
+  scope :active, -> { where.not(workflow_state: 'deleted') }
 
+  include Workflow
+  workflow do
+    state :active
+
+    state :deleted do
+      event :reactivate, transitions_to: :active
+    end
+  end
+
+  alias_method :destroy_permanently!, :destroy
+  def destroy
+    return if self.new_record?
+    self.workflow_state = 'deleted'
+    self.save!
+  end
 
   def update_account_associations_if_changed
-    if (self.account_id_changed? || self.user_id_changed?)
+    being_deleted = self.workflow_state == 'deleted' && self.workflow_state_was != 'deleted'
+    if (self.account_id_changed? || self.user_id_changed?) || being_deleted
       if self.new_record?
         return if %w{creation_pending deleted}.include?(self.user.workflow_state)
         account_chain = self.account.account_chain
@@ -157,7 +174,7 @@ class AccountUser < ActiveRecord::Base
   def self.account_ids_for_user(user)
     @account_ids_for ||= {}
     @account_ids_for[user.id] ||= Rails.cache.fetch(['account_ids_for_user', user].cache_key) do
-      AccountUser.for_user(user).map(&:account_id)
+      AccountUser.active.for_user(user).map(&:account_id)
     end
   end
 

app/models/alert.rb

@@ -58,7 +58,7 @@ class Alert < ActiveRecord::Base
     recipients << student_id if include_student
     recipients.concat(Array(teachers)) if teachers.present? && include_teachers
     if context_type == 'Account' && !admin_role_ids.empty?
-      recipients.concat context.account_users.where(:role_id => admin_role_ids).distinct.pluck(:user_id)
+      recipients.concat context.account_users.active.where(:role_id => admin_role_ids).distinct.pluck(:user_id)
     end
     recipients.uniq
   end

app/models/conversation_participant.rb

@@ -37,7 +37,7 @@ class ConversationParticipant < ActiveRecord::Base
   scope :sent, -> { where("visible_last_authored_at IS NOT NULL").order("visible_last_authored_at DESC, conversation_id DESC") }
   scope :for_masquerading_user, lambda { |user|
     # site admins can see everything
-    next all if user.account_users.map(&:account_id).include?(Account.site_admin.id)
+    next all if user.account_users.active.map(&:account_id).include?(Account.site_admin.id)
 
     # we need to ensure that the user can access *all* of each conversation's
     # accounts (and that each conversation has at least one account). so given

app/models/course.rb

@@ -679,7 +679,7 @@ class Course < ActiveRecord::Base
     }
 
     p.dispatch :new_course
-    p.to { self.root_account.account_users }
+    p.to { self.root_account.account_users.active }
     p.whenever { |record|
       record.root_account &&
       ((record.just_created && record.name != Course.default_name) ||
@@ -1492,7 +1492,7 @@ class Course < ActiveRecord::Base
       if account_chain_ids == [Account.site_admin.id]
         Account.site_admin.account_users_for(user)
       else
-        AccountUser.where(:account_id => account_chain_ids, :user_id => user).to_a
+        AccountUser.active.where(:account_id => account_chain_ids, :user_id => user).to_a
       end
     end
     @account_users[user.global_id] ||= []

app/models/lti/lti_user_creator.rb

@@ -101,7 +101,7 @@ module Lti
     def current_account_enrollments()
       unless @current_account_enrollments
         if @canvas_context.respond_to?(:account_chain) && !@canvas_context.account_chain.empty?
-          @current_account_enrollments = @canvas_user.account_users.where(account_id: @canvas_context.account_chain).uniq.to_a
+          @current_account_enrollments = @canvas_user.account_users.active.where(account_id: @canvas_context.account_chain).uniq.to_a
         else
           @current_account_enrollments = []
         end

app/models/user.rb

@@ -118,7 +118,6 @@ class User < ActiveRecord::Base
   has_many :web_conference_participants
   has_many :web_conferences, :through => :web_conference_participants
   has_many :account_users
-  has_many :accounts, :through => :account_users
   has_many :media_objects, :as => :context, :inverse_of => :context
   has_many :user_generated_media_objects, :class_name => 'MediaObject'
   has_many :user_notes
@@ -501,9 +500,7 @@ class User < ActiveRecord::Base
         data[:courses] += Course.select([:id, :account_id]).where(:id => course_ids.to_a).to_a unless course_ids.empty?
 
         data[:pseudonyms] += Pseudonym.active.select([:user_id, :account_id]).distinct.where(:user_id => shard_user_ids).to_a
-        AccountUser.unscoped do
+        data[:account_users] += AccountUser.active.select([:user_id, :account_id]).distinct.where(:user_id => shard_user_ids).to_a
-          data[:account_users] += AccountUser.select([:user_id, :account_id]).distinct.where(:user_id => shard_user_ids).to_a
-        end
       end
       # now make it easy to get the data by user id
       data[:enrollments] = data[:enrollments].group_by(&:user_id)
@@ -888,7 +885,7 @@ class User < ActiveRecord::Base
         user_observer_scope = self.user_observers.shard(self)
         user_observee_scope = self.user_observees.shard(self)
         pseudonym_scope = self.pseudonyms.active.shard(self)
-        account_users = self.account_users.shard(self)
+        account_users = self.account_users.active.shard(self)
         has_other_root_accounts = false
       else
         # make sure to do things on the root account's shard. but note,
@@ -1053,7 +1050,7 @@ class User < ActiveRecord::Base
     can :read_grades
 
     given do |user|
-      self.check_accounts_right?(user, :manage_user_logins) && self.all_accounts.select(&:root_account?).all? {|a| has_subset_of_account_permissions?(user, a) }
+      self.check_accounts_right?(user, :manage_user_logins) && self.adminable_accounts.select(&:root_account?).all? {|a| has_subset_of_account_permissions?(user, a) }
     end
     can :manage_user_details and can :rename and can :read_profile
 
@@ -2579,8 +2576,8 @@ class User < ActiveRecord::Base
     @menu_data = {
       :group_memberships => coalesced_group_memberships,
       :group_memberships_count => cached_group_memberships.length,
-      :accounts => self.all_accounts,
+      :accounts => self.adminable_accounts,
-      :accounts_count => self.all_accounts.length,
+      :accounts_count => self.adminable_accounts.length,
     }
   end
 
@@ -2872,16 +2869,18 @@ class User < ActiveRecord::Base
     associated_shards.select { |shard| shard.in_current_region? || shard.default? }
   end
 
-  def all_accounts
+  def adminable_accounts
-    @all_accounts ||= shard.activate do
+    @adminable_accounts ||= shard.activate do
-      Rails.cache.fetch(['all_accounts', self, ApplicationController.region].cache_key) do
+      Rails.cache.fetch(['adminable_accounts', self, ApplicationController.region].cache_key) do
-        self.accounts.active.shard(in_region_associated_shards).to_a
+        Account.shard(self).active.joins(:account_users).
+          where(account_users: {user_id: self.id}).
+          where.not(account_users: {workflow_state: 'deleted'})
       end
     end
   end
 
   def all_paginatable_accounts
-    ShardedBookmarkedCollection.build(Account::Bookmarker, self.accounts.active)
+    ShardedBookmarkedCollection.build(Account::Bookmarker, self.adminable_accounts)
   end
 
   def all_pseudonyms

app/views/shared/_new_nav_header.html.erb

@@ -53,7 +53,7 @@
               </a>
             </li>
           <% end %>
-          <% if @current_user && @current_user.all_accounts.count > 0 %>
+          <% if @current_user && @current_user.adminable_accounts.count > 0 %>
             <li class="menu-item ic-app-header__menu-list-item <%= ' ic-app-header__menu-list-item--active' if active_path?('/accounts') %>">
               <a id="global_nav_accounts_link" href="/accounts" class="ic-app-header__menu-list-link">
                 <div class="menu-item-icon-container" aria-hidden="true">

app/views/users/show.html.erb

@@ -76,11 +76,11 @@
         </ul>
       </div>
     </div>
-    <% if @user.all_accounts && !@user.all_accounts.empty? %>
+    <% if @user.adminable_accounts && !@user.adminable_accounts.empty? %>
-      <h3><%= t('accounts', 'Accounts') %> <%= count_if_any(@user.all_accounts.count) %></h3>
+      <h3><%= t('accounts', 'Accounts') %> <%= count_if_any(@user.adminable_accounts.count) %></h3>
       <div class="accounts" style="font-size: 1.2em">
         <ul class="unstyled_list context_list" style="margin-left: 5px; font-size: 0.9em; margin-bottom: 10px; max-height: 200px; overflow: auto;">
-          <% @user.all_accounts.each do |account| %>
+          <% @user.adminable_accounts.each do |account| %>
             <% num_enrollments += 1 %>
             <li>
               <a href="<%= url_for(account) %>">

db/migrate/20170716050447_add_workflow_state_to_account_user.rb

@@ -0,0 +1,29 @@
+#
+# Copyright (C) 2017 - present Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+
+class AddWorkflowStateToAccountUser < ActiveRecord::Migration[5.0]
+  tag :predeploy
+  disable_ddl_transaction!
+
+  def change
+    add_column :account_users, :workflow_state, :string
+    change_column_default(:account_users, :workflow_state, 'active')
+    DataFixup::BackfillNulls.run(AccountUser, :workflow_state, default_value: 'active')
+    change_column_null(:account_users, :workflow_state, false)
+    add_index :account_users, :workflow_state, algorithm: :concurrently
+  end
+end

lib/api/v1/admin.rb

@@ -23,12 +23,11 @@ module Api::V1::Admin
   def admin_json(admin, current_user, session)
     # admin is an AccountUser
     {
-      :id => admin.id,
+      id: admin.id,
-      :role => admin.role.name,
+      role: admin.role.name,
-      :role_id => admin.role_id,
+      role_id: admin.role_id,
-      :user => user_json(admin.user, current_user, session)
+      user: user_json(admin.user, current_user, session),
-    }.tap do |hash|
+      workflow_state: admin.workflow_state
-      hash[:status] = 'deleted' if admin.destroyed?
+    }
-    end
   end
 end

spec/apis/v1/admins_api_spec.rb

@@ -73,22 +73,23 @@ describe "Admins API", type: :request do
 
     it "should return json of the new admin association" do
       json = api_call(:post, "/api/v1/accounts/#{@admin.account.id}/admins",
-        { :controller => 'admins', :action => 'create', :format => 'json', :account_id => @admin.account.id.to_s },
+                      {:controller => 'admins', :action => 'create', :format => 'json', :account_id => @admin.account.id.to_s},
-        { :user_id => @new_user.id })
+                      {:user_id => @new_user.id})
       @new_user.reload
       admin = @new_user.account_users.first
       expect(json).to eq({
-        "id" => admin.id,
+                           "id" => admin.id,
-        "role_id" => admin.role_id,
+                           "role_id" => admin.role_id,
-        "role" => admin.role.name,
+                           "role" => admin.role.name,
-        "user" => {
+                           "user" => {
-          "id" => @new_user.id,
+                             "id" => @new_user.id,
-          "name" => @new_user.name,
+                             "name" => @new_user.name,
-          "short_name" => @new_user.short_name,
+                             "short_name" => @new_user.short_name,
-          "sortable_name" => @new_user.sortable_name,
+                             "sortable_name" => @new_user.sortable_name,
-          "login_id" => "user",
+                             "login_id" => "user",
-        }
+                           },
-      })
+                           "workflow_state" => 'active'
+                         })
     end
 
     it "should not send a notification email if passed a 0 'send_confirmation' value" do
@@ -162,18 +163,18 @@ describe "Admins API", type: :request do
         @au = @account.account_users.create! :user => @new_user
       end
 
-      it "should remove AccountAdmin membership implicitly" do
+      it "should remove AccountAdmin membership" do
         json = api_call(:delete, @path, @path_opts)
         expect(json['user']['id']).to eq @new_user.id
         expect(json['id']).to eq @au.id
         expect(json['role']).to eq 'AccountAdmin'
-        expect(json['status']).to eq 'deleted'
+        expect(json['workflow_state']).to eq 'deleted'
-        expect(@account.account_users.where(user_id: @new_user)).not_to be_exists
+        expect(@au.reload.workflow_state).to eq 'deleted'
       end
 
       it "should remove AccountAdmin membership explicitly" do
         api_call(:delete, @path + "?role=AccountAdmin", @path_opts.merge(:role => "AccountAdmin"))
-        expect(@account.account_users.where(user_id: @new_user)).not_to be_exists
+        expect(@account.account_users.active.where(user_id: @new_user)).not_to be_exists
       end
 
       it "should 404 if the user doesn't exist" do
@@ -187,34 +188,34 @@ describe "Admins API", type: :request do
       it "should work by sis user id" do
         api_call(:delete, @base_path + "sis_user_id:badmin",
                  @path_opts.merge(:user_id => "sis_user_id:badmin"))
-        expect(@account.account_users.where(user_id: @new_user)).not_to be_exists
+        expect(@account.account_users.active.where(user_id: @new_user)).not_to be_exists
       end
     end
 
     context "with custom membership" do
       before :once do
-        @role = custom_account_role('CustomAdmin', :account => @account)
+        @role = custom_account_role('CustomAdmin', account: @account)
-        @au = @account.account_users.create! :user => @new_user, :role => @role
+        @au = @account.account_users.create!(user: @new_user, role: @role)
       end
 
       it "should remove a custom membership from a user" do
-        api_call(:delete, @path + "?role_id=#{@role.id}", @path_opts.merge(:role_id => @role.id))
+        api_call(:delete, @path + "?role_id=#{@role.id}", @path_opts.merge(role_id: @role.id))
-        expect(@account.account_users.find_by_user_id_and_role_id(@new_user.id, @role.id)).to be_nil
+        expect(@account.account_users.active.find_by_user_id_and_role_id(@new_user.id, @role.id)).to be_nil
       end
 
       it "should still work using the deprecated role param" do
-        api_call(:delete, @path + "?role=CustomAdmin", @path_opts.merge(:role => "CustomAdmin"))
+        api_call(:delete, @path + "?role=CustomAdmin", @path_opts.merge(role: "CustomAdmin"))
-        expect(@account.account_users.where(:user_id => @new_user, :role_id => @role.id).exists?).to eq false
+        expect(@account.account_users.active.where(user_id: @new_user, role_id: @role.id).exists?).to eq false
       end
 
       it "should 404 if the membership type doesn't exist" do
-        api_call(:delete, @path + "?role=Blah", @path_opts.merge(:role => "Blah"), {}, {}, :expected_status => 404)
+        api_call(:delete, @path + "?role=Blah", @path_opts.merge(role: "Blah"), {}, {}, expected_status: 404)
-        expect(@account.account_users.where(:user_id => @new_user, :role_id => @role.id).exists?).to eq true
+        expect(@account.account_users.where(user_id: @new_user, role_id: @role.id).exists?).to eq true
       end
 
       it "should 404 if the membership type isn't specified" do
-        api_call(:delete, @path, @path_opts, {}, {}, :expected_status => 404)
+        api_call(:delete, @path, @path_opts, {}, {}, expected_status: 404)
-        expect(@account.account_users.where(:user_id => @new_user, :role_id => @role.id).exists?).to eq true
+        expect(@account.account_users.where(user_id: @new_user, role_id: @role.id).exists?).to eq true
       end
     end
 
@@ -227,17 +228,17 @@ describe "Admins API", type: :request do
 
       it "should leave the AccountAdmin membership alone when deleting the custom membership" do
         api_call(:delete, @path + "?role_id=#{@role.id}", @path_opts.merge(:role_id => @role.id))
-        expect(@account.account_users.where(:user_id => @new_user.id).map(&:role_id)).to eq [admin_role.id]
+        expect(@account.account_users.active.where(:user_id => @new_user.id).map(&:role_id)).to eq [admin_role.id]
       end
 
       it "should leave the custom membership alone when deleting the AccountAdmin membership implicitly" do
         api_call(:delete, @path, @path_opts)
-        expect(@account.account_users.where(:user_id => @new_user.id).map(&:role_id)).to eq [@role.id]
+        expect(@account.account_users.active.where(:user_id => @new_user.id).map(&:role_id)).to eq [@role.id]
       end
 
       it "should leave the custom membership alone when deleting the AccountAdmin membership explicitly" do
         api_call(:delete, @path + "?role_id=#{admin_role.id}", @path_opts.merge(:role_id => admin_role.id))
-        expect(@account.account_users.where(:user_id => @new_user.id).map(&:role_id)).to eq [@role.id]
+        expect(@account.account_users.active.where(:user_id => @new_user.id).map(&:role_id)).to eq [@role.id]
       end
     end
   end
@@ -273,49 +274,53 @@ describe "Admins API", type: :request do
 
       it "should return the correct format" do
         json = api_call(:get, @path, @path_opts)
-        expect(json).to be_include({"id"=>@admin.account_users.first.id,
+        expect(json).to be_include({"id" => @admin.account_users.first.id,
-                                "role"=>"AccountAdmin",
+                                    "role" => "AccountAdmin",
-                                "role_id" => admin_role.id,
+                                    "role_id" => admin_role.id,
-                                "user"=>
+                                    "user" =>
-                                    {"id"=>@admin.id,
+                                      {"id" => @admin.id,
-                                     "name"=>@admin.name,
+                                       "name" => @admin.name,
-                                     "sortable_name"=>@admin.sortable_name,
+                                       "sortable_name" => @admin.sortable_name,
-                                     "short_name"=>@admin.short_name,
+                                       "short_name" => @admin.short_name,
-                                     "login_id"=>@admin.pseudonym.unique_id}})
+                                       "login_id" => @admin.pseudonym.unique_id},
+                                    "workflow_state" => 'active'})
       end
 
       it "should scope the results to the user_id if given" do
         json = api_call(:get, @path, @path_opts.merge(user_id: @admin.id))
-        expect(json).to eq [{"id"=>@admin.account_users.first.id,
+        expect(json).to eq [{"id" => @admin.account_users.first.id,
-                        "role"=>"AccountAdmin",
+                             "role" => "AccountAdmin",
-                        "role_id" => admin_role.id,
+                             "role_id" => admin_role.id,
-                        "user"=>
+                             "user" =>
-                            {"id"=>@admin.id,
+                               {"id" => @admin.id,
-                             "name"=>@admin.name,
+                                "name" => @admin.name,
-                             "sortable_name"=>@admin.sortable_name,
+                                "sortable_name" => @admin.sortable_name,
-                             "short_name"=>@admin.short_name,
+                                "short_name" => @admin.short_name,
-                             "login_id"=>@admin.pseudonym.unique_id}}]
+                                "login_id" => @admin.pseudonym.unique_id},
+                             "workflow_state" => 'active'}]
       end
 
       it "should scope the results to the array of user_ids if given" do
         json = api_call(:get, @path, @path_opts.merge(user_id: [@admin.id, @another_admin.id]))
-        expect(json).to eq [{"id"=>@admin.account_users.first.id,
+        expect(json).to eq [{"id" => @admin.account_users.first.id,
-                        "role"=>"AccountAdmin",
+                             "role" => "AccountAdmin",
-                        "role_id" => admin_role.id,
+                             "role_id" => admin_role.id,
-                        "user"=>
+                             "user" =>
-                            {"id"=>@admin.id,
+                               {"id" => @admin.id,
-                             "name"=>@admin.name,
+                                "name" => @admin.name,
-                             "sortable_name"=>@admin.sortable_name,
+                                "sortable_name" => @admin.sortable_name,
-                             "short_name"=>@admin.short_name,
+                                "short_name" => @admin.short_name,
-                             "login_id"=>@admin.pseudonym.unique_id}},
+                                "login_id" => @admin.pseudonym.unique_id},
-                       {"id"=>@another_admin.account_users.first.id,
+                             "workflow_state" => 'active'},
-                        "role"=>"MT 1",
+                            {"id" => @another_admin.account_users.first.id,
-                        "role_id"=>@roles[1].id,
+                             "role" => "MT 1",
-                        "user"=>
+                             "role_id" => @roles[1].id,
-                            {"id"=>@another_admin.id,
+                             "user" =>
-                             "name"=>@another_admin.name,
+                               {"id" => @another_admin.id,
-                             "sortable_name"=>@another_admin.sortable_name,
+                                "name" => @another_admin.name,
-                             "short_name"=>@another_admin.short_name}}]
+                                "sortable_name" => @another_admin.sortable_name,
+                                "short_name" => @another_admin.short_name},
+                             "workflow_state" => 'active'}]
       end
 
       context 'sharding' do

spec/controllers/accounts_controller_spec.rb

@@ -185,7 +185,7 @@ describe AccountsController do
       account_with_admin_logged_in(account: a)
       post 'remove_account_user', params: {account_id: a.id, id: au_id}
       expect(response).to be_redirect
-      expect(AccountUser.where(id: au_id).first).to be_nil
+      expect(AccountUser.active.where(id: au_id).first).to be_nil
     end
 
     it "should verify that the membership is in the caller's account" do

spec/models/account_authorization_config_spec.rb

@@ -300,7 +300,15 @@ describe AccountAuthorizationConfig do
         @user.account_users.create!(account: @pseudonym.account)
         aac.apply_federated_attributes(@pseudonym, 'admin_roles' => '')
         @user.reload
-        expect(@user.account_users).not_to be_exists
+        expect(@user.account_users.active).not_to be_exists
+      end
+
+      it "reactivates previously deleted roles" do
+        au = @user.account_users.create!(account: @pseudonym.account)
+        au.destroy
+        aac.apply_federated_attributes(@pseudonym, 'admin_roles' => 'AccountAdmin')
+        @user.reload
+        expect(au.reload).to be_active
       end
     end
 

spec/models/account_user_spec.rb

@@ -50,6 +50,7 @@ describe AccountUser do
         RoleOverride.clear_cached_contexts
         @account.instance_variable_set(:@account_users_cache, {})
         expect(@account.grants_right?(@user, :read)).to be_falsey
+        expect(au.reload.workflow_state).to eq 'deleted'
       end
     end
   end

spec/models/sharded_bookmarked_collection_spec.rb

@@ -28,7 +28,7 @@ describe ShardedBookmarkedCollection do
   context "without sharding" do
 
     it "returns a paginatable collection" do
-      collection = ShardedBookmarkedCollection.build(Account::Bookmarker, @user.accounts) do |scope|
+      collection = ShardedBookmarkedCollection.build(Account::Bookmarker, @user.adminable_accounts) do |scope|
         scope.active
       end
       expect(collection.paginate(per_page: 10).size).to equal 1

spec/models/split_user_spec.rb

@@ -326,7 +326,7 @@ describe SplitUsers do
       expect { SplitUsers.split_db_users(user2) }.not_to raise_error
     end
 
-    it 'should restore admins even with stale data' do
+    it 'should restore admins to the original state' do
       admin = account1.account_users.create(user: user1)
       admin2 = sub_account.account_users.create(user: user1)
       admin3 = sub_account.account_users.create(user: user2)
@@ -334,7 +334,8 @@ describe SplitUsers do
       admin.reload.destroy
       SplitUsers.split_db_users(user2)
 
-      expect{admin.reload}.to raise_error(ActiveRecord::RecordNotFound)
+      expect(admin.reload.workflow_state).to eq 'active'
+      expect(admin.reload.user).to eq user1
       expect(admin2.reload.user).to eq user1
       expect(admin3.reload.user).to eq user2
     end

spec/models/user_spec.rb

@@ -266,7 +266,7 @@ describe User do
     user.reload
     expect(user.associated_account_ids.include?(account1.id)).to be_truthy
     expect(user.associated_account_ids.include?(account2.id)).to be_falsey
-    expect(user.account_users.where(:account_id => [account2, sub])).to be_empty
+    expect(user.account_users.active.where(account_id: [account2, sub])).to be_empty
   end
 
   it "should search by multiple fields" do
@@ -3267,7 +3267,7 @@ describe User do
     end
   end
 
-  describe "all_accounts" do
+  describe "adminable_accounts" do
     specs_require_sharding
 
     it "should include accounts from multiple shards" do
@@ -3278,7 +3278,7 @@ describe User do
         @account2.account_users.create!(user: @user)
       end
 
-      expect(@user.all_accounts.map(&:id).sort).to eq [Account.site_admin, @account2].map(&:id).sort
+      expect(@user.adminable_accounts.map(&:id).sort).to eq [Account.site_admin, @account2].map(&:id).sort
     end
 
     it "should exclude deleted accounts" do
@@ -3290,7 +3290,7 @@ describe User do
         @account2.destroy
       end
 
-      expect(@user.all_accounts.map(&:id).sort).to eq [Account.site_admin].map(&:id).sort
+      expect(@user.adminable_accounts.map(&:id).sort).to eq [Account.site_admin].map(&:id).sort
     end
   end
 

spec/selenium/helpers/basic/settings_specs.rb

@@ -59,7 +59,7 @@ shared_examples_for "settings basic tests" do |account_type|
       f("#enrollment_#{admin_id} .remove_account_user_link").click
       driver.switch_to.alert.accept
       wait_for_ajax_requests
-      expect(AccountUser.where(id: admin_id)).not_to be_exists
+      expect(AccountUser.active.where(id: admin_id)).not_to be_exists
     end
   end