-
Notifications
You must be signed in to change notification settings - Fork 0
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
Handle courses with missing start_at date #33
Conversation
This happens sometimes for CS88. * Try to request the date of the *term* if present * If no dates, fallback to created_at which should always be present
Coverage for unit tests for Python 3.11
|
Coverage for unit tests for Python 3.12
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33 +/- ##
==========================================
- Coverage 39.18% 39.04% -0.14%
==========================================
Files 27 27
Lines 1692 1703 +11
==========================================
+ Hits 663 665 +2
- Misses 1029 1038 +9 ☔ View full report in Codecov by Sentry. |
server/services/canvas/__init__.py
Outdated
start_at_date = course['start_at_date'] or course['created_at_date'] | ||
start_at = course['start_at'] or course['created_at'] | ||
course['start_at_date'] = start_at_date | ||
course['start_at'] = start_at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait shouldn't we use dot notation
course.start_at
instead course['start_at']
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh sure...I wasn't sure, I thought these were dicts, but yeah maybe we should... I assume a.b.c is valid syntax then? I don't remember all the rules of Python or if we need to check for presence before an error is realized..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries I will take it from here.
|
||
|
||
def get_user_courses_categorized(user: FakeUser | User) \ | ||
-> tuple[list[FakeCourse | Course], list[FakeCourse | Course], list[FakeCourse | Course]]: | ||
courses_raw = user.get_courses(enrollment_status='active') | ||
courses_raw = user.get_courses(enrollment_status='active', include=['term'], per_page=100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would need to change fake canvas client to support include
keyword
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh does it not? I thought it did.. :(
https://canvasapi.readthedocs.io/en/stable/keyword-args.html?highlight=include#list-parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real one does.
Our fake one does not.
I will do it.
Coverage for unit tests for Python 3.10
|
Coverage for unit tests for Python 3.11
|
Coverage for unit tests for Python 3.12
|
Coverage for e2e tests for Python 3.11
|
Coverage for e2e tests for Python 3.10
|
Coverage for e2e tests for Python 3.12
|
Coverage for unit tests for Python 3.11
|
Coverage for unit tests for Python 3.10
|
Coverage for unit tests for Python 3.12
|
Coverage for e2e tests for Python 3.11
|
Coverage for e2e tests for Python 3.10
|
Coverage for e2e tests for Python 3.12
|
Coverage for unit tests for Python 3.11
|
Coverage for unit tests for Python 3.10
|
Coverage for unit tests for Python 3.12
|
Coverage for e2e tests for Python 3.10
|
Coverage for e2e tests for Python 3.12
|
Coverage for e2e tests for Python 3.11
|
This happens sometimes for CS88.