Merge pull request #5742 from mishaschwartz/v2.0.3

v2.0.3

Changelog.md

@@ -1,5 +1,9 @@
 # Changelog
 
+## [v2.0.3]
+- Fix bug where repository access files were not taking multiple courses into account (#5734)
+- Fix bug where sections and grace credits could not be updated from the student edit view (#5739)
+
 ## [v2.0.2]
 - Fix bug in displaying feedback files for test results (#5719)
 - Require starter group names to be unique for an assignment (#5721)

app/MARKUS_VERSION

@@ -1 +1 @@
-VERSION=2.0.2,PATCH_LEVEL=DEV
+VERSION=2.0.3,PATCH_LEVEL=DEV

app/controllers/students_controller.rb

@@ -163,7 +163,7 @@ def update_settings
   private
 
   def role_params
-    params.permit(:grace_credits, :section_id)
+    params.require(:role).permit(:grace_credits, :section_id)
   end
 
   def settings_params

app/jobs/update_repo_permissions_job.rb

@@ -18,8 +18,7 @@ def perform(repo_class_name)
     redis = Redis::Namespace.new(Rails.root.to_s)
     redis.del('repo_permissions')
     permissions = repo_class.get_all_permissions
-    full_access_users = repo_class.get_full_access_users
-    repo_class.update_permissions_file(permissions, full_access_users)
+    repo_class.update_permissions_file(permissions)
   ensure
     redis = Redis::Namespace.new(Rails.root.to_s)
     if redis.get('repo_permissions') == self.job_id

app/jobs/upload_roles_job.rb

@@ -13,7 +13,8 @@ def perform(role_class, course, data, encoding)
         next if user_name.blank?
         end_user = EndUser.find_by_user_name(user_name)
         raise I18n.t('users.not_found', user_names: user_name) if end_user.nil?
-        role = role_class.new(end_user: end_user, course: course, section_id: find_section_id(row))
+        role = role_class.find_or_initialize_by(end_user: end_user, course: course)
+        role.section_id = find_section_id(row)
         raise "#{user_name}: #{role.errors.full_messages.join('; ')}" unless role.save
         progress.increment
       end

app/lib/git_repository.rb

@@ -415,22 +415,19 @@ def self.tmp_repo(connect_string)
   # Helper method to generate all the permissions for students for all groupings in all assignments.
   # This is done as a single operation to mirror the SVN repo code. We found
   # a substantial performance improvement by writing the auth file only once in the SVN case.
-  def self.update_permissions_file(permissions, full_access_users)
-
+  def self.update_permissions_file(permissions)
     # If we're not in authoritative mode, bail out
     unless Settings.repository.is_repository_admin
       raise NotAuthorityError.new(
         'Unable to set bulk permissions: Not in authoritative mode!')
     end
 
     # Create auth csv file
-    sorted_permissions = permissions.sort.to_h
     FileUtils.mkdir_p(File.dirname(Repository::PERMISSION_FILE))
     CSV.open(Repository::PERMISSION_FILE, 'wb') do |csv|
       csv.flock(File::LOCK_EX)
       begin
-        csv << ['*'] + full_access_users
-        sorted_permissions.each do |repo_name, users|
+        permissions.each do |repo_name, users|
           csv << [repo_name] + users
         end
       ensure

app/lib/memory_repository.rb

@@ -227,8 +227,7 @@ def expand_path(file_name, dir_string = "/")
     return expanded
   end
 
-  def self.update_permissions_file(permissions, full_access_users)
-    @@permissions = { '*' => full_access_users }
+  def self.update_permissions_file(permissions)
     permissions.each do |repo_loc, users|
       @@permissions[repo_loc] = users
     end

app/lib/repository.rb

@@ -169,34 +169,7 @@ def get_revision_by_timestamp(at_or_earlier_than, path = nil, later_than = nil)
       raise NotImplementedError,  "Repository.get_revision_by_timestamp: Not yet implemented"
     end
 
-    def self.get_full_access_users
-      Instructor.joins(:end_user).pluck(:user_name)
-    end
-
-    # Gets a list of users with permission to access the repo.
-    # All permissions are rw for the time being
-    def get_users
-      unless Settings.repository.is_repository_admin # are we admin?
-        raise NotAuthorityError.new('Unable to get permissions: Not in authoritative mode!')
-      end
-      repo_name = get_repo_name
-      permissions = self.get_all_permissions
-      permissions.fetch(repo_name, []) + self.get_full_access_users
-    end
-
-    # TODO All permissions are rw for the time being
-    def get_permissions(user_name)
-      unless Settings.repository.is_repository_admin # are we admin?
-        raise NotAuthorityError.new('Unable to get permissions: Not in authoritative mode!')
-      end
-      unless get_users.include?(user_name)
-        raise UserNotFound.new("User #{user_name} not found in this repo")
-      end
-
-      Repository::Permission::READ_WRITE
-    end
-
-    #Converts a pathname to an absolute pathname
+    # Converts a pathname to an absolute pathname
     def expand_path(file_name, dir_string)
       raise NotImplementedError, "Repository.expand_path: Not yet implemented"
     end
@@ -286,22 +259,31 @@ def self.visibility_hash
     # Builds a hash of all repositories and users allowed to access them (assumes all permissions are rw)
     def self.get_all_permissions
       visibility = self.visibility_hash
-      permissions = Hash.new { |h,k| h[k]=[] }
+      permissions = Hash.new { |h, k| h[k] = [] }
+      instructors = Instructor.joins(:course, :end_user)
+                              .pluck('courses.name', 'users.user_name')
+                              .group_by(&:first)
+                              .transform_values { |val| val.map(&:second) }
+      instructors.each do |course_name, instructor_names|
+        permissions[File.join(course_name, '*')] = instructor_names
+      end
       self.get_repo_auth_records.each do |assignment|
         assignment.valid_groupings.each do |valid_grouping|
           next unless visibility[assignment.id][valid_grouping.inviter&.section&.id]
-          repo_name = valid_grouping.group.repo_name
+          repo_name = valid_grouping.group.repository_relative_path
           accepted_students = valid_grouping.accepted_students.map(&:user_name)
           permissions[repo_name] = accepted_students
         end
       end
       # NOTE: this will allow graders to access the files in the entire repository
       # even if they are the grader for only a single assignment
-      graders_info = TaMembership.joins(role: :end_user, grouping: [:group, assignment: :assignment_properties])
+      graders_info = TaMembership.joins(role: [:end_user, :course],
+                                        grouping: [:group, { assignment: :assignment_properties }])
                                  .where('assignment_properties.anonymize_groups': false)
-                                 .pluck(:repo_name, :user_name)
-      graders_info.each do |repo_name, user_name|
-        permissions[repo_name] << user_name
+                                 .pluck(:repo_name, :user_name, 'courses.name')
+      graders_info.each do |repo_name, user_name, course_name|
+        repo_path = File.join(course_name, repo_name) # NOTE: duplicates functionality of Group.repository_relative_path
+        permissions[repo_path] << user_name
       end
       permissions
     end
@@ -313,8 +295,8 @@ def self.reserved_locations
     end
 
     # Generate and write the the authorization file for all repos.
-    def self.update_permissions_file(_permissions, _full_access_users)
-      raise NotImplementedError, "Repository.update_permissions: Not yet implemented"
+    def self.update_permissions_file(_permissions)
+      raise NotImplementedError
     end
 
     # Returns a set of file names that are used internally by the repository and are not part of any student submission.

app/lib/subversion_repository.rb

@@ -474,13 +474,9 @@ def __get_file_paths(revision_number)
   ####################################################################
 
   # Generate and write the SVN authorization file for the repo.
-  def self.update_permissions_file(permissions, full_access_users)
+  def self.update_permissions_file(permissions)
     return true unless Settings.repository.is_repository_admin
-    authz_string = "[/]\n"
-    full_access_users.each do |user_name|
-      authz_string += "#{user_name} = rw\n"
-    end
-    authz_string += "\n"
+    authz_string = ''
     permissions.each do |repo_name, users|
       authz_string += "[#{repo_name}:/]\n"
       users.each do |user_name|

app/views/students/_student_form.html.erb

@@ -4,16 +4,22 @@
   <div class='inline-labels'>
     <%= f.fields_for(@role.end_user || EndUser.new) do |u_f| %>
       <%= u_f.label :user_name, User.human_attribute_name(:user_name) %>
-      <%= u_f.text_field :user_name %>
+      <%= u_f.text_field :user_name, readonly: @role.persisted? %>
     <% end %>
 
     <%= f.label :grace_credits, User.human_attribute_name(:grace_credits) %>
     <%= f.text_field :grace_credits %>
 
     <%= f.label :section_id, Section.model_name.human %>
     <span id='select_sections'>
-      <%= render partial: 'sections/select_sections',
-                 locals: { user: @role, new_student: true } %>
+      <% selected ||= @role.has_section? ? @role.section.id : [''] %>
+      <%= f.select :section_id,
+                   options_for_select([''] + @sections.map {|s| [s.name, s.id] },
+                                      selected: selected) %>
+      <%= link_to t('sections.new.title'),
+                  add_new_section_course_students_path(@current_course),
+                  remote: true,
+                  class: 'button inline-button' %>
     </span>
   </div>
 

spec/jobs/upload_users_job_spec.rb → spec/jobs/upload_roles_job_spec.rb

@@ -15,13 +15,6 @@
           create :end_user, user_name: :c6conley
           create :end_user, user_name: :c8rada
         end
-        context 'and there are duplicates in the file' do
-          let(:data) { fixture_file_upload('tas/form_invalid_record.csv', 'text/csv').read }
-          it 'does not create roles' do
-            expect { subject }.to raise_exception(RuntimeError)
-            expect(role_type.count).to eq 0
-          end
-        end
         context 'and a user already has a role in the course' do
           before do
             create :instructor, end_user: EndUser.find_by_user_name(:c6conley), course: course

spec/lib/repo/git_repository_spec.rb

@@ -2,7 +2,7 @@
   context 'writes to repository permissions file' do
 
     before :all do
-      GitRepository.send :update_permissions_file, { mock_repo: [:student1, :student2] }, ['instructor1']
+      GitRepository.public_send :update_permissions_file, { mock_repo: [:student1, :student2] }
     end
 
     after :all do
@@ -11,15 +11,10 @@
 
     let(:file_contents) { File.read(Repository::PERMISSION_FILE).lines.map(&:chomp) }
 
-    it 'give instructors access to all repos' do
-      expect(file_contents[0].split(',')[0]).to eq('*')
-      expect(file_contents[0].split(',')[1]).to eq('instructor1')
-    end
-
-    it 'gives other users access to specific repos' do
-      expect(file_contents[1].split(',')[0]).to eq('mock_repo')
-      expect(file_contents[1].split(',')[1]).to eq('student1')
-      expect(file_contents[1].split(',')[2]).to eq('student2')
+    it 'gives users access to specific repos' do
+      expect(file_contents.first.split(',')[0]).to eq('mock_repo')
+      expect(file_contents.first.split(',')[1]).to eq('student1')
+      expect(file_contents.first.split(',')[2]).to eq('student2')
     end
   end
 end

spec/lib/repo/memory_repository_spec.rb

@@ -4,14 +4,10 @@
     before :all do
       @repo_loc = 'mock_repo'
       @students = [:student1, :student2]
-      MemoryRepository.send :update_permissions_file, { @repo_loc => @students }, ['instructor1']
+      MemoryRepository.public_send :update_permissions_file, { @repo_loc => @students }
     end
 
-    it 'give instructors access to all repos' do
-      expect(MemoryRepository.class_variable_get(:@@permissions)['*']).to eq(['instructor1'])
-    end
-
-    it 'gives other users access to specific repos' do
+    it 'gives users access to specific repos' do
       expect(MemoryRepository.class_variable_get(:@@permissions)[@repo_loc]).to eq(@students)
     end
   end