Skip to content
This repository was archived by the owner on Sep 5, 2023. It is now read-only.

Fetch all lineitems using the pagination #328

Merged
merged 5 commits into from
Sep 17, 2020

Conversation

netoisc
Copy link
Contributor

@netoisc netoisc commented Sep 12, 2020

When the code fetch the course lineitems the response sometimes contains a header named Link that indicate us the next url to use for fetching more items. This change fetchs them recursively.

@netoisc netoisc added fix Fixes a bug lti13 LTI 1.3 authenticator and handlers labels Sep 12, 2020
@netoisc netoisc requested a review from jgwerner September 12, 2020 00:54
@netoisc netoisc self-assigned this Sep 12, 2020
@codecov
Copy link

codecov bot commented Sep 12, 2020

Codecov Report

Merging #328 into main will decrease coverage by 0.44%.
The diff coverage is 60.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #328      +/-   ##
==========================================
- Coverage   78.82%   78.38%   -0.45%     
==========================================
  Files          21       21              
  Lines        1360     1383      +23     
==========================================
+ Hits         1072     1084      +12     
- Misses        288      299      +11     
Impacted Files Coverage Δ
src/illumidesk/grades/senders.py 67.87% <60.71%> (-2.55%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b3230e...cf25a54. Read the comment docs.

@jgwerner
Copy link
Member

@netoisc let's add tests, even if basic, before merging.

Copy link
Member

@jgwerner jgwerner left a comment

Choose a reason for hiding this comment

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

Add tests for _find_next_url and _get_lineitems_from_url.

… link header is considerate to make another request
@jgwerner jgwerner self-requested a review September 17, 2020 02:36
Copy link
Member

@jgwerner jgwerner left a comment

Choose a reason for hiding this comment

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

LGTM

@jgwerner jgwerner merged commit 6626af4 into IllumiDesk:main Sep 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fix Fixes a bug lti13 LTI 1.3 authenticator and handlers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants