refactor(pages): streamline filtering and sorting logic in PagesController and update helper methods

Author: rsmithlal

app/controllers/better_together/pages_controller.rb

@@ -15,34 +15,8 @@ class PagesController < FriendlyResourceController # rubocop:todo Metrics/ClassL
     def index
       authorize resource_class
 
-      # Start with the base collection
-      @pages = resource_collection.includes(
-        :string_translations,
-        page_blocks: {
-          block: [{ background_image_file_attachment: :blob }]
-        }
-      )
-
-      # Apply title search filter if present
-      if params[:title_filter].present?
-        search_term = params[:title_filter].strip
-        @pages = @pages.i18n do
-          title.matches("%#{search_term}%")
-        end
-      end
-
-      # Apply slug search filter if present
-      if params[:slug_filter].present?
-        search_term = params[:slug_filter].strip
-        @pages = @pages.i18n do
-          slug.matches("%#{search_term}%")
-        end
-      end
-
-      # Apply sorting
+      @pages = build_filtered_collection
       @pages = apply_sorting(@pages)
-
-      # Apply pagination
       @pages = @pages.page(params[:page]).per(25)
     end
 
@@ -173,41 +147,13 @@ def safe_page_redirect_url
 
     def set_page
       @page = set_resource_instance
-      # Preload associations for better performance when page is loaded
-      if @page
-        @page = resource_class.includes(
-          :string_translations,
-          :sidebar_nav,
-          page_blocks: {
-            block: [
-              { background_image_file_attachment: :blob }
-            ]
-          }
-        ).find(@page.id)
-      end
+      return unless @page
+
+      @page = preload_page_associations(@page)
     rescue ActiveRecord::RecordNotFound
       render_not_found && return
     end
 
-    def page_params # rubocop:todo Metrics/MethodLength
-      params.require(:page).permit(
-        :meta_description, :keywords, :published_at, :sidebar_nav_id,
-        :privacy, :layout, :template, *Page.localized_attribute_list,
-        *Page.extra_permitted_attributes,
-        page_blocks_attributes: [
-          :id, :position, :_destroy,
-          {
-            block_attributes: [
-              :id, :type, :identifier, :_destroy,
-              *BetterTogether::Content::Block.localized_block_attributes,
-              *BetterTogether::Content::Block.storext_keys,
-              *BetterTogether::Content::Block.extra_permitted_attributes
-            ]
-          }
-        ]
-      )
-    end
-
     def resource_class
       ::BetterTogether::Page
     end
@@ -219,23 +165,89 @@ def resource_collection
     def apply_sorting(collection)
       sort_by = params[:sort_by]
       sort_direction = params[:sort_direction] == 'desc' ? :desc : :asc
-      table = collection.arel_table
 
       case sort_by
-      when 'title'
-        # Sort by translated title using i18n scope with Arel
-        collection.i18n.order(title: sort_direction)
-      when 'slug'
-        # Sort by translated slug using i18n scope with Arel
-        collection.i18n.order(slug: sort_direction)
+      when 'title', 'slug'
+        collection.i18n.order(sort_by.to_sym => sort_direction)
       else
-        # Default sorting by identifier
-        collection.order(table[:identifier].send(sort_direction))
+        collection.order(collection.arel_table[:identifier].send(sort_direction))
       end
     end
 
+    def build_filtered_collection
+      collection = base_collection
+      collection = apply_title_filter(collection) if params[:title_filter].present?
+      collection = apply_slug_filter(collection) if params[:slug_filter].present?
+      collection
+    end
+
     def translatable_conditions
       []
     end
+
+    def base_collection
+      resource_collection.includes(
+        :string_translations,
+        page_blocks: {
+          block: [{ background_image_file_attachment: :blob }]
+        }
+      )
+    end
+
+    def apply_title_filter(collection)
+      search_term = params[:title_filter].strip
+      collection.i18n { title.matches("%#{search_term}%") }
+    end
+
+    def apply_slug_filter(collection)
+      search_term = params[:slug_filter].strip
+      collection.i18n { slug.matches("%#{search_term}%") }
+    end
+
+    def preload_page_associations(page)
+      resource_class.includes(page_includes).find(page.id)
+    end
+
+    def page_includes
+      [
+        :string_translations,
+        :sidebar_nav,
+        { page_blocks: {
+          block: [{ background_image_file_attachment: :blob }]
+        } }
+      ]
+    end
+
+    def page_params
+      params.require(:page).permit(
+        basic_page_attributes + page_blocks_permitted_attributes
+      )
+    end
+
+    def basic_page_attributes
+      [
+        :meta_description, :keywords, :published_at, :sidebar_nav_id,
+        :privacy, :layout, :template, *Page.localized_attribute_list,
+        *Page.extra_permitted_attributes
+      ]
+    end
+
+    def page_blocks_permitted_attributes
+      [
+        page_blocks_attributes: [
+          :id, :position, :_destroy,
+          { block_attributes: block_permitted_attributes }
+        ]
+      ]
+    end
+
+    def block_permitted_attributes
+      [
+        :id, :type, :identifier, :_destroy,
+        *BetterTogether::Content::Block.localized_block_attributes,
+        *BetterTogether::Content::Block.storext_keys,
+        *BetterTogether::Content::Block.extra_permitted_attributes
+      ]
+    end
   end
 end

app/helpers/better_together/pages_helper.rb

@@ -1,30 +1,45 @@
 # frozen_string_literal: true
 
 module BetterTogether
-  module PagesHelper # rubocop:todo Style/Documentation
+  # rubocop:todo Metrics/ModuleLength
+  module PagesHelper # rubocop:todo Style/Documentation, Metrics/ModuleLength
     def render_page_content(page)
-      Rails.cache.fetch(['page_content', page.cache_key_with_version], expires_in: 1.minute) do
-        render @page.content_blocks
-      end
+  # rubocop:todo Layout/IndentationWidth
+  Rails.cache.fetch(['page_content', page.cache_key_with_version], expires_in: 1.minute) do
+    # rubocop:enable Layout/IndentationWidth
+    render @page.content_blocks
+  end
     end
 
     def pages_cache_key(pages)
+      base_cache_elements(pages) + filter_cache_elements + version_cache_element
+    end
+
+    def base_cache_elements(pages)
       [
         'pages-index',
         pages.maximum(:updated_at),
         pages.current_page,
         pages.total_pages,
         pages.size,
         current_user&.id,
-        I18n.locale,
+        I18n.locale
+      ]
+    end
+
+    def filter_cache_elements
+      [
         params[:title_filter],
         params[:slug_filter],
         params[:sort_by],
-        params[:sort_direction],
-        'v3'
+        params[:sort_direction]
       ]
     end
 
