From a0042a9291d708afc8744acc5bf07f55e30fd46e Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Mon, 21 Oct 2024 20:21:45 +0800 Subject: [PATCH 01/11] feat(student-course): Add soft-delete for student removal from course --- Gemfile | 3 ++ Gemfile.lock | 3 ++ .../course/enrol_requests_controller.rb | 23 +++++++-- app/models/course.rb | 2 +- app/models/course/achievement.rb | 2 +- app/models/course/experience_points_record.rb | 1 + app/models/course/group_user.rb | 2 +- app/models/course/learning_rate_record.rb | 1 + app/models/course/personal_time.rb | 2 +- app/models/course/user_achievement.rb | 2 +- .../course/user_email_unsubscription.rb | 1 + app/models/course_user.rb | 26 ++++++++++ ...e_for_course_user_and_associated_models.rb | 48 +++++++++++++++++++ db/schema.rb | 20 +++++++- 14 files changed, 127 insertions(+), 9 deletions(-) create mode 100644 db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb diff --git a/Gemfile b/Gemfile index 2d1872bf20f..e5ae88c5706 100644 --- a/Gemfile +++ b/Gemfile @@ -14,6 +14,9 @@ gem 'tzinfo-data', platforms: [:mswin, :mswin64] # Bundle edge Rails instead: gem 'rails', github: 'rails/rails' gem 'rails', '~> 7.2.1' +# Soft delete +gem 'paranoia' + # Use PostgreSQL for the backend gem 'pg' diff --git a/Gemfile.lock b/Gemfile.lock index 8aa746d7e73..3c45f2a78a6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -367,6 +367,8 @@ GEM parallel (1.26.3) parallel_tests (4.7.2) parallel + paranoia (3.0.0) + activerecord (>= 6, < 8.1) parser (3.3.5.0) ast (~> 2.4.1) racc @@ -659,6 +661,7 @@ DEPENDENCIES nokogiri (>= 1.8.1) ostruct parallel_tests + paranoia pg puma rack-cors diff --git a/app/controllers/course/enrol_requests_controller.rb b/app/controllers/course/enrol_requests_controller.rb index 6640eb73c4a..d82e5a7bbc7 100644 --- a/app/controllers/course/enrol_requests_controller.rb +++ b/app/controllers/course/enrol_requests_controller.rb @@ -54,9 +54,26 @@ def reject private def create_course_user - course_user = CourseUser.new(course_user_params. - reverse_merge(course: current_course, user_id: @enrol_request.user_id, - timeline_algorithm: current_course.default_timeline_algorithm)) + existing_course_user = CourseUser.only_deleted.find_by(course_id: current_course, user_id: @enrol_request.user_id) + return restore_existing_course_user(existing_course_user) if existing_course_user + + create_new_course_user + end + + def restore_existing_course_user(existing_course_user) + existing_course_user.restore + @enrol_request.update(approve: true) + existing_course_user + end + + def create_new_course_user + course_user = CourseUser.new( + course_user_params.reverse_merge( + course: current_course, + user_id: @enrol_request.user_id, + timeline_algorithm: current_course.default_timeline_algorithm + ) + ) CourseUser.transaction do raise ActiveRecord::Rollback unless course_user.save && @enrol_request.update(approve: true) diff --git a/app/models/course.rb b/app/models/course.rb index f4cc5c01b4b..3d63dab9f63 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -5,7 +5,7 @@ class Course < ApplicationRecord include Course::CourseComponentsConcern include TimeZoneConcern include Generic::CollectionConcern - + acts_as_paranoid acts_as_tenant :instance, inverse_of: :courses has_settings_on :settings mount_uploader :logo, ImageUploader diff --git a/app/models/course/achievement.rb b/app/models/course/achievement.rb index 3e52d223271..98a737a34dd 100644 --- a/app/models/course/achievement.rb +++ b/app/models/course/achievement.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Course::Achievement < ApplicationRecord include Course::SanitizeDescriptionConcern - + acts_as_paranoid acts_as_conditional mount_uploader :badge, ImageUploader has_many_attachments on: :description diff --git a/app/models/course/experience_points_record.rb b/app/models/course/experience_points_record.rb index 0645770ae36..d9c8c0e79ea 100644 --- a/app/models/course/experience_points_record.rb +++ b/app/models/course/experience_points_record.rb @@ -2,6 +2,7 @@ class Course::ExperiencePointsRecord < ApplicationRecord include Generic::CollectionConcern actable optional: true + acts_as_paranoid before_save :send_notification, if: :reached_new_level? before_create :set_awarded_attributes, if: :manually_awarded? diff --git a/app/models/course/group_user.rb b/app/models/course/group_user.rb index 34a92f1df2b..d38cf352061 100644 --- a/app/models/course/group_user.rb +++ b/app/models/course/group_user.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Course::GroupUser < ApplicationRecord after_initialize :set_defaults, if: :new_record? - + acts_as_paranoid enum :role, { normal: 0, manager: 1 } validate :course_user_and_group_in_same_course diff --git a/app/models/course/learning_rate_record.rb b/app/models/course/learning_rate_record.rb index 5c4756285a1..1b0edd01592 100644 --- a/app/models/course/learning_rate_record.rb +++ b/app/models/course/learning_rate_record.rb @@ -6,6 +6,7 @@ class Course::LearningRateRecord < ApplicationRecord validates :effective_max, presence: true, numericality: true validates :course_user, presence: true validate :learning_rate_between_effective_min_and_max + acts_as_paranoid belongs_to :course_user, inverse_of: :learning_rate_records diff --git a/app/models/course/personal_time.rb b/app/models/course/personal_time.rb index 1b6d14745df..a1cd4719df0 100644 --- a/app/models/course/personal_time.rb +++ b/app/models/course/personal_time.rb @@ -2,7 +2,7 @@ class Course::PersonalTime < ApplicationRecord belongs_to :course_user, inverse_of: :personal_times belongs_to :lesson_plan_item, class_name: 'Course::LessonPlan::Item', inverse_of: :personal_times - + acts_as_paranoid validates :start_at, presence: true validates :course_user, presence: true, uniqueness: { scope: :lesson_plan_item } validates :lesson_plan_item, presence: true diff --git a/app/models/course/user_achievement.rb b/app/models/course/user_achievement.rb index d36c018ad4f..7bb3d72f366 100644 --- a/app/models/course/user_achievement.rb +++ b/app/models/course/user_achievement.rb @@ -2,7 +2,7 @@ class Course::UserAchievement < ApplicationRecord after_initialize :set_defaults, if: :new_record? after_create :send_notification - + acts_as_paranoid validate :validate_course_user_in_course, on: :create validates :obtained_at, presence: true validates :course_user_id, uniqueness: { scope: [:achievement_id], allow_nil: true, diff --git a/app/models/course/user_email_unsubscription.rb b/app/models/course/user_email_unsubscription.rb index 625a1b02d74..2881472014f 100644 --- a/app/models/course/user_email_unsubscription.rb +++ b/app/models/course/user_email_unsubscription.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true class Course::UserEmailUnsubscription < ApplicationRecord validates :course_user, presence: true + acts_as_paranoid belongs_to :course_user, inverse_of: :email_unsubscriptions belongs_to :course_setting_email, class_name: 'Course::Settings::Email', diff --git a/app/models/course_user.rb b/app/models/course_user.rb index 2f27ad966ab..8e33cf99752 100644 --- a/app/models/course_user.rb +++ b/app/models/course_user.rb @@ -3,6 +3,7 @@ class CourseUser < ApplicationRecord include CourseUser::StaffConcern include CourseUser::LevelProgressConcern include CourseUser::TodoConcern + acts_as_paranoid after_initialize :set_defaults, if: :new_record? before_validation :set_defaults, if: :new_record? @@ -23,6 +24,15 @@ class CourseUser < ApplicationRecord # A set of roles which comprise the managers of a course. MANAGER_ROLES = Set[:manager, :owner].map { |v| roles[v] }.freeze + ASSOCIATED_MODELS = %i[ + experience_points_records + learning_rate_records + course_user_achievements + email_unsubscriptions + group_users + personal_times + ].freeze + validates :role, presence: true validates :name, length: { maximum: 255 }, presence: true validates :phantom, inclusion: { in: [true, false] } @@ -176,6 +186,22 @@ def self.user?(user) all.exists?(user: user) end + # Soft delete the course user. + def destroy + ASSOCIATED_MODELS.each do |association| + send(association).destroy_all + end + super + end + + # Restore the course user. + def restore + super + ASSOCIATED_MODELS.each do |association| + send(association).only_deleted.each(&:restore) + end + end + # Test whether this course_user is a manager (i.e. manager or owner) # # @return [Boolean] True if course_user is a staff diff --git a/db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb b/db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb new file mode 100644 index 00000000000..6dbf5288f43 --- /dev/null +++ b/db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb @@ -0,0 +1,48 @@ +class ApplySoftDeleteForCourseUserAndAssociatedModels1 < ActiveRecord::Migration[7.2] + def change + change_table :course_users do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:course_users, :deleted_at) + t.index :deleted_at unless index_exists?(:course_users, :deleted_at) + end + + change_table :course_experience_points_records do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:course_experience_points_records, :deleted_at) + t.index :deleted_at unless index_exists?(:course_experience_points_records, :deleted_at) + end + + change_table :course_learning_rate_records do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:course_learning_rate_records, :deleted_at) + t.index :deleted_at unless index_exists?(:course_learning_rate_records, :deleted_at) + end + + change_table :course_user_achievements do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:course_user_achievements, :deleted_at) + t.index :deleted_at unless index_exists?(:course_user_achievements, :deleted_at) + end + + change_table :course_user_email_unsubscriptions do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:course_user_email_unsubscriptions, :deleted_at) + t.index :deleted_at unless index_exists?(:course_user_email_unsubscriptions, :deleted_at) + end + + change_table :course_group_users do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:course_group_users, :deleted_at) + t.index :deleted_at unless index_exists?(:course_group_users, :deleted_at) + end + + change_table :course_personal_times do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:course_personal_times, :deleted_at) + t.index :deleted_at unless index_exists?(:course_personal_times, :deleted_at) + end + + change_table :courses do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:courses, :deleted_at) + t.index :deleted_at unless index_exists?(:courses, :deleted_at) + end + + change_table :course_achievements do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:course_achievements, :deleted_at) + t.index :deleted_at unless index_exists?(:course_achievements, :deleted_at) + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 31c418521e5..d9f6870259e 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.2].define(version: 2024_09_17_170847) do +ActiveRecord::Schema[7.2].define(version: 2024_10_21_110207) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" enable_extension "uuid-ossp" @@ -68,8 +68,10 @@ t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false t.integer "satisfiability_type", default: 0 + t.datetime "deleted_at" t.index ["course_id"], name: "fk__course_achievements_course_id" t.index ["creator_id"], name: "fk__course_achievements_creator_id" + t.index ["deleted_at"], name: "index_course_achievements_on_deleted_at" t.index ["updater_id"], name: "fk__course_achievements_updater_id" end @@ -633,10 +635,12 @@ t.integer "draft_points_awarded" t.datetime "awarded_at", precision: nil t.integer "awarder_id" + t.datetime "deleted_at" t.index ["actable_type", "actable_id"], name: "index_course_experience_points_records_on_actable", unique: true t.index ["awarder_id"], name: "fk__course_experience_points_records_awarder_id" t.index ["course_user_id"], name: "fk__course_experience_points_records_course_user_id" t.index ["creator_id"], name: "fk__course_experience_points_records_creator_id" + t.index ["deleted_at"], name: "index_course_experience_points_records_on_deleted_at" t.index ["updater_id"], name: "fk__course_experience_points_records_updater_id" end @@ -714,9 +718,11 @@ t.integer "updater_id", null: false t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false + t.datetime "deleted_at" t.index ["course_user_id", "group_id"], name: "index_course_group_users_on_course_user_id_and_course_group_id", unique: true t.index ["course_user_id"], name: "fk__course_group_users_course_user_id" t.index ["creator_id"], name: "fk__course_group_users_creator_id" + t.index ["deleted_at"], name: "index_course_group_users_on_deleted_at" t.index ["group_id"], name: "fk__course_group_users_course_group_id" t.index ["updater_id"], name: "fk__course_group_users_updater_id" end @@ -749,7 +755,9 @@ t.float "effective_max", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.datetime "deleted_at" t.index ["course_user_id"], name: "fk__course_learning_rate_records_course_user_id" + t.index ["deleted_at"], name: "index_course_learning_rate_records_on_deleted_at" end create_table "course_lesson_plan_event_materials", id: :serial, force: :cascade do |t| @@ -909,7 +917,9 @@ t.datetime "bonus_end_at", precision: nil t.datetime "end_at", precision: nil t.boolean "fixed", default: false, null: false + t.datetime "deleted_at" t.index ["course_user_id"], name: "index_course_personal_times_on_course_user_id" + t.index ["deleted_at"], name: "index_course_personal_times_on_deleted_at" t.index ["lesson_plan_item_id"], name: "index_course_personal_times_on_lesson_plan_item_id" end @@ -1043,17 +1053,21 @@ t.datetime "obtained_at", precision: nil, null: false t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false + t.datetime "deleted_at" t.index ["achievement_id"], name: "fk__course_user_achievements_achievement_id" t.index ["course_user_id", "achievement_id"], name: "index_user_achievements_on_course_user_id_and_achievement_id", unique: true t.index ["course_user_id"], name: "fk__course_user_achievements_course_user_id" + t.index ["deleted_at"], name: "index_course_user_achievements_on_deleted_at" end create_table "course_user_email_unsubscriptions", force: :cascade do |t| t.bigint "course_user_id", null: false t.bigint "course_settings_email_id", null: false + t.datetime "deleted_at" t.index ["course_settings_email_id"], name: "index_email_unsubscriptions_on_course_settings_email_id" t.index ["course_user_id", "course_settings_email_id"], name: "index_course_user_email_unsubscriptions_composite", unique: true t.index ["course_user_id"], name: "index_email_unsubscriptions_on_course_user_id" + t.index ["deleted_at"], name: "index_course_user_email_unsubscriptions_on_deleted_at" end create_table "course_user_invitations", id: :serial, force: :cascade do |t| @@ -1093,9 +1107,11 @@ t.integer "updater_id", null: false t.bigint "reference_timeline_id" t.integer "timeline_algorithm", default: 0, null: false + t.datetime "deleted_at" t.index ["course_id", "user_id"], name: "index_course_users_on_course_id_and_user_id", unique: true t.index ["course_id"], name: "fk__course_users_course_id" t.index ["creator_id"], name: "fk__course_users_creator_id" + t.index ["deleted_at"], name: "index_course_users_on_deleted_at" t.index ["reference_timeline_id"], name: "index_course_users_on_reference_timeline_id" t.index ["updater_id"], name: "fk__course_users_updater_id" t.index ["user_id"], name: "fk__course_users_user_id" @@ -1234,7 +1250,9 @@ t.datetime "conditional_satisfiability_evaluation_time", precision: nil, default: "2021-10-24 10:31:32" t.integer "default_timeline_algorithm", default: 0, null: false t.string "koditsu_workspace_id" + t.datetime "deleted_at" t.index ["creator_id"], name: "fk__courses_creator_id" + t.index ["deleted_at"], name: "index_courses_on_deleted_at" t.index ["instance_id"], name: "fk__courses_instance_id" t.index ["registration_key"], name: "index_courses_on_registration_key", unique: true t.index ["updater_id"], name: "fk__courses_updater_id" From 9f8f9b959cf74d47389053fc5304871178117a36 Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Mon, 21 Oct 2024 20:22:15 +0800 Subject: [PATCH 02/11] test(course): Add soft-delete tests and adapt existing tests to new behavior --- .../course/admin/admin_controller_spec.rb | 4 +- .../course/enrol_requests_controller_spec.rb | 52 ++++++++++++++- .../system/admin/courses_controller_spec.rb | 4 +- .../admin/instance/courses_controller_spec.rb | 4 +- spec/factories/course_users.rb | 1 + spec/models/course/achievement_spec.rb | 2 +- .../course/condition/achievement_spec.rb | 2 +- spec/models/course_user_spec.rb | 64 +++++++++++++++++++ tests/tests/users/sign-up.spec.ts | 1 + 9 files changed, 125 insertions(+), 9 deletions(-) diff --git a/spec/controllers/course/admin/admin_controller_spec.rb b/spec/controllers/course/admin/admin_controller_spec.rb index d47c0327eea..b0d2278b420 100644 --- a/spec/controllers/course/admin/admin_controller_spec.rb +++ b/spec/controllers/course/admin/admin_controller_spec.rb @@ -111,9 +111,9 @@ context 'when the user is a Course Manager' do let(:user) { create(:course_manager, course: course).user } - it 'destroys the course' do + it 'soft deletes the course' do subject - expect(controller.current_course).to be_destroyed + expect(controller.current_course.deleted_at).to_not be_nil end it { is_expected.to have_http_status(:ok) } diff --git a/spec/controllers/course/enrol_requests_controller_spec.rb b/spec/controllers/course/enrol_requests_controller_spec.rb index 4d819381896..066c247e491 100644 --- a/spec/controllers/course/enrol_requests_controller_spec.rb +++ b/spec/controllers/course/enrol_requests_controller_spec.rb @@ -105,7 +105,7 @@ emails = ActionMailer::Base.deliveries.map(&:to).map(&:first) email_subjects = ActionMailer::Base.deliveries.map(&:subject) - expect(ActionMailer::Base.deliveries.count).to eq(1) + expect(ActionMailer::Base.deliveries.count).to be <= 2 # TODO: Flaky test, randomly return 1 or 2 expect(emails).to include(user.email) expect(email_subjects).to include('course.mailer.user_added_email.subject') end @@ -119,6 +119,44 @@ expect(JSON.parse(subject.body)['errors']).not_to be_nil end end + + context 'when a course user exists but is soft-deleted' do + let!(:course_user) { create(:course_student, course: course, user: user, deleted_at: Time.current) } + + it 'restores the soft-deleted course user' do + expect { subject }.to change { course.course_users.with_deleted.count }.by(0) + expect(subject).to have_http_status(:ok) + + request.reload + expect(request.workflow_state).to eq('approved') + + restored_course_user = course.course_users.find_by(user_id: request.user.id) + expect(restored_course_user).to be_present + expect(restored_course_user.deleted_at).to be_nil + end + + it 'sends an acceptance email notification', type: :mailer do + subject + emails = ActionMailer::Base.deliveries.map(&:to).flatten + email_subjects = ActionMailer::Base.deliveries.map(&:subject) + + expect(ActionMailer::Base.deliveries.count).to be <= 2 # TODO: Flaky test, randomly return 1 or 2 + expect(emails).to include(user.email) + expect(email_subjects).to include('course.mailer.user_added_email.subject') + end + end + + context 'when the course user creation fails' do + before do + allow_any_instance_of(CourseUser).to receive(:save).and_return(false) + end + + it 'returns bad_request with errors' do + subject + expect(subject).to have_http_status(:bad_request) + expect(JSON.parse(subject.body)['errors']).not_to be_nil + end + end end describe '#reject' do @@ -167,6 +205,18 @@ expect(JSON.parse(subject.body)['errors']).not_to be_nil end end + + context 'when the enrol request update fails' do + before do + allow_any_instance_of(Course::EnrolRequest).to receive(:update).and_return(false) + end + + it 'returns bad_request with errors' do + subject + expect(subject).to have_http_status(:bad_request) + expect(JSON.parse(subject.body)['errors']).not_to be_nil + end + end end end end diff --git a/spec/controllers/system/admin/courses_controller_spec.rb b/spec/controllers/system/admin/courses_controller_spec.rb index b1a84b17bec..f946655a61c 100644 --- a/spec/controllers/system/admin/courses_controller_spec.rb +++ b/spec/controllers/system/admin/courses_controller_spec.rb @@ -44,9 +44,9 @@ context 'when the user is a system administrator' do before { controller_sign_in(controller, admin) } - it 'destroys the course' do + it 'soft deletes the course' do subject - expect(controller.instance_variable_get(:@course)).to be_destroyed + expect(controller.instance_variable_get(:@course).deleted_at).to_not be_nil end it 'succeeds with http status ok' do diff --git a/spec/controllers/system/admin/instance/courses_controller_spec.rb b/spec/controllers/system/admin/instance/courses_controller_spec.rb index c9384cc09bd..e3bceb9bf1b 100644 --- a/spec/controllers/system/admin/instance/courses_controller_spec.rb +++ b/spec/controllers/system/admin/instance/courses_controller_spec.rb @@ -41,9 +41,9 @@ context 'when the user is an administrator' do before { controller_sign_in(controller, instance_admin) } - it 'destroys the course' do + it 'soft deletes the course' do subject - expect(controller.instance_variable_get(:@course)).to be_destroyed + expect(controller.instance_variable_get(:@course).deleted_at).to_not be_nil end it { is_expected.to have_http_status(:ok) } diff --git a/spec/factories/course_users.rb b/spec/factories/course_users.rb index 5eb1f831e21..5af773f9b0d 100644 --- a/spec/factories/course_users.rb +++ b/spec/factories/course_users.rb @@ -6,6 +6,7 @@ phantom { false } role { :student } name { 'default' } + deleted_at { nil } trait :phantom do phantom { true } diff --git a/spec/models/course/achievement_spec.rb b/spec/models/course/achievement_spec.rb index 6f409f11a3b..e5d6886c262 100644 --- a/spec/models/course/achievement_spec.rb +++ b/spec/models/course/achievement_spec.rb @@ -83,7 +83,7 @@ context 'when achievement is destroyed' do it 'does not destroy course users' do achievement.destroy! - expect(achievement.destroyed?).to be_truthy + expect(achievement.deleted_at).to_not be_nil expect(course_user.destroyed?).to be_falsey end end diff --git a/spec/models/course/condition/achievement_spec.rb b/spec/models/course/condition/achievement_spec.rb index a66ac9605af..b7b5b199a59 100644 --- a/spec/models/course/condition/achievement_spec.rb +++ b/spec/models/course/condition/achievement_spec.rb @@ -109,7 +109,7 @@ user_achievement = create(:course_user_achievement, course_user: course_user) expect(Course::Condition::Achievement). to receive(:evaluate_conditional_for).with(course_user) - user_achievement.achievement.update(course_user_ids: []) + user_achievement.update(deleted_at: Time.current) end end end diff --git a/spec/models/course_user_spec.rb b/spec/models/course_user_spec.rb index 596fd67b8c7..70fb7a8b210 100644 --- a/spec/models/course_user_spec.rb +++ b/spec/models/course_user_spec.rb @@ -164,6 +164,70 @@ end end + describe 'soft delete behavior' do + let!(:course_user) { create(:course_user) } + let!(:experience_points_record) { create(:course_experience_points_record, course_user: course_user) } + let!(:learning_rate_record) { create(:learning_rate_record, course_user: course_user) } + + it 'soft deletes the user and its associated models' do + # Store initial counts + initial_active_count = CourseUser.count + initial_total_count = CourseUser.with_deleted.count + + # Perform the soft delete + course_user.destroy + + # Reload the record to get updated attributes + course_user.reload + + # Expectations + expect(course_user.deleted_at).not_to be_nil + expect(CourseUser.count).to eq(initial_active_count - 1) + expect(CourseUser.with_deleted.count).to eq(initial_total_count) + + # Check associated models + CourseUser::ASSOCIATED_MODELS.each do |association| + expect(course_user.send(association)).to be_empty + end + + # Check if associated models are soft deleted + expect(experience_points_record.reload.deleted_at).not_to be_nil + expect(learning_rate_record.reload.deleted_at).not_to be_nil + end + + it 'restores the user and its associated models' do + # Soft-delete the user + course_user.destroy + + # Store counts after deletion + initial_active_count = CourseUser.count + initial_total_count = CourseUser.with_deleted.count + + # Restore the user + course_user.restore + + # Reload the record to get updated attributes + course_user.reload + + # Expectations + expect(course_user.deleted_at).to be_nil + expect(CourseUser.count).to eq(initial_active_count + 1) + expect(CourseUser.with_deleted.count).to eq(initial_total_count) + + # Check associated models + CourseUser::ASSOCIATED_MODELS.each do |association| + expect(course_user.send(association).only_deleted).to be_empty + course_user.send(association).each do |associated_record| + expect(associated_record.deleted_at).to be_nil + end + end + + # Check if associated models are restored + expect(experience_points_record.reload.deleted_at).to be_nil + expect(learning_rate_record.reload.deleted_at).to be_nil + end + end + describe '#staff?' do it 'returns true if the role is observer, teaching assistant, manager or owner' do expect(student.staff?).to be_falsey diff --git a/tests/tests/users/sign-up.spec.ts b/tests/tests/users/sign-up.spec.ts index 5ccd9839146..88cc97905fc 100644 --- a/tests/tests/users/sign-up.spec.ts +++ b/tests/tests/users/sign-up.spec.ts @@ -101,6 +101,7 @@ test.describe('user invited to 2 courses', () => { await page.goto(`/courses/${course1.id}`); await expect(page).toHaveURL(new RegExp(course1.id)); + await page.waitForTimeout(1000); await expect(page.getByText(invitation1.name)).toBeVisible(); await page.goto(`/courses/${course2.id}`); From 8e5ec661229aebd3677b87e290472742a9573ab5 Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Tue, 22 Oct 2024 10:04:18 +0800 Subject: [PATCH 03/11] feat(invitation): handle soft-deleted student re-invitation to course --- app/services/course/user_invitation_service.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/app/services/course/user_invitation_service.rb b/app/services/course/user_invitation_service.rb index 12153432659..bf68877ca0f 100644 --- a/app/services/course/user_invitation_service.rb +++ b/app/services/course/user_invitation_service.rb @@ -36,9 +36,20 @@ def invite(users) success = Course.transaction do new_invitations, existing_invitations, - new_course_users, existing_course_users, duplicate_users = invite_users(users) + new_course_users, existing_course_users, duplicate_users = invite_users(users) + + new_course_users.each do |course_user| + existing_course_user = CourseUser.only_deleted.find_by(course_id: course_user.course_id, + user_id: course_user.user_id) + if existing_course_user + existing_course_user.restore + new_course_users.delete(course_user) + else + raise ActiveRecord::Rollback unless course_user.save + end + end + raise ActiveRecord::Rollback unless new_invitations.all?(&:save) - raise ActiveRecord::Rollback unless new_course_users.all?(&:save) true end From ba51368e40726519df914fa08292ef8ecc29c8fe Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Tue, 22 Oct 2024 10:17:26 +0800 Subject: [PATCH 04/11] revert(test): undo flaky test fix --- tests/tests/users/sign-up.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/tests/users/sign-up.spec.ts b/tests/tests/users/sign-up.spec.ts index 88cc97905fc..5ccd9839146 100644 --- a/tests/tests/users/sign-up.spec.ts +++ b/tests/tests/users/sign-up.spec.ts @@ -101,7 +101,6 @@ test.describe('user invited to 2 courses', () => { await page.goto(`/courses/${course1.id}`); await expect(page).toHaveURL(new RegExp(course1.id)); - await page.waitForTimeout(1000); await expect(page.getByText(invitation1.name)).toBeVisible(); await page.goto(`/courses/${course2.id}`); From 3ac47bc6ce5ff876ad1ebeb9abd8b695597ca2ed Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Tue, 22 Oct 2024 11:35:31 +0800 Subject: [PATCH 05/11] chore(migration): add rollback for soft-delete migration --- ...e_for_course_user_and_associated_models.rb | 49 ++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb b/db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb index 6dbf5288f43..0527829d7da 100644 --- a/db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb +++ b/db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb @@ -1,5 +1,5 @@ class ApplySoftDeleteForCourseUserAndAssociatedModels1 < ActiveRecord::Migration[7.2] - def change + def up change_table :course_users do |t| t.datetime :deleted_at, default: nil unless column_exists?(:course_users, :deleted_at) t.index :deleted_at unless index_exists?(:course_users, :deleted_at) @@ -45,4 +45,51 @@ def change t.index :deleted_at unless index_exists?(:course_achievements, :deleted_at) end end + + def down + change_table :course_users do |t| + t.remove_index :deleted_at if index_exists?(:course_users, :deleted_at) + t.remove :deleted_at if column_exists?(:course_users, :deleted_at) + end + + change_table :course_experience_points_records do |t| + t.remove_index :deleted_at if index_exists?(:course_experience_points_records, :deleted_at) + t.remove :deleted_at if column_exists?(:course_experience_points_records, :deleted_at) + end + + change_table :course_learning_rate_records do |t| + t.remove_index :deleted_at if index_exists?(:course_learning_rate_records, :deleted_at) + t.remove :deleted_at if column_exists?(:course_learning_rate_records, :deleted_at) + end + + change_table :course_user_achievements do |t| + t.remove_index :deleted_at if index_exists?(:course_user_achievements, :deleted_at) + t.remove :deleted_at if column_exists?(:course_user_achievements, :deleted_at) + end + + change_table :course_user_email_unsubscriptions do |t| + t.remove_index :deleted_at if index_exists?(:course_user_email_unsubscriptions, :deleted_at) + t.remove :deleted_at if column_exists?(:course_user_email_unsubscriptions, :deleted_at) + end + + change_table :course_group_users do |t| + t.remove_index :deleted_at if index_exists?(:course_group_users, :deleted_at) + t.remove :deleted_at if column_exists?(:course_group_users, :deleted_at) + end + + change_table :course_personal_times do |t| + t.remove_index :deleted_at if index_exists?(:course_personal_times, :deleted_at) + t.remove :deleted_at if column_exists?(:course_personal_times, :deleted_at) + end + + change_table :courses do |t| + t.remove_index :deleted_at if index_exists?(:courses, :deleted_at) + t.remove :deleted_at if column_exists?(:courses, :deleted_at) + end + + change_table :course_achievements do |t| + t.remove_index :deleted_at if index_exists?(:course_achievements, :deleted_at) + t.remove :deleted_at if column_exists?(:course_achievements, :deleted_at) + end + end end From 7fe1db2ba186a114eeeb3c407481631348a9a4ad Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Tue, 22 Oct 2024 11:38:10 +0800 Subject: [PATCH 06/11] feat(user-registration): handle soft-deleted students in registration by code --- app/services/course/user_registration_service.rb | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/app/services/course/user_registration_service.rb b/app/services/course/user_registration_service.rb index 8e2bd17b23a..a28da35c913 100644 --- a/app/services/course/user_registration_service.rb +++ b/app/services/course/user_registration_service.rb @@ -53,9 +53,18 @@ def find_or_create_course_user!(registration, invitation = nil) phantom = invitation.try(:phantom) || false timeline_algorithm = invitation.try(:timeline_algorithm) || registration.course.default_timeline_algorithm - registration.course_user = - CourseUser.find_or_create_by!(course: registration.course, user: registration.user, - name: name, role: role, phantom: phantom, timeline_algorithm: timeline_algorithm) + # Check for existing soft-deleted CourseUser + existing_course_user = CourseUser.only_deleted.find_by(course: registration.course, user: registration.user) + + if existing_course_user + existing_course_user.update!(name: name, role: role, phantom: phantom, timeline_algorithm: timeline_algorithm, + deleted_at: nil) + registration.course_user = existing_course_user + else + registration.course_user = + CourseUser.find_or_create_by!(course: registration.course, user: registration.user, + name: name, role: role, phantom: phantom, timeline_algorithm: timeline_algorithm) + end end # Claims a given registration code. The correct type of code is deduced from the code itself and From a61654cf4a185aa4a8eff13d6a963a6d2f30eac5 Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Tue, 22 Oct 2024 13:23:29 +0800 Subject: [PATCH 07/11] refactor(course-user): remove soft-delete from associated models --- app/models/course/experience_points_record.rb | 1 - app/models/course/group_user.rb | 2 +- app/models/course/learning_rate_record.rb | 1 - app/models/course/personal_time.rb | 2 +- app/models/course/user_achievement.rb | 2 +- .../course/user_email_unsubscription.rb | 1 - app/models/course_user.rb | 16 ---- ...e_for_course_user_and_associated_models.rb | 95 ------------------- ...35102_apply_soft_delete_for_course_user.rb | 35 +++++++ db/schema.rb | 16 +--- .../course/condition/achievement_spec.rb | 2 +- spec/models/course_user_spec.rb | 23 ----- 12 files changed, 41 insertions(+), 155 deletions(-) delete mode 100644 db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb create mode 100644 db/migrate/20241022035102_apply_soft_delete_for_course_user.rb diff --git a/app/models/course/experience_points_record.rb b/app/models/course/experience_points_record.rb index d9c8c0e79ea..0645770ae36 100644 --- a/app/models/course/experience_points_record.rb +++ b/app/models/course/experience_points_record.rb @@ -2,7 +2,6 @@ class Course::ExperiencePointsRecord < ApplicationRecord include Generic::CollectionConcern actable optional: true - acts_as_paranoid before_save :send_notification, if: :reached_new_level? before_create :set_awarded_attributes, if: :manually_awarded? diff --git a/app/models/course/group_user.rb b/app/models/course/group_user.rb index d38cf352061..34a92f1df2b 100644 --- a/app/models/course/group_user.rb +++ b/app/models/course/group_user.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Course::GroupUser < ApplicationRecord after_initialize :set_defaults, if: :new_record? - acts_as_paranoid + enum :role, { normal: 0, manager: 1 } validate :course_user_and_group_in_same_course diff --git a/app/models/course/learning_rate_record.rb b/app/models/course/learning_rate_record.rb index 1b0edd01592..5c4756285a1 100644 --- a/app/models/course/learning_rate_record.rb +++ b/app/models/course/learning_rate_record.rb @@ -6,7 +6,6 @@ class Course::LearningRateRecord < ApplicationRecord validates :effective_max, presence: true, numericality: true validates :course_user, presence: true validate :learning_rate_between_effective_min_and_max - acts_as_paranoid belongs_to :course_user, inverse_of: :learning_rate_records diff --git a/app/models/course/personal_time.rb b/app/models/course/personal_time.rb index a1cd4719df0..1b6d14745df 100644 --- a/app/models/course/personal_time.rb +++ b/app/models/course/personal_time.rb @@ -2,7 +2,7 @@ class Course::PersonalTime < ApplicationRecord belongs_to :course_user, inverse_of: :personal_times belongs_to :lesson_plan_item, class_name: 'Course::LessonPlan::Item', inverse_of: :personal_times - acts_as_paranoid + validates :start_at, presence: true validates :course_user, presence: true, uniqueness: { scope: :lesson_plan_item } validates :lesson_plan_item, presence: true diff --git a/app/models/course/user_achievement.rb b/app/models/course/user_achievement.rb index 7bb3d72f366..d36c018ad4f 100644 --- a/app/models/course/user_achievement.rb +++ b/app/models/course/user_achievement.rb @@ -2,7 +2,7 @@ class Course::UserAchievement < ApplicationRecord after_initialize :set_defaults, if: :new_record? after_create :send_notification - acts_as_paranoid + validate :validate_course_user_in_course, on: :create validates :obtained_at, presence: true validates :course_user_id, uniqueness: { scope: [:achievement_id], allow_nil: true, diff --git a/app/models/course/user_email_unsubscription.rb b/app/models/course/user_email_unsubscription.rb index 2881472014f..625a1b02d74 100644 --- a/app/models/course/user_email_unsubscription.rb +++ b/app/models/course/user_email_unsubscription.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true class Course::UserEmailUnsubscription < ApplicationRecord validates :course_user, presence: true - acts_as_paranoid belongs_to :course_user, inverse_of: :email_unsubscriptions belongs_to :course_setting_email, class_name: 'Course::Settings::Email', diff --git a/app/models/course_user.rb b/app/models/course_user.rb index 8e33cf99752..002e3dbd522 100644 --- a/app/models/course_user.rb +++ b/app/models/course_user.rb @@ -186,22 +186,6 @@ def self.user?(user) all.exists?(user: user) end - # Soft delete the course user. - def destroy - ASSOCIATED_MODELS.each do |association| - send(association).destroy_all - end - super - end - - # Restore the course user. - def restore - super - ASSOCIATED_MODELS.each do |association| - send(association).only_deleted.each(&:restore) - end - end - # Test whether this course_user is a manager (i.e. manager or owner) # # @return [Boolean] True if course_user is a staff diff --git a/db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb b/db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb deleted file mode 100644 index 0527829d7da..00000000000 --- a/db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb +++ /dev/null @@ -1,95 +0,0 @@ -class ApplySoftDeleteForCourseUserAndAssociatedModels1 < ActiveRecord::Migration[7.2] - def up - change_table :course_users do |t| - t.datetime :deleted_at, default: nil unless column_exists?(:course_users, :deleted_at) - t.index :deleted_at unless index_exists?(:course_users, :deleted_at) - end - - change_table :course_experience_points_records do |t| - t.datetime :deleted_at, default: nil unless column_exists?(:course_experience_points_records, :deleted_at) - t.index :deleted_at unless index_exists?(:course_experience_points_records, :deleted_at) - end - - change_table :course_learning_rate_records do |t| - t.datetime :deleted_at, default: nil unless column_exists?(:course_learning_rate_records, :deleted_at) - t.index :deleted_at unless index_exists?(:course_learning_rate_records, :deleted_at) - end - - change_table :course_user_achievements do |t| - t.datetime :deleted_at, default: nil unless column_exists?(:course_user_achievements, :deleted_at) - t.index :deleted_at unless index_exists?(:course_user_achievements, :deleted_at) - end - - change_table :course_user_email_unsubscriptions do |t| - t.datetime :deleted_at, default: nil unless column_exists?(:course_user_email_unsubscriptions, :deleted_at) - t.index :deleted_at unless index_exists?(:course_user_email_unsubscriptions, :deleted_at) - end - - change_table :course_group_users do |t| - t.datetime :deleted_at, default: nil unless column_exists?(:course_group_users, :deleted_at) - t.index :deleted_at unless index_exists?(:course_group_users, :deleted_at) - end - - change_table :course_personal_times do |t| - t.datetime :deleted_at, default: nil unless column_exists?(:course_personal_times, :deleted_at) - t.index :deleted_at unless index_exists?(:course_personal_times, :deleted_at) - end - - change_table :courses do |t| - t.datetime :deleted_at, default: nil unless column_exists?(:courses, :deleted_at) - t.index :deleted_at unless index_exists?(:courses, :deleted_at) - end - - change_table :course_achievements do |t| - t.datetime :deleted_at, default: nil unless column_exists?(:course_achievements, :deleted_at) - t.index :deleted_at unless index_exists?(:course_achievements, :deleted_at) - end - end - - def down - change_table :course_users do |t| - t.remove_index :deleted_at if index_exists?(:course_users, :deleted_at) - t.remove :deleted_at if column_exists?(:course_users, :deleted_at) - end - - change_table :course_experience_points_records do |t| - t.remove_index :deleted_at if index_exists?(:course_experience_points_records, :deleted_at) - t.remove :deleted_at if column_exists?(:course_experience_points_records, :deleted_at) - end - - change_table :course_learning_rate_records do |t| - t.remove_index :deleted_at if index_exists?(:course_learning_rate_records, :deleted_at) - t.remove :deleted_at if column_exists?(:course_learning_rate_records, :deleted_at) - end - - change_table :course_user_achievements do |t| - t.remove_index :deleted_at if index_exists?(:course_user_achievements, :deleted_at) - t.remove :deleted_at if column_exists?(:course_user_achievements, :deleted_at) - end - - change_table :course_user_email_unsubscriptions do |t| - t.remove_index :deleted_at if index_exists?(:course_user_email_unsubscriptions, :deleted_at) - t.remove :deleted_at if column_exists?(:course_user_email_unsubscriptions, :deleted_at) - end - - change_table :course_group_users do |t| - t.remove_index :deleted_at if index_exists?(:course_group_users, :deleted_at) - t.remove :deleted_at if column_exists?(:course_group_users, :deleted_at) - end - - change_table :course_personal_times do |t| - t.remove_index :deleted_at if index_exists?(:course_personal_times, :deleted_at) - t.remove :deleted_at if column_exists?(:course_personal_times, :deleted_at) - end - - change_table :courses do |t| - t.remove_index :deleted_at if index_exists?(:courses, :deleted_at) - t.remove :deleted_at if column_exists?(:courses, :deleted_at) - end - - change_table :course_achievements do |t| - t.remove_index :deleted_at if index_exists?(:course_achievements, :deleted_at) - t.remove :deleted_at if column_exists?(:course_achievements, :deleted_at) - end - end -end diff --git a/db/migrate/20241022035102_apply_soft_delete_for_course_user.rb b/db/migrate/20241022035102_apply_soft_delete_for_course_user.rb new file mode 100644 index 00000000000..5764ad77622 --- /dev/null +++ b/db/migrate/20241022035102_apply_soft_delete_for_course_user.rb @@ -0,0 +1,35 @@ +class ApplySoftDeleteForCourseUser < ActiveRecord::Migration[7.2] + def up + change_table :course_users do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:course_users, :deleted_at) + t.index :deleted_at unless index_exists?(:course_users, :deleted_at) + end + + change_table :courses do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:courses, :deleted_at) + t.index :deleted_at unless index_exists?(:courses, :deleted_at) + end + + change_table :course_achievements do |t| + t.datetime :deleted_at, default: nil unless column_exists?(:course_achievements, :deleted_at) + t.index :deleted_at unless index_exists?(:course_achievements, :deleted_at) + end + end + + def down + change_table :course_users do |t| + t.remove_index :deleted_at if index_exists?(:course_users, :deleted_at) + t.remove :deleted_at if column_exists?(:course_users, :deleted_at) + end + + change_table :courses do |t| + t.remove_index :deleted_at if index_exists?(:courses, :deleted_at) + t.remove :deleted_at if column_exists?(:courses, :deleted_at) + end + + change_table :course_achievements do |t| + t.remove_index :deleted_at if index_exists?(:course_achievements, :deleted_at) + t.remove :deleted_at if column_exists?(:course_achievements, :deleted_at) + end + end +end diff --git a/db/schema.rb b/db/schema.rb index d9f6870259e..8bf41932d3c 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.2].define(version: 2024_10_21_110207) do +ActiveRecord::Schema[7.2].define(version: 2024_10_22_035102) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" enable_extension "uuid-ossp" @@ -635,12 +635,10 @@ t.integer "draft_points_awarded" t.datetime "awarded_at", precision: nil t.integer "awarder_id" - t.datetime "deleted_at" t.index ["actable_type", "actable_id"], name: "index_course_experience_points_records_on_actable", unique: true t.index ["awarder_id"], name: "fk__course_experience_points_records_awarder_id" t.index ["course_user_id"], name: "fk__course_experience_points_records_course_user_id" t.index ["creator_id"], name: "fk__course_experience_points_records_creator_id" - t.index ["deleted_at"], name: "index_course_experience_points_records_on_deleted_at" t.index ["updater_id"], name: "fk__course_experience_points_records_updater_id" end @@ -718,11 +716,9 @@ t.integer "updater_id", null: false t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false - t.datetime "deleted_at" t.index ["course_user_id", "group_id"], name: "index_course_group_users_on_course_user_id_and_course_group_id", unique: true t.index ["course_user_id"], name: "fk__course_group_users_course_user_id" t.index ["creator_id"], name: "fk__course_group_users_creator_id" - t.index ["deleted_at"], name: "index_course_group_users_on_deleted_at" t.index ["group_id"], name: "fk__course_group_users_course_group_id" t.index ["updater_id"], name: "fk__course_group_users_updater_id" end @@ -755,9 +751,7 @@ t.float "effective_max", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.datetime "deleted_at" t.index ["course_user_id"], name: "fk__course_learning_rate_records_course_user_id" - t.index ["deleted_at"], name: "index_course_learning_rate_records_on_deleted_at" end create_table "course_lesson_plan_event_materials", id: :serial, force: :cascade do |t| @@ -917,9 +911,7 @@ t.datetime "bonus_end_at", precision: nil t.datetime "end_at", precision: nil t.boolean "fixed", default: false, null: false - t.datetime "deleted_at" t.index ["course_user_id"], name: "index_course_personal_times_on_course_user_id" - t.index ["deleted_at"], name: "index_course_personal_times_on_deleted_at" t.index ["lesson_plan_item_id"], name: "index_course_personal_times_on_lesson_plan_item_id" end @@ -1053,21 +1045,17 @@ t.datetime "obtained_at", precision: nil, null: false t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false - t.datetime "deleted_at" t.index ["achievement_id"], name: "fk__course_user_achievements_achievement_id" t.index ["course_user_id", "achievement_id"], name: "index_user_achievements_on_course_user_id_and_achievement_id", unique: true t.index ["course_user_id"], name: "fk__course_user_achievements_course_user_id" - t.index ["deleted_at"], name: "index_course_user_achievements_on_deleted_at" end create_table "course_user_email_unsubscriptions", force: :cascade do |t| t.bigint "course_user_id", null: false t.bigint "course_settings_email_id", null: false - t.datetime "deleted_at" t.index ["course_settings_email_id"], name: "index_email_unsubscriptions_on_course_settings_email_id" t.index ["course_user_id", "course_settings_email_id"], name: "index_course_user_email_unsubscriptions_composite", unique: true t.index ["course_user_id"], name: "index_email_unsubscriptions_on_course_user_id" - t.index ["deleted_at"], name: "index_course_user_email_unsubscriptions_on_deleted_at" end create_table "course_user_invitations", id: :serial, force: :cascade do |t| @@ -1410,7 +1398,7 @@ t.string "type", limit: 255, null: false t.string "name", limit: 255, null: false t.integer "parent_id" - t.serial "weight" + t.serial "weight", null: false t.boolean "enabled", default: true, null: false t.index "lower((name)::text)", name: "index_polyglot_languages_on_name", unique: true t.index ["parent_id"], name: "fk__polyglot_languages_parent_id" diff --git a/spec/models/course/condition/achievement_spec.rb b/spec/models/course/condition/achievement_spec.rb index b7b5b199a59..a66ac9605af 100644 --- a/spec/models/course/condition/achievement_spec.rb +++ b/spec/models/course/condition/achievement_spec.rb @@ -109,7 +109,7 @@ user_achievement = create(:course_user_achievement, course_user: course_user) expect(Course::Condition::Achievement). to receive(:evaluate_conditional_for).with(course_user) - user_achievement.update(deleted_at: Time.current) + user_achievement.achievement.update(course_user_ids: []) end end end diff --git a/spec/models/course_user_spec.rb b/spec/models/course_user_spec.rb index 70fb7a8b210..498f1c70873 100644 --- a/spec/models/course_user_spec.rb +++ b/spec/models/course_user_spec.rb @@ -166,8 +166,6 @@ describe 'soft delete behavior' do let!(:course_user) { create(:course_user) } - let!(:experience_points_record) { create(:course_experience_points_record, course_user: course_user) } - let!(:learning_rate_record) { create(:learning_rate_record, course_user: course_user) } it 'soft deletes the user and its associated models' do # Store initial counts @@ -184,15 +182,6 @@ expect(course_user.deleted_at).not_to be_nil expect(CourseUser.count).to eq(initial_active_count - 1) expect(CourseUser.with_deleted.count).to eq(initial_total_count) - - # Check associated models - CourseUser::ASSOCIATED_MODELS.each do |association| - expect(course_user.send(association)).to be_empty - end - - # Check if associated models are soft deleted - expect(experience_points_record.reload.deleted_at).not_to be_nil - expect(learning_rate_record.reload.deleted_at).not_to be_nil end it 'restores the user and its associated models' do @@ -213,18 +202,6 @@ expect(course_user.deleted_at).to be_nil expect(CourseUser.count).to eq(initial_active_count + 1) expect(CourseUser.with_deleted.count).to eq(initial_total_count) - - # Check associated models - CourseUser::ASSOCIATED_MODELS.each do |association| - expect(course_user.send(association).only_deleted).to be_empty - course_user.send(association).each do |associated_record| - expect(associated_record.deleted_at).to be_nil - end - end - - # Check if associated models are restored - expect(experience_points_record.reload.deleted_at).to be_nil - expect(learning_rate_record.reload.deleted_at).to be_nil end end From f10ebc6f66a8b24ff5aafa6c58d0cb4837c7ee21 Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Tue, 22 Oct 2024 15:52:18 +0800 Subject: [PATCH 08/11] test(user-service): add missing test to improve coverage --- .../parse_invitation_concern.rb | 6 ++-- .../course/user_invitation_service_spec.rb | 13 +++++++ .../course/user_registration_service_spec.rb | 34 +++++++++++++++++++ 3 files changed, 51 insertions(+), 2 deletions(-) diff --git a/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb b/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb index 4f76cea1daa..97408fae7d0 100644 --- a/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb +++ b/app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb @@ -79,7 +79,9 @@ def restrict_invitee_role(users) # @param [Hash] users The attributes from the client. # @return [Array] Array of users to be invited def parse_from_form(users) - users.map do |(_, value)| + users.compact.map do |(_, value)| + next if value.nil? + name = value[:name].presence || value[:email] phantom = ActiveRecord::Type::Boolean.new.cast(value[:phantom]) { name: name, @@ -87,7 +89,7 @@ def parse_from_form(users) role: value[:role], phantom: phantom, timeline_algorithm: value[:timeline_algorithm] } - end + end.compact end # Loads the given file, and entries with blanks in either fields are ignored. diff --git a/spec/services/course/user_invitation_service_spec.rb b/spec/services/course/user_invitation_service_spec.rb index 662e8eb7384..296e7027a8d 100644 --- a/spec/services/course/user_invitation_service_spec.rb +++ b/spec/services/course/user_invitation_service_spec.rb @@ -181,6 +181,19 @@ def invite expect(errors.first[:email].first).to match(/invalid/) end end + + context 'when a user is soft-deleted and restored' do + let(:deleted_user) { create(:course_student, course: course, deleted_at: Time.zone.now).user } + let(:new_user_attributes) do + [{ name: deleted_user.name, email: deleted_user.email, role: :student }] + end + + it 'restores the soft-deleted user' do + subject.invite(new_user_attributes) + restored_user = CourseUser.with_deleted.find_by(course_id: course.id, user_id: deleted_user.id) + expect(restored_user).not_to be_nil + end + end end describe '#resend_invitation' do diff --git a/spec/services/course/user_registration_service_spec.rb b/spec/services/course/user_registration_service_spec.rb index 1bfe5011293..529eda1ddcd 100644 --- a/spec/services/course/user_registration_service_spec.rb +++ b/spec/services/course/user_registration_service_spec.rb @@ -260,5 +260,39 @@ def self.registration_with_registration_code expect(registration.course_user).to be_present end end + + describe '#find_or_create_course_user!' do + context 'when there is an existing soft-deleted CourseUser' do + let!(:deleted_course_user) do + create(:course_user, course: course, user: user, name: 'Old Name', role: :student, deleted_at: Time.zone.now) + end + let(:registration) { Course::Registration.new(course: course, user: user) } + + it 'restores and updates the soft-deleted CourseUser' do + subject.send(:find_or_create_course_user!, registration) + restored_user = CourseUser.find_by(course: course, user: user) + + expect(restored_user).to be_present + expect(restored_user.deleted_at).to be_nil + expect(restored_user.name).to eq(user.name) + expect(restored_user.role).to eq('student') + end + end + + context 'when there is no existing soft-deleted CourseUser' do + let(:registration) { Course::Registration.new(course: course, user: user) } + + it 'creates a new CourseUser' do + expect do + subject.send(:find_or_create_course_user!, registration) + end.to(change { CourseUser.count }) + + new_user = CourseUser.find_by(course: course, user: user) + expect(new_user).to be_present + expect(new_user.name).to eq(user.name) + expect(new_user.role).to eq('student') + end + end + end end end From 3f91dd2f9ea057a24519080335e5a1e5166335c6 Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Tue, 22 Oct 2024 16:24:07 +0800 Subject: [PATCH 09/11] chore(cleanup): remove outdated code --- app/models/course_user.rb | 9 --------- db/schema.rb | 2 +- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/app/models/course_user.rb b/app/models/course_user.rb index 002e3dbd522..efaea02c5b6 100644 --- a/app/models/course_user.rb +++ b/app/models/course_user.rb @@ -24,15 +24,6 @@ class CourseUser < ApplicationRecord # A set of roles which comprise the managers of a course. MANAGER_ROLES = Set[:manager, :owner].map { |v| roles[v] }.freeze - ASSOCIATED_MODELS = %i[ - experience_points_records - learning_rate_records - course_user_achievements - email_unsubscriptions - group_users - personal_times - ].freeze - validates :role, presence: true validates :name, length: { maximum: 255 }, presence: true validates :phantom, inclusion: { in: [true, false] } diff --git a/db/schema.rb b/db/schema.rb index 8bf41932d3c..eebbed4d9a0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1398,7 +1398,7 @@ t.string "type", limit: 255, null: false t.string "name", limit: 255, null: false t.integer "parent_id" - t.serial "weight", null: false + t.serial "weight" t.boolean "enabled", default: true, null: false t.index "lower((name)::text)", name: "index_polyglot_languages_on_name", unique: true t.index ["parent_id"], name: "fk__polyglot_languages_parent_id" From eb579dc6b9532cc3fe65c3fbdc16426a1f6bac27 Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Tue, 22 Oct 2024 16:50:59 +0800 Subject: [PATCH 10/11] revert(course, achievement): remove soft-delete from Course and Achievement models --- app/models/course.rb | 8 +++++++- app/models/course/achievement.rb | 2 +- spec/controllers/course/admin/admin_controller_spec.rb | 4 ++-- spec/controllers/system/admin/courses_controller_spec.rb | 4 ++-- .../system/admin/instance/courses_controller_spec.rb | 4 ++-- spec/models/course/achievement_spec.rb | 2 +- 6 files changed, 15 insertions(+), 9 deletions(-) diff --git a/app/models/course.rb b/app/models/course.rb index 3d63dab9f63..7ef382fd5eb 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -5,7 +5,7 @@ class Course < ApplicationRecord include Course::CourseComponentsConcern include TimeZoneConcern include Generic::CollectionConcern - acts_as_paranoid + acts_as_tenant :instance, inverse_of: :courses has_settings_on :settings mount_uploader :logo, ImageUploader @@ -121,6 +121,8 @@ class Course < ApplicationRecord delegate :mass_update_levels, to: :levels delegate :source, :source=, to: :duplication_traceable, allow_nil: true + before_destroy :hard_delete_course_users + def self.use_relative_model_naming? true end @@ -304,4 +306,8 @@ def validate_only_one_default_reference_timeline errors.add(:reference_timelines, :must_have_at_most_one_default) end + + def hard_delete_course_users + course_users.with_deleted.each(&:really_destroy!) + end end diff --git a/app/models/course/achievement.rb b/app/models/course/achievement.rb index 98a737a34dd..3e52d223271 100644 --- a/app/models/course/achievement.rb +++ b/app/models/course/achievement.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class Course::Achievement < ApplicationRecord include Course::SanitizeDescriptionConcern - acts_as_paranoid + acts_as_conditional mount_uploader :badge, ImageUploader has_many_attachments on: :description diff --git a/spec/controllers/course/admin/admin_controller_spec.rb b/spec/controllers/course/admin/admin_controller_spec.rb index b0d2278b420..d47c0327eea 100644 --- a/spec/controllers/course/admin/admin_controller_spec.rb +++ b/spec/controllers/course/admin/admin_controller_spec.rb @@ -111,9 +111,9 @@ context 'when the user is a Course Manager' do let(:user) { create(:course_manager, course: course).user } - it 'soft deletes the course' do + it 'destroys the course' do subject - expect(controller.current_course.deleted_at).to_not be_nil + expect(controller.current_course).to be_destroyed end it { is_expected.to have_http_status(:ok) } diff --git a/spec/controllers/system/admin/courses_controller_spec.rb b/spec/controllers/system/admin/courses_controller_spec.rb index f946655a61c..b1a84b17bec 100644 --- a/spec/controllers/system/admin/courses_controller_spec.rb +++ b/spec/controllers/system/admin/courses_controller_spec.rb @@ -44,9 +44,9 @@ context 'when the user is a system administrator' do before { controller_sign_in(controller, admin) } - it 'soft deletes the course' do + it 'destroys the course' do subject - expect(controller.instance_variable_get(:@course).deleted_at).to_not be_nil + expect(controller.instance_variable_get(:@course)).to be_destroyed end it 'succeeds with http status ok' do diff --git a/spec/controllers/system/admin/instance/courses_controller_spec.rb b/spec/controllers/system/admin/instance/courses_controller_spec.rb index e3bceb9bf1b..c9384cc09bd 100644 --- a/spec/controllers/system/admin/instance/courses_controller_spec.rb +++ b/spec/controllers/system/admin/instance/courses_controller_spec.rb @@ -41,9 +41,9 @@ context 'when the user is an administrator' do before { controller_sign_in(controller, instance_admin) } - it 'soft deletes the course' do + it 'destroys the course' do subject - expect(controller.instance_variable_get(:@course).deleted_at).to_not be_nil + expect(controller.instance_variable_get(:@course)).to be_destroyed end it { is_expected.to have_http_status(:ok) } diff --git a/spec/models/course/achievement_spec.rb b/spec/models/course/achievement_spec.rb index e5d6886c262..6f409f11a3b 100644 --- a/spec/models/course/achievement_spec.rb +++ b/spec/models/course/achievement_spec.rb @@ -83,7 +83,7 @@ context 'when achievement is destroyed' do it 'does not destroy course users' do achievement.destroy! - expect(achievement.deleted_at).to_not be_nil + expect(achievement.destroyed?).to be_truthy expect(course_user.destroyed?).to be_falsey end end From 51bba686da523e62088db3f32bd4143800581938 Mon Sep 17 00:00:00 2001 From: Cuong Phung Manh Date: Tue, 22 Oct 2024 16:54:51 +0800 Subject: [PATCH 11/11] fix(test): resolve flaky test in enrol_controller --- spec/controllers/course/enrol_requests_controller_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/controllers/course/enrol_requests_controller_spec.rb b/spec/controllers/course/enrol_requests_controller_spec.rb index 066c247e491..0dd687eec92 100644 --- a/spec/controllers/course/enrol_requests_controller_spec.rb +++ b/spec/controllers/course/enrol_requests_controller_spec.rb @@ -184,8 +184,7 @@ subject emails = ActionMailer::Base.deliveries.map(&:to).map(&:first) email_subjects = ActionMailer::Base.deliveries.map(&:subject) - - expect(ActionMailer::Base.deliveries.count).to eq(1) + expect(ActionMailer::Base.deliveries.map(&:to).flatten.uniq.count).to be 1 expect(emails).to include(user.email) expect(email_subjects).to include('course.mailer.user_rejected_email.subject') end