From 79ce069f6868561db67eee5c9a843a7e6171d246 Mon Sep 17 00:00:00 2001 From: kiragrammel Date: Thu, 14 Sep 2023 16:56:36 +0200 Subject: [PATCH] Add CRUD operations for Programming Groups * Correct sorting in table * Modify page when nested in exercises * Fix links between pages * Link from statistics page to programming_groups/index * Link from submission page to programming_groups/ * Allow filtering for exercise ID on ProgrammingGroup#index * Add search fields for internal and external user id on pg/index --- .../programming_groups_controller.rb | 45 ++- app/helpers/statistics_helper.rb | 1 + app/models/exercise.rb | 3 +- app/models/programming_group.rb | 31 +- app/models/programming_group_membership.rb | 4 + app/models/user.rb | 4 +- app/policies/exercise_policy.rb | 4 + app/policies/programming_group_policy.rb | 6 +- app/views/application/_navigation.html.slim | 2 +- app/views/exercises/index.html.slim | 1 + app/views/exercises/show.html.slim | 1 + .../programming_groups/_form_edit.html.slim | 16 + app/views/programming_groups/edit.html.slim | 3 + app/views/programming_groups/index.html.slim | 43 +++ app/views/programming_groups/show.html.slim | 19 + app/views/submissions/index.html.slim | 2 +- app/views/submissions/show.html.slim | 2 +- config/locales/de.yml | 11 +- config/locales/en.yml | 11 +- config/routes.rb | 4 +- .../programming_groups_controller_spec.rb | 345 ++++++++++++++++++ spec/policies/exercise_policy_spec.rb | 12 +- .../policies/programming_group_policy_spec.rb | 11 + 23 files changed, 543 insertions(+), 38 deletions(-) create mode 100644 app/views/programming_groups/_form_edit.html.slim create mode 100644 app/views/programming_groups/edit.html.slim create mode 100644 app/views/programming_groups/index.html.slim create mode 100644 app/views/programming_groups/show.html.slim create mode 100644 spec/controllers/programming_groups_controller_spec.rb diff --git a/app/controllers/programming_groups_controller.rb b/app/controllers/programming_groups_controller.rb index 77711cfc..78494a99 100644 --- a/app/controllers/programming_groups_controller.rb +++ b/app/controllers/programming_groups_controller.rb @@ -4,7 +4,17 @@ class ProgrammingGroupsController < ApplicationController include CommonBehavior include LtiHelper - before_action :set_exercise_and_authorize + before_action :set_exercise_and_authorize, only: %i[new create] + before_action :set_programming_group_and_authorize, only: MEMBER_ACTIONS + + def index + set_exercise_and_authorize if params[:exercise_id].present? + @search = ProgrammingGroup.ransack(params[:q], {auth_object: current_user}) + @programming_groups = @search.result.includes(:exercise, :programming_group_memberships, :internal_users, :external_users).order(:id).paginate(page: params[:page], per_page: per_page_param) + authorize! + end + + def show; end def new Event.create(category: 'page_visit', user: current_user, exercise: @exercise, data: 'programming_groups_new', file_id: nil) @@ -23,9 +33,13 @@ class ProgrammingGroupsController < ApplicationController end end + def edit + @members = @programming_group.programming_group_memberships.includes(:user) + end + def create - programming_partner_ids = programming_group_params[:programming_partner_ids].split(',').map(&:strip).uniq - users = programming_partner_ids.map do |partner_id| + programming_partner_ids = programming_group_params&.fetch(:programming_partner_ids, [])&.split(',')&.map(&:strip)&.uniq + users = programming_partner_ids&.map do |partner_id| User.find_by_id_with_type(partner_id) rescue ActiveRecord::RecordNotFound partner_id @@ -33,12 +47,12 @@ class ProgrammingGroupsController < ApplicationController @programming_group = ProgrammingGroup.new(exercise: @exercise, users:) authorize! - unless programming_partner_ids.include? current_user.id_with_type + unless programming_partner_ids&.include? current_user.id_with_type @programming_group.add(current_user) end unless @programming_group.valid? - Event.create(category: 'pp_invalid_partners', user: current_user, exercise: @exercise, data: programming_group_params[:programming_partner_ids], file_id: nil) + Event.create(category: 'pp_invalid_partners', user: current_user, exercise: @exercise, data: programming_group_params&.fetch(:programming_partner_ids), file_id: nil) end create_and_respond(object: @programming_group, path: proc { implement_exercise_path(@exercise) }) do @@ -65,14 +79,26 @@ class ProgrammingGroupsController < ApplicationController end end + def update + myparams = programming_group_params || {} + @members = @programming_group.programming_group_memberships.includes(:user) + myparams[:users] = @members.where(id: myparams&.fetch(:programming_group_membership_ids, [])&.compact_blank).map(&:user) + update_and_respond(object: @programming_group, params: myparams) + end + + def destroy + session.delete(:pg_id) if current_contributor == @programming_group + destroy_and_respond(object: @programming_group) + end + private def authorize! - authorize(@programming_group) + authorize(@programming_group || @programming_groups) end def programming_group_params - params.require(:programming_group).permit(:programming_partner_ids) + params.require(:programming_group).permit(:programming_partner_ids, programming_group_membership_ids: []) if params[:programming_group].present? end def set_exercise_and_authorize @@ -80,6 +106,11 @@ class ProgrammingGroupsController < ApplicationController authorize(@exercise, :implement?) end + def set_programming_group_and_authorize + @programming_group = ProgrammingGroup.find(params[:id]) + authorize! + end + def redirect_to_exercise skip_authorization redirect_to(implement_exercise_path(@exercise), diff --git a/app/helpers/statistics_helper.rb b/app/helpers/statistics_helper.rb index 7fe1421b..36741430 100644 --- a/app/helpers/statistics_helper.rb +++ b/app/helpers/statistics_helper.rb @@ -44,6 +44,7 @@ module StatisticsHelper key: 'programming_groups', name: t('activerecord.models.programming_group.other'), data: ProgrammingGroup.count, + url: programming_groups_path, }, { key: 'currently_active', diff --git a/app/models/exercise.rb b/app/models/exercise.rb index d9dd27a7..2fc64cdd 100644 --- a/app/models/exercise.rb +++ b/app/models/exercise.rb @@ -36,6 +36,7 @@ class Exercise < ApplicationRecord has_many :request_for_comments scope :with_submissions, -> { where('id IN (SELECT exercise_id FROM submissions)') } + scope :with_programming_groups, -> { where('id IN (SELECT exercise_id FROM programming_groups)') } validate :valid_main_file? validate :valid_submission_deadlines? @@ -612,7 +613,7 @@ class Exercise < ApplicationRecord end def self.ransackable_attributes(_auth_object = nil) - %w[title internal_title] + %w[title id internal_title] end def self.ransackable_associations(_auth_object = nil) diff --git a/app/models/programming_group.rb b/app/models/programming_group.rb index e4d13ae6..fb530b40 100644 --- a/app/models/programming_group.rb +++ b/app/models/programming_group.rb @@ -9,10 +9,10 @@ class ProgrammingGroup < ApplicationRecord has_many :internal_users, through: :programming_group_memberships, source_type: 'InternalUser', source: :user has_many :testruns, through: :submissions has_many :runners, as: :contributor, dependent: :destroy - has_many :events - has_many :events_synchronized_editor, class_name: 'Event::SynchronizedEditor' - has_many :pair_programming_exercise_feedbacks - has_many :pair_programming_waiting_users + has_many :events, dependent: :destroy + has_many :events_synchronized_editor, class_name: 'Event::SynchronizedEditor', dependent: :destroy + has_many :pair_programming_exercise_feedbacks, dependent: :destroy + has_many :pair_programming_waiting_users, dependent: :destroy belongs_to :exercise validate :min_group_size @@ -20,11 +20,6 @@ class ProgrammingGroup < ApplicationRecord validate :no_erroneous_users accepts_nested_attributes_for :programming_group_memberships - def initialize(attributes = nil) - @erroneous_users = [] - super - end - def external_user? false end @@ -76,15 +71,27 @@ class ProgrammingGroup < ApplicationRecord def users=(users) self.internal_users = [] self.external_users = [] - users.each do |user| - next @erroneous_users << user unless user.is_a?(User) + users&.each do |user| + next erroneous_users << user unless user.is_a?(User) add(user) end end + def self.ransackable_associations(_auth_object = nil) + %w[exercise programming_group_memberships] + end + + def self.ransortable_attributes(_auth_object = nil) + %w[id created_at] + end + private + def erroneous_users + @erroneous_users ||= [] + end + def min_group_size if users.size < 2 errors.add(:base, :size_too_small) @@ -98,7 +105,7 @@ class ProgrammingGroup < ApplicationRecord end def no_erroneous_users - @erroneous_users.each do |partner_id| + erroneous_users.each do |partner_id| errors.add(:base, :invalid_partner_id, partner_id:) end end diff --git a/app/models/programming_group_membership.rb b/app/models/programming_group_membership.rb index 975c2550..eec727b8 100644 --- a/app/models/programming_group_membership.rb +++ b/app/models/programming_group_membership.rb @@ -12,4 +12,8 @@ class ProgrammingGroupMembership < ApplicationRecord errors.add(:base, :already_exists, id_with_type: user.id_with_type) end end + + def self.ransackable_associations(_auth_object = nil) + %w[user] + end end diff --git a/app/models/user.rb b/app/models/user.rb index 6d08f9cb..ba2dfdd9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -111,9 +111,9 @@ class User < ApplicationRecord def self.ransackable_attributes(auth_object) if auth_object.present? && auth_object.admin? - %w[name email external_id consumer_id platform_admin] + %w[name email external_id consumer_id platform_admin id] else - %w[name external_id] + %w[name external_id id] end end end diff --git a/app/policies/exercise_policy.rb b/app/policies/exercise_policy.rb index bf419dd3..cb8338dc 100644 --- a/app/policies/exercise_policy.rb +++ b/app/policies/exercise_policy.rb @@ -37,6 +37,10 @@ class ExercisePolicy < AdminOrAuthorPolicy end end + def programming_groups_for_exercise? + admin? + end + def submit? everyone && @record.teacher_defined_assessment? end diff --git a/app/policies/programming_group_policy.rb b/app/policies/programming_group_policy.rb index aea09c55..540c2607 100644 --- a/app/policies/programming_group_policy.rb +++ b/app/policies/programming_group_policy.rb @@ -1,11 +1,11 @@ # frozen_string_literal: true -class ProgrammingGroupPolicy < ApplicationPolicy - def new? +class ProgrammingGroupPolicy < AdminOnlyPolicy + def create? everyone end - def create? + def new? everyone end diff --git a/app/views/application/_navigation.html.slim b/app/views/application/_navigation.html.slim index 831db2bc..926cf087 100644 --- a/app/views/application/_navigation.html.slim +++ b/app/views/application/_navigation.html.slim @@ -12,7 +12,7 @@ li.dropdown-divider role='separator' = render('navigation_submenu', title: t('activerecord.models.exercise.other'), models: [Exercise, ExerciseCollection, ProxyExercise, Tag, Tip, Submission], link: exercises_path, cached: true) - = render('navigation_submenu', title: t('navigation.sections.users'), models: [InternalUser, ExternalUser], + = render('navigation_submenu', title: t('navigation.sections.contributors'), models: [InternalUser, ExternalUser, ProgrammingGroup], cached: true) = render('navigation_collection_link', model: StudyGroup, cached: true) = render('navigation_collection_link', model: ExecutionEnvironment, cached: true) diff --git a/app/views/exercises/index.html.slim b/app/views/exercises/index.html.slim index 6cb35f75..e42503ea 100644 --- a/app/views/exercises/index.html.slim +++ b/app/views/exercises/index.html.slim @@ -50,6 +50,7 @@ h1 = Exercise.model_name.human(count: 2) li = link_to(t('shared.show'), exercise, 'data-turbolinks' => "false", class: 'dropdown-item') if policy(exercise).show? li = link_to(t('activerecord.models.user_exercise_feedback.other'), feedback_exercise_path(exercise), class: 'dropdown-item') if policy(exercise).feedback? li = link_to(t('activerecord.models.request_for_comment.other'), rfcs_for_exercise_path(exercise), class: 'dropdown-item') if policy(exercise).rfcs_for_exercise? + li = link_to(t('activerecord.models.programming_group.other'), exercise_programming_groups_path(exercise), class: 'dropdown-item') if policy(exercise).programming_groups_for_exercise? li = link_to(t('shared.destroy'), exercise, data: {confirm: t('shared.confirm_destroy')}, method: :delete, class: 'dropdown-item') if policy(exercise).destroy? li = link_to(t('.clone'), clone_exercise_path(exercise), data: {confirm: t('shared.confirm_destroy')}, method: :post, class: 'dropdown-item') if policy(exercise).clone? li = link_to(t('exercises.export_codeharbor.label'), '', class: 'dropdown-item export-start', data: {'exercise-id' => exercise.id}) if policy(exercise).export_external_confirm? diff --git a/app/views/exercises/show.html.slim b/app/views/exercises/show.html.slim index ce05ac87..0f6e486d 100644 --- a/app/views/exercises/show.html.slim +++ b/app/views/exercises/show.html.slim @@ -19,6 +19,7 @@ h1.d-inline-block li = link_to(t('shared.statistics'), statistics_exercise_path(@exercise), 'data-turbolinks' => "false", class: 'dropdown-item') if policy(@exercise).statistics? li = link_to(t('activerecord.models.user_exercise_feedback.other'), feedback_exercise_path(@exercise), class: 'dropdown-item') if policy(@exercise).feedback? li = link_to(t('activerecord.models.request_for_comment.other'), rfcs_for_exercise_path(@exercise), class: 'dropdown-item') if policy(@exercise).rfcs_for_exercise? + li = link_to(t('activerecord.models.programming_group.other'), exercise_programming_groups_path(@exercise), class: 'dropdown-item') if policy(@exercise).programming_groups_for_exercise? li = link_to(t('shared.destroy'), @exercise, data: {confirm: t('shared.confirm_destroy')}, method: :delete, class: 'dropdown-item') if policy(@exercise).destroy? li = link_to(t('exercises.index.clone'), clone_exercise_path(@exercise), data: {confirm: t('shared.confirm_destroy')}, method: :post, class: 'dropdown-item') if policy(@exercise).clone? li = link_to(t('exercises.export_codeharbor.label'), '', class: 'dropdown-item export-start', data: {'exercise-id' => @exercise.id}) if policy(@exercise).export_external_confirm? diff --git a/app/views/programming_groups/_form_edit.html.slim b/app/views/programming_groups/_form_edit.html.slim new file mode 100644 index 00000000..0bb36d5e --- /dev/null +++ b/app/views/programming_groups/_form_edit.html.slim @@ -0,0 +1,16 @@ += form_for(@programming_group) do |f| + = render('shared/form_errors', object: @programming_group) + + h3 = t('activerecord.attributes.programming_group.member') + .table-responsive + table.table class="#{@members.present? ? 'sortable' : ''}" + thead + tr + th = t('activerecord.attributes.exercise.selection') + th = t('navigation.sections.contributors') + = collection_check_boxes :programming_group, :programming_group_membership_ids, @members, :id, :id do |b| + tr + td = b.check_box class: 'form-check-input' + td = link_to_if(policy(b.object.user).show?, b.object.user.displayname, b.object.user) + + .actions = render('shared/submit_button', f: f, object: @programming_group) diff --git a/app/views/programming_groups/edit.html.slim b/app/views/programming_groups/edit.html.slim new file mode 100644 index 00000000..0982e637 --- /dev/null +++ b/app/views/programming_groups/edit.html.slim @@ -0,0 +1,3 @@ +h1 = @programming_group + += render('form_edit') diff --git a/app/views/programming_groups/index.html.slim b/app/views/programming_groups/index.html.slim new file mode 100644 index 00000000..a94e9e36 --- /dev/null +++ b/app/views/programming_groups/index.html.slim @@ -0,0 +1,43 @@ +- if params[:exercise_id].nil? + h1 = ProgrammingGroup.model_name.human(count: 2) + = render(layout: 'shared/form_filters') do |f| + .col-auto + = f.label(:exercise_id_eq, t('activerecord.attributes.programming_group.exercise'), class: 'visually-hidden form-label') + = f.collection_select(:exercise_id_eq, Exercise.with_programming_groups, :id, :title, class: 'form-control', prompt: t('activerecord.attributes.programming_group.exercise')) + .col-auto + = f.label(:programming_group_memberships_user_of_ExternalUser_type_id_eq, t('activerecord.attributes.programming_group.external_user_id'), class: 'visually-hidden form-label') + = f.search_field(:programming_group_memberships_user_of_ExternalUser_type_id_eq, class: 'form-control', placeholder: t('activerecord.attributes.programming_group.external_user_id')) + .col-auto + = f.label(:programming_group_memberships_user_of_InternalUser_type_id_eq, t('activerecord.attributes.programming_group.internal_user_id'), class: 'visually-hidden form-label') + = f.search_field(:programming_group_memberships_user_of_InternalUser_type_id_eq, class: 'form-control', placeholder: t('activerecord.attributes.programming_group.internal_user_id')) +- else + h1 = "#{ProgrammingGroup.model_name.human(count: 2)} for Exercise '#{@exercise.title}'" + +.table-responsive + table.table.mt-4 class="#{@programming_groups.present? ? 'sortable' : ''}" + thead + tr + th.sortable_nosort = sort_link(@search, :id, t('activerecord.attributes.programming_group.name')) + - if params[:exercise_id].blank? + th.sorttable_nosort = sort_link(@search, :exercise_id, t('activerecord.attributes.programming_group.exercise')) + th = t('activerecord.attributes.programming_group.member') + th = t('activerecord.attributes.programming_group.member_count') + th.sorttable_nosort = sort_link(@search, :created_at, t('shared.created_at')) + th colspan=3 = t('shared.actions') + tbody + - if params[:exercise_id].nil? + - filtered_programming_groups = @programming_groups + - else + - filtered_programming_groups = @programming_groups.where(exercise_id: params[:exercise_id]) + - filtered_programming_groups.each do |programming_group| + tr + td = link_to_if(policy(programming_group).show?, programming_group.displayname, programming_group) + - if params[:exercise_id].blank? + td = link_to_if(policy(programming_group.exercise).show?, programming_group.exercise.title, programming_group.exercise, 'data-turbolinks' => "false") + td == programming_group.users.map { |user| link_to_if(policy(user).show?, user.name, user) }.join(', ') + td = programming_group.users.size + td = l(programming_group.created_at, format: :short) + td = link_to(t('shared.show'), programming_group) if policy(programming_group).show? + td = link_to(t('shared.edit'), edit_programming_group_path(programming_group)) if policy(programming_group).edit? + td = link_to(t('shared.destroy'), programming_group, data: { confirm: t('shared.confirm_destroy') }, method: :delete) if policy(programming_group).destroy? += render('shared/pagination', collection: @programming_groups) diff --git a/app/views/programming_groups/show.html.slim b/app/views/programming_groups/show.html.slim new file mode 100644 index 00000000..cdeb47bc --- /dev/null +++ b/app/views/programming_groups/show.html.slim @@ -0,0 +1,19 @@ +h1 + = @programming_group + - if policy(@programming_group).edit? + = render('shared/edit_button', object: @programming_group) + += row(label: 'activerecord.attributes.programming_group.name', value: @programming_group.displayname) += row(label: 'activerecord.attributes.programming_group.exercise', value: link_to_if(policy(@programming_group.exercise).show?, @programming_group.exercise.title, @programming_group.exercise)) += row(label: 'activerecord.attributes.programming_group.member_count', value: @programming_group.users.size) += row(label: 'shared.created_at', value: l(@programming_group.created_at, format: :short)) + +h2.mt-4 = t('activerecord.attributes.study_group.members') +.table-responsive + table.table class="#{@programming_group.users.present? ? 'sortable' : ''}" + thead + tr + th = t('navigation.sections.contributors') + - @programming_group.users.each do |user| + tr + td = link_to_if(policy(user).show?, user.displayname, user) diff --git a/app/views/submissions/index.html.slim b/app/views/submissions/index.html.slim index c54b9da9..b5df6709 100644 --- a/app/views/submissions/index.html.slim +++ b/app/views/submissions/index.html.slim @@ -22,7 +22,7 @@ h1 = Submission.model_name.human(count: 2) - @submissions.each do |submission| tr td = link_to_if(submission.exercise && policy(submission.exercise).show?, submission.exercise, submission.exercise) - td = link_to_if(submission.contributor.is_a?(User) && policy(submission.contributor).show?, submission.contributor, submission.contributor) + td = link_to_if(policy(submission.contributor).show?, submission.contributor, submission.contributor) td = t("submissions.causes.#{submission.cause}") td = submission.score td = l(submission.created_at, format: :short) diff --git a/app/views/submissions/show.html.slim b/app/views/submissions/show.html.slim index 762ff71e..7109046a 100644 --- a/app/views/submissions/show.html.slim +++ b/app/views/submissions/show.html.slim @@ -8,7 +8,7 @@ h1 = @submission = row(label: 'submission.exercise', value: link_to_if(policy(@submission.exercise).show?, @submission.exercise, @submission.exercise)) -= row(label: 'submission.contributor', value: link_to_if(@submission.contributor.is_a?(User) && policy(@submission.contributor).show?, @submission.contributor, @submission.contributor)) += row(label: 'submission.contributor', value: link_to_if(policy(@submission.contributor).show?, @submission.contributor, @submission.contributor)) = row(label: 'submission.study_group', value: link_to_if(@submission.study_group.present? && policy(@submission.study_group).show?, @submission.study_group, @submission.study_group)) = row(label: 'submission.cause', value: t("submissions.causes.#{@submission.cause}")) = row(label: 'submission.score', value: @submission.score) diff --git a/config/locales/de.yml b/config/locales/de.yml index ee846bf3..dcc63768 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -58,6 +58,12 @@ de: uuid: UUID unpublished: Deaktiviert programming_group: + exercise: Aufgabe + external_user_id: Externe Nutzer-ID + internal_user_id: Interne Nutzer-ID + member: Mitglieder + member_count: Anzahl der Mitglieder + name: Name programming_partner_ids: Nutzer-ID der Programmierpartnerin / des Programmierpartners programming_group/programming_group_memberships: base: Programmiergruppenmitgliedschaft @@ -599,10 +605,10 @@ de: enter_partner_id: "Kennen Sie eine Person in dem Kurs, mit der Sie gemeinsam die Aufgabe lösen möchten? Dann geben Sie hier die Nutzer-ID dieser Person ein." find_partner_title: "Eine:n Programmierpartner:in für die Aufgabe finden" find_partner_description: "Wenn Sie keine Person aus dem Kurs kennen, haben Sie die Möglichkeit mit einer anderen Person gepaart zu werden. Sie werden dann zur Aufgabe weitergeleitet, sobald eine andere Person ebenfalls diese Aufgabe im Team lösen möchte." + find_programming_partner: Programmierpartner:in finden info_pair_programming: "Pair Programming (Programmieren in Paaren) ist eine Methode, bei der zwei Personen gemeinsam programmieren. Dabei übernehmen sie abwechselnd zwei verschiedene Rollen: Den Driver, der den Code schreibt und sich auf die Details fokussiert und den Navigator, der Tippfehler korrigiert, die Aufgabenstellung im Blick behält und Verbesserungsideen vorschlägt. Kommunikation miteinander ist von entscheidender Bedeutung für erfolgreiches Pair Programming." info_forced_work_together: "Sie können die Aufgabe '%{exercise_title}' nur gemeinsam mit einer anderen Person lösen. Ihr:e Teampartner:in kann sehen, was Sie in dieser Aufgabe schreiben und umgekehrt. Für das Lösen der Aufgabe erhalten Sie Bonuspunkte." info_work_together: "Sie haben die Möglichkeit, die Aufgabe '%{exercise_title}' zusammen mit einer anderen Person zu lösen. Ihr:e Teampartner:in kann sehen, was Sie in dieser Aufgabe schreiben und umgekehrt. Beachten Sie dabei, dass anschließend keiner die Zusammenarbeit beenden kann. Für die nächste Aufgabe können Sie sich erneuert entscheiden, ob und mit wem Sie zusammen arbeiten möchten." - find_programming_partner: Programmierpartner:in finden own_user_id: "Ihre Nutzer-ID:" pair_programming_info: Pair Programming Info work_alone: "Alleine arbeiten" @@ -1044,10 +1050,11 @@ de: update: "Aktualisieren" navigation: sections: + contributors: "Mitwirkende" errors: "Fehler" files: "Dateien" - users: "Benutzer" integrations: "Integrationen" + users: "Nutzer" exercise_collections: form: add_exercises: "Aufgaben hinzufügen" diff --git a/config/locales/en.yml b/config/locales/en.yml index 8eb0d5fd..a42d0ae7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -58,6 +58,12 @@ en: uuid: UUID unpublished: Unpublished programming_group: + exercise: Exercise + external_user_id: External User ID + internal_user_id: Internal User ID + member: Member + member_count: Member Count + name: Name programming_partner_ids: Programming Partner ID programming_group/programming_group_memberships: base: Programming Group Membership @@ -599,10 +605,10 @@ en: enter_partner_id: "Do you know a person in the course with whom you would like to solve the task together? Then enter that person's user ID here." find_partner_title: "Find a programming partner for the exercise" find_partner_description: "If you don't know a person from the course, you have the possibility to be paired with another person. Then, you will be redirected to the task as soon as another person also wants to solve this task in a team." + find_programming_partner: Find Programming Partner info_pair_programming: "Pair Programming is a method where two people program together. They alternate between two distinct roles: the Driver, responsible for writing the code and focusing on the details, and the Navigator, tasked with correcting typos, overseeing the task's progress, and offering suggestions for improvement. Effective communication in the pair is crucial for the success of pair programming." info_forced_work_together: "You can solve the exercise '%{exercise_title}' only together with another person. Your team partner can see what you write in this exercise and vice versa. You will get bonus points for solving the exercise." info_work_together: "You have the possibility to solve the task '%{exercise_title}' together with another person. Your team partner can see what you write in this task and vice versa. Note that no one can stop the collaboration afterwards. For the next task you can decide again if and with whom you want to work together." - find_programming_partner: Find Programming Partner own_user_id: "Your user ID:" pair_programming_info: Pair Programming Info work_alone: Work Alone @@ -1044,10 +1050,11 @@ en: update: "Update" navigation: sections: + contributors: "Contributors" errors: "Errors" files: "Files" - users: "Users" integrations: "Integrations" + users: "Users" exercise_collections: form: add_exercises: "Add exercises" diff --git a/config/routes.rb b/config/routes.rb index 384c60d7..bf1b39e1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -97,9 +97,11 @@ Rails.application.routes.draw do post :export_external_confirm end - resources :programming_groups, only: %i[new create] + resources :programming_groups end + resources :programming_groups, except: %i[new create] + resources :exercise_collections do member do get :statistics diff --git a/spec/controllers/programming_groups_controller_spec.rb b/spec/controllers/programming_groups_controller_spec.rb new file mode 100644 index 00000000..015f94fe --- /dev/null +++ b/spec/controllers/programming_groups_controller_spec.rb @@ -0,0 +1,345 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ProgrammingGroupsController do + render_views + + let(:user) { create(:admin) } + let(:other_user) { create(:external_user) } + let(:exercise) { create(:math) } + let(:exercise_id) { exercise.id } + let(:programming_group) { create(:programming_group, exercise:, users: [user, other_user]) } + + before { allow(controller).to receive(:current_user).and_return(user) } + + describe 'POST #create' do + let(:user_to_params) { ->(users) { users.map(&:id_with_type).join(', ') } } + let(:perform_request) { proc { post :create, params: {exercise_id:, programming_group: pg_params} } } + + context 'with a valid programming group' do + let(:pg_params) { {programming_partner_ids: user_to_params.call([other_user])} } + + context 'when the request is performed' do + before { perform_request.call } + + expect_assigns(exercise: :exercise, programming_group: ProgrammingGroup) + expect_redirect(ProgrammingGroup.last) + end + + it 'creates the programming group' do + expect { perform_request.call }.to change(ProgrammingGroup, :count).by(1).and change(ProgrammingGroupMembership, :count).by(2) + end + + it 'does not create a new event' do + expect { perform_request.call }.not_to change(Event, :count) + end + + it 'updates the status of the initiating waiting user' do + pp_waiting_user = PairProgrammingWaitingUser.create(user:, exercise:, status: :waiting) + expect { perform_request.call && pp_waiting_user.reload }.to change(pp_waiting_user, :status).from('waiting').to('created_pg') + end + + it 'updates the status of other waiting users' do + pp_waiting_user = PairProgrammingWaitingUser.create(user: other_user, exercise:, status: :waiting) + expect { perform_request.call && pp_waiting_user.reload }.to change(pp_waiting_user, :status).from('waiting').to('invited_to_pg') + end + + it 'stores the programming group ID in the session' do + allow(controller.session).to receive(:[]=).and_call_original + perform_request.call + expect(controller.session).to have_received(:[]=).with(:pg_id, ProgrammingGroup.last.id) + end + end + + context 'with an invalid programming group' do + let(:pg_params) { {} } + + before { post :create, params: {exercise_id:, programming_group: pg_params} } + + expect_assigns(exercise: :exercise, programming_group: ProgrammingGroup) + expect_http_status(:ok) + expect_template(:new) + + it 'does not create a new programming group' do + expect { perform_request.call }.not_to change(ProgrammingGroup, :count) + end + + it 'creates a new event' do + expect { perform_request.call }.to change(Event, :count).by(1) + end + + it 'does not update the status of the initiating waiting user' do + pp_waiting_user = PairProgrammingWaitingUser.create(user:, exercise:, status: :waiting) + expect { perform_request.call && pp_waiting_user.reload }.not_to change(pp_waiting_user, :status) + end + + it 'does not update the status of other waiting users' do + pp_waiting_user = PairProgrammingWaitingUser.create(user: other_user, exercise:, status: :waiting) + expect { perform_request.call && pp_waiting_user.reload }.not_to change(pp_waiting_user, :status) + end + end + + context 'with a duplicated membership' do + let(:pg_params) { {programming_partner_ids: user_to_params.call(programming_group.users)} } + + before { programming_group.save! } + + context 'when the request is performed' do + before { perform_request.call } + + expect_assigns(exercise: :exercise, programming_group: ProgrammingGroup) + expect_http_status(:ok) + expect_template(:new) + end + + it 'does not create a new programming group' do + expect { perform_request.call }.not_to change(ProgrammingGroup, :count) + end + + it 'creates a new event' do + expect { perform_request.call }.to change(Event, :count).by(1) + end + + it 'does not update the status of the initiating waiting user' do + pp_waiting_user = PairProgrammingWaitingUser.create(user:, exercise:, status: :waiting) + expect { perform_request.call && pp_waiting_user.reload }.not_to change(pp_waiting_user, :status) + end + + it 'does not update the status of other waiting users' do + pp_waiting_user = PairProgrammingWaitingUser.create(user: other_user, exercise:, status: :waiting) + expect { perform_request.call && pp_waiting_user.reload }.not_to change(pp_waiting_user, :status) + end + end + + context 'with a user providing their own ID' do + let(:pg_params) { {programming_partner_ids: user_to_params.call([user, other_user])} } + + context 'when the request is performed' do + before { perform_request.call } + + expect_assigns(exercise: :exercise, programming_group: ProgrammingGroup) + expect_redirect(ProgrammingGroup.last) + end + + it 'creates a new programming group' do + expect { perform_request.call }.to change(ProgrammingGroup, :count).by(1).and change(ProgrammingGroupMembership, :count).by(2) + end + + it 'does not create a new event' do + expect { perform_request.call }.not_to change(Event, :count) + end + + it 'updates the status of the initiating waiting user' do + pp_waiting_user = PairProgrammingWaitingUser.create(user:, exercise:, status: :waiting) + expect { perform_request.call && pp_waiting_user.reload }.to change(pp_waiting_user, :status).from('waiting').to('created_pg') + end + + it 'updates the status of other waiting users' do + pp_waiting_user = PairProgrammingWaitingUser.create(user: other_user, exercise:, status: :waiting) + expect { perform_request.call && pp_waiting_user.reload }.to change(pp_waiting_user, :status).from('waiting').to('invited_to_pg') + end + + it 'stores the programming group ID in the session' do + allow(controller.session).to receive(:[]=).and_call_original + perform_request.call + expect(controller.session).to have_received(:[]=).with(:pg_id, ProgrammingGroup.last.id) + end + end + + context 'with invalid programming partner IDs' do + let(:pg_params) { {programming_partner_ids: 'test1234'} } + + before { post :create, params: {exercise_id:, programming_group: pg_params} } + + expect_assigns(exercise: :exercise, programming_group: ProgrammingGroup) + expect_http_status(:ok) + expect_template(:new) + + it 'does not create a new programming group' do + expect { perform_request.call }.not_to change(ProgrammingGroup, :count) + end + + it 'creates a new event' do + expect { perform_request.call }.to change(Event, :count).by(1) + end + end + + context 'with too many users' do + let(:third_user) { create(:external_user) } + let(:pg_params) { {programming_partner_ids: user_to_params.call([other_user, third_user])} } + + before { post :create, params: {exercise_id:, programming_group: pg_params} } + + expect_assigns(exercise: :exercise, programming_group: ProgrammingGroup) + expect_http_status(:ok) + expect_template(:new) + + it 'does not create a new programming group' do + expect { perform_request.call }.not_to change(ProgrammingGroup, :count) + end + + it 'creates a new event' do + expect { perform_request.call }.to change(Event, :count).by(1) + end + end + end + + describe 'DELETE #destroy' do + before { delete :destroy, params: {id: programming_group.id} } + + expect_assigns(programming_group: ProgrammingGroup) + + it 'destroys the programming group' do + programming_group = create(:programming_group) + expect { delete :destroy, params: {id: programming_group.id} }.to change(ProgrammingGroup, :count).by(-1) + end + + it 'removes the programming group ID from the session' do + # Setup: Construct a programming group and set it as the current group in the session. + programming_group = create(:programming_group, users: [user, other_user]) + allow(controller.session).to receive(:[]).and_call_original + allow(controller.session).to receive(:[]).with(:pg_id).and_return programming_group.id + + # Test: Destroy the programming group and verify that it is no longer retained in the session. + allow(controller.session).to receive(:delete).and_call_original + delete :destroy, params: {id: programming_group.id} + expect(controller.session).to have_received(:delete).with(:pg_id) + end + + expect_redirect(:programming_groups) + end + + describe 'GET #edit' do + before { get :edit, params: {id: programming_group.id} } + + expect_assigns(programming_group: ProgrammingGroup) + expect_http_status(:ok) + expect_template(:edit) + end + + describe 'GET #index' do + before do + create_pair(:programming_group) + get :index + end + + expect_assigns(programming_groups: ProgrammingGroup.all) + expect_http_status(:ok) + expect_template(:index) + end + + describe 'GET #new' do + let(:perform_request) { proc { get :new, params: {exercise_id:} } } + + context 'when the request is performed' do + before { perform_request.call } + + expect_assigns(programming_group: ProgrammingGroup) + expect_http_status(:ok) + expect_template(:new) + end + + it 'creates a new event' do + expect { perform_request.call }.to change(Event, :count).by(1) + end + + context 'with an existing programming group' do + let(:programming_group) { create(:programming_group, exercise:, users: [user, other_user]) } + + before { programming_group.save! } + + context 'when the request is performed' do + before { perform_request.call } + + expect_redirect { implement_exercise_path(exercise) } + + it 'stores the programming group ID in the session' do + allow(controller.session).to receive(:[]=).and_call_original + perform_request.call + expect(controller.session).to have_received(:[]=).with(:pg_id, programming_group.id) + end + end + + context 'when the user has already started the exercise' do + before { create(:submission, exercise:, user:) } + + context 'when the request is performed' do + before { perform_request.call } + + expect_redirect { implement_exercise_path(exercise) } + + it 'does not store the programming group ID in the session' do + allow(controller.session).to receive(:[]=).and_call_original + perform_request.call + expect(controller.session).not_to have_received(:[]=).with(:pg_id) + end + end + end + end + end + + describe 'GET #show' do + before { get :show, params: {id: programming_group.id} } + + expect_assigns(programming_group: :programming_group) + expect_http_status(:ok) + expect_template(:show) + end + + describe 'PUT #update' do + let(:perform_request) { proc { put :update, params: {programming_group: pg_params, id: programming_group.id} } } + + before do + # In order to test a successful update, we need to remove a user from the programming group. + # Since otherwise the group size is fixed to exactly two members, we temporarily allow a larger group size. + allow_any_instance_of(ProgrammingGroup).to receive(:max_group_size).and_return(true) + # The programming group needs to be saved, otherwise we cannot attempt to update it. + programming_group.save! + end + + context 'with a valid programming group' do + let(:programming_group) { create(:programming_group, exercise:, users: create_list(:external_user, 3)) } + let(:pg_params) { {programming_group_membership_ids: programming_group.programming_group_memberships.map(&:id)[0..1]} } + + context 'when the request is performed' do + before { perform_request.call } + + expect_assigns(programming_group: ProgrammingGroup) + expect_redirect(:programming_group) + end + + it 'does not update the programming group' do + expect { perform_request.call }.not_to change(ProgrammingGroup, :count) + end + + it 'removes the desired programming group membership' do + expect { perform_request.call }.to change(ProgrammingGroupMembership, :count).by(-1) + end + + it 'does not update any existing programming group membership' do + expect { perform_request.call && programming_group.programming_group_memberships.first.reload }.not_to change(programming_group.programming_group_memberships.first, :updated_at) + end + end + + context 'with an invalid programming group' do + let(:pg_params) { {programming_group_membership_ids: []} } + + context 'when the request is performed' do + before { perform_request.call } + + expect_assigns(programming_group: ProgrammingGroup) + expect_http_status(:ok) + expect_template(:edit) + end + + it 'does not update the programming group' do + expect { perform_request.call }.not_to change(ProgrammingGroup, :count) + end + + it 'does not update the programming group memberships' do + expect { perform_request.call }.not_to change(ProgrammingGroupMembership, :count) + end + end + end +end diff --git a/spec/policies/exercise_policy_spec.rb b/spec/policies/exercise_policy_spec.rb index 71d8db70..62f9f163 100644 --- a/spec/policies/exercise_policy_spec.rb +++ b/spec/policies/exercise_policy_spec.rb @@ -7,11 +7,13 @@ RSpec.describe ExercisePolicy do let(:exercise) { build(:dummy, public: true) } - permissions :batch_update? do - it 'grants access to admins only' do - expect(policy).to permit(build(:admin), exercise) - %i[external_user teacher].each do |factory_name| - expect(policy).not_to permit(create(factory_name), exercise) + %i[batch_update? programming_groups_for_exercise?].each do |action| + permissions(action) do + it 'grants access to admins only' do + expect(policy).to permit(build(:admin), exercise) + %i[external_user teacher].each do |factory_name| + expect(policy).not_to permit(create(factory_name), exercise) + end end end end diff --git a/spec/policies/programming_group_policy_spec.rb b/spec/policies/programming_group_policy_spec.rb index 50a2cc6e..a75afbb8 100644 --- a/spec/policies/programming_group_policy_spec.rb +++ b/spec/policies/programming_group_policy_spec.rb @@ -7,6 +7,17 @@ RSpec.describe ProgrammingGroupPolicy do let(:programming_group) { build(:programming_group) } + %i[index? destroy? show? edit? update?].each do |action| + permissions(action) do + it 'grants access to admins only' do + expect(policy).to permit(create(:admin), programming_group) + %i[external_user teacher].each do |factory_name| + expect(policy).not_to permit(create(factory_name), programming_group) + end + end + end + end + %i[new? create?].each do |action| permissions(action) do it 'grants access to everyone' do