learningequality / ka-lite

KA Lite: lightweight web server for serving core Khan Academy content (videos and exercises) without needing internet connectivity
https://learningequality.org/ka-lite/
Other
458 stars 305 forks source link

Set max_limit to 0 in order to fetch all users. #5527

Closed mrpau-eugene closed 6 years ago

mrpau-eugene commented 6 years ago

Summary

Reference: https://stackoverflow.com/a/18477897

FYI @benjaoming @Aypak

It seems that tastypie automatically limits the API request to 1000 and I have no idea why do they do that.

I was not able to replicate the issue when there are 1,000 users registered in a facility. But I think this fixes the issue since it is already returning the number of expected users

{"meta": {"limit": 0, "offset": 0, "total_count": 1011}

TODO

If not all TODOs are marked, this PR is considered WIP (work in progress)

Issues addressed

5523

codecov[bot] commented 6 years ago

Codecov Report

Merging #5527 into 0.17.x will increase coverage by <.01%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.17.x    #5527      +/-   ##
==========================================
+ Coverage   62.77%   62.78%   +<.01%     
==========================================
  Files         117      117              
  Lines        6555     6556       +1     
==========================================
+ Hits         4115     4116       +1     
  Misses       2440     2440
Impacted Files Coverage Δ
kalite/facility/api_resources.py 95.23% <100%> (+0.03%) :arrow_up:

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 f862026...4908f62. Read the comment docs.

benjaoming commented 6 years ago

Ping @Aypak

Great work, it does seem very likely to fix the issue.

But okay, so it's very much a hotfix, because it means that JS is loading 1000+ usernames in from an API request in order to process a simplified login, right? So the behavior us very bad in that sense... but IMO it's fine that at least we fix the issue in question and move on :)

benjaoming commented 6 years ago

Could you add a release note about it? We can include it in the upcoming 0.17.4 patch release.

rtibbles commented 6 years ago

Oh tastypie...

Aypak commented 6 years ago

Thanks a million!