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
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.

Thank you for the updates, @ozer550. This looks good and manual QA confirms it is working now as expected. Can you rebase, and then we can merge?(Let's use a squash merge for this :) )

@ozer550 ozer550 force-pushed the fix-view-learn-device-regression branch from f7dc54c to e247d00 Compare March 14, 2025 16:18
@@ -74,19 +74,33 @@
default: false,
},
},
data() {
return {
userSet: new Set(),
Copy link
Member

Choose a reason for hiding this comment

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

We should be a little careful with this, as VueJS < 3 does not support reactive updates of ES6 Set objects: vuejs/vue#2410

I think in this case, the fact that we're always replacing the entire object is probably sufficient for this to retrigger a reactive update in the computed prop, but please be cautious in general with using Set and Map. This is the reason that we often just us a plain JS object when a Set or a Map might be more performant/semantic.

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
4 participants