-
Notifications
You must be signed in to change notification settings - Fork 662
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
[Data rearchitecture] Add first_revision field to article course timeslices #6242
Conversation
…thod. It incorrectly fetched all CWTs for revisions between some dates, without taking into account wiki_id. This caused the same timeslice dates to be processed multiple times. The operation is idempotent so the results were correct, but had unnecesary processing.
Code looks good to me. |
|
||
ArticleCourseTimeslice.where.not(revision_count: 0).in_batches do |act_batch| | ||
# rubocop:disable Rails/SkipsModelValidations | ||
act_batch.update_all('first_revision = start') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I've ever done an update_all like this with a string argument, but I assume you've tested this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I ran it locally. In data-rearchitecture I ran the raw SQL query because I have problems to open a rails console:
UPDATE article_course_timeslices
SET first_revision = start
WHERE revision_count != 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. I usually can't open a rails console in a new production server until I jump through some hoops related to installing binstubs, but I always forget exactly what commands are needed (and forget to add them to the docs).
What this PR does
This PR adds a new
first_revision
ACT field to make existingfirst_revision
article course field depend only on it.In order to merge and deploy this we should:
first_revision
field to ACT.first_revision
fields. The basic idea will be, if therevision_count
field is 0, then keep thefirst_revision
as nil. If therevision_count
is different from 0, then setfirst_revision
to start field.Explanation
While the solution we implemented in PR #6230 works fine for most cases, there are some specific edge cases where it could have an incorrect value. For example:
1- The student who made the first revision for a given article is removed from the course. While some CWTs will be marked as needs_update, the first_revision field in AC won't be updated (assuming that other students have also updated the article; otherwise, the AC is deleted).
2- The start date for a course changes to a later date. While all timeslices for dates before the new course start date are deleted, ACs with edits also after the new course start date persist. This means that the first_revision field for those ACs will reflect a date that is before the new course start date.
These are the two cases where we'll have an incorrect first_revision date.
A better approach is to make the first_revision field work similarly to the other AC metrics—that is, by depending only on the ACT values. To do that, the only way is to add a first_revision field to the ACT table and calculate the AC first_revision field based on the first_revision value of the earliest (min) ACT. If we do that, removing a user and their corresponding ACTs won't cause issues, because the AC first_revision will update to the first_revision from the new minimum ACT. This PR implements this approach
Open questions and concerns
Note: commit f68c376 is not related to these changes. It's bug that I found when working on this so I fix it.