Add Pagination for People

[yez]

Why

Closes #528

Adds pagination to the peoples_controller#index route.

Pre-Merge Checklist

• All new features have been described in the pull request
• Security & accessibility have been considered
• All outstanding questions and concerns have been resolved
• Any next steps that seem like good ideas have been created as issues for future discussion & implementation
• High quality tests have been added, or an explanation has been given why the features cannot be tested
• New features have been documented, and the code is understandable and well commented
• Entry added to CHANGELOG.md if appropriate

What

Using the pagy gem,
add pagination to the peoples_controller#index route.

How

Two modules have been included:

1. Pagy::Backend in ApplicationController
2. Pagy::FrontEnd in ApplicationHelper

The navigation for People has been added to the index view and is
present only when there are enough People to paginate. pagy takes
care of all query parameters around pagination.

Testing

For testing, I introduced a new folder for controller testing and added
the rails-controller-testing gem to allow expectations via assigns.

I was not sure of any front-end specific testing for this feature but am
happy to create one if we need it.

Next Steps

N/A

Outstanding Questions, Concerns and Other Notes

Not any at the moment.

Accessibility

It should speed up the page and help with lower bandwith devices

Security

N/A

Meta

N/A

[solebared]

Awesome @yez , thank's for working on this! 🚀
Looks like it was pretty straightforward to get pagy working, which is a good sign!
I commented about a couple of design questions, but nothing major.
LMK your thoughts.

[solebared]

I try not to include much in ApplicationController (or ApplicationHelper). It's so easy for these base classes to quickly become 'kitchen-sink's of clutter, all of which is inherited by all controllers.

I realize that is what's recommended in the pagy docs and also pretty common in the rails world, so definitely not a critique of your work!

If you're open to trying some alternatives:

• Call Pagy::Backend.pagy directly in actions. I think that should work and is very explicit and straightforward. Pagy::Frontend.pagy_nav in the view. On the downside, it's more wordy 😁
• Include the backend and frontend only in controllers that need them (+ a helper call might also be necessary?).
• Extract a thin module/concern to reduce the duplication of includeing the pagy modules in controllers

Curious your thoughts on this?

[yez]

I am open to calling Pagy::Backend.pagy explicitly, that's no problem. I originally had the include only in the PeopleController but thought that more pagination was likely coming soon so I'd get ahead of it (a pre-mature optimization on my part).

I'll see if the backend and frontend direct calls work the same and then update if they do.

[yez]

Looks like calling the pagy method directly on the Pagy::Bakend module doesn't work, we have to include it. I'll move the include to the people_controller instead. Same thing for Pagy::Frontend, I'll move that to a new people_helper to reduce the impact on the rest of the app.

[solebared]

I noticed that pagy ships with some bulma specific helpers. Not sure exactly what that provides, but am curious.

[yez]

I've not used bulma before so I'm happy to try out the bulma specific helpers. I'll see if one is obviously best.

[solebared]

I really appreciate you taking the time to add tests! 🙌🏾

That said, i'm quite hesitant to add this gem since the rails team very intentionally deprecated and removed access to assigns.

Relatedly, we've been preferring request specs over controller specs. My impression is that is becoming the recommended best practice.

I'd love to hear your thinking on all this?

[yez]

I can see the hesitation towards assigns and the rails-controller-testing gem. I introduced them because what I care about testing here wasn't so much the view rendering but if the count of people is what I'd expect.

Without the gem I could do the same test with an assertion against controller.instance_variable_get(:@people) but I'm not sure that's much better (reaching into the instance variables of an object without a reader is smelly code).

I could move this to a request spec and assert that the expected number of <td>s exist or <td>s with the expected people's names in them.

[yez]

@exbinary I have addressed all your comments. I think the request spec might be a little slow with all the matching, so I'm open to suggestions there.

[solebared]

Thanks for pulling this out, feels better!

[solebared]

What do you think about a concern that handles both the backend and frontend inclusion, eg:

# app/controllers/concerns/pagination.rb
module Pagination
  extend ActiveSupport::Concern

  include Pagy::Frontend
  include Pagy::Backend

  included do
    helper_method :pagy_bulma_nav
  end
end

[yez]

Oh that's good, I'll do this.

[solebared]

Cool, looks more bulma-ish :)

[solebared]

This is great! Appreciate you @yez 💯 🙏🏾