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

Conversation

jorg-vr
Copy link
Contributor

@jorg-vr jorg-vr commented Oct 30, 2023

This pull request adds an active job that permanently destroys removed 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

This job should be started once and reschedules itself every month.


Some relevant stats from naos:

> Exercise.where(status: 'removed').where('updated_at < ?', 1.month.ago).count
411
>Exercise.where(status: 'removed').where('updated_at < ?', 1.month.ago).filter { |activity| activity.draft? || ( activity.series_memberships.empty? && ( activity.submissions.empty? || (activity.submissions.count < 25 && activity.submissions.order(:created_at).last.created_at < 1.month.ago))) }.count
366
> ContentPage.where(status: 'removed').where('updated_at < ?', 1.month.ago).count
18
> ContentPage.where(status: 'removed').where('updated_at < ?', 1.month.ago).filter { |activity| activity.draft? || activity.series_memberships.empty? }.count
16
  • tests are added

Closes #4969 and #4764

Sorry, something went wrong.

@jorg-vr jorg-vr added the enhancement A change that isn't substantial enough to be called a feature label Oct 30, 2023
@jorg-vr jorg-vr self-assigned this Oct 30, 2023
@jorg-vr jorg-vr requested a review from a team as a code owner October 30, 2023 15:03
@jorg-vr jorg-vr requested review from bmesuere and niknetniko and removed request for a team October 30, 2023 15:03
Copy link
Member

@niknetniko niknetniko left a comment

Choose a reason for hiding this comment

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

While the code looks good, does this need a test? Or it might be too much work to properly test, I don't know.

Copy link
Member

@bmesuere bmesuere left a comment

Choose a reason for hiding this comment

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

The code and criteria look good, but I have two remarks.

Right now, from the code, it isn't clear what the side effects will be: if a draft exercise was added to a series, will it be removed from the series? If it has submissions, will those be removed? Maybe add this in comments.

Running this as a cronjob is ok, but we don't have any other tasks like this right now. We should therefore discuss if this is the best way to run this. An alternative is using delayed job and a function that schedules itself as a last step of its execution.

lib/tasks/remove_activities.rb Outdated Show resolved Hide resolved
@bmesuere bmesuere requested a review from chvp November 6, 2023 19:12
config/deploy/production.rb Outdated Show resolved Hide resolved
Comment on lines +16 to +21
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
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

app/jobs/remove_activities_job.rb Outdated Show resolved Hide resolved
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.

config/deploy/production.rb Outdated Show resolved Hide resolved
@jorg-vr jorg-vr requested a review from chvp November 7, 2023 16:01
@jorg-vr jorg-vr merged commit e4a4977 into main Nov 10, 2023
13 checks passed
@jorg-vr jorg-vr deleted the chore/remove-activities branch November 10, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A change that isn't substantial enough to be called a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto remove deleted exercises
4 participants