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: add cache wrapper for can_view_courses filter #299

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

Ian2012
Copy link
Contributor

@Ian2012 Ian2012 commented Aug 15, 2023

This PR adds a cache wrapper for the can_view_courses function. The cache can be configured using the superset data_cache

It also removes the caching for the get_courses function such that people will get the latest courses at login.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 15, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Aug 15, 2023

Thanks for the pull request, @Ian2012! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@itsjeyd itsjeyd added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Aug 16, 2023
@itsjeyd
Copy link

itsjeyd commented Aug 16, 2023

Thanks for these changes @Ian2012!

@Ian2012 Ian2012 force-pushed the cag/add-cache-to-filters branch 2 times, most recently from 45f6298 to e0af695 Compare August 25, 2023 16:01
@Ian2012 Ian2012 changed the title WIP fix: add cache wrapper for can_view_courses filter fix: add cache wrapper for can_view_courses filter Aug 25, 2023
@Ian2012 Ian2012 marked this pull request as ready for review August 25, 2023 16:02
Copy link
Contributor

@SoryRawyer SoryRawyer left a comment

Choose a reason for hiding this comment

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

Looks good! I tried memoized_func locally and it seemed to work as expected so I think this should help cut down on dashboard loading times.

Copy link
Contributor

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

Seems to do the right thing, I'm not sure about removing the get_courses memo though. I think it would still get called repeatedly for different field names, which is likely to happen a few times in quick succession in a dashboard load. Current cache time for all caches if 5 mins, that seems reasonable enough for getting fresh permissions. Since it was monitoring the access key it would get refreshed on a new login as well. What do you think?

@Ian2012
Copy link
Contributor Author

Ian2012 commented Aug 25, 2023

We would have two caches, right?

Once for the get_courses and another one for the macro

@bmtcril
Copy link
Contributor

bmtcril commented Aug 25, 2023

Right, so if the macro got called with "course id" and then again with "course key" theoretically we wouldn't have to hit the LMS twice.

@Ian2012 Ian2012 merged commit ff530c8 into main Aug 25, 2023
5 checks passed
@Ian2012 Ian2012 deleted the cag/add-cache-to-filters branch August 25, 2023 18:10
@openedx-webhooks
Copy link

@Ian2012 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@itsjeyd itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants