The problem:
ContextModule#add_item is called when creating a new module item
(including through the UI, not during course copy), or during course
copy, including updating an existing module item. During course copy to
a course where the module item already exists, such as a blueprint
course sync, the code in add_item() previously tried to overwrite the
existing associated_asset and create a new Lti::ResourceLink with the
same lookup_uuid. Because a Lti::ResourceLink already exists with that
lookup_uuid, the validation would fail. This would cause two things:
* In add_item(), when saving the added_item with the invalid
associated_asset, the added_item's `associated_item_type` would be set
to "Lti::ResourceLink", but the `associated_item_id` would be set to
nil. Regardless, the added_item was being successfully saved without
error.
* Because we used `context.lti_resource_links.new(...)`, the invalid
Lti::ResourceLink would be in `context.lti_resource_links`. Much later
in the code, in `course_content_importer.rb`, on this line:
course.save! if course.changed?
`course.save!` would throw an error (crash the import) because one of
the course's lti_resource_links was invalid.
The solution:
* Use existing Lti::ResourceLinks if one exists with the lookup_uuid,
instead of trying to create a duplicate.
* If the module item already has a resource link (associated_asset) we
don't bother changing it since a module item can never have its
resource link changed anywhere else in the code.
* If for some reason the Lti::ResourceLink is invalid (which it
shouldn't be now), use `migration.add_issue` instead of crashing the
whole import where we do the `course.save!`.
This commit also removes duplicative code in lti_resource_link_importer,
and DRYs up code there and in ContextModule#add_item by creating a new
Lti::ResourceLink.find_or_initialize_for_context_and_lookup_uuid method.
flag=none
closes INTEROP-7036
Test plan:
* It's sometimes tricky to trigger the actual crash, which happens when
there are changes to the actual course to be saved, so it's best when
testing to add some debugging write before the
"course.save! if course.changed?" line in
app/models/importers/course_content_importer.rb (line 192), like this:
puts "DEBUG-TEMP: changed=#{course.changed?} valid=#{course.valid?}"
In all runs, course.valid? should be true even if it the course has
not changed.
* Run a jobs container
* Create a course with one or two module items that are an LTI tool. You
can put some custom parameters in there (when making the item with deep
linking) if you like.
* Make the course a blueprint course and add an associated/child course
that will be the destination course when syncing
* Change the module item (you can just edit it and change the name) at
least twice and sync changes (in the upper right click the blueprint
icon). To attempt to trigger the actual crash, try editing the
blueprint's course settings and changing the navigation (making items
hidden or shown again), and when running the sync, choose "Include
course settings". After each change, from a rails console, check the
following:
* the destination course's module items (stored as ContentTags in the
database) should have an association_asset with that is an
Lti::ResourceLink with the same lookup_uuid as the source course's
(but the resource links' context will be the destination course) (
* also check that the name change(s) you made gets updated in the
destination courses' module item(s).
* check the debugging you added ("DEBUG-TEMP") that the course is
valid every time
* Modify one of the destination course's modules item by setting its
associated_asset_id to null. This simulates the state of module items
in the DB due to the bug. Run a blueprint sync and make sure the
associated_asset_id gets fixed to what it was before (the existing
Lti::ResourceLink)
* Modify the custom parameters of a resource link (may have to do this
in a rails console) in the blueprint course and run a sync. Check
that the custom parameters in the resource link of the destination
course (for that lookup_uuid) have also been updated.
* Add a new module item in the blueprint course, but before you sync,
modify the call to
Lti::ResourceLink.find_or_initialize_for_context_and_lookup_uuid in
context_module.rb to have it send in a nil context_external_tool.
Restart the jobs container and run a sync. The sync should not
crash. (A warning like 'The External Tool resource
link (including any possible custom parameters) could not be set
for module item "%{title}"' should be made on the migration, but
migration warnings appear to not be shown in the UI anywhere for
blueprint course syncs.)
Change-Id: I9bc946e935653796bb19bb23ad2deb62df3986df
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/273602
QA-Review: Mysti Lilla <mysti@instructure.com>
Product-Review: Evan Battaglia <ebattaglia@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Mysti Lilla <mysti@instructure.com>
dc0e7b50e8