From d2585a34fe9d15d8cafb5601882cdf12a8419ca7 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 15 Nov 2019 09:30:52 +0100 Subject: [PATCH 1/7] Refactor api pages controller spec --- .../alchemy/api/pages_controller_spec.rb | 27 +++++-------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/spec/controllers/alchemy/api/pages_controller_spec.rb b/spec/controllers/alchemy/api/pages_controller_spec.rb index 367e453ba2..4465ae68a5 100644 --- a/spec/controllers/alchemy/api/pages_controller_spec.rb +++ b/spec/controllers/alchemy/api/pages_controller_spec.rb @@ -8,16 +8,19 @@ module Alchemy describe '#index' do let!(:page) { create(:alchemy_page, :public) } + let(:result) { JSON.parse(response.body) } - it "returns all public pages as json objects" do + it 'returns JSON' do get :index, params: {format: :json} expect(response.status).to eq(200) expect(response.media_type).to eq('application/json') + expect(result).to have_key('pages') + end - result = JSON.parse(response.body) + it "returns all public pages" do + get :index, params: {format: :json} - expect(result).to have_key('pages') expect(result['pages'].size).to eq(2) end @@ -27,12 +30,6 @@ module Alchemy it "returns only page with this page layout" do get :index, params: {page_layout: 'news', format: :json} - expect(response.status).to eq(200) - expect(response.media_type).to eq('application/json') - - result = JSON.parse(response.body) - - expect(result).to have_key('pages') expect(result['pages'].size).to eq(1) end end @@ -41,12 +38,6 @@ module Alchemy it "returns all pages" do get :index, params: {page_layout: '', format: :json} - expect(response.status).to eq(200) - expect(response.media_type).to eq('application/json') - - result = JSON.parse(response.body) - - expect(result).to have_key('pages') expect(result['pages'].size).to eq(2) end end @@ -59,12 +50,6 @@ module Alchemy it "returns all pages" do get :index, params: {format: :json} - expect(response.status).to eq(200) - expect(response.media_type).to eq('application/json') - - result = JSON.parse(response.body) - - expect(result).to have_key('pages') expect(result['pages'].size).to eq(Alchemy::Page.count) end end From ce190a7a3326c1baf7c869254edc83d77cb01570 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 15 Nov 2019 09:35:56 +0100 Subject: [PATCH 2/7] Ignore local rspec conf --- .gitignore | 1 + .rspec | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) delete mode 100644 .rspec diff --git a/.gitignore b/.gitignore index 24a452fb1b..3a28ffe484 100644 --- a/.gitignore +++ b/.gitignore @@ -21,3 +21,4 @@ spec/dummy/public/assets .ruby-gemset .ruby-version .env +.rspec diff --git a/.rspec b/.rspec deleted file mode 100644 index 5052887a0f..0000000000 --- a/.rspec +++ /dev/null @@ -1 +0,0 @@ ---color \ No newline at end of file From f6986a59b78af1aba67e702d1080fd28866db61d Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 15 Nov 2019 09:38:32 +0100 Subject: [PATCH 3/7] API: Add support for pagination page results --- .../alchemy/api/pages_controller.rb | 31 ++++++++++++++++++- .../alchemy/api/pages_controller_spec.rb | 27 ++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/app/controllers/alchemy/api/pages_controller.rb b/app/controllers/alchemy/api/pages_controller.rb index 3a44c30ad6..ed78c0062e 100644 --- a/app/controllers/alchemy/api/pages_controller.rb +++ b/app/controllers/alchemy/api/pages_controller.rb @@ -16,7 +16,12 @@ def index if params[:page_layout].present? @pages = @pages.where(page_layout: params[:page_layout]) end - render json: @pages, adapter: :json, root: 'pages' + + if params[:page] + @pages = @pages.page(params[:page]).per(params[:per_page]) + end + + render json: @pages, adapter: :json, root: 'pages', meta: meta_data end # Returns all pages as nested json object for tree views @@ -54,5 +59,29 @@ def load_page ) || raise(ActiveRecord::RecordNotFound) end + + def meta_data + { + total_count: total_count_value, + per_page: per_page_value, + page: page_value + } + end + + def total_count_value + params[:page] ? @pages.total_count : @pages.size + end + + def per_page_value + if params[:page] + (params[:per_page] || Kaminari.config.default_per_page).to_i + else + @pages.size + end + end + + def page_value + params[:page] ? params[:page].to_i : nil + end end end diff --git a/spec/controllers/alchemy/api/pages_controller_spec.rb b/spec/controllers/alchemy/api/pages_controller_spec.rb index 4465ae68a5..14ed8f945e 100644 --- a/spec/controllers/alchemy/api/pages_controller_spec.rb +++ b/spec/controllers/alchemy/api/pages_controller_spec.rb @@ -53,6 +53,33 @@ module Alchemy expect(result['pages'].size).to eq(Alchemy::Page.count) end end + + it 'includes meta data' do + get :index, params: { format: :json } + + expect(result['pages'].size).to eq(2) + expect(result['meta']['page']).to be_nil + expect(result['meta']['per_page']).to eq(2) + expect(result['meta']['total_count']).to eq(2) + end + + context 'with page param given' do + let!(:page1) { create(:alchemy_page) } + let!(:page2) { create(:alchemy_page) } + + before do + expect(Kaminari.config).to receive(:default_per_page).at_least(:once) { 1 } + end + + it 'returns paginated result' do + get :index, params: { page: 2, format: :json } + + expect(result['pages'].size).to eq(1) + expect(result['meta']['page']).to eq(2) + expect(result['meta']['per_page']).to eq(1) + expect(result['meta']['total_count']).to eq(2) + end + end end describe '#nested' do From feaa50f492d69ec65016a7e16bb6913b0bdcdb9f Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 15 Nov 2019 09:53:37 +0100 Subject: [PATCH 4/7] API: Add support for Ransack search queries --- app/controllers/alchemy/api/pages_controller.rb | 8 ++++++++ spec/controllers/alchemy/api/pages_controller_spec.rb | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/app/controllers/alchemy/api/pages_controller.rb b/app/controllers/alchemy/api/pages_controller.rb index ed78c0062e..7180202d84 100644 --- a/app/controllers/alchemy/api/pages_controller.rb +++ b/app/controllers/alchemy/api/pages_controller.rb @@ -14,8 +14,16 @@ def index @pages = Page.accessible_by(current_ability, :index) end if params[:page_layout].present? + Alchemy::Deprecation.warn <<~WARN + Passing page_layout parameter to Alchemy::Api::PagesController#index is deprecated. + Please pass a Ransack search query instead: + q: { + page_layout_eq: '#{params[:page_layout]}' + } + WARN @pages = @pages.where(page_layout: params[:page_layout]) end + @pages = @pages.ransack(params[:q]).result if params[:page] @pages = @pages.page(params[:page]).per(params[:per_page]) diff --git a/spec/controllers/alchemy/api/pages_controller_spec.rb b/spec/controllers/alchemy/api/pages_controller_spec.rb index 14ed8f945e..90749576a7 100644 --- a/spec/controllers/alchemy/api/pages_controller_spec.rb +++ b/spec/controllers/alchemy/api/pages_controller_spec.rb @@ -80,6 +80,14 @@ module Alchemy expect(result['meta']['total_count']).to eq(2) end end + + context 'with ransack query param given' do + it 'returns filtered result' do + get :index, params: { q: { name_eq: page.name }, format: :json } + + expect(result['pages'].size).to eq(1) + end + end end describe '#nested' do From c9cf9422efdadc95d30a174ce8b77c2435527d10 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 15 Nov 2019 09:54:39 +0100 Subject: [PATCH 5/7] API: Eager load page associations In order to prevent N+1 queries in the pages API we eager load all necessary associations --- .../alchemy/api/pages_controller.rb | 23 +++++++++++++++++++ app/serializers/alchemy/page_serializer.rb | 4 ---- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/app/controllers/alchemy/api/pages_controller.rb b/app/controllers/alchemy/api/pages_controller.rb index 7180202d84..8a14668fbf 100644 --- a/app/controllers/alchemy/api/pages_controller.rb +++ b/app/controllers/alchemy/api/pages_controller.rb @@ -13,6 +13,7 @@ def index else @pages = Page.accessible_by(current_ability, :index) end + @pages = @pages.includes(*page_includes) if params[:page_layout].present? Alchemy::Deprecation.warn <<~WARN Passing page_layout parameter to Alchemy::Api::PagesController#index is deprecated. @@ -91,5 +92,27 @@ def per_page_value def page_value params[:page] ? params[:page].to_i : nil end + + def page_includes + [ + :tags, + { + elements: [ + { + nested_elements: [ + { + contents: :essence + }, + :tags + ] + }, + { + contents: :essence + }, + :tags + ] + } + ] + end end end diff --git a/app/serializers/alchemy/page_serializer.rb b/app/serializers/alchemy/page_serializer.rb index eb6552f4dd..776cda862e 100644 --- a/app/serializers/alchemy/page_serializer.rb +++ b/app/serializers/alchemy/page_serializer.rb @@ -16,9 +16,5 @@ class PageSerializer < ActiveModel::Serializer :status has_many :elements - - def elements - object.elements.published - end end end From 1f1955da258b13b1d0537828f916b266cb98c322 Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 15 Nov 2019 14:49:03 +0100 Subject: [PATCH 6/7] Add active_record_query_trace gem The Rails included query trace only works for application code. Since we build a gem we need to use this very useful gem. --- Gemfile | 1 + .../config/initializers/active_record_query_trace.rb | 10 ++++++++++ 2 files changed, 11 insertions(+) create mode 100644 spec/dummy/config/initializers/active_record_query_trace.rb diff --git a/Gemfile b/Gemfile index e046fc412b..012ed4fc7f 100644 --- a/Gemfile +++ b/Gemfile @@ -27,5 +27,6 @@ group :development, :test do gem 'localeapp', '~> 3.0', require: false gem 'dotenv', '~> 2.2' gem 'github_fast_changelog', require: false + gem 'active_record_query_trace', require: false end end diff --git a/spec/dummy/config/initializers/active_record_query_trace.rb b/spec/dummy/config/initializers/active_record_query_trace.rb new file mode 100644 index 0000000000..4fb6ca5f30 --- /dev/null +++ b/spec/dummy/config/initializers/active_record_query_trace.rb @@ -0,0 +1,10 @@ +if Rails.env.development? + require 'active_record_query_trace' + ActiveRecordQueryTrace.enabled = true + ActiveRecordQueryTrace.level = :custom + ActiveRecordQueryTrace.backtrace_cleaner = ->(trace) do + trace.reject do |line| + line =~ /\b(active_record_query_trace|active_support|active_record|rack-mini-profiler)\b/ + end + end +end From ac18eca900c29a02b6349444c523637fbe17c42a Mon Sep 17 00:00:00 2001 From: Thomas von Deyen Date: Fri, 15 Nov 2019 14:53:41 +0100 Subject: [PATCH 7/7] Add missing inverse_of's Without these inverse_of's Rails loads the records even if they have been eager loaded. --- app/models/alchemy/content.rb | 2 +- app/models/alchemy/element.rb | 2 +- lib/alchemy/essence.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/alchemy/content.rb b/app/models/alchemy/content.rb index 0ab8dfd34e..622fd96d14 100644 --- a/app/models/alchemy/content.rb +++ b/app/models/alchemy/content.rb @@ -24,7 +24,7 @@ class Content < BaseRecord # Concerns include Alchemy::Content::Factory - belongs_to :essence, polymorphic: true, dependent: :destroy + belongs_to :essence, polymorphic: true, dependent: :destroy, inverse_of: :content belongs_to :element, touch: true, inverse_of: :contents has_one :page, through: :element diff --git a/app/models/alchemy/element.rb b/app/models/alchemy/element.rb index b5a584f210..609f3672b1 100644 --- a/app/models/alchemy/element.rb +++ b/app/models/alchemy/element.rb @@ -62,7 +62,7 @@ class Element < BaseRecord # Content positions are scoped by their essence_type, so positions can be the same for different contents. # In order to get contents in creation order we also order them by id. - has_many :contents, -> { order(:position, :id) }, dependent: :destroy + has_many :contents, -> { order(:position, :id) }, dependent: :destroy, inverse_of: :element has_many :all_nested_elements, -> { order(:position) }, diff --git a/lib/alchemy/essence.rb b/lib/alchemy/essence.rb index 370198965d..ed472b5926 100644 --- a/lib/alchemy/essence.rb +++ b/lib/alchemy/essence.rb @@ -37,7 +37,7 @@ def acts_as_essence(options = {}) stampable stamper_class_name: Alchemy.user_class_name validate :validate_ingredient, on: :update, if: -> { validations.any? } - has_one :content, as: :essence, class_name: "Alchemy::Content" + has_one :content, as: :essence, class_name: "Alchemy::Content", inverse_of: :essence has_one :element, through: :content, class_name: "Alchemy::Element" has_one :page, through: :element, class_name: "Alchemy::Page"