Implement Soft-Delete for Students in Classes to Improve Performance #7588

phungmanhcuong · 2024-10-09T02:01:21Z

To avoid long waiting from user when deleting student from class, need apply soft delete
To enhance user experience and minimize wait times when removing a student from the class, implementing a soft delete feature is crucial.

db/migrate/20241014031756_add_deleted_at_to_course_users.rb

db/migrate/20241014031828_add_deleted_at_to_course_reference_timelines.rb

db/migrate/20241014031821_add_deleted_at_to_course_groups.rb

db/migrate/20241014031810_add_deleted_at_to_course_achievements.rb

db/schema.rb

app/models/course/group_user.rb

spec/models/course_user_spec.rb

app/controllers/users_controller.rb

Gemfile

cysjonathan · 2024-10-22T00:06:21Z

db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb

@@ -0,0 +1,48 @@
+class ApplySoftDeleteForCourseUserAndAssociatedModels1 < ActiveRecord::Migration[7.2]


NameError: uninitialized constant ApplySoftDeleteForCourseUserAndAssociatedModels (NameError)

You get this error when run "rails db:migrate" right ?

tests/tests/users/sign-up.spec.ts

cysjonathan · 2024-10-22T00:34:34Z

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


Why do we need course soft-deletion in this PR?

Because we apply the soft-delete for CourseUser, which depend on Course.
We also allow to delete the Course, but if the Course is deleted, the CourseUser will need hard-delete to avoid "FK" violation.
So we need soft-delete for Course also.

We can hard-delete CourseUser if associated Course is deleted.
However, if we want to implement soft-delete for Course, we should also ensure that the soft-deletion is performant + there must be a way to restore the course. We can do this in a separate PR.

The soft-delete for Course PR might need come first this PR, otherwise, a few of tests (which delete the Course) will be failed.

We can hard-delete CourseUser if associated Course is deleted.

db/migrate/20241021110207_apply_soft_delete_for_course_user_and_associated_models.rb

cysjonathan · 2024-10-22T00:38:05Z

app/models/course_user.rb

