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 view learn device regression #13138

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

ozer550
Copy link
Member

@ozer550 ozer550 commented Mar 3, 2025

Summary

  • Only show "View learner devices" link when a learner with LOD setup actually exists.
  • Only show learners that are using devices with LOD in ClassLearnersListPage.
lod-link-fix.mp4

References

closes #12947

Reviewer guidance

  • Create a facility, users, and class where there is a coach and learner(s) but the learners are NOT on LODs
  • Go to the exams page
    Observe that in the main exam page, and when clicking into a specific exam, see that the "View learner devices" link (and link to that page) is absent.
  • Not setup the learner on LOD. Observe that "View learner devices" link is present and only shows the learners which are present on the lod.

@ozer550 ozer550 requested a review from nucleogenesis March 3, 2025 07:28
@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Mar 3, 2025
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

I tested things out locally and things worked as expected. Overall, the code changes LGTM so once the tests are fixed up should be good to go.

@ozer550
Copy link
Member Author

ozer550 commented Mar 4, 2025

I tested things out locally and things worked as expected. Overall, the code changes LGTM so once the tests are fixed up should be good to go.

Done!

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Hi @ozer550 - I think you're on the right track here, but there are a few adjustments needed to stay fully in line with the original intention of the issue/fix. I know some of the different learner setups can be complicated, so let me know if you want to discuss the scenarios more! I think it might make more sense once you can fully wrap your mind around picturing what this could look like in a classroom

@ozer550 ozer550 requested a review from marcellamaki March 6, 2025 06:42
@nucleogenesis nucleogenesis self-assigned this Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Properly conditionalize "View Learner Devices" in Coach
3 participants