Closed pengyin-shan closed 1 year ago
@pengyin-shan the main change was done to sort broken REST V0 pagination. Here is the problem cited:
Diff in files in commit:https://github.com/DMPRoadmap/roadmap/pull/3105/files
So two of the three files changed were api v0 files: app/controllers/api/v0/base_controller.rb app/controllers/api/v0/plans_controller.rb
It looks like the other file changed app/controllers/concerns/paginable.rb since release 2.2.1. Looks like it was re-written. Here is a diff with current branch,
diff_concerns_paginable_4_to_2.2.1.txt
I guessing because of these substantial differences your code broke when this change to scope was made
Here is relevant diff ('<' is for current 4.* development branch and '>' is for release 2.2.1)
Let me know if you don't think this might be the cause.
I see. @johnpinto1 please pardon me if I'm wrong...the app/controllers/concerns/paginable.rb
is shared by all paginate parts right? So pagniation pages, such as org_admin/plans
or org_admin/templates
are also affected by this change. Do the DMPOnline's org admin pages have the correct pagination after this change?
@briri can you also verify if DMPTool has the correct pagination after applying PR3105? I'd like to check if I need to update DMP Assisant separately for this issue. Currently, I see pagination problems in both the 3.1.0 version (DMP Assisant fork) and 4.0.1 (development branch, roadmap).
@pengyin-shan Yes it affects all pagination.
Following your last comment, I just noticed "View all" appears for templates, but is missing for plans. @briri I think this might be a bug we missed, unless this was deliberate.
Templates
Plans
@johnpinto1 I believe 'View All' was omitted (for Super Admins) from the Plans page due to the time it takes to render the page
@pengyin-shan I would try adding some debug statements around this line that calls the Kaminari gem's paginate
method. That will at least tell you if it's passing in the correct parameters.
@johnpinto1 @briri : thank you for the update! Just curious what's the config.x.results_per_page
value on your side? For the DMP Assistant, I set it to 10 (i.e. 10 result maximum) per page, then I found the setting doesn't apply to pages (for example, /org_admin/templates has more than 10 results in one page), thus it reflects the pagination doesn't work.
@pengyin-shan @briri Looks like the above change in app/controllers/concerns/paginable.rb requires config.x.application.api_max_page_size
and is not using config.x.results_per_page
We are using
config/initializers/_dmproadmap.rb 76: config.x.application.api_max_page_size = 100
My fault and it is confusing. We use paginable for API and UI.
@pengyin-shan we have (but it does not seem to be used)
[dmp@dmp-live initializers]$ grep results_per_pag * _dmproadmap.rb: config.x.results_per_page = 10
and _dmproadmap.rb: config.x.application.api_max_page_size = 100
@pengyin-shan we have (but it does not seem to be used)
[dmp@dmp-live initializers]$ grep results_per_pag * _dmproadmap.rb: config.x.results_per_page = 10
and _dmproadmap.rb: config.x.application.api_max_page_size = 100
Ah thanks for this info! so based on what I observed from DMP Assistant so far, config.x.results_per_page = 10
defines the pagination size on most of the front-end pages, such as the home page (plan dashboard), org_admin pages (such as /org_admin/templates
.
Currently, the changes in PR 3105 seem to fix API0 pagination, but meanwhile, it deactivates the results_per_page
setting, so the front-end pages lose the pagination effect.
For now, I'm going to reverse PR 3105 for DMP Assistant only, since we only want users to see 10 results per time for convenience. @johnpinto1 @briri we can discuss later if we want a universal solution to make both API and front-end pagination work.
Hmm. Pagination works for my API and the UI. I have api_max_page_size = 100
and results_per_page = 10
.
Looking at a diff between my paginable.rb and the one here I see a difference on this line. It references api_max_page_size
and my copy has results_per_page
. It should probably be the later and the api_max_page_size
is only used in v0. v1 is hardcoded :/
I'll submit a PR to fix this in the latest version
Hmm. Pagination works for my API and the UI. I have
api_max_page_size = 100
andresults_per_page = 10
.Looking at a diff between my paginable.rb and the one here I see a difference on this line. It references
api_max_page_size
and my copy hasresults_per_page
. It should probably be the later.
Yes, I also use results_per_page
after reversing the PR 3105 back.
See if #3279 helps
I believe this problem has been fixed, so I'm going to close this ticket. We can re-open this one if the pagination problem come back
What version of the DMPRoadmap code are you running? (e.g. v2.2.0) v3.1.0 and v4.0.1 (current development branch)
Expected behaviour:![Screenshot 2023-01-10 at 4 48 30 PM](https://user-images.githubusercontent.com/92752107/211669595-778a6636-be75-4888-94c9-104a70608ebd.png)
Actual behaviour:![Screenshot 2023-01-10 at 4 49 04 PM](https://user-images.githubusercontent.com/92752107/211670095-cac2f9e6-d119-4835-922a-70ae18be8f99.png)
No pagination anymore (1,2...are gone). Show all items in the
/org_admin/templates
pageSteps to reproduce: Navigate to
/org_admin/templates
.A reverse of changes in: https://github.com/DMPRoadmap/roadmap/pull/3105 fixes the problem. Thus, I'd like to confirm if this PR represents a universal change? Or there's supposed to be other changes with this PR to make pagination work correctly?
@johnpinto1 I saw you submitted the PR, do you have any context of this change?