From 29bd44354a0330b72fa6d6e86f431d79560a437e Mon Sep 17 00:00:00 2001 From: Ahmad Amireh Date: Fri, 4 Jul 2014 01:24:27 +0300 Subject: [PATCH] Draft-State Quizzes - "Save It Now" should work Closes CNVS-13730 This patch enables the (re)publishing functionality in DS mode. The issue was that the internals were relying on "workflow_state_changed?" to be truthy in order to re-publish the quiz, which is not the usual case in DS where the quiz's WState is always "available" and doesn't really change between authoring updates. A side change: Canvas::DraftStateValidations is now covered and responsible for adding the required validations to classes that mix it in. TEST PLAN ---- ---- - create a quiz with draft-state on - add a question - save the quiz, publish it - open a tab on the quiz show page and keep it - in another tab, edit the quiz, modify the question, hit "Update Question" and switch back to the show quiz tab - refresh the page, verify you get the "Save it Now" button - push the button - refresh again - verify that the button no longer re-appears, and that your changes were actually published (you can do this by taking the quiz as a student) Change-Id: Idf3b4302b23d6abc483b80796fee753b0e6fba85 Reviewed-on: https://gerrit.instructure.com/37303 Reviewed-by: Derek DeVries Tested-by: Jenkins QA-Review: Caleb Guanzon Product-Review: Ahmad Amireh --- app/models/assignment.rb | 1 - app/models/quizzes/quiz.rb | 32 +++++++-------- lib/canvas/draft_state_validations.rb | 6 +++ .../canvas/draft_state_validations_spec.rb | 40 +++++++++++++++++++ spec/models/quizzes/quiz_spec.rb | 34 ++++++++++++++++ 5 files changed, 96 insertions(+), 17 deletions(-) create mode 100644 spec/lib/canvas/draft_state_validations_spec.rb diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 3c947708521..ef405bc333b 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -75,7 +75,6 @@ class Assignment < ActiveRecord::Base has_one :external_tool_tag, :class_name => 'ContentTag', :as => :context, :dependent => :destroy validates_associated :external_tool_tag, :if => :external_tool? - validate :validate_draft_state_change, :if => :workflow_state_changed? validate :group_category_changes_ok? validate :positive_points_possible? diff --git a/app/models/quizzes/quiz.rb b/app/models/quizzes/quiz.rb index 03f3627fb1f..7bfb407eeda 100644 --- a/app/models/quizzes/quiz.rb +++ b/app/models/quizzes/quiz.rb @@ -73,7 +73,6 @@ class Quizzes::Quiz < ActiveRecord::Base validate :validate_quiz_type, :if => :quiz_type_changed? validate :validate_ip_filter, :if => :ip_filter_changed? validate :validate_hide_results, :if => :hide_results_changed? - validate :validate_draft_state_change, :if => :workflow_state_changed? validate :validate_correct_answer_visibility, :if => lambda { |quiz| quiz.show_correct_answers_at_changed? || quiz.hide_correct_answers_at_changed? @@ -81,7 +80,7 @@ class Quizzes::Quiz < ActiveRecord::Base sanitize_field :description, CanvasSanitize::SANITIZE copy_authorized_links(:description) { [self.context, nil] } - before_save :generate_quiz_data_on_publish + before_save :generate_quiz_data_on_publish, :if => :needs_republish? before_save :build_assignment before_save :set_defaults before_save :flag_columns_that_need_republish @@ -149,25 +148,26 @@ class Quizzes::Quiz < ActiveRecord::Base # generate quiz data and update time when we publish. This method makes it # harder to mess up (like someone setting using workflow_state directly) def generate_quiz_data_on_publish - # when draft state is turned on permanently, remove this conditional, the - # @publishing ivar from publish!, and change the filter to: - # - # before_save :generate_quiz_data_on_publish, :if => :workflow_state_changed? - # - if context.feature_enabled?(:draft_state) - return unless workflow_state_changed? - - # pre-draft state we need ability to republish things. Since workflow_state - # is stays available, we need to flag when we're forcing to publish! - else - return unless @publishing - end - if workflow_state == 'available' self.generate_quiz_data self.published_at = Time.zone.now end end + private :generate_quiz_data_on_publish + + # @return [Boolean] Whether the quiz has unsaved changes due for a republish. + def needs_republish? + # TODO: remove this conditional and the non-DS scenario once Draft State is + # permanently turned on + if context.feature_enabled?(:draft_state) + return true if @publishing || workflow_state_changed? + + # pre-draft state we need ability to republish things. Since workflow_state + # stays available, we need to flag when we're forcing to publish! + else + return true if @publishing + end + end # some attributes require us to republish for non-draft state # We can safely remove this when draft state is permanent diff --git a/lib/canvas/draft_state_validations.rb b/lib/canvas/draft_state_validations.rb index 05ac8f7dcd7..aeaead04523 100644 --- a/lib/canvas/draft_state_validations.rb +++ b/lib/canvas/draft_state_validations.rb @@ -1,5 +1,11 @@ module Canvas module DraftStateValidations + def self.included(base) + base.class_eval do + validate :validate_draft_state_change, :if => :workflow_state_changed? + end + end + def validate_draft_state_change old_draft_state, new_draft_state = self.changes['workflow_state'] return if old_draft_state == new_draft_state diff --git a/spec/lib/canvas/draft_state_validations_spec.rb b/spec/lib/canvas/draft_state_validations_spec.rb new file mode 100644 index 00000000000..20ae9823288 --- /dev/null +++ b/spec/lib/canvas/draft_state_validations_spec.rb @@ -0,0 +1,40 @@ +# +# Copyright (C) 2011 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 . +# + +require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb') + +describe Canvas::DraftStateValidations do + shared_examples_for "Canvas::DraftStateValidations" do + describe ":validate_draft_state_change" do + it "should work" do + subject.workflow_state = 'unpublished' + subject.stubs(has_student_submissions?: true) + subject.stubs(workflow_state_changed?: true) + subject.stubs({ + changes: { 'workflow_state' => [ 'published', 'unpublished' ] } + }) + subject.save + + subject.errors[:workflow_state].should be_present + subject.errors[:workflow_state][0].to_s.should match( + %r{can't unpublish if there are student submissions}i + ) + end + end + end +end diff --git a/spec/models/quizzes/quiz_spec.rb b/spec/models/quizzes/quiz_spec.rb index 61a4e4fb8f5..42b841b5d88 100644 --- a/spec/models/quizzes/quiz_spec.rb +++ b/spec/models/quizzes/quiz_spec.rb @@ -17,6 +17,7 @@ # require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb') +require File.expand_path(File.dirname(__FILE__) + '/../../lib/canvas/draft_state_validations_spec.rb') describe Quizzes::Quiz do before :once do @@ -103,6 +104,8 @@ describe Quizzes::Quiz do end end + it_should_behave_like 'Canvas::DraftStateValidations' + it "should flag as edited if shuffle answers changes to off" do q = @course.quizzes.create!(:title => "new quiz") q.quiz_questions.create! @@ -1171,6 +1174,37 @@ describe Quizzes::Quiz do end end + describe '#needs_republish?' do + subject { @course.quizzes.create!(title: 'Test Quiz') } + + it 'should be true if publish! was manually called' do + subject.needs_republish?.should be_false + + # intercepting the call to save! and running our expectations there + # because by the time it's saved, #needs_republish? will be reset + subject.expects(:save!).with { |*args| + subject.needs_republish?.should be_true + true + } + + subject.publish! + end + + context 'with draft-state' do + before do + subject.context.root_account.enable_feature!(:draft_state) + end + + it 'should be true if the workflow_state has changed' do + subject.workflow_state = 'deleted' + subject.save! + subject.reload + subject.workflow_state = 'available' + subject.needs_republish?.should be_true + end + end + end + describe "#current_regrade" do before(:once) { @quiz = @course.quizzes.create! title: 'Test Quiz' }