Merge pull request #6339 from mishaschwartz/v2.1.6

V2.1.6

Changelog.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## [v2.1.6]
+- Fix bug where TAs were able to access urls for results they haven't been assigned to (#6321)
+- Fix bug where the marking state was left as "complete" after a new criterion is created (#6303)
+- Fix bug where writing starter files to repositories failed if the starter files contained a directory (#6294)
+- Fix bug where csv summary download fails if criteria are created after marking (#6302)
+
 ## [v2.1.5]
 - Add admin users to the .access file so that they can be authenticated as having access to the git repos (#6237)
 - Optionally log which user makes each request by tagging the log files with user_names (#6241)

app/MARKUS_VERSION

@@ -1 +1 @@
-VERSION=v2.1.5,PATCH_LEVEL=DEV
+VERSION=v2.1.6,PATCH_LEVEL=DEV

app/jobs/update_results_marking_states_job.rb

@@ -0,0 +1,7 @@
+class UpdateResultsMarkingStatesJob < ApplicationJob
+  def perform(assignment_id, marking_state)
+    Result.includes(submission: :grouping)
+          .where(groupings: { assessment_id: assignment_id })
+          .update!(marking_state: Result::MARKING_STATES[marking_state])
+  end
+end

app/models/assignment.rb

@@ -707,7 +707,7 @@ def summary_csv(role)
             row += Array.new(2 + self.ta_criteria.count, nil)
           else
             row << result.total_mark
-            row += self.ta_criteria.map { |crit| marks[crit.id][:mark] }
+            row += self.ta_criteria.map { |crit| marks[crit.id]&.[](:mark) }
             row << extra_marks_hash[result&.id]
           end
           csv << row

app/models/criterion.rb

@@ -3,6 +3,7 @@
 class Criterion < ApplicationRecord
   belongs_to :assignment, foreign_key: :assessment_id, inverse_of: :criteria
   before_validation :update_assigned_groups_count
+  after_create :update_result_marking_states
   after_update :update_results_with_change
   after_destroy :update_results
 
@@ -167,7 +168,7 @@ def scale_marks
   end
 
   def results_unreleased?
-    return true if self.marks.joins(:result).where('results.released_to_students' => true).empty?
+    return true if self.assignment&.released_marks.blank?
 
     errors.add(:base, 'Cannot update criterion once results are released.')
     false
@@ -232,4 +233,8 @@ def update_results
     result_ids = Result.includes(submission: :grouping).where(groupings: { assessment_id: assessment_id }).ids
     Result.update_total_marks(result_ids)
   end
+
+  def update_result_marking_states
+    UpdateResultsMarkingStatesJob.perform_later(assessment_id, :incomplete)
+  end
 end

app/models/starter_file_entry.rb

@@ -49,8 +49,8 @@ def add_files_to_repo(repo, txn)
       repo_entry_path = File.join(assignment_path, repo_entry_path)
 
       entry_exists = rev.path_exists? repo_entry_path
-      if abs_path.directory? && !entry_exists
-        txn.add_path(repo_entry_path)
+      if abs_path.directory?
+        txn.add_path(repo_entry_path) unless entry_exists
       else
         content = File.read(abs_path.to_s, mode: 'rb')
         if entry_exists

app/policies/grouping_policy.rb

@@ -6,6 +6,10 @@ def member?
     record.accepted_students.include?(role)
   end
 
+  def assigned_grader?
+    role.ta? && record.tas.exists?(role.id)
+  end
+
   def not_in_progress?
     !record.student_test_run_in_progress?
   end

app/policies/result_policy.rb

@@ -11,7 +11,9 @@ class ResultPolicy < ApplicationPolicy
   authorize :from_codeviewer, :select_file, optional: true
 
   def view?
-    true
+    check?(:manage_submissions?, role) ||
+      check?(:assigned_grader?, record.grouping) ||
+      check?(:member?, record.submission.grouping)
   end
 
   def run_tests?
@@ -25,26 +27,26 @@ def run_tests?
   end
 
   def grade?
-    role.instructor? || role.ta?
+    check?(:manage_submissions?, role) || check?(:assigned_grader?, record.grouping)
   end
 
   def review?
-    role.instructor? || role.ta? || (
+    check?(:manage_submissions?, role) || check?(:assigned_grader?, record.grouping) || (
       record&.submission&.assignment&.has_peer_review &&
           role.is_reviewer_for?(record&.submission&.assignment&.pr_assignment, record)
     )
   end
 
   def set_released_to_students?
-    check?(:review?) && check?(:manage_submissions?, role)
+    check?(:manage_submissions?, role)
   end
 
   def manage?
     role.instructor?
   end
 
   def download?
-    role.instructor? || role.ta? || (
+    check?(:manage_submissions?, role) || check?(:assigned_grader?, record.grouping) || (
       from_codeviewer && role.is_reviewer_for?(record.submission.grouping.assignment.pr_assignment, record)
     ) || (
       record.submission.grouping.accepted_students.ids.include?(role.id) && (

spec/controllers/groups_controller_spec.rb

@@ -944,13 +944,36 @@
       let(:grouping) { create :grouping_with_inviter, assignment: assignment, inviter: role }
 
       shared_examples 'populate starter files properly' do
-        it 'populates the grouping repository with the correct content' do
-          subject
-          grouping.access_repo do |repo|
-            rev = repo.get_latest_revision
-            expect(rev.path_exists?(File.join(assignment.short_identifier, 'q1', 'q1.txt'))).to be true
-            expect(rev.path_exists?(File.join(assignment.short_identifier, 'q2.txt'))).to be true
+        shared_examples 'write starter files to repo' do
+          it 'populates the grouping repository with the correct content' do
+            subject
+            grouping.access_repo do |repo|
+              rev = repo.get_latest_revision
+              expect(rev.path_exists?(File.join(assignment.short_identifier, 'q1', 'q1.txt'))).to be true
+              expect(rev.path_exists?(File.join(assignment.short_identifier, 'q2.txt'))).to be true
+            end
+          end
+          it 'writes the correct content' do
+            subject
+            grouping.access_repo do |repo|
+              rev_file = repo.get_latest_revision.files_at_path(assignment.short_identifier)['q2.txt']
+              expect(repo.download_as_string(rev_file)).to eq 'q2 content'
+            end
+          end
+        end
+        context 'when the repo is empty' do
+          include_examples 'write starter files to repo'
+        end
+        context 'when some files already exist in the repo' do
+          before do
+            grouping.access_repo do |repo|
+              txn = repo.get_transaction(role.user_name)
+              txn.add_path(File.join(assignment.short_identifier, 'q1'))
+              txn.add(File.join(assignment.short_identifier, 'q2.txt'), 'other_content', 'application/octet-stream')
+              repo.commit(txn)
+            end
           end
+          include_examples 'write starter files to repo'
         end
       end
       context 'when the grouping was created after any starter file groups' do