From 8bc46231259d09d7498223781e78f55ec5864f2e Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Tue, 1 Oct 2024 14:24:41 +0200 Subject: [PATCH 1/5] Fix filtering on new saved annotation page --- .../saved_annotations_controller.rb | 4 ++-- app/policies/annotation_policy.rb | 3 +-- .../saved_annotation_controller_test.rb | 21 +++++++++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/app/controllers/saved_annotations_controller.rb b/app/controllers/saved_annotations_controller.rb index 06a6551648..6aef09a539 100644 --- a/app/controllers/saved_annotations_controller.rb +++ b/app/controllers/saved_annotations_controller.rb @@ -42,8 +42,8 @@ def new authorize SavedAnnotation @annotations = AnnotationPolicy::Scope.new(current_user, Annotation.all).resolve @annotations = @annotations.where(saved_annotation_id: nil).where(user_id: current_user.id) - @courses = Course.where(id: @annotations.joins(:submission).pluck('submissions.course_id').uniq) - @exercises = Activity.where(id: @annotations.joins(:submission).pluck('submissions.exercise_id').uniq) + @courses = Course.where(id: @annotations.joins('INNER JOIN submissions AS submission ON submission.id = annotations.submission_id').pluck('submission.course_id').uniq) + @exercises = Activity.where(id: @annotations.joins('INNER JOIN submissions AS submission ON submission.id = annotations.submission_id').pluck('submission.exercise_id').uniq) @annotations = apply_scopes(@annotations.order_by_created_at(:DESC)) .includes(:course).includes(:user).includes(:submission) .paginate(page: parse_pagination_param(params[:page]), per_page: parse_pagination_param(params[:per_page])) diff --git a/app/policies/annotation_policy.rb b/app/policies/annotation_policy.rb index 6fa65d1c96..e70da78894 100644 --- a/app/policies/annotation_policy.rb +++ b/app/policies/annotation_policy.rb @@ -4,8 +4,7 @@ def resolve if user&.zeus? scope.all elsif user - common = scope.joins(:submission) - common.released.where(submissions: { user: user }).or(common.where(submissions: { course_id: user.administrating_courses.map(&:id) })) + scope.released.where(submission: { user: user }).or(scope.where(submission: { course_id: user.administrating_courses.map(&:id) })) else scope.none end diff --git a/test/controllers/saved_annotation_controller_test.rb b/test/controllers/saved_annotation_controller_test.rb index 5132a21100..a99b186b1b 100644 --- a/test/controllers/saved_annotation_controller_test.rb +++ b/test/controllers/saved_annotation_controller_test.rb @@ -49,4 +49,25 @@ def setup assert_response :forbidden end + + test 'staff should be able to filter existing annotations on new page' do + get new_saved_annotation_path + + assert_response :success + + get new_saved_annotation_path, params: { exercise_id: 1 } + + assert_response :success + end + + test 'zeus should be able to filter existing annotations on new page' do + sign_in users(:zeus) + get new_saved_annotation_path + + assert_response :success + + get new_saved_annotation_path, params: { exercise_id: 1 } + + assert_response :success + end end From fb3f62028527d6fc215e1cd7b54867cc84156c36 Mon Sep 17 00:00:00 2001 From: jorg-vr Date: Tue, 8 Oct 2024 15:15:07 +0200 Subject: [PATCH 2/5] Fix tests --- app/controllers/saved_annotations_controller.rb | 4 ++-- app/policies/annotation_policy.rb | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/controllers/saved_annotations_controller.rb b/app/controllers/saved_annotations_controller.rb index 6aef09a539..10fac38979 100644 --- a/app/controllers/saved_annotations_controller.rb +++ b/app/controllers/saved_annotations_controller.rb @@ -42,8 +42,8 @@ def new authorize SavedAnnotation @annotations = AnnotationPolicy::Scope.new(current_user, Annotation.all).resolve @annotations = @annotations.where(saved_annotation_id: nil).where(user_id: current_user.id) - @courses = Course.where(id: @annotations.joins('INNER JOIN submissions AS submission ON submission.id = annotations.submission_id').pluck('submission.course_id').uniq) - @exercises = Activity.where(id: @annotations.joins('INNER JOIN submissions AS submission ON submission.id = annotations.submission_id').pluck('submission.exercise_id').uniq) + @courses = Course.where(id: @annotations.pluck('submission.course_id').uniq) + @exercises = Activity.where(id: @annotations.pluck('submission.exercise_id').uniq) @annotations = apply_scopes(@annotations.order_by_created_at(:DESC)) .includes(:course).includes(:user).includes(:submission) .paginate(page: parse_pagination_param(params[:page]), per_page: parse_pagination_param(params[:per_page])) diff --git a/app/policies/annotation_policy.rb b/app/policies/annotation_policy.rb index e70da78894..28ea358662 100644 --- a/app/policies/annotation_policy.rb +++ b/app/policies/annotation_policy.rb @@ -1,12 +1,13 @@ class AnnotationPolicy < ApplicationPolicy class Scope < ApplicationPolicy::Scope def resolve + annotations = scope.joins('INNER JOIN submissions AS submission ON submission.id = annotations.submission_id') if user&.zeus? - scope.all + annotations.all elsif user - scope.released.where(submission: { user: user }).or(scope.where(submission: { course_id: user.administrating_courses.map(&:id) })) + annotations.released.where(submission: { user: user }).or(annotations.where(submission: { course_id: user.administrating_courses.map(&:id) })) else - scope.none + annotations.none end end end From 5cc5dd7cd85f68e4792dbcca343c2609233f4ac0 Mon Sep 17 00:00:00 2001 From: "jorg.vr" Date: Thu, 24 Oct 2024 17:27:26 +0200 Subject: [PATCH 3/5] Remove explicit sql --- app/controllers/auth/omniauth_callbacks_controller.rb | 1 + app/controllers/saved_annotations_controller.rb | 5 +++-- app/models/event.rb | 2 +- app/policies/annotation_policy.rb | 8 ++++---- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/controllers/auth/omniauth_callbacks_controller.rb b/app/controllers/auth/omniauth_callbacks_controller.rb index 0507db299c..45944b0105 100644 --- a/app/controllers/auth/omniauth_callbacks_controller.rb +++ b/app/controllers/auth/omniauth_callbacks_controller.rb @@ -235,6 +235,7 @@ def find_identity_by_uid identity = Identity.joins(:user).find_by(user: { first_name: auth_hash.info.first_name, last_name: auth_hash.info.last_name }, provider: provider, identifier_based_on_email: true) if identity.nil? return nil if identity.nil? + Event.new # Update the identifier to the new uid identity.update(identifier: auth_uid, identifier_based_on_email: false) elsif provider.class.sym == :smartschool && auth_username.present? diff --git a/app/controllers/saved_annotations_controller.rb b/app/controllers/saved_annotations_controller.rb index 10fac38979..99adf95528 100644 --- a/app/controllers/saved_annotations_controller.rb +++ b/app/controllers/saved_annotations_controller.rb @@ -42,8 +42,9 @@ def new authorize SavedAnnotation @annotations = AnnotationPolicy::Scope.new(current_user, Annotation.all).resolve @annotations = @annotations.where(saved_annotation_id: nil).where(user_id: current_user.id) - @courses = Course.where(id: @annotations.pluck('submission.course_id').uniq) - @exercises = Activity.where(id: @annotations.pluck('submission.exercise_id').uniq) + submissions = Submission.where(id: @annotations.map(&:submission_id).uniq) + @courses = Course.where(id: submissions.map(&:course_id).uniq) + @exercises = Activity.where(id: submissions.map(&:exercise_id).uniq) @annotations = apply_scopes(@annotations.order_by_created_at(:DESC)) .includes(:course).includes(:user).includes(:submission) .paginate(page: parse_pagination_param(params[:page]), per_page: parse_pagination_param(params[:per_page])) diff --git a/app/models/event.rb b/app/models/event.rb index e06dd8b847..a905251810 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -11,7 +11,7 @@ # class Event < ApplicationRecord - enum :event_type, { rejudge: 0, permission_change: 1, exercise_repository: 2, error: 3 } + enum :event_type, { rejudge: 0, permission_change: 1, exercise_repository: 2, error: 3, other: 4} belongs_to :user, optional: true validates :event_type, presence: true diff --git a/app/policies/annotation_policy.rb b/app/policies/annotation_policy.rb index 28ea358662..1045da391d 100644 --- a/app/policies/annotation_policy.rb +++ b/app/policies/annotation_policy.rb @@ -1,13 +1,13 @@ class AnnotationPolicy < ApplicationPolicy class Scope < ApplicationPolicy::Scope def resolve - annotations = scope.joins('INNER JOIN submissions AS submission ON submission.id = annotations.submission_id') if user&.zeus? - annotations.all + scope.all elsif user - annotations.released.where(submission: { user: user }).or(annotations.where(submission: { course_id: user.administrating_courses.map(&:id) })) + common = scope.joins(:submission) + common.released.where(submission: { user: user }).or(common.where(submission: { course_id: user.administrating_courses.map(&:id) })) else - annotations.none + scope.none end end end From 40633be94fb5f8e57a8880d6ea64cc740949a66c Mon Sep 17 00:00:00 2001 From: "jorg.vr" Date: Thu, 24 Oct 2024 17:28:39 +0200 Subject: [PATCH 4/5] Undo unwanted changes --- app/controllers/auth/omniauth_callbacks_controller.rb | 1 - app/models/event.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/auth/omniauth_callbacks_controller.rb b/app/controllers/auth/omniauth_callbacks_controller.rb index 45944b0105..0507db299c 100644 --- a/app/controllers/auth/omniauth_callbacks_controller.rb +++ b/app/controllers/auth/omniauth_callbacks_controller.rb @@ -235,7 +235,6 @@ def find_identity_by_uid identity = Identity.joins(:user).find_by(user: { first_name: auth_hash.info.first_name, last_name: auth_hash.info.last_name }, provider: provider, identifier_based_on_email: true) if identity.nil? return nil if identity.nil? - Event.new # Update the identifier to the new uid identity.update(identifier: auth_uid, identifier_based_on_email: false) elsif provider.class.sym == :smartschool && auth_username.present? diff --git a/app/models/event.rb b/app/models/event.rb index a905251810..e06dd8b847 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -11,7 +11,7 @@ # class Event < ApplicationRecord - enum :event_type, { rejudge: 0, permission_change: 1, exercise_repository: 2, error: 3, other: 4} + enum :event_type, { rejudge: 0, permission_change: 1, exercise_repository: 2, error: 3 } belongs_to :user, optional: true validates :event_type, presence: true From 2e997f8934af41fefe1d107274bc2c25b59de9e3 Mon Sep 17 00:00:00 2001 From: "jorg.vr" Date: Thu, 24 Oct 2024 17:41:33 +0200 Subject: [PATCH 5/5] Use pluck where posible --- app/controllers/saved_annotations_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/saved_annotations_controller.rb b/app/controllers/saved_annotations_controller.rb index 99adf95528..d74155918b 100644 --- a/app/controllers/saved_annotations_controller.rb +++ b/app/controllers/saved_annotations_controller.rb @@ -42,9 +42,9 @@ def new authorize SavedAnnotation @annotations = AnnotationPolicy::Scope.new(current_user, Annotation.all).resolve @annotations = @annotations.where(saved_annotation_id: nil).where(user_id: current_user.id) - submissions = Submission.where(id: @annotations.map(&:submission_id).uniq) - @courses = Course.where(id: submissions.map(&:course_id).uniq) - @exercises = Activity.where(id: submissions.map(&:exercise_id).uniq) + submissions = Submission.where(id: @annotations.pluck(:submission_id).uniq) + @courses = Course.where(id: submissions.pluck(:course_id).uniq) + @exercises = Activity.where(id: submissions.pluck(:exercise_id).uniq) @annotations = apply_scopes(@annotations.order_by_created_at(:DESC)) .includes(:course).includes(:user).includes(:submission) .paginate(page: parse_pagination_param(params[:page]), per_page: parse_pagination_param(params[:per_page]))