dont allow duplicate thresholds

closes MBL-10504

test plan:
- using the api endpoint below, try to create a threshold
  with an alert_type that already exists for a user/observer
- it shouldn't create a new threshold, just update the
  value for "threshold"

endpoint: POST /users/:user_id/observer_alert_thresholds

Change-Id: I64870ea5414b83d984cb4a71ff106bfb1d532d1b
Reviewed-on: https://gerrit.instructure.com/150515
Tested-by: Jenkins
Reviewed-by: Matthew Sessions <msessions@instructure.com>
QA-Review: Kausty Saxena <ksaxena@instructure.com>
Product-Review: Cameron Sutter <csutter@instructure.com>

app/controllers/observer_alert_thresholds_api_controller.rb

@@ -51,11 +51,19 @@ class ObserverAlertThresholdsApiController < ApplicationController
     return render_unauthorized_action unless link
 
     attrs = create_params.merge(user_observation_link: link)
-    begin
+
+    threshold = link.observer_alert_thresholds.active.where(alert_type: attrs[:alert_type]).take
+    if threshold
+      # update if duplicate
+      threshold.update(threshold: attrs[:threshold])
+    else
       threshold = link.observer_alert_thresholds.create(attrs)
+    end
+
+    if threshold.valid?
       render json: observer_alert_threshold_json(threshold, @current_user, session)
-    rescue ActiveRecord::NotNullViolation
-      render :json => ['missing required parameters'], :status => :bad_request
+    else
+      render json: threshold.errors, status: :bad_request
     end
   end
 

app/models/observer_alert.rb

@@ -20,9 +20,21 @@ class ObserverAlert < ActiveRecord::Base
   belongs_to :observer_alert_threshold, :inverse_of => :observer_alerts
   belongs_to :context, polymorphic: [:discussion_topic, :assignment, :course, :account_notification]
 
+  ALERT_TYPES = %w(
+    assignment_missing
+    assignment_grade_high
+    assignment_grade_low
+    course_grade_high
+    course_grade_low
+    course_announcement
+    institution_announcement
+  ).freeze
+  validates :alert_type, inclusion: { in: ALERT_TYPES }
+  validates :user_observation_link_id, :observer_alert_threshold_id, :alert_type, :action_date, :title, presence: true
+
   scope :active, -> { where.not(workflow_state: ['dismissed', 'deleted']) }
   scope :unread, -> { where(workflow_state: 'unread')}
-  
+
   def self.clean_up_old_alerts
     ObserverAlert.where('created_at < ?', 6.months.ago).delete_all
   end

app/models/observer_alert_threshold.rb

@@ -19,6 +19,19 @@ class ObserverAlertThreshold < ActiveRecord::Base
   belongs_to :user_observation_link, :inverse_of => :observer_alert_thresholds
   has_many :observer_alerts, :inverse_of => :observer_alert_threshold
 
+  ALERT_TYPES = %w(
+    assignment_missing
+    assignment_grade_high
+    assignment_grade_low
+    course_grade_high
+    course_grade_low
+    course_announcement
+    institution_announcement
+  ).freeze
+  validates :alert_type, inclusion: { in: ALERT_TYPES }
+  validates :user_observation_link_id, :alert_type, presence: true
+  validates :alert_type, uniqueness: { scope: :user_observation_link }
+
   scope :active, -> { where.not(workflow_state: 'deleted') }
 
   def destroy

db/migrate/20180516171715_add_index_to_observer_alert_threshold.rb

@@ -0,0 +1,24 @@
+#
+# Copyright (C) 2018 - 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 AddIndexToObserverAlertThreshold < ActiveRecord::Migration[5.1]
+  tag :predeploy
+  def change
+    add_index :observer_alert_thresholds, [:alert_type, :user_observation_link_id], unique: true, name: 'observer_alert_thresholds_on_alert_type_and_observer'
+  end
+end

spec/apis/v1/observer_alert_thresholds_api_controller_spec.rb

@@ -24,7 +24,7 @@ describe ObserverAlertThresholdsApiController, type: :request do
 
   context '#index' do
     before :once do
-      observer_alert_threshold_model(active_all: true, alert_type: 'missing_assignment')
+      observer_alert_threshold_model(alert_type: 'assignment_missing')
       @path = "/api/v1/users/#{@observer.id}/observer_alert_thresholds?student_id=#{@observee.id}"
       @params = {user_id: @observer.to_param, student_id: @observee.to_param,
         controller: 'observer_alert_thresholds_api', action: 'index', format: 'json'}
@@ -35,7 +35,7 @@ describe ObserverAlertThresholdsApiController, type: :request do
         json = api_call_as_user(@observer, :get, @path, @params)
         expect(json.length).to eq 1
         expect(json[0]['user_observation_link_id']).to eq @observation_link.id
-        expect(json[0]['alert_type']).to eq 'missing_assignment'
+        expect(json[0]['alert_type']).to eq 'assignment_missing'
       end
 
       it 'only returns active thresholds' do
@@ -87,7 +87,7 @@ describe ObserverAlertThresholdsApiController, type: :request do
 
   context '#show' do
     before :once do
-      observer_alert_threshold_model(active_all: true, alert_type: 'missing_assignment')
+      observer_alert_threshold_model(alert_type: 'assignment_missing')
       @path = "/api/v1/users/#{@observer.id}/observer_alert_thresholds/#{@observer_alert_threshold.id}"
       @params = {user_id: @observer.to_param, observer_alert_threshold_id: @observer_alert_threshold.to_param,
         controller: 'observer_alert_thresholds_api', action: 'show', format: 'json'}
@@ -97,7 +97,7 @@ describe ObserverAlertThresholdsApiController, type: :request do
       json = api_call_as_user(@observer, :get, @path, @params)
       expect(json['id']).to eq @observer_alert_threshold.id
       expect(json['user_observation_link_id']).to eq @observation_link.id
-      expect(json['alert_type']).to eq 'missing_assignment'
+      expect(json['alert_type']).to eq 'assignment_missing'
     end
 
     it 'errors without proper user_observation_link' do
@@ -163,11 +163,22 @@ describe ObserverAlertThresholdsApiController, type: :request do
       expect(response.code).to eq "200"
       expect(json['something_sneaky']).to eq nil
     end
+
+    it 'updates if threshold already exists' do
+      observer_alert_threshold_model(uol: @uol, alert_type: 'assignment_grade_low', threshold: '50')
+      create_params = {alert_type: 'assignment_grade_low', threshold: '65'}
+      params = {user_id: @observer.to_param, student_id: @uol.user_id, observer_alert_threshold: create_params,
+        controller: 'observer_alert_thresholds_api', action: 'create', format: 'json'}
+      json = api_call_as_user(@observer, :post, @path, params)
+      expect(json['id']).to eq @observer_alert_threshold.id
+      expect(json['threshold']).to eq '65'
+      expect(@uol.observer_alert_thresholds.active.where(alert_type: 'assignment_grade_low').count).to eq 1
+    end
   end
 
   context '#update' do
     before :once do
-      observer_alert_threshold_model(active_all: true, alert_type: 'assignment_grade_low', threshold: "88")
+      observer_alert_threshold_model(alert_type: 'assignment_grade_low', threshold: "88")
       @path = "/api/v1/users/#{@observer.id}/observer_alert_thresholds/#{@observer_alert_threshold.id}"
       @params = {user_id: @observer.to_param, observer_alert_threshold_id: @observer_alert_threshold.to_param,
         controller: 'observer_alert_thresholds_api', action: 'update', format: 'json'}
@@ -195,7 +206,7 @@ describe ObserverAlertThresholdsApiController, type: :request do
 
   context '#destroy' do
     before :once do
-      observer_alert_threshold_model(active_all: true, alert_type: 'assignment_grade_low', threshold: "88")
+      observer_alert_threshold_model(alert_type: 'assignment_grade_low', threshold: "88")
       @path = "/api/v1/users/#{@observer.id}/observer_alert_thresholds/#{@observer_alert_threshold.id}"
       @params = {user_id: @observer.to_param, observer_alert_threshold_id: @observer_alert_threshold.to_param,
         controller: 'observer_alert_thresholds_api', action: 'destroy', format: 'json'}

spec/factories/observer_alert_factory.rb

@@ -29,7 +29,7 @@ module Factories
     valid_attrs = [:title, :alert_type, :workflow_state, :action_date]
     default_attrs = {
       title: 'value for type',
-      alert_type: 'value for type',
+      alert_type: 'course_announcement',
       workflow_state: 'active',
       action_date: Time.zone.now
     }

spec/factories/observer_alert_threshold_factory.rb

@@ -28,7 +28,7 @@ module Factories
 
     valid_attrs = [:alert_type, :threshold, :workflow_state]
     default_attrs = {
-      alert_type: 'value for type',
+      alert_type: 'course_announcement',
       threshold: nil,
       workflow_state: 'active',
     }

spec/lib/api/v1/observer_alert_spec.rb

@@ -50,7 +50,7 @@ describe "Api::V1::ObserverAlert" do
       alert = observer_alert_model
       json = api.observer_alert_json(alert, user, session)
       expect(json['title']).to eq('value for type')
-      expect(json['alert_type']).to eq('value for type')
+      expect(json['alert_type']).to eq('course_announcement')
       expect(json['workflow_state']).to eq('active')
       expect(json['user_observation_link_id']).to eq @observation_link.id
       expect(json['observer_alert_threshold_id']).to eq @observer_alert_threshold.id

spec/lib/api/v1/observer_alert_threshold_spec.rb

@@ -40,7 +40,7 @@ describe "Api::V1::ObserverAlertThreshold" do
 
     it "returns json" do
       json = api.observer_alert_threshold_json(observer_alert_threshold, user, session)
-      expect(json['alert_type']).to eq('value for type')
+      expect(json['alert_type']).to eq('course_announcement')
       expect(json['workflow_state']).to eq('active')
       expect(json['user_observation_link_id']).to eq @observation_link.id
     end

spec/models/observer_alert_spec.rb

@@ -23,24 +23,28 @@ describe ObserverAlert do
 
   it 'can link to a threshold and observation link' do
     assignment = assignment_model()
-    assignment.save!
     observer = user_factory()
     student = user_factory()
-    link = UserObservationLink.new(:student => student, :observer => observer,
-                            :root_account => @account)
-    link.save!
-    threshold = ObserverAlertThreshold.new(:user_observation_link => link, :alert_type => 'missing')
-    threshold.save!
+    link = UserObservationLink.create!(student: student, observer: observer, root_account: @account)
+    threshold = ObserverAlertThreshold.create!(user_observation_link: link, alert_type: 'assignment_missing')
 
-    alert = ObserverAlert.new(:user_observation_link => link, :observer_alert_threshold => threshold,
-                      :context => assignment, :alert_type => 'missing', :action_date => Time.zone.now,
-                      :title => 'Assignment missing')
-    alert.save!
+    alert = ObserverAlert.create(user_observation_link: link, observer_alert_threshold: threshold,
+                      context: assignment, alert_type: 'assignment_missing', action_date: Time.zone.now,
+                      title: 'Assignment missing')
 
-    saved_alert = ObserverAlert.find(alert.id)
-    expect(saved_alert).not_to be_nil
-    expect(saved_alert.user_observation_link).not_to be_nil
-    expect(saved_alert.observer_alert_threshold).not_to be_nil
+    expect(alert.valid?).to eq true
+    expect(alert.user_observation_link).not_to be_nil
+    expect(alert.observer_alert_threshold).not_to be_nil
+  end
+
+  it 'wont allow random types of alert_type' do
+    assignment = assignment_model
+    link = UserObservationLink.create!(student: user_model, observer: user_model, root_account: @account)
+    threshold = ObserverAlertThreshold.create!(user_observation_link: link, alert_type: 'assignment_missing')
+    alert = ObserverAlert.create(user_observation_link: link, observer_alert_threshold: threshold,
+      context: assignment, alert_type: 'jigglypuff', action_date: Time.zone.now, title: 'Assignment missing')
+
+    expect(alert.valid?).to eq false
   end
 
   describe 'course_announcement' do

spec/models/observer_alert_threshold_spec.rb

@@ -21,14 +21,18 @@ describe ObserverAlertThreshold do
   it 'can link to an user_observation_link' do
     observer = user_factory()
     student = user_factory()
-    link = UserObservationLink.new(:student => student, :observer => observer, 
+    link = UserObservationLink.create!(:student => student, :observer => observer,
                             :root_account => @account)
-    link.save!
-    threshold = ObserverAlertThreshold.new(:user_observation_link => link, :alert_type => 'missing')
-    threshold.save!
+    threshold = ObserverAlertThreshold.create(:user_observation_link => link, :alert_type => 'assignment_missing')
 
-    saved_threshold = ObserverAlertThreshold.find(threshold.id)
-    expect(saved_threshold).not_to be_nil
-    expect(saved_threshold.user_observation_link).not_to be_nil
+    expect(threshold.valid?).to eq true
+    expect(threshold.user_observation_link).not_to be_nil
+  end
+
+  it 'wont allow random types of alert_type' do
+    link = UserObservationLink.create!(student: user_model, observer: user_model, root_account: @account)
+    threshold = ObserverAlertThreshold.create(user_observation_link: link, alert_type: 'jigglypuff')
+
+    expect(threshold.valid?).to eq false
   end
 end