Merge pull request #6288 from mishaschwartz/v2.1.5

v2.1.5

Changelog.md

@@ -1,7 +1,16 @@
 # Changelog
 
+## [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)
+- Allow users to upload and download csv files for marks spreadsheets in the same format (#6267)
+- Hide manual submission collection button from users who don't have permission (#6282)
+- Fix bug where gzipped binary feedback files were not unzipped correctly (#6283)
+- Fix bug where remark request summary table wasn't being rendered correctly (#6284)
+- Fix bug where test results were being associated with the wrong test runs (#6287)
+
 ## [v2.1.4]
-- Fix bug where git hooks are not run server side when symlinked (#6276)
+- Fix bug where git hooks are not run server side when symlinked (#6276/#6277)
 
 ## [v2.1.3]
 - Fix bug where automated test results were occasionally associated with the wrong grouping (#6238)

app/MARKUS_VERSION

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

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

@@ -83,7 +83,7 @@ export class SummaryPanel extends React.Component {
     },
     {
       Header: "Old Mark",
-      accessor: "old_mark",
+      accessor: "old_mark.mark",
       className: "number",
       show: this.props.remark_submitted,
     },

app/assets/javascripts/Components/repo_browser.jsx

@@ -77,6 +77,18 @@ class RepoBrowser extends React.Component {
     if (this.state.revision_identifier === this.props.collected_revision_id) {
       className = "collected-checked";
     }
+    let manualCollectionForm = "";
+    if (this.props.enableCollect) {
+      manualCollectionForm = (
+        <ManualCollectionForm
+          course_id={this.props.course_id}
+          assignment_id={this.props.assignment_id}
+          late_penalty={this.props.late_penalty}
+          grouping_id={this.props.grouping_id}
+          revision_identifier={this.state.revision_identifier}
+        />
+      );
+    }
     return (
       <div>
         <h3>
@@ -114,13 +126,7 @@ class RepoBrowser extends React.Component {
           enableUrlSubmit={this.props.enableUrlSubmit}
           readOnly={this.isReadOnly()}
         />
-        <ManualCollectionForm
-          course_id={this.props.course_id}
-          assignment_id={this.props.assignment_id}
-          late_penalty={this.props.late_penalty}
-          grouping_id={this.props.grouping_id}
-          revision_identifier={this.state.revision_identifier}
-        />
+        {manualCollectionForm}
       </div>
     );
   }

app/jobs/autotest_results_job.rb

@@ -34,7 +34,7 @@ def perform(_retry: 3)
         if %(started queued deferred).include? status
           outstanding_results = true
         else
-          test_run = TestRun.find_by(autotest_test_id: autotest_test_id)
+          test_run = test_runs.find_by(autotest_test_id: autotest_test_id)
           if %(finished failed).include? status
             results(test_run.grouping.assignment, test_run) unless test_run.nil?
           else

app/lib/repository.rb

@@ -262,6 +262,8 @@ def self.visibility_hash
     def self.get_all_permissions
       visibility = self.visibility_hash
       permissions = Hash.new { |h, k| h[k] = [] }
+      admins = AdminUser.pluck(:user_name)
+      permissions['*/*'] = admins unless admins.empty?
       instructors = Instructor.joins(:course, :user)
                               .pluck('courses.name', 'users.user_name')
                               .group_by(&:first)

app/models/grade_entry_form.rb

@@ -175,25 +175,25 @@ def from_csv(grades_data, overwrite)
       next unless row.any?
       # grab names and totals from the first two rows
       if names.empty?
-        names = row.drop(1)
+        names = row
         next
       elsif totals.empty?
-        totals = row.drop(1)
-        if self.show_total && names.last == GradeEntryForm.human_attribute_name(:total)
-          self.update_grade_entry_items(names[0...-1], totals[0...-1], overwrite)
-        else
-          self.update_grade_entry_items(names, totals, overwrite)
-        end
-        updated_columns = self.grade_entry_items.reload.ids
+        totals = row.map { |cell| Float(cell, exception: false)&.>=(0) ? cell : nil }
+        totals[-1] = nil if self.show_total && names.last == GradeEntryForm.human_attribute_name(:total)
+        names = names.reject.with_index { |_cell, i| totals[i].nil? }
+        self.update_grade_entry_items(names, totals.compact, overwrite)
+        updated_columns = self.grade_entry_items.reload.pluck(:name, :id).to_h
         next
+      else
+        s_id = grade_entry_students[row[0]]
+        raise CsvInvalidLineError if s_id.nil?
+        row = row.reject.with_index { |_cell, i| totals[i].nil? }
       end
-
-      s_id = grade_entry_students[row[0]]
-      raise CsvInvalidLineError if s_id.nil?
-
-      row.drop(1).zip(updated_columns).take([row.size - 1, updated_columns.size].min).each do |grade, item_id|
+      row.each_with_index do |grade, i|
+        item_id = updated_columns[names[i]]
         begin
-          new_grade = grade.blank? ? nil : Float(grade)
+          new_grade = Float(grade, exception: false)
+          new_grade = new_grade&.>=(0) ? Float(grade) : nil
         rescue ArgumentError
           raise CsvInvalidLineError
         end

app/models/test_run.rb

@@ -132,7 +132,7 @@ def create_annotations(annotation_data)
   end
 
   def unzip_file_data(file_data)
-    return ActiveSupport::Gzip.decompress(file_data['content']) if file_data['compression'] == 'gzip'
+    return Zlib.gunzip(file_data['content']) if file_data['compression'] == 'gzip'
     file_data['content']
   end
 end

app/views/submissions/repo_browser.html.erb

@@ -17,6 +17,7 @@
           collected_revision_id: '<%= @collected_revision&.revision_identifier %>',
           enableSubdirs: <%= allowed_to? :manage_subdirectories? %>,
           enableUrlSubmit: <%= @grouping.assignment.url_submit %>,
+          enableCollect: <%= allowed_to? :manually_collect_and_begin_grading? %>,
           collection_date: '<%= l(@grouping.collection_date) %>',
           due_date: '<%= l(@grouping.due_date) %>',
           is_timed: <%= @grouping.assignment.is_timed %>,

config/application.rb

@@ -110,6 +110,19 @@ class Application < Rails::Application
                             error_grouping: true
     end
 
+    if Settings.logging.tag_with_usernames && Settings.rails.session_store.type == 'cookie_store'
+      config.log_tags = [proc do |request|
+        session_info = request.cookie_jar.encrypted[Rails.application.config.session_options[:key]] || {}
+        real_user_name = session_info['real_user_name']
+        user_name = session_info['user_name']
+        if user_name && user_name != real_user_name
+          "#{real_user_name} as #{user_name}"
+        else
+          real_user_name
+        end
+      end]
+    end
+
     # TODO: review initializers 01 and 02
     # TODO review markus custom config format
     # TODO handle namespaces properly for app/lib

config/initializers/config.rb

@@ -144,6 +144,7 @@
         required(:old_files).filled(:integer, gt?: 0)
         required(:log_file).filled(:string)
         required(:error_file).filled(:string)
+        required(:tag_with_usernames).filled(:bool)
       end
       required(:scanned_exams).hash do
         required(:enable).filled(:bool)

config/locales/views/about/en.yml

@@ -4,7 +4,7 @@ en:
     contact_us: Contact Us
     credits: Credits
     credits_thanks: 'Thanks to everyone who contributed to and made MarkUs possible. In particular to:'
-    email_more_info_html: ' E-mail: <a href="mailto:[email protected]">[email protected]</a> <br>To submit a bug or issue please visit <a href="https://github.com/MarkUsProject/Markus/issues">https://github.com/MarkUsProject/Markus/issues</a>'
+    email_more_info_html: ' To submit a bug or issue please visit <a href="https://github.com/MarkUsProject/Markus/issues">https://github.com/MarkUsProject/Markus/issues</a>'
     license: License
     license_text_html: ' <p> MarkUs is made available under the <a href="https://www.opensource.org">OSI</a>-approved <a href="https://www.opensource.org/licenses/mit-license.html">MIT license</a>. </p> <p> Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons to whom the Software is furnished to do so, subject to the following conditions: </p> <p> The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. </p> <p> THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. </p> <p>Copyright (c) 2008-2018 by the authors.</p> '
     markus_version: 'MarkUs version: '

config/settings.yml

@@ -71,6 +71,7 @@ logging:
   rotate_interval: daily
   size_threshold: 1024000000
   old_files: 10
+  tag_with_usernames: true
 
 # resque-scheduler configuration: https://github.com/resque/resque-scheduler#static-schedules
 # See also: https://github.com/resque/resque-scheduler/issues/613#issuecomment-351484064

doc/README_FOR_APP

@@ -8,6 +8,3 @@ Tickets and/or bug reports:
 
 Wiki:
         https://github.com/MarkUsProject/Wiki
-
-Contact us:
-	[email protected]

spec/fixtures/files/grade_entry_forms/different_total.csv

@@ -1,3 +1,3 @@
-,Test1
-,101
-c8shosta,50
+User name,Last name,First name,Section,Id number,Email,Test1
+, , , , ,Out of,101
+c8shosta,Shostakovich,Dmitri,,,,50

spec/fixtures/files/grade_entry_forms/extra_column.csv

@@ -1,3 +1,3 @@
-,Test2,Test1
-,100,100
-c8shosta,64,50
+User name,Last name,First name,Section,Id number,Email,Test2,Test1
+, , , , ,Out of,100,100
+c8shosta,Shostakovich,Dmitri,,,,64,50

spec/fixtures/files/grade_entry_forms/good.csv

@@ -1,3 +1,3 @@
-,Test
-,100
-c8shosta,89
+User name,Last name,First name,Section,Id number,Email,Test
+, , , , ,Out of,100
+c8shosta,Shostakovich,Dmitri,,,,89

spec/fixtures/files/grade_entry_forms/good_overwrite.csv

@@ -1,3 +1,3 @@
-,Test1
-,100
-c8shosta,89
+User name,Last name,First name,Section,Id number,Email,Test1
+, , , , ,Out of,100
+c8shosta,Shostakovich,Dmitri,,,,89

spec/fixtures/files/grade_entry_forms/invalid_username.csv

@@ -1,3 +1,3 @@
-,Test,Test2
-,100,100
-invalid_username,89,64
+User name,Last name,First name,Section,Id number,Email,Test,Test2
+, , , , ,Out of,100,100
+invalid_username,,,,,,89

spec/fixtures/files/grade_entry_forms/total_column_included.csv

@@ -1,3 +1,3 @@
-,Test,Total
-Out of,100,100
-c8shosta,22,99
+User name,Last name,First name,Section,Id number,Email,Test,Total
+, , , , ,Out of,100,100
+c8shosta,Shostakovich,Dmitri,,,,22,99

spec/jobs/autotest_results_job_spec.rb

@@ -163,6 +163,38 @@
             subject
           end
         end
+        context 'when there is a test run with the same autotest_test_id in a different course' do
+          let(:status_return) { { 1 => 'finished' } }
+          let(:other_course) { create :course, autotest_setting: create(:autotest_setting) }
+          let(:other_role) { create :instructor, course: other_course }
+          let(:other_assignment) do
+            create :assignment,
+                   assignment_properties_attributes: { remote_autotest_settings_id: 11 },
+                   course: other_course
+          end
+          let(:other_grouping) { create :grouping, assignment: other_assignment }
+          let(:test_runs) { [] }
+          let(:other_test_runs) do
+            [
+              create(:test_run, grouping: grouping, autotest_test_id: 1, status: :in_progress),
+              create(:test_run, grouping: other_grouping, role: other_role,
+                                autotest_test_id: 1, status: :in_progress)
+            ]
+          end
+          before do
+            grouping.course.autotest_setting = create(:autotest_setting)
+            other_test_runs
+          end
+          it 'should get results for both test runs' do
+            allow_any_instance_of(AutotestResultsJob).to receive(:send_request).and_return(dummy_return)
+            called_test_runs = []
+            allow_any_instance_of(AutotestResultsJob).to receive(:results) do |_job, _assignment, test_run|
+              called_test_runs << test_run
+            end
+            subject
+            expect(called_test_runs.map(&:id)).to contain_exactly(*other_test_runs.map(&:id))
+          end
+        end
       end
     end
     context 'tests are not set up' do

spec/models/test_run_spec.rb

@@ -50,31 +50,68 @@
     let(:test_run) { create :test_run, status: :in_progress, grouping: grouping }
     let(:criterion) { create :flexible_criterion, max_mark: 2, assignment: assignment }
     let(:test_group) { create :test_group, criterion: criterion, assignment: assignment }
+    let(:png_file_content) { fixture_file_upload('page_white_text.png').read }
+    let(:text_file_content) { 'test123' }
     let(:results) do
-      JSON.parse({ status: :finished,
-                   error: nil,
-                   test_groups: [{
-                     time: 10,
-                     timeout: nil,
-                     stderr: '',
-                     malformed: '',
-                     extra_info: { test_group_id: test_group.id },
-                     tests: [{
-                       name: :test1,
-                       status: :pass,
-                       marks_earned: 1,
-                       marks_total: 1,
-                       output: 'output',
-                       time: 1
-                     }, {
-                       name: :test2,
-                       status: :fail,
-                       marks_earned: 0,
-                       marks_total: 1,
-                       output: 'failure',
-                       time: nil
-                     }]
-                   }] }.to_json)
+      { status: :finished,
+        error: nil,
+        test_groups: [{
+          time: 10,
+          timeout: nil,
+          stderr: '',
+          malformed: '',
+          extra_info: { test_group_id: test_group.id },
+          feedback: [
+            { filename: 'test.txt', mime_type: 'text', content: text_file_content },
+            {
+              filename: 'test_compressed.txt',
+              mime_type: 'text',
+              compression: 'gzip',
+              content: Zlib.gzip(text_file_content)
+            },
+            { filename: 'test.png', mime_type: 'image/png', content: png_file_content },
+            {
+              filename: 'test_compressed.png',
+              mime_type: 'image/png',
+              compression: 'gzip',
+              content: Zlib.gzip(png_file_content)
+            }
+          ],
+          tests: [{
+            name: :test1,
+            status: :pass,
+            marks_earned: 1,
+            marks_total: 1,
+            output: 'output',
+            time: 1
+          }, {
+            name: :test2,
+            status: :fail,
+            marks_earned: 0,
+            marks_total: 1,
+            output: 'failure',
+            time: nil
+          }]
+        }] }.deep_stringify_keys
+    end
+    context 'when there are feedback files' do
+      let(:feedback_files) { test_group.test_group_results.first.feedback_files }
+      before { test_run.update_results!(results) }
+      it 'should create 4 feedback files' do
+        expect(feedback_files.count).to eq 4
+      end
+      it 'should create a feedback file from uncompressed text' do
+        expect(feedback_files.find_by(filename: 'test.txt').file_content).to eq(text_file_content)
+      end
+      it 'should create a feedback file from compressed text' do
+        expect(feedback_files.find_by(filename: 'test_compressed.txt').file_content).to eq(text_file_content)
+      end
+      it 'should create a feedback file from uncompressed binary data' do
+        expect(feedback_files.find_by(filename: 'test.png').file_content).to eq(png_file_content)
+      end
+      it 'should create a feedback file from compressed binary data' do
+        expect(feedback_files.find_by(filename: 'test_compressed.png').file_content).to eq(png_file_content)
+      end
     end
     context 'there is a failure reported' do
       before { results['status'] = 'failed' }