Fix masterylog end timestamp issues #12870

rtibbles · 2024-11-21T00:26:35Z

Summary

Fixes bug and adds regression test to ensure the end_timestamp on masterylogs get properly updated after initialization
Adds tests for and updates behaviour to ensure that our upgrade machinery can handle compound version ranges
Adds upgrade function to remediate the masterylog end timestamp issues if the upgrade is from an affected Kolibri version
Updates any masterlogs whose end timestamp is the same as the start timestamp, as would be the case for those affected by this bug
Sets to the end timestamp of the most recent attempt (if any)
Otherwise set to the end timestamp of the content summary log

References

Fixes #12855

Reviewer guidance

Are all three cases needed? Possible we could just check attempts and then content summary log.

…

github-actions · 2024-11-21T00:58:12Z

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.18.0a0.dev0_git.4.gedd03569.pex
Windows Installer (EXE)	kolibri-0.18.0a0.dev0+git.4.gedd03569-windows-setup-unsigned.exe
Debian Package	kolibri_0.18.0a0.dev0+git.4.gedd03569-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.18.0a0.dev0+git.4.gedd03569.dmg
Android Package (APK)	kolibri-0.18.0a0.dev0+git.4.gedd03569-0.1.4-debug.apk
TAR file	kolibri-0.18.0a0.dev0+git.4.gedd03569.tar.gz
WHL file	kolibri-0.18.0a0.dev0+git.4.gedd03569-py2.py3-none-any.whl

nucleogenesis

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.

nucleogenesis · 2024-11-21T22:56:59Z

kolibri/core/logger/upgrade.py

@@ -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")


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.

Yes, except this code only exists in 0.18.0, so I would be mystified if it managed to run in 0.17.3 :)

nucleogenesis · 2024-11-21T23:02:58Z

kolibri/utils/tests/test_version.py

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


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? 😅

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 · 2024-11-22T00:25:11Z

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.

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.

jredrejo

Code seems to work right and I see it correct. I have a couple of comments and would like to know your opinion before approving it.
Thanks!

jredrejo · 2024-11-22T16:07:39Z

kolibri/core/logger/api.py

@@ -611,7 +611,8 @@ def _update_and_return_mastery_log_id(
                    mastery_level=context["mastery_level"],
                    summarylog_id=summarylog_id,
                )
-                update_fields = tuple()
+                update_fields = ("end_timestamp", "_morango_dirty_bit")


wondering if we could win some performance doing this inside the if time_spent_delta: condition below. If there has not been any difference of time we don't need to update it nor force morango to sync the dates

Yes, that does make sense, and preserves the original intention of the code better.

jredrejo · 2024-11-22T16:09:36Z

kolibri/core/logger/upgrade.py

+
+
+@version_upgrade(old_version=">0.15.0,<0.18.0")
+def fix_masterylog_end_timestamps():


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?

We should skip both, yes. Let the Kolibris sort out their data before syncing to KDP.

rtibbles · 2024-11-22T21:18:31Z

I think on reflection, coming to update this, I am going to remove the completion_timestamp condition - the attempt logs should cover it, and do so more accurately.

…ve deprecated calls. Update upgrade functionality to accept compound ranges.

rtibbles requested a review from jredrejo November 21, 2024 00:26

github-actions bot added the DEV: backend Python, databases, networking, filesystem... label Nov 21, 2024

rtibbles added this to the Kolibri 0.18: General maintenance milestone Nov 21, 2024

rtibbles force-pushed the end_your_mastery_timestamp branch from b8690cc to 297a82c Compare November 21, 2024 01:52

nucleogenesis reviewed Nov 22, 2024

View reviewed changes

rtibbles force-pushed the end_your_mastery_timestamp branch from 297a82c to 58671a5 Compare November 22, 2024 00:23

jredrejo reviewed Nov 22, 2024

View reviewed changes

rtibbles added 3 commits November 22, 2024 13:21

Ensure mastery log end_timestamp is consistently updated.

a6d70b0

Add tests for compound version range comparisons. Fix behaviour. Remo…

e78979b

…ve deprecated calls. Update upgrade functionality to accept compound ranges.

Add upgrade to remediate end_timestamps for mastery logs.

7925bfc

rtibbles force-pushed the end_your_mastery_timestamp branch from 58671a5 to 7925bfc Compare November 22, 2024 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix masterylog end timestamp issues #12870

Fix masterylog end timestamp issues #12870

rtibbles commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

nucleogenesis left a comment

nucleogenesis Nov 21, 2024

rtibbles Nov 22, 2024

nucleogenesis Nov 21, 2024

rtibbles Nov 22, 2024

rtibbles commented Nov 22, 2024

jredrejo left a comment

jredrejo Nov 22, 2024

rtibbles Nov 22, 2024

rtibbles Nov 22, 2024

jredrejo Nov 22, 2024

rtibbles Nov 22, 2024

rtibbles commented Nov 22, 2024



		@version_upgrade(old_version=">0.15.0,<0.18.0")
		def fix_masterylog_end_timestamps():

Fix masterylog end timestamp issues #12870

Are you sure you want to change the base?

Fix masterylog end timestamp issues #12870

Conversation

rtibbles commented Nov 21, 2024 • edited Loading

Summary

References

Reviewer guidance

github-actions bot commented Nov 21, 2024 • edited Loading

nucleogenesis left a 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

rtibbles commented Nov 22, 2024

jredrejo left a 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

rtibbles commented Nov 22, 2024

rtibbles commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading