fix course[event] in "Update a course" API

test plan:
 - Consult the API docs for "Update a course"
   (/doc/api/courses.html#method.courses.update)
 - ensure the course[event] parameter can be used as
   documented (claim, offer, conclude, delete, undelete)
 - ensure a teacher can do all of these except undelete
   which requires account admin rights
 - ensure if an invalid course[event] is supplied,
   a sensible error is returned
 - ensure if an invalid transition is attempted
   (e.g. trying to publish a deleted course without
   undeleting it first), a sensible error is returned

fixes ADMIN-406

Change-Id: Ie2ea4ab13d384bfd226fd40e8115c3570213b5d7
Reviewed-on: https://gerrit.instructure.com/184959
Tested-by: Jenkins
Reviewed-by: Mysti Lilla <mysti@instructure.com>
QA-Review: Mysti Lilla <mysti@instructure.com>
Product-Review: Mysti Lilla <mysti@instructure.com>

app/controllers/courses_controller.rb

@@ -2239,9 +2239,9 @@ class CoursesController < ApplicationController
   #     in prior-enrollment lists.
   #   * 'delete' completely removes the course from the web site (including course menus and prior-enrollment lists).
   #     All enrollments are deleted. Course content may be physically deleted at a future date.
-  #   * 'undelete' attempts to recover a course that has been deleted. (Recovery is not guaranteed; please conclude
-  #     rather than delete a course if there is any possibility the course will be used again.) The recovered course
-  #     will be unpublished. Deleted enrollments will not be recovered.
+  #   * 'undelete' attempts to recover a course that has been deleted. This action requires account administrative rights.
+  #     (Recovery is not guaranteed; please conclude rather than delete a course if there is any possibility the course
+  #     will be used again.) The recovered course will be unpublished. Deleted enrollments will not be recovered.
   #
   # @argument course[default_view]  [String, "feed"|"wiki"|"modules"|"syllabus"|"assignments"]
   #   The type of page that users will see when they first visit the course
@@ -2321,8 +2321,12 @@ class CoursesController < ApplicationController
     params[:course][:event] = :offer if params[:offer].present?
 
     if params[:course][:event] && params[:course].keys.size == 1
-      if authorized_action(@course, @current_user, :change_course_state) && process_course_event
-        render_update_success
+      if authorized_action(@course, @current_user, :change_course_state)
+        if process_course_event
+          render_update_success
+        else
+          render_update_failure
+        end
       end
       return
     end
@@ -2421,7 +2425,10 @@ class CoursesController < ApplicationController
       end
 
       if params[:course][:event] && @course.grants_right?(@current_user, session, :change_course_state)
-        return unless process_course_event
+        unless process_course_event
+          render_update_failure
+          return
+        end
       end
 
       if params[:course][:image_url] && params[:course][:image_id]
@@ -2516,32 +2523,74 @@ class CoursesController < ApplicationController
         end
         render_update_success
       else
-        respond_to do |format|
-          format.html do
-            flash[:error] = t('There was an error saving the changes to the course')
-            redirect_to course_url(@course)
-          end
-          format.json { render :json => @course.errors, :status => :bad_request }
-        end
+        render_update_failure
       end
     end
   end
 
+  def render_update_failure
+    respond_to do |format|
+      format.html do
+        flash[:error] = t('There was an error saving the changes to the course')
+        redirect_to course_url(@course)
+      end
+      format.json { render :json => @course.errors, :status => :bad_request }
+    end
+  end
+
+  # prevent API from failing when a no-op event is given
+  def non_event?(course, event)
+    case event
+    when :offer
+      course.available?
+    when :claim
+      course.claimed?
+    when :complete
+      course.completed?
+    when :delete
+      course.deleted?
+    else
+      false
+    end
+  end
+
   def process_course_event
     event = params[:course].delete(:event)
     event = event.to_sym
+    event = :complete if event == :conclude
+    return true if non_event?(@course, event)
     if event == :claim && !@course.unpublishable?
-      flash[:error] = t('errors.unpublish', 'Course cannot be unpublished if student submissions exist.')
-      redirect_to(course_url(@course))
+      cant_unpublish_message = t('errors.unpublish', 'Course cannot be unpublished if student submissions exist.')
+      respond_to do |format|
+        format.json do
+          @course.errors.add(:workflow_state, cant_unpublish_message)
+        end
+        format.html do
+          flash[:error] = cant_unpublish_message
+          redirect_to(course_url(@course))
+        end
+      end
       return false
     else
-      @course.process_event(event)
-      logging_source = api_request? ? :api : :manual
-      if event == :offer
-        Auditors::Course.record_published(@course, @current_user, source: logging_source)
-      elsif event == :claim
-        Auditors::Course.record_claimed(@course, @current_user, source: logging_source)
+      result = @course.process_event(event)
+      if result
+        opts = { source: api_request? ? :api : :manual }
+        case event
+        when :offer
+          Auditors::Course.record_published(@course, @current_user, opts)
+        when :claim
+          Auditors::Course.record_claimed(@course, @current_user, opts)
+        when :complete
+          Auditors::Course.record_concluded(@course, @current_user, opts)
+        when :delete
+          Auditors::Course.record_deleted(@course, @current_user, opts)
+        when :undelete
+          Auditors::Course.record_restored(@course, @current_user, opts)
+        end
+      else
+        @course.errors.add(:workflow_state, @course.halted_because)
       end
+      result
     end
   end
 

app/models/course.rb

@@ -1263,22 +1263,31 @@ class Course < ActiveRecord::Base
       event :claim, :transitions_to => :claimed
       event :offer, :transitions_to => :available
       event :complete, :transitions_to => :completed
+      event :delete, :transitions_to => :deleted
     end
 
     state :claimed do
       event :offer, :transitions_to => :available
       event :complete, :transitions_to => :completed
+      event :delete, :transitions_to => :deleted
     end
 
     state :available do
       event :complete, :transitions_to => :completed
       event :claim, :transitions_to => :claimed
+      event :delete, :transitions_to => :deleted
     end
 
     state :completed do
       event :unconclude, :transitions_to => :available
+      event :offer, :transitions_to => :available
+      event :claim, :transitions_to => :claimed
+      event :delete, :transitions_to => :deleted
+    end
+
+    state :deleted do
+      event :undelete, :transitions_to => :claimed
     end
-    state :deleted
   end
 
   def api_state

spec/controllers/courses_controller_spec.rb

@@ -1522,11 +1522,18 @@ describe CoursesController do
     end
 
     it "should log published event on update" do
+      @course.claim!
       expect(Auditors::Course).to receive(:record_published).once
       user_session(@teacher)
       put 'update', params: {:id => @course.id, :offer => true}
     end
 
+    it "should not log published event if course was already published" do
+      expect(Auditors::Course).to receive(:record_published).never
+      user_session(@teacher)
+      put 'update', params: {:id => @course.id, :offer => true}
+    end
+
     it "should log claimed event on update" do
       expect(Auditors::Course).to receive(:record_claimed).once
       user_session(@teacher)
@@ -1564,6 +1571,64 @@ describe CoursesController do
       expect(@course.workflow_state).to eq 'claimed'
     end
 
+    it "concludes a course" do
+      expect(Auditors::Course).to receive(:record_concluded).once
+      user_session(@teacher)
+      put 'update', params: {:id => @course.id, :course => {:event => "conclude"}, :format => :json}
+      json = JSON.parse response.body
+      expect(json['course']['workflow_state']).to eq 'completed'
+      @course.reload
+      expect(@course.workflow_state).to eq 'completed'
+    end
+
+    it "publishes a course" do
+      @course.claim!
+      expect(Auditors::Course).to receive(:record_published).once
+      user_session(@teacher)
+      put 'update', params: {:id => @course.id, :course => {:event => 'offer'}, :format => :json}
+      json = JSON.parse response.body
+      expect(json['course']['workflow_state']).to eq 'available'
+      @course.reload
+      expect(@course.workflow_state).to eq 'available'
+    end
+
+    it "deletes a course" do
+      user_session(@teacher)
+      expect(Auditors::Course).to receive(:record_deleted).once
+      put 'update', params: {:id => @course.id, :course => {:event => 'delete'}, :format => :json}
+      json = JSON.parse response.body
+      expect(json['course']['workflow_state']).to eq 'deleted'
+      @course.reload
+      expect(@course.workflow_state).to eq 'deleted'
+    end
+
+    it "doesn't allow a teacher to undelete a course" do
+      @course.destroy
+      expect(Auditors::Course).to receive(:record_restored).never
+      user_session(@teacher)
+      put 'update', params: {:id => @course.id, :course => {:event => 'undelete'}, :format => :json}
+      expect(response.status).to eq 401
+    end
+
+    it "undeletes a course" do
+      @course.destroy
+      expect(Auditors::Course).to receive(:record_restored).once
+      user_session(account_admin_user)
+      put 'update', params: {:id => @course.id, :course => {:event => 'undelete'}, :format => :json}
+      json = JSON.parse response.body
+      expect(json['course']['workflow_state']).to eq 'claimed'
+      @course.reload
+      expect(@course.workflow_state).to eq 'claimed'
+    end
+
+    it "returns an error if a bad event is given" do
+      user_session(@teacher)
+      put 'update', params: {:id => @course.id, :course => {:event => 'boogie'}, :format => :json}
+      expect(response.status).to eq 400
+      json = JSON.parse response.body
+      expect(json['errors'].keys).to include 'workflow_state'
+    end
+
     it "should lock active course announcements" do
       user_session(@teacher)
       active_announcement  = @course.announcements.create!(:title => 'active', :message => 'test')