Deprecate pagination in searcher #2119

cbrunsdon · 2017-07-26T01:38:23Z

This deprecates doing pagination in the base searcher and instead pushes the responsibility onto the caller (in our cases the controllers).

We don't gain anything from doing the pagination inside the searcher, and it doesn't have anything to do with the core goal of it (searching).

Also throws the explicit dependencies on kaminari on frontend and backend (as they call kaminari functions).

It shouldn't be the searcher's responsibility to also handle how pagination is happening.

houndci-bot · 2017-07-26T01:38:31Z

frontend/app/controllers/spree/taxons_controller.rb

@@ -9,8 +9,8 @@ def show
      @taxon = Spree::Taxon.find_by!(permalink: params[:id])
      return unless @taxon

-      @searcher = build_searcher(params.merge(taxon: @taxon.id, include_images: true))
-      @products = @searcher.retrieve_products
+      @searcher = build_searcher(params.merge(taxon: @taxon.id, include_images: true).reject { |k, v| ["per_page", "page"].include?(k) } )


Unused block argument - v. If it's necessary, use _ or _v as an argument name to indicate that it won't be used.

houndci-bot · 2017-07-26T01:38:31Z

frontend/app/controllers/spree/products_controller.rb

@@ -9,8 +9,8 @@ class ProductsController < Spree::StoreController
    respond_to :html

    def index
-      @searcher = build_searcher(params.merge(include_images: true))
-      @products = @searcher.retrieve_products
+      @searcher = build_searcher(params.merge(include_images: true).reject { |k, v| ["per_page", "page"].include?(k) } )


Unused block argument - v. If it's necessary, use _ or _v as an argument name to indicate that it won't be used.

houndci-bot · 2017-07-26T01:38:31Z

core/lib/spree/core/search/base.rb

+
+            per_page = params[:per_page].to_i
+            @properties[:per_page] = per_page > 0 ? per_page : Spree::Config[:products_per_page]
+            @properties[:page] = (params[:page].to_i <= 0) ? 1 : params[:page].to_i


Omit parentheses for ternary conditions.

houndci-bot · 2017-07-26T01:38:31Z

frontend/app/controllers/spree/taxons_controller.rb

@@ -9,8 +9,8 @@ def show
      @taxon = Spree::Taxon.find_by!(permalink: params[:id])
      return unless @taxon

-      @searcher = build_searcher(params.merge(taxon: @taxon.id, include_images: true))
-      @products = @searcher.retrieve_products
+      @searcher = build_searcher(params.merge(taxon: @taxon.id, include_images: true).reject { |k, v| ["per_page", "page"].include?(k) } )


Unused block argument - v. If it's necessary, use _ or _v as an argument name to indicate that it won't be used.

houndci-bot · 2017-07-26T01:38:31Z

frontend/app/controllers/spree/products_controller.rb

@@ -9,8 +9,8 @@ class ProductsController < Spree::StoreController
    respond_to :html

    def index
-      @searcher = build_searcher(params.merge(include_images: true))
-      @products = @searcher.retrieve_products
+      @searcher = build_searcher(params.merge(include_images: true).reject { |k, v| ["per_page", "page"].include?(k) } )


Unused block argument - v. If it's necessary, use _ or _v as an argument name to indicate that it won't be used.

houndci-bot · 2017-07-26T01:38:31Z

core/lib/spree/core/search/base.rb

+
+            per_page = params[:per_page].to_i
+            @properties[:per_page] = per_page > 0 ? per_page : Spree::Config[:products_per_page]
+            @properties[:page] = (params[:page].to_i <= 0) ? 1 : params[:page].to_i


Omit parentheses for ternary conditions.

houndci-bot · 2017-07-26T02:19:52Z

frontend/app/controllers/spree/taxons_controller.rb

@@ -9,8 +9,9 @@ def show
      @taxon = Spree::Taxon.find_by!(permalink: params[:id])
      return unless @taxon

-      @searcher = build_searcher(params.merge(taxon: @taxon.id, include_images: true))
-      @products = @searcher.retrieve_products
+      binding.pry


Remove debugger entry point binding.pry.

houndci-bot · 2017-07-26T02:19:52Z

frontend/app/controllers/spree/products_controller.rb

@@ -9,8 +9,9 @@ class ProductsController < Spree::StoreController
    respond_to :html

    def index
