ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

add syllabus to blueprint sync history 

test plan (ADMIN-51):
 - have a blueprint syncing to multiple associated courses
 - change the syllabus text in the blueprint and in one associated course
 - in the blueprint, the "Unsynced Changes" should include the syllabus
   and the Sync button should be present
 - perform a sync
 - the syllabus text should be updated in courses where the syllabus
   was not changed downstream
 - the sync history in the blueprint course should show the syllabus
   was synced, and should show the exception in the course whose
   syllabus was changed downstream
 - the sync history in the associated courses should show that the
   syllabus was changed and indicate whether this change was applied
   (course settings / Blueprint information in the right sidebar)

test plan (ADMIN-1283)
 - have a blueprint and associated course that are syncing
   a syllabus body
 - detach the associated course and attach it to a different
   blueprint
 - the associated course's syllabus body should be overwritten
   with the new blueprint's syllabus body after a sync happens
 - changing the syllabus body in the associated course should
   prevent future syncs from overwriting the syllabus

also regression test:
 - course settings (e.g. tab order) are copied to a newly added
   course
 - course settings are overwritten in child courses if the
   "copy course settings" box is checked

fixes ADMIN-51
fixes ADMIN-1283

Change-Id: I44f7086746f279059d5bb86e177ceb8f18a15e56
Reviewed-on: https://gerrit.instructure.com/158247
Tested-by: Jenkins
Reviewed-by: James Williams  <jamesw@instructure.com>
QA-Review: Luke Kingsley <lkingsley@instructure.com>
Product-Review: Jeremy Stanley <jeremy@instructure.com>
This commit is contained in:
Jeremy Stanley 2018-07-20 16:26:00 -06:00
parent d8f93be62e
commit 1966f31381
13 changed files with 108 additions and 38 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
app/controllers/master_courses/master_templates_controller.rb
View File

@ -310,7 +310,7 @@ class MasterCourses::MasterTemplatesController < ApplicationController
end
if ids_to_remove.any?
@template.child_subscriptions.active.where(:child_course_id => ids_to_remove).preload(:child_course => :wiki).each(&:destroy)
@template.child_subscriptions.active.where(:child_course_id => ids_to_remove).preload(:child_course).each(&:destroy)
end
render :json => {:success => true}
@ -463,6 +463,7 @@ class MasterCourses::MasterTemplatesController < ApplicationController
locked = !!tag&.restrictions&.values&.any?
changed_asset_json(asset, action, locked)
end
changes << changed_syllabus_json(@course) if @course.syllabus_updated_at&.>(cutoff_time)
render :json => changes
end
@ -631,6 +632,7 @@ class MasterCourses::MasterTemplatesController < ApplicationController
next unless result = results[sub.id]
skipped_items = result[:skipped]
next unless skipped_items.present?
get_syllabus_exception!(skipped_items, sub, exceptions)
sub.content_tags.where(:migration_id => skipped_items).each do |child_tag|
exceptions[child_tag.migration_id] ||= []
exceptions[child_tag.migration_id] << { :course_id => sub.child_course_id,
@ -641,9 +643,17 @@ class MasterCourses::MasterTemplatesController < ApplicationController
exceptions
end
def get_syllabus_exception!(skipped_items, child_subscription, exceptions)
if skipped_items.delete(:syllabus)
exceptions['syllabus'] ||= []
exceptions['syllabus'] << { :course_id => child_subscription.child_course_id, :conflicting_changes => ['content'] }
end
end
def render_changes(tag_association, subscriptions)
changes = []
exceptions = get_exceptions_by_subscription(subscriptions)
updated_syllabus = @mm.export_results[:selective][:updated].delete('syllabus')
[:created, :updated, :deleted].each do |action|
migration_ids = @mm.export_results[:selective][action].values.flatten
@ -655,6 +665,7 @@ class MasterCourses::MasterTemplatesController < ApplicationController
tag.migration_id, exceptions)
end
end
changes << changed_syllabus_json(@course, exceptions) if updated_syllabus
render :json => changes
end

