Merge pull request #5609 from dodona-edu/feat/link-submissions-to-series

Save series_id for submissions

app/assets/javascripts/exercise.ts

@@ -139,26 +139,36 @@ function initExerciseDescription(): void {
     initCodeFragments();
 }
 
-async function initExerciseShow(exerciseId: number, programmingLanguage: string, loggedIn: boolean, editorShown: boolean, courseId: number, _deadline: string, baseSubmissionsUrl: string, boilerplate: string): Promise<void> {
+async function initExerciseShow(options: {
+    exerciseId: number,
+    programmingLanguage: string,
+    loggedIn: boolean,
+    editorShown: boolean,
+    courseId: number,
+    deadline: string,
+    baseSubmissionsUrl: string,
+    boilerplate: string,
+    seriesId: number
+}): Promise<void> {
     let editor: EditorView;
     let lastSubmission: string;
     let lastTimeout: number;
 
     async function init(): Promise<void> {
-        if (editorShown) {
+        if (options.editorShown) {
             const editorReady = initEditor();
             initDeadlineTimeout();
             enableSubmissionTableLinks();
-            if (loggedIn) {
+            if (options.loggedIn) {
                 swapActionButtons();
             }
             await editorReady;
-            initRestoreBoilerplateButton(boilerplate);
+            initRestoreBoilerplateButton(options.boilerplate);
         }
 
         // submit source code if button is clicked on editor panel
         document.getElementById("editor-process-btn")?.addEventListener("click", () => {
-            if (!loggedIn) return;
+            if (!options.loggedIn) return;
             // test submitted source code
             const source = editor.state.doc.toString();
             disableSubmitButton();
@@ -193,7 +203,7 @@ async function initExerciseShow(exerciseId: number, programmingLanguage: string,
     }
 
     async function initEditor(): Promise<void> {
-        editor = await configureEditor(document.getElementById("editor-text"), programmingLanguage, enableSubmitButton);
+        editor = await configureEditor(document.getElementById("editor-text"), options.programmingLanguage, enableSubmitButton);
         editor.focus();
         // Make editor available globally
         window.dodona.editor = editor;
@@ -226,8 +236,9 @@ async function initExerciseShow(exerciseId: number, programmingLanguage: string,
             "body": JSON.stringify({
                 submission: {
                     code: code,
-                    exercise_id: exerciseId,
-                    course_id: courseId,
+                    exercise_id: options.exerciseId,
+                    course_id: options.courseId,
+                    series_id: options.seriesId,
                 },
             }),
             "headers": {
@@ -275,7 +286,7 @@ async function initExerciseShow(exerciseId: number, programmingLanguage: string,
                     return;
                 }
                 event.preventDefault();
-                loadFeedback(baseSubmissionsUrl + element.dataset.submission_id, element.dataset.submission_id);
+                loadFeedback(options.baseSubmissionsUrl + element.dataset.submission_id, element.dataset.submission_id);
             });
         });
     }
@@ -307,7 +318,7 @@ async function initExerciseShow(exerciseId: number, programmingLanguage: string,
                     (submissionRow.querySelector(".load-submission") as HTMLButtonElement).click();
                 } else if (document.getElementById("activity-feedback-link").classList.contains("active") &&
                     document.getElementById("activity-feedback-link").dataset.submission_id === lastSubmission) {
-                    loadFeedback(baseSubmissionsUrl + lastSubmission, lastSubmission);
+                    loadFeedback(options.baseSubmissionsUrl + lastSubmission, lastSubmission);
                 }
                 showFABStatus(status);
                 setTimeout(enableSubmitButton, 100);
@@ -318,7 +329,7 @@ async function initExerciseShow(exerciseId: number, programmingLanguage: string,
     }
 
     function enableSubmitButton(): void {
-        if (!loggedIn) {
+        if (!options.loggedIn) {
             return;
         }
 
@@ -329,7 +340,7 @@ async function initExerciseShow(exerciseId: number, programmingLanguage: string,
     }
 
     function disableSubmitButton(): void {
-        if (!loggedIn) {
+        if (!options.loggedIn) {
             return;
         }
 
@@ -428,12 +439,12 @@ async function initExerciseShow(exerciseId: number, programmingLanguage: string,
     }
 
     function initDeadlineTimeout(): void {
-        if (!_deadline) {
+        if (!options.deadline) {
             return;
         }
         const deadlineWarningElement = document.getElementById("deadline-warning");
         const deadlineInfoElement = document.getElementById("deadline-info");
-        const deadline = new Date(_deadline);
+        const deadline = new Date(options.deadline);
         const infoDeadline = new Date(deadline.getTime() - (5 * 60 * 1000));
 
         function showDeadlineAlerts(): void {

app/controllers/activities_controller.rb

@@ -7,7 +7,7 @@ class ActivitiesController < ApplicationController
 
   before_action :set_activity, only: %i[show description edit update media info]
   before_action :set_course, only: %i[show edit update media info]
-  before_action :set_series, only: %i[show edit update info]
+  before_action :set_series, only: %i[show edit update media info]
   before_action :ensure_trailing_slash, only: :show
   before_action :set_lti_message, only: %i[show]
   before_action :set_lti_provider, only: %i[show]
@@ -110,10 +110,10 @@ def show
 
     # Double check if activity still exists within this course (And throw a 404 when it does not)
     @course&.activities&.find(@activity.id) if current_user&.course_admin?(@course)
+
     # We still need to check access because an unauthenticated user should be able to see public activities
-    raise Pundit::NotAuthorizedError, 'Not allowed' unless @activity.accessible?(current_user, @course)
+    raise Pundit::NotAuthorizedError, 'Not allowed' unless @activity.accessible?(current_user, course: @course, series: @series)
 
-    @series = Series.find_by(id: params[:series_id])
     # Double check if activity still exists within this series, redirect to course activity if it does not
     redirect_to helpers.activity_scoped_path(activity: @activity, course: @course) if @series&.activities&.exclude?(@activity)
 
@@ -199,7 +199,7 @@ def update
   def media
     if params.key?(:token)
       raise Pundit::NotAuthorizedError, 'Not allowed' unless @activity.access_token == params[:token]
-    elsif !@activity.accessible?(current_user, @course)
+    elsif !@activity.accessible?(current_user, course: @course, series: @series)
       raise Pundit::NotAuthorizedError, 'Not allowed'
     end
 

app/controllers/activity_read_states_controller.rb

@@ -45,8 +45,12 @@ def create
     authorize ActivityReadState
     args = permitted_attributes(ActivityReadState)
     args[:user_id] = current_user.id
+    # check if user is member of course
     course = Course.find(args[:course_id]) if args[:course_id].present?
     args.delete(:course_id) if args[:course_id].present? && course.subscribed_members.exclude?(current_user)
+    # check if series is part of course
+    series = Series.find(args[:series_id]) if args[:series_id].present? && args[:course_id].present?
+    args.delete(:series_id) if args[:series_id].present? && course.series.exclude?(series)
     read_state = ActivityReadState.new args
     can_read = Pundit.policy!(current_user, read_state.activity).read?
     if can_read && read_state.save

app/controllers/submissions_controller.rb

@@ -95,8 +95,10 @@ def edit
       format.html do
         if @submission.course.nil?
           redirect_to activity_url(@submission.exercise, anchor: 'submission-card', edit_submission: @submission)
-        else
+        elsif @submission.series.nil?
           redirect_to course_activity_url(@submission.course, @submission.exercise, anchor: 'submission-card', edit_submission: @submission)
+        else
+          redirect_to course_series_activity_url(@submission.course, @submission.series, @submission.exercise, anchor: 'submission-card', edit_submission: @submission)
         end
       end
     end
@@ -108,13 +110,18 @@ def create
     para[:user_id] = current_user.id
     para[:code].gsub!(/\r\n?/, "\n")
     para[:evaluate] = true # immediately evaluate after create
+    # check if user is member of course
     course = Course.find(para[:course_id]) if para[:course_id].present?
     para.delete(:course_id) if para[:course_id].present? && course.subscribed_members.exclude?(current_user)
+    # check if series is part of course
+    series = Series.find(para[:series_id]) if para[:series_id].present? && para[:course_id].present?
+    para.delete(:series_id) if para[:series_id].present? && course.series.exclude?(series)
+
     submission = Submission.new(para)
     can_submit = true
     if submission.exercise.present?
       can_submit &&= Pundit.policy!(current_user, submission.exercise).submit?
-      can_submit &&= submission.exercise.accessible?(current_user, course)
+      can_submit &&= submission.exercise.accessible?(current_user, course: course, series: series)
     end
     if can_submit && submission.save
       render json: { status: 'ok', id: submission.id, exercise_id: submission.exercise_id, course_id: submission.course_id, url: submission_url(submission, format: :json) }

app/models/activity.rb

@@ -313,28 +313,51 @@ def usable_by?(course)
     access_public? || course.usable_repositories.pluck(:id).include?(repository.id)
   end
 
-  def accessible?(user, course)
-    if course.present?
+  def accessible?(user, course: nil, series: nil)
+    # first validate the course/series accessibility for the user
+    # and make sure the activity is present in the course/series
+    if series.present?
+      return false if course.present? && course != series.course
+      return false unless series.accessible_to?(user) && series.activities.pluck(:id).include?(id)
+
+      # if course was not present, set to series.course
+      course = series.course
+    elsif course.present?
+      # no series specified, so check if the activity is accessible in any series
       if user&.course_admin? course
         return false unless course.activities.pluck(:id).include? id
       elsif user&.member_of? course
         return false unless course.accessible_activities.pluck(:id).include? id
       else
         return false unless course.visible_activities.pluck(:id).include? id
       end
+    end
+
+    # we are now sure that the activity is present within the course or series
+    # and the user has access to the course and series
+
+    # now validate the activity specific access
+    if course.present?
       return true if user&.zeus?
+      # repositories must give courses access to private activities
       return false unless access_public? ||
                           repository.allowed_courses.pluck(:id).include?(course&.id)
+      # course admins can access all other activities
       return true if user&.course_admin? course
+      # only course admins can access private activities
       return false if draft?
+      # course members can access al other activities
       return true if user&.member_of?(course)
+      # non-members can only access public activities of moderated courses
       return false if course.moderated && access_private?
 
+      # non-members can access all activities of courses to which they can freely subscribe
       course.open_for_user?(user)
     else
       return true if user&.repository_admin? repository
       return false if draft?
 
+      # non draft public activities are always accessible
       access_public?
     end
   end

app/models/activity_read_state.rb

@@ -8,10 +8,12 @@
 #  user_id     :integer          not null
 #  created_at  :datetime         not null
 #  updated_at  :datetime         not null
+#  series_id   :integer
 #
 class ActivityReadState < ApplicationRecord
   belongs_to :activity
   belongs_to :course, optional: true
+  belongs_to :series, optional: true
   belongs_to :user
 
   validates :activity, uniqueness: { scope: %i[user course] }
@@ -65,6 +67,6 @@ def invalidate_caches
   end
 
   def activity_accessible_for_user?
-    errors.add(:activity, 'not accessible') unless activity.accessible?(user, course)
+    errors.add(:activity, 'not accessible') unless activity.accessible?(user, course: course, series: series)
   end
 end

app/models/course.rb

@@ -303,6 +303,10 @@ def open_for_user?(user)
     open_for_all? || (open_for_institution? && institution == user&.institution) || (open_for_institutional_users? && user&.institutional?)
   end
 
+  def visible_for_user?(user)
+    visible_for_all? || (visible_for_institution? && institution == user&.institution) || user&.member_of?(self)
+  end
+
   def invalidate_subscribed_members_count_cache
     Rails.cache.delete(format(SUBSCRIBED_MEMBERS_COUNT_CACHE_STRING, id: id))
   end

app/models/series.rb

@@ -206,4 +206,24 @@ def invalidate_caches(user)
     invalidate_started?(user: user)
     invalidate_wrong?(user: user)
   end
+
+  def visible_to?(user)
+    return true if user&.course_admin? course
+
+    return true if open?
+
+    return false if hidden?
+
+    return false if closed?
+
+    return false if timed? && visibility_start > Time.zone.now
+
+    true
+  end
+
+  def accessible_to?(user)
+    return true if visible_to?(user)
+
+    user&.member_of?(course) && hidden?
+  end
 end

app/models/submission.rb

@@ -14,6 +14,7 @@
 #  fs_key      :string(24)
 #  number      :integer
 #  annotated   :boolean          default(FALSE), not null
+#  series_id   :integer
 #
 
 class Submission < ApplicationRecord
@@ -32,6 +33,7 @@ class Submission < ApplicationRecord
   belongs_to :exercise, optional: false
   belongs_to :user, optional: false
   belongs_to :course, optional: true
+  belongs_to :series, optional: true
   has_one :judge, through: :exercise
   has_one :notification, as: :notifiable, dependent: :destroy
   has_many :annotations, dependent: :destroy
@@ -145,17 +147,6 @@ def result=(result)
     File.binwrite(File.join(fs_path, RESULT_FILENAME), ActiveSupport::Gzip.compress(result.force_encoding('UTF-8')))
   end
 
-  def series
-    return nil if course.nil?
-
-    series = course.series
-    # we want to avoid accidentally linking a hidden series to a student
-    series = series.visible
-    # There could actually be multiple series with the same exercise and the same course
-    # But for now we just return the first one, as there is only one in most cases
-    series.joins(:series_memberships).find_by(series_memberships: { activity: exercise })
-  end
-
   def clean_messages(messages, levels)
     messages.select { |m| !m.is_a?(Hash) || !m.key?(:permission) || levels.include?(m[:permission]) }
   end

app/models/user.rb

@@ -479,7 +479,7 @@ def jump_back_in
 
     # Don't give information about the exercise if the submission was within a course, but not within a visible series
     # This is to prevent students from seeing exercises that are not made public explicitly
-    if latest_submission.course.present? && (latest_submission.series.nil? || !latest_submission.series.open?)
+    if latest_submission.course.present? && !latest_submission.series&.visible_to?(self)
       # Don't show complete courses
       return [] if latest_submission.course.incomplete_series(self).blank?
 
@@ -504,7 +504,7 @@ def jump_back_in
       }
     end
 
-    if latest_submission.series.present? && (latest_submission.series.open? || course_admin?(latest_submission.course))
+    if latest_submission.series.present? && latest_submission.series.visible_to?(self)
       next_activity = latest_submission.series.next_activity(latest_submission.exercise)
       if next_activity.present?
         # start working on the next exercise, if that one was not accepted
@@ -531,7 +531,7 @@ def jump_back_in
       end
 
       next_series = latest_submission.series.next
-      if next_series.present? && (next_series.open? || course_admin?(latest_submission.course)) && !next_series.completed?(user: self)
+      if next_series.present? && next_series.visible_to?(self) && !next_series.completed?(user: self)
         # start working on the next series
         result << {
           submission: nil,

app/policies/activity_read_state_policy.rb

@@ -20,6 +20,6 @@ def create?
   end
 
   def permitted_attributes
-    %i[activity_id course_id]
+    %i[activity_id course_id series_id]
   end
 end

app/policies/course_policy.rb

@@ -37,9 +37,7 @@ def info?
   def copy?
     create? && (
       user&.zeus? ||
-      record.visible_for_all? ||
-      (record.visible_for_institution? && record.institution == user&.institution) ||
-      record.subscribed_members.include?(user)
+      record.visible_for_user?(user)
     )
   end