From 319c3ab3b47659c68e89d3ad0dc9eeac79f6dac7 Mon Sep 17 00:00:00 2001 From: kiragrammel Date: Thu, 10 Aug 2023 17:07:04 +0200 Subject: [PATCH] Add ProgrammingGroup & ProgrammingGroupMembership * User can create programming group with other users for exercise * Submission is shared in a group * Also adjust specs --- .rubocop/rails.yml | 5 + app/controllers/application_controller.rb | 9 + .../community_solutions_controller.rb | 2 +- app/controllers/concerns/file_parameters.rb | 2 +- app/controllers/concerns/lti.rb | 19 +- .../concerns/submission_parameters.rb | 3 +- app/controllers/exercises_controller.rb | 23 +- app/controllers/flowr_controller.rb | 2 +- .../programming_groups_controller.rb | 54 +++ .../remote_evaluation_controller.rb | 1 + .../user_exercise_feedbacks_controller.rb | 15 +- app/helpers/statistics_helper.rb | 17 +- app/models/exercise.rb | 33 +- app/models/programming_group.rb | 82 ++++ app/models/programming_group_membership.rb | 15 + app/models/submission.rb | 17 +- app/models/user.rb | 20 + app/policies/application_policy.rb | 7 + app/policies/programming_group_policy.rb | 11 + app/policies/submission_policy.rb | 2 +- .../statistics.html.slim | 2 +- .../exercise_collections/statistics.html.slim | 6 +- app/views/exercises/_editor_output.html.slim | 2 +- app/views/exercises/statistics.html.slim | 17 +- app/views/programming_groups/_form.html.slim | 7 + app/views/programming_groups/new.html.slim | 14 + app/views/submissions/index.html.slim | 2 +- app/views/submissions/show.html.slim | 2 +- config/locales/de.yml | 35 +- config/locales/en.yml | 35 +- config/routes.rb | 2 + ...0230710131250_create_programming_groups.rb | 11 + ...28_create_programming_group_memberships.rb | 12 + db/schema.rb | 19 + spec/concerns/file_parameters_spec.rb | 4 +- spec/concerns/lti_spec.rb | 1 + .../submissions_controller_spec.rb | 426 ++++++++++-------- spec/factories/programming_group.rb | 14 + spec/factories/submission.rb | 5 + .../policies/programming_group_policy_spec.rb | 19 + spec/policies/submission_policy_spec.rb | 12 +- .../exercises/implement.html.slim_spec.rb | 5 +- 42 files changed, 715 insertions(+), 276 deletions(-) create mode 100644 app/controllers/programming_groups_controller.rb create mode 100644 app/models/programming_group.rb create mode 100644 app/models/programming_group_membership.rb create mode 100644 app/policies/programming_group_policy.rb create mode 100644 app/views/programming_groups/_form.html.slim create mode 100644 app/views/programming_groups/new.html.slim create mode 100644 db/migrate/20230710131250_create_programming_groups.rb create mode 100644 db/migrate/20230710132428_create_programming_group_memberships.rb create mode 100644 spec/factories/programming_group.rb create mode 100644 spec/policies/programming_group_policy_spec.rb diff --git a/.rubocop/rails.yml b/.rubocop/rails.yml index dcee9f40..b6fa4720 100644 --- a/.rubocop/rails.yml +++ b/.rubocop/rails.yml @@ -20,3 +20,8 @@ Rails/UnknownEnv: Rails/I18nLazyLookup: Enabled: false + +Rails/DynamicFindBy: + Whitelist: + - find_by_sql # Default value for this cop + - find_by_id_with_type # custom method defined in the `User` model diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 717972e3..5b971045 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -23,6 +23,15 @@ class ApplicationController < ActionController::Base @current_user ||= find_or_login_current_user&.store_current_study_group_id(session[:study_group_id]) end + def current_contributor + @current_contributor ||= if session[:pg_id] + current_user.programming_groups.find(session[:pg_id]) + else + current_user + end + end + helper_method :current_contributor + def find_or_login_current_user login_from_authentication_token || login_from_lti_session || diff --git a/app/controllers/community_solutions_controller.rb b/app/controllers/community_solutions_controller.rb index f1936075..61c91c4c 100644 --- a/app/controllers/community_solutions_controller.rb +++ b/app/controllers/community_solutions_controller.rb @@ -100,6 +100,6 @@ class CommunitySolutionsController < ApplicationController def set_exercise_and_submission @exercise = @community_solution.exercise - @submission = current_user.submissions.final.where(exercise_id: @community_solution.exercise.id).order('created_at DESC').first + @submission = current_contributor.submissions.final.where(exercise: @community_solution.exercise).order(created_at: :desc).first end end diff --git a/app/controllers/concerns/file_parameters.rb b/app/controllers/concerns/file_parameters.rb index 9b52992b..7b519161 100644 --- a/app/controllers/concerns/file_parameters.rb +++ b/app/controllers/concerns/file_parameters.rb @@ -17,7 +17,7 @@ module FileParameters # avoid that public files from other contexts can be created # `next` is similar to an early return and will proceed with the next iteration of the loop next true if file.context_type == 'Exercise' && file.context_id != exercise.id - next true if file.context_type == 'Submission' && (file.context.contributor_id != current_user.id || file.context.contributor_type != current_user.class.name) + next true if file.context_type == 'Submission' && (file.context.contributor_id != current_contributor.id || file.context.contributor_type != current_contributor.class.name) next true if file.context_type == 'CommunitySolution' && controller_name != 'community_solutions' # Optimization: We already queried the ancestor file, let's reuse the object. diff --git a/app/controllers/concerns/lti.rb b/app/controllers/concerns/lti.rb index 37dd82a3..a863e560 100644 --- a/app/controllers/concerns/lti.rb +++ b/app/controllers/concerns/lti.rb @@ -21,18 +21,17 @@ module Lti # exercise_id.nil? ==> the user has logged out. All session data is to be destroyed # exercise_id.exists? ==> the user has submitted the results of an exercise to the consumer. # Only the lti_parameters are deleted. - def clear_lti_session_data(exercise_id = nil, _user_id = nil) + def clear_lti_session_data(exercise_id = nil) if exercise_id.nil? session.delete(:external_user_id) session.delete(:study_group_id) session.delete(:embed_options) - session.delete(:lti_exercise_id) - session.delete(:lti_parameters_id) end + session.delete(:pg_id) - # March 2022: We temporarily allow reusing the LTI credentials and don't remove them on purpose. + # We allow reusing the LTI credentials and don't remove them on purpose. # This allows users to jump between remote and web evaluation with the same behavior. - # LtiParameter.where(external_users_id: user_id, exercises_id: exercise_id).destroy_all + # Also it prevents the user from deleting the lti_parameters for their programming team members. end private :clear_lti_session_data @@ -159,20 +158,20 @@ module Lti session: session.to_hash, exercise_id: submission.exercise_id, }) - normalized_lit_score = submission.normalized_score + normalized_lti_score = submission.normalized_score if submission.before_deadline? # Keep the full score elsif submission.within_grace_period? # Reduce score by 20% - normalized_lit_score *= 0.8 + normalized_lti_score *= 0.8 elsif submission.after_late_deadline? # Reduce score by 100% - normalized_lit_score *= 0.0 + normalized_lti_score *= 0.0 end begin - response = provider.post_replace_result!(normalized_lit_score) - {code: response.response_code, message: response.post_response.body, status: response.code_major, score_sent: normalized_lit_score} + response = provider.post_replace_result!(normalized_lti_score) + {code: response.response_code, message: response.post_response.body, status: response.code_major, score_sent: normalized_lti_score} rescue IMS::LTI::XMLParseError, Net::OpenTimeout, Net::ReadTimeout, Errno::ECONNRESET, SocketError # A parsing error might happen if the LTI provider is down and doesn't return a valid XML response {status: 'error'} diff --git a/app/controllers/concerns/submission_parameters.rb b/app/controllers/concerns/submission_parameters.rb index b513dbc0..12de8b6b 100644 --- a/app/controllers/concerns/submission_parameters.rb +++ b/app/controllers/concerns/submission_parameters.rb @@ -22,8 +22,7 @@ module SubmissionParameters def merge_user(params) # The study_group_id might not be present in the session (e.g. for internal users), resulting in session[:study_group_id] = nil which is intended. params.merge( - contributor_id: current_user.id, - contributor_type: current_user.class.name, + contributor: current_contributor, study_group_id: current_user.current_study_group_id ) end diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index e1767ea0..3aa13c9f 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -92,7 +92,7 @@ class ExercisesController < ApplicationController authorize! @feedbacks = @exercise.user_exercise_feedbacks.paginate(page: params[:page], per_page: per_page_param) @submissions = @feedbacks.map do |feedback| - feedback.exercise.final_submission(feedback.user) + feedback.exercise.final_submission(feedback.user.programming_groups.where(exercise: @exercise).presence || feedback.user) end end @@ -298,7 +298,7 @@ class ExercisesController < ApplicationController private :update_exercise_tips def implement - user_solved_exercise = @exercise.solved_by?(current_user) + user_solved_exercise = @exercise.solved_by?(current_contributor) count_interventions_today = UserExerciseIntervention.where(user: current_user).where('created_at >= ?', Time.zone.now.beginning_of_day).count user_got_intervention_in_exercise = UserExerciseIntervention.where(user: current_user, @@ -330,7 +330,7 @@ class ExercisesController < ApplicationController @hide_rfc_button = @embed_options[:disable_rfc] - @submission = current_user.submissions.where(exercise_id: @exercise.id).order('created_at DESC').first + @submission = current_contributor.submissions.order(created_at: :desc).find_by(exercise: @exercise) @files = (@submission ? @submission.collect_files : @exercise.files).select(&:visible).sort_by(&:filepath) @paths = collect_paths(@files) end @@ -363,7 +363,7 @@ class ExercisesController < ApplicationController private :set_available_tips def working_times - working_time_accumulated = @exercise.accumulated_working_time_for_only(current_user) + working_time_accumulated = @exercise.accumulated_working_time_for_only(current_contributor) working_time_75_percentile = @exercise.get_quantiles([0.75]).first render(json: {working_time_75_percentile:, working_time_accumulated:}) @@ -375,8 +375,8 @@ class ExercisesController < ApplicationController render(json: {success: 'false', error: "undefined intervention #{params[:intervention_type]}"}) else uei = UserExerciseIntervention.new( - user: current_user, exercise: @exercise, intervention:, - accumulated_worktime_s: @exercise.accumulated_working_time_for_only(current_user) + user: current_contributor, exercise: @exercise, intervention:, + accumulated_worktime_s: @exercise.accumulated_working_time_for_only(current_contributor) ) uei.save render(json: {success: 'true'}) @@ -465,7 +465,7 @@ class ExercisesController < ApplicationController def statistics # Show general statistic page for specific exercise - user_statistics = {'InternalUser' => {}, 'ExternalUser' => {}} + contributor_statistics = {'InternalUser' => {}, 'ExternalUser' => {}, 'ProgrammingGroup' => {}} query = Submission.select('contributor_id, contributor_type, MAX(score) AS maximum_score, COUNT(id) AS runs') .where(exercise_id: @exercise.id) @@ -481,11 +481,11 @@ class ExercisesController < ApplicationController end query.each do |tuple| - user_statistics[tuple['contributor_type']][tuple['contributor_id'].to_i] = tuple + contributor_statistics[tuple['contributor_type']][tuple['contributor_id'].to_i] = tuple end render locals: { - user_statistics:, + contributor_statistics:, } end @@ -495,7 +495,7 @@ class ExercisesController < ApplicationController if policy(@exercise).detailed_statistics? submissions = Submission.where(contributor: @external_user, exercise: @exercise) .in_study_group_of(current_user) - .order('created_at') + .order(:created_at) @show_autosaves = params[:show_autosaves] == 'true' || submissions.none? {|s| s.cause != 'autosave' } submissions = submissions.where.not(cause: 'autosave') unless @show_autosaves interventions = UserExerciseIntervention.where('user_id = ? AND exercise_id = ?', @external_user.id, @@ -526,7 +526,8 @@ class ExercisesController < ApplicationController def submit @submission = Submission.create(submission_params) @submission.calculate_score - if @submission.user.external_user? && lti_outcome_service?(@submission.exercise_id, @submission.user.id) + + if @submission.users.map {|user| user.external_user? && lti_outcome_service?(@submission.exercise_id, user.id) }.any? transmit_lti_score else redirect_after_submit diff --git a/app/controllers/flowr_controller.rb b/app/controllers/flowr_controller.rb index 397b2d0e..8c6a30e1 100644 --- a/app/controllers/flowr_controller.rb +++ b/app/controllers/flowr_controller.rb @@ -5,7 +5,7 @@ class FlowrController < ApplicationController require_user! # get the latest submission for this user that also has a test run (i.e. structured_errors if applicable) submission = Submission.joins(:testruns) - .where(submissions: {contributor: current_user}) + .where(submissions: {contributor: current_contributor}) .includes(structured_errors: [structured_error_attributes: [:error_template_attribute]]) .merge(Testrun.order(created_at: :desc)).first diff --git a/app/controllers/programming_groups_controller.rb b/app/controllers/programming_groups_controller.rb new file mode 100644 index 00000000..c49bd67c --- /dev/null +++ b/app/controllers/programming_groups_controller.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +class ProgrammingGroupsController < ApplicationController + include CommonBehavior + include LtiHelper + + before_action :set_exercise_and_authorize + + def new + @programming_group = ProgrammingGroup.new(exercise: @exercise) + authorize! + existing_programming_group = current_user.programming_groups.find_by(exercise: @exercise) + if existing_programming_group + session[:pg_id] = existing_programming_group.id + redirect_to(implement_exercise_path(@exercise), + notice: t("sessions.create_through_lti.session_#{lti_outcome_service?(@exercise.id, current_user.id) ? 'with' : 'without'}_outcome", consumer: @consumer)) + end + end + + def create + programming_partner_ids = programming_group_params[: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 + end + @programming_group = ProgrammingGroup.new(exercise: @exercise, users:) + authorize! + + unless programming_partner_ids.include? current_user.id_with_type + @programming_group.add(current_user) + end + + create_and_respond(object: @programming_group, path: proc { implement_exercise_path(@exercise) }) do + session[:pg_id] = @programming_group.id + nil + end + end + + private + + def authorize! + authorize(@programming_group || @programming_groups) + end + + def programming_group_params + params.require(:programming_group).permit(:programming_partner_ids) + end + + def set_exercise_and_authorize + @exercise = Exercise.find(params[:exercise_id]) + authorize(@exercise, :implement?) + end +end diff --git a/app/controllers/remote_evaluation_controller.rb b/app/controllers/remote_evaluation_controller.rb index 63aecf23..062fdb7e 100644 --- a/app/controllers/remote_evaluation_controller.rb +++ b/app/controllers/remote_evaluation_controller.rb @@ -34,6 +34,7 @@ class RemoteEvaluationController < ApplicationController end def try_lti + # TODO: Need to consider and support programming groups if !@submission.user.nil? && lti_outcome_service?(@submission.exercise_id, @submission.user.id) lti_response = send_score(@submission) process_lti_response(lti_response) diff --git a/app/controllers/user_exercise_feedbacks_controller.rb b/app/controllers/user_exercise_feedbacks_controller.rb index 57921821..e4e51285 100644 --- a/app/controllers/user_exercise_feedbacks_controller.rb +++ b/app/controllers/user_exercise_feedbacks_controller.rb @@ -41,9 +41,9 @@ class UserExerciseFeedbacksController < ApplicationController Sentry.set_extras(params: uef_params) @exercise = Exercise.find(uef_params[:exercise_id]) - rfc = RequestForComment.unsolved.where(exercise_id: @exercise.id, user_id: current_user.id).first + rfc = RequestForComment.unsolved.where(exercise_id: @exercise.id, user: current_user).first submission = begin - current_user.submissions.where(exercise_id: @exercise.id).order('created_at DESC').first + current_contributor.submissions.where(exercise_id: @exercise.id).order('created_at DESC').first rescue StandardError nil end @@ -69,11 +69,11 @@ class UserExerciseFeedbacksController < ApplicationController def update submission = begin - current_user.submissions.where(exercise_id: @exercise.id).order('created_at DESC').final.first + current_contributor.submissions.where(exercise: @exercise).order(created_at: :desc).final.first rescue StandardError nil end - rfc = RequestForComment.unsolved.where(exercise_id: @exercise.id, user_id: current_user.id).first + rfc = RequestForComment.unsolved.where(exercise: @exercise, user: current_user).first authorize! if @exercise && validate_inputs(uef_params) path = @@ -123,18 +123,15 @@ class UserExerciseFeedbacksController < ApplicationController params[:user_exercise_feedback][:exercise_id] end - user_id = current_user.id - user_type = current_user.class.name latest_submission = Submission - .where(contributor_id: user_id, contributor_type: user_type, exercise_id:) + .where(contributor: current_contributor, exercise_id:) .order(created_at: :desc).final.first authorize(latest_submission, :show?) params[:user_exercise_feedback] .permit(:feedback_text, :difficulty, :exercise_id, :user_estimated_worktime) - .merge(user_id:, - user_type:, + .merge(user: current_user, submission: latest_submission, normalized_score: latest_submission&.normalized_score) end diff --git a/app/helpers/statistics_helper.rb b/app/helpers/statistics_helper.rb index d8e5abc7..7fe1421b 100644 --- a/app/helpers/statistics_helper.rb +++ b/app/helpers/statistics_helper.rb @@ -9,9 +9,9 @@ module StatisticsHelper def statistics_data [ { - key: 'users', - name: t('statistics.sections.users'), - entries: user_statistics, + key: 'contributors', + name: t('statistics.sections.contributors'), + entries: contributor_statistics, }, { key: 'exercises', @@ -26,7 +26,7 @@ module StatisticsHelper ] end - def user_statistics + def contributor_statistics [ { key: 'internal_users', @@ -40,10 +40,15 @@ module StatisticsHelper data: ExternalUser.count, url: external_users_path, }, + { + key: 'programming_groups', + name: t('activerecord.models.programming_group.other'), + data: ProgrammingGroup.count, + }, { key: 'currently_active', name: t('statistics.entries.users.currently_active'), - data: Submission.where(created_at: 5.minutes.ago.., contributor_type: ExternalUser.name).distinct.count(:contributor_id), + data: Submission.from(Submission.where(created_at: 5.minutes.ago..).distinct.select(:contributor_id, :contributor_type)).count, url: statistics_graphs_path, }, ] @@ -108,6 +113,7 @@ module StatisticsHelper ] end + # TODO: Need to consider and support programming groups def user_activity_live_data [ { @@ -209,6 +215,7 @@ module StatisticsHelper ] end + # TODO: Need to consider and support programming groups def ranged_user_data(interval = 'year', from = DateTime.new(0), to = DateTime.now) [ { diff --git a/app/models/exercise.rb b/app/models/exercise.rb index 17feeb51..925dc1e1 100644 --- a/app/models/exercise.rb +++ b/app/models/exercise.rb @@ -29,7 +29,7 @@ class Exercise < ApplicationRecord has_many :external_users, source: :contributor, source_type: 'ExternalUser', through: :submissions has_many :internal_users, source: :contributor, source_type: 'InternalUser', through: :submissions - alias users external_users + has_many :programming_groups scope :with_submissions, -> { where('id IN (SELECT exercise_id FROM submissions)') } @@ -57,10 +57,10 @@ class Exercise < ApplicationRecord end def finishers_percentage - if users.distinct.count.zero? + if contributors.empty? 0 else - (100.0 / users.distinct.count * finishers.count).round(2) + (100.0 / contributors.size * finishers_count).round(2) end end @@ -72,8 +72,11 @@ class Exercise < ApplicationRecord end def average_number_of_submissions - user_count = internal_users.distinct.count + external_users.distinct.count - user_count.zero? ? 0 : submissions.count / user_count.to_f + contributors.empty? ? 0 : submissions.count / contributors.size.to_f + end + + def contributors + @contributors ||= internal_users.distinct + external_users.distinct + programming_groups.distinct end def time_maximum_score(contributor) @@ -201,6 +204,17 @@ class Exercise < ApplicationRecord total_working_time FROM working_times_with_index JOIN internal_users ON contributor_type = 'InternalUser' AND contributor_id = internal_users.id + UNION ALL + SELECT index, + contributor_id, + contributor_type, + concat('PG ', programming_groups.id::varchar) AS name, + score, + start_time, + working_time_per_score, + total_working_time + FROM working_times_with_index + JOIN programming_groups ON contributor_type = 'ProgrammingGroup' AND contributor_id = programming_groups.id ORDER BY index, score ASC; " end @@ -262,7 +276,7 @@ class Exercise < ApplicationRecord (created_at - Lag(created_at) OVER (partition BY contributor_id, exercise_id ORDER BY created_at)) AS working_time FROM submissions WHERE #{self.class.sanitize_sql(['exercise_id = ?', id])} - AND contributor_type = 'ExternalUser' + AND contributor_type IN ('ExternalUser', 'ProgrammingGroup') GROUP BY contributor_id, id, exercise_id), max_points AS @@ -367,7 +381,7 @@ class Exercise < ApplicationRecord end def retrieve_working_time_statistics - @working_time_statistics = {'InternalUser' => {}, 'ExternalUser' => {}} + @working_time_statistics = {'InternalUser' => {}, 'ExternalUser' => {}, 'ProgrammingGroup' => {}} self.class.connection.exec_query(user_working_time_query).each do |tuple| tuple = tuple.merge('working_time' => format_time_difference(tuple['working_time'])) @working_time_statistics[tuple['contributor_type']][tuple['contributor_id'].to_i] = tuple @@ -532,9 +546,8 @@ class Exercise < ApplicationRecord maximum_score(contributor).to_i == maximum_score.to_i end - def finishers - ExternalUser.joins(:submissions).where(submissions: {exercise_id: id, score: maximum_score, -cause: %w[submit assess remoteSubmit remoteAssess]}).distinct + def finishers_count + Submission.from(submissions.where(score: maximum_score, cause: %w[submit assess remoteSubmit remoteAssess]).group(:contributor_id, :contributor_type).select(:contributor_id, :contributor_type), 'submissions').count end def set_default_values diff --git a/app/models/programming_group.rb b/app/models/programming_group.rb new file mode 100644 index 00000000..deb340c9 --- /dev/null +++ b/app/models/programming_group.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +class ProgrammingGroup < ApplicationRecord + include Contributor + + has_many :programming_group_memberships, dependent: :destroy + has_many :external_users, through: :programming_group_memberships, source_type: 'ExternalUser', source: :user + has_many :internal_users, through: :programming_group_memberships, source_type: 'InternalUser', source: :user + belongs_to :exercise + + validate :group_size + validate :no_erroneous_users + accepts_nested_attributes_for :programming_group_memberships + + def initialize(attributes = nil) + @erroneous_users = [] + super + end + + def external_user? + false + end + + def internal_user? + false + end + + def self.nested_resource? + true + end + + def programming_group? + true + end + + def add(user) + # Accessing the `users` method here will preload all users, which is otherwise done during validation. + internal_users << user if user.internal_user? && users.exclude?(user) + external_users << user if user.external_user? && users.exclude?(user) + user + end + + def to_s + displayname + end + + def displayname + "Programming Group #{id}" + end + + def programming_partner_ids + users.map(&:id_with_type) + end + + def users + internal_users + external_users + end + + def users=(users) + self.internal_users = [] + self.external_users = [] + users.each do |user| + next @erroneous_users << user unless user.is_a?(User) + + add(user) + end + end + + private + + def group_size + if users.size < 2 + errors.add(:base, :size_too_small) + end + end + + def no_erroneous_users + @erroneous_users.each do |partner_id| + errors.add(:base, :invalid_partner_id, partner_id:) + end + end +end diff --git a/app/models/programming_group_membership.rb b/app/models/programming_group_membership.rb new file mode 100644 index 00000000..d7182673 --- /dev/null +++ b/app/models/programming_group_membership.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class ProgrammingGroupMembership < ApplicationRecord + belongs_to :user, polymorphic: true + belongs_to :programming_group + + validate :unique_membership_for_exercise + validates :user_id, uniqueness: {scope: %i[programming_group_id user_type]} + + def unique_membership_for_exercise + if user.programming_groups.where(exercise: programming_group.exercise).any? + errors.add(:base, :already_exists, id_with_type: user.id_with_type) + end + end +end diff --git a/app/models/submission.rb b/app/models/submission.rb index bf110622..ce6485b5 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -24,6 +24,9 @@ class Submission < ApplicationRecord belongs_to :internal_users, lambda { where(submissions: {contributor_type: 'InternalUser'}).includes(:submissions) }, foreign_key: :contributor_id, class_name: 'InternalUser', optional: true + belongs_to :programming_groups, lambda { + where(submissions: {contributor_type: 'ProgrammingGroup'}).includes(:submissions) + }, foreign_key: :contributor_id, class_name: 'ProgrammingGroup', optional: true delegate :execution_environment, to: :exercise scope :final, -> { where(cause: %w[submit remoteSubmit]) } @@ -49,12 +52,6 @@ class Submission < ApplicationRecord # after_save :trigger_working_times_action_cable - def build_files_hash(files, attribute) - files.map(&attribute.to_proc).zip(files).to_h - end - - private :build_files_hash - def collect_files @collect_files ||= begin ancestors = build_files_hash(exercise.files.includes(:file_type), :id) @@ -202,8 +199,16 @@ class Submission < ApplicationRecord %w[study_group_id exercise_id cause] end + def users + contributor.try(:users) || [contributor] + end + private + def build_files_hash(files, attribute) + files.map(&attribute.to_proc).zip(files).to_h + end + def prepared_runner request_time = Time.zone.now begin diff --git a/app/models/user.rb b/app/models/user.rb index f980ca47..62744170 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -9,6 +9,8 @@ class User < ApplicationRecord has_many :authentication_token, dependent: :destroy has_many :study_group_memberships, as: :user has_many :study_groups, through: :study_group_memberships, as: :user + has_many :programming_group_memberships, as: :user + has_many :programming_groups, through: :programming_group_memberships, as: :user has_many :exercises, as: :user has_many :file_types, as: :user has_many :submissions, as: :contributor @@ -43,6 +45,10 @@ class User < ApplicationRecord is_a?(ExternalUser) end + def programming_group? + false + end + def learner? return true if current_study_group_id.nil? @@ -57,6 +63,10 @@ class User < ApplicationRecord @admin ||= platform_admin? end + def id_with_type + self.class.name.downcase.first + id.to_s + end + def store_current_study_group_id(study_group_id) @current_study_group_id = study_group_id self @@ -79,6 +89,16 @@ class User < ApplicationRecord } end + def self.find_by_id_with_type(id_with_type) + if id_with_type[0].casecmp('e').zero? + ExternalUser.find(id_with_type[1..]) + elsif id_with_type[0].casecmp('i').zero? + InternalUser.find(id_with_type[1..]) + else + raise ActiveRecord::RecordNotFound + end + end + def self.ransackable_attributes(auth_object) if auth_object.present? && auth_object.admin? %w[name email external_id consumer_id platform_admin] diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index f8c0848b..77cab4a9 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -49,6 +49,13 @@ class ApplicationPolicy end private :teacher_in_study_group? + def author_in_programming_group? + return false unless @record.contributor.programming_group? + + @record.contributor.users.include?(@user) + end + private :author_in_programming_group? + def initialize(user, record) @user = user @record = record diff --git a/app/policies/programming_group_policy.rb b/app/policies/programming_group_policy.rb new file mode 100644 index 00000000..e0fce84e --- /dev/null +++ b/app/policies/programming_group_policy.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class ProgrammingGroupPolicy < ApplicationPolicy + def new? + everyone + end + + def create? + everyone + end +end diff --git a/app/policies/submission_policy.rb b/app/policies/submission_policy.rb index 52dbae1e..77254a8a 100644 --- a/app/policies/submission_policy.rb +++ b/app/policies/submission_policy.rb @@ -9,7 +9,7 @@ class SubmissionPolicy < ApplicationPolicy # download_submission_file? is used in the live_streams_controller.rb %i[download? download_file? download_submission_file? run? score? show? statistics? stop? test? insights?].each do |action| - define_method(action) { admin? || author? } + define_method(action) { admin? || author? || author_in_programming_group? } end def render_file? diff --git a/app/views/execution_environments/statistics.html.slim b/app/views/execution_environments/statistics.html.slim index 53fc392c..43795d57 100644 --- a/app/views/execution_environments/statistics.html.slim +++ b/app/views/execution_environments/statistics.html.slim @@ -4,7 +4,7 @@ h1 = @execution_environment table.table.table-striped class="#{@execution_environment.present? ? 'sortable' : ''}" thead tr - - ['.exercise', '.users', '.score', '.maximum_score', '.stddev_score', '.percentage_correct', '.runs', '.worktime', '.stddev_worktime'].each do |title| + - ['.exercise', '.users_and_programming_groups', '.score', '.maximum_score', '.stddev_score', '.percentage_correct', '.runs', '.worktime', '.stddev_worktime'].each do |title| th.header = t(title) tbody - @execution_environment.exercises.each do |exercise| diff --git a/app/views/exercise_collections/statistics.html.slim b/app/views/exercise_collections/statistics.html.slim index 9f43fefd..2269c315 100644 --- a/app/views/exercise_collections/statistics.html.slim +++ b/app/views/exercise_collections/statistics.html.slim @@ -33,7 +33,7 @@ h4.mt-4 = t('activerecord.attributes.exercise_collections.exercises') tr th = '#' th = t('activerecord.attributes.exercise.title') - th = t('activerecord.attributes.exercise.number_of_users') + th = t('activerecord.attributes.exercise.number_of_users_and_programming_groups') th = t('activerecord.attributes.exercise.distinct_final_submissions') th = t('activerecord.attributes.exercise.finishing_rate') th = t('activerecord.attributes.exercise.average_score_percentage') @@ -44,8 +44,8 @@ h4.mt-4 = t('activerecord.attributes.exercise_collections.exercises') tr td = exercise_collection_item.position td = link_to_if(policy(exercise).show?, exercise.title, exercise) - td = exercise.users.distinct.count - td = exercise.submissions.send(:final).distinct.count(:user_id) + td = exercise.contributors.size + td = exercise.submissions.send(:final).distinct.count(:contributor_id) td = exercise.finishers_percentage td = exercise.average_percentage td = link_to(t('shared.statistics'), statistics_exercise_path(exercise), 'data-turbolinks' => "false") if policy(exercise).statistics? diff --git a/app/views/exercises/_editor_output.html.slim b/app/views/exercises/_editor_output.html.slim index d7c7ac47..e37122d0 100644 --- a/app/views/exercises/_editor_output.html.slim +++ b/app/views/exercises/_editor_output.html.slim @@ -52,7 +52,7 @@ div.d-grid id='output_sidebar_uncollapsed' class='d-none col-sm-12 enforce-botto | 0 = row(label: 'exercises.implement.feedback') = row(label: 'exercises.implement.messages') - #score data-maximum-score=@exercise.maximum_score data-score=@exercise.final_submission(current_user).try(:score) + #score data-maximum-score=@exercise.maximum_score data-score=@exercise.final_submission(current_contributor).try(:score) h4 span == "#{t('activerecord.attributes.submission.score')}: " span.score diff --git a/app/views/exercises/statistics.html.slim b/app/views/exercises/statistics.html.slim index 2caa92de..706e45b2 100644 --- a/app/views/exercises/statistics.html.slim +++ b/app/views/exercises/statistics.html.slim @@ -5,25 +5,26 @@ - append_javascript_pack_tag('d3-tip') h1 = @exercise -= row(label: '.participants', value: @exercise.users.distinct.count) += row(label: '.participants', value: @exercise.contributors.size) - [:intermediate, :final].each do |scope| = row(label: ".#{scope}_submissions") do - = "#{@exercise.submissions.send(scope).count} (#{t('.users', count: @exercise.submissions.send(scope).distinct.count(:contributor_id))})" + /TODO: Refactor next line + = "#{@exercise.submissions.send(scope).count} (#{t('.users_and_programming_groups', count: Submission.from(@exercise.submissions.send(scope).group(:contributor_id, :contributor_type).select(:contributor_id, :contributor_type)).count)})" = row(label: '.finishing_rate') do p - - if @exercise.finishers.count + - if @exercise.finishers_count span.number - = @exercise.finishers.count + = @exercise.finishers_count =<> t('shared.out_of') span.number - = @exercise.users.distinct.count + = @exercise.contributors.size =< t('exercises.statistics.external_users') - else = empty - - finishers_count = @exercise.users.distinct.count - - finishers_percentage = finishers_count == 0 ? 0 : (100.0 / finishers_count * @exercise.finishers.count).round(2) + - finishers_count = @exercise.contributors.size + - finishers_percentage = finishers_count == 0 ? 0 : (100.0 / finishers_count * @exercise.finishers_count).round(2) p = progress_bar(finishers_percentage) = row(label: '.average_score') do @@ -42,7 +43,7 @@ h1 = @exercise = row(label: '.average_worktime') do p = @exercise.average_working_time -- Hash[:internal_users => t('.internal_users'), :external_users => t('.external_users')].each_pair do |symbol, label| +- Hash[:internal_users => t('.internal_users'), :external_users => t('.external_users'), :programming_groups => t('.programming_groups')].each_pair do |symbol, label| - submissions = Submission.where(contributor: @exercise.send(symbol), exercise: @exercise).in_study_group_of(current_user) - if !policy(@exercise).detailed_statistics? - submissions = submissions.final diff --git a/app/views/programming_groups/_form.html.slim b/app/views/programming_groups/_form.html.slim new file mode 100644 index 00000000..20a5cc20 --- /dev/null +++ b/app/views/programming_groups/_form.html.slim @@ -0,0 +1,7 @@ += form_for(@programming_group, url: exercise_programming_groups_path) do |f| + = render('shared/form_errors', object: @programming_group) + .mb-3 + = f.label(:programming_partner_ids, class: 'form-label') + = f.text_field(:programming_partner_ids, class: 'form-control', required: true, value: (@programming_group.programming_partner_ids - [current_user.id_with_type]).join(', ')) + .help-block.form-text = t('.hints.programming_partner_ids') + .actions.mb-0 = render('shared/submit_button', f: f, object: @programming_group) diff --git a/app/views/programming_groups/new.html.slim b/app/views/programming_groups/new.html.slim new file mode 100644 index 00000000..cb41fe5c --- /dev/null +++ b/app/views/programming_groups/new.html.slim @@ -0,0 +1,14 @@ +h1 = t('shared.new_model', model: ProgrammingGroup.model_name.human) +p + => t('programming_groups.new.own_user_id') + b + = current_user.id_with_type +p + = t('programming_groups.new.enter_partner_id', exercise_title: @exercise.title) += render('form') + +div.mt-4 + a.btn.btn-success href=new_exercise_programming_group_path(@exercise) == t('programming_groups.new.check_invitation') + + p.mt-4 + == t('programming_groups.new.work_alone', path: implement_exercise_path(@exercise)) diff --git a/app/views/submissions/index.html.slim b/app/views/submissions/index.html.slim index b5df6709..c54b9da9 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(policy(submission.contributor).show?, submission.contributor, submission.contributor) + td = link_to_if(submission.contributor.is_a?(User) && 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 7109046a..762ff71e 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(policy(@submission.contributor).show?, @submission.contributor, @submission.contributor)) += row(label: 'submission.contributor', value: link_to_if(@submission.contributor.is_a?(User) && 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 bf00edda..df031a31 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -45,7 +45,7 @@ de: maximum_score: Erreichbare Punktzahl submission_deadline: Abgabefrist late_submission_deadline: Verspätete Abgabefrist - number_of_users: "# Nutzer" + number_of_users_and_programming_groups: "# Nutzer und Programmiergruppen" public: Öffentlich selection: Ausgewählt title: Titel @@ -56,6 +56,10 @@ de: token: "Aufgaben-Token" uuid: UUID unpublished: Deaktiviert + programming_group: + programming_partner_ids: Nutzer-IDs der Programmierpartner + programming_group/programming_group_memberships: + base: Programmiergruppenmitgliedschaft proxy_exercise: title: Title files_count: Anzahl der Aufgaben @@ -172,7 +176,7 @@ de: exercises: "Aufgaben" solutions: "Gesamtanzahl Lösungsversuche" submissions: "Gesamtanzahl Submissions" - users: "Teilnehmer" + users_and_programming_groups: "Teilnehmer und Programmiergruppen" user_exercise_feedback: user: "Autor" exercise: "Aufgabe" @@ -225,6 +229,12 @@ de: internal_user: one: Interner Nutzer other: Interne Nutzer + programming_group: + one: Programmiergruppe + other: Programmiergruppen + programming_group_membership: + one: Programmiergruppenmitgliedschaft + other: Programmiergruppenmitgliedschaften request_for_comment: one: Kommentaranfrage other: Kommentaranfragen @@ -259,6 +269,11 @@ de: attributes: password: weak: ist zu schwach. Versuchen Sie es mit einem langen Passwort, welches Groß-, Kleinbuchstaben, Zahlen und Sonderzeichen enthält. + programming_group: + size_too_small: Die Größe dieser Programmiergruppe ist zu klein. Geben Sie mindestens eine andere Nutzer-ID an. + invalid_partner_id: Die Nutzer-ID '%{partner_id}' ist ungültig und wurde entfernt. Bitte überprüfen Sie die Nutzer-IDs der Programmierpartner. + programming_group_membership: + already_exists: 'existiert bereits für diese Aufgabe für den Nutzer mit der ID %{id_with_type}.' admin: dashboard: show: @@ -346,7 +361,7 @@ de: permission_denied: Der Zugriff auf die angeforderte Datei wurde verweigert. Bitte überprüfen Sie, dass die Datei existiert, der aktuelle Benutzer Leseberechtigungen besitzt und versuchen Sie ggf. die Datei mit "root"-Rechten anzufordern. Dazu müssen Sie den "sudo"-Schalter neben der Befehlszeile aktivieren und anschließend das Dateisystem vor dem Herunterladen einer Datei aktualisieren. statistics: exercise: Übung - users: Anzahl (externer) Nutzer + users_and_programming_groups: Anzahl Nutzer und Programmiergruppen score: Durchschnittliche Punktzahl stddev_score: stdabw (Punktzahl) stddev_worktime: stdabw (Arbeitszeit) @@ -519,7 +534,7 @@ de: final_submissions: Finale Abgaben intermediate_submissions: Intermediäre Abgaben participants: Bearbeitende Nutzer - users: '%{count} verschiedene Nutzer' + users_and_programming_groups: '%{count} verschiedene Nutzer und Programmiergruppen' user: Nutzer score: Maximale Punktzahl deadline: Abgabefrist @@ -528,6 +543,7 @@ de: average_worktime: Durchschnittliche Arbeitszeit internal_users: Interne Nutzer external_users: Externe Nutzer + programming_groups: Programmiergruppen finishing_rate: Abschlussrate submit: failure: Beim Übermitteln Ihrer Punktzahl ist ein Fehler aufgetreten. Bitte versuchen Sie es später erneut. @@ -563,6 +579,15 @@ de: proxy_exercises: index: clone: Duplizieren + programming_groups: + form: + hints: + programming_partner_ids: "Sie können mehrere Nutzer-IDs mit Kommata getrennt eingeben, wie z.B.: 'e123, e234'." + new: + check_invitation: "Einladung prüfen" + enter_partner_id: "Bitte geben Sie hier die Nutzer-IDs der Personen ein, mit denen Sie zusammen die Aufgabe '%{exercise_title}' lösen möchten. Beachten Sie jedoch, dass anschließend keiner aus der Gruppe austreten kann. Alle Teammitglieder können also sehen, was Sie in dieser Aufgabe schreiben und umgekehrt. Für die nächste Aufgabe können Sie sich erneuert entscheiden, ob und mit wem Sie zusammen arbeiten möchten." + own_user_id: "Ihre Nutzer-ID:" + work_alone: "Sie können sich einmalig dafür entscheiden, die Aufgabe alleine zu bearbeiten. Anschließend können Sie jedoch nicht mehr in die Gruppenarbeit für diese Aufgabe wechseln. Klicken Sie hier, um die Aufgabe im Einzelmodus zu starten." external_users: statistics: title: Statistiken für Externe Benutzer @@ -967,7 +992,7 @@ de: subscription_not_existent: "Das Abonnement, von dem Sie sich abmelden wollen, existiert nicht." statistics: sections: - users: "Benutzer" + contributors: "Mitwirkende" exercises: "Aufgaben" request_for_comments: "Kommentaranfragen" entries: diff --git a/config/locales/en.yml b/config/locales/en.yml index de22f48f..6704839f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -45,7 +45,7 @@ en: maximum_score: Maximum Score submission_deadline: Submission Deadline late_submission_deadline: Late Submission Deadline - number_of_users: "# Users" + number_of_users_and_programming_groups: "# Users and Programming Groups" public: Public selection: Selected title: Title @@ -56,6 +56,10 @@ en: token: "Exercise Token" uuid: UUID unpublished: Unpublished + programming_group: + programming_partner_ids: Programming Partner IDs + programming_group/programming_group_memberships: + base: Programming Group Membership proxy_exercise: title: Title files_count: Exercises Count @@ -172,7 +176,7 @@ en: exercises: "Exercises" solutions: "Solution Attempts (accumulated)" submissions: "Submissions (accumulated)" - users: "Users" + users_and_programming_groups: "Users and Programming Groups" user_exercise_feedback: user: "Author" exercise: "Exercise" @@ -225,6 +229,12 @@ en: internal_user: one: Internal User other: Internal Users + programming_group: + one: Programming Group + other: Programming Groups + programming_group_membership: + one: Programming Group Membership + other: Programming Group Memberships request_for_comment: one: Request for Comments other: Requests for Comments @@ -259,6 +269,11 @@ en: attributes: password: weak: is too weak. Try to use a long password with upper and lower case letters, numbers and special characters. + programming_group: + size_too_small: The size of this programming group is too small. Enter at least one other user ID to work with. + invalid_partner_id: The user ID '%{partner_id}' is invalid and was removed. Please check the user IDs of your programming partners. + programming_group_membership: + already_exists: 'already exists for this exercise for the user with ID %{id_with_type}.' admin: dashboard: show: @@ -346,7 +361,7 @@ en: permission_denied: Access to the requested file has been denied. Please verify that the file exists, the current user has read permissions, and try requesting the file with "root" privileges if necessary. To retrieve files as "root", you must enable the "sudo" switch shown next to the command input and then reload the file system before accessing any file. statistics: exercise: Exercise - users: (External) Users Count + users_and_programming_groups: Users and Programming Groups Count score: Average Score stddev_score: stddev (score) stddev_worktime: stddev (working time) @@ -519,7 +534,7 @@ en: final_submissions: Final Submissions intermediate_submissions: Intermediate Submissions participants: Participating Users - users: '%{count} distinct users' + users_and_programming_groups: '%{count} distinct users and programming groups' user: User score: Maximum Score deadline: Deadline @@ -528,6 +543,7 @@ en: average_worktime: Average Working Time internal_users: Internal Users external_users: External Users + programming_groups: Programming Groups finishing_rate: Finishing Rate submit: failure: An error occurred while transmitting your score. Please try again later. @@ -563,6 +579,15 @@ en: proxy_exercises: index: clone: Duplicate + programming_groups: + form: + hints: + programming_partner_ids: "You can enter several user IDs separated by commas such as 'e123, e234'." + new: + check_invitation: "Check invitation" + enter_partner_id: "Please enter the user IDs from the practice partners with whom you want to solve the exercise '%{exercise_title}'. However, note that no one can leave the group afterward. Hence, all team members can see what you write in this exercise and vice versa. For the next exercise, you can decide again whether and with whom you want to work together." + own_user_id: "Your user ID:" + work_alone: "You can choose once to work on the exercise alone. Afterward, however, you will not be able to switch to group work for this exercise. Click here to get to the exercise in single mode." external_users: statistics: title: External User Statistics @@ -967,7 +992,7 @@ en: subscription_not_existent: "The subscription you want to unsubscribe from does not exist." statistics: sections: - users: "Users" + contributors: "Contributors" exercises: "Exercises" request_for_comments: "Requests for Comment" entries: diff --git a/config/routes.rb b/config/routes.rb index 220e487d..384c60d7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -96,6 +96,8 @@ Rails.application.routes.draw do post :export_external_check post :export_external_confirm end + + resources :programming_groups, only: %i[new create] end resources :exercise_collections do diff --git a/db/migrate/20230710131250_create_programming_groups.rb b/db/migrate/20230710131250_create_programming_groups.rb new file mode 100644 index 00000000..02d56de2 --- /dev/null +++ b/db/migrate/20230710131250_create_programming_groups.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class CreateProgrammingGroups < ActiveRecord::Migration[7.0] + def change + create_table :programming_groups do |t| + t.belongs_to :exercise, foreign_key: true, null: false, index: true + + t.timestamps + end + end +end diff --git a/db/migrate/20230710132428_create_programming_group_memberships.rb b/db/migrate/20230710132428_create_programming_group_memberships.rb new file mode 100644 index 00000000..c195bcf6 --- /dev/null +++ b/db/migrate/20230710132428_create_programming_group_memberships.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class CreateProgrammingGroupMemberships < ActiveRecord::Migration[7.0] + def change + create_table :programming_group_memberships, id: :uuid do |t| + t.belongs_to :programming_group, foreign_key: true, null: false, index: true + t.belongs_to :user, polymorphic: true, null: false, index: true + + t.timestamps + end + end +end diff --git a/db/schema.rb b/db/schema.rb index a0271ab5..2d3779cc 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -351,6 +351,23 @@ ActiveRecord::Schema[7.0].define(version: 2023_07_27_080619) do t.index ["external_users_id"], name: "index_lti_parameters_on_external_users_id" end + create_table "programming_group_memberships", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| + t.bigint "programming_group_id", null: false + t.string "user_type", null: false + t.bigint "user_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["programming_group_id"], name: "index_programming_group_memberships_on_programming_group_id" + t.index ["user_type", "user_id"], name: "index_programming_group_memberships_on_user" + end + + create_table "programming_groups", force: :cascade do |t| + t.bigint "exercise_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["exercise_id"], name: "index_programming_groups_on_exercise_id" + end + create_table "proxy_exercises", id: :serial, force: :cascade do |t| t.string "title" t.string "description" @@ -582,6 +599,8 @@ ActiveRecord::Schema[7.0].define(version: 2023_07_27_080619) do add_foreign_key "exercise_tips", "exercise_tips", column: "parent_exercise_tip_id" add_foreign_key "exercise_tips", "exercises" add_foreign_key "exercise_tips", "tips" + add_foreign_key "programming_group_memberships", "programming_groups" + add_foreign_key "programming_groups", "exercises" add_foreign_key "remote_evaluation_mappings", "study_groups" add_foreign_key "structured_error_attributes", "error_template_attributes" add_foreign_key "structured_error_attributes", "structured_errors" diff --git a/spec/concerns/file_parameters_spec.rb b/spec/concerns/file_parameters_spec.rb index 02d64715..a0f25887 100644 --- a/spec/concerns/file_parameters_spec.rb +++ b/spec/concerns/file_parameters_spec.rb @@ -25,7 +25,7 @@ describe FileParameters do it 'new file' do submission = create(:submission, exercise: hello_world, id: 1337) - controller.instance_variable_set(:@current_user, submission.contributor) + controller.instance_variable_set(:@current_contributor, submission.contributor) new_file = create(:file, context: submission) expect(file_accepted?(new_file)).to be true @@ -61,7 +61,7 @@ describe FileParameters do submission_learner1 = create(:submission, exercise: hello_world, contributor: learner1) _submission_learner2 = create(:submission, exercise: hello_world, contributor: learner2) - controller.instance_variable_set(:@current_user, learner2) + controller.instance_variable_set(:@current_contributor, learner2) other_submissions_file = create(:file, context: submission_learner1) expect(file_accepted?(other_submissions_file)).to be false end diff --git a/spec/concerns/lti_spec.rb b/spec/concerns/lti_spec.rb index e661cde3..2a9cd308 100644 --- a/spec/concerns/lti_spec.rb +++ b/spec/concerns/lti_spec.rb @@ -22,6 +22,7 @@ describe Lti do expect(controller.session).to receive(:delete).with(:external_user_id) expect(controller.session).to receive(:delete).with(:study_group_id) expect(controller.session).to receive(:delete).with(:embed_options) + expect(controller.session).to receive(:delete).with(:pg_id) controller.send(:clear_lti_session_data) end end diff --git a/spec/controllers/submissions_controller_spec.rb b/spec/controllers/submissions_controller_spec.rb index 0b9f6660..515f585d 100644 --- a/spec/controllers/submissions_controller_spec.rb +++ b/spec/controllers/submissions_controller_spec.rb @@ -5,166 +5,243 @@ require 'rails_helper' describe SubmissionsController do render_views - let(:submission) { create(:submission) } - let(:contributor) { create(:admin) } + let(:exercise) { create(:math) } + let(:submission) { create(:submission, exercise:, contributor:) } - before { allow(controller).to receive(:current_user).and_return(contributor) } - - describe 'POST #create' do - before do - controller.request.accept = 'application/json' - end - - context 'with a valid submission' do - let(:exercise) { create(:hello_world) } - let(:perform_request) { proc { post :create, format: :json, params: {submission: attributes_for(:submission, exercise_id: exercise.id)} } } - - before { perform_request.call } - - expect_assigns(submission: Submission) - - it 'creates the submission' do - expect { perform_request.call }.to change(Submission, :count).by(1) + shared_examples 'a regular user' do |record_not_found_status_code| + describe 'POST #create' do + before do + controller.request.accept = 'application/json' end - expect_json - expect_http_status(:created) + context 'with a valid submission' do + let(:exercise) { create(:hello_world) } + let(:perform_request) { proc { post :create, format: :json, params: {submission: attributes_for(:submission, exercise_id: exercise.id)} } } + + before { perform_request.call } + + expect_assigns(submission: Submission) + + it 'creates the submission' do + expect { perform_request.call }.to change(Submission, :count).by(1) + end + + expect_json + expect_http_status(:created) + end + + context 'with an invalid submission' do + before { post :create, params: {submission: {}} } + + expect_assigns(submission: Submission) + expect_json + expect_http_status(:unprocessable_entity) + end end - context 'with an invalid submission' do - before { post :create, params: {submission: {}} } + describe 'GET #download_file' do + context 'with an invalid filename' do + before { get :download_file, params: {filename: SecureRandom.hex, id: submission.id, format: :json} } - expect_assigns(submission: Submission) - expect_json - expect_http_status(:unprocessable_entity) - end - end + expect_http_status(record_not_found_status_code) + end - describe 'GET #download_file' do - context 'with an invalid filename' do - before { get :download_file, params: {filename: SecureRandom.hex, id: submission.id, format: :json} } + context 'with a valid binary filename' do + let(:exercise) { create(:sql_select) } + let(:submission) { create(:submission, exercise:, contributor:) } - expect_http_status(:not_found) - end + before { get :download_file, params: {filename: file.name_with_extension, id: submission.id} } - context 'with a valid binary filename' do - let(:submission) { create(:submission, exercise: create(:sql_select)) } + context 'with a binary file' do + let(:file) { submission.collect_files.detect {|file| file.name == 'exercise' && file.file_type.file_extension == '.sql' } } - before { get :download_file, params: {filename: file.name_with_extension, id: submission.id} } + expect_assigns(file: :file) + expect_assigns(submission: :submission) + expect_content_type('application/octet-stream') + expect_http_status(:ok) - context 'with a binary file' do - let(:file) { submission.collect_files.detect {|file| file.name == 'exercise' && file.file_type.file_extension == '.sql' } } + it 'sets the correct filename' do + expect(response.headers['Content-Disposition']).to include("attachment; filename=\"#{file.name_with_extension}\"") + end + end + end - expect_assigns(file: :file) - expect_assigns(submission: :submission) - expect_content_type('application/octet-stream') - expect_http_status(:ok) + context 'with a valid filename' do + let(:exercise) { create(:audio_video) } + let(:submission) { create(:submission, exercise:, contributor:) } - it 'sets the correct filename' do - expect(response.headers['Content-Disposition']).to include("attachment; filename=\"#{file.name_with_extension}\"") + before { get :download_file, params: {filename: file.name_with_extension, id: submission.id} } + + context 'with a binary file' do + let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.mp4' } } + + expect_assigns(file: :file) + expect_assigns(submission: :submission) + + it 'sets the correct redirect' do + expect(response.location).to eq protected_upload_url(id: file, filename: file.filepath) + end + end + + context 'with a non-binary file' do + let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.js' } } + + expect_assigns(file: :file) + expect_assigns(submission: :submission) + expect_content_type('application/octet-stream') + expect_http_status(:ok) + + it 'sets the correct filename' do + expect(response.headers['Content-Disposition']).to include("attachment; filename=\"#{file.name_with_extension}\"") + end end end end - context 'with a valid filename' do - let(:submission) { create(:submission, exercise: create(:audio_video)) } + describe 'GET #render_file' do + let(:file) { submission.files.first } + let(:signed_url) { AuthenticatedUrlHelper.sign(render_submission_url(submission, filename), submission) } + let(:token) { Rack::Utils.parse_nested_query(URI.parse(signed_url).query)['token'] } - before { get :download_file, params: {filename: file.name_with_extension, id: submission.id} } + context 'with an invalid filename' do + let(:filename) { SecureRandom.hex } - context 'with a binary file' do - let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.mp4' } } + before { get :render_file, params: {filename:, id: submission.id, token:} } - expect_assigns(file: :file) - expect_assigns(submission: :submission) - - it 'sets the correct redirect' do - expect(response.location).to eq protected_upload_url(id: file, filename: file.filepath) - end + expect_http_status(record_not_found_status_code) end - context 'with a non-binary file' do - let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.js' } } + context 'with a valid filename' do + let(:exercise) { create(:audio_video) } + let(:submission) { create(:submission, exercise:, contributor:) } + let(:filename) { file.name_with_extension } - expect_assigns(file: :file) - expect_assigns(submission: :submission) - expect_content_type('application/octet-stream') - expect_http_status(:ok) + before { get :render_file, params: {filename:, id: submission.id, token:} } - it 'sets the correct filename' do - expect(response.headers['Content-Disposition']).to include("attachment; filename=\"#{file.name_with_extension}\"") + context 'with a binary file' do + let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.mp4' } } + let(:signed_url_video) { AuthenticatedUrlHelper.sign(render_protected_upload_url(id: file, filename: file.filepath), file) } + + expect_assigns(file: :file) + expect_assigns(submission: :submission) + + it 'sets the correct redirect' do + expect(response.location).to eq signed_url_video + end + end + + context 'with a non-binary file' do + let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.js' } } + + expect_assigns(file: :file) + expect_assigns(submission: :submission) + expect_content_type('text/javascript') + expect_http_status(:ok) + + it 'renders the file content' do + expect(response.body).to eq(file.content) + end end end end - end - describe 'GET #index' do - before do - create_pair(:submission) - get :index - end + describe 'GET #run' do + let(:file) { submission.collect_files.detect(&:main_file?) } + let(:perform_request) { get :run, format: :json, params: {filename: file.filepath, id: submission.id} } - expect_assigns(submissions: Submission.all) - expect_http_status(:ok) - expect_template(:index) - end - - describe 'GET #render_file' do - let(:file) { submission.files.first } - let(:signed_url) { AuthenticatedUrlHelper.sign(render_submission_url(submission, filename), submission) } - let(:token) { Rack::Utils.parse_nested_query(URI.parse(signed_url).query)['token'] } - - context 'with an invalid filename' do - let(:filename) { SecureRandom.hex } - - before { get :render_file, params: {filename:, id: submission.id, token:} } - - expect_http_status(:not_found) - end - - context 'with a valid filename' do - let(:submission) { create(:submission, exercise: create(:audio_video)) } - let(:filename) { file.name_with_extension } - - before { get :render_file, params: {filename:, id: submission.id, token:} } - - context 'with a binary file' do - let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.mp4' } } - let(:signed_url_video) { AuthenticatedUrlHelper.sign(render_protected_upload_url(id: file, filename: file.filepath), file) } - - expect_assigns(file: :file) - expect_assigns(submission: :submission) - - it 'sets the correct redirect' do - expect(response.location).to eq signed_url_video + context 'when no errors occur during execution' do + before do + allow_any_instance_of(described_class).to receive(:hijack) + allow_any_instance_of(described_class).to receive(:close_client_connection) + allow_any_instance_of(Submission).to receive(:run).and_return({}) + allow_any_instance_of(described_class).to receive(:save_testrun_output) + perform_request end - end - context 'with a non-binary file' do - let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.js' } } - - expect_assigns(file: :file) expect_assigns(submission: :submission) - expect_content_type('text/javascript') - expect_http_status(:ok) - - it 'renders the file content' do - expect(response.body).to eq(file.content) - end + expect_assigns(file: :file) + expect_http_status(204) end end - end - describe 'GET #run' do - let(:file) { submission.collect_files.detect(&:main_file?) } - let(:perform_request) { get :run, format: :json, params: {filename: file.filepath, id: submission.id} } + describe 'GET #score' do + let(:perform_request) { proc { get :score, format: :json, params: {id: submission.id} } } - context 'when no errors occur during execution' do before do allow_any_instance_of(described_class).to receive(:hijack) - allow_any_instance_of(described_class).to receive(:close_client_connection) - allow_any_instance_of(Submission).to receive(:run).and_return({}) - allow_any_instance_of(described_class).to receive(:save_testrun_output) - perform_request + allow_any_instance_of(described_class).to receive(:kill_client_socket) + perform_request.call + end + + expect_assigns(submission: :submission) + expect_http_status(204) + end + + describe 'GET #show' do + before { get :show, params: {id: submission.id} } + + expect_assigns(submission: :submission) + expect_http_status(:ok) + expect_template(:show) + end + + describe 'GET #show.json' do + # Render views requested in controller tests in order to get json responses + # https://github.com/rails/jbuilder/issues/32 + render_views + + before { get :show, params: {id: submission.id}, format: :json } + + expect_assigns(submission: :submission) + expect_http_status(:ok) + + %i[run test].each do |action| + describe "##{action}_url" do + let(:url) { response.parsed_body.with_indifferent_access.fetch("#{action}_url") } + + it "starts like the #{action} path" do + filename = File.basename(__FILE__) + expect(url).to start_with(Rails.application.routes.url_helpers.send(:"#{action}_submission_path", submission, filename).sub(filename, '')) + end + + it 'ends with a placeholder' do + expect(url).to end_with("#{Submission::FILENAME_URL_PLACEHOLDER}.json") + end + end + end + + describe '#render_url' do + let(:supported_urls) { response.parsed_body.with_indifferent_access.fetch('render_url') } + let(:file) { submission.collect_files.detect(&:main_file?) } + let(:url) { supported_urls.find {|hash| hash[:filepath] == file.filepath }['url'] } + + it 'starts like the render path' do + expect(url).to start_with(Rails.application.routes.url_helpers.render_submission_url(submission, file.filepath, host: request.host)) + end + + it 'includes a token' do + expect(url).to include '?token=' + end + end + + describe '#score_url' do + let(:url) { response.parsed_body.with_indifferent_access.fetch('score_url') } + + it 'corresponds to the score path' do + expect(url).to eq(Rails.application.routes.url_helpers.score_submission_path(submission, format: :json)) + end + end + end + + describe 'GET #test' do + let(:file) { submission.collect_files.detect(&:teacher_defined_assessment?) } + let(:output) { {} } + + before do + file.update(hidden: false) + allow_any_instance_of(described_class).to receive(:hijack) + allow_any_instance_of(described_class).to receive(:kill_client_socket) + get :test, params: {filename: "#{file.filepath}.json", id: submission.id} end expect_assigns(submission: :submission) @@ -173,88 +250,57 @@ describe SubmissionsController do end end - describe 'GET #show' do - before { get :show, params: {id: submission.id} } - - expect_assigns(submission: :submission) - expect_http_status(:ok) - expect_template(:show) - end - - describe 'GET #show.json' do - # Render views requested in controller tests in order to get json responses - # https://github.com/rails/jbuilder/issues/32 - render_views - - before { get :show, params: {id: submission.id}, format: :json } - - expect_assigns(submission: :submission) - expect_http_status(:ok) - - %i[run test].each do |action| - describe "##{action}_url" do - let(:url) { response.parsed_body.with_indifferent_access.fetch("#{action}_url") } - - it "starts like the #{action} path" do - filename = File.basename(__FILE__) - expect(url).to start_with(Rails.application.routes.url_helpers.send(:"#{action}_submission_path", submission, filename).sub(filename, '')) - end - - it 'ends with a placeholder' do - expect(url).to end_with("#{Submission::FILENAME_URL_PLACEHOLDER}.json") - end - end - end - - describe '#render_url' do - let(:supported_urls) { response.parsed_body.with_indifferent_access.fetch('render_url') } - let(:file) { submission.collect_files.detect(&:main_file?) } - let(:url) { supported_urls.find {|hash| hash[:filepath] == file.filepath }['url'] } - - it 'starts like the render path' do - expect(url).to start_with(Rails.application.routes.url_helpers.render_submission_url(submission, file.filepath, host: request.host)) + shared_examples 'denies access for regular, non-admin users' do # rubocop:disable RSpec/SharedContext + describe 'GET #index' do + before do + create_pair(:submission, contributor:, exercise:) + get :index end - it 'includes a token' do - expect(url).to include '?token=' - end - end - - describe '#score_url' do - let(:url) { response.parsed_body.with_indifferent_access.fetch('score_url') } - - it 'corresponds to the score path' do - expect(url).to eq(Rails.application.routes.url_helpers.score_submission_path(submission, format: :json)) - end + expect_redirect(:root) end end - describe 'GET #score' do - let(:perform_request) { proc { get :score, format: :json, params: {id: submission.id} } } + context 'with an admin user' do + let(:contributor) { create(:admin) } + + before { allow(controller).to receive(:current_user).and_return(contributor) } + + describe 'GET #index' do + before do + create_pair(:submission, contributor:, exercise:) + get :index + end + + expect_assigns(submissions: Submission.all) + expect_http_status(:ok) + expect_template(:index) + end + + it_behaves_like 'a regular user', :not_found + end + + context 'with a programming group' do + let(:group_author) { create(:external_user) } + let(:other_group_author) { create(:external_user) } + let(:contributor) { create(:programming_group, exercise:, users: [group_author, other_group_author]) } before do - allow_any_instance_of(described_class).to receive(:hijack) - allow_any_instance_of(described_class).to receive(:kill_client_socket) - perform_request.call + allow(controller).to receive_messages(current_contributor: contributor, current_user: group_author) end - expect_assigns(submission: :submission) - expect_http_status(204) + it_behaves_like 'a regular user', :unauthorized + it_behaves_like 'denies access for regular, non-admin users' end - describe 'GET #test' do - let(:file) { submission.collect_files.detect(&:teacher_defined_assessment?) } - let(:output) { {} } + context 'with a learner' do + let(:contributor) { create(:external_user) } before do - file.update(hidden: false) - allow_any_instance_of(described_class).to receive(:hijack) - allow_any_instance_of(described_class).to receive(:kill_client_socket) - get :test, params: {filename: "#{file.filepath}.json", id: submission.id} + allow(controller).to receive_messages(current_user: contributor) end - expect_assigns(submission: :submission) - expect_assigns(file: :file) - expect_http_status(204) + it_behaves_like 'a regular user', :unauthorized + it_behaves_like 'denies access for regular, non-admin users' end end diff --git a/spec/factories/programming_group.rb b/spec/factories/programming_group.rb new file mode 100644 index 00000000..6307addd --- /dev/null +++ b/spec/factories/programming_group.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :programming_group do + exercise factory: :math + + after(:build) do |programming_group| + # Do not change anything if users were provided explicitly + next if programming_group.users.present? + + programming_group.users = build_list(:external_user, 2) + end + end +end diff --git a/spec/factories/submission.rb b/spec/factories/submission.rb index 28f841ed..fedcf974 100644 --- a/spec/factories/submission.rb +++ b/spec/factories/submission.rb @@ -10,6 +10,11 @@ FactoryBot.define do submission.exercise.files.editable.visible.each do |file| submission.add_file(content: file.content, file_id: file.id) end + + # Do not change anything if a study group was provided explicitly or user has no study groups + next if submission.study_group.present? || submission.users.first.study_groups.blank? + + submission.update!(study_group: submission.users.first.study_groups.first) end end end diff --git a/spec/policies/programming_group_policy_spec.rb b/spec/policies/programming_group_policy_spec.rb new file mode 100644 index 00000000..26a6e7f6 --- /dev/null +++ b/spec/policies/programming_group_policy_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe ProgrammingGroupPolicy do + subject(:policy) { described_class } + + let(:programming_group) { build(:programming_group) } + + %i[new? create?].each do |action| + permissions(action) do + it 'grants access to everyone' do + %i[external_user teacher admin].each do |factory_name| + expect(policy).to permit(create(factory_name), programming_group) + end + end + end + end +end diff --git a/spec/policies/submission_policy_spec.rb b/spec/policies/submission_policy_spec.rb index 4d571c4f..15f9fa6c 100644 --- a/spec/policies/submission_policy_spec.rb +++ b/spec/policies/submission_policy_spec.rb @@ -15,13 +15,23 @@ describe SubmissionPolicy do %i[download_file? render_file? run? score? show? statistics? stop? test?].each do |action| permissions(action) do + let(:exercise) { build(:math) } + let(:group_author) { build(:external_user) } + let(:other_group_author) { build(:external_user) } + it 'grants access to admins' do expect(policy).to permit(build(:admin), Submission.new) end it 'grants access to authors' do contributor = create(:external_user) - expect(policy).to permit(contributor, build(:submission, exercise: Exercise.new, contributor:)) + expect(policy).to permit(contributor, build(:submission, exercise:, contributor:)) + end + + it 'grants access to other authors of the programming group' do + contributor = build(:programming_group, exercise:, users: [group_author, other_group_author]) + expect(policy).to permit(group_author, build(:submission, exercise:, contributor:)) + expect(policy).to permit(other_group_author, build(:submission, exercise:, contributor:)) end end end diff --git a/spec/views/exercises/implement.html.slim_spec.rb b/spec/views/exercises/implement.html.slim_spec.rb index 6f0f8165..e8009daa 100644 --- a/spec/views/exercises/implement.html.slim_spec.rb +++ b/spec/views/exercises/implement.html.slim_spec.rb @@ -6,9 +6,12 @@ describe 'exercises/implement.html.slim' do let(:exercise) { create(:fibonacci) } let(:files) { exercise.files.visible } let(:non_binary_files) { files.reject {|file| file.file_type.binary? } } + let(:user) { create(:admin) } before do - allow(view).to receive(:current_user).and_return(create(:admin)) + without_partial_double_verification do + allow(view).to receive_messages(current_user: user, current_contributor: user) + end assign(:exercise, exercise) assign(:files, files) assign(:paths, [])