-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CV2-3776: implement a custom pagination for TiplineMessageType #1697
CV2-3776: implement a custom pagination for TiplineMessageType #1697
Conversation
items.where('id > ?', start_idx) | ||
else | ||
items | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end
at 20, 6 is not aligned with if
at 8, 21.
index_from_cursor(after) > items.map(&:id).min | ||
else | ||
false | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end
at 30, 6 is not aligned with if
at 22, 27.
index_from_cursor(before) < items.map(&:id).max | ||
else | ||
false | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end
at 40, 6 is not aligned with if
at 32, 23.
def load_nodes | ||
@nodes ||= begin | ||
sliced_nodes = if before && after | ||
end_idx = index_from_cursor(before) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use 2 (not -13) spaces for indentation.
|
||
@has_previous_page = if last | ||
# There are items preceding the ones in this result | ||
sliced_nodes.count > last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use 2 (not -19) spaces for indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very cool approach, Sawy! Can you please add a test for it (or update the previous one)?
Already I did, updated the existing one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reviewed this in a call with Sawy - good to go once tests pass and Sawy makes the change we discussed about sorting.
Code Climate has analyzed commit 37882af and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (100% is the threshold). This pull request will bring the total coverage in the repository to 100.0% (0.0% change). View more on Code Climate. |
Description
Currently to get the cursor we should retrieve all messages and the cursor is the index of the item not item ID so I implement a custom pagination to override current behavior for cursor to use item id instead of index of the item so I can retrieve a cursor for a specific message
so I did the following:
cursor_for
method in a custom classload_nodes
method in a custom classReferences: CV2-3776
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can verify the changes. Please describe whether or not you implemented automated tests.
Things to pay attention to during code review
Please describe parts of the change that require extra attention during code review, for example:
Checklist