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

Schedule fetching canvas groups rosters #6906

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Conversation

marcospri
Copy link
Member

@marcospri marcospri commented Dec 6, 2024

Use the same logic as for assignments and courses.

For:

Testing

  • Truncate any segment rosters you might have.
truncate lms_segment_roster ;
TRUNCATE TABLE
  • Launch an LTI1.3 groups assignment

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

  • On make shell, schedule fetching rosters
from lms.tasks.roster import schedule_fetching_rosters
schedule_fetching_rosters.delay()

segment roster fetching is scheduled:

worker (stderr)      | [2024-12-09 10:09:17,408: INFO/ForkPoolWorker-1] lms.tasks.roster.fetch_segment_roster[3da79f05-6966-4626-a447-1ba2c3a9898c] Task lms.tasks.roster.fetch_segment_roster[3da79f05-6966-4626-a447-1ba2c3a9898c] succeeded in 0.6005973350256681s: None
worker (stderr)      | [2024-12-09 10:09:17,531: INFO/ForkPoolWorker-2] lms.tasks.roster.fetch_segment_roster[b947bcf5-1c92-4557-84f0-385c1a0ca838] Task lms.tasks.roster.fetch_segment_roster[b947bcf5-1c92-4557-84f0-385c1a0ca838] succeeded in 0.7232479890226386s: None
  • We have now roster data for those segments on the DB:
select name, lms_user_id from lms_segment_roster join lms_segment on lms_segment.id = lms_segment_id;
  name   | lms_user_id 
---------+-------------
 Group 2 |        2054
 Group 2 |        2055
 Group 2 |        2056
 Group 1 |        2050
(4 rows)
  • Schedule it again:

schedule_fetching_rosters.delay()

No segment roster fetch is scheduled now.

  • Remove task_done rows
truncate task_done ; 
  • Schedule the fetch again, nothing happens yet

  • Change the date of the existing roster:

 update lms_segment_roster set updated = now() - '1 month'::interval;
  • Schedule again, this time both rosters are re-fetched

  • Lets now remove the task done and mark the roster as old:

truncate task_done ;
update lms_segment_roster set updated = now() - '1 month'::interval;
  • We'll also mark the launch event on the course as old:
 update event set timestamp = now() - '1 month'::interval where id in (select event.id from event join grouping on course_id = grouping.id where lms_name = 'LTI 1.3 Testing' order by timestamp desc limit 1);
  • Scheduling the fetching again doesn't trigger a segment roster fetch this time.

@hypothesis hypothesis deleted a comment from sentry-io bot Dec 6, 2024
@marcospri marcospri force-pushed the schedule-segment-roster branch from e37a1bf to 77f4759 Compare December 6, 2024 14:05
@@ -156,6 +158,69 @@ def schedule_fetching_assignment_rosters() -> None:
)


def schedule_fetching_segment_rosters() -> None:
Copy link
Member Author

Choose a reason for hiding this comment

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

This is very similar to the existing assignment and course schedulers.

# Only fetch roster for segments for which we haven't schedule a fetch recently
no_recent_scheduled_roster_fetch_clause = ~exists(
select(TaskDone).where(
TaskDone.key == func.concat("roster::segment::scheduled::", LMSSegment.id),
Copy link
Member Author

Choose a reason for hiding this comment

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

Key for segments

.join(Course, Event.course_id == Course.id)
.where(
Event.timestamp >= now - LAUNCHED_WINDOW,
Course.authority_provided_id == LMSCourse.h_authority_provided_id,
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 based this on any launches on the courses the segments belong to.

no_recent_scheduled_roster_fetch_clause,
recent_launches_clause,
# Only canvas groups for now
LMSSegment.type == "canvas_group",
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 are limiting segments to only "canvas_group"s only

@@ -180,9 +245,25 @@ def fetch_course_roster(*, lms_course_id) -> None:
retry_backoff_max=7200,
)
def fetch_assignment_roster(*, assignment_id) -> None:
"""Fetch the roster for one course."""
"""Fetch the roster for one assignment."""
Copy link
Member Author

Choose a reason for hiding this comment

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

Fix existing comment.

@marcospri marcospri requested a review from acelaya December 9, 2024 09:22
@marcospri marcospri marked this pull request as ready for review December 9, 2024 09:22
@marcospri marcospri force-pushed the schedule-segment-roster branch from 77f4759 to 48080f3 Compare December 9, 2024 09:23
Use the same logic as for assignments and courses.
@marcospri marcospri force-pushed the schedule-segment-roster branch from 48080f3 to 4509b1f Compare December 10, 2024 09:30
@marcospri marcospri merged commit 52498bd into main Dec 10, 2024
9 checks passed
@marcospri marcospri deleted the schedule-segment-roster branch December 10, 2024 09:58
Copy link

sentry-io bot commented Dec 10, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

Did you find this useful? React with a 👍 or 👎

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