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

fix: AA-1058: Bust cache when Schedules are updated #117

Merged
merged 1 commit into from
Oct 21, 2021

Conversation

Dillon-Dumesnil
Copy link
Contributor

@Dillon-Dumesnil Dillon-Dumesnil commented Oct 20, 2021

Description: We had a bug reported where learners would reset their due dates
for their Personalized Learner Schedule, but the dates wouldn't
update. This was a result of the cache key not changing, so this
fix adds in the learner's schedule to the transformer.

JIRA: https://openedx.atlassian.net/browse/AA-1058

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed
  • PR author is listed in AUTHORS

Post merge:

  • Create a tag
  • Check new version is pushed to PyPi after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Copy link

@MatthewPiatetsky MatthewPiatetsky left a comment

Choose a reason for hiding this comment

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

i think this could use a test as well as a small explanation for the block on line 10 so someone browsing the code can easily understand

if Schedule:
try:
schedule = Schedule.objects.get(
enrollment__user=self.user, enrollment__course__id=usage_info.course_key
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so this is basically manually doing the needs_schedule flow inside get_dates_for_course, but bringing it outside, so that the schedule can be part of the cache key?

Would another solution be to look up the schedule inside get_dates if needs_schedule is true and then add it to the cache key (rather than making the cache key before looking up the schedule), so that callers don't have to work around this quirk?

Copy link

Choose a reason for hiding this comment

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

Dumb question, How does this bust the cache? Just trying to understand what's happening here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdeery Not a dumb question at all! There is a lot of institutional knowledge that is working behind the scenes here.

So the issue has to do with Personalized Learning Schedules for our self-paced users where we base their due dates off of their Schedule start date. Verified learners are able to reset their due dates if they fall behind which will update their Schedule start date to today's date and thus push all of their deadlines down the road giving them time to work on the course again. The issue here is that after learners reset their due dates, edx-when was still finding their old cached dates and so when the page reloaded, none of their due dates were reset for them despite the backend/database knowing the start date changed. This busts the cache because, if provided, the schedule start date becomes part of the cache key. So now when a learner resets their due dates and we fetch their schedule, it will result in a new cache key and us returning the correct dates.

I hope that clears it up, but if not, please follow-up!

Copy link

Choose a reason for hiding this comment

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

Thanks! That's super helpful

@Dillon-Dumesnil
Copy link
Contributor Author

Update: Going with MT's idea to move it down a level so other people/us don't hit this again in the future. Will attempt a test!

@Dillon-Dumesnil Dillon-Dumesnil force-pushed the ddumesnil/update-transformer-aa-1058 branch 4 times, most recently from c80afb0 to 14d42b4 Compare October 21, 2021 16:28
We had a bug reported where learners would reset their due dates
for their Personalized Learner Schedule, but the dates wouldn't
update. This was a result of the cache key not changing, so this
fix adds in the learner's schedule to the transformer.
@Dillon-Dumesnil Dillon-Dumesnil force-pushed the ddumesnil/update-transformer-aa-1058 branch from 14d42b4 to 2566443 Compare October 21, 2021 16:35
Copy link
Contributor

@mikix mikix left a comment

Choose a reason for hiding this comment

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

Nice

Copy link

@MatthewPiatetsky MatthewPiatetsky left a comment

Choose a reason for hiding this comment

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

failing codecov but if mike terry is fine with it then i am too :shipit:

@Dillon-Dumesnil Dillon-Dumesnil merged commit 5764a6a into master Oct 21, 2021
@Dillon-Dumesnil Dillon-Dumesnil deleted the ddumesnil/update-transformer-aa-1058 branch October 21, 2021 18:20
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.

4 participants