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

Collections Pagination #554

Merged
merged 28 commits into from
Apr 24, 2018
Merged

Collections Pagination #554

merged 28 commits into from
Apr 24, 2018

Conversation

adorsk
Copy link
Contributor

@adorsk adorsk commented Apr 17, 2018

What are the relevant tickets?

Partially implements #521

What's this PR do?

  1. Adds pagination parameters to getCollections api calls
  2. Adds collectionsPagination actions/reducers.
  3. Adds withPagedCollections higher-order component.
  4. Wraps CollectionListPage with withPagedCollections
  5. Adds Paginator component to CollectionListPage.

How should this be manually tested?

  1. Create a whole lotta collections. Here's a lil' django-shell-ism for ya:
from django.contrib.auth.models import User
user = User.objects.first()
from ui.factories import CollectionFactory
for i in range(500):
    collection = CollectionFactory(
        title=f'Collection_{i}',
        owner=user
    )
    collection.save()
  1. Go to the collections page (`http://localhost:8089/collections/).
  2. Confirm that the pager shows and works as you think a pager should.
  3. Confirm that the sidenav always shows the first page of collections, no matter what page is active in the main page.

What GIF best describes this PR or how it makes you feel?

Alt Text

@codecov-io
Copy link

codecov-io commented Apr 17, 2018

Codecov Report

Merging #554 into master will increase coverage by 0.13%.
The diff coverage is 98.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #554      +/-   ##
==========================================
+ Coverage    95.9%   96.03%   +0.13%     
==========================================
  Files         151      159       +8     
  Lines        5465     5830     +365     
  Branches      220      220              
==========================================
+ Hits         5241     5599     +358     
- Misses        189      196       +7     
  Partials       35       35
Impacted Files Coverage Δ
static/js/components/Paginator.js 100% <100%> (ø)
static/js/containers/withPagedCollections_test.js 100% <100%> (ø)
static/js/reducers/collectionsPagination.js 100% <100%> (ø)
static/js/actions/index.js 100% <100%> (ø) ⬆️
static/js/actions/collectionsPagination.js 100% <100%> (ø)
static/js/containers/CollectionListPage_test.js 100% <100%> (ø) ⬆️
static/js/actions/collectionsPagination_test.js 100% <100%> (ø)
odl_video/settings.py 84.97% <100%> (ø) ⬆️
static/js/reducers/collectionsPagination_test.js 100% <100%> (ø)
static/js/lib/api_test.js 100% <100%> (ø) ⬆️
... and 13 more

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 39dca4b...92b1d4b. Read the comment docs.

@pdpinch
Copy link
Member

pdpinch commented Apr 19, 2018

@adorsk does this close #526 ?

@adorsk
Copy link
Contributor Author

adorsk commented Apr 20, 2018

@pdpinch yes, it does close #526 (and consequently, #521 as well).

@mbertrand mbertrand self-assigned this Apr 23, 2018
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Overall functionality works great! A couple of minor suggestions:

  • The default value of settings.PAGE_SIZE_COLLECTIONS could be lowered to something like 20-50 now.
  • Having << and >> controls for going to the first and last page would be useful (maybe check with Peter about whether that's worth doing in this PR or a future one, ditto for sorting controls).

.travis.yml Outdated
@@ -7,7 +7,7 @@ matrix:
- install:
- env | grep TRAVIS > .env
- env | grep CI >> .env
- docker-compose -f docker-compose-travis.yml run celery echo
- docker-compose -f docker-compose-travis.yml build web celery
Copy link
Member

Choose a reason for hiding this comment

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

Is this change necessary? run celery echo seems to be the standard command here (also used in micromasters, open-discussions, and the cookiecutter django project template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was done during debugging of a travis issue (see comment below on the -e flags, but it's not necessary. Reverted to the run celery echo form.

requirements.txt Outdated
-e git+https://github.com/mitodl/django-elastic-transcoder.git#egg=django-elastic-transcoder
-e git+https://github.com/Brown-University-Library/django-shibboleth-remoteuser.git#egg=django-shibboleth-remoteuser
-e git+https://github.com/mitodl/mit-moira.git#egg=mit-moira
-e git+https://github.com/mitodl/pycaption.git#egg=pycaption
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert these changes? For some reason the -e flag causes issues with the Salt deployment script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. This is a little problematic.

The story about why the -e flags were added is a little complicated.
Last week travis builds started to fail, due to failures in tox. Tox was failing because when it created a virtualenv it was installing the latest version of pip (pip10). Pip10 would fail unless git+... dependency specs were prefixed with -e. I do not understand yet why installing a package with the '-e' flag would cause failures on travis.

Unfortunately I couldn't find a way to pin a pip version in tox. So thus this change.

So, we have two options:

  1. Update salt deployment to use the -e flag.
  2. Update the requirements file to satisfy pip 10 in some way that is compatible with salt.

Can you point me towards the salt deployment script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, updated default page size to be 50.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure where the salt deployment script is, but @blarghmatey should be able to help with that. Ideally it would be best to go back to having the -e flags if possible since they're autogenerated with the requirements.txt file.

@adorsk
Copy link
Contributor Author

adorsk commented Apr 24, 2018

@mbertrand reverted requirements.txt to take out -e flags, should work with travis now per bug fix from pip 10.0.1 (pypa/pip#5251).

Ready for review again.

Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@adorsk adorsk merged commit 31aa66e into master Apr 24, 2018
@adorsk adorsk deleted the dorska/collections_pagination branch April 24, 2018 17:59
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.

4 participants