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.

Sign up for GitHub

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

Jump to bottom

term lookup causing error #40

Open
lsloan opened this issue Dec 19, 2023 · 2 comments
Open

term lookup causing error #40

lsloan opened this issue Dec 19, 2023 · 2 comments
Assignees
@lsloan
Labels
🪰 bug Something isn't working

Comments

@lsloan
Copy link
Member

lsloan commented Dec 19, 2023 •
edited

Term lookup API call is not reading all of the pages from the API. It reads only the first page and stops. If the later usage of the data from the call doesn't include the term number from the app parameter, the lookup fails.
@lsloan lsloan added the 🪰 bug Something isn't working label Dec 19, 2023
@lsloan lsloan self-assigned this Dec 19, 2023
lsloan added a commit to lsloan/tool-migration that referenced this issue Dec 19, 2023
@lsloan
tl-its-umich-edu#40 - disable term display; possible API bug
6cd2318
lsloan added a commit that referenced this issue Dec 19, 2023
@lsloan
Merge pull request #41 from lsloan/40-term-lookup
d1d2716 
disable term display temporarily (iss. #40)
@lsloan
Copy link
Member Author

lsloan commented Dec 19, 2023

This was temporarily disabled in #41.

It needs to be corrected, preferably by resolving #43 and reenabling this feature.

lsloan added a commit to lsloan/tool-migration that referenced this issue Dec 20, 2023
@lsloan
tl-its-umich-edu#40 - proposed safe usage of term data
8980fce 
But the API call that provides the data needs to be corrected first.  See tl-its-umich-edu#43.
lsloan added a commit to lsloan/tool-migration that referenced this issue Dec 20, 2023
@lsloan
tl-its-umich-edu#40 - proposed partial fix for getting term IDs
45c3325 
This would probably work without changing to the `canvasapi` module, but we're likely to run into some other problem in the future if we don't change.
@ssciolla
Copy link
Contributor

ssciolla commented Jan 5, 2024

Hi @lsloan, this is certainly a bug and something I overlooked before. However, I'd caution you to not jump to #43 too quickly, since I'm not sure if canvasapi supports async/await.

It would be easy enough to modify AccountManager.get_term_names to use API.get_results_from_pages instead of API.get. This is already being done when fetching courses here:

tool-migration/migration/manager.py

Lines 73 to 78 in d1d2716

term_results = await self.api.get_results_from_pages(
f'/accounts/{self.account_id}/courses',
params={'enrollment_term_id': term_id},
page_size=50,
limit=limit_for_term
)

Wishing you the best in 2024.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lsloan lsloan

Labels
🪰 bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@lsloan @ssciolla