From adbdbdcd9c5fac88912407a5c2ebcdef9d7b60d3 Mon Sep 17 00:00:00 2001 From: wdransfield Date: Mon, 3 Jun 2019 15:40:07 -0600 Subject: [PATCH] Support versioned text entry originality reports Closes PLAT-4296, PLAT-4559, PLAT-4558 Test Plan: - Create a text entry submission - Create an originality report for the submission - Resubmit the assignment - Create an originality report with a different score for the resubmission - Verify the gradebook submission details dialog shows the correct report for each submission version - Create a text entry submission with an originality report - Verify the report can be viewed in speedgrader - Resubmit the assignment and create a new originality report - Verify speed grader shows the correct originality score for each submission - Verify speed grader correctly displays originality reports tied to an attachment - Verify speed grader correctly dispalys originality reports from tii plugin Change-Id: I433be93083a501cb03cbaa04bb2329c50bbfffca Reviewed-on: https://gerrit.instructure.com/196697 Tested-by: Jenkins Product-Review: Jesse Poulos QA-Review: Kai Bjorkman Reviewed-by: Marc Phillips --- app/coffeescripts/SubmissionDetailsDialog.js | 12 ++++- app/coffeescripts/gradebook/Turnitin.coffee | 5 +- app/coffeescripts/gradezilla/Turnitin.coffee | 6 ++- .../originalityReportSubmissionKey.test.js | 45 ++++++++++++++++ .../helpers/originalityReportSubmissionKey.js | 27 ++++++++++ app/models/originality_report.rb | 15 ++++-- .../grade_summary_assignment_presenter.rb | 1 + .../submissions/_originality_score.html.erb | 2 +- public/javascripts/speed_grader.js | 9 +++- spec/coffeescripts/TurnitinSpec.js | 15 ++++++ spec/javascripts/jsx/speed_graderSpec.js | 44 ++++++++++++++++ spec/models/originality_report_spec.rb | 18 ++++++- spec/models/speed_grader/assignment_spec.rb | 5 +- spec/models/submission_spec.rb | 2 +- .../gradebook_originality_report_spec.rb | 51 ------------------- 15 files changed, 192 insertions(+), 65 deletions(-) create mode 100644 app/jsx/gradebook/shared/helpers/__tests__/originalityReportSubmissionKey.test.js create mode 100644 app/jsx/gradebook/shared/helpers/originalityReportSubmissionKey.js diff --git a/app/coffeescripts/SubmissionDetailsDialog.js b/app/coffeescripts/SubmissionDetailsDialog.js index 5dab3835067..985031f2c9e 100644 --- a/app/coffeescripts/SubmissionDetailsDialog.js +++ b/app/coffeescripts/SubmissionDetailsDialog.js @@ -20,6 +20,7 @@ import $ from 'jquery' import submissionDetailsDialog from 'jst/SubmissionDetailsDialog' import I18n from 'i18n!submission_details_dialog' import GradeFormatHelper from 'jsx/gradebook/shared/helpers/GradeFormatHelper' +import originalityReportSubmissionKey from 'jsx/gradebook/shared/helpers/originalityReportSubmissionKey' import {extractDataForTurnitin} from './gradebook/Turnitin' import OutlierScoreHelper from 'jsx/grading/helpers/OutlierScoreHelper' import 'jst/_submission_detail' // a partial needed by the SubmissionDetailsDialog template @@ -150,9 +151,18 @@ export default class SubmissionDetailsDialog { }) submission.turnitin = extractDataForTurnitin( submission, - `submission_${submission.id}`, + originalityReportSubmissionKey(submission), this.options.context_url ) + + if (Object.keys(submission.turnitin).length === 0) { + submission.turnitin = extractDataForTurnitin( + submission, + `submission_${submission.id}`, + this.options.context_url + ) + } + submission.attachments && submission.attachments.forEach(attachment => { attachment.turnitin = extractDataForTurnitin( diff --git a/app/coffeescripts/gradebook/Turnitin.coffee b/app/coffeescripts/gradebook/Turnitin.coffee index 02698262e84..bd7a1b35904 100644 --- a/app/coffeescripts/gradebook/Turnitin.coffee +++ b/app/coffeescripts/gradebook/Turnitin.coffee @@ -17,6 +17,7 @@ import I18n from 'i18n!turnitin' import {max, invert} from 'underscore' +import originalityReportSubmissionKey from 'jsx/gradebook/shared/helpers/originalityReportSubmissionKey' export extractDataTurnitin = (submission) -> plagData = submission?.turnitin_data @@ -31,7 +32,7 @@ export extractDataTurnitin = (submission) -> if turnitin = plagData?['attachment_' + attachment.id] data.items.push turnitin else if submission.submission_type is "online_text_entry" - if turnitin = plagData?['submission_' + submission.id] + if turnitin = (plagData?[originalityReportSubmissionKey(submission)] || plagData?['submission_' + submission.id]) data.items.push turnitin return unless data.items.length @@ -53,7 +54,7 @@ export extractDataForTurnitin = (submission, key, urlPrefix) -> return {} unless data and data[key] and (data[key].similarity_score? or data[key].status == 'pending') data = data[key] data.state = "#{data.state || 'no'}_score" - if data.similarity_score || data.similarity_score == 0 + if data.similarity_score? data.score = "#{data.similarity_score}%" data.reportUrl = "#{urlPrefix}/assignments/#{submission.assignment_id}/submissions/#{submission.user_id}/#{type}/#{key}" data.tooltip = I18n.t('tooltip.score', 'Similarity Score - See detailed report') diff --git a/app/coffeescripts/gradezilla/Turnitin.coffee b/app/coffeescripts/gradezilla/Turnitin.coffee index 57820ae0945..65b1380711b 100644 --- a/app/coffeescripts/gradezilla/Turnitin.coffee +++ b/app/coffeescripts/gradezilla/Turnitin.coffee @@ -17,6 +17,7 @@ import I18n from 'i18n!turnitin' import {max, invert} from 'underscore' +import originalityReportSubmissionKey from 'jsx/gradebook/shared/helpers/originalityReportSubmissionKey' export extractDataTurnitin = (submission) -> plagData = submission?.turnitin_data @@ -31,7 +32,7 @@ export extractDataTurnitin = (submission) -> if turnitin = plagData?['attachment_' + attachment.id] data.items.push turnitin else if submission.submission_type is "online_text_entry" - if turnitin = plagData?['submission_' + submission.id] + if turnitin = (plagData?[originalityReportSubmissionKey(submission)] || plagData?['submission_' + submission.id]) data.items.push turnitin return unless data.items.length @@ -53,7 +54,8 @@ export extractDataForTurnitin = (submission, key, urlPrefix) -> return {} unless data and data[key] and (data[key].similarity_score? or data[key].status == 'pending') data = data[key] data.state = "#{data.state || 'no'}_score" - data.score = if data.similarity_score then "#{data.similarity_score}%" else "" + if data.similarity_score || data.similarity_score == 0 + data.score = "#{data.similarity_score}%" data.reportUrl = "#{urlPrefix}/assignments/#{submission.assignment_id}/submissions/#{submission.user_id}/#{type}/#{key}" data.tooltip = I18n.t('tooltip.score', 'Similarity Score - See detailed report') data diff --git a/app/jsx/gradebook/shared/helpers/__tests__/originalityReportSubmissionKey.test.js b/app/jsx/gradebook/shared/helpers/__tests__/originalityReportSubmissionKey.test.js new file mode 100644 index 00000000000..50c94ac2b07 --- /dev/null +++ b/app/jsx/gradebook/shared/helpers/__tests__/originalityReportSubmissionKey.test.js @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2019 - present 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 . + */ + +import originalityReportSubmissionKey from '../originalityReportSubmissionKey' + +function submission(overrides = {}) { + return { + id: 1, + submitted_at: '05 October 2011 14:48 UTC', + ...overrides + } +} + +describe('originalityReportSubmissionKey', () => { + it('returns the key for the submission', () => { + expect(originalityReportSubmissionKey(submission())).toEqual( + 'submission_1_2011-10-05T14:48:00Z' + ) + }) + + describe('when the submission does not have a valid "submitted_at"', () => { + const overrides = { + submitted_at: 'banana' + } + + it('returns the an empty string', () => { + expect(originalityReportSubmissionKey(submission(overrides))).toEqual('') + }) + }) +}) diff --git a/app/jsx/gradebook/shared/helpers/originalityReportSubmissionKey.js b/app/jsx/gradebook/shared/helpers/originalityReportSubmissionKey.js new file mode 100644 index 00000000000..d17c077f172 --- /dev/null +++ b/app/jsx/gradebook/shared/helpers/originalityReportSubmissionKey.js @@ -0,0 +1,27 @@ +/* + * Copyright (C) 2019 - present 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 . + */ + +export default function originalityReportSubmissionKey(submission) { + try { + let submittedAt = new Date(submission.submitted_at) + submittedAt = `${submittedAt.toISOString().split('.')[0]}Z` + return (submittedAt && `submission_${submission.id}_${submittedAt}`) || '' + } catch (_error) { + return '' + } +} diff --git a/app/models/originality_report.rb b/app/models/originality_report.rb index 968858766ef..013a4bfc24d 100644 --- a/app/models/originality_report.rb +++ b/app/models/originality_report.rb @@ -31,8 +31,12 @@ class OriginalityReport < ActiveRecord::Base alias_attribute :file_id, :attachment_id alias_attribute :originality_report_file_id, :originality_report_attachment_id - before_validation :set_submission_time before_validation :infer_workflow_state + after_validation :set_submission_time + + def self.submission_asset_key(submission) + "#{submission.asset_string}_#{submission.submitted_at.utc.iso8601}" + end def state if workflow_state != 'scored' @@ -67,7 +71,11 @@ class OriginalityReport < ActiveRecord::Base def asset_key return Attachment.asset_string(attachment_id) if attachment_id.present? - Submission.asset_string(submission_id) + if submission_time.present? + "#{Submission.asset_string(submission_id)}_#{submission_time&.utc&.iso8601}" + else + Submission.asset_string(submission_id) + end end def copy_to_group_submissions! @@ -75,6 +83,7 @@ class OriginalityReport < ActiveRecord::Base group_submissions = assignment.submissions.where.not(id: submission.id).where(group: submission.group) group_submissions.find_each do |s| copy_of_report = self.dup + copy_of_report.submission_time = nil # We don't want a single submission to have # multiple originality reports with the same @@ -98,7 +107,7 @@ class OriginalityReport < ActiveRecord::Base end def set_submission_time - self.submission_time = submission&.submitted_at + self.submission_time ||= submission.reload.submitted_at end def infer_workflow_state diff --git a/app/presenters/grade_summary_assignment_presenter.rb b/app/presenters/grade_summary_assignment_presenter.rb index feb6362f615..81b3d98a22b 100644 --- a/app/presenters/grade_summary_assignment_presenter.rb +++ b/app/presenters/grade_summary_assignment_presenter.rb @@ -202,6 +202,7 @@ class GradeSummaryAssignmentPresenter plag_data = submission.originality_data end t = if is_text_entry? + plag_data[OriginalityReport.submission_asset_key(submission)] || plag_data[submission.asset_string] elsif is_online_upload? && file plag_data[file.asset_string] diff --git a/app/views/submissions/_originality_score.html.erb b/app/views/submissions/_originality_score.html.erb index 4865f5cb940..88b6d80fcb4 100644 --- a/app/views/submissions/_originality_score.html.erb +++ b/app/views/submissions/_originality_score.html.erb @@ -27,7 +27,7 @@ <% elsif can_do(@submission, @current_user, :view_turnitin_report) && - (turnitin_score = @submission.originality_data[attachment&.asset_string] || @submission.originality_data[@submission.asset_string]) && + (turnitin_score = @submission.originality_data[attachment&.asset_string] || @submission.originality_data[OriginalityReport.submission_asset_key(@submission)]) && (@submission.originality_data[:provider] == nil || @submission.originality_reports.present?) && (turnitin_score[:similarity_score] || turnitin_score[:state] == 'pending') %> diff --git a/public/javascripts/speed_grader.js b/public/javascripts/speed_grader.js index 03cd5cda9bc..dc8196f9a59 100644 --- a/public/javascripts/speed_grader.js +++ b/public/javascripts/speed_grader.js @@ -31,6 +31,7 @@ import numberHelper from 'jsx/shared/helpers/numberHelper' import GradeFormatHelper from 'jsx/gradebook/shared/helpers/GradeFormatHelper' import AssessmentAuditButton from 'jsx/speed_grader/AssessmentAuditTray/components/AssessmentAuditButton' import AssessmentAuditTray from 'jsx/speed_grader/AssessmentAuditTray' +import originalityReportSubmissionKey from 'jsx/gradebook/shared/helpers/originalityReportSubmissionKey' import PostPolicies from 'jsx/speed_grader/PostPolicies' import SpeedGraderProvisionalGradeSelector from 'jsx/speed_grader/SpeedGraderProvisionalGradeSelector' import SpeedGraderPostGradesMenu from 'jsx/speed_grader/SpeedGraderPostGradesMenu' @@ -2012,8 +2013,14 @@ EG = { var $turnitinScoreContainer = $grade_container.find('.turnitin_score_container').empty(), $turnitinInfoContainer = $grade_container.find('.turnitin_info_container').empty(), assetString = `submission_${submission.id}`, + turnitinAsset = null + + if (turnitinEnabled && submission.turnitin_data) { turnitinAsset = - turnitinEnabled && submission.turnitin_data && submission.turnitin_data[assetString] + submission.turnitin_data[originalityReportSubmissionKey(submission)] || + submission.turnitin_data[assetString] + } + // There might be a previous submission that was text_entry, but the // current submission is an upload. The turnitin asset for the text // entry would still exist diff --git a/spec/coffeescripts/TurnitinSpec.js b/spec/coffeescripts/TurnitinSpec.js index 82da12d9c83..d421307fa4a 100644 --- a/spec/coffeescripts/TurnitinSpec.js +++ b/spec/coffeescripts/TurnitinSpec.js @@ -95,6 +95,21 @@ test('uses the score when the score is 0', () => { ) }) +test('correctly finds text entry plagiarism data', () => { + submissionWithReport.turnitin_data = { + 'submission_7_2016-11-29T22:29:44Z': { + similarity_score: 0.8, + state: 'acceptable', + report_url: 'http://www.instructure.com', + status: 'pending' + } + } + submissionWithReport.submission_type = 'online_text_entry' + + const plagiarismData = Turnitin.extractDataTurnitin(submissionWithReport) + equal(plagiarismData.items.length, 1) +}) + test('uses originality_report type in url if submission has an OriginalityReport', () => { const tii_data = Turnitin.extractDataForTurnitin( submissionWithReport, diff --git a/spec/javascripts/jsx/speed_graderSpec.js b/spec/javascripts/jsx/speed_graderSpec.js index 0be7fe2ab82..deeee4c22f7 100644 --- a/spec/javascripts/jsx/speed_graderSpec.js +++ b/spec/javascripts/jsx/speed_graderSpec.js @@ -5882,6 +5882,50 @@ QUnit.module('SpeedGrader', function(suiteHooks) { /* eslint-disable-line qunit/ teardownFixtures() }) + QUnit.module('when text entry submission', textEntryHooks => { + const resubmissionTurnitinData = { + similarity_score: '80' + } + + textEntryHooks.beforeEach(() => { + const originalityData = Object.assign({}, turnitinData) + originalityData['submission_1_2019-06-05T19:51:35Z'] = originalityData.submission_1 + originalityData['submission_1_2019-07-05T19:51:35Z'] = resubmissionTurnitinData + delete originalityData.submission_1 + submission.submission_history[0].turnitin_data = originalityData + submission.submission_history[0].has_originality_score = true + submission.submission_history[0].submitted_at = '2019-06-05T19:51:35Z' + + window.jsonData = testJsonData + SpeedGrader.EG.jsonReady() + }) + + test('displays the report for the current submission', () => { + SpeedGrader.EG.currentStudent = { + ...student, + submission + } + SpeedGrader.EG.handleSubmissionSelectionChange() + strictEqual( + document.querySelector(gradeSimilaritySelector).innerHTML.trim(), + '60%' + ) + }) + + test('displays the report for a past submission', () => { + submission.submission_history[0].submitted_at = '2019-07-05T19:51:35Z' + SpeedGrader.EG.currentStudent = { + ...student, + submission + } + SpeedGrader.EG.handleSubmissionSelectionChange() + strictEqual( + document.querySelector(gradeSimilaritySelector).innerHTML.trim(), + '80%' + ) + }) + }) + QUnit.module('when anonymous grading is inactive', () => { test('links to a detailed report for Turnitin submissions', () => { submission.submission_history[0].turnitin_data = turnitinData diff --git a/spec/models/originality_report_spec.rb b/spec/models/originality_report_spec.rb index 8f04e9a743c..49c47bab5fe 100644 --- a/spec/models/originality_report_spec.rb +++ b/spec/models/originality_report_spec.rb @@ -200,6 +200,20 @@ describe OriginalityReport do let(:attachment) { attachment_model } let(:submission) { submission_model } + context 'when "submission_time" is blank' do + let(:originality_report) do + o = OriginalityReport.create!( + submission: submission, + originality_score: 23 + ) + o.update_attribute(:submission_time, nil) + o + end + let(:subject) { originality_report.asset_key } + + it { is_expected.to eq submission.asset_string } + end + it 'returns the attachment asset string if attachment is present' do report = OriginalityReport.create!( submission: submission, @@ -214,7 +228,7 @@ describe OriginalityReport do submission: submission, originality_score: 23 ) - expect(report.asset_key).to eq submission.asset_string + expect(report.asset_key).to eq "#{submission.asset_string}_#{submission.submitted_at.utc.iso8601}" end end @@ -245,7 +259,7 @@ describe OriginalityReport do end describe '#state' do - let(:report) { OriginalityReport.new(workflow_state: 'pending') } + let(:report) { OriginalityReport.new(workflow_state: 'pending', submission: submission_model) } it "returns the workflow state unless it is 'scored'" do expect(report.state).to eq 'pending' diff --git a/spec/models/speed_grader/assignment_spec.rb b/spec/models/speed_grader/assignment_spec.rb index 87cedb04aa4..0e702a60329 100644 --- a/spec/models/speed_grader/assignment_spec.rb +++ b/spec/models/speed_grader/assignment_spec.rb @@ -1448,7 +1448,10 @@ describe SpeedGrader::Assignment do OriginalityReport.create!(originality_score: '1', submission: submission) json = SpeedGrader::Assignment.new(assignment, test_teacher).json keys = json['submissions'].first['submission_history'].first['submission']['turnitin_data'].keys - expect(keys).to include submission.asset_string, attachment.asset_string + expect(keys).to include( + OriginalityReport.submission_asset_key(submission), + attachment.asset_string + ) end it 'does not override "turnitin_data"' do diff --git a/spec/models/submission_spec.rb b/spec/models/submission_spec.rb index c87a124a0b3..d9f59a38ead 100644 --- a/spec/models/submission_spec.rb +++ b/spec/models/submission_spec.rb @@ -2175,7 +2175,7 @@ describe Submission do submission.update_attributes!(attachment_ids: attachment.id.to_s) originality_report.update_attributes!(attachment: nil) expect(submission.originality_data).to eq({ - submission.asset_string => { + OriginalityReport.submission_asset_key(submission) => { similarity_score: originality_report.originality_score, state: originality_report.state, report_url: originality_report.originality_report_url, diff --git a/spec/selenium/grades/gradebook/gradebook_originality_report_spec.rb b/spec/selenium/grades/gradebook/gradebook_originality_report_spec.rb index 0af32f42b63..da3ec602610 100644 --- a/spec/selenium/grades/gradebook/gradebook_originality_report_spec.rb +++ b/spec/selenium/grades/gradebook/gradebook_originality_report_spec.rb @@ -59,55 +59,4 @@ describe "gradebook - originality reports" do fj('button.ui-dialog-titlebar-close:visible').click end - - context 'group assignment' do - let(:course) { @first_assignment.course } - let!(:group) do - group = course.groups.create!(name: 'group one') - group.add_user(@student_1) - group.add_user(@student_2) - submission_one.update!(group: group) - submission_two.update!(group: group) - group - end - let(:submission_one) do - @first_assignment.submit_homework(@student_1, submission_type: 'online_text_entry', body: 'asdf') - end - let(:submission_two) do - @first_assignment.submit_homework(@student_2, submission_type: 'online_text_entry', body: 'asdf') - end - let(:originality_report) { submission_one.originality_reports.create!(originality_score: 1.0) } - - before { originality_report.copy_to_group_submissions! } - - it 'should show originality data for all submissions in a group' do - get "/courses/#{@course.id}/gradebook" - icons = ff('.gradebook-cell-turnitin') - expect(icons).to have_size 2 - end - - it 'shows the correct originality score for the first student' do - get "/courses/#{@course.id}/gradebook" - icons = ff('.gradebook-cell-turnitin') - - cell = icons.first.find_element(:xpath, '..') - driver.action.move_to(f('#gradebook_settings')).move_to(cell).perform - expect(cell.find_element(:css, "a")).to be_displayed - cell.find_element(:css, "a").click - score = f('.turnitin_similarity_score') - expect(score.text).to eq "#{originality_report.originality_score.to_i}%" - end - - it 'shows the correct originality score for the last student' do - get "/courses/#{@course.id}/gradebook" - - icons = ff('.gradebook-cell-turnitin') - cell = icons.second.find_element(:xpath, '..') - driver.action.move_to(f('#gradebook_settings')).move_to(cell).perform - expect(cell.find_element(:css, "a")).to be_displayed - cell.find_element(:css, "a").click - score = f('.turnitin_similarity_score') - expect(score.text).to eq "#{originality_report.originality_score.to_i}%" - end - end end