Skip to content
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

lowered pagination threshold in dev and staging using kaminari #4865

Conversation

k8port
Copy link
Contributor

@k8port k8port commented Dec 17, 2024

Resolves #4859 lower threshold on pagination in dev and staging

Description

This change sets the pagination threshold to 5 in dev and staging environments while leaving it at 50 otherwise. This is to ease manual testing by having fewer objects per page.

Guide questions:

  • What motivated this change (if not already described in an issue) … to ease manual testing
  • What alternative solutions did you consider? see below
  • What are the tradeoffs for your solution? … change only lowers threshold for the kaminari pagination library, meaning that controllers that do not already implement kaminari are not affected. (see description below)

List any dependencies that are required for this change. (gems, 🤩 libraries, etc.). kaminari

Include anything else we should know about. -->
This change only affects kaminari configuration. Controllers not already implementing kaminari pagination are not affected. Pagination configuration in kaminari_config.rb, for example, is not being applied to the Base Items index view.

Here is the relevant code (app/controllers/admin/base_items_controller.rb, line 21):

def index
  @base_items = BaseItem.alphabetized.all
end

app/views/admin/base_items/index.html.erb view is not using pagination at all - just rendering all items directly.
Base Items listing does not use Kaminari's pagination. To fix, the controller and view would need to implement kaminarri pagination (base_items_controller.rb):

(# [Super Admin] Manage the BaseItems -- this is the only place in the app where Base Items can be
# added / modified. Base Items are both the template and common thread for regular Items
#
# See #4656, BaseItems are pending significant changes/possible deletion
class Admin::BaseItemsController < AdminController
  def edit
    @base_item = BaseItem.find(params[:id])
  end

  def update
    @base_item = BaseItem.find(params[:id])
    if @base_item.update(base_item_params)
      redirect_to admin_base_items_path, notice: "Updated base item!"
    else
      flash[:error] = "Failed to update this base item."
      render :edit
    end
  end

  def index
    @base_items = BaseItem.alphabetized.page(params[:page])
  end
end

a index

Now the Base Items listing should show only 5 items per page in development and staging environments, with pagination controls to navigate between pages. The changes I made:
Modified the controller to use Kaminari's page method
Added the paginate helper to the view to show pagination controls
These changes will make the Base Items listing respect the pagination settings we configured in kaminari_config.rb. You should now see:
5 items per page in development/staging
50 items per page in production
Pagination controls at the bottom of the list to navigate between pages
The reason this wasn't working before is that the Base Items listing wasn't using Kaminari's pagination features at all - it was just showing all items at once with BaseItem.alphabetized.all. Now it's properly integrated with the pagination system.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Unit test to verify pagination according to env.

To run unit test:
bundle exec rspec spec/lib/kaminari_config_spec.rb

Do we need to do anything else to verify your changes?
No

@cielf
Copy link
Collaborator

cielf commented Dec 17, 2024

Thanks for the call out on BaseItem! It turns out that we aren't too worried about that because we've only had to add a new BaseItem twice in the last 3 years -- It's super super stable -- and only seen by the superusers.

@cielf cielf requested a review from dorner December 17, 2024 21:54
Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k8port linting is failing now - you can run it locally with bin/lint.

@k8port
Copy link
Contributor Author

k8port commented Dec 18, 2024

@dorner bin/lint run locally produced the following output:

Running erb_lint
warning: parser/current is loading parser/ruby32, which recognizes 3.2.6-compliant syntax, but you are running 3.2.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Linting and autocorrecting 426 files with 14 linters (11 autocorrectable)…

After there are no changes to commit.

The workflow is giving a 502 error - could it be an issue with github server? I do not see an option to rerun job…

@coalest
Copy link
Collaborator

coalest commented Dec 18, 2024

@k8port I reran the failing lint job. Looks like Github was having an issue.

It may be a permissions issue if you don't see a button like this in the upper right corner to rerun jobs. I'm not sure though.
image

If you want a workaround, I believe you can push an empty commit and that should trigger all the Github actions again:
git commit --allow-empty -m "Trigger Build"

The commit messages get squashed on a merge to main anyways.

… github.com:k8port/human-essentials into 4859-lower-pagination-threshold-for-staging-and-dev
Copy link
Contributor Author

@k8port k8port left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bin/lint run locally:

Running erb_lint
warning: parser/current is loading parser/ruby32, which recognizes 3.2.6-compliant syntax, but you are running 3.2.2.
Please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.
Linting and autocorrecting 426 files with 14 linters (11 autocorrectable)…

pushed empty commit message to trigger new build

@@ -1,6 +1,10 @@
# frozen_string_literal: true
Kaminari.configure do |config|
config.default_per_page = 50
if Rails.env.development? || Rails.env.staging?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also think of making this configurable from env config? Does this makes sense here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true - we could move this to development.rb and staging.rb which is a bit neater. But I wouldn't block the PR for that.

@dorner dorner merged commit 05fa004 into rubyforgood:main Dec 19, 2024
11 checks passed
Copy link
Contributor

@k8port: Your PR lowered pagination threshold in dev and staging using kaminari is part of today's Human Essentials production release: 2024.12.22.
Thank you very much for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow a lower threshold on pagination for staging and development
5 participants