kredits/contracts
kredits/contracts
9
1
Fork 0
Code Issues 21 Pull Requests Actions Projects 1 Releases 8 Activity

Fix page number calculation for zero records #125

Merged
bumi merged 1 commits from bugfix/pagination into master 2019-05-07 15:12:17 +00:00
Conversation 8 Commits 1 Files Changed 1 +1 -1
1 changed files with 1 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
lib/utils/pagination.js
View File

@ -4,8 +4,8 @@ function pageNumber (number, size, recordCount) {
number = parseInt(number) || 1; number = parseInt(number) || 1;
// Ensure page number is in range // Ensure page number is in range
number = number < 1 ? 1 : number;
number = number > numberOfPages ? numberOfPages : number; number = number > numberOfPages ? numberOfPages : number;
number = number < 1 ? 1 : number;
raucao commented 2019-05-04 08:28:21 +00:00 (Migrated from github.com)
Review
Copy Link

Aside from the fix looking good, are these two lines actually a good idea (as opposed to throwing an exception)?

As far as I understand this code, if I ask for page -2, it will give me page 1 (instead of the second-last e.g.), and if I ask for page 24 out of 23 it will give me 23. But in both cases, I wouldn't know that I got a different page than I asked for, and that the pages I asked for are invalid, right?

Aside from the fix looking good, are these two lines actually a good idea (as opposed to throwing an exception)? As far as I understand this code, if I ask for page -2, it will give me page 1 (instead of the second-last e.g.), and if I ask for page 24 out of 23 it will give me 23. But in both cases, I wouldn't know that I got a different page than I asked for, and that the pages I asked for are invalid, right?
bumi commented 2019-05-04 08:38:37 +00:00 (Migrated from github.com)
Review
Copy Link

at least in the zero records case I do not want an exception.

at least in the zero records case I do not want an exception.
raucao commented 2019-05-04 08:50:57 +00:00 (Migrated from github.com)
Review
Copy Link

That much is certain. Just asking in general, because I'm not sure it's worth an issue, if I'm not making sense.

That much is certain. Just asking in general, because I'm not sure it's worth an issue, if I'm not making sense.
bumi commented 2019-05-04 09:05:04 +00:00 (Migrated from github.com)
Review
Copy Link

not sure when this would happen. - we would need then also proper exception handling on the other sides.
need to pick @fsmanuel's thoughts

not sure when this would happen. - we would need then also proper exception handling on the other sides. need to pick @fsmanuel's thoughts
fsmanuel commented 2019-05-07 14:53:16 +00:00 (Migrated from github.com)
Review
Copy Link

It is not optimal but 100% fail save. I guess it depends on how we use it (infinite scroll, normal pagination, etc.). The client somehow needs to implement some of the logic no matter what so I guess the wrapper should just return valid data. I don't like to throw an error here right now because error handling is always a pain.
Ideally we would also return some meta data (current page, page count, per page, etc) so the client can better work with it...

It is not optimal but 100% fail save. I guess it depends on how we use it (infinite scroll, normal pagination, etc.). The client somehow needs to implement some of the logic no matter what so I guess the wrapper should just return valid data. I don't like to throw an error here right now because error handling is always a pain. Ideally we would also return some meta data (current page, page count, per page, etc) so the client can better work with it...
raucao commented 2019-05-07 15:11:56 +00:00 (Migrated from github.com)
Review
Copy Link

Ideally we would also return some meta data (current page, page count, per page, etc) so the client can better work with it...

👍

> Ideally we would also return some meta data (current page, page count, per page, etc) so the client can better work with it... :+1:
return number; return number;
} }