@@ -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[


Are these associated models exhaustive? How about course_discussion_posts, submissions, video_submissions, activities, etc?

Are we sure we want to cascade delete / soft-delete the associated models in the first place?

Please confirm if you have also checked the effects on jobs or not?

I have updated to skip cascading soft-delete on associated models, so we don't need to worry about side effects from this.

This variable ASSOCIATED_MODELS is unused, it should not exist.

cysjonathan · 2024-10-22T00:38:59Z

app/models/course_user.rb

@@ -176,6 +186,22 @@ def self.user?(user)
    all.exists?(user: user)
  end

+  # Soft delete the course user.
+  def destroy


Soft-delete of course-user with a lot of activities still take >5 minutes. Your PR does not resolve the root issue. Please revise.

The only way is verify by similar data volumn on production. We can keep monitoring it on staging to see.
In theory, the soft-delete won't effect to the index and fk-constrains, so it would be much faster than hard-delete.

I already verified the slow deletion using production-grade data, the problem is that there are many individual associated updates deletes involved.

I suggest we simply do soft-delete on course-user and do not touch associated models.
Subsequent calls to query data should ensure scope of course-user does not include soft-deleted course-user. discard gem might be better for this.

Spend time researching about discard. Might apply it later then.

cysjonathan · 2024-10-22T00:47:53Z

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'


The paranoia gem github page also recommended to look at discard gem

paranoia has some surprising behaviour (like overriding ActiveRecord's delete and destroy) and is not recommended for new projects. See discard's README for more details.

Have you taken a look at discard?

Learning the discard.

app/models/course_user.rb

…ehavior

… by code

cysjonathan

You still need to squash your commits before asking for review please.

More importantly, your PR does not resolve performance issue.

cysjonathan · 2024-10-22T08:11:45Z

app/models/course_user.rb

@@ -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[


This variable ASSOCIATED_MODELS is unused, it should not exist.

cysjonathan · 2024-10-22T08:11:59Z

app/services/concerns/course/user_invitation_service/parse_invitation_concern.rb

      name = value[:name].presence || value[:email]
      phantom = ActiveRecord::Type::Boolean.new.cast(value[:phantom])
      { name: name,
        email: value[:email],
        role: value[:role],
        phantom: phantom,
        timeline_algorithm: value[:timeline_algorithm] }
-    end
+    end.compact


Why is there this change here?

cysjonathan · 2024-10-22T08:13:40Z

db/migrate/20241022035102_apply_soft_delete_for_course_user.rb

+    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)


Why is there still deleted_at on course_achievements here?

db/schema.rb

cysjonathan · 2024-10-22T08:16:43Z

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


We can hard-delete CourseUser if associated Course is deleted.

…vement models

phungmanhcuong self-assigned this Oct 9, 2024

phungmanhcuong marked this pull request as draft October 9, 2024 02:01

phungmanhcuong force-pushed the mcphung/soft-delete-for-removing-student-from-class branch 5 times, most recently from 16d69dc to 116376f Compare October 14, 2024 10:06

phungmanhcuong marked this pull request as ready for review October 14, 2024 10:08

phungmanhcuong force-pushed the mcphung/soft-delete-for-removing-student-from-class branch 2 times, most recently from eaa0dae to 99f1231 Compare October 15, 2024 05:26

phungmanhcuong requested review from cysjonathan, syoopie and bivanalhar October 15, 2024 05:31

phungmanhcuong force-pushed the mcphung/soft-delete-for-removing-student-from-class branch from 5234c89 to 99f1231 Compare October 15, 2024 06:38

cysjonathan removed the request for review from syoopie October 15, 2024 09:25

cysjonathan requested changes Oct 15, 2024

View reviewed changes

phungmanhcuong force-pushed the mcphung/soft-delete-for-removing-student-from-class branch from a7f602a to 4ab1cad Compare October 15, 2024 09:47

cysjonathan requested changes Oct 15, 2024

View reviewed changes

app/controllers/users_controller.rb Outdated Show resolved Hide resolved

app/controllers/users_controller.rb Outdated Show resolved Hide resolved

Gemfile Outdated Show resolved Hide resolved

phungmanhcuong force-pushed the mcphung/soft-delete-for-removing-student-from-class branch 12 times, most recently from cf811d9 to 65d2f44 Compare October 21, 2024 05:43

phungmanhcuong force-pushed the mcphung/soft-delete-for-removing-student-from-class branch 3 times, most recently from 1634ca3 to ddc8aeb Compare October 21, 2024 11:07

phungmanhcuong changed the title ~~refactor(soft-delete-student-class): soft-delete when removing studen…~~ Implement Soft-Delete for Students in Classes to Improve Performance Oct 21, 2024

phungmanhcuong force-pushed the mcphung/soft-delete-for-removing-student-from-class branch 2 times, most recently from 3010168 to 6bdaff3 Compare October 21, 2024 12:25

cysjonathan requested changes Oct 22, 2024

View reviewed changes

phungmanhcuong force-pushed the mcphung/soft-delete-for-removing-student-from-class branch 5 times, most recently from 6fbf860 to 3b490c8 Compare October 22, 2024 07:52

phungmanhcuong added 8 commits October 22, 2024 16:02

feat(student-course): Add soft-delete for student removal from course

a0042a9

test(course): Add soft-delete tests and adapt existing tests to new b…

9f8f9b9

…ehavior

feat(invitation): handle soft-deleted student re-invitation to course

8e5ec66

revert(test): undo flaky test fix

ba51368

chore(migration): add rollback for soft-delete migration

3ac47bc

feat(user-registration): handle soft-deleted students in registration…

7fe1db2

… by code

refactor(course-user): remove soft-delete from associated models

a61654c

test(user-service): add missing test to improve coverage

f10ebc6

cysjonathan requested changes Oct 22, 2024

View reviewed changes

chore(cleanup): remove outdated code

3f91dd2

phungmanhcuong force-pushed the mcphung/soft-delete-for-removing-student-from-class branch from 3b490c8 to 3f91dd2 Compare October 22, 2024 08:24

phungmanhcuong added 2 commits October 22, 2024 16:50

revert(course, achievement): remove soft-delete from Course and Achie…

eb579dc

…vement models

fix(test): resolve flaky test in enrol_controller

51bba68

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Soft-Delete for Students in Classes to Improve Performance #7588

Implement Soft-Delete for Students in Classes to Improve Performance #7588

phungmanhcuong commented Oct 9, 2024 •

edited by cysjonathan

Loading

cysjonathan Oct 22, 2024

phungmanhcuong Oct 22, 2024

cysjonathan Oct 22, 2024

phungmanhcuong Oct 22, 2024

cysjonathan Oct 22, 2024

phungmanhcuong Oct 22, 2024

cysjonathan Oct 22, 2024

cysjonathan Oct 22, 2024

cysjonathan Oct 22, 2024

phungmanhcuong Oct 22, 2024

cysjonathan Oct 22, 2024

cysjonathan Oct 22, 2024

phungmanhcuong Oct 22, 2024

cysjonathan Oct 22, 2024

phungmanhcuong Oct 22, 2024

cysjonathan Oct 22, 2024

phungmanhcuong Oct 22, 2024

cysjonathan left a comment •

edited

Loading

cysjonathan Oct 22, 2024

cysjonathan Oct 22, 2024

cysjonathan Oct 22, 2024

cysjonathan Oct 22, 2024

		@@ -0,0 +1,48 @@
		class ApplySoftDeleteForCourseUserAndAssociatedModels1 < ActiveRecord::Migration[7.2]

Implement Soft-Delete for Students in Classes to Improve Performance #7588

Are you sure you want to change the base?

Implement Soft-Delete for Students in Classes to Improve Performance #7588

Conversation

phungmanhcuong commented Oct 9, 2024 • edited by cysjonathan Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cysjonathan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phungmanhcuong commented Oct 9, 2024 •

edited by cysjonathan

Loading

cysjonathan left a comment •

edited

Loading