Merge pull request #6234 from mishaschwartz/v2.1.2

V2.1.2

Changelog.md

@@ -1,5 +1,12 @@
 # Changelog
 
+## [v2.1.2]
+- Fix bug preventing use of the update_grades API route (#6188)
+- Fix overly restrictive policies for admin users (#6209)
+- Add check for version format to ensure that Wiki links are well-formed (#6221)
+- Fix bug where empty groups are hidden by default (#6222)
+- Fix bug where test run table did not re-render when switching results (#6223)
+
 ## [v2.1.1]
 - Fix bug where files could not be uploaded using drag and drop if no files or folders previously existed. (#6117)
 - Added drag and drop functionality to upload starter files and automated tests. (#6117)

app/MARKUS_VERSION

@@ -1 +1 @@
-VERSION=2.1.1,PATCH_LEVEL=DEV
+VERSION=v2.1.2,PATCH_LEVEL=DEV

app/assets/javascripts/Components/groups_manager.jsx

@@ -46,7 +46,7 @@ class GroupsManager extends React.Component {
       this.groupsTable.resetSelection();
       var inactive_groups_count = 0;
       res.groups.forEach(group => {
-        if (group.members.every(member => member[2])) {
+        if (group.members.length && group.members.every(member => member[2])) {
           group.inactive = true;
           inactive_groups_count += 1;
         } else {

app/assets/javascripts/Components/test_run_table.jsx

@@ -26,7 +26,7 @@ export class TestRunTable extends React.Component {
       prevProps.instructor_run !== this.props.instructor_run ||
       prevProps.instructor_view !== this.props.instructor_view
     ) {
-      this.fetchData();
+      this.setState({loading: true}, this.fetchData);
     }
   }
 
@@ -84,6 +84,7 @@ export class TestRunTable extends React.Component {
         <ReactTable
           ref={this.testRuns}
           data={this.state.data}
+          key={this.state.data.length ? this.state.data[0]["test_runs.id"] : "empty-table"}
           columns={[
             {
               id: "created_at",

app/controllers/application_controller.rb

@@ -74,6 +74,7 @@ def set_markus_version
       version_info[k.downcase] = v
     end
     @markus_version = version_info['version']
+    raise "Invalid Version: #{@markus_version}" unless @markus_version.match(/master|v\d+\.\d+\.\d+/)
   end
 
   # Set locale according to URL parameter. If unknown parameter is

app/controllers/grade_entry_forms_controller.rb

@@ -135,7 +135,7 @@ def populate_grades_table
 
     if current_role.instructor?
       students = grade_entry_form.grade_entry_students
-                                 .joins(role: :user)
+                                 .joins(:user)
                                  .pluck_to_hash(*student_pluck_attrs)
       grades = grade_entry_form.grade_entry_students
                                .joins(:grades)
@@ -144,7 +144,7 @@ def populate_grades_table
     elsif current_role.ta?
       students = current_role.grade_entry_students
                              .where(grade_entry_form: grade_entry_form)
-                             .joins(role: :user)
+                             .joins(:user)
                              .pluck_to_hash(*student_pluck_attrs)
       grades = current_role.grade_entry_students
                            .where(grade_entry_form: grade_entry_form)

app/controllers/marks_graders_controller.rb

@@ -33,7 +33,7 @@ def index
 
         # Student information
         student_data = gef.grade_entry_students
-                          .left_outer_joins(role: :user, tas: :user)
+                          .left_outer_joins(:user, tas: :user)
                           .pluck('roles.id',
                                  'users.user_name',
                                  'users.first_name',

app/lib/session_handler.rb

@@ -27,18 +27,24 @@ def real_user
   end
 
   def current_role
-    if current_user&.admin_user?
-      @current_role ||= AdminRole.find_or_create_by(user: current_user, course: current_course)
+    return @current_role if defined? @current_role
+    if current_course.nil?
+      @current_role = nil
+    elsif current_user&.admin_user?
+      @current_role = AdminRole.find_or_create_by(user: current_user, course: current_course)
     else
-      @current_role ||= Role.find_by(user: current_user, course: current_course)
+      @current_role = Role.find_by(user: current_user, course: current_course)
     end
   end
 
   def real_role
-    if real_user&.admin_user?
-      @real_role ||= AdminRole.find_or_create_by(user: real_user, course: current_course)
+    return @real_role if defined? @real_role
+    if current_course.nil?
+      @real_role = nil
+    elsif real_user&.admin_user?
+      @real_role = AdminRole.find_or_create_by(user: real_user, course: current_course)
     else
-      @real_role ||= Role.find_by(user: real_user, course: current_course)
+      @real_role = Role.find_by(user: real_user, course: current_course)
     end
   end
 

app/models/grade_entry_form.rb

@@ -101,7 +101,7 @@ def export_as_csv(role)
     elsif role.ta?
 
       students = role.grade_entry_students
-                     .joins(role: :user)
+                     .joins(:user)
                      .joins('LEFT OUTER JOIN sections ON sections.id = roles.section_id')
                      .where(grade_entry_form: self, 'roles.hidden': false,
                             'grade_entry_students.assessment_id': self.id)
@@ -125,13 +125,13 @@ def export_as_csv(role)
 
     if role.instructor?
       grade_data = self.grades
-                       .joins(:grade_entry_item, grade_entry_student: [role: :user])
+                       .joins(:grade_entry_item, grade_entry_student: :user)
                        .pluck('users.user_name', 'grade_entry_items.position', :grade)
                        .group_by { |x| x[0] }
       num_items = self.grade_entry_items.count
     elsif role.ta?
       grade_data = role.grade_entry_students
-                       .joins(role: :user)
+                       .joins(:user)
                        .joins(:grades)
                        .joins(:grade_entry_items)
                        .where(grade_entry_form: self)
@@ -160,7 +160,7 @@ def export_as_csv(role)
   end
 
   def from_csv(grades_data, overwrite)
-    grade_entry_students = self.grade_entry_students.joins(role: :user).pluck('users.user_name', :id).to_h
+    grade_entry_students = self.grade_entry_students.joins(:user).pluck('users.user_name', :id).to_h
     all_grades = Set.new(
       self.grades.where.not(grade: nil).pluck(:grade_entry_student_id, :grade_entry_item_id)
     )

app/models/grade_entry_student.rb

@@ -18,6 +18,8 @@ class GradeEntryStudent < ApplicationRecord
   has_many :grade_entry_student_tas
   has_many :tas, through: :grade_entry_student_tas
 
+  has_one :user, through: :role
+
   validates :released_to_student, inclusion: { in: [true, false] }
 
   before_save :refresh_total_grade

app/models/grade_entry_student_ta.rb

@@ -46,8 +46,8 @@ def self.from_csv(grade_entry_form, csv_data, remove_existing)
 
     new_mappings = []
     tas = grade_entry_form.course.tas.joins(:user).pluck('users.user_name', :id).to_h
-    grade_entry_students = grade_entry_form.grade_entry_students.joins(role: :user).pluck('users.user_name',
-                                                                                          :id).to_h
+    grade_entry_students = grade_entry_form.grade_entry_students.joins(:user).pluck('users.user_name',
+                                                                                    :id).to_h
 
     result = MarkusCsv.parse(csv_data.read) do |row|
       raise CsvInvalidLineError if row.empty?

app/policies/admin/main_admin_policy.rb

@@ -4,14 +4,9 @@ class MainAdminPolicy < ApplicationPolicy
     default_rule :manage?
 
     skip_pre_check :role_exists?
-    pre_check :admin_user?
 
     def manage?
       real_user.admin_user?
     end
-
-    def admin_user?
-      allow! if real_user.admin_user?
-    end
   end
 end

app/policies/course_policy.rb

@@ -10,7 +10,7 @@ def show?
   end
 
   def index?
-    true
+    user.end_user?
   end
 
   def edit?

app/policies/submission_policy.rb

@@ -19,7 +19,7 @@ def manage_files?
   end
 
   def manage_subdirectories?
-    user.end_user?
+    true
   end
 
   def view_files?

app/policies/user_policy.rb

@@ -21,6 +21,6 @@ def update_settings?
 
   # Any standard user can reset their API key
   def reset_api_key?
-    user.end_user?
+    true
   end
 end

spec/controllers/api/grade_entry_forms_controller_spec.rb

@@ -286,7 +286,32 @@
       end
     end
     context 'PUT update_grades' do
-      # TODO
+      let(:student) { create :student }
+      let(:grade_params) do
+        { short_identifier: 'A0', course_id: course.id, description: 'Test',
+          due_date: '2012-03-26 18:04:39', is_hidden: false,
+          id: grade_entry_form.id,
+          grade_entry_items: [
+            { name: 'col1', out_of: 10, bonus: false },
+            { name: 'col2', out_of: 2, bonus: true }
+          ] }
+      end
+      let!(:grade_entry_item) { create :grade_entry_item, grade_entry_form: grade_entry_form, out_of: 5, name: 'col1' }
+      it 'creates new grades' do
+        put :update_grades,
+            params: { course_id: course.id, id: grade_entry_form.id, user_name: student.user_name,
+                      grade_entry_items: { col1: 2 } }
+        expect(grade_entry_form.grade_entry_students.find_by(role: student).grades.count).to eq(1)
+      end
+      it 'updates existing grades' do
+        put :update_grades,
+            params: { course_id: course.id, id: grade_entry_form.id, user_name: student.user_name,
+                      grade_entry_items: { col1: 2 } }
+        put :update_grades,
+            params: { course_id: course.id, id: grade_entry_form.id, user_name: student.user_name,
+                      grade_entry_items: { col1: 5 } }
+        expect(grade_entry_form.grade_entry_students.find_by(role: student).grades.first.grade).to eq(5)
+      end
     end
   end
 end

spec/controllers/grade_entry_forms_controller_spec.rb

@@ -31,7 +31,7 @@
       @file_total_included = fixture_file_upload('grade_entry_forms/total_column_included.csv', 'text/csv')
 
       @student = grade_entry_form_with_data.grade_entry_students
-                                           .joins(role: :user)
+                                           .joins(:user)
                                            .find_by('users.user_name': 'c8shosta')
       @original_item = grade_entry_form_with_data.grade_entry_items.first
       @student.grades.find_or_create_by(grade_entry_item: @original_item).update(
@@ -304,7 +304,7 @@
       end
       context 'when the ta has been assigned to a student' do
         let(:student) do
-          gef.grade_entry_students.joins(role: :user).find_by('users.user_name': 'c8shosta')
+          gef.grade_entry_students.joins(:user).find_by('users.user_name': 'c8shosta')
         end
         let!(:grade_entry_student_ta) { create(:grade_entry_student_ta, ta: role, grade_entry_student: student) }
         it 'returns data for only the assigned student' do
@@ -339,9 +339,9 @@
     before :each do
       create(:student, user: create(:end_user, user_name: 'paneroar'))
       @student = grade_entry_form_with_data.grade_entry_students
-                                           .joins(role: :user).find_by('users.user_name': 'c8shosta')
+                                           .joins(:user).find_by('users.user_name': 'c8shosta')
       @another = grade_entry_form_with_data.grade_entry_students
-                                           .joins(role: :user).find_by('users.user_name': 'paneroar')
+                                           .joins(:user).find_by('users.user_name': 'paneroar')
       @this_form = grade_entry_form_with_data
     end
 
@@ -459,7 +459,7 @@
     end
     describe 'When the grader is not allowed to release and unrelease the grades' do
       let(:student) do
-        grade_entry_form_with_data.grade_entry_students.joins(role: :user).find_by('users.user_name': 'c8shosta')
+        grade_entry_form_with_data.grade_entry_students.joins(:user).find_by('users.user_name': 'c8shosta')
       end
       it 'should respond with 403' do
         post_as user, :update_grade_entry_students,
@@ -680,7 +680,7 @@
       end
       context 'who has been assigned a student' do
         let(:student) do
-          grade_entry_form_with_data.grade_entry_students.joins(role: :user).find_by('users.user_name': 'c8shosta')
+          grade_entry_form_with_data.grade_entry_students.joins(:user).find_by('users.user_name': 'c8shosta')
         end
         let!(:grade_entry_student_ta) { create(:grade_entry_student_ta, ta: role, grade_entry_student: student) }
         it 'returns data' do
@@ -695,7 +695,7 @@
           get_as role, :populate_grades_table, params: { course_id: course.id, id: grade_entry_form_with_data.id }
           students = role.grade_entry_students
                          .where(grade_entry_form: grade_entry_form_with_data)
-                         .joins(role: :user).pluck('users.user_name')
+                         .joins(:user).pluck('users.user_name')
           expect(response.parsed_body['data'].map { |stu| stu['user_name'] }).to match_array(students)
         end
       end

spec/controllers/main_controller_spec.rb

@@ -26,6 +26,30 @@
       end
     end
   end
+  describe 'tests for all routes' do
+    # check_timeout is used here as a basic example but any route could be used in its place
+    describe 'set_markus_version' do
+      let(:version_number) { "#{rand(0..100)}.#{rand(0..100)}.#{rand(0..100)}" }
+      it 'should allow a master version' do
+        allow_any_instance_of(File).to receive(:read).and_return('VERSION=master')
+        expect { get :check_timeout }.not_to raise_error
+      end
+      it 'should not allow a generic release version' do
+        allow_any_instance_of(File).to receive(:read).and_return('VERSION=release')
+        expect { get :check_timeout }.to raise_error(RuntimeError)
+      end
+      it 'should allow a properly formatted release version' do
+        version = "VERSION=v#{version_number}"
+        allow_any_instance_of(File).to receive(:read).and_return(version)
+        expect { get :check_timeout }.not_to raise_error
+      end
+      it 'should not allow a release version without a v prefix' do
+        version = "VERSION=#{version_number}"
+        allow_any_instance_of(File).to receive(:read).and_return(version)
+        expect { get :check_timeout }.to raise_error(RuntimeError)
+      end
+    end
+  end
   context 'An Instructor' do
     let :all_assignments do
       a2 = create(:assignment, due_date: 1.day.ago)

spec/controllers/marks_graders_controller_spec.rb

@@ -25,7 +25,7 @@
     it 'accepts a valid file and can preserve existing TA mappings' do
       create(:student, user: create(:end_user, user_name: 'c5granad'))
       ges = grade_entry_form_with_data.grade_entry_students
-                                      .joins(role: :user)
+                                      .joins(:user)
                                       .find_by('users.user_name': 'c5granad')
       create(:grade_entry_student_ta, grade_entry_student: ges, ta: @ta)
       post_as instructor,
@@ -45,7 +45,7 @@
       # check that the ta was assigned to each student
       @student_user_names.each do |name|
         expect(
-          GradeEntryStudentTa.joins(grade_entry_student: [role: :user])
+          GradeEntryStudentTa.joins(grade_entry_student: :user)
                              .exists?('users.user_name': name)
         ).to be true
       end
@@ -55,7 +55,7 @@
     it 'accepts a valid file and can remove existing TA mappings' do
       create(:student, user: create(:end_user, user_name: 'c5granad'))
       ges = grade_entry_form_with_data.grade_entry_students
-                                      .joins(role: :user)
+                                      .joins(:user)
                                       .find_by('users.user_name': 'c5granad')
       create(:grade_entry_student_ta, grade_entry_student: ges, ta: @ta)
       post_as instructor,
@@ -76,7 +76,7 @@
       # check that the ta was assigned to each student
       @student_user_names.each do |name|
         expect(
-          GradeEntryStudentTa.joins(grade_entry_student: [role: :user])
+          GradeEntryStudentTa.joins(grade_entry_student: :user)
             .exists?(grade_entry_student: { users: { user_name: name } })
         ).to be true
       end

spec/factories/admin_roles.rb

@@ -1,5 +1,5 @@
 FactoryBot.define do
   factory :admin_role, class: AdminRole, parent: :role do
-    user { association(:admin_user) }
+    association :user, factory: :admin_user
   end
 end

spec/policies/course_policy_spec.rb

@@ -1,5 +1,5 @@
 describe CoursePolicy do
-  let(:context) { { role: role, real_user: role.user } }
+  let(:context) { { role: role, real_user: role.user, user: role.user } }
   let(:role) { create :instructor }
   describe_rule :show? do
     succeed 'role is an instructor'
@@ -11,6 +11,9 @@
     end
   end
   describe_rule :index? do
-    succeed
+    succeed 'role is an end user'
+    failed 'user is an adminuser' do
+      let(:role) { create :admin_role }
+    end
   end
 end