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

MBS-8702: Improve caching #93

Merged
merged 13 commits into from
Feb 21, 2024
Merged

MBS-8702: Improve caching #93

merged 13 commits into from
Feb 21, 2024

Conversation

sh-csg
Copy link
Contributor

@sh-csg sh-csg commented Jan 31, 2024

No description provided.

@sh-csg sh-csg added this to the 1.0 milestone Jan 31, 2024
@sh-csg sh-csg self-assigned this Jan 31, 2024
@sh-csg sh-csg force-pushed the MBS-8702-Improve-caching branch 3 times, most recently from 3500067 to 687e7db Compare January 31, 2024 17:08
Copy link
Member

@PhMemmel PhMemmel left a comment

Choose a reason for hiding this comment

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

see comments in the code

classes/task/fill_backlink_cache.php Outdated Show resolved Hide resolved
lib.php Outdated Show resolved Hide resolved
db/tasks.php Outdated Show resolved Hide resolved
@sh-csg sh-csg force-pushed the MBS-8702-Improve-caching branch from 687e7db to 9ba6ca0 Compare February 16, 2024 14:20
@sh-csg sh-csg force-pushed the MBS-8702-Improve-caching branch from 9ba6ca0 to ff95cac Compare February 19, 2024 06:46
classes/task/fill_backlink_cache.php Show resolved Hide resolved
classes/task/fill_backlink_cache.php Show resolved Hide resolved
classes/task/fill_backlink_cache.php Show resolved Hide resolved
lib.php Outdated Show resolved Hide resolved
tests/mod_learningmap_backlink_cache_test.php Outdated Show resolved Hide resolved
tests/mod_learningmap_backlink_cache_test.php Outdated Show resolved Hide resolved
classes/cachemanager.php Outdated Show resolved Hide resolved
classes/cachemanager.php Outdated Show resolved Hide resolved
classes/cachemanager.php Outdated Show resolved Hide resolved
classes/cachemanager.php Show resolved Hide resolved
classes/cachemanager.php Show resolved Hide resolved
classes/cachemanager.php Show resolved Hide resolved
classes/cachemanager.php Show resolved Hide resolved
classes/autoupdate.php Show resolved Hide resolved
'callback' => '\mod_learningmap\autoupdate::reset_backlink_cache',
'callback' => '\mod_learningmap\autoupdate::reset_backlink_cache_if_necessary',
],
[
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment that this is needed in case the course format changed. Maybe it would also be nice to make a separate callback function handling this event because it should be possible to extract if the event actually was caused by a course format change?

Currently the cache is rebuilt on every course update, right?

Copy link
Member

@PhMemmel PhMemmel left a comment

Choose a reason for hiding this comment

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

While testing it turns out that only one link is being generated, even if an activity belongs to two learningmaps. Can you confirm?

Also: Is it intentional that there is a backlink shown despite the activity is not yet accessible to the student?

@PhMemmel
Copy link
Member

PhMemmel commented Feb 21, 2024

While testing it turns out that only one link is being generated, even if an activity belongs to two learningmaps. Can you confirm?

backlink config was not present in backup routine, fixed

Also: Is it intentional that there is a backlink shown despite the activity is not yet accessible to the student?

Yes :)

Copy link
Member

@PhMemmel PhMemmel left a comment

Choose a reason for hiding this comment

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

Great job! Happy to merge!

@PhMemmel PhMemmel merged commit 5ac6e32 into master Feb 21, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants