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
bumi commented 2019-05-03 23:36:54 +00:00 (Migrated from github.com)
Copy Link

So far it returned 0 because of those in range checks which both
applied because numberOfPages is 0 if we have no record.

So far it returned 0 because of those in range checks which both applied because numberOfPages is 0 if we have no record.
raucao (Migrated from github.com) reviewed 2019-05-04 08:28:21 +00:00
lib/utils/pagination.js
@ -7,3 +7,3 @@
number = number < 1 ? 1 : number;
number = number > numberOfPages ? numberOfPages : number;
number = number < 1 ? 1 : number;
raucao (Migrated from github.com) commented 2019-05-04 08:28:21 +00:00
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 (Migrated from github.com) reviewed 2019-05-04 08:38:37 +00:00
lib/utils/pagination.js
@ -7,3 +7,3 @@
number = number < 1 ? 1 : number;
number = number > numberOfPages ? numberOfPages : number;
number = number < 1 ? 1 : number;
bumi (Migrated from github.com) commented 2019-05-04 08:38:37 +00:00
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 (Migrated from github.com) reviewed 2019-05-04 08:50:57 +00:00
lib/utils/pagination.js
@ -7,3 +7,3 @@
number = number < 1 ? 1 : number;
number = number > numberOfPages ? numberOfPages : number;
number = number < 1 ? 1 : number;
raucao (Migrated from github.com) commented 2019-05-04 08:50:57 +00:00
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 (Migrated from github.com) reviewed 2019-05-04 09:05:04 +00:00
lib/utils/pagination.js
@ -7,3 +7,3 @@
number = number < 1 ? 1 : number;
number = number > numberOfPages ? numberOfPages : number;
number = number < 1 ? 1 : number;
bumi (Migrated from github.com) commented 2019-05-04 09:05:04 +00:00
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
raucao (Migrated from github.com) approved these changes 2019-05-04 09:08:05 +00:00
fsmanuel (Migrated from github.com) reviewed 2019-05-07 14:53:16 +00:00
lib/utils/pagination.js
@ -7,3 +7,3 @@
number = number < 1 ? 1 : number;
number = number > numberOfPages ? numberOfPages : number;
number = number < 1 ? 1 : number;
fsmanuel (Migrated from github.com) commented 2019-05-07 14:53:16 +00:00
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...
fsmanuel (Migrated from github.com) approved these changes 2019-05-07 14:54:11 +00:00
fsmanuel (Migrated from github.com) left a comment
Copy Link

🦅

🦅
raucao (Migrated from github.com) reviewed 2019-05-07 15:11:57 +00:00
lib/utils/pagination.js
@ -7,3 +7,3 @@
number = number < 1 ? 1 : number;
number = number > numberOfPages ? numberOfPages : number;
number = number < 1 ? 1 : number;
raucao (Migrated from github.com) commented 2019-05-07 15:11:56 +00:00
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:
Sign in to join this conversation.
Reviewers
No Reviewers
raucao
fsmanuel
Labels
Clear labels
good first issue
Good for newcomers
ipfs
rsk
scaling
bug
Something is not working
dev environment
Config, builds, CI, deployment, etc.
docs
Documentation
duplicate
This issue or pull request already exists
enhancement
Improving existing functionality
feature
New functionality
idea
Something to consider
invalid
Not a bug
kredits-1
Small contribution
kredits-2
Medium contribution
kredits-3
Large contribution
question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
ui/ux
User interface, process design, etc.
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
bkero bumi fsmanuel galfert greg haythem hueso raucao slvrbckt
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/contracts#125
No description provided.
Delete Branch "bugfix/pagination"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?