Add pagination to Studio's Files and Uploads page

[andy-armstrong]

These changes represent the initial implementation of STUD-813 which adds pagination to the Files and Uploads page. @cahrens and @singingwolfboy, please pre-review this code since this is my first major change, and I want to be sure that I'm following best practices.

Note that I will be adding tests in my next commit, so this pull request will not be merged as is.

[singingwolfboy]

What happens if someone accidentally or maliciously passes in a non-integer string for this parameter? Your code should do a try/catch when converting to int, and if a TypeError or ValueError occurs, handle it in some way.

[cahrens]

If you want to create a helper method for this, a good place would be /edx-platform/common/djangoapps/util/string_utils.py

[cahrens]

If I type letters ("abc") in the pagination field, I see the text on top of the page number. I would expect the letters to disappear (and to stay on the same page).

[singingwolfboy]

Translation strings should not contain HTML. Use placeholders (the same way you're already using placeholders for %(start)s and %(end)s) to replace the HTML into the string after it's been localized.

[cahrens]

If I type -5 in the pagination field and press Enter, about half of the time I get a JavaScript alert dialog with message "Error: [object Object"].

[singingwolfboy]

application/json should be the default accepts header for AJAX requests. You can also pass AJAX options through the model.save() call -- it proxies through Backbone.sync to $.ajax. Backbone's source code is actually very readable, if you want to see how it works under the covers.

[cahrens]

Get rid of this from the URL (form submission). awefjaseofj?page-number=-2#

[cahrens]

Get rid of old start and max (not working anyway).

[cahrens]

This general approach looks good.

[andy-armstrong]

@frrrances, can you check out this PR when you get a chance. Also, it would be great to finalize how we are going to disable the Next/Previous buttons. Thanks!

[andy-armstrong]

@frrrances I've fixed all the bugs including the performance problem you were seeing (I think). Can you try this again. Two issues remain:

1. I tried to implement the 'is-disabled' logic but it doesn't seem to disable the links.
2. The 'no assets' variant of the page doesn't get the same box rendering as the 'no templates' does.

Can you help me look into these two. In the meantime I'm going to get back to testing.

[andy-armstrong]

@frrrances One more thing: the page input field looks odd when entering text as it still shows the current page number in gray underneath. How would you generally handle this? Could the label be hidden when the input has focus?

[frrrances]

@andy-armstrong looks like my sass changed got dropped at some point. I've added them back in and adjusted the focus to handle the transparency a bit better.

[frrrances]

Performance and everything else looks good to me now. 👍

[andy-armstrong]

@cahrens @singingwolfboy @frrrances @jzoldak This feature is ready for re-review.

BTW, the BDD is also now available if you'd like to review that too:

https://edx-wiki.atlassian.net/wiki/display/STU/BDD+specs+for+Asset+Pagination

[cahrens]

Personally, I would create a helper function (can be scoped just to this method) for the 5 lines of duplicated code. But that's a nit. Actually, if you allow URL to be an argument, you could remove another duplicated line of code.

[cahrens]

currentPage and pageSize are unused variables.

[frrrances]

@talbs Since I worked on this feature, can you review my sass/experience work?

[cahrens]

Need to update CHANGELOG.rst. Also, you'll want to squash commits at some point. I'd prefer you do it at the end so I can easily see what changes you make in response to feedback.

[andy-armstrong]

@singingwolfboy @cahrens, thanks for the detailed feedback. Hopefully I've addressed everything in this latest commit. BTW, Frances wants me to hold off pushing these until Brian returns on Friday so he can review her sass changes.

[cahrens]

gettext is not listed in requireJS imports. This code does not work if you kill the server and try paging.

[andy-armstrong]

Good catch. I guess I should have added a unit test for this method too...

[cahrens]

There is still a problem with create_sinon being called in a beforeEach section of assets_spec.coffee. I put a couple comments there, but the new comments show up linked to a previous discussion, and therefore you don't see them here at the end of the review.

[cahrens]

Still need to update the documentation in assets.py--

Update the documentation, which says that JSON is not supported.

Also, documentation around what is returned for HTML needs to be updated.

[andy-armstrong]

@cahrens Can you re-re-review :-)

[cahrens]

My comments above are on a particular commit-- sorry.

Here's a helper function declaration that works. It's not pretty-- I'm sure it could be improved by someone who understands Coffee better! I'm returning requests just in case you need the array later in a particular test.

        setup = (that, response) ->
            requests = create_sinon["requests"](that)
            that.view.setPage(0)
            create_sinon.respondWithJson(requests, response)
            return requests

        it "should render both assets", ->
            setup(this, @mockAssetsResponse)
            expect(@view.$el).toContainText("test asset 1")
            expect(@view.$el).toContainText("test asset 2")

[singingwolfboy]

@cahrens It might make more sense to just explicitly bind the value of this using Javascript's call or apply methods:

setup = (response) ->
    requests = create_sinon.requests(this)
    @view.setPage(0)
    create_sinon.respondWithJson(requests, response)
    return requests

it "should render both assets", ->
    setup.call(this, @mockAssetsResponse)
    expect(@view.$el).toContainText("test asset 1")
    expect(@view.$el).toContainText("test asset 2")

Aside from that, your Coffeescript looks good to me! :)

[cahrens]

@singingwolfboy Yes, that's better. And in that case, setup could access @mockAssetsResponse, so it wouldn't need to be passed in.

[cahrens]

👍 After removing AlertView.Error, checking with Don about getting total number of assets, and a few minor updates I suggested, if you are so inclined.

[andy-armstrong]

@dmitchell Can you review the Python changes that you helped me with to implement the count parameter.

[dmitchell]

Yep, those look like the paired programming changes we made :-) 👍

[andy-armstrong]

Unless I hear otherwise, I'll merge this request on Friday once Brian gives the FED his blessing. Thanks for your patience, everyone!

[talbs]

This looks solid - one question though: should we make this blockish/hover knock-out UI styling a Sass mixin/extend yet? If we're using it in several views now, I'd say "yes" for maintenance and easse of ripping off your good work's sake. If you'd rather do that in a follow up PR, that's fine too.

[frrrances]

Good call. I'll put that in a new PR since I know Andy and Christina want this to get in today if possible.

[talbs]

Hi, all.

My apologies for the holiday break slow-down with this work. I've taken a spin through the UI, design, and Front-end aspects of this feature. Nice work!

@frrrances, I made a few comments here and there about specifics. While they'd be good to address if you agree with them, none are major blockers to this work getting merged if its in a hurry. Also, very solid and well done work overall with the UI and styling.

Let me know if I can help with anything else in getting this fella to "Mergetown".

👍