Fix page number calculation for zero records #125

2019-05-03T23:36:54Z

bumi commented

2019-05-03 23:36:54 +00:00

(Migrated from github.com)

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

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

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

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

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

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

🦅

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

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.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kredits/contracts#125