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

Expose course rosters on the dashboard #6925

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

marcospri
Copy link
Member

@marcospri marcospri commented Dec 19, 2024

Although we don't expose a list of students with metrics for a full course we do list students of a course on the dropdowns.

This commit uses the roster data for the dropdown when the feature flag is enabled and the data is available.

Testing

  • Make sure the Enable roster data is enabled for the LTI1.3 applications instance

http://localhost:8001/admin/instances/102/settings

  • Launch the following assignment

https://hypothesis.instructure.com/courses/319/assignments/3308

  • Fetch the rosters for it, in make shell
from lms.tasks.roster import schedule_fetching_rosters

schedule_fetching_rosters.delay()
  • Open the dashboard for the assignment

  • Navigate un level up, to the course, open the student drop down. It includes all students in the course, not only the ones that have launched.

Although we don't expose a list of students with metrics for a full course
we do list students of a course on the dropdowns.

This commit uses the roster data for the dropdown when the feature flag is enabled
and the data is aviailable.
@@ -210,7 +211,40 @@ def get_assignment_roster(
assignment_id=assignment.id,
h_userids=h_userids,
# For launch data we always add the "active" column as true for compatibility with the roster query.
).add_columns(True)
).add_columns(true())
Copy link
Member Author

Choose a reason for hiding this comment

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

true() is the right type for this.

.join(LTIRole)
.where(
CourseRoster.lms_course_id == lms_course.id,
CourseRoster.active.is_(True),
Copy link
Member Author

Choose a reason for hiding this comment

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

We now return all rows, not only active ones

@@ -46,34 +46,45 @@ def get_course_roster(
lms_course: LMSCourse,
role_scope: RoleScope | None = None,
role_type: RoleType | None = None,
) -> Select[tuple[LMSUser]]:
h_userids: list[str] | None = None,
) -> Select[tuple[LMSUser, bool]]:
Copy link
Member Author

Choose a reason for hiding this comment

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

We return a tuple of (LMSUser, active)

@marcospri marcospri requested a review from acelaya January 13, 2025 10:40
Copy link
Contributor

@acelaya acelaya 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 in general, and it works as described.

lms/services/dashboard.py Outdated Show resolved Hide resolved
Segment rosters are used a bit differently than assignments ones.

We allow filtering by multiple rosters and we'll display a list of those students with the metrics. For assignments we only allow to select one assignment at the time.

This adds a bit of extra complexity as we need to query the roster as a combination of rosters
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.

2 participants