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

Avoid caching when getting course structure #7654

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

merkushin
Copy link
Member

@merkushin merkushin commented Aug 1, 2024

Resolves #7146

Proposed Changes

  • Avoid caching when getting course structure

Testing Instructions

  1. Go to Sensei LMS > Courses, create a course with modules and lessons in them.
  2. Go to Sensei LMS > Modules, change the order of the modules for the course. Save it.
  3. Go to Sensei LMS > Lessons, change the order of the lessons in modules and outside the modules. Save it.
  4. Open the course on the frontend and check the order is the same you set on previous steps.

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Legacy courses (course without blocks) are tested
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

@merkushin merkushin added this to the 4.24.2 milestone Aug 1, 2024
Copy link

github-actions bot commented Aug 1, 2024

Test the previous changes of this PR with WordPress Playground.

@merkushin merkushin linked an issue Aug 1, 2024 that may be closed by this pull request
@merkushin merkushin self-assigned this Aug 1, 2024
@merkushin merkushin requested a review from a team August 1, 2024 22:56
@merkushin merkushin force-pushed the fix/module-lesson-order branch from 041bc28 to 5f7ffb7 Compare August 1, 2024 23:05
Copy link

github-actions bot commented Aug 1, 2024

Test the previous changes of this PR with WordPress Playground.

@donnapep
Copy link
Collaborator

donnapep commented Aug 2, 2024

@merkushin I noticed this one is in Draft but assigned to Nexus. Is it ready for review?

@renatho
Copy link
Contributor

renatho commented Aug 2, 2024

Hey @merkushin! As per your concern about performance that you mentioned to me, I think it's very important and it would be worth checking where this cache would be skipped to see if it could have a negative impact.

That said, I tested it in Pressable and I still see the issue here:

Screen.Recording.2024-08-02.at.15.24.10.mov

@merkushin
Copy link
Member Author

@donnapep not yet. It is unfinished yet, especially considering Renatho's reply.

I asked Renatho to look at it as he had some experience with addressing a similar issue.
And I had a concern about performance (Renatho mentioned it in his comment here: #7654 (comment)).

@donnapep donnapep removed the request for review from a team August 2, 2024 19:16
@donnapep
Copy link
Collaborator

donnapep commented Aug 2, 2024

I asked Renatho to look at it as he had some experience with addressing a similar issue.

OK, in that case I'm going to remove Nexus as reviewer so that I don't get notifications in my inbox. Not sure there's a way to disable those.

@donnapep donnapep modified the milestones: 4.24.2, 4.24.3 Aug 7, 2024
@donnapep donnapep removed this from the 4.24.3 milestone Aug 22, 2024
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.

Cache issues with lesson ordering
3 participants