Implement Pagination for Content libraries (SOL-4)

[Kelketek]

• Background: This PR provides pagination for content Libraries in the container view, when displaying the blocks contained in the library.
• Jira tickets: Implements SOL-4
• Discussions: Architecture discussed extensively on the wiki and in meetings, then the revised proposal was presented to the Arch Council on Oct. 21 and given thumbs up.
• Dependencies: https://github.com/edx/edx-platform/pull/6046 (merged, in feature branch https://github.com/edx/edx-platform/commits/content-libraries)
• Sandbox URL: http://sandbox.opencraft.com:18010/library/library-v1:TST-X-01+now
• Internal code reviews PRs: Content libraries paging open-craft/openedx-platform#9 Implement Pagination for Content libraries open-craft/openedx-platform#18
• Partner information: 3rd party-hosted open edX instance, for an edX solutions client.

[openedx-webhooks]

Thanks for the pull request, @Kelketek! I've created OSPR-261 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• edx-code email threads
• timeline information ('this must be merged by XX date', and why that is)
• partner information ('this is a course on edx.org')
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here.

[Kelketek]

@sarina This should be ready for review now-- removed the WIP flag, but I'm not sure what to do about Jenkins' quality report, since it's flagging a couple of 'TODO' comments, both of which are not intended to be addressed in this PR but which we don't really want to forget either.

[Kelketek]

Sorry @sarina , just found out you're not the person to ping for this. :) @explorerleslie @cahrens , this is ready for your review, and a sandbox is available, linked in the description with the standard staff@example.com account.

[sarina]

@Kelketek this PR will help the TODO issue: https://github.com/edx/edx-platform/pull/6122

[cahrens]

When you add a new component to the page, it is currently scrolling to show the top of the page. It should instead scroll down to show the newly added component. Related to this, it seems like the entire page is re-rendering each time a component is added, which does not seem necessary. Adding a new component will either case you go to go the next page (if at least 10 components already exist on the page), or it will be added as the last component on the current page. It seems like you can easily avoid re-rendering in the 2nd case.

(also note that I screwed up Xavier's test library by adding a discussion component-- I am so used to just clicking anything and forgot that it isn't expected to work)

[cahrens]

Drag and drop is now disabled. Is that intentional?

[singingwolfboy]

Can you add a trailing comma here, so that if something else needs to be added to the context, this line won't have to change again?

[singingwolfboy]

@Kelketek: now that #6122 is merged, you should be able to get a passing Jenkins build if you rebase on top of the latest master. Can you do that?

[Kelketek]

@singingwolfboy Certainly! I'll get right on that.

[Kelketek]

@cahrens Yes, drag-and-drop's disabling is intentional, as I remember-- I can try to find the historical conversation on it, if you wish.

[Kelketek]

@singingwolfboy Wait, if I rebase on top of master with that, won't a merge with this feature branch be unclean? The feature branch would need a rebase first. Has it been?

[singingwolfboy]

@Kelketek no, rebasing a branch makes it more clean than merging master into it. See the How to Rebase a Pull Request page for more info.

[cahrens]

I think the feature branch should be rebased on master, then this branch rebased on the feature branch.

[Kelketek]

@singingwolfboy That's not what I mean-- This branch is not intended to be merged into master. It's intended to be merged into the content-libraries feature branch. That branch is the one I need to rebase against. If I rebase against master, then I will have diverged from the content-libraries feature branch.

[singingwolfboy]

Oh! I totally missed that, sorry. We'll need to get the content-libraries branch rebased onto master, and then you can rebase this pull request onto the newly-rebased content-libraries branch.

[Kelketek]

@singingwolfboy What's the procedure for that? Does one just create a new branch with the rebase, and someone on your end does a reset --hard against it?

[singingwolfboy]

@Kelketek yes, that's basically the procedure. I'll take care of it -- I just want to make sure that everyone within edX (who cares about this branch) is aware of the change.

[Kelketek]

@singingwolfboy Thank you. I'll let the others at OpenCraft know that the feature branch will be rebased, and to prepare accordingly, since that means they may have to discard commits that aren't theirs on their rebase against the feature branch.

[cahrens]

@Kelketek Copy-paste makes me cringe. Should library-container view extend the container view?

[e-kolpakov]

@cahrens I've just went through that file again - there're like two or three methods that can be shared. So it's not good and shame on me (since I have implemented it initially), but that's not that bad to cringe :) But of course it's useful to factor it out so that those methods are shared.

On the other hand, if library container extend ordinary container there would be more overloads than shared methods, so it might make sense to have a common ancestor, but not making library-container a descendant of ordinary container.

[cahrens]

From a functionality perspective, extension seems like the right thing here-- a library is a container page that adds additional features like paging.

[cahrens]

Can you give a high-level description of what changed in this file? I'm having trouble understanding via the diff.

[e-kolpakov]

@cahrens parameterized the whole test suite. It basically now works in two modes: with ordinary container and with library container. It tests common functionality found in both of them. The change was to wrap entire test suite into a function with some parameters and call it multiple times. As a result, every line changed indentation, and github diff viewer is unable to detect that, hence such a terrible diff.

[singingwolfboy]

if you add ?w=1 to the end of the URL, Github will ignore whitespace when calculating diffs. That might make this easier to read.

[Kelketek]

@cahrens This is ready for another reviewing pass. Jenkins is still barfing on the quality checks even post-rebase, and it is now freaking out on bok_choy, heaven knows why, but I can't reproduce that locally either.

[cahrens]

OK. I don't think I will be able to get back to this until Monday or Tuesday.

[antoviaque]

Updated the sandbox with the latest code: http://sandbox.opencraft.com:18010/

[cahrens]

I still think it would be good to make the last component unique in some way, and verify that the one displayed on the new page is the 11th one you added.

Actually, I think this is covered below, so I don't feel as strongly about it.

[cahrens]

👍 Just a few nits, but otherwise good.

[Kelketek]

@antoviaque Jenkins has finished. The only failure is that unreproducible quality check issue as before.

[antoviaque]

@Kelketek Great, thank you! Merging then. Congratulations on getting this to completion, and thanks again @cahrens for the extensive review.