state object information is inconsistent when adding/removing a model

[alanrubin]

Hi @wyuenho ,

First thanks for this great library. I'm trying to replace Backbone-paginator for a more stable library and found yours really good (and tested!).

I think I found a bug when adding/removing a model in the "client" mode. The status object is not consistent regarding the totalRecords and totalPages, and is not adjusted accordingly.

Additionally, when removing the last record of a page, the state continues to indicate that lastPage/currentPage is the empty page - I think behaviour should be that the empty page is automatically removed and the paginated collection points now to the lastPage-1 as the currentPage. The only exception should be when removing all the models from the collection - I think in this case I think totalPages and currentPage should be 1 (never 0 so not to break behaviour of the library).

I have committed to my fork tests for those issues (branch change_col_bug - alanrubin@f44b5fd). What do you think ?

I may have some time tomorrow to try to generate a fix for it.

Thanks,
Alan

[wyuenho]

Hi! Thanks for the suggestion. I do realize that I didn't update the state when adding and removing models, that is mainly because I just don't know what behavior is desirable under what circumstances. Perhaps you can help me by laying out a couple concrete use cases and what the behavior you think should be?

Specifically, I'd like to see how you sync with the server before and/or after an add and/or remove operation.

[alanrubin]

Thanks for the reply. Let me clarify that I'm working only on client mode, so no experience when server mode behavior. After rethinking the behaviour, I think it should be behave in client mode:

Add use case

• If element is added to page collection. Add it to the collection at the same position it was added in the page. Update the state accordingly (specially totalRecords and totalPages, as a new page may have been created), but leave the page collection with n (pageSize) + 1 (the new model) until next pagination occurs - when that happens we will have again n (pageSize) elements in each page.
• If element is added to the full collection. Update the state accordingly as above. In any case (new model is added to the current page or not), reprocess the pagination so that currentPage will always have pageSize elements.
• If new element is added to the full collection and it creates a new page, we should automatically navigate to currentPage+1.

Remove use case

• Same logic as above. If element is removed from page collection, update the state and leave page with n(pageSize) - 1 (the removed model) until next pagination occurs. If element is removed from full collection, update the state and reprocess pagination.
• When removing element from full collection, in case current page does not exist anymore (removed the last single element of the last page), we automatically navigate to currentPage - 1, so state remains valid.
• In case totalRecords is 0 (all elements were deleted), we still remain with one page, so state must be totalRecords = 0, totalPages = 1, currentPage = 1.

Those changes imply that some of the checks being done at _checkState are not valid anymore. What do you think ? Did I forget any use case ?

Regarding the server mode, I guess you can follow the same logic. If it is a page collection add/remove operation, leave it unchanged (I mean remove or add the element but do not fetch updated page from server) and only fetch new data when navigating to other pages. And leave the remove/add call to server to happen only if the developer calls .save() in the page collection.

Regarding the full collection, I guess it is not relevant in server mode, Am I wrong ?

[wyuenho]

1. How many additional and removal do you expect to do on the client side? Would it be easier to do them in server mode and refetch the current page after each add/remove? You'll get the most updated state from the server this way if your server can supply it.
2. Automatic repagination is already taken cared of by the event handlers. But you are right, the state should be updated after addition and removal.
3. Setting the current page to the page the new model was added to doesn't make sense. The current page is the page you are currently viewing. If you want to see the newly added element on the current page then you should add it to the current page.
4. Not sure about setting totalPages to 1 when there's absolutely no records, the same reason there's only 1 page if there are only <= pageSize many records. If you need to display a blank row for an empty PageableCollection, you should check for this case yourself in your view code.
5. Yes fullCollection doesn't exist under server mode.

I'll make a commit later for this issue. Thanks!

[alanrubin]

Here are my comments:

(1) I don't think I have an answer for that, my user can do as many as he wants. I don't support right now paging in the server and I don't have a need for that (at least now). Another reason is that my client collections sometimes reach 50 elements, so I don't think it is a good idea to fetch the whole collection again.

(2) Yes, I saw it is implemented in _onFullCollectionEvent and _onPageableCollectionEvent. Looks good. Agreed on state.

(3) Not sure if that was clear, but that is done only in case we added the element to the full collection and it creates a new page. I added it so that it will behave similar to remove. But thinking again you are right. If currentPage is valid, we shouldn't change it. That must be the role and won't create some unexpected behaviour.

(4) Yes, you are right, in case there are no record, my view should check and not render the pagination tab (render a empty message instead). In this case, it doesn't matter what is the totalPages.

Thanks for preparing a commit. I will check it when it is ready. Let me know if you need any help.

[wyuenho]

I've decided to set totalPages and totalRecords to null when empty to be consistent with the default state, and to prevent fetch from sending out those params when they are 0. Try it out and let me know if it works for you.

[alanrubin]

Looks sensible. But I have checked the new version and it is failing one of my tests - When removing the last element from the page collection, I get the exception RangeError: currentPage must be firstPage <= currentPage <= totalPages if 1-based. at the collection .remove method call (_checkState line 527). We agreed that in this case, the currentPage will be currentPage-1.

Another issue is that it seems to me that the condition (pageSize < 1 || pageSize > totalRecords) (_checkState line 510) will fail when the number of total records is less then the pageSize - scenario which I think is ok, as you can have a collection of 2 elements with a pageSize of 4.

Can you please check ?

[wyuenho]

I didn't end up using your tests for remove. When you remove the last element from the collection, _checkState is essentially a no-op as totalRecords is set to null. This is done so the state of removing the last element will be the same as the state as a new collection instance with no elements.

See https://github.com/wyuenho/backbone-pageable/blob/master/test/client-pageable.js#L202.

[alanrubin]

No problem about the tests. The test I mentioned was a test I created for my application and not the one at fork.

Regarding the first issue, my scenario is a full collection with 9 totalRecords, and pageSize as 4. I call .getPage(3) at the backbone-pageable collection, going to page 3 (one element). I call .remove in the backbone-pageable collection (not the full collection) for the only element - I get the exception described. My expectation was that I would not get an exception, and backbone-pageable collection will now be empty and currentPage still 3.

[wyuenho]

@alanrubin Give 0.9.13 a whirl and see if it behaves saner for you.

[alanrubin]

That did the job. Thanks !

[wyuenho]

Hey @alanrubin I'm preparing to release 1.0. Is there anything you'd like me to change/add before I release it?

[alanrubin]

Hi, I think I have detected another bug: When reseting the fullCollection with new models, the "state" (most specifically the totalRecords) are updated after the pageableCollection is reseted (backbone-pageable line 475-477).

In my case, the scenario is the following: If I have a pagination Backbone.View (renders the pagination controls for navigating - forward, back, pages number) listening to the "reset" event in the pageableCollection, it will be rendered with the previous state when a reset is done. That could be true for the event "remove" as well, but I still have to check that.

My fix was:

if (event == "reset" || event == "sort") {
        options = collection;
        state.totalRecords = this.models.length;
        pageableCollection.state = pageableCollection._checkState(state);
        resetQuickly(pageableCollection, this.models.slice(pageStart, pageEnd),
                     options);
}

What do you think ?

[alanrubin]

Another thing: In case you are interested, I have extended the pageable collection with methods similar to backbone.paginator, so I can use it straightforward with code already written for it. It includes filtering, sorting and some small behaviour changes to the library. Check it out, could give you some inspiration :) https://gist.github.com/4548113

[wyuenho]

@alanrubin can you open a new bug with a failing test case?

[alanrubin]

I will open a new bug... But it will take some time to write the test - sorry no qunit experience, only jasmine. Will try to write the test tomorrow.

[wyuenho]

doesn't have to be a real qunit test case, just a code snippet with some comment on your expectations will do