diff --git a/app/assets/javascripts/exercise.ts b/app/assets/javascripts/exercise.ts index 0c750c9907..a5ff82c5ca 100644 --- a/app/assets/javascripts/exercise.ts +++ b/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 { +async function initExerciseShow(options: { + exerciseId: number, + programmingLanguage: string, + loggedIn: boolean, + editorShown: boolean, + courseId: number, + deadline: string, + baseSubmissionsUrl: string, + boilerplate: string, + seriesId: number +}): Promise { let editor: EditorView; let lastSubmission: string; let lastTimeout: number; async function init(): Promise { - 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 { - 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 { diff --git a/app/controllers/activities_controller.rb b/app/controllers/activities_controller.rb index c55b68f742..45c2b8564e 100644 --- a/app/controllers/activities_controller.rb +++ b/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 diff --git a/app/controllers/activity_read_states_controller.rb b/app/controllers/activity_read_states_controller.rb index 3bf67cb25d..89c5d15eac 100644 --- a/app/controllers/activity_read_states_controller.rb +++ b/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 diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 1ee113dfbe..327d8636fe 100644 --- a/app/controllers/submissions_controller.rb +++ b/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) } diff --git a/app/models/activity.rb b/app/models/activity.rb index 53f51ba712..7c9ed803ed 100644 --- a/app/models/activity.rb +++ b/app/models/activity.rb @@ -313,8 +313,17 @@ 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 @@ -322,19 +331,33 @@ def accessible?(user, course) 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 diff --git a/app/models/activity_read_state.rb b/app/models/activity_read_state.rb index caa2ae8279..fd8b2a10b7 100644 --- a/app/models/activity_read_state.rb +++ b/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 diff --git a/app/models/course.rb b/app/models/course.rb index 42061e2122..f6be23a577 100644 --- a/app/models/course.rb +++ b/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 diff --git a/app/models/series.rb b/app/models/series.rb index 8f13885d7d..653cf6509f 100644 --- a/app/models/series.rb +++ b/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 diff --git a/app/models/submission.rb b/app/models/submission.rb index f41c91f50a..a1e8120aed 100644 --- a/app/models/submission.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index bc3fc08e4f..208f6ee54a 100644 --- a/app/models/user.rb +++ b/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, diff --git a/app/policies/activity_read_state_policy.rb b/app/policies/activity_read_state_policy.rb index 9cf615c2b5..3f53d9ed29 100644 --- a/app/policies/activity_read_state_policy.rb +++ b/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 diff --git a/app/policies/course_policy.rb b/app/policies/course_policy.rb index 1dab3e2993..04e4d5ec13 100644 --- a/app/policies/course_policy.rb +++ b/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 diff --git a/app/policies/series_policy.rb b/app/policies/series_policy.rb index 0aa8689c17..88251a7008 100644 --- a/app/policies/series_policy.rb +++ b/app/policies/series_policy.rb @@ -19,15 +19,9 @@ def index? end def show? - return true if course_admin? - return false if record.closed? - return false if record.hidden? && user.nil? - return false if record.timed? && record.visibility_start > Time.zone.now + return false unless record.accessible_to?(user) - course = record.course - course.visible_for_all? || - (course.visible_for_institution? && course.institution == user&.institution) || - user&.member_of?(course) + record.course.visible_for_user?(user) end def info? @@ -135,6 +129,10 @@ def valid_token?(token) record.access_token == token end + def media? + show? + end + private def course_member? diff --git a/app/policies/submission_policy.rb b/app/policies/submission_policy.rb index 65e5e39d06..444cac8d21 100644 --- a/app/policies/submission_policy.rb +++ b/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 diff --git a/app/views/activities/_activities_table.html.erb b/app/views/activities/_activities_table.html.erb index e171667bf2..11f59272f3 100644 --- a/app/views/activities/_activities_table.html.erb +++ b/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) %> diff --git a/app/views/activities/_series_activities_table.html.erb b/app/views/activities/_series_activities_table.html.erb index 74168ee498..4546ddaec0 100644 --- a/app/views/activities/_series_activities_table.html.erb +++ b/app/views/activities/_series_activities_table.html.erb @@ -53,7 +53,7 @@ <%= "#{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 %> diff --git a/app/views/activities/show.html.erb b/app/views/activities/show.html.erb index 27faf4d7ba..ffd5adead9 100644 --- a/app/views/activities/show.html.erb +++ b/app/views/activities/show.html.erb @@ -209,15 +209,21 @@ end %> <% end %> diff --git a/app/views/pages/_user_card.html.erb b/app/views/pages/_user_card.html.erb index 361b65d424..6fa5e88641 100644 --- a/app/views/pages/_user_card.html.erb +++ b/app/views/pages/_user_card.html.erb @@ -71,7 +71,7 @@ <% end %> <% end %> <%= submission_status_icon(submission) %> - <% 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 %> ><%= exercise.name %> diff --git a/app/views/submissions/_submission_basic.json.jbuilder b/app/views/submissions/_submission_basic.json.jbuilder index a652c7b752..6ac1d14884 100644 --- a/app/views/submissions/_submission_basic.json.jbuilder +++ b/app/views/submissions/_submission_basic.json.jbuilder @@ -8,3 +8,4 @@ end json.has_annotations submission.annotated? json.exercise activity_scoped_url(activity: submission.exercise, course: submission.course, options: { format: :json }) json.course course_url(submission.course, format: :json) if submission.course +json.series series_url(submission.series, format: :json) if submission.series diff --git a/db/migrate/20240614111643_add_series_id_to_submissions.rb b/db/migrate/20240614111643_add_series_id_to_submissions.rb new file mode 100644 index 0000000000..455058b08d --- /dev/null +++ b/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 diff --git a/db/migrate/20240617141422_add_series_id_to_activity_read_state.rb b/db/migrate/20240617141422_add_series_id_to_activity_read_state.rb new file mode 100644 index 0000000000..debd34b200 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 56ec0e3916..fb4d3b5f6f 100644 --- a/db/schema.rb +++ b/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" diff --git a/test/controllers/activities_controller_test.rb b/test/controllers/activities_controller_test.rb index da467bd029..b508ef2507 100644 --- a/test/controllers/activities_controller_test.rb +++ b/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 @@ -1012,6 +1012,7 @@ class ExerciseDescriptionTest < ActionDispatch::IntegrationTest test 'activities for hidden series should only be shown with token' do course = courses(:course1) + course.subscribed_members << users(:student) exercise = exercises(:python_exercise) other_exercise = create :exercise series = create :series, course: course, exercises: [exercise, other_exercise], visibility: :hidden diff --git a/test/controllers/activity_read_states_controller_test.rb b/test/controllers/activity_read_states_controller_test.rb index 21defab52f..f1ade1306d 100644 --- a/test/controllers/activity_read_states_controller_test.rb +++ b/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 } } diff --git a/test/controllers/series_controller_test.rb b/test/controllers/series_controller_test.rb index 6651d1666e..6f11a70944 100644 --- a/test/controllers/series_controller_test.rb +++ b/test/controllers/series_controller_test.rb @@ -297,13 +297,22 @@ def assert_show_and_overview(authorized, token: nil) test 'student should see hidden series with token' do sign_in @student + @series.course.subscribed_members << @student @series.update(visibility: :hidden) assert_show_and_overview true, token: @series.access_token end + test 'student that is not member of course should not see hidden series with token' do + sign_in @student + @series.update(visibility: :hidden) + + assert_show_and_overview false, token: @series.access_token + end + test 'student should not see hidden series with wrong token' do sign_in @student + @series.course.subscribed_members << @student @series.update(visibility: :hidden) assert_show_and_overview false, token: 'hunter2' @@ -311,6 +320,7 @@ def assert_show_and_overview(authorized, token: nil) test 'student should not see closed series with token' do sign_in @student + @series.course.subscribed_members << @student @series.update(visibility: :closed) assert_show_and_overview false, token: @series.access_token diff --git a/test/factories/submissions.rb b/test/factories/submissions.rb index f3a9dc1b71..3fc8ff92f1 100644 --- a/test/factories/submissions.rb +++ b/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 diff --git a/test/models/activity_read_state_test.rb b/test/models/activity_read_state_test.rb index c81a8d69cc..3470ad7399 100644 --- a/test/models/activity_read_state_test.rb +++ b/test/models/activity_read_state_test.rb @@ -8,6 +8,7 @@ # user_id :integer not null # created_at :datetime not null # updated_at :datetime not null +# series_id :integer # require 'test_helper' @@ -55,7 +56,7 @@ class ActivityReadStateTest < ActiveSupport::TestCase series.update(visibility: :closed) - assert_not content_page.accessible?(user, series.course) + assert_not content_page.accessible?(user, course: series.course) assert_predicate read_state, :valid? assert read_state.update(updated_at: Time.current) end diff --git a/test/models/exercise_test.rb b/test/models/exercise_test.rb index c262d3be84..0ad668d269 100644 --- a/test/models/exercise_test.rb +++ b/test/models/exercise_test.rb @@ -60,106 +60,137 @@ class ExerciseTest < ActiveSupport::TestCase course = create :course, users: [@user] User.any_instance.stubs(:course_admin?).returns(true) - assert_not @exercise.accessible?(@user, course) + assert_not @exercise.accessible?(@user, course: course) end test 'accessible? should return false if user is not course admin of course and exercise is not in course' do course = create :course, users: [@user] - assert_not @exercise.accessible?(@user, course) + assert_not @exercise.accessible?(@user, course: course) end test 'accessible? should return false if user is not course admin of course and series is not visible course' do course = create :course, users: [@user] - create :series, course: course, visibility: 'closed', exercises: [@exercise] + series = create :series, course: course, visibility: 'closed', exercises: [@exercise] - assert_not @exercise.accessible?(@user, course) + assert_not @exercise.accessible?(@user, course: course) + assert_not @exercise.accessible?(@user, course: course, series: series) + assert_not @exercise.accessible?(@user, series: series) end test 'accessible? should return true if user is course admin of course, repository admin and exercise is in course' do course = create :course, users: [@user] User.any_instance.stubs(:course_admin?).returns(true) User.any_instance.stubs(:repository_admin?).returns(true) - create :series, course: course, exercises: [@exercise] + series = create :series, course: course, exercises: [@exercise] - assert @exercise.accessible?(@user, course) + assert @exercise.accessible?(@user, course: course) + assert @exercise.accessible?(@user, course: course, series: series) + assert @exercise.accessible?(@user, series: series) end test 'accessible? should return true if user is repository admin and series is visible' do User.any_instance.stubs(:repository_admin?).returns(true) - create :series, course: @course, exercises: [@exercise] + series = create :series, course: @course, exercises: [@exercise] - assert @exercise.accessible?(@user, @course) + assert @exercise.accessible?(@user, course: @course) + assert @exercise.accessible?(@user, course: @course, series: series) + assert @exercise.accessible?(@user, series: series) end test 'accessible? should return false if not allowed to use exercise' do exercise = create :exercise, access: 'private' - create :series, course: @course, exercises: [exercise] + series = create :series, course: @course, exercises: [exercise] - assert_not exercise.accessible?(@user, @course) + assert_not exercise.accessible?(@user, course: @course) + assert_not exercise.accessible?(@user, course: @course, series: series) + assert_not @exercise.accessible?(@user, series: series) end test 'accessible? should return true if repository allows access to course' do exercise = create :exercise, access: :private - create :series, course: @course, exercises: [exercise] + series = create :series, course: @course, exercises: [exercise] exercise.repository.allowed_courses << @course - assert exercise.accessible?(@user, @course) + assert exercise.accessible?(@user, course: @course) + assert exercise.accessible?(@user, course: @course, series: series) + assert exercise.accessible?(@user, series: series) end test 'accessible? should return false if user is not a member of the course' do course = create :course, registration: 'closed' - create :series, course: course, exercises: [@exercise] + series = create :series, course: course, exercises: [@exercise] - assert_not @exercise.accessible?(@user, course) + assert_not @exercise.accessible?(@user, course: course) + assert_not @exercise.accessible?(@user, course: course, series: series) + assert_not @exercise.accessible?(@user, series: series) end test 'accessible? should return true if user is a member of the course' do course = create :course, users: [@user] - create :series, course: course, exercises: [@exercise] + series = create :series, course: course, exercises: [@exercise] - assert @exercise.accessible?(@user, course) + assert @exercise.accessible?(@user, course: course) + assert @exercise.accessible?(@user, course: course, series: series) + assert @exercise.accessible?(@user, series: series) end test 'accessible? should return true if user repository admin of repository' do exercise = create :exercise, access: 'private' User.any_instance.stubs(:repository_admin?).returns(true) - assert exercise.accessible?(@user, nil) + assert exercise.accessible?(@user) end test 'accessible? should return true if exercise is public' do - assert @exercise.accessible?(@user, nil) + assert @exercise.accessible?(@user) end test 'accessible? should return false if exercise is private' do exercise = build :exercise, access: 'private' - assert_not exercise.accessible?(@user, nil) + assert_not exercise.accessible?(@user) end test 'exercise should be accessible if private and included in unmoderated open course' do exercise = create :exercise, access: 'private' course = create :course, moderated: false, registration: :open_for_all exercise.repository.allowed_courses << course - create :series, course: course, exercises: [exercise] + series = create :series, course: course, exercises: [exercise] - assert exercise.accessible?(@user, course) + assert exercise.accessible?(@user, course: course) + assert exercise.accessible?(@user, course: course, series: series) + assert exercise.accessible?(@user, series: series) end test 'exercise should not be accessible if private and included in a moderated but open course' do exercise = create :exercise, access: 'private' course = create :course, moderated: true, registration: :open_for_all exercise.repository.allowed_courses << course - create :series, course: course, exercises: [exercise] + series = create :series, course: course, exercises: [exercise] - assert_not exercise.accessible?(@user, course) + assert_not exercise.accessible?(@user, course: course) + assert_not exercise.accessible?(@user, course: course, series: series) + assert_not exercise.accessible?(@user, series: series) end test 'exercise should not be accessible if included via hidden series and user is not a member' do - create :series, course: @course, exercises: [@exercise], visibility: :hidden + series = create :series, course: @course, exercises: [@exercise], visibility: :hidden - assert_not @exercise.accessible?(@user, @course) + assert_not @exercise.accessible?(@user, course: @course) + assert_not @exercise.accessible?(@user, course: @course, series: series) + assert_not @exercise.accessible?(@user, series: series) + end + + test 'exercise should only be accessible in series if included in series' do + series = create :series, course: @course, exercises: [] + second_series = create :series, course: @course, exercises: [@exercise] + + assert @exercise.accessible?(@user, course: @course) + assert_not @exercise.accessible?(@user, course: @course, series: series) + assert_not @exercise.accessible?(@user, series: series) + assert @exercise.accessible?(@user, course: @course, series: second_series) + assert @exercise.accessible?(@user, series: second_series) end test 'convert_visibility_to_access should convert "visible" to "public"' do diff --git a/test/models/submission_test.rb b/test/models/submission_test.rb index bfa9008e8d..ca15907bb2 100644 --- a/test/models/submission_test.rb +++ b/test/models/submission_test.rb @@ -14,6 +14,7 @@ # fs_key :string(24) # number :integer # annotated :boolean default(FALSE), not null +# series_id :integer # require 'test_helper' @@ -405,7 +406,7 @@ class SubmissionTest < ActiveSupport::TestCase series = course.series.second exercise = create :exercise series.exercises << exercise - submission = create :submission, exercise: exercise, course: course + submission = create :submission, exercise: exercise, course: course, series: series assert_equal series, submission.series end @@ -428,32 +429,6 @@ class SubmissionTest < ActiveSupport::TestCase assert_nil submission.series end - test 'if multiple series have the same exercise in the course, series should return any of them' do - course = create :course, series_count: 4 - exercise = create :exercise - course.series.first.exercises << exercise - course.series.second.exercises << exercise - submission = create :submission, exercise: exercise, course: course - - assert_includes [course.series.first, course.series.second], submission.series - end - - test 'if a series is hidden, it should not be returned as the series for this submission' do - course = create :course, series_count: 4 - exercise = create :exercise - course.series.first.exercises << exercise - course.series.second.exercises << exercise - submission = create :submission, exercise: exercise, course: course - - course.series.first.update!(visibility: :hidden) - - assert_equal course.series.second, submission.series - - course.series.second.update!(visibility: :hidden) - - assert_nil submission.series - end - test 'Annotations should be counted once an evaluation is released' do submission = create :submission, status: :correct, course: courses(:course1) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index b940f77a77..6c047ae0b2 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -866,7 +866,7 @@ def setup series = course.series.first series.exercises << create(:exercise) series.exercises << create(:exercise) - submission = create :wrong_submission, user: user, course: course, exercise: course.series.first.exercises.first + submission = create :wrong_submission, user: user, course: course, exercise: course.series.first.exercises.first, series: course.series.first assert_equal submission, user.jump_back_in.first[:submission] assert_equal submission.exercise, user.jump_back_in.first[:activity] @@ -908,7 +908,7 @@ def setup series.exercises << a1 series.exercises << a2 course.series.second.exercises << create(:exercise) - create :correct_submission, user: user, course: course, exercise: a1 + create :correct_submission, user: user, course: course, exercise: a1, series: series assert_nil user.jump_back_in.first[:submission] assert_equal a2, user.jump_back_in.first[:activity] @@ -939,7 +939,7 @@ def setup series1.exercises << a1 series1.exercises << a2 series2.exercises << create(:exercise) - create :correct_submission, user: user, course: course, exercise: a2 + create :correct_submission, user: user, course: course, exercise: a2, series: series1 assert_nil user.jump_back_in.first[:submission] assert_nil user.jump_back_in.first[:activity] @@ -1002,38 +1002,67 @@ def setup course = create :course, series_count: 2, exercises_per_series: 1 series = course.series.first - create :correct_submission, user: user, course: course, exercise: series.exercises.first + create :wrong_submission, user: user, course: course, exercise: series.exercises.first, series: series + + assert_equal course.series.first, user.jump_back_in.first[:series] + assert_equal course.series.second, user.jump_back_in.second[:series] + assert_equal 3, user.jump_back_in.count course.series.second.update(visibility: :hidden) - assert_empty user.jump_back_in + assert_equal 2, user.jump_back_in.count + assert_equal course.series.first, user.jump_back_in.first[:series] + assert_nil user.jump_back_in.second[:series] course.series.second.update(visibility: :closed) - assert_empty user.jump_back_in + assert_equal 2, user.jump_back_in.count + assert_equal course.series.first, user.jump_back_in.first[:series] + assert_nil user.jump_back_in.second[:series] + + course.series.first.update(visibility: :hidden) + + # no vissible series in course + assert_equal 0, user.jump_back_in.count course.series.second.update(visibility: :open) - assert_equal course.series.second, user.jump_back_in.first[:series] + # the series of the submission is hidden. thus only refer the course + assert_equal 1, user.jump_back_in.count + assert_nil user.jump_back_in.first[:submission] + assert_nil user.jump_back_in.first[:activity] + assert_nil user.jump_back_in.first[:series] + assert_equal course, user.jump_back_in.first[:course] + course.series.first.update(visibility: :open) CourseMembership.create user: user, course: course, status: :course_admin # Refetch user to fix cached course_admin? method user = User.find(user.id) assert user.course_admin?(course) + assert_equal course.series.first, user.jump_back_in.first[:series] + assert_equal course.series.second, user.jump_back_in.second[:series] course.series.second.update(visibility: :hidden) - assert_equal course.series.second, user.jump_back_in.first[:series] + assert_equal course.series.first, user.jump_back_in.first[:series] + assert_equal course.series.second, user.jump_back_in.second[:series] course.series.second.update(visibility: :closed) - assert_equal course.series.second, user.jump_back_in.first[:series] + assert_equal course.series.first, user.jump_back_in.first[:series] + assert_equal course.series.second, user.jump_back_in.second[:series] + + course.series.first.update(visibility: :hidden) + + assert_equal course.series.first, user.jump_back_in.first[:series] + assert_equal course.series.second, user.jump_back_in.second[:series] course.series.second.update(visibility: :open) - assert_equal course.series.second, user.jump_back_in.first[:series] + assert_equal course.series.first, user.jump_back_in.first[:series] + assert_equal course.series.second, user.jump_back_in.second[:series] end test 'jump back in should only return activities the user has access to' do @@ -1042,7 +1071,7 @@ def setup course = create :course, series_count: 2, exercises_per_series: 1 series = course.series.first a1 = series.exercises.first - s = create :wrong_submission, user: user, course: course, exercise: a1 + s = create :wrong_submission, user: user, course: course, exercise: a1, series: series assert_equal 3, user.jump_back_in.count assert_equal s, user.jump_back_in.first[:submission] @@ -1074,13 +1103,13 @@ def setup series.update(visibility: :hidden) - assert_equal 1, user.jump_back_in.count - assert_nil user.jump_back_in.first[:activity] + assert_equal 3, user.jump_back_in.count + assert_equal a1, user.jump_back_in.first[:activity] series.update(visibility: :closed) - assert_equal 1, user.jump_back_in.count - assert_nil user.jump_back_in.first[:activity] + assert_equal 3, user.jump_back_in.count + assert_equal a1, user.jump_back_in.first[:activity] series.update(visibility: :open)