Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create task that destroys removed activities #5095

Merged
merged 7 commits into from
Nov 10, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions app/jobs/remove_activities_job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
class RemoveActivitiesJob < ApplicationJob
# permanently remove activities that match all of the following criteria:
# - status is 'removed'
# - updated_at is more than 1 month ago
# - one of the following is true:
# - draft is true (never published)
# - series_memberships is empty and less then 25 submissions and latest submission is more than 1 month ago
#
# Destroy is called on each activity individually to ensure that callbacks are run
# This means the activity will be removed from any series, evaluations it is a member of
# and any submissions will be removed
queue_as :cron

def perform
ContentPage.where(status: 'removed').where('updated_at < ?', 1.month.ago).find_each do |activity|
if activity.draft? || activity.series_memberships.empty?
# destroy series memberships first explicitly, as they are dependent: :restrict_with_error
activity.series_memberships.destroy_all

activity.destroy
end
Comment on lines +16 to +21
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work if the content page has been marked as read? And if so, do we want it to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works with activity_read_states (see the first test)

We could also add a limit of at most 25 activity_read_states, but as a lot less information is lost in the case of the activity_read_states compared to submissions, I did not care about it

end

Exercise.where(status: 'removed').where('updated_at < ?', 1.month.ago).find_each do |activity|
if activity.draft? ||
(activity.series_memberships.empty? &&
(activity.submissions.empty? ||
(activity.submissions.count < 25 && activity.submissions.reorder(:created_at).last.created_at < 1.month.ago)))
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
# destroy submissions first explicitly, as they are dependent: :restrict_with_error
activity.submissions.destroy_all

# destroy series memberships first explicitly, as they are dependent: :restrict_with_error
activity.series_memberships.destroy_all

activity.destroy
end
end

# rerun this job in 1 month
RemoveActivitiesJob.set(wait: 1.month).perform_later
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also delayed_cron_job that takes care of this, but we might not want to introduce a dependency to replace one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no preference, do you want me to replace this functionality?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave the code as-is for now. If we add more similar tasks later, we can always add the gem.

end
end
2 changes: 1 addition & 1 deletion config/deploy/production.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
set :delayed_job_pools_per_server,
'dodona' => {
'default,statistics,exports,cleaning' => 2,
'git' => 1,
'git,cron' => 1,
jorg-vr marked this conversation as resolved.
Show resolved Hide resolved
},
'sisyphus' => {
'submissions,low_priority_submissions,high_priority_submissions' => 6
Expand Down
105 changes: 105 additions & 0 deletions test/jobs/remove_activities_job_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
require 'test_helper'

class RemoveActivitiesJobTest < ActiveJob::TestCase
test 'should remove "removed" draft activities' do
c = create :content_page
create :activity_read_state, activity: c
c.update status: :removed, draft: true, updated_at: 2.months.ago
e = create :exercise
create :submission, exercise: e
e.update status: :removed, draft: true, updated_at: 2.months.ago
s = create :series
s.activities << c
s.activities << e

assert_difference 'ContentPage.count', -1 do
assert_difference 'Exercise.count', -1 do
RemoveActivitiesJob.perform_now
end
end
end

test 'should remove "removed" activities with no series memberships and no submissions' do
create :content_page, status: :removed, draft: false, updated_at: 2.months.ago
create :exercise, status: :removed, draft: false, updated_at: 2.months.ago

assert_difference 'ContentPage.count', -1 do
assert_difference 'Exercise.count', -1 do
RemoveActivitiesJob.perform_now
end
end
end

test 'should not remove "removed" activities with series memberships' do
c = create :content_page, status: :removed, draft: false, updated_at: 2.months.ago
e = create :exercise, status: :removed, draft: false, updated_at: 2.months.ago
s = create :series
s.activities << c
s.activities << e

assert_no_difference 'ContentPage.count' do
assert_no_difference 'Exercise.count' do
RemoveActivitiesJob.perform_now
end
end
end

test 'should not remove "removed" activities with more than 25 submissions' do
e = create :exercise, status: :removed, draft: false, updated_at: 2.months.ago
create_list :submission, 26, exercise: e

assert_no_difference 'Exercise.count' do
RemoveActivitiesJob.perform_now
end
end

test 'should remove "removed" activities with less than 25 submissions and last submission more than 1 month ago' do
e = create :exercise, status: :removed, draft: false, updated_at: 2.months.ago
create_list :submission, 21, exercise: e, created_at: 2.months.ago

assert_difference 'Submission.count', -21 do
assert_difference 'Exercise.count', -1 do
RemoveActivitiesJob.perform_now
end
end
end

test 'should not remove "removed" activities with less than 25 submissions and last submission less than 1 month ago' do
e = create :exercise, status: :removed, draft: false, updated_at: 2.months.ago
create_list :submission, 5, exercise: e, created_at: 2.months.ago
create_list :submission, 3, exercise: e, created_at: 2.weeks.ago

assert_no_difference 'Submission.count' do
assert_no_difference 'Exercise.count' do
RemoveActivitiesJob.perform_now
end
end
end

test 'should not removed non removed activities' do
create :exercise, updated_at: 2.months.ago
create :content_page, updated_at: 2.months.ago
create :exercise, draft: true, updated_at: 2.months.ago
create :content_page, draft: true, updated_at: 2.months.ago

assert_no_difference 'Exercise.count' do
assert_no_difference 'ContentPage.count' do
RemoveActivitiesJob.perform_now
end
end
end

test 'should not remove activities updated less than 1 month ago' do
create :exercise, status: :removed, draft: true, updated_at: 2.weeks.ago

assert_no_difference 'Exercise.count' do
RemoveActivitiesJob.perform_now
end
end

test 'should reschedule itself' do
assert_enqueued_with(job: RemoveActivitiesJob) do
RemoveActivitiesJob.perform_now
end
end
end