-      @searcher = build_searcher(params.merge(include_images: true))
-      @products = @searcher.retrieve_products
+      binding.pry


Remove debugger entry point binding.pry.

houndci-bot · 2017-07-26T02:19:52Z

frontend/app/controllers/spree/taxons_controller.rb

@@ -9,8 +9,9 @@ def show
      @taxon = Spree::Taxon.find_by!(permalink: params[:id])
      return unless @taxon

-      @searcher = build_searcher(params.merge(taxon: @taxon.id, include_images: true))
-      @products = @searcher.retrieve_products
+      binding.pry


Remove debugger entry point binding.pry.

houndci-bot · 2017-07-26T02:19:53Z

frontend/app/controllers/spree/products_controller.rb

@@ -9,8 +9,9 @@ class ProductsController < Spree::StoreController
    respond_to :html

    def index
-      @searcher = build_searcher(params.merge(include_images: true))
-      @products = @searcher.retrieve_products
+      binding.pry


Remove debugger entry point binding.pry.

These two places are the only place we support the searcher on the frontend

tvdeyen

A very good change. Should we add a changelog note?

kennyadsl

Great change!

jhawthorn · 2017-07-26T18:37:25Z

Should solidus_api also receive an explicit dependency as it calls kaminari functions?

We actually use kaminari on the frontend, backend, and api. We should explicitely add it as a dependency

cbrunsdon · 2017-07-26T23:00:03Z

Good call hawth, added.

ahwong5 · 2017-12-09T05:06:58Z

@cbrunsdon You have omitted to update the home controller as well, therefore the products at frontend homepage has lost the ability to paginate.

And also when performing Search, it doesn't paginate as well. I'm not sure which file need to update.

Thanks.

Deprecate pagination inside of our searcher

61194f4

It shouldn't be the searcher's responsibility to also handle how pagination is happening.

houndci-bot reviewed Jul 26, 2017

View reviewed changes

cbrunsdon force-pushed the deprecate_pagination_in_searcher branch 2 times, most recently from 4169eb5 to 6874572 Compare July 26, 2017 02:19

houndci-bot reviewed Jul 26, 2017

View reviewed changes

cbrunsdon force-pushed the deprecate_pagination_in_searcher branch from 6874572 to 0e23f24 Compare July 26, 2017 02:36

Paginate searcher inside frontend controllers

aa13a58

These two places are the only place we support the searcher on the frontend

cbrunsdon force-pushed the deprecate_pagination_in_searcher branch from 0e23f24 to 8aa9c81 Compare July 26, 2017 02:57

tvdeyen approved these changes Jul 26, 2017

View reviewed changes

kennyadsl approved these changes Jul 26, 2017

View reviewed changes

Add kaminari to dependcies for frontend/backend/api

8a0e7cd

We actually use kaminari on the frontend, backend, and api. We should explicitely add it as a dependency

cbrunsdon force-pushed the deprecate_pagination_in_searcher branch from 8aa9c81 to 8a0e7cd Compare July 26, 2017 22:59

jhawthorn approved these changes Jul 27, 2017

View reviewed changes

jhawthorn merged commit 1f46990 into solidusio:master Jul 27, 2017

jhawthorn mentioned this pull request Dec 14, 2017

Revert "deprecate pagination in searcher" #2455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate pagination in searcher #2119

Deprecate pagination in searcher #2119

cbrunsdon commented Jul 26, 2017

houndci-bot Jul 26, 2017

houndci-bot Jul 26, 2017

houndci-bot Jul 26, 2017

houndci-bot Jul 26, 2017

houndci-bot Jul 26, 2017

houndci-bot Jul 26, 2017

houndci-bot Jul 26, 2017

houndci-bot Jul 26, 2017

houndci-bot Jul 26, 2017

houndci-bot Jul 26, 2017

tvdeyen left a comment

kennyadsl left a comment

jhawthorn commented Jul 26, 2017

cbrunsdon commented Jul 26, 2017

ahwong5 commented Dec 9, 2017 •

edited

Loading

Deprecate pagination in searcher #2119

Deprecate pagination in searcher #2119

Conversation

cbrunsdon commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

jhawthorn commented Jul 26, 2017

cbrunsdon commented Jul 26, 2017

ahwong5 commented Dec 9, 2017 • edited Loading

ahwong5 commented Dec 9, 2017 •

edited

Loading