3
app/jsx/blueprint_courses/labels.js
View File

@ -33,7 +33,8 @@ const itemTypeLabels = {
learning_outcome_group: I18n.t('Outcome Group'),
rubric: I18n.t('Rubric'),
context_external_tool: I18n.t('External Tool'),
folder: I18n.t('Folder')
folder: I18n.t('Folder'),
syllabus: I18n.t('Syllabus')
}
const itemTypeLabelPlurals = {

2
app/jsx/blueprint_courses/propTypes.js
View File

@ -66,7 +66,7 @@ propTypes.migrationExceptionList = arrayOf(propTypes.migrationException)
propTypes.migrationChange = shape({
asset_id: string.isRequired,
asset_type: oneOf(['assignment', 'quiz', 'discussion_topic', 'wiki_page', 'attachment', 'context_module', 'learning_outcome', 'learning_outcome_group', 'announcement']).isRequired,
asset_type: oneOf(['assignment', 'quiz', 'discussion_topic', 'wiki_page', 'attachment', 'context_module', 'learning_outcome', 'learning_outcome_group', 'announcement', 'rubric', 'syllabus']).isRequired,
asset_name: string.isRequired,
change_type: oneOf(['created', 'updated', 'deleted']).isRequired,
htnl_url: string,

13
app/models/content_export.rb
View File

@ -343,7 +343,6 @@ class ContentExport < ActiveRecord::Base
#
# Returns: bool
def export_symbol?(symbol)
return false if symbol == :all_course_settings && should_skip_course_settings?
selected_content.empty? || is_set?(selected_content[symbol]) || is_set?(selected_content[:everything])
end
@ -367,18 +366,6 @@ class ContentExport < ActiveRecord::Base
@selective_export
end
def should_skip_course_settings?
if for_master_migration?
if master_migration.migration_settings.has_key?(:copy_settings)
!master_migration.migration_settings[:copy_settings]
else
selective_export?
end
else
false
end
end
def exported_assets
@exported_assets ||= Set.new
end

5
app/models/content_migration.rb
View File

@ -615,9 +615,10 @@ class ContentMigration < ActiveRecord::Base
self.migration_type == 'master_course_import'
end
def add_skipped_item(child_tag)
def add_skipped_item(item)
@skipped_master_course_items ||= Set.new
@skipped_master_course_items << child_tag.migration_id
item = item.migration_id if item.is_a?(MasterCourses::ChildContentTag)
@skipped_master_course_items << item
end
def process_master_deletions(deletions)

12
app/models/course.rb
View File

@ -31,7 +31,7 @@ class Course < ActiveRecord::Base
include OutcomeImportContext
attr_accessor :teacher_names, :master_course
attr_writer :student_count, :primary_enrollment_type, :primary_enrollment_role_id, :primary_enrollment_rank, :primary_enrollment_state, :primary_enrollment_date, :invitation, :updating_master_template_id
attr_writer :student_count, :primary_enrollment_type, :primary_enrollment_role_id, :primary_enrollment_rank, :primary_enrollment_state, :primary_enrollment_date, :invitation, :master_migration
time_zone_attribute :time_zone
def time_zone
@ -1138,11 +1138,14 @@ class Course < ActiveRecord::Base
def handle_syllabus_changes_for_master_migration
if self.syllabus_body_changed?
if @updating_master_template_id
self.syllabus_updated_at = Time.now.utc
if @master_migration
updating_master_template_id = @master_migration.master_course_subscription.master_template_id
# master migration sync
self.syllabus_master_template_id ||= @updating_master_template_id if self.syllabus_body_was.blank? # sync if there was no syllabus before
if self.syllabus_master_template_id.to_i != @updating_master_template_id
self.syllabus_master_template_id ||= updating_master_template_id if self.syllabus_body_was.blank? # sync if there was no syllabus before
if self.syllabus_master_template_id.to_i != updating_master_template_id
self.restore_syllabus_body! # revert the change
@master_migration.add_skipped_item(:syllabus)
end
elsif self.syllabus_master_template_id
# local change - remove the template id to prevent future syncs
@ -2790,6 +2793,7 @@ class Course < ActiveRecord::Base
add_setting :timetable_data, :arbitrary => true
add_setting :syllabus_master_template_id
add_setting :syllabus_updated_at
def user_can_manage_own_discussion_posts?(user)
return true if allow_student_discussion_editing?

2
app/models/importers/course_content_importer.rb
View File

@ -298,7 +298,7 @@ module Importers
def self.import_syllabus_from_migration(course, syllabus_body, migration)
if migration.for_master_course_import?
course.updating_master_template_id = migration.master_course_subscription.master_template_id
course.master_migration = migration
end
course.syllabus_body = migration.convert_html(syllabus_body, :syllabus, nil, :syllabus)
end

15
app/models/master_courses/child_subscription.rb
View File

@ -31,7 +31,8 @@ class MasterCourses::ChildSubscription < ActiveRecord::Base
end
end
before_save :check_migration_id_deactivation
after_create :link_syllabus!
before_update :check_migration_id_deactivation
after_save :invalidate_course_cache
@ -61,9 +62,11 @@ class MasterCourses::ChildSubscription < ActiveRecord::Base
# mess up the migration ids so restrictions no longer get applied
if workflow_state_changed?
if deleted? && workflow_state_was == 'active'
self.unlink_syllabus!
self.add_deactivation_prefix!
elsif active? && workflow_state_was == 'deleted'
self.use_selective_copy = false # require a full import next time
self.link_syllabus!
self.remove_deactivation_prefix!
end
end
@ -74,6 +77,16 @@ class MasterCourses::ChildSubscription < ActiveRecord::Base
"deletedsub_#{self.id}_"
end
def link_syllabus!
self.child_course.syllabus_master_template_id = self.master_template_id
self.child_course.save!
end
def unlink_syllabus!
self.child_course.syllabus_master_template_id = nil
self.child_course.save!
end
def add_deactivation_prefix!
where_clause = ["migration_id LIKE ?", "#{MasterCourses::MasterTemplate.migration_id_prefix(self.shard.id, self.master_template_id)}%"]
update_query = ["migration_id = concat(?, migration_id)", self.deactivation_prefix]

23
app/models/master_courses/master_migration.rb
View File

@ -181,7 +181,7 @@ class MasterCourses::MasterMigration < ActiveRecord::Base
ce.settings[:master_migration_type] = type
ce.settings[:master_migration_id] = self.id # so we can find on the import side when we copy attachments
ce.settings[:primary_master_migration] = is_primary
ce.settings[:selected_content] = {}
ce.settings[:selected_content] = selected_content(type)
ce.user = self.user
ce.save!
ce.master_migration = self # don't need to reload
@ -190,10 +190,24 @@ class MasterCourses::MasterMigration < ActiveRecord::Base
ce.settings[:referenced_file_migration_ids] = ce.referenced_files.values
ce.save!
end
detect_updated_attachments(type) if ce.exported_for_course_copy? && is_primary
if ce.exported_for_course_copy? && is_primary
detect_updated_attachments(type)
detect_updated_syllabus(type, ce)
end
ce
end
def selected_content(type)
{}.tap do |h|
h[:all_course_settings] = if migration_settings.has_key?(:copy_settings)
migration_settings[:copy_settings]
else
type == :full
end
h[:syllabus_body] = type == :full || master_template.course.syllabus_updated_at&.>(last_export_at)
end
end
def last_export_at
self.master_template.last_export_started_at
end
@ -218,6 +232,11 @@ class MasterCourses::MasterMigration < ActiveRecord::Base
end
end
def detect_updated_syllabus(type, content_export)
selected_content = content_export.settings[:selected_content]
@updates['syllabus'] = true if @updates && selected_content && selected_content[:syllabus_body]
end
def add_exported_asset(asset)
return unless @export_type == :selective
@export_count += 1

13
lib/api/v1/master_courses.rb
View File

@ -68,4 +68,17 @@ module Api::V1::MasterCourses
json[:exceptions] = exceptions[migration_id] || [] unless migration_id.nil?
json
end
def changed_syllabus_json(course, exceptions=nil)
{
asset_id: course.id,
asset_type: 'syllabus',
asset_name: I18n.t('Syllabus'),
change_type: :updated,
html_url: syllabus_course_assignments_url(course),
locked: false
}.tap do |json|
json[:exceptions] = exceptions['syllabus'] if exceptions
end
end
end

40
spec/apis/v1/master_courses/master_templates_api_spec.rb
View File

@ -394,19 +394,22 @@ describe MasterCourses::MasterTemplatesController, type: :request do
describe "migration_details / import_details" do
before :once do
setup_template
@master = @course
@minions = (1..2).map do |n|
@template.add_child_course!(course_factory(:name => "Minion #{n}", :active_all => true)).child_course
end
Timecop.travel(1.hour.ago) do
setup_template
@master = @course
@minions = (1..2).map do |n|
@template.add_child_course!(course_factory(:name => "Minion #{n}", :active_all => true)).child_course
end
# set up some stuff
@file = attachment_model(:context => @master, :display_name => 'Some File')
@assignment = @master.assignments.create! :title => 'Blah', :points_possible => 10
@full_migration = run_master_migration
# set up some stuff
@file = attachment_model(:context => @master, :display_name => 'Some File')
@assignment = @master.assignments.create! :title => 'Blah', :points_possible => 10
@full_migration = run_master_migration
end
# prepare some exceptions
@minions.first.attachments.first.update_attribute :display_name, 'Some Renamed Nonsense'
@minions.first.syllabus_body = 'go away'; @minions.first.save!
@minions.last.assignments.first.update_attribute :points_possible, 11
# now push some incremental changes
@ -417,6 +420,7 @@ describe MasterCourses::MasterTemplatesController, type: :request do
@quiz = @master.quizzes.create! :title => 'TestQuiz'
@file.update_attribute :display_name, 'I Can Rename Files Too'
@assignment.destroy
@master.syllabus_body = 'syllablah frd'; @master.save!
run_master_migration
end
@ -434,7 +438,10 @@ describe MasterCourses::MasterTemplatesController, type: :request do
"locked"=>false,"exceptions"=>[{"course_id"=>@minions.last.id, "conflicting_changes"=>["points"]}]},
{"asset_id"=>@file.id,"asset_type"=>"attachment","asset_name"=>"I Can Rename Files Too",
"change_type"=>"updated","html_url"=>"http://www.example.com/courses/#{@master.id}/files/#{@file.id}",
"locked"=>false,"exceptions"=>[{"course_id"=>@minions.first.id, "conflicting_changes"=>["content"]}]}
"locked"=>false,"exceptions"=>[{"course_id"=>@minions.first.id, "conflicting_changes"=>["content"]}]},
{"asset_id"=>@master.id,"asset_type"=>"syllabus","asset_name"=>"Syllabus","change_type"=>"updated",
"html_url"=>"http://www.example.com/courses/#{@master.id}/assignments/syllabus","locked"=>false,
"exceptions"=>[{"course_id"=>@minions.first.id,"conflicting_changes"=>["content"]}]}
])
end
@ -460,7 +467,10 @@ describe MasterCourses::MasterTemplatesController, type: :request do
"html_url"=>"http://www.example.com/courses/#{minion.id}/assignments/#{minion_assignment.id}","locked"=>false,"exceptions"=>[]},
{"asset_id"=>minion_file.id,"asset_type"=>"attachment","asset_name"=>"Some Renamed Nonsense",
"change_type"=>"updated","html_url"=>"http://www.example.com/courses/#{minion.id}/files/#{minion_file.id}",
"locked"=>false,"exceptions"=>[{"course_id"=>minion.id, "conflicting_changes"=>["content"]}]}
"locked"=>false,"exceptions"=>[{"course_id"=>minion.id, "conflicting_changes"=>["content"]}]},
{"asset_id"=>minion.id,"asset_type"=>"syllabus","asset_name"=>"Syllabus","change_type"=>"updated",
"html_url"=>"http://www.example.com/courses/#{minion.id}/assignments/syllabus","locked"=>false,
"exceptions"=>[{"course_id"=>minion.id,"conflicting_changes"=>["content"]}]}
])
end
@ -488,6 +498,11 @@ describe MasterCourses::MasterTemplatesController, type: :request do
:id => minion_migration.to_param, :course_id => @minions.first.to_param, :action => 'import_details' },
{}, {}, { :expected_status => 401 })
end
it "syncs syllabus content unless changed downstream" do
expect(@minions.first.reload.syllabus_body).to include "go away"
expect(@minions.last.reload.syllabus_body).to include "syllablah frd"
end
end
describe 'unsynced_changes' do
@ -511,6 +526,7 @@ describe MasterCourses::MasterTemplatesController, type: :request do
@file.update_attribute(:display_name, 'Renamed')
@folder.update_attribute(:name, 'Blergh')
@new_page = @master.wiki_pages.create! :title => 'New News'
@master.syllabus_body = "srslywat"; @master.save!
json = api_call_as_user(@admin, :get, "/api/v1/courses/#{@master.id}/blueprint_templates/default/unsynced_changes",
:controller => 'master_courses/master_templates', :format => 'json', :template_id => 'default',
@ -524,6 +540,8 @@ describe MasterCourses::MasterTemplatesController, type: :request do
"html_url"=>"http://www.example.com/courses/#{@master.id}/pages/new-news","locked"=>false},
{"asset_id"=>@folder.id,"asset_type"=>"folder","asset_name"=>"Blergh","change_type"=>"updated",
"html_url"=>"http://www.example.com/courses/#{@master.id}/folders/#{@folder.id}","locked"=>false},
{"asset_id"=>@master.id,"asset_type"=>"syllabus","asset_name"=>"Syllabus","change_type"=>"updated",
"html_url"=>"http://www.example.com/courses/#{@master.id}/assignments/syllabus","locked"=>false}
])
end

