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

Store the API ID for canvas courses on LMSCourse #6944

Open
wants to merge 2 commits into
base: api-course-id-model
Choose a base branch
from

Conversation

marcospri
Copy link
Member

@marcospri marcospri commented Jan 8, 2025

For:

Course have a LTI identifier that it's send on every launch but also have a API only ID that's needed to talk to the proprietary API of each LMS.

We do store the canvas version of this ID in a json dict in Grouping.extra but this train of PR promotes it to its own column.

Testing

  • Apply the migration
    tox -e dev --run-command 'alembic upgrade head'

  • Log into canvas (eng+canvasteacher@hypothes.is) and launch:

https://hypothesis.instructure.com/courses/125/assignments/875

  • Check the correct value (125) is present on lms_course now, in make sql:
select * from lms_course where lms_api_course_id = '125';


-[ RECORD 1 ]---------------+----------------------------------------------------
id                          | 1056
tool_consumer_instance_guid | VCSy8DAKCFIQYaItpSGyXhOXIlk5A6NWduVVG1u3:canvas-lms
lti_context_id              | f3cd019017839c4630358662a05540f2f6ec5f93
h_authority_provided_id     | 653e9620a656a954c684942f6443fa3e3410c03a
copied_from_id              | 
name                        | Developer Test Course with Sections Enabled
created                     | 2025-01-07 10:57:47.775449
updated                     | 2025-01-08 13:01:36.984534
lti_context_memberships_url | 
starts_at                   | 
ends_at                     | 
lms_api_course_id           | 125

Test invalid dates which produce a ValueError in the course service.
@marcospri marcospri changed the title Api course id code Store the API ID for canvas courses on LMSCourse Jan 8, 2025
[(None, None), ("2022-01-01T00:00:00Z", datetime(2022, 1, 1, tzinfo=UTC))],
[
(None, None),
("NOT A DATE", 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 was a test case missing from a previous PR.

We need to test that invalid dates are also handled. Coverage doesn't catch it because it's handled in the same line as the None case.

Added it here in a separate commit.

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.

1 participant