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.

Sign up for GitHub

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

Replace kaminari with pagy #681

Merged
merged 5 commits into from
Oct 7, 2023
Merged

Replace kaminari with pagy #681

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2fa1100
Replace kaminari with pagy
amadayami May 23, 2023
8b7bf2b
Update pagy_helper.rb
amadayami Jun 6, 2023
bf7126f
change pagy params to a class constant
amadayami Jun 21, 2023
037b44a
Merge remote-tracking branch 'refugerestrooms/develop' into kaminari_…
amadayami Oct 6, 2023
d90e215
Updated syntax for rubocop
amadayami Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,17 @@ gem 'country_select'
gem 'devise', '~> 4.8.1'
gem 'geocoder', '~> 1.6.1'
gem 'grape', '~> 1.6.2'
gem 'grape-pagy'
gem 'grape-swagger'
gem 'grape-kaminari', '~> 0.4.3'
gem 'haml'
gem 'high_voltage', '~> 3.0.0'
gem 'http_accept_language'
gem 'jbuilder', '~> 2.5'
gem 'kaminari', '~> 1.2.2'
gem 'kaminari-grape', '~> 1.0'
gem 'mail_form', '>= 1.7.0'
gem 'net-smtp', require: false
gem 'net-imap', require: false
gem 'net-pop', require: false
gem 'pagy'
gem 'pg'
gem 'pg_search'
gem 'puma'
Expand Down
15 changes: 6 additions & 9 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,9 @@ GEM
mustermann-grape (~> 1.0.0)
rack (>= 1.3.0)
rack-accept
grape-kaminari (0.4.3)
grape (>= 1.6.1)
kaminari-grape
grape-pagy (0.6.0)
grape (>= 1.5)
pagy (>= 6.0)
grape-swagger (1.5.0)
grape (~> 1.3)
haml (6.0.4)
Expand Down Expand Up @@ -214,9 +214,6 @@ GEM
activerecord
kaminari-core (= 1.2.2)
kaminari-core (1.2.2)
kaminari-grape (1.0.1)
grape
kaminari-core (~> 1.0)
loofah (2.20.0)
crass (~> 1.0.2)
nokogiri (>= 1.5.9)
Expand Down Expand Up @@ -255,6 +252,7 @@ GEM
nokogiri (1.14.3-x86_64-linux)
racc (~> 1.4)
orm_adapter (0.5.0)
pagy (6.0.4)
parallel (1.22.1)
parser (3.1.2.1)
ast (~> 2.4.1)
Expand Down Expand Up @@ -440,19 +438,18 @@ DEPENDENCIES
factory_bot_rails (~> 4.8.2)
geocoder (~> 1.6.1)
grape (~> 1.6.2)
grape-kaminari (~> 0.4.3)
grape-pagy
grape-swagger
haml
high_voltage (~> 3.0.0)
http_accept_language
i18n-debug
jbuilder (~> 2.5)
kaminari (~> 1.2.2)
kaminari-grape (~> 1.0)
mail_form (>= 1.7.0)
net-imap
net-pop
net-smtp
pagy
pg
pg_search
poltergeist
Expand Down
27 changes: 21 additions & 6 deletions app/controllers/api/v1/restrooms.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
module API
module V1
class Restrooms < Grape::API
include Grape::Kaminari
paginate per_page: 10, max_per_page: 100
helpers Grape::Pagy::Helpers

version 'v1'
format :json
Expand All @@ -11,6 +10,10 @@ class Restrooms < Grape::API
resource :restrooms do
desc "Get all restroom records ordered by date descending."
params do
use :pagy,
items_param: :per_page,
items: 10,
max_items: 100
Copy link

Choose a reason for hiding this comment

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

I noticed these lines are being reused in each desc, would it be possible to set this param for all of them outside of each example? Kinda like line 5 on the old file.

Something like use :page, items_param :per_page, items: 10, max_items: 100 on line 5 ofthis file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have changed it so that those parameters are written as a class constant for the descs to reference instead and have committed that change. Grape Pagy does not appear to have anything equivalent to the old line 5, so this would be the best way to extract those parameters.

optional :ada, type: Boolean, desc: "Only return restrooms that are ADA accessible."
optional :unisex, type: Boolean, desc: "Only return restrooms that are unisex."
end
Expand All @@ -20,11 +23,15 @@ class Restrooms < Grape::API
r = r.accessible if params[:ada].present?
r = r.unisex if params[:unisex].present?

paginate(r.order(created_at: :desc))
pagy(r.order(created_at: :desc))
end

desc "Perform full-text search of restroom records."
params do
use :pagy,
items_param: :per_page,
items: 10,
max_items: 100
optional :ada, type: Boolean, desc: "Only return restrooms that are ADA accessible."
optional :unisex, type: Boolean, desc: "Only return restrooms that are unisex."
requires :query, type: String, desc: "Your search query."
Expand All @@ -35,11 +42,15 @@ class Restrooms < Grape::API
r = r.accessible if params[:ada].present?
r = r.unisex if params[:unisex].present?