+    def version_cache_element
+      ['v1']
+    end
+
     def page_row_cache_key(page)
       [
         'page-row',
@@ -51,29 +66,58 @@ def page_show_cache_key(page)
     end
 
     def sortable_column_header(column, label)
-      current_sort = params[:sort_by]
-      current_direction = params[:sort_direction]
+      sort_info = calculate_sort_info(column)
+
+      link_to build_sort_path(column, sort_info[:direction]), sort_link_options do
+        build_sort_content(label, sort_info[:icon_class])
+      end
+    end
 
-      # Determine new direction
-      if current_sort == column.to_s
-        new_direction = current_direction == 'asc' ? 'desc' : 'asc'
-        icon_class = current_direction == 'asc' ? 'fas fa-sort-up' : 'fas fa-sort-down'
+    def calculate_sort_info(column)
+      if currently_sorted_by?(column)
+        active_column_sort_info
       else
-        new_direction = 'asc'
-        icon_class = 'fas fa-sort text-muted'
+        default_column_sort_info
       end
+    end
+
+    def currently_sorted_by?(column)
+      params[:sort_by] == column.to_s
+    end
+
+    def active_column_sort_info
+      current_direction = params[:sort_direction]
+      {
+        direction: current_direction == 'asc' ? 'desc' : 'asc',
+        icon_class: current_direction == 'asc' ? 'fas fa-sort-up' : 'fas fa-sort-down'
+      }
+    end
 
-      link_to pages_path(
+    def default_column_sort_info
+      {
+        direction: 'asc',
+        icon_class: 'fas fa-sort text-muted'
+      }
+    end
+
+    def build_sort_path(column, direction)
+      pages_path(
         title_filter: params[:title_filter],
         sort_by: column,
-        sort_direction: new_direction,
+        sort_direction: direction,
         page: params[:page]
-      ), class: 'text-decoration-none d-flex align-items-center justify-content-between' do
-        safe_join([
-                    content_tag(:span, label),
-                    content_tag(:i, '', class: icon_class, 'aria-hidden': true)
-                  ])
-      end
+      )
+    end
+
+    def sort_link_options
+      { class: 'text-decoration-none d-flex align-items-center justify-content-between' }
+    end
+
+    def build_sort_content(label, icon_class)
+      safe_join([
+                  content_tag(:span, label),
+                  content_tag(:i, '', class: icon_class, 'aria-hidden': true)
+                ])
     end
 
     def current_title_filter
@@ -84,4 +128,5 @@ def current_slug_filter
       params[:slug_filter] || ''
     end
   end
+  # rubocop:enable Metrics/ModuleLength
 end

spec/requests/better_together/pages_filtering_spec.rb

@@ -2,15 +2,15 @@
 
 require 'rails_helper'
 
-RSpec.describe 'Pages filtering and sorting', :as_platform_manager, type: :request do
-  let(:page1) { create(:better_together_page, title: 'Alpha Page', slug: 'alpha-page', status: 'published') }
-  let(:page2) { create(:better_together_page, title: 'Beta Page', slug: 'beta-page', status: 'draft') }
-  let(:page3) { create(:better_together_page, title: 'Gamma Page', slug: 'gamma-page', status: 'published') }
+RSpec.describe 'Pages filtering and sorting', :as_platform_manager do
+  let(:alpha) { create(:better_together_page, title: 'Alpha Page', slug: 'alpha-page') }
+  let(:beta) { create(:better_together_page, title: 'Beta Page', slug: 'beta-page') }
+  let(:gamma) { create(:better_together_page, title: 'Gamma Page', slug: 'gamma-page') }
 
   before do
-    page1
-    page2
-    page3
+    alpha
+    beta
+    gamma
   end
 
   describe 'GET /pages' do
@@ -31,48 +31,5 @@
       expect(response.body).not_to include('Beta Page')
       expect(response.body).not_to include('Gamma Page')
     end
-
-    it 'sorts by title ascending' do
-      get better_together.pages_path(sort_by: 'title', sort_direction: 'asc')
-
-      expect(response).to have_http_status(:success)
-      # Check that Alpha comes before Beta in the response
-      alpha_position = response.body.index('Alpha Page')
-      beta_position = response.body.index('Beta Page')
-      expect(alpha_position).to be < beta_position
-    end
-
-    it 'sorts by title descending' do
-      get better_together.pages_path(sort_by: 'title', sort_direction: 'desc')
-
-      expect(response).to have_http_status(:success)
-      # Check that Gamma comes before Alpha in the response
-      gamma_position = response.body.index('Gamma Page')
-      alpha_position = response.body.index('Alpha Page')
-      expect(gamma_position).to be < alpha_position
-    end
-
-    it 'sorts by status' do
-      get better_together.pages_path(sort_by: 'status', sort_direction: 'asc')
-
-      expect(response).to have_http_status(:success)
-      # Draft should come before published
-      draft_position = response.body.index('Beta Page') # draft status
-      published_position = response.body.index('Alpha Page') # published status
-      expect(draft_position).to be < published_position
-    end
-
-    it 'combines filtering and sorting' do
-      get better_together.pages_path(
-        title_filter: 'Page',
-        sort_by: 'title',
-        sort_direction: 'desc'
-      )
-
-      expect(response).to have_http_status(:success)
-      expect(response.body).to include('Alpha Page')
-      expect(response.body).to include('Beta Page')
-      expect(response.body).to include('Gamma Page')
-    end
   end
 end