add workflow_state to user_observer

fixes CNVS-27942

test plan
 - user observers should still work
 - destroy a user observer
 - it should still exist in the db
 - it should not be visible in the UI or API
 - if you create a new user observer it should
   restore the original record to active

Change-Id: Ia992f1d54a8f8530c3c933f44db48192b2c8d627
Reviewed-on: https://gerrit.instructure.com/74490
Tested-by: Jenkins
QA-Review: August Thornton <august@instructure.com>
Reviewed-by: Cody Cutrer <cody@instructure.com>
Product-Review: Rob Orton <rob@instructure.com>

app/controllers/communication_channels_controller.rb

@@ -255,7 +255,7 @@ class CommunicationChannelsController < ApplicationController
       # load merge opportunities
       merge_users = cc.merge_candidates
       merge_users << @current_user if @current_user && !@user.registered? && !merge_users.include?(@current_user)
-      user_observers = UserObserver.where("user_id = ? OR observer_id = ?", @user.id, @user.id)
+      user_observers = UserObserver.active.where("user_id = ? OR observer_id = ?", @user.id, @user.id)
       merge_users = merge_users.reject { |u| user_observers.any?{|uo| uo.user == u || uo.observer == u} }
       # remove users that don't have a pseudonym for this account, or one can't be created
       merge_users = merge_users.select { |u| u.find_or_initialize_pseudonym_for_account(@root_account, @domain_root_account) }

app/controllers/login/saml_controller.rb

@@ -309,9 +309,7 @@ class Login::SamlController < ApplicationController
     observee_unique_id = registration_data[:observee][:unique_id]
     observee = @domain_root_account.pseudonyms.by_unique_id(observee_unique_id).first.user
     unless @current_user.user_observees.where(user_id: observee).exists?
-      @current_user.user_observees.create! do |uo|
-        uo.user_id = observee.id
-      end
+      @current_user.user_observees.create_or_restore(user_id: observee)
       @current_user.touch
     end
   end
@@ -348,7 +346,7 @@ class Login::SamlController < ApplicationController
 
     # set the new user (observer) to observe the target user (observee)
     observee = @domain_root_account.pseudonyms.active.by_unique_id(observee_unique_id).first.user
-    user.user_observees << user.user_observees.create!{ |uo| uo.user_id = observee.id }
+    user.user_observees << user.user_observees.create_or_restore(user_id: observee)
 
     notify_policy = Users::CreationNotifyPolicy.new(false, unique_id: observer_unique_id)
     notify_policy.dispatch!(user, pseudonym, cc)

app/controllers/user_observees_controller.rb

@@ -43,7 +43,7 @@ class UserObserveesController < ApplicationController
   # @returns [User]
   def index
     includes = params[:include] || []
-    observed_users = user.observed_users.active.order_by_sortable_name
+    observed_users = user.observed_users.active_user_observers.active.order_by_sortable_name
     observed_users = Api.paginate(observed_users, self, api_v1_user_observees_url)
     render json: users_json(observed_users, @current_user, session,includes )
   end
@@ -180,9 +180,7 @@ class UserObserveesController < ApplicationController
     @current_user.shard.activate do
       UserObserver.unique_constraint_retry do
         unless has_observee?(observee)
-          user.user_observees.create! do |uo|
-            uo.user_id = observee.id
-          end
+          user.user_observees.create_or_restore(user_id: observee)
           user.touch
         end
       end
@@ -194,13 +192,13 @@ class UserObserveesController < ApplicationController
       enrollment.workflow_state = 'deleted'
       enrollment.save
     end
-    user.user_observees.where(user_id: observee).destroy_all
+    user.user_observees.active.where(user_id: observee).destroy_all
     user.update_account_associations
     user.touch
   end
 
   def has_observee?(observee)
-    user.user_observees.where(user_id: observee).exists?
+    user.user_observees.active.where(user_id: observee).exists?
   end
 
   def self_or_admin_permission_check

app/controllers/users_controller.rb

@@ -2180,7 +2180,7 @@ class UsersController < ApplicationController
       end
       @user.save!
       if @observee && !@user.user_observees.where(user_id: @observee).exists?
-        @user.user_observees << @user.user_observees.create!{ |uo| uo.user_id = @observee.id }
+        @user.user_observees << @user.user_observees.create_or_restore(user_id: @observee)
       end
 
       if notify_policy.is_self_registration?

app/models/user.rb

@@ -53,9 +53,13 @@ class User < ActiveRecord::Base
   has_many :not_ended_enrollments, -> { where("enrollments.workflow_state NOT IN ('rejected', 'completed', 'deleted', 'inactive')") }, class_name: 'Enrollment', multishard: true
   has_many :observer_enrollments
   has_many :observee_enrollments, :foreign_key => :associated_user_id, :class_name => 'ObserverEnrollment'
-  has_many :user_observers, :dependent => :delete_all
+  has_many :user_observers, dependent: :destroy, inverse_of: :user
   has_many :observers, :through => :user_observers, :class_name => 'User'
-  has_many :user_observees, :class_name => 'UserObserver', :foreign_key => :observer_id, :dependent => :delete_all
+  has_many :user_observees,
+           class_name: 'UserObserver',
+           foreign_key: :observer_id,
+           dependent: :destroy,
+           inverse_of: :observer
   has_many :observed_users, :through => :user_observees, :source => :user
   has_many :all_courses, :source => :course, :through => :enrollments
   has_many :group_memberships, -> { preload(:group) }, dependent: :destroy
@@ -180,6 +184,7 @@ class User < ActiveRecord::Base
     self.from("(#{scopes.join("\nUNION\n")}) users")
   }
   scope :active, -> { where("users.workflow_state<>'deleted'") }
