Save series_id for submissions

app/assets/javascripts/exercise.ts

@@ -139,7 +139,7 @@ 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(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;
@@ -228,6 +228,7 @@ async function initExerciseShow(exerciseId: number, programmingLanguage: string,
                     code: code,
                     exercise_id: exerciseId,
                     course_id: courseId,
+                    series_id: seriesId,
                 },
             }),
             "headers": {

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 @@
       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 @@
     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/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/series_policy.rb

@@ -135,6 +135,10 @@ def valid_token?(token)
       record.access_token == token
   end
 
+  def media?
+    show?
+  end
+
   private
 
   def course_member?

app/policies/submission_policy.rb

@@ -44,7 +44,7 @@ def media?
   end
 
   def permitted_attributes
-    %i[code exercise_id course_id]
+    %i[code exercise_id course_id series_id]
   end
 
   private

app/views/activities/_activities_table.html.erb

@@ -39,7 +39,7 @@
             <% if user_signed_in? %>
               <%= render partial: 'activities/user_status_icon', locals: {activity: activity, series: local_assigns[:series], user: user} %>
             <% end %>
-            <% if activity.accessible?(current_user, @course) %>
+            <% if activity.accessible?(current_user, course: @course, series: local_assigns[:series]) %>
               <%= link_to activity.name, get_activity_path.call(activity) %>
             <% elsif activity.access_public? %>
               <%= link_to activity.name, activity_path(activity) %>

app/views/activities/_series_activities_table.html.erb

@@ -51,7 +51,7 @@
 
         <td>
           <%= "#{index + 1}." if local_assigns[:series]&.activity_numbers_enabled? %>
-          <% if activity.accessible?(current_user, @course) %>
+          <% if activity.accessible?(current_user, course: @course, series: local_assigns[:series]) %>
             <%= link_to activity.name, get_activity_path.call(activity), class: ('blur' if (local_assigns[:series].hidden? || local_assigns[:series].closed?) && Current.demo_mode) %>
           <% else %>
             <%= activity.name %>

app/views/activities/show.html.erb

@@ -218,6 +218,7 @@ end %>
             <%= raw "\"#{@series&.deadline&.httpdate}\"" || "null" %>,
             "<%= submissions_url %>",
             `<%= escape_javascript (@activity.exercise? && @activity.boilerplate)|| "" %>`,
+            <%= @series&.id || "null" %>,
         );
     });
 </script>

app/views/pages/_user_card.html.erb

@@ -71,7 +71,7 @@
             <% end %>
           <% end %>
           <span class='float-start'><%= submission_status_icon(submission) %></span>
-          <% if exercise.accessible?(current_user, submission.course) %>
+          <% if exercise.accessible?(current_user, course: submission.course, series: submission.series) %>
             <%= link_to exercise.name, activity_scoped_path(course: submission.course, activity: exercise), class: "course-link #{'blur' if Current.demo_mode }", title: exercise.name %>
           <% else %>
             <span title="<%= exercise.name %>"  <%='class="blur"' if Current.demo_mode %>><%= exercise.name %></span>

db/migrate/20240614111643_add_series_id_to_submissions.rb

@@ -0,0 +1,5 @@
+class AddSeriesIdToSubmissions < ActiveRecord::Migration[7.1]
+  def change
+    add_column :submissions, :series_id, :integer
+  end
+end

db/migrate/20240617141422_add_series_id_to_activity_read_state.rb

@@ -0,0 +1,5 @@
+class AddSeriesIdToActivityReadState < ActiveRecord::Migration[7.1]
+  def change
+    add_column :activity_read_states, :series_id, :integer
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2024_03_07_124219) do
+ActiveRecord::Schema[7.1].define(version: 2024_06_17_141422) do
   create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t|
     t.string "name", null: false
     t.string "record_type", null: false
@@ -82,6 +82,7 @@
     t.integer "user_id", null: false
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
+    t.integer "series_id"
     t.index ["activity_id", "course_id", "user_id"], name: "activity_read_states_unique", unique: true
     t.index ["course_id"], name: "fk_rails_f674cacc14"
     t.index ["user_id"], name: "fk_rails_96d00253e9"
@@ -499,6 +500,7 @@
     t.string "fs_key", limit: 24
     t.integer "number"
     t.boolean "annotated", default: false, null: false
+    t.integer "series_id"
     t.index ["accepted"], name: "index_submissions_on_accepted"
     t.index ["course_id"], name: "index_submissions_on_course_id"
     t.index ["exercise_id", "status", "course_id"], name: "ex_st_co_idx"

test/controllers/activities_controller_test.rb

@@ -838,7 +838,7 @@ def create_exercises_return_valid
 
     get course_series_activity_url(right_course, wrong_series, right_exercise)
 
-    assert_redirected_to course_activity_url(right_course, right_exercise)
+    assert_redirected_to root_url
   end
 
   test 'should not show activity if series not in course' do

test/controllers/activity_read_states_controller_test.rb

@@ -122,6 +122,28 @@ def setup
     assert_predicate ActivityReadState.where(user: @user, activity: cp, course: course), :any?
   end
 
+  test 'should mark content_page as read within series' do
+    course = create :course, series_count: 1, content_pages_per_series: 1, subscribed_members: [@user]
+    series = course.series.first
+    cp = series.content_pages.first
+    post activity_activity_read_states_url(cp, format: :js), params: { activity_read_state: { activity_id: cp.id, course_id: course.id, series_id: series.id } }
+
+    assert_response :success
+
+    assert_predicate ActivityReadState.where(user: @user, activity: cp, course: course, series: series), :any?
+  end
+
+  test 'Should not mark content_page as read in other series' do
+    course = create :course, series_count: 2, content_pages_per_series: 1, subscribed_members: [@user]
+    series = course.series.first
+    cp = course.series.second.content_pages.first
+    post activity_activity_read_states_url(cp, format: :js), params: { activity_read_state: { activity_id: cp.id, course_id: course.id, series_id: series.id } }
+
+    assert_response :unprocessable_entity
+
+    assert_not_predicate ActivityReadState.where(user: @user, activity: cp, course: course, series: series), :any?
+  end
+
   test 'should mark content_page as read as json' do
     cp = create :content_page
     post activity_activity_read_states_url(cp, format: :json), params: { activity_read_state: { activity_id: cp.id } }

test/factories/submissions.rb

@@ -14,6 +14,7 @@
 #  fs_key      :string(24)
 #  number      :integer
 #  annotated   :boolean          default(FALSE), not null
+#  series_id   :integer
 #
 
 FactoryBot.define do