diff --git a/node_modules/col-canvas/lib/poller.js b/node_modules/col-canvas/lib/poller.js index 9c463595..13e98664 100644 --- a/node_modules/col-canvas/lib/poller.js +++ b/node_modules/col-canvas/lib/poller.js @@ -482,8 +482,7 @@ var handleSubmission = function(ctx, users, assignment, activities, submission, 'objectType': CollabosphereConstants.ACTIVITY.OBJECT_TYPES.CANVAS_SUBMISSION, 'metadata': { 'submission_id': submission.id, - 'attempt': submission.attempt, - 'attachment_ids': _.pluck(submission.attachments, 'id') + 'attempt': submission.attempt }, 'user': submission.user_id }; @@ -496,75 +495,104 @@ var handleSubmission = function(ctx, users, assignment, activities, submission, return callback(); } - // Create a context for the user who created the submission. This will ensure that - // any activities are created in their name - ctx = { - 'course': ctx.course, - 'user': users[submission.user_id] - }; - - // Get the attachments the user previously submitted, if any - var filters = { - 'user': ctx.user.id, - 'assignment': assignment.id - }; - AssetsAPI.getAssets(ctx, filters, null, null, function(err, assets) { + // Before we do anything further, we update the activity so the activity is in sync with the data + // from the Canvas REST API. If we didn't do this first, we might end up in a situation where we + // endlessly remove and create new assets. Assume we update the activity as the very last thing we do + // and the following scenario happens: + // - A user re-submits an assignments + // - The poller will: + // 1. Get the activity from the DB for this user's submission + // 2. As the submission attempt number is 2 and the activity's is 1, a resubmission is detected + // 3. Therefore the poller will, delete the assets from the old submission + // 4. Create assets for the new submission + // 5. Update the activity in the database with the attempt number + // + // If step 5 fails for whatever reason, the next time the poller runs, each step will be repeated. + // This means that assets will be removed and the same assets will remain at the top of the asset + // library. This is unacceptable, so we update the activity first. + // + // When a submission is dealt with for the first time, this method won't do anything + updateActivityIfNecessary(retrievedActivity, submission, function(err) { if (err) { return callback(err); } - // Delete any assets the user previously submitted for this assignment from the asset library. - // Note that this won't actually delete the records from the database as we still need - // activities such as liking, adding comments, etc.. to be retained - var assetIds = _.pluck(assets.results, 'id'); - AssetsAPI.deleteAssets(ctx, assetIds, function(err) { + // Create a context for the user who created the submission. This will ensure that + // any activities are created in their name + ctx = { + 'course': ctx.course, + 'user': users[submission.user_id] + }; + + // Get the attachments the user previously submitted, if any + var filters = { + 'user': ctx.user.id, + 'assignment': assignment.id + }; + AssetsAPI.getAssets(ctx, filters, null, null, function(err, assets) { if (err) { - log.error({ - 'assets': assetIds, - 'err': err - }, 'Failed to delete a set of assets'); return callback(err); } - // Create a link asset for url submissions - if (submission.submission_type === 'online_url') { - var opts = { - 'assignment': assignment.id, - 'categories': [assignment.category.id], - 'skipCreateActivity': true - }; - return AssetsAPI.createLink(ctx, null, submission.url, opts, callback); - - // Create a file asset for each attachment on an "upload" submission - } else if (submission.submission_type === 'online_upload') { - var handleFilesSubmissionAttachmentIterator = handleFilesSubmissionAttachment.bind(null, ctx, assignment); - async.eachSeries(submission.attachments, handleFilesSubmissionAttachmentIterator, function(err) { - if (err) { - return callback(err); - } - - // Update the activity to indicate this attempt has been seen - if (retrievedActivity.metadata.attempt !== submission.attempt) { - var metadata = { - 'submission_id': submission.id, - 'attempt': submission.attempt, - 'attachment_ids': _.pluck(submission.attachments, 'id') - }; - return ActivitiesAPI.updateActivity(retrievedActivity, {'metadata': metadata}, callback); - } else { - return callback(); - } - }); - - // Ignore all other types of submissions - } else { - return callback(); - } + // Delete any assets the user previously submitted for this assignment from the asset library. + // Note that this won't actually delete the records from the database as we still need + // activities such as liking, adding comments, etc.. to be retained + var assetIds = _.pluck(assets.results, 'id'); + AssetsAPI.deleteAssets(ctx, assetIds, function(err) { + if (err) { + log.error({ + 'assets': assetIds, + 'err': err + }, 'Failed to delete a set of assets'); + return callback(err); + } + + // Create a link asset for url submissions + if (submission.submission_type === 'online_url') { + var opts = { + 'assignment': assignment.id, + 'categories': [assignment.category.id], + 'skipCreateActivity': true + }; + return AssetsAPI.createLink(ctx, null, submission.url, opts, callback); + + // Create a file asset for each attachment on an "upload" submission + } else if (submission.submission_type === 'online_upload') { + var handleFilesSubmissionAttachmentIterator = handleFilesSubmissionAttachment.bind(null, ctx, assignment); + return async.eachSeries(submission.attachments, handleFilesSubmissionAttachmentIterator, callback); + + // Ignore all other types of submissions + } else { + return callback(); + } + }); }); }); }); }; +/** + * Update the given activity with the submission's data + * + * @param {Activity} activity The activity to update + * @param {Object} submission The submission whose data should be persisted + * @param {Function} callback Standard callback function + * @param {Object} callback.err An error that occurred, if any + * @api private + */ +var updateActivityIfNecessary = function(activity, submission, callback) { + // Update the activity to indicate this attempt has been seen + if (activity.metadata.attempt !== submission.attempt) { + var metadata = { + 'submission_id': submission.id, + 'attempt': submission.attempt + }; + return ActivitiesAPI.updateActivity(activity, {'metadata': metadata}, callback); + } else { + return callback(); + } +}; + /** * Download a submission's file attachment and create a file asset for it in the asset library * @@ -887,7 +915,7 @@ var getOrCreateActivity = function(ctx, activities, users, activity, callback) { var msg = 'A user linked with an activity could not be found. This can happen when a user has made some ' + 'submissions or discussion entries but was removed from the course before the Collabosphere ' + 'tools were added to the course. No activity will be created!'; - log.warn({ + log.debug({ 'activity': activity, 'course': ctx.course.id, 'canvas_course_id': ctx.course.canvas_course_id, diff --git a/node_modules/col-canvas/tests/test-poller.js b/node_modules/col-canvas/tests/test-poller.js index 0c34755c..92421840 100644 --- a/node_modules/col-canvas/tests/test-poller.js +++ b/node_modules/col-canvas/tests/test-poller.js @@ -688,6 +688,75 @@ describe('Canvas poller', function() { }); }); + /** + * Test that verifies that the poller only creates a new asset once when resubmitting an assignment + */ + it('only creates a new asset once when resubmitting an assignment', function(callback) { + getClient(null, function(client, course, user) { + + // Get the actual course object so we can pass it into the poller + getCourse(course.id, function(dbCourse) { + + // Submit an assignment + var submission = new CanvasSubmission(user.id, 'online_url', 'http://www.google.com'); + var assignments = [ + new CanvasAssignment(course.id, [submission]) + ]; + CanvasTestsUtil.mockPollingRequests(dbCourse, [], assignments, []); + CanvasPoller.handleCourse(dbCourse, function(err) { + assert.ok(!err); + + // Assert the asset was created + AssetsTestsUtil.assertGetAssets(client, course, null, null, null, 1, function(assets) { + var preResubmitAsset = assets.results[0]; + assert.strictEqual(preResubmitAsset.url, 'http://www.google.com'); + + // Do another poll and ensure the asset does not get recreated + CanvasTestsUtil.mockPollingRequests(dbCourse, [], assignments, []); + CanvasPoller.handleCourse(dbCourse, function(err) { + assert.ok(!err); + + // Assert the asset was not recreated + AssetsTestsUtil.assertGetAssets(client, course, null, null, null, 1, function(assets) { + var postSecondRunAsset = assets.results[0]; + assert.strictEqual(postSecondRunAsset.url, 'http://www.google.com'); + assert.strictEqual(postSecondRunAsset.id, preResubmitAsset.id); + + // Resubmit the assignment + submission.attempt++; + submission.url = 'http://www.yahoo.com'; + CanvasTestsUtil.mockPollingRequests(dbCourse, [], assignments, []); + CanvasPoller.handleCourse(dbCourse, function(err) { + assert.ok(!err); + + AssetsTestsUtil.assertGetAssets(client, course, null, null, null, 1, function(assets) { + var postResubmitAsset = assets.results[0]; + assert.strictEqual(postResubmitAsset.url, 'http://www.yahoo.com'); + assert.notStrictEqual(preResubmitAsset.id, postResubmitAsset.id); + + // Verify that subsequent runs don't create new assets + CanvasTestsUtil.mockPollingRequests(dbCourse, [], assignments, []); + CanvasPoller.handleCourse(dbCourse, function(err) { + assert.ok(!err); + AssetsTestsUtil.assertGetAssets(client, course, null, null, null, 1, function(assets) { + var secondRunAsset = assets.results[0]; + assert.strictEqual(secondRunAsset.url, 'http://www.yahoo.com'); + assert.notStrictEqual(preResubmitAsset.id, secondRunAsset.id); + assert.strictEqual(postResubmitAsset.id, secondRunAsset.id); + + return callback(); + }); + }); + }); + }); + }); + }); + }); + }); + }); + }); + }); + /** * Test that verifies that the poller creates categories for each assignment */