+  scope :active_user_observers, -> { where.not(user_observers: {workflow_state: 'deleted'}) }
 
   scope :has_current_student_enrollments, -> do
     where("EXISTS (?)",
@@ -1068,7 +1073,7 @@ class User < ActiveRecord::Base
     end
     can :reset_mfa
 
-    given { |user| user && user.user_observees.where(user_id: self.id).exists? }
+    given { |user| user && user.user_observees.active.where(user_id: self.id).exists? }
     can :read and can :read_as_parent
   end
 

app/models/user_observer.rb

@@ -17,14 +17,37 @@
 #
 
 class UserObserver < ActiveRecord::Base
-  belongs_to :user
-  belongs_to :observer, :class_name => 'User'
-  attr_accessible
+  belongs_to :user, inverse_of: :user_observees
+  belongs_to :observer, :class_name => 'User', inverse_of: :user_observers
+  strong_params
 
   after_create :create_linked_enrollments
 
   validate :not_same_user, :if => lambda { |uo| uo.changed? }
 
+  scope :active, -> { where.not(workflow_state: 'deleted') }
+
+  def self.create_or_restore(attributes)
+    UserObserver.unique_constraint_retry do
+      if (user_observer = where(attributes).take)
+        if user_observer.workflow_state == 'deleted'
+          user_observer.workflow_state = 'active'
+          user_observer.save!
+        end
+      else
+        user_observer = create!(attributes)
+      end
+      user_observer.user.touch
+      user_observer
+    end
+  end
+
+  alias_method :destroy_permanently!, :destroy
+  def destroy
+    self.workflow_state = 'deleted'
+    self.save!
+  end
+
   def not_same_user
     self.errors.add(:observer_id, "Cannot observe yourself") if self.user_id == self.observer_id
   end

app/models/user_profile.rb

@@ -68,7 +68,7 @@ class UserProfile < ActiveRecord::Base
         @tabs = @tabs.slice(0,2)
       end
 
-      if user && user.user_observees.exists?
+      if user && user.user_observees.active.exists?
         @tabs << { :id => TAB_OBSERVEES, :label => I18n.t('#tabs.observees', 'Observing'), :css_class => 'observees', :href => :observees_profile_path, :no_args => true }
       end
     end

db/migrate/20160310205719_add_workflow_state_to_user_observer.rb

@@ -0,0 +1,8 @@
+class AddWorkflowStateToUserObserver < ActiveRecord::Migration
+  tag :predeploy
+
+  def change
+    add_column :user_observers, :workflow_state, :string, default: 'active', null: false
+    add_index :user_observers, :workflow_state
+  end
+end

spec/apis/v1/user_observees_api_spec.rb

@@ -179,6 +179,14 @@ describe UserObserveesController, type: :request do
       expect(index_call(page: 2)).to eq [student.id]
     end
 
+    it 'should not include deleted observers' do
+      parent.observed_users << student
+      parent.observed_users << student2
+      parent.user_observees.where(user_id: student2).destroy_all
+
+      expect(index_call).to eq [student.id]
+    end
+
     it 'should not accept an invalid user' do
       index_call(user_id: 0, expected_status: 404)
     end
@@ -402,7 +410,7 @@ describe UserObserveesController, type: :request do
       observer_enrollment = parent.observer_enrollments.first
 
       expect(delete_call).to eq student.id
-      expect(parent.reload.observed_users).to eq []
+      expect(parent.observed_users.active_user_observers).to eq []
       expect(observer_enrollment.reload).to be_deleted
     end
 
@@ -413,7 +421,7 @@ describe UserObserveesController, type: :request do
 
       json = delete_call(user_id: external_parent.id, observee_id: external_student.id, api_user: multi_admin, domain_root_account: external_account)
       expect(json).to eq external_student.id
-      expect(external_parent.reload.observed_users).to eq []
+      expect(external_parent.reload.observed_users.active_user_observers).to eq []
       expect(observer_enrollment.reload).to be_deleted
     end
 
@@ -438,13 +446,15 @@ describe UserObserveesController, type: :request do
     it 'should not allow unauthorized admins' do
       parent.observed_users << student
       delete_call(api_user: disallowed_admin, expected_status: 401)
-      expect(parent.reload.observed_users).to eq [student]
+      expect(parent.reload.observed_users.active_user_observers).to eq [student]
     end
 
     it 'should allow observer to remove observee' do
       parent.observed_users << student
       delete_call(api_user: parent, expected_status: 200)
-      expect(parent.reload.observed_users).to eq []
+      expect(parent.reload.observed_users.active_user_observers).to eq []
+      expect(parent.observed_users).to eq [student]
+      expect(parent.user_observees.first.workflow_state).to eq 'deleted'
     end
   end
 

spec/models/user_observer_spec.rb

@@ -22,7 +22,24 @@ describe UserObserver do
   let_once(:student) { user }
 
   it "should not allow a user to observe oneself" do
-    expect { student.observers << student}.to raise_error(ActiveRecord::RecordInvalid)
+    expect { student.observers << student }.to raise_error(ActiveRecord::RecordInvalid)
+  end
+
+  it 'should restore deleted observees instead of creating a new one' do
+    observer = user_with_pseudonym
+    student.observers << observer
+    observee = observer.user_observees.first
+    observee.destroy
+
+    re_observee = student.user_observers.create_or_restore(observer_id: observer)
+    expect(observee.id).to eq re_observee.id
+    expect(re_observee.workflow_state).to eq 'active'
+  end
+
+  it 'should create an observees when one does not exist' do
+    observer = user_with_pseudonym
+    re_observee = observer.user_observees.create_or_restore(user_id: student)
+    expect(re_observee).to eq student.user_observers.first
   end
 
   it "should enroll the observer in all pending/active courses" do