paginate(r.text_search(params[:query]))
pagy(r.text_search(params[:query]))
end

desc "Search by location."
params do
use :pagy,
items_param: :per_page,
items: 10,
max_items: 100
optional :ada, type: Boolean, desc: "Only return restrooms that are ADA accessible."
optional :unisex, type: Boolean, desc: "Only return restrooms that are unisex."
requires :lat, type: Float, desc: "latitude"
Expand All @@ -50,11 +61,15 @@ class Restrooms < Grape::API
r = r.current
r = r.accessible if params[:ada].present?
r = r.unisex if params[:unisex]
paginate(r.near([params[:lat], params[:lng]], 20, order: 'distance'))
pagy(r.near([params[:lat], params[:lng]], 20, order: 'distance'))
end

desc "Search for restroom records updated or created on or after a given date"
params do
use :pagy,
items_param: :per_page,
items: 10,
max_items: 100
optional :ada, type: Boolean, desc: "Only return restrooms that are ADA accessible."
optional :unisex, type: Boolean, desc: "Only return restrooms that are unisex."
optional(
Expand All @@ -77,7 +92,7 @@ class Restrooms < Grape::API
end
r = r.accessible if params[:ada].present?
r = r.unisex if params[:unisex].present?
paginate(r.order(created_at: :desc))
pagy(r.order(created_at: :desc))
end
end
# rubocop:enable Metrics/BlockLength
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/application_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
class ApplicationController < ActionController::Base
# Prevent CSRF attacks by raising an exception.
# For APIs, you may want to use :null_session instead.
include Pagy::Backend

protect_from_forgery with: :exception

around_action :switch_locale
Expand Down
5 changes: 2 additions & 3 deletions app/controllers/restrooms_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,15 +104,14 @@ def restrooms_filters

# rubocop:disable Metrics/AbcSize
def list_restrooms
@restrooms = Restroom.current.where(@filters).page(params[:page])
@restrooms = Restroom.current.where(@filters)
@restrooms =
if params[:search].present? || params[:map] == "1"
@restrooms.near([params[:lat], params[:long]], 20, order: 'distance')
else
@restrooms.reverse_order
end

@restrooms = @restrooms.out_of_range? ? @restrooms.page(1) : @restrooms
@pagy, @restrooms = pagy(@restrooms)[0].overflow? ? pagy(@restrooms, page: 1) : pagy(@restrooms)
end
# rubocop:enable Metrics/AbcSize

Expand Down
3 changes: 3 additions & 0 deletions app/helpers/pagy_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module PagyHelper
include Pagy::Frontend
end
11 changes: 0 additions & 11 deletions app/views/kaminari/_first_page.html.erb
View file
Open in desktop

This file was deleted.

8 changes: 0 additions & 8 deletions app/views/kaminari/_gap.html.erb
View file
Open in desktop

This file was deleted.

11 changes: 0 additions & 11 deletions app/views/kaminari/_last_page.html.erb
View file
Open in desktop

This file was deleted.

11 changes: 0 additions & 11 deletions app/views/kaminari/_next_page.html.erb
View file
Open in desktop

This file was deleted.

12 changes: 0 additions & 12 deletions app/views/kaminari/_page.html.erb
View file
Open in desktop

This file was deleted.

23 changes: 0 additions & 23 deletions app/views/kaminari/_paginator.html.erb
View file
Open in desktop

This file was deleted.

11 changes: 0 additions & 11 deletions app/views/kaminari/_prev_page.html.erb
View file
Open in desktop

This file was deleted.

6 changes: 3 additions & 3 deletions app/views/restrooms/index.html.haml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,20 +26,20 @@
.col-sm-12.headroom
%ul{:class => "restrooms-list", :id => "list"}
= render @restrooms
= paginate @restrooms
!= pagy_nav @pagy
Copy link

Choose a reason for hiding this comment

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

Is there a comparison here we could make to keep the = instead of !=?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! It can be rewritten in this way to maintain the = that was there previously. This is functionally the same as using !=.

Suggested change
!= pagy_nav @pagy
= pagy_nav(@pagy).html_safe

Originally, the != evaluates the Ruby without ever sanitizing the HTML. Rewritten, the = is maintained but it's assumed to be safe by marking it with html_safe. It has to be done in this format because pagy is an "agnostic pagination gem it does not use the specific html_safe rails helper on its output".

/ .row
/ .col-xs-12.headroom
/ .restrooms-index-content
/ = content_tag 'div', id: 'map-container', data: { latitude: params[:lat], longitude: params[:long] } do
/ #map-content
/ .restrooms-list
/ = render @restrooms
/ = paginate @restrooms
/ != pagy_nav @pagy


/ #results.headroom
/ = content_tag 'div', id: 'mapContainer', data: { latitude: params[:lat], longitude: params[:long] } do
/ #mapArea
/ #list.restrooms-list
/ = render @restrooms
/ = paginate @restrooms
/ != pagy_nav @pagy
Loading