3
spec/models/master_courses/child_subscription_spec.rb
View File

@ -55,6 +55,7 @@ describe MasterCourses::ChildSubscription do
@template = MasterCourses::MasterTemplate.set_as_master_course(master_course)
child_course = course_factory
sub = @template.add_child_course!(@course)
expect(child_course.reload.syllabus_master_template_id).to eq @template.id.to_s
original_page = master_course.wiki_pages.create!(:title => "blah")
mc_tag = @template.create_content_tag_for!(original_page)
@ -65,10 +66,12 @@ describe MasterCourses::ChildSubscription do
sub.destroy!
expect(page_copy.reload.migration_id).to eq (sub.deactivation_prefix + mc_tag.migration_id)
expect(child_tag.reload.migration_id).to eq (sub.deactivation_prefix + mc_tag.migration_id)
expect(child_course.reload.syllabus_master_template_id).to be_empty
sub.undestroy
expect(page_copy.reload.migration_id).to eq mc_tag.migration_id
expect(child_tag.reload.migration_id).to eq mc_tag.migration_id
expect(child_course.reload.syllabus_master_template_id).to eq @template.id.to_s
end
end
end

2
spec/models/master_courses/master_migration_spec.rb
View File

@ -1169,8 +1169,8 @@ describe MasterCourses::MasterMigration do
@copy_to2 = course_factory
child_syllabus1 = "<p>some child syllabus</p>"
@copy_to2.update_attribute(:syllabus_body, child_syllabus1)
@template.add_child_course!(@copy_to2)
@copy_to2.update_attribute(:syllabus_body, child_syllabus1)
master_syllabus1 = "<p>some original syllabus</p>"
@copy_from.update_attribute(:syllabus_body, master_syllabus1)