diff --git a/app/helpers/assignments_helper.rb b/app/helpers/assignments_helper.rb index 1a217e9080d..3a4a99da8bc 100644 --- a/app/helpers/assignments_helper.rb +++ b/app/helpers/assignments_helper.rb @@ -65,7 +65,13 @@ module AssignmentsHelper if assignment.expects_submission? && can_do(assignment, user, :submit) submit_text = user_submission.try(:has_submission?) ? I18n.t("Re-submit Assignment") : I18n.t("Submit Assignment") late = user_submission.try(:late?) ? "late" : "" - link_to(submit_text, '#', :role => "button", :class => "Button Button--primary submit_assignment_link #{late}") + link_to( + submit_text, + '#', + :role => "button", + :class => "Button Button--primary submit_assignment_link #{late}", + :disabled => user_submission && user_submission.attempts_left == 0, + ) end end diff --git a/app/models/submission.rb b/app/models/submission.rb index 4525c679ebf..caa032cb050 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -58,6 +58,8 @@ class Submission < ActiveRecord::Base }.freeze }.freeze + SUBMISSION_TYPES_GOVERNED_BY_ALLOWED_ATTEMPTS = %w[online_upload online_url online_text_entry].freeze + attr_readonly :assignment_id attr_accessor :visible_to_user, :skip_grade_calc, @@ -119,6 +121,7 @@ class Submission < ActiveRecord::Base validates :late_policy_status, inclusion: ['none', 'missing', 'late'], allow_nil: true validate :ensure_grader_can_grade validate :extra_attempts_can_only_be_set_on_online_uploads + validate :ensure_attempts_are_in_range scope :active, -> { where("submissions.workflow_state <> 'deleted'") } scope :for_enrollments, -> (enrollments) { where(user_id: enrollments.select(:user_id)) } @@ -1518,14 +1521,26 @@ class Submission < ActiveRecord::Base def extra_attempts_can_only_be_set_on_online_uploads return true unless changes.key?("extra_attempts") && assignment - allowed_submission_types = %w[online_upload online_url online_text_entry] - return true if (assignment.submission_types.split(",") & allowed_submission_types).any? + return true if (assignment.submission_types.split(",") & SUBMISSION_TYPES_GOVERNED_BY_ALLOWED_ATTEMPTS).any? - error_msg = 'extra_attempts can only be set on submissions for an assignment with a type of online_upload, online_url, or online_text_entry' + error_msg = 'can only be set on submissions for an assignment with a type of online_upload, online_url, or online_text_entry' errors.add(:extra_attempts, error_msg) false end + def attempts_left + return nil if self.assignment.allowed_attempts.nil? || self.assignment.allowed_attempts < 0 + [0, self.assignment.allowed_attempts + (self.extra_attempts || 0) - (self.attempt || 0)].max + end + + def ensure_attempts_are_in_range + return true unless changes.key?("submitted_at") && assignment + return true unless (assignment.submission_types.split(",") & SUBMISSION_TYPES_GOVERNED_BY_ALLOWED_ATTEMPTS).any? + return true if attempts_left.nil? || attempts_left > 0 + errors.add(:attempt, 'you have reached the maximum number of allowed attempts for this assignment') + false + end + def can_autograde? result = GRADE_STATUS_MESSAGES_MAP[can_autograde_symbolic_status] result ||= { status: false, message: I18n.t('Cannot autograde at this time') } diff --git a/app/stylesheets/bundles/submission.scss b/app/stylesheets/bundles/submission.scss index 5367adbc0bc..aa75458ac9c 100644 --- a/app/stylesheets/bundles/submission.scss +++ b/app/stylesheets/bundles/submission.scss @@ -62,6 +62,15 @@ } .submission-details-header__info { margin-top: $ic-sp / 3; + display: flex; + + .submission-details-header__attempts_info { + margin: 0px 0px 0px 40px; + + > span { + margin: 0px 20px 0px 0px; + } + } } .submission-details-header__time { margin-#{direction(right)}: $ic-sp / 4; diff --git a/app/stylesheets/pages/assignments/_assignments.scss b/app/stylesheets/pages/assignments/_assignments.scss index f92e1ae0a59..3b1daca3421 100644 --- a/app/stylesheets/pages/assignments/_assignments.scss +++ b/app/stylesheets/pages/assignments/_assignments.scss @@ -139,7 +139,7 @@ padding-#{direction(right)}: 0.5em; } .value { - padding-#{direction(right)}: 2.5em; + padding-#{direction(right)}: 2.0em; } } .form-horizontal.display-only .control-group { diff --git a/app/views/assignments/show.html.erb b/app/views/assignments/show.html.erb index a802a6f3a97..d18a9c770d5 100644 --- a/app/views/assignments/show.html.erb +++ b/app/views/assignments/show.html.erb @@ -151,6 +151,16 @@ <% end %> <% end %> + <% if @assignment.allowed_attempts&.> 0 %> +
  • + <%= t :attempts, 'Attempts' %> + <%= @current_user_submission&.attempt || 0 %> +
    • +
  • + <%= t :allowed_attempts, 'Allowed Attempts' %> + <%= @assignment.allowed_attempts + (@current_user_submission&.extra_attempts || 0) %> +
    • + <% end %> <%= render :partial => "shared/available_dates", :locals => {:association => @assignment} %>
    @@ -192,6 +202,14 @@ <% end -%> + <% if @assignment.allowed_attempts&.> 0 %> +
    +
    <%= t :allowed_attempts, 'Allowed Attempts' %>
    +
    + <%= @assignment.allowed_attempts %> +
    +
    + <% end -%> <% if turnitin_active? %>
    <%= t :turnitin, 'Turnitin' %>
    diff --git a/app/views/submissions/show.html.erb b/app/views/submissions/show.html.erb index dfada585f6c..af7796505e8 100644 --- a/app/views/submissions/show.html.erb +++ b/app/views/submissions/show.html.erb @@ -165,6 +165,16 @@ <% end %> <% end %> + <% if @assignment.allowed_attempts&.> 0 %> +
    + <%= t :attempts, 'Attempts' %> + <%= @submission.attempt || 0 %> +
    +
    + <%= t :allowed_attempts, 'Allowed Attempts' %> + <%= @assignment.allowed_attempts + (@submission.extra_attempts || 0) %> +
    + <% end %>
    @@ -175,7 +185,7 @@ <% end %> <% if @assignment.expects_submission? && can_do(@assignment, @current_user, :submit) && @current_user == @submission.user %> - + > <%= @submission.has_submission? ? t('links.resubmit', "Re-submit Assignment") : t('links.submit', "Submit Assignment") %> <% end %> diff --git a/spec/helpers/assignments_helper_spec.rb b/spec/helpers/assignments_helper_spec.rb index 55e3d8deacc..343358289f3 100644 --- a/spec/helpers/assignments_helper_spec.rb +++ b/spec/helpers/assignments_helper_spec.rb @@ -119,4 +119,43 @@ describe AssignmentsHelper do expect(turnitin_active?).to be_falsey end end + + describe "#assignment_submission_button" do + before do + student_in_course + assignment_model(course: @course) + @assignment.update_attribute(:submission_types, "online_upload") + allow(self).to receive(:can_do).and_return true + end + + context "the submission has 0 attempts left" do + it "returns a disabled button" do + @assignment.update_attribute(:allowed_attempts, 2) + submission = @assignment.submissions.find_by!(user_id: @student) + submission.update_attribute(:attempt, 2) + button = assignment_submission_button(@assignment, @student, submission) + expect(button["disabled"]).to eq("disabled") + end + end + + context "the submission has > 0 attempts left" do + it "returns an enabled button" do + @assignment.update_attribute(:allowed_attempts, 2) + submission = @assignment.submissions.find_by!(user_id: @student) + submission.update_attribute(:attempt, 1) + button = assignment_submission_button(@assignment, @student, submission) + expect(button["disabled"]).to be_nil + end + end + + context "the submission has unlimited attempts" do + it "returns an enabled button" do + @assignment.update_attribute(:allowed_attempts, -1) + submission = @assignment.submissions.find_by!(user_id: @student) + submission.update_attribute(:attempt, 3) + button = assignment_submission_button(@assignment, @student, submission) + expect(button["disabled"]).to be_nil + end + end + end end diff --git a/spec/models/submission_spec.rb b/spec/models/submission_spec.rb index b6fb56c5a6e..02584b02b8b 100644 --- a/spec/models/submission_spec.rb +++ b/spec/models/submission_spec.rb @@ -5853,4 +5853,110 @@ describe Submission do end end end + + describe '#ensure_attempts_are_in_range' do + let(:submission) { @assignment.submissions.first } + + context 'the assignment is of a type that is restricted by attempts' do + before do + @assignment.allowed_attempts = 10 + @assignment.submission_types = 'online_upload' + @assignment.save! + end + + context 'attempts_left <= 0' do + before do + submission.attempt = 10 + submission.save! + end + + context 'the submitted_at changed' do + it 'is invalid' do + submission.submitted_at = Time.zone.now + expect(submission).to_not be_valid + end + end + + context 'the submitted_at did not change' do + it 'is valid' do + expect(submission).to be_valid + end + end + end + end + + context 'the assignment is of a type that is not restricted by attempts' do + before do + @assignment.allowed_attempts = 10 + @assignment.submission_types = 'online_discussion' + @assignment.save! + submission.attempt = 10 + submission.save! + end + + it 'is valid' do + expect(submission).to be_valid + end + end + end + + describe '#attempts_left' do + let(:submission) { @assignment.submissions.first } + + context 'allowed_attempts is set to a number > 0 on the assignment' do + before do + @assignment.allowed_attempts = 10 + @assignment.submission_types = 'online_upload' + @assignment.save! + end + + context 'the submission has extra_attempts set to a value > 0' do + it 'returns assignment.allowed_attempts + submission.extra_attempts - submission.attempt' do + submission.extra_attempts = 12 + submission.attempt = 6 + submission.save! + expect(submission.attempts_left).to eq(10 + 12 - 6) + end + + it 'correctly recalculates when allowed_attempts and extra_attempts change' do + submission.extra_attempts = 12 + submission.attempt = 22 + submission.save! + expect(submission.attempts_left).to eq(0) + @assignment.allowed_attempts = 11 + @assignment.save! + expect(submission.attempts_left).to eq(1) + submission.extra_attempts = 13 + submission.save! + expect(submission.attempts_left).to eq(2) + end + + it 'will never return negative values' do + submission.attempt = 1000 + submission.save! + expect(submission.attempts_left).to eq(0) + end + end + + context 'the submission has extra_attempts set to nil' do + it 'returns allowed_attempts from the assignment' do + submission.extra_attempts = nil + submission.attempt = 6 + submission.save! + expect(submission.attempts_left).to eq(10 - 6) + end + end + end + + context 'allowed_attempts is set to nil or -1 on the assignment' do + it 'returns nil' do + @assignment.allowed_attempts = nil + @assignment.save! + expect(submission.attempts_left).to be_nil + @assignment.allowed_attempts = -1 + @assignment.save! + expect(submission.attempts_left).to be_nil + end + end + end end diff --git a/spec/selenium/assignments/assignment_allowed_attempts_spec.rb b/spec/selenium/assignments/assignment_allowed_attempts_spec.rb new file mode 100644 index 00000000000..ae24166757f --- /dev/null +++ b/spec/selenium/assignments/assignment_allowed_attempts_spec.rb @@ -0,0 +1,106 @@ +# +# Copyright (C) 2018 - 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 . + +require_relative '../common' +require_relative '../helpers/assignments_common' + +describe "allowed_attempts feature for assignments" do + include_context "in-process server selenium tests" + include AssignmentsCommon + + context "as a student" do + before do + course_with_student_logged_in + end + + describe "the assignments page" do + context "with allowed_attempts on the assignment" do + before do + @assignment = @course.assignments.create!({ name: "Test Assignment", allowed_attempts: 2 }) + @assignment.update_attribute(:submission_types, "online_text_entry") + end + + it "prevents submitting if the student has exceeded the max number of attempts" do + submission = @assignment.submit_homework(@student, { body: "blah" }) + submission.update_attributes(attempt: 2, submission_type: "online_text_entry") + get "/courses/#{@course.id}/assignments/#{@assignment.id}" + expect(f(".student-assignment-overview")).to include_text("Allowed Attempts") + expect(f('.submit_assignment_link')).to be_disabled + end + + it "allows submitting if the student has not exceeded the max number of attempts" do + get "/courses/#{@course.id}/assignments/#{@assignment.id}" + expect(f(".student-assignment-overview")).to include_text("Allowed Attempts") + expect(f('.submit_assignment_link')).to_not be_disabled + end + end + + context "without allowed_attempts on the assignment" do + before do + @assignment = @course.assignments.create!({ name: "Test Assignment", allowed_attempts: -1 }) + @assignment.update_attribute(:submission_types, "online_text_entry") + @assignment.submit_homework(@student, { body: "blah" }) + end + + it "does not show the attempt data" do + get "/courses/#{@course.id}/assignments/#{@assignment.id}" + expect(f(".student-assignment-overview")).to_not include_text("Allowed Attempts") + expect(f('.submit_assignment_link')).to_not be_disabled + end + end + end + + describe "the assignments detail page" do + context "with allowed_attempts on the assignment" do + before do + @assignment = @course.assignments.create!({ name: "Test Assignment", allowed_attempts: 2 }) + @assignment.update_attribute(:submission_types, "online_text_entry") + @submission = @assignment.submit_homework(@student, { body: "blah" }) + @submission.update_attributes(submission_type: "online_text_entry") + end + + it "prevents submitting if the student has exceeded the max number of attempts" do + @submission.update_attributes(attempt: 2) + get "/courses/#{@course.id}/assignments/#{@assignment.id}/submissions/#{@student.id}" + expect(f(".submission-details-header__info")).to include_text("Allowed Attempts") + expect(fln("Re-submit Assignment")).to be_disabled + end + + it "allows submitting if the student has not exceeded the max number of attempts" do + get "/courses/#{@course.id}/assignments/#{@assignment.id}/submissions/#{@student.id}" + expect(f(".submission-details-header__info")).to include_text("Allowed Attempts") + expect(fln("Re-submit Assignment")).to_not be_disabled + end + end + + context "without allowed_attempts on the assignment" do + before do + @assignment = @course.assignments.create!({ name: "Test Assignment", allowed_attempts: -1 }) + @assignment.update_attribute(:submission_types, "online_text_entry") + @submission = @assignment.submit_homework(@student, { body: "blah" }) + @submission.update_attributes(attempt: 2, submission_type: "online_text_entry") + end + + it "does not show the attempt data and allows submitting" do + get "/courses/#{@course.id}/assignments/#{@assignment.id}/submissions/#{@student.id}" + expect(f(".submission-details-header__info")).to_not include_text("Allowed Attempts") + expect(fln("Re-submit Assignment")).to_not be_disabled + end + end + end + end +end