From 872c8be6197bdd534b388708732768ab0f089c69 Mon Sep 17 00:00:00 2001 From: ichandrasharma Date: Sat, 1 Feb 2025 23:29:45 +0530 Subject: [PATCH 01/10] Fix: Display specific error message for course slug collision revision --- app/assets/javascripts/utils/api.js | 4 ++++ app/controllers/courses_controller.rb | 6 +++++- app/models/course.rb | 4 ++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/utils/api.js b/app/assets/javascripts/utils/api.js index 06c96f5b9a..63b4a257b4 100644 --- a/app/assets/javascripts/utils/api.js +++ b/app/assets/javascripts/utils/api.js @@ -321,6 +321,10 @@ const API = { }); if (!response.ok) { + if (response.status === 422 || response.status === 400) { + // Check for 422 or 400 + const errorMessage = 'Slug must be unique, This course slug is already in use. Please choose a different one.'; + throw { message: errorMessage }}; const data = await response.text(); this.obj = data; this.status = response.statusText; diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index ded7925149..69e0f0bd16 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -44,13 +44,17 @@ def update validate handle_course_announcement(@course.instructors.first) slug_from_params if should_set_slug? - @course.update update_params + if @course.update update_params update_courses_wikis update_course_wiki_namespaces update_flags ensure_passcode_set UpdateCourseWorker.schedule_edits(course: @course, editing_user: current_user) render json: { course: @course } + else + # Handle update failures by checking for validation errors, such as a duplicate slug. + render json: { error: @course.errors.full_messages.first }, status: :unprocessable_entity + end rescue Wiki::InvalidWikiError => e message = I18n.t('courses.error.invalid_wiki', domain: e.domain) render json: { errors: e, message: }, diff --git a/app/models/course.rb b/app/models/course.rb index 02e50ddfe4..4f1796f4c9 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -60,6 +60,10 @@ class Course < ApplicationRecord ###################### # Users for a course # ###################### + + # Validations for slug to avoid duplicacy + validates :slug, uniqueness: { message: '' } + has_many :courses_users, class_name: 'CoursesUsers', dependent: :destroy has_many :users, -> { distinct }, through: :courses_users has_many :students, -> { where('courses_users.role = 0') }, From 32e29a5bcf787d045399d3e5cdcc6bca4cab9c42 Mon Sep 17 00:00:00 2001 From: ichandrasharma Date: Sat, 8 Feb 2025 04:44:40 +0530 Subject: [PATCH 02/10] Fix: Error message for course slug collision JSON error response shown --- app/assets/javascripts/utils/api.js | 11 ++++++----- app/controllers/courses_controller.rb | 9 ++++++--- app/models/course.rb | 23 +++++++++++++++++++---- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/utils/api.js b/app/assets/javascripts/utils/api.js index 63b4a257b4..a18b92d946 100644 --- a/app/assets/javascripts/utils/api.js +++ b/app/assets/javascripts/utils/api.js @@ -321,10 +321,11 @@ const API = { }); if (!response.ok) { - if (response.status === 422 || response.status === 400) { - // Check for 422 or 400 - const errorMessage = 'Slug must be unique, This course slug is already in use. Please choose a different one.'; - throw { message: errorMessage }}; + if (response.status === 422 ) { + const data = await response.json(); + const errorMessage = data.errors?.[0] || 'Unknown error'; + throw new Error(errorMessage);} + else { const data = await response.text(); this.obj = data; this.status = response.statusText; @@ -336,7 +337,7 @@ const API = { }); response.responseText = data; throw response; - } + }} return response.json(); }, diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index 69e0f0bd16..dcf824f3af 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -51,10 +51,13 @@ def update ensure_passcode_set UpdateCourseWorker.schedule_edits(course: @course, editing_user: current_user) render json: { course: @course } - else - # Handle update failures by checking for validation errors, such as a duplicate slug. - render json: { error: @course.errors.full_messages.first }, status: :unprocessable_entity + else + render json: { errors: @course.errors.full_messages }, status: :unprocessable_entity end + rescue Course::DuplicateCourseSlugError => e + message = I18n.t('courses.error.duplicate_slug', slug: e.slug) + render json: { errors: [message] }, status: :unprocessable_entity + rescue Wiki::InvalidWikiError => e message = I18n.t('courses.error.invalid_wiki', domain: e.domain) render json: { errors: e, message: }, diff --git a/app/models/course.rb b/app/models/course.rb index 4f1796f4c9..70939c1faa 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -60,10 +60,6 @@ class Course < ApplicationRecord ###################### # Users for a course # ###################### - - # Validations for slug to avoid duplicacy - validates :slug, uniqueness: { message: '' } - has_many :courses_users, class_name: 'CoursesUsers', dependent: :destroy has_many :users, -> { distinct }, through: :courses_users has_many :students, -> { where('courses_users.role = 0') }, @@ -597,4 +593,23 @@ def check_course_times def set_needs_update self.needs_update = true if start_changed? end + + # Handle duplicate slug collision + validate :check_duplicate_slug, on: :update + + def check_duplicate_slug + if Course.where(slug: slug).where.not(id: id).exists? + raise DuplicateCourseSlugError.new(slug) + end + end + + class DuplicateCourseSlugError < StandardError + def initialize(slug, msg = 'Duplicate Slug') + @msg = msg + @slug = slug + super('#{msg}: #{slug}') + end + + attr_reader :slug + end end From c8d140dc3641a52ce4112a606196b4da54931d56 Mon Sep 17 00:00:00 2001 From: ichandrasharma Date: Tue, 11 Feb 2025 14:25:20 +0530 Subject: [PATCH 03/10] remove special handling in JS --- app/assets/javascripts/utils/api.js | 19 +++++++++---------- app/controllers/courses_controller.rb | 8 +++----- app/models/course.rb | 2 +- 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/utils/api.js b/app/assets/javascripts/utils/api.js index a18b92d946..6e596df7be 100644 --- a/app/assets/javascripts/utils/api.js +++ b/app/assets/javascripts/utils/api.js @@ -321,23 +321,22 @@ const API = { }); if (!response.ok) { - if (response.status === 422 ) { - const data = await response.json(); - const errorMessage = data.errors?.[0] || 'Unknown error'; - throw new Error(errorMessage);} - else { const data = await response.text(); this.obj = data; this.status = response.statusText; SentryLogger.obj = this.obj; SentryLogger.status = this.status; - Sentry.captureMessage('saveCourse failed', { - level: 'error', - extra: SentryLogger - }); + try { + Sentry.captureMessage('saveCourse failed', { + level: 'error', + extra: SentryLogger + }); + } catch (error) { + console.error("Sentry.captureMessage failed:", error); + }; response.responseText = data; throw response; - }} + } return response.json(); }, diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index dcf824f3af..37e465b822 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -44,19 +44,17 @@ def update validate handle_course_announcement(@course.instructors.first) slug_from_params if should_set_slug? - if @course.update update_params + @course.update update_params update_courses_wikis update_course_wiki_namespaces update_flags ensure_passcode_set UpdateCourseWorker.schedule_edits(course: @course, editing_user: current_user) render json: { course: @course } - else - render json: { errors: @course.errors.full_messages }, status: :unprocessable_entity - end rescue Course::DuplicateCourseSlugError => e message = I18n.t('courses.error.duplicate_slug', slug: e.slug) - render json: { errors: [message] }, status: :unprocessable_entity + render json: { errors: e, message: }, + status: :unprocessable_entity rescue Wiki::InvalidWikiError => e message = I18n.t('courses.error.invalid_wiki', domain: e.domain) diff --git a/app/models/course.rb b/app/models/course.rb index 70939c1faa..57d62f679f 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -607,7 +607,7 @@ class DuplicateCourseSlugError < StandardError def initialize(slug, msg = 'Duplicate Slug') @msg = msg @slug = slug - super('#{msg}: #{slug}') + super(msg) end attr_reader :slug From 1bf6f90b62d6a6b3a9148cdfb60a5272375c61f6 Mon Sep 17 00:00:00 2001 From: ichandrasharma Date: Wed, 12 Feb 2025 17:25:24 +0530 Subject: [PATCH 04/10] fixed linting --- app/controllers/courses_controller.rb | 6 +++--- app/models/course.rb | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index 37e465b822..e9d0cd1293 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -52,9 +52,9 @@ def update UpdateCourseWorker.schedule_edits(course: @course, editing_user: current_user) render json: { course: @course } rescue Course::DuplicateCourseSlugError => e - message = I18n.t('courses.error.duplicate_slug', slug: e.slug) - render json: { errors: e, message: }, - status: :unprocessable_entity + message = I18n.t('courses.error.duplicate_slug', slug: e.slug) + render json: { errors: e, message: message }, + status: :unprocessable_entity rescue Wiki::InvalidWikiError => e message = I18n.t('courses.error.invalid_wiki', domain: e.domain) diff --git a/app/models/course.rb b/app/models/course.rb index 57d62f679f..8422001c79 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -601,15 +601,15 @@ def check_duplicate_slug if Course.where(slug: slug).where.not(id: id).exists? raise DuplicateCourseSlugError.new(slug) end - end + end class DuplicateCourseSlugError < StandardError + attr_reader :slug + def initialize(slug, msg = 'Duplicate Slug') - @msg = msg @slug = slug super(msg) end - - attr_reader :slug end + end From b52397372ec47a25902e5dab799554faacb330a1 Mon Sep 17 00:00:00 2001 From: ichandrasharma Date: Wed, 12 Feb 2025 17:57:42 +0530 Subject: [PATCH 05/10] JS linting fixed --- app/assets/javascripts/utils/api.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/utils/api.js b/app/assets/javascripts/utils/api.js index 6e596df7be..02f90a4581 100644 --- a/app/assets/javascripts/utils/api.js +++ b/app/assets/javascripts/utils/api.js @@ -332,7 +332,7 @@ const API = { extra: SentryLogger }); } catch (error) { - console.error("Sentry.captureMessage failed:", error); + console.error('Sentry.captureMessage failed:', error); }; response.responseText = data; throw response; From dbc8060ed4e8daa0d0e9591890467caf9a663086 Mon Sep 17 00:00:00 2001 From: ichandrasharma Date: Wed, 12 Feb 2025 18:04:29 +0530 Subject: [PATCH 06/10] JS linting fixed --- app/assets/javascripts/utils/api.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/utils/api.js b/app/assets/javascripts/utils/api.js index 02f90a4581..bedf350acf 100644 --- a/app/assets/javascripts/utils/api.js +++ b/app/assets/javascripts/utils/api.js @@ -333,7 +333,7 @@ const API = { }); } catch (error) { console.error('Sentry.captureMessage failed:', error); - }; + } response.responseText = data; throw response; } From 59387ac30c1d81c71f6b57c8892723b49480a9bb Mon Sep 17 00:00:00 2001 From: ichandrasharma Date: Wed, 12 Feb 2025 19:43:56 +0530 Subject: [PATCH 07/10] rubocop linting fixed --- app/controllers/courses_controller.rb | 14 +++++++------- app/models/course.rb | 9 ++++----- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index e9d0cd1293..211f9682c8 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -52,14 +52,14 @@ def update UpdateCourseWorker.schedule_edits(course: @course, editing_user: current_user) render json: { course: @course } rescue Course::DuplicateCourseSlugError => e - message = I18n.t('courses.error.duplicate_slug', slug: e.slug) - render json: { errors: e, message: message }, - status: :unprocessable_entity - + render_error(:unprocessable_entity, 'courses.error.duplicate_slug', slug: e.slug) rescue Wiki::InvalidWikiError => e - message = I18n.t('courses.error.invalid_wiki', domain: e.domain) - render json: { errors: e, message: }, - status: :not_found + render_error(:not_found, 'courses.error.invalid_wiki', domain: e.domain) + end + + def render_error(status, error_key, **params) + render json: { errors: params, message: I18n.t(error_key, **params) }, + status: end def destroy diff --git a/app/models/course.rb b/app/models/course.rb index 8422001c79..1d40f5906b 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -598,18 +598,17 @@ def set_needs_update validate :check_duplicate_slug, on: :update def check_duplicate_slug - if Course.where(slug: slug).where.not(id: id).exists? - raise DuplicateCourseSlugError.new(slug) - end + return unless Course.where(slug:).where.not(id:).exists? + + raise DuplicateCourseSlugError, slug end class DuplicateCourseSlugError < StandardError attr_reader :slug - + def initialize(slug, msg = 'Duplicate Slug') @slug = slug super(msg) end end - end From da3230758cd37fc8336646cbc9f7a12ee807353f Mon Sep 17 00:00:00 2001 From: ichandrasharma Date: Thu, 13 Feb 2025 14:29:35 +0530 Subject: [PATCH 08/10] rspec tests added --- spec/models/course_spec.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index eec826e0b9..b3d2b7a078 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -865,6 +865,26 @@ end end + describe 'validations' do + let!(:existing_course) { create(:course, slug: 'existing-slug') } + + context 'when updating a course' do + let!(:duplicate_course) { create(:course, slug: 'duplicate-slug') } + + it 'raises Course::DuplicateCourseSlugError if the slug is duplicated' do + # Attempting to update the course with an existing slug should raise an error + expect do + duplicate_course.update!(slug: existing_course.slug) + end.to raise_error(Course::DuplicateCourseSlugError, 'Duplicate Slug') + end + + it 'does not raise an error if the slug is unique' do + # Updating the course with a unique slug should pass without errors + expect { existing_course.update!(slug: 'new-unique-slug') }.not_to raise_error + end + end + end + describe '#approved' do let(:course) { create(:course) } From 667b85af8ec117d8e46f4617966931c9beefc2f2 Mon Sep 17 00:00:00 2001 From: ichandrasharma Date: Sat, 15 Feb 2025 10:18:12 +0530 Subject: [PATCH 09/10] fix rspec test suite error --- app/models/course.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/course.rb b/app/models/course.rb index 1d40f5906b..7b6ea21c44 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -598,7 +598,7 @@ def set_needs_update validate :check_duplicate_slug, on: :update def check_duplicate_slug - return unless Course.where(slug:).where.not(id:).exists? + return unless Course.find_by(slug: @slug).present? raise DuplicateCourseSlugError, slug end From f9fdb98bcb506553d6c6532f46ed33059ca2e333 Mon Sep 17 00:00:00 2001 From: ichandrasharma Date: Sun, 16 Feb 2025 12:27:20 +0530 Subject: [PATCH 10/10] fix rspec test suite error --- app/models/course.rb | 2 +- spec/lib/revision_stat_spec.rb | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/models/course.rb b/app/models/course.rb index 7b6ea21c44..6507c84d3a 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -598,7 +598,7 @@ def set_needs_update validate :check_duplicate_slug, on: :update def check_duplicate_slug - return unless Course.find_by(slug: @slug).present? + return unless Course.where(slug: slug).where.not(id: id).exists? raise DuplicateCourseSlugError, slug end diff --git a/spec/lib/revision_stat_spec.rb b/spec/lib/revision_stat_spec.rb index 58f816d04c..c62da37685 100644 --- a/spec/lib/revision_stat_spec.rb +++ b/spec/lib/revision_stat_spec.rb @@ -49,7 +49,10 @@ context 'course id' do context 'not for this course' do - before { course.update(id: 1000) } + before do + allow_any_instance_of(Course).to receive(:check_duplicate_slug).and_return(true) + course.update(id: 1000) + end it 'does not include in scope' do expect(subject).to eq(0)