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.

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

Eagerload at the controller or job layer #2313

Merged
merged 7 commits into from
Apr 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 5 additions & 1 deletion app/controllers/alchemy/admin/pages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ def process_url(ancestors_path, item)
end

def load_resource
@page = Page.find(params[:id])
@page = Page.includes(page_includes).find(params[:id])
end

def pages_from_raw_request
Expand Down Expand Up @@ -402,6 +402,10 @@ def load_languages_and_layouts
def set_preview_mode
@preview_mode = true
end

def page_includes
Alchemy::EagerLoading.page_includes(version: :draft_version)
end
end
end
end
24 changes: 19 additions & 5 deletions app/controllers/alchemy/pages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,14 @@ def locale_prefix_not_allowed?
# If no index page and no admin users are present we show the "Welcome to Alchemy" page.
#
def load_index_page
@page ||= Language.current_root_page
@page ||= begin
Alchemy::Page.
contentpages.
language_roots.
where(language: Language.current).
includes(page_includes).
first
end
render template: "alchemy/welcome", layout: false if signup_required?
end

Expand All @@ -120,10 +127,13 @@ def load_index_page
def load_page
page_not_found! unless Language.current

@page ||= Language.current.pages.contentpages.find_by(
urlname: params[:urlname],
language_code: params[:locale] || Language.current.code,
)
@page ||= begin
Alchemy::Page.
contentpages.
where(language: Language.current).
includes(page_includes).
find_by(urlname: params[:urlname])
end
end

def enforce_locale
Expand Down Expand Up @@ -234,5 +244,9 @@ def must_not_cache?
def page_not_found!
not_found_error!("Alchemy::Page not found \"#{request.fullpath}\"")
end

def page_includes
Alchemy::EagerLoading.page_includes(version: :public_version)
end
end
end
5 changes: 4 additions & 1 deletion app/jobs/alchemy/publish_page_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ module Alchemy
class PublishPageJob < BaseJob
queue_as :default

def perform(page, public_on:)
def perform(page_id, public_on:)
page = Alchemy::Page.includes(
Alchemy::EagerLoading.page_includes(version: :draft_version)
).find(page_id)
Alchemy::Page::Publisher.new(page).publish!(public_on: public_on)
end
end
Expand Down
39 changes: 39 additions & 0 deletions app/models/alchemy/eager_loading.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true

module Alchemy
# Eager loading parameters for loading pages
class EagerLoading
PAGE_VERSIONS = %i[draft_version public_version]

class << self
# Eager loading parameters for {ActiveRecord::Base.includes}
#
# Pass this to +includes+ whereever you load an {Alchemy::Page}
#
# Alchemy::Page.includes(Alchemy::EagerLoading.page_includes).find_by(urlname: "my-page")
#
# @param version [Symbol] Type of page version to eager load
# @return [Array]
def page_includes(version: :public_version)
raise UnsupportedPageVersion unless version.in? PAGE_VERSIONS

[
:tags,
{
language: :site,
version => {
elements: [
:page,
:touchable_pages,
{
ingredients: :related_object,
contents: :essence,
},
],
},
},
]
end
end
end
end
2 changes: 1 addition & 1 deletion app/models/alchemy/page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,7 @@ def copy_children_to(new_parent)
#
def publish!(current_time = Time.current)
update(published_at: current_time)
PublishPageJob.perform_later(self, public_on: current_time)
PublishPageJob.perform_later(id, public_on: current_time)
end

# Sets the public_on date on the published version
Expand Down
2 changes: 1 addition & 1 deletion app/models/alchemy/page_version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def still_public_for?(time = Time.current)
end

def element_repository
ElementsRepository.new(elements.includes({ contents: :essence }, :tags))
ElementsRepository.new(elements)
end

private
Expand Down
10 changes: 8 additions & 2 deletions lib/alchemy/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ class DefaultLanguageNotFoundError < StandardError
# Raised if no default language configuration can be found.
def message
"No default language configuration found!" \
" Please ensure that you have a 'default_language' defined in Alchemy configuration file."
" Please ensure that you have a 'default_language' defined in Alchemy configuration file."
end
end

class DefaultSiteNotFoundError < StandardError
# Raised if no default site configuration can be found.
def message
"No default site configuration found!" \
" Please ensure that you have a 'default_site' defined in Alchemy configuration file."
" Please ensure that you have a 'default_site' defined in Alchemy configuration file."
end
end

Expand Down Expand Up @@ -90,4 +90,10 @@ def message
"You need to provide a current_user method in your ApplicationController that returns the current authenticated user."
end
end

class UnsupportedPageVersion < StandardError
def message
"Unknown Version! Please use one of #{Alchemy::EagerLoading::PAGE_VERSIONS.join(", ")}"
end
end
end
6 changes: 3 additions & 3 deletions spec/jobs/alchemy/publish_page_job_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,20 @@

RSpec.describe Alchemy::PublishPageJob, type: :job do
describe "#perfom_later" do
let(:page) { build_stubbed(:alchemy_page) }
let(:page) { create(:alchemy_page) }
let(:public_on) { Time.current }

it "enqueues job" do
expect {
described_class.perform_later(page, public_on: public_on)
described_class.perform_later(page.id, public_on: public_on)
}.to have_enqueued_job
end

it "calls the page publisher" do
expect_any_instance_of(Alchemy::Page::Publisher).to receive(:publish!).with(
public_on: public_on,
)
described_class.new.perform(page, public_on: public_on)
described_class.new.perform(page.id, public_on: public_on)
end
end
end
61 changes: 61 additions & 0 deletions spec/models/alchemy/eager_loading_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# frozen_string_literal: true

require "rails_helper"

RSpec.describe Alchemy::EagerLoading do
describe ".page_includes" do
context "with no version param given" do
subject { described_class.page_includes }

it "returns public version includes" do
is_expected.to match_array([
:tags,
{
language: :site,
public_version: {
elements: [
:page,
:touchable_pages,
{
ingredients: :related_object,
contents: :essence,
},
],
},
},
])
end
end

context "with version param given" do
subject { described_class.page_includes(version: :draft_version) }

it "returns version includes" do
is_expected.to match_array([
:tags,
{
language: :site,
draft_version: {
elements: [
:page,
:touchable_pages,
{
ingredients: :related_object,
contents: :essence,
},
],
},
},
])
end
end

context "with unknown version param given" do
subject { described_class.page_includes(version: :foo_baz) }

it "returns version includes" do
expect { subject }.to raise_error(Alchemy::UnsupportedPageVersion)
end
end
end
end
31 changes: 15 additions & 16 deletions spec/requests/alchemy/admin/pages_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -643,45 +643,44 @@ module Alchemy

describe "#fold" do
let(:page) { create(:alchemy_page) }
let(:user) { create(:alchemy_dummy_user, :as_editor) }

before do
allow(Page).to receive(:find).and_return(page)
allow(page).to receive(:editable_by?).with(user).and_return(true)
allow_any_instance_of(described_class).to receive(:current_alchemy_user) { user }
end

context "if page is currently not folded" do
before { allow(page).to receive(:folded?).and_return(false) }
subject { patch fold_admin_page_path(page), xhr: true }

context "if page is currently not folded" do
it "should fold the page" do
expect(page).to receive(:fold!).with(user.id, true).and_return(true)
patch fold_admin_page_path(page), xhr: true
expect { subject }.to change { page.folded?(user.id) }.from(false).to(true)
end
end

context "if page is already folded" do
before { allow(page).to receive(:folded?).and_return(true) }
before do
page.fold!(user.id, true)
end

it "should unfold the page" do
expect(page).to receive(:fold!).with(user.id, false).and_return(true)
patch fold_admin_page_path(page), xhr: true
expect { subject }.to change { page.folded?(user.id) }.from(true).to(false)
end
end
end

describe "#unlock" do
subject { post unlock_admin_page_path(page), xhr: true }
let(:page) { create(:alchemy_page) }
let(:user) { create(:alchemy_dummy_user, :as_editor) }

let(:page) { create(:alchemy_page, name: "Best practices") }
subject { post unlock_admin_page_path(page), xhr: true }

before do
allow(Page).to receive(:find).with(page.id.to_s).and_return(page)
allow(page).to receive(:editable_by?).with(user).and_return(true)
allow(Page).to receive(:from_current_site).and_return(double(locked_by: nil))
expect(page).to receive(:unlock!) { true }
page.lock_to!(user)
allow_any_instance_of(described_class).to receive(:current_alchemy_user) { user }
end

it "should unlock the page" do
is_expected.to eq(200)
expect { subject }.to change { page.reload.locked? }.from(true).to(false)
end

context "requesting for html format" do
Expand Down