Merge pull request #5901 from pretendWhale/2.0.9

2.0.9

Changelog.md

@@ -6,7 +6,13 @@
 - Switch Javascript bundling to jsbundling-rails gem, and update to webpack v5 (#5833)
 - Remove group name displayed attribute from assignment properties table (#5834)
 - Replace uses of first name and last name attributes with display name (#5844)
+- Upgrade to Rails 7 (#5885)
 
+## [v2.0.9]
+- Fix bug when downloading all automated test files where the files were saved to a sub directory (#5864)
+- Fix bugs in assigning scanned exams and improve error message when assigning by name (#5895)
+- Remove MarkUs logo from mobile view left navigation menu (#5899)
+- Allow adding annotations to remark requests (#5900)
 
 ## [v2.0.8]
 - Fix bug where "run tests" grader permission was not working for submission table (#5860)

app/MARKUS_VERSION

@@ -1 +1 @@
-VERSION=2.0.8,PATCH_LEVEL=DEV
+VERSION=2.0.9,PATCH_LEVEL=DEV

app/assets/javascripts/Components/Result/left_pane.jsx

@@ -86,11 +86,7 @@ export class LeftPane extends React.Component {
             assignment_id={this.props.assignment_id}
             grouping_id={this.props.grouping_id}
             revision_identifier={this.props.revision_identifier}
-            show_annotation_manager={
-              !this.props.released_to_students &&
-              !this.props.remark_submitted &&
-              !this.props.is_reviewer
-            }
+            show_annotation_manager={!this.props.released_to_students && !this.props.is_reviewer}
             canDownload={this.props.is_reviewer === undefined ? undefined : !this.props.is_reviewer}
             fileData={this.props.submission_files}
             annotation_categories={this.props.annotation_categories}

app/assets/javascripts/Components/Result/marks_panel.jsx

@@ -232,7 +232,7 @@ class FlexibleCriterionInput extends React.Component {
   listDeductions = () => {
     let label = I18n.t("annotations.list_deductions");
     let deductiveAnnotations = this.props.annotations.filter(a => {
-      return !!a.deduction && a.criterion_id === this.props.id;
+      return !!a.deduction && a.criterion_id === this.props.id && !a.is_remark;
     });
 
     if (deductiveAnnotations.length === 0) {

app/assets/javascripts/Components/Result/result.jsx

@@ -132,12 +132,7 @@ class Result extends React.Component {
 
   /* Interaction with external components/libraries */
   updateContextMenu = () => {
-    if (
-      this.state.released_to_students ||
-      this.state.remark_submitted ||
-      this.props.role === "Student"
-    )
-      return;
+    if (this.state.released_to_students || this.props.role === "Student") return;
 
     window.annotation_context_menu.setup(
       Routes.course_annotations_path,

app/assets/stylesheets/common/_navigation.scss

@@ -67,6 +67,10 @@ nav {
   margin-right: $dimen-horizontal-nav;
   min-height: 38px;
   width: 90px;
+
+  @include breakpoint(mobile) {
+    display: none;
+  }
 }
 
 .color-dark {

app/controllers/groups_controller.rb

@@ -167,12 +167,12 @@ def get_names
                           .where('(lower(first_name) like ? OR
                                    lower(last_name) like ? OR
                                    lower(user_name) like ? OR
-                                   id_number like ?) AND users.id NOT IN (?)',
+                                   id_number like ?) AND roles.id NOT IN (?)',
                                  "#{params[:term].downcase}%",
                                  "#{params[:term].downcase}%",
                                  "#{params[:term].downcase}%",
                                  "#{params[:term]}%",
-                                 Membership.select(:user_id)
+                                 Membership.select(:role_id)
                                            .joins(:grouping)
                                            .where('groupings.assessment_id = ?', params[:assignment_id]))
                           .pluck_to_hash(:id, 'users.id_number', 'users.user_name',
@@ -196,14 +196,19 @@ def assign_student_and_next
         student = current_course.students.find(params[:s_id])
       end
       # if the user has typed in the whole name without select, or if they typed a name different from the select s_id
-      if student.nil? || (student.first_name + ' ' + student.last_name) != params[:names]
-        student = current_course.students.where(
+      if student.nil? || ("#{student.first_name} #{student.last_name}") != params[:names]
+        student = current_course.students.joins(:end_user).where(
           'lower(CONCAT(first_name, \' \', last_name)) like ? OR lower(CONCAT(last_name, \' \', first_name)) like ?',
           params[:names].downcase, params[:names].downcase
         ).first
       end
+      if student.nil?
+        flash_message(:error, t('exam_templates.assign_scans.student_not_found', name: params[:names]))
+        head :not_found
+        return
+      end
       StudentMembership
-        .find_or_create_by(user: student, grouping: @grouping, membership_status: StudentMembership::STATUSES[:inviter])
+        .find_or_create_by(role: student, grouping: @grouping, membership_status: StudentMembership::STATUSES[:inviter])
     end
     next_grouping
   end

app/controllers/tas_controller.rb

@@ -20,7 +20,7 @@ def new
   end
 
   def create
-    end_user = EndUser.find_by_user_name(params[:user_name])
+    end_user = EndUser.find_by_user_name(end_user_params[:user_name])
     @role = current_course.tas.create(end_user: end_user, **permission_params)
     respond_with @role, location: course_tas_path(current_course)
   end

app/models/annotation.rb

@@ -30,7 +30,7 @@ class Annotation < ApplicationRecord
 
   def modify_mark_with_deduction
     criterion = self.annotation_text.annotation_category.flexible_criterion
-    self.result.marks.find_or_create_by(criterion: criterion).update_deduction
+    self.result.marks.find_or_create_by(criterion: criterion).update_deduction unless self.is_remark
   end
 
   def get_data(include_creator=false)
@@ -65,6 +65,9 @@ def get_data(include_creator=false)
   def check_if_released
     annotation_results = result.submission.results
 
+    return if is_remark && annotation_results.where.not(remark_request_submitted_at: nil)
+                                             .where('results.released_to_students': false)
+
     return if annotation_results.where('results.released_to_students': true).empty? &&
               annotation_results.where.not('remark_request_submitted_at': nil).empty?
 

app/models/assignment.rb

@@ -1238,7 +1238,7 @@ def zip_automated_test_files(user)
     zip_path = File.join('tmp', zip_name + '.zip')
     FileUtils.rm_rf zip_path
     Zip::File.open(zip_path, create: true) do |zip_file|
-      add_test_files_to_zip(zip_file, zip_name)
+      self.add_test_files_to_zip(zip_file, '')
     end
     zip_path
   end
@@ -1262,10 +1262,10 @@ def automated_test_config_to_zip(zip_file, zip_dir, specs_file_path)
 
   private
 
-  def add_test_files_to_zip(zip_file, zip_name)
+  def add_test_files_to_zip(zip_file, zip_base_dir)
     files_dir = Pathname.new self.autotest_files_dir
     self.autotest_files.map do |file|
-      path = File.join zip_name, file
+      path = zip_base_dir.empty? ? file : File.join(zip_base_dir, file)
       abs_path = files_dir.join(file)
       if abs_path.directory?
         zip_file.mkdir(path)

config/locales/views/exam_templates/en.yml

@@ -13,6 +13,7 @@ en:
       no_cover_page: This submission does not have a cover page.
       not_all_submissions_collected: Not all submissions have been collected.
       skip_group: Skip group
+      student_not_found: Student with name %{name} does not exist.
       title: Assign Scans
     back_to_exam_templates_page: Back to Exam Templates page
     back_to_log_page: Back to Log page

spec/controllers/automated_tests_controller_spec.rb

@@ -135,6 +135,38 @@
         subject
         expect(response.status).to eq(200)
       end
+      context 'non empty automated test files' do
+        before :each do
+          create_automated_test(assignment)
+        end
+        after :each do
+          # Clear uploaded autotest files to prepare for next test
+          FileUtils.rm_rf(assignment.autotest_files_dir)
+          FileUtils.rm_f(assignment.autotest_settings_file)
+        end
+        it 'should receive the appropriate files' do
+          subject
+          received_content = []
+          Zip::InputStream.open(StringIO.new(content)) do |io|
+            while (entry = io.get_next_entry)
+              unless entry.name_is_directory?
+                received_content << {
+                  name: entry.name,
+                  file_content: entry.get_input_stream.read
+                }
+              end
+            end
+          end
+          expected_content = [{
+            name: File.join('Helpers', 'test_helpers.py'),
+            file_content: "def initialize_tests()\n\treturn True"
+          }, {
+            name: 'tests.py',
+            file_content: "def sample_test()\n\tassert True == True"
+          }]
+          expect(received_content).to match_array(expected_content)
+        end
+      end
     end
     context 'POST upload_files' do
       before { post_as role, :upload_files, params: params }

spec/controllers/groups_controller_spec.rb

@@ -400,13 +400,18 @@
       let!(:assignment) { create(:assignment_for_scanned_exam) }
       let!(:student1) do
         create(:student,
-               end_user: create(:end_user,
-                                user_name: 'c9test1', first_name: 'first', last_name: 'last', id_number: '12345'))
+               end_user: create(:end_user, user_name: 'c9test1', first_name: 'first', last_name: 'last',
+                                           id_number: '12345'))
       end
       let!(:student2) do
         create(:student,
                end_user: create(:end_user, user_name: 'zzz', first_name: 'zzz', last_name: 'zzz', id_number: '789'))
       end
+      let!(:student3) do
+        create(:student,
+               end_user: create(:end_user, user_name: 'zz396', first_name: 'zzfirst', last_name: 'zzlast',
+                                           id_number: '781034'))
+      end
       let(:expected) do
         [{ 'id' => student1.id,
            'id_number' => student1.id_number,
@@ -457,6 +462,107 @@
 
         expect(response.parsed_body).to eq expected
       end
+
+      describe 'when users are already in groups' do
+        let!(:grouping) { create :grouping_with_inviter, assignment: assignment, inviter: student1 }
+        let(:assignment2) { create :assignment }
+        let(:grouping2) { create :grouping_with_inviter, assignment: assignment2, inviter: student2 }
+        it 'does not match a student already in a grouping' do
+          post_as instructor, :get_names, params: { course_id: course.id,
+                                                    assignment_id: assignment.id,
+                                                    assignment: assignment.id,
+                                                    term: '123',
+                                                    format: :json }
+          expect(response.parsed_body).to be_empty
+        end
+        it 'does match students not in the group but in other assignment groups' do
+          post_as instructor, :get_names, params: { course_id: course.id,
+                                                    assignment_id: assignment.id,
+                                                    assignment: assignment.id,
+                                                    term: '789',
+                                                    format: :json }
+
+          expect(response.parsed_body).to match_array [{ 'id' => student2.id,
+                                                         'id_number' => student2.id_number,
+                                                         'user_name' => student2.user_name,
+                                                         'value' => "#{student2.first_name} #{student2.last_name}" }]
+        end
+      end
+      describe 'when multiple students match' do
+        let(:expected) do
+          [{ 'id' => student2.id,
+             'id_number' => student2.id_number,
+             'user_name' => student2.user_name,
+             'value' => "#{student2.first_name} #{student2.last_name}" },
+           { 'id' => student3.id,
+             'id_number' => student3.id_number,
+             'user_name' => student3.user_name,
+             'value' => "#{student3.first_name} #{student3.last_name}" }]
+        end
+        it 'returns multiple matches' do
+          post_as instructor, :get_names, params: { course_id: course.id,
+                                                    assignment_id: assignment.id,
+                                                    assignment: assignment.id,
+                                                    term: 'zz',
+                                                    format: :json }
+
+          expect(response.parsed_body).to match_array expected
+        end
+      end
+
+      describe '#assign_student_and_next' do
+        let!(:assignment) { create(:assignment_for_scanned_exam) }
+        let!(:student1) do
+          create(:student,
+                 end_user: create(:end_user,
+                                  user_name: 'c9test1', first_name: 'first', last_name: 'last', id_number: '12345'))
+        end
+        let!(:student2) do
+          create(:student,
+                 end_user: create(:end_user, user_name: 'zzz', first_name: 'zzz', last_name: 'zzz', id_number: '789'))
+        end
+        let!(:grouping1) { create :grouping, assignment: assignment }
+        let!(:grouping2) { create :grouping, assignment: assignment }
+        let!(:submission1) { create :version_used_submission, grouping: grouping1 }
+        let!(:submission2) { create :version_used_submission, grouping: grouping2 }
+        it 'assigns a student to the grouping and returns the next one' do
+          post_as instructor, :assign_student_and_next, params: { course_id: course.id,
+                                                                  assignment_id: assignment.id,
+                                                                  assignment: assignment.id,
+                                                                  names: "#{student1.first_name} #{student1.last_name}",
+                                                                  s_id: student1.id,
+                                                                  g_id: grouping1.id,
+                                                                  format: :json }
+          expect(grouping1.memberships.first.role).to eq student1
+        end
+        it 'assigns a student to the grouping and returns the next one based on names' do
+          post_as instructor, :assign_student_and_next, params: { course_id: course.id,
+                                                                  assignment_id: assignment.id,
+                                                                  assignment: assignment.id,
+                                                                  names: "#{student1.first_name} #{student1.last_name}",
+                                                                  g_id: grouping1.id,
+                                                                  format: :json }
+          expect(grouping1.memberships.first.role).to eq student1
+        end
+        it 'flashes an error if the student cannot be found' do
+          post_as instructor, :assign_student_and_next, params: { course_id: course.id,
+                                                                  assignment_id: assignment.id,
+                                                                  assignment: assignment.id,
+                                                                  names: 'Student Whodoesntexist',
+                                                                  g_id: grouping1.id,
+                                                                  format: :json }
+          expect(flash[:error]).not_to be_empty
+        end
+        it 'returns a not_found status if the student cannot be found' do
+          post_as instructor, :assign_student_and_next, params: { course_id: course.id,
+                                                                  assignment_id: assignment.id,
+                                                                  assignment: assignment.id,
+                                                                  names: 'Student Whodoesntexist',
+                                                                  g_id: grouping1.id,
+                                                                  format: :json }
+          expect(response).to have_http_status(404)
+        end
+      end
     end
   end
 

spec/controllers/tas_controller_spec.rb

@@ -107,9 +107,9 @@
             manage_assessments: false,
             manage_submissions: true,
             run_tests: true
-          }
+          },
+          end_user: { user_name: end_user.user_name }
         },
-        user_name: end_user.user_name,
         course_id: course.id
       }
     end
@@ -136,7 +136,8 @@
         context 'when no permissions are selected' do
           let(:params) do
             # Rails strips empty params so a dummy value has to be given
-            { user_name: end_user.user_name, course_id: course.id, role: { grader_permission_attributes: { a: 1 } } }
+            { course_id: course.id,
+              role: { end_user: { user_name: end_user.user_name }, grader_permission_attributes: { a: 1 } } }
           end
           it 'default value for all permissions should be false' do
             ta = course.tas.where(end_user: end_user).first