Add pagination to Issues page #5057
Conversation
nertc
force-pushed
the
issues_pagination_issues_page
branch
2 times, most recently
from
b340779 to
a63759c
Compare
|
|
|
No please don't add random waits! |
|
Also why on earth does this need a whole load of javascript when all we're doing is adding next/previous page buttons? IF you want to do a partial load instead of reloading the whole page then you should be using turbo not doing it yourself. |
|
I agree with Tom here, the SPA-like behaviour should be implemented using the Turbo framework, not with custom javascript. Otherwise, I'm happy to see pagination used for these pages. I would suggest just implementing the pagination first, then returning to the concept of using turbo drive separately, and ideally for all the paginated pages at the same time. Using Turbo will also avoid having to define an extra |
|
Note that we already have #4646 to add turbo to everything that uses the server side pagination helpers. |
|
and #4565 for changeset element pages |
|
Thank you for review, I'll update this PR accordingly to use Turbo instead of the JS code. |
nertc
force-pushed
the
issues_pagination_issues_page
branch
from
a63759c to
cb5dffd
Compare
|
I updated this PR to handle navigation using Turbo. It depends on Turbo Frames functionality. On my local machine all calls (first load and navigation) now need ~1.5s instead of ~8s. I tried to decrease loading time of navigation (like it was with the last JS solution ~200ms), but both, using Turbo Streams and custom partial rendering endpoint (like it was in the last solution), required extra JS code for good navigation history, so I sticked to the current simpler solution. Current solution solves @gravitystorm 's comment:
Thank you for linking your PRs, they were good examples. |
nertc
force-pushed
the
issues_pagination_issues_page
branch
from
cb5dffd to
3814fe2
Compare
|
@AntonKhorev suggested a perfect solution in the comment of the PR #4646. I updated code according to the suggestion and without any sacrifice of performance, extra JS code or history management, pagination time was again decreased to ~150ms (previous solution needed ~1.5s). |
|
Right but ideally we don't want to be doing this to each user of pagination separately, we want to do it across all users which is what my PR is all about. I will be reviewing @AntonKhorev's suggestion in the context of that PR when I get time. |
app/views/issues/_page.html.erb
Outdated
| <turbo-frame id="pagination" data-turbo-action="advance"> | ||
| <% if @issues.length == 0 %> | ||
| <p><%= t(@user_not_found ? ".user_not_found" : ".issues_not_found") %></p> | ||
| <% else %> |
There was a problem hiding this comment.
I think this first branch should be outside the pagination frame because it's not actually paginated - only the else clause should be inside the frame as that is the part that can be navigated to different pages.
app/views/issues/_page.html.erb
Outdated
| @@ -0,0 +1,40 @@ | |||
| <turbo-frame id="pagination" data-turbo-action="advance"> | |||
There was a problem hiding this comment.
This shouldn't have data-turbo-action but should have target="_top" and the advance action (and pagination as a target) should be on the next/previous buttons (which #4646 takes care of) so that other links inside the pagination section will break out to a full page load.
There was a problem hiding this comment.
I agree with you. I wrote it this way for it to be both functional and not to conflict with #4646 after merging, but I'll change it back.
app/controllers/issues_controller.rb
Outdated
|
|
||
| @issues, @newer_issues_id, @older_issues_id = get_page_items(@issues, :limit => @params[:limit]) | ||
|
|
||
| render :partial => "page" if turbo_frame_request_id == "pagination" || turbo_frame_request_id == "search" |
There was a problem hiding this comment.
I think the second part of this condition is wrong (in fact it's not clear to be the search frame is needed at all) and instead the search form should have a data-turbo-frame="pagination" attribute to target loading of the pagination frame.
There was a problem hiding this comment.
It was written to update only the list part of the page when the search button was clicked. But it may be out of scope of this PR (as it introduces new action other than pagination). I'll remove and it may be a separate PR when we will finish pagination functionality.
There was a problem hiding this comment.
Right, but you can do that by telling the search form to target the pagination frame, which is all that is actually being updated given that is what you render.
If you thought the search form needed to be updated as well then the above should be rendering a larger section for search but I don't think that does need to be updated does, only the results (which are in the pagination frame) need to be updated?
app/views/issues/index.html.erb
Outdated
| :include_blank => t(".select_status"), | ||
| :data => { :behavior => "category_dropdown" }, | ||
| :class => "form-select" %> | ||
| <turbo-frame id="search"> |
There was a problem hiding this comment.
Do we actually need this frame? When does it get updated?
nertc
force-pushed
the
issues_pagination_issues_page
branch
from
3814fe2 to
53bcd58
Compare
|
@tomhughes Thank you for review. I updated PR according to your comments. |
nertc
force-pushed
the
issues_pagination_issues_page
branch
from
53bcd58 to
4ae4c60
Compare
|
As #4646 is merged now, can you rebase this on current master? |
nertc
force-pushed
the
issues_pagination_issues_page
branch
from
4ae4c60 to
5716817
Compare
|
@tomhughes Sorry for the late reply. I rebased this branch. Now it has no conflicts with #4646. |
test/system/issues_test.rb
Outdated
| click_on I18n.t("issues.page.older_issues") | ||
| assert_no_content I18n.t("issues.index.user_not_found") | ||
| assert_no_content I18n.t("issues.index.issues_not_found") | ||
| assert_css "tr", :count => 31, :wait => 1.5 |
There was a problem hiding this comment.
I assume that :wait is a timeout? Why do we need that - what happens if we don't have it? Is there a default timeout that is too short or does it wait forever?
There was a problem hiding this comment.
When I run this test on my local machine, it succeeds even without :wait. But when I was pushing it to the GitHub, workflow tests were failing, because they were not waiting for paginated pages to be rendered. That's why I added :wait. Maybe there is some difference in the default waiting time configuration. I checked default_max_wait_time on my local machine and it was 2 (it's even more than the hardcoded 1.5).
There was a problem hiding this comment.
That doesn't really make any sense - a shorter timeout shouldn't fix it. I suspect it was actually just chance that you set a timeout and then that run happened to work within the timeout.
All of which is why I hat tests which rely on timeouts, because they're not deterministic and can fail arbitrarily on different machines or even on the same machine under different conditions.
It seems it's unavoidable with browser based testing like this though, so I suspect we should just increase the default to 5 or 10 or something.
There was a problem hiding this comment.
I agree with you, it seems strange. I thought, maybe default time of my local machine was different with the one of the versions of GitHub workflow tests. I'll remove it and push without :wait-s. Let's see if it still fails.
There was a problem hiding this comment.
All tests have passed. It's strange because in the first version all the tests were failing. Maybe changing a solution helped timing.
nertc
force-pushed
the
issues_pagination_issues_page
branch
from
5716817 to
3cf5667
Compare
This PR addresses heavy calls and long load time of the Issues page. Problem was mentioned in #4990 by @AntonKhorev.
Added pagination functionality. When pagination buttons will be clicked, next or previous 50 issues will be called and displayed without a page refresh (Turbo will make a call and render returned Turbo Frame). In terms of performance, on the local environment I have 1000 users and 1000 issues. Initial loading time was decreased from ~8s to ~1s and after that every pagination needs ~150ms. In the real environment, it will have much better improvement, as instead of searching for 30000 issues, it will take only 50 (in theory, the difference should be 30x better in the real environment, then it is on my local machine). Because of the pagination functionality, now issues are sorted according to IDs (it should be the same sort as creation time sort).
To address @AntonKhorev 's comment about browser built-in search, user now can change
?limit=50query parameter in the url to any number. Therefore, if any user wants to use browser built-in search, he can just change it, for example, to?limit=10000and 10000 issues will be rendered.To address @nertc 's comment about clicking
go backbutton of the browser, now every page is added to the browser history. Therefore,go backbutton will work fine, if user enters any report and then decides to go back to the list.Video:
OpenStreetMap.and.47.more.pages.-.Work.-.Microsoft.Edge.2024-08-11.14-25-49.mp4
Issues not found:
User not found: