Fix masterylog end timestamp issues #12870
Conversation
github-actions
bot
added
the
DEV: backend
Build Artifacts
|
rtibbles
force-pushed
the
end_your_mastery_timestamp
branch
from
b8690cc
to
297a82c
Compare
There was a problem hiding this comment.
After looking through everything I don't know if I'm quite qualified to fully give this the Approval - just came out the other side of my review w/ some non-blocking questions about it so I'll leave approval to @jredrejo
The Django upgrade handling stuff might be a really interesting quick demo. Mostly curious about what parts of it are automagical and how - I went into this expecting it to be a part of a migration but seeing the upgrade.py it looks a lot more like a Django ™️ feature for handling upgrades.
| @@ -57,3 +63,36 @@ def fix_duplicated_attempt_logs(): | |||
| item and non-null masterylog_id. | |||
| """ | |||
| consolidate_quiz_attempt_logs(AttemptLog.objects.all()) | |||
|
|
|||
|
|
|||
| @version_upgrade(old_version=">0.15.0,<0.18.0") | |||
There was a problem hiding this comment.
This just makes it so that the function below is only run when the current version is between these versions? Like if you are going from >0.15.0,<0.18.0 to anything else, this will run?
If so then what if you're upgrading from 0.15.2 to 0.17.3, then you upgrade from 0.17.3 to >=0.18.x would this end up being run again?
Not that it'd matter since it'd basically do nothing in that case since it'd have been fixed already, I'm just asking out of curiosity.
There was a problem hiding this comment.
Yes, except this code only exists in 0.18.0, so I would be mystified if it managed to run in 0.17.3 :)
| @parameterized.expand( | ||
| [ | ||
| ("0.15.8", ">=0.15.8", True), | ||
| ("0.15.7", ">=0.15.8", False), | ||
| ("0.15.8a5", ">=0.15.8", False), | ||
| ("0.15.9a5", ">=0.15.8", True), | ||
| ("0.15.8a5.dev0+git.682.g0be46de2", ">=0.15.8", False), | ||
| ("0.15.9a5.dev0+git.682.g0be46de2", ">=0.15.8", True), | ||
| ("0.15.9", ">0.15.8", True), |
There was a problem hiding this comment.
This looks like it was a lot of fun to write out - were you able to generate & format this list somehow or did you have to do it all by hand?
Also - what's going on here in general? 😅
There was a problem hiding this comment.
Copilot autocompleted it for me mostly incorrectly, and then I corrected it.
This is a list of testing parameters (each tuple is a set of parameters for the test) - to verify the version range checking behaviour.
rtibbles
force-pushed
the
end_your_mastery_timestamp
branch
from
297a82c
to
58671a5
Compare
It's a Kolibri ™️ feature for handling upgrades - mostly to do data migrations outside of the irritating straight jacket of the Django migration system - also because it's completely under our control, we can do non-Django operations in it, and also do the version control stuff specific to Kolibri. |
|
|
||
|
|
||
| @version_upgrade(old_version=">0.15.0,<0.18.0") | ||
| def fix_masterylog_end_timestamps(): |
There was a problem hiding this comment.
what would be now a KDP scenario?
There we have logs that have not run consolidate_quiz_attempt_logs mixed with logs that have it. And KDP identifies as 0.17.3, so it'd run fix_masterylog_end_timestamps
So, if I understand this correctly, in KDP we should either apply both upgrades, in order, or skip both. Correct?
There was a problem hiding this comment.
We should skip both, yes. Let the Kolibris sort out their data before syncing to KDP.
|
I think on reflection, coming to update this, I am going to remove the |
…ve deprecated calls. Update upgrade functionality to accept compound ranges.
rtibbles
force-pushed
the
end_your_mastery_timestamp
branch
from
58671a5
to
7925bfc
Compare
There was a problem hiding this comment.
Thanks, I've created https://github.com/learningequality/kolibri-data-portal/issues/838 to ensure we don't make KDP server struggle when upgrading to 0.18
Summary
References
Fixes #12855
Reviewer guidance
Are all three cases needed? Possible we could just check attempts and then content summary log.
…