From e016f432e2f2496ecaff8933edc1cab9a0eecbca Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Mon, 18 Mar 2019 19:12:33 +0000 Subject: [PATCH 1/9] Install interactor gem This is a pattern we hope to use for storing the business logic of controllers. --- Gemfile | 1 + Gemfile.lock | 2 ++ 2 files changed, 3 insertions(+) diff --git a/Gemfile b/Gemfile index 96234100e0..72ded87d19 100644 --- a/Gemfile +++ b/Gemfile @@ -16,6 +16,7 @@ gem "govuk_publishing_components", "~> 16.10" gem "govuk_sidekiq", "~> 3" gem "hashdiff", "~> 0.3" gem "image_processing", "~> 1" +gem "interactor", "~> 3" gem "kaminari", "~> 1" gem "notifications-ruby-client", "~> 3.1" gem "pg", "~> 1" diff --git a/Gemfile.lock b/Gemfile.lock index 25ce858a49..2369257ee5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -188,6 +188,7 @@ GEM image_processing (1.9.0) mini_magick (>= 4.9.3, < 5) ruby-vips (>= 2.0.13, < 3) + interactor (3.1.1) io-like (0.3.0) jaro_winkler (1.5.2) jasmine (3.4.0) @@ -464,6 +465,7 @@ DEPENDENCIES govuk_test (~> 0.3) hashdiff (~> 0.3) image_processing (~> 1) + interactor (~> 3) jasmine (~> 3.4) jasmine_selenium_runner (~> 3) kaminari (~> 1) From 1aa79d1985dde5be910124b9ce74ab1b4ece4fac Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Tue, 19 Mar 2019 18:35:31 +0000 Subject: [PATCH 2/9] Refactor ImagesController using interactor pattern This demonstrates a demo of how the ImagesController could work using the interactor gem. There were some small changes I made here compared to the existing code: 1. We no longer preview on image creation. There isn't any point doing this as it won't succeed - as initial image is invalid 2. We round the number as part of cropping an image. This saves this frequently being off by 1 pixel, fixing this made tests easier. --- app/controllers/images_controller.rb | 172 ++++++-------------- app/interactors/images/create.rb | 40 +++++ app/interactors/images/destroy.rb | 34 ++++ app/interactors/images/update.rb | 82 ++++++++++ app/interactors/images/update_crop.rb | 50 ++++++ spec/interactors/images/create_spec.rb | 46 ++++++ spec/interactors/images/destroy_spec.rb | 58 +++++++ spec/interactors/images/update_crop_spec.rb | 87 ++++++++++ spec/interactors/images/update_spec.rb | 141 ++++++++++++++++ 9 files changed, 592 insertions(+), 118 deletions(-) create mode 100644 app/interactors/images/create.rb create mode 100644 app/interactors/images/destroy.rb create mode 100644 app/interactors/images/update.rb create mode 100644 app/interactors/images/update_crop.rb create mode 100644 spec/interactors/images/create_spec.rb create mode 100644 spec/interactors/images/destroy_spec.rb create mode 100644 spec/interactors/images/update_crop_spec.rb create mode 100644 spec/interactors/images/update_spec.rb diff --git a/app/controllers/images_controller.rb b/app/controllers/images_controller.rb index ba575188b1..765b36f783 100644 --- a/app/controllers/images_controller.rb +++ b/app/controllers/images_controller.rb @@ -7,28 +7,21 @@ def index end def create - Edition.find_and_lock_current(document: params[:document]) do |edition| - @issues = ::Requirements::ImageUploadChecker.new(params[:image]).issues - - if @issues.any? - flash.now["alert_with_items"] = { - "title" => I18n.t!("images.index.flashes.upload_requirements"), - "items" => @issues.items, - } - - render :index, - assigns: { edition: edition }, - layout: rendering_context, - status: :unprocessable_entity - next - end - - image_revision = ImageUploadService.new(params[:image], edition.revision).call(current_user) - updater = Versioning::RevisionUpdater.new(edition.revision, current_user) - updater.update_image(image_revision) - edition.assign_revision(updater.next_revision, current_user).save! - PreviewService.new(edition).try_create_preview - redirect_to crop_image_path(params[:document], image_revision.image_id, wizard: "upload") + result = Images::Create.call(params: params, user: current_user) + if result.failure? + flash.now["alert_with_items"] = { + "title" => I18n.t!("images.index.flashes.upload_requirements"), + "items" => result.issues.items, + } + + render :index, + assigns: { edition: result.edition }, + layout: rendering_context, + status: :unprocessable_entity + else + redirect_to crop_image_path(result.edition.document, + result.image_revision.image_id, + wizard: "upload") end end @@ -39,21 +32,10 @@ def crop end def update_crop - Edition.find_and_lock_current(document: params[:document]) do |edition| - image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) - image_updater = Versioning::ImageRevisionUpdater.new(image_revision, current_user) - image_updater.assign(update_crop_params) - - if image_updater.changed? - updater = Versioning::RevisionUpdater.new(edition.revision, current_user) - updater.update_image(image_updater.next_revision) - edition.assign_revision(updater.next_revision, current_user).save! - TimelineEntry.create_for_revision(entry_type: :image_updated, edition: edition) - PreviewService.new(edition).try_create_preview - end - - redirect_to edit_image_path(edition.document, image_revision.image_id, wizard: params[:wizard]) - end + result = Images::UpdateCrop.call(params: params, user: current_user) + redirect_to edit_image_path(result.edition.document, + result.image_revision.image_id, + wizard: params[:wizard]) end def edit @@ -63,80 +45,49 @@ def edit end def update - Edition.find_and_lock_current(document: params[:document]) do |edition| # rubocop:disable Metrics/BlockLength - image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) - image_updater = Versioning::ImageRevisionUpdater.new(image_revision, current_user) - - image_updater.assign(update_params) - next_image_revision = image_updater.next_revision - - issues = Requirements::ImageRevisionChecker.new(next_image_revision) - .pre_preview_metadata_issues - - if issues.any? - flash.now["alert_with_items"] = { - "title" => I18n.t!("images.edit.flashes.requirements"), - "items" => issues.items, - } - - render :edit, - assigns: { edition: edition, image_revision: next_image_revision, issues: issues }, - layout: rendering_context, - status: :unprocessable_entity - next - end - - updater = Versioning::RevisionUpdater.new(edition.revision, current_user) - updater.update_image(next_image_revision, params[:lead_image] == "on") - - if updater.changed? - timeline_entry_type = if updater.selected_lead_image? - :lead_image_selected - elsif updater.removed_lead_image? - :lead_image_removed - else - :image_updated - end - - TimelineEntry.create_for_revision(entry_type: timeline_entry_type, edition: edition) - edition.assign_revision(updater.next_revision, current_user).save! - PreviewService.new(edition).try_create_preview - end - - if updater.selected_lead_image? - redirect_to document_path(edition.document), + result = Images::Update.call(params: params, user: current_user) + + if result.failure? + flash.now["alert_with_items"] = { + "title" => I18n.t!("images.edit.flashes.requirements"), + "items" => result.issues.items, + } + + render :edit, + assigns: { edition: result.edition, + image_revision: result.image_revision, + issues: result.issues }, + layout: rendering_context, + status: :unprocessable_entity + else + document = result.edition.document + + if result.updater.selected_lead_image? + redirect_to document_path(document), notice: t("documents.show.flashes.lead_image.selected", - file: image_revision.filename) - elsif updater.removed_lead_image? - redirect_to images_path(edition.document), + file: result.image_revision.filename) + elsif result.updater.removed_lead_image? + redirect_to images_path(document), notice: t("images.index.flashes.lead_image.removed", - file: image_revision.filename) + file: result.image_revision.filename) else - redirect_to images_path(edition.document) + redirect_to images_path(document) end end end def destroy - Edition.find_and_lock_current(document: params[:document]) do |edition| - image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) - updater = Versioning::RevisionUpdater.new(edition.revision, current_user) - - updater.remove_image(image_revision) - edition.assign_revision(updater.next_revision, current_user).save! - - TimelineEntry.create_for_revision(entry_type: :image_deleted, edition: edition) - PreviewService.new(edition).try_create_preview - - if updater.removed_lead_image? - redirect_to images_path(edition.document), - notice: t("images.index.flashes.lead_image.deleted", - file: image_revision.filename) - else - redirect_to images_path(edition.document), - notice: t("images.index.flashes.deleted", - file: image_revision.filename) - end + result = Images::Destroy.call(params: params, user: current_user) + document = result.edition.document + + if result.updater.removed_lead_image? + redirect_to images_path(document), + notice: t("images.index.flashes.lead_image.deleted", + file: result.image_revision.filename) + else + redirect_to images_path(document), + notice: t("images.index.flashes.deleted", + file: result.image_revision.filename) end end @@ -151,19 +102,4 @@ def download type: image_revision.content_type, ) end - -private - - def update_params - params.require(:image_revision).permit(:caption, :alt_text, :credit) - end - - def update_crop_params - image_aspect_ratio = Image::HEIGHT.to_f / Image::WIDTH - - params - .require(:image_revision) - .permit(:crop_x, :crop_y, :crop_width, :crop_width) - .tap { |p| p[:crop_height] = (p[:crop_width].to_i * image_aspect_ratio).round } - end end diff --git a/app/interactors/images/create.rb b/app/interactors/images/create.rb new file mode 100644 index 0000000000..7d993cd897 --- /dev/null +++ b/app/interactors/images/create.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +class Images::Create + include Interactor + delegate :params, :user, to: :context + + def initialize(params:, user:) + super + end + + def call + Edition.find_and_lock_current(document: params[:document]) do |edition| + check_issues(edition, params[:image]) + image_revision = upload_image(edition, params[:image]) + update_edition(edition, image_revision) + update_context(edition: edition, image_revision: image_revision) + end + end + +private + + def check_issues(edition, image_params) + issues = Requirements::ImageUploadChecker.new(image_params).issues + context.fail!(edition: edition, issues: issues) if issues.any? + end + + def upload_image(edition, image_params) + ImageUploadService.new(image_params, edition.revision).call(user) + end + + def update_edition(edition, image_revision) + updater = Versioning::RevisionUpdater.new(edition.revision, user) + updater.update_image(image_revision, false) + edition.assign_revision(updater.next_revision, user).save! + end + + def update_context(attributes) + attributes.each { |k, v| context[k.to_sym] = v } + end +end diff --git a/app/interactors/images/destroy.rb b/app/interactors/images/destroy.rb new file mode 100644 index 0000000000..7addeca9f0 --- /dev/null +++ b/app/interactors/images/destroy.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class Images::Destroy + include Interactor + delegate :params, :user, to: :context + + def initialize(params:, user:) + super + end + + def call + Edition.find_and_lock_current(document: params[:document]) do |edition| + image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) + updater = Versioning::RevisionUpdater.new(edition.revision, user) + + updater.remove_image(image_revision) + edition.assign_revision(updater.next_revision, user).save! + + TimelineEntry.create_for_revision(entry_type: :image_deleted, + edition: edition) + PreviewService.new(edition).try_create_preview + + update_context(edition: edition, + image_revision: image_revision, + updater: updater) + end + end + +private + + def update_context(attributes) + attributes.each { |k, v| context[k.to_sym] = v } + end +end diff --git a/app/interactors/images/update.rb b/app/interactors/images/update.rb new file mode 100644 index 0000000000..025e7fbcef --- /dev/null +++ b/app/interactors/images/update.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +class Images::Update + include Interactor + delegate :params, :user, to: :context + + def initialize(params:, user:) + super + end + + def call + Edition.find_and_lock_current(document: params[:document]) do |edition| + image_revision = update_image_revision(edition, image_params) + check_issues(edition, image_revision) + + updater = Versioning::RevisionUpdater.new(edition.revision, user) + updater.update_image(image_revision, params[:lead_image] == "on") + + if updater.changed? + create_timeline_entry(edition, updater) + edition.assign_revision(updater.next_revision, user).save! + PreviewService.new(edition).try_create_preview + end + + update_context(edition: edition, + image_revision: image_revision, + updater: updater) + end + end + +private + + def update_image_revision(edition, update_params) + image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) + updater = Versioning::ImageRevisionUpdater.new(image_revision, user) + updater.assign(update_params) + updater.next_revision + end + + def image_params + params.require(:image_revision).permit(:caption, :alt_text, :credit) + end + + def check_issues(edition, image_revision) + checker = Requirements::ImageRevisionChecker.new(image_revision) + issues = checker.pre_preview_metadata_issues + if issues.any? + context.fail!(edition: edition, + image_revision: image_revision, + issues: issues) + end + end + + def upload_image(image_params) + upload_service = ImageUploadService.new(image_params, context.edition.revision) + context.image_revision = upload_service.call(context.user) + end + + def update_edition + updater = Versioning::RevisionUpdater.new(context.edition.revision, + context.user) + + updater.update_image(context.image_revision, false) + context.edition.assign_revision(updater.next_revision, context.user).save! + end + + def create_timeline_entry(edition, updater) + timeline_entry_type = if updater.selected_lead_image? + :lead_image_selected + elsif updater.removed_lead_image? + :lead_image_removed + else + :image_updated + end + + TimelineEntry.create_for_revision(entry_type: timeline_entry_type, edition: edition) + end + + def update_context(attributes) + attributes.each { |k, v| context[k.to_sym] = v } + end +end diff --git a/app/interactors/images/update_crop.rb b/app/interactors/images/update_crop.rb new file mode 100644 index 0000000000..d777c77cd1 --- /dev/null +++ b/app/interactors/images/update_crop.rb @@ -0,0 +1,50 @@ +# frozen_string_literal: true + +class Images::UpdateCrop + include Interactor + delegate :params, :user, to: :context + + def initialize(params:, user:) + super + end + + def call + Edition.find_and_lock_current(document: params[:document]) do |edition| + image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) + + updater = Versioning::ImageRevisionUpdater.new(image_revision, user) + updater.assign(crop_params) + + if updater.changed? + update_edition(edition, updater.next_revision) + TimelineEntry.create_for_revision(entry_type: :image_updated, + edition: edition) + PreviewService.new(edition).try_create_preview + end + + update_context(edition: edition, image_revision: updater.next_revision) + end + end + +private + + def crop_params + image_aspect_ratio = Image::HEIGHT.to_f / Image::WIDTH + + params + .require(:image_revision) + .permit(:crop_x, :crop_y, :crop_width, :crop_width) + .tap { |p| p[:crop_height] = (p[:crop_width].to_i * image_aspect_ratio).round + end + + def update_edition(edition, image_revision) + updater = Versioning::RevisionUpdater.new(edition.revision, user) + + updater.update_image(image_revision, false) + edition.assign_revision(updater.next_revision, user).save! + end + + def update_context(attributes) + attributes.each { |k, v| context[k.to_sym] = v } + end +end diff --git a/spec/interactors/images/create_spec.rb b/spec/interactors/images/create_spec.rb new file mode 100644 index 0000000000..ace3183953 --- /dev/null +++ b/spec/interactors/images/create_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +RSpec.describe Images::Create do + def strong_params(**params) + ActionController::Parameters.new(params) + end + + describe ".call" do + let(:edition) { create(:edition) } + let(:user) { create(:user) } + + context "when an image upload has issues" do + it "fails with issues" do + result = Images::Create.call( + params: strong_params( + document: edition.document.to_param, + image: nil, + ), + user: user, + ) + expect(result).to be_failure + expect(result.issues.any?).to be(true) + end + end + + context "when an image upload doesn't have issues" do + let(:params) do + strong_params( + document: edition.document.to_param, + image: fixture_file_upload("files/960x640.jpg", "image/jpeg"), + ) + end + + it "uploads an image" do + expect { Images::Create.call(params: params, user: user) } + .to change { Image.count } + .by(1) + end + + it "updates the edition revision" do + expect { Images::Create.call(params: params, user: user) } + .to(change { edition.reload.revision }) + end + end + end +end diff --git a/spec/interactors/images/destroy_spec.rb b/spec/interactors/images/destroy_spec.rb new file mode 100644 index 0000000000..e392ca42e0 --- /dev/null +++ b/spec/interactors/images/destroy_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +RSpec.describe Images::Destroy do + def strong_params(**params) + ActionController::Parameters.new(params) + end + + describe ".call" do + let(:user) { create(:user) } + let(:image_revision) { create(:image_revision, :on_asset_manager) } + let(:edition) { create(:edition, image_revisions: [image_revision]) } + let(:params) do + strong_params( + document: edition.document.to_param, + image_id: image_revision.image_id, + ) + end + + let(:preview_service) { double(try_create_preview: nil) } + + before do + allow(PreviewService) + .to receive(:new).with(edition).and_return(preview_service) + end + + it "creates a revision without the image revision" do + Images::Destroy.call(params: params, user: user) + revision = edition.reload.revision + expect(revision.image_revisions).not_to include(image_revision) + end + + it "creates a timeline entry" do + expect(TimelineEntry) + .to receive(:create_for_revision) + .with(entry_type: :image_deleted, edition: edition) + Images::Destroy.call(params: params, user: user) + end + + it "creates a preview" do + expect(preview_service).to receive(:try_create_preview) + Images::Destroy.call(params: params, user: user) + end + + context "when the image does not exist" do + let(:edition) { create(:edition) } + + it "raises an ActiveRecord::RecordNotFound error" do + params = strong_params( + document: edition.document.to_param, + image_id: Image.maximum(:id).to_i + 1, + ) + + expect { Images::Destroy.call(params: params, user: user) } + .to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end diff --git a/spec/interactors/images/update_crop_spec.rb b/spec/interactors/images/update_crop_spec.rb new file mode 100644 index 0000000000..d3b8fd8f7d --- /dev/null +++ b/spec/interactors/images/update_crop_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +RSpec.describe Images::UpdateCrop do + def strong_params(**params) + ActionController::Parameters.new(params) + end + + describe ".call" do + let(:user) { create(:user) } + let(:preview_service) { double(try_create_preview: nil) } + let(:image_revision) { create(:image_revision) } + let(:edition) { create(:edition, image_revisions: [image_revision]) } + + before do + allow(PreviewService) + .to receive(:new).with(edition).and_return(preview_service) + end + + context "when the crop has changed" do + let(:params) do + strong_params( + document: edition.document.to_param, + image_id: image_revision.image_id, + crop_x: image_revision.crop_x + 10, + crop_y: image_revision.crop_y + 10, + crop_width: image_revision.crop_width, + ) + end + + it "creates a new revision" do + expect { Images::UpdateCrop.call(params: params, user: user) } + .to(change { edition.reload.revision }) + end + + it "creates a timeline entry" do + expect(TimelineEntry) + .to receive(:create_for_revision) + .with(entry_type: :image_updated, edition: edition) + Images::UpdateCrop.call(params: params, user: user) + end + + it "creates a preview" do + expect(preview_service).to receive(:try_create_preview) + Images::UpdateCrop.call(params: params, user: user) + end + end + + context "when the crop hasn't changed" do + let(:params) do + strong_params( + document: edition.document.to_param, + image_id: image_revision.image_id, + crop_x: image_revision.crop_x, + crop_y: image_revision.crop_y, + crop_width: image_revision.crop_width, + ) + end + + it "doesn't create a new revision" do + expect { Images::UpdateCrop.call(params: params, user: user) } + .not_to(change { edition.reload.revision }) + end + + it "doesn't create a timeline entry" do + expect { Images::UpdateCrop.call(params: params, user: user) } + .not_to change(TimelineEntry, :count) + end + + it "creates a preview" do + expect(preview_service).not_to receive(:try_create_preview) + Images::UpdateCrop.call(params: params, user: user) + end + end + + context "when the image does not exist" do + it "raises an ActiveRecord::RecordNotFound error" do + params = strong_params( + document: edition.document.to_param, + image_id: Image.maximum(:id).to_i + 1, + ) + + expect { Images::UpdateCrop.call(params: params, user: user) } + .to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end diff --git a/spec/interactors/images/update_spec.rb b/spec/interactors/images/update_spec.rb new file mode 100644 index 0000000000..646596f3e6 --- /dev/null +++ b/spec/interactors/images/update_spec.rb @@ -0,0 +1,141 @@ +# frozen_string_literal: true + +RSpec.describe Images::Update do + def strong_params(**params) + ActionController::Parameters.new(params) + end + + describe ".call" do + let(:user) { create(:user) } + let(:image_revision) { create(:image_revision) } + let(:edition) { create(:edition, image_revisions: [image_revision]) } + let(:preview_service) { double(try_create_preview: nil) } + let(:success_params) do + { + document: edition.document.to_param, + image_id: image_revision.image_id, + image_revision: { alt_text: "alt text" }, + } + end + + before do + allow(PreviewService) + .to receive(:new).with(edition).and_return(preview_service) + end + + context "when an image is updated" do + it "creates a new revision" do + expect { Images::Update.call(params: strong_params(success_params), user: user) } + .to(change { edition.reload.revision }) + image_revision = edition.image_revisions.first + alt_text = success_params[:image_revision][:alt_text] + expect(image_revision.alt_text).to eq(alt_text) + end + + it "creates a preview" do + expect(preview_service).to receive(:try_create_preview) + Images::Update.call(params: strong_params(success_params), user: user) + end + end + + context "when an image is updated without changing lead image" do + it "creates a image_updated timeline entry" do + expect(TimelineEntry) + .to receive(:create_for_revision) + .with(entry_type: :image_updated, edition: edition) + Images::Update.call(params: strong_params(success_params), user: user) + end + end + + context "when a new lead image is selected" do + let(:params) do + strong_params(success_params.merge(lead_image: "on")) + end + + it "sets the edition lead image" do + expect { Images::Update.call(params: params, user: user) } + .to change { edition.reload.lead_image_revision } + .from(nil) + end + + it "creates a lead image selected timeline entry" do + expect(TimelineEntry) + .to receive(:create_for_revision) + .with(entry_type: :lead_image_selected, edition: edition) + Images::Update.call(params: params, user: user) + end + end + + context "when a lead image is removed" do + let(:edition) { create(:edition, lead_image_revision: image_revision) } + let(:params) do + strong_params(success_params.merge(lead_image: nil)) + end + + it "removes the edition lead image" do + expect { Images::Update.call(params: params, user: user) } + .to change { edition.reload.lead_image_revision } + .to(nil) + end + + it "creates a lead image removed timeline entry" do + expect(TimelineEntry) + .to receive(:create_for_revision) + .with(entry_type: :lead_image_removed, edition: edition) + Images::Update.call(params: params, user: user) + end + end + + context "when the image isn't changed" do + let(:image_revision) { create(:image_revision, alt_text: "existing") } + let(:params) do + strong_params(success_params.merge(image_revision: { alt_text: "existing" })) + end + + it "doesn't create a timeline entry" do + expect { Images::Update.call(params: params, user: user) } + .not_to change(TimelineEntry, :count) + end + + it "doesn't update the editions revision" do + expect { Images::Update.call(params: params, user: user) } + .not_to(change { edition.reload.revision }) + end + + it "doesn't preview" do + expect(preview_service).not_to receive(:try_create_preview) + Images::Update.call(params: params, user: user) + end + end + + context "when there are issues" do + it "fails with issues" do + result = Images::Update.call( + params: strong_params( + document: edition.document.to_param, + image_id: image_revision.image_id, + image_revision: { alt_text: "" }, + ), + user: user, + ) + expect(result).to be_failure + expect(result.issues.any?).to be(true) + end + end + + context "when the image does not exist" do + let(:edition) { create(:edition) } + + it "raises an ActiveRecord::RecordNotFound error" do + params = strong_params( + document: edition.document.to_param, + image_id: Image.maximum(:id).to_i + 1, + image_revision: { caption: "Caption" }, + ) + + expect { Images::Update.call(params: params, user: user) } + .to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end From b848d1f76640decf7579ee8ba2f0fe8d6fec7249 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Mon, 8 Apr 2019 09:54:21 +0100 Subject: [PATCH 3/9] Iterate interactors following feedback Following feedback on the first draft of interactors for images this makes a number of changes: - Changes the class naming to be suffixed with interactor. As these are mostly named with verbs this is a bit weird as it sounds like they create an interactor, however it is consistent with most other app level naming patterns. - Makes clearer the properties in play on a context by delegating each of them to context in the interactor class. - Drops using an update_context method to set context at the end of an interactor and instead uses an approach where context properties are set in the call method. - Run an interactor in an explicit transaction rather than using the `Edition.find_and_lock_current` method. Since we're setting context.edition we'd still have a line of code so we don't really benefit from the yield. - Uses a suggestion from Ben to define each instance variable we use from a result in a controller, by using values_at method on the result object (once converted to a hash) - Rather than checking on failure of a result check on a particular problem, such as issues. --- app/controllers/images_controller.rb | 79 ++++++++++-------- app/interactors/images/create.rb | 40 --------- app/interactors/images/create_interactor.rb | 43 ++++++++++ app/interactors/images/destroy.rb | 34 -------- app/interactors/images/destroy_interactor.rb | 45 ++++++++++ app/interactors/images/update.rb | 82 ------------------ app/interactors/images/update_crop.rb | 50 ----------- .../images/update_crop_interactor.rb | 65 +++++++++++++++ app/interactors/images/update_interactor.rb | 83 +++++++++++++++++++ ...eate_spec.rb => create_interactor_spec.rb} | 8 +- ...roy_spec.rb => destroy_interactor_spec.rb} | 10 +-- ...spec.rb => update_crop_interactor_spec.rb} | 32 +++---- ...date_spec.rb => update_interactor_spec.rb} | 26 +++--- 13 files changed, 319 insertions(+), 278 deletions(-) delete mode 100644 app/interactors/images/create.rb create mode 100644 app/interactors/images/create_interactor.rb delete mode 100644 app/interactors/images/destroy.rb create mode 100644 app/interactors/images/destroy_interactor.rb delete mode 100644 app/interactors/images/update.rb delete mode 100644 app/interactors/images/update_crop.rb create mode 100644 app/interactors/images/update_crop_interactor.rb create mode 100644 app/interactors/images/update_interactor.rb rename spec/interactors/images/{create_spec.rb => create_interactor_spec.rb} (80%) rename spec/interactors/images/{destroy_spec.rb => destroy_interactor_spec.rb} (82%) rename spec/interactors/images/{update_crop_spec.rb => update_crop_interactor_spec.rb} (67%) rename spec/interactors/images/{update_spec.rb => update_interactor_spec.rb} (80%) diff --git a/app/controllers/images_controller.rb b/app/controllers/images_controller.rb index 765b36f783..0b22db524a 100644 --- a/app/controllers/images_controller.rb +++ b/app/controllers/images_controller.rb @@ -7,20 +7,24 @@ def index end def create - result = Images::Create.call(params: params, user: current_user) - if result.failure? + result = Images::CreateInteractor.call(params: params, user: current_user) + edition, image_revision, issues = result.to_h.values_at(:edition, + :image_revision, + :issues) + + if issues flash.now["alert_with_items"] = { "title" => I18n.t!("images.index.flashes.upload_requirements"), - "items" => result.issues.items, + "items" => issues.items, } render :index, - assigns: { edition: result.edition }, + assigns: { edition: edition }, layout: rendering_context, status: :unprocessable_entity else - redirect_to crop_image_path(result.edition.document, - result.image_revision.image_id, + redirect_to crop_image_path(edition.document, + image_revision.image_id, wizard: "upload") end end @@ -32,9 +36,10 @@ def crop end def update_crop - result = Images::UpdateCrop.call(params: params, user: current_user) - redirect_to edit_image_path(result.edition.document, - result.image_revision.image_id, + result = Images::UpdateCropInteractor.call(params: params, user: current_user) + edition, image_revision = result.to_h.values_at(:edition, :image_revision) + redirect_to edit_image_path(edition.document, + image_revision.image_id, wizard: params[:wizard]) end @@ -45,49 +50,51 @@ def edit end def update - result = Images::Update.call(params: params, user: current_user) + result = Images::UpdateInteractor.call(params: params, user: current_user) + + edition, image_revision, issues, lead_selected, lead_removed = + result.to_h.values_at(:edition, + :image_revision, + :issues, + :selected_lead_image, + :removed_lead_image) - if result.failure? + if issues flash.now["alert_with_items"] = { "title" => I18n.t!("images.edit.flashes.requirements"), - "items" => result.issues.items, + "items" => issues.items, } render :edit, - assigns: { edition: result.edition, - image_revision: result.image_revision, - issues: result.issues }, + assigns: { edition: edition, + image_revision: image_revision, + issues: issues }, layout: rendering_context, status: :unprocessable_entity + elsif lead_selected + redirect_to document_path(edition.document), + notice: t("documents.show.flashes.lead_image.selected", file: image_revision.filename) + elsif lead_removed + redirect_to images_path(edition.document), + notice: t("images.index.flashes.lead_image.removed", file: image_revision.filename) else - document = result.edition.document - - if result.updater.selected_lead_image? - redirect_to document_path(document), - notice: t("documents.show.flashes.lead_image.selected", - file: result.image_revision.filename) - elsif result.updater.removed_lead_image? - redirect_to images_path(document), - notice: t("images.index.flashes.lead_image.removed", - file: result.image_revision.filename) - else - redirect_to images_path(document) - end + redirect_to images_path(edition.document) end end def destroy - result = Images::Destroy.call(params: params, user: current_user) - document = result.edition.document - - if result.updater.removed_lead_image? - redirect_to images_path(document), + result = Images::DestroyInteractor.call(params: params, user: current_user) + edition, image_revision, removed_lead = result.to_h.values_at(:edition, + :image_revision, + :removed_lead_image) + if removed_lead + redirect_to images_path(edition.document), notice: t("images.index.flashes.lead_image.deleted", - file: result.image_revision.filename) + file: image_revision.filename) else - redirect_to images_path(document), + redirect_to images_path(edition.document), notice: t("images.index.flashes.deleted", - file: result.image_revision.filename) + file: image_revision.filename) end end diff --git a/app/interactors/images/create.rb b/app/interactors/images/create.rb deleted file mode 100644 index 7d993cd897..0000000000 --- a/app/interactors/images/create.rb +++ /dev/null @@ -1,40 +0,0 @@ -# frozen_string_literal: true - -class Images::Create - include Interactor - delegate :params, :user, to: :context - - def initialize(params:, user:) - super - end - - def call - Edition.find_and_lock_current(document: params[:document]) do |edition| - check_issues(edition, params[:image]) - image_revision = upload_image(edition, params[:image]) - update_edition(edition, image_revision) - update_context(edition: edition, image_revision: image_revision) - end - end - -private - - def check_issues(edition, image_params) - issues = Requirements::ImageUploadChecker.new(image_params).issues - context.fail!(edition: edition, issues: issues) if issues.any? - end - - def upload_image(edition, image_params) - ImageUploadService.new(image_params, edition.revision).call(user) - end - - def update_edition(edition, image_revision) - updater = Versioning::RevisionUpdater.new(edition.revision, user) - updater.update_image(image_revision, false) - edition.assign_revision(updater.next_revision, user).save! - end - - def update_context(attributes) - attributes.each { |k, v| context[k.to_sym] = v } - end -end diff --git a/app/interactors/images/create_interactor.rb b/app/interactors/images/create_interactor.rb new file mode 100644 index 0000000000..219de566cc --- /dev/null +++ b/app/interactors/images/create_interactor.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +class Images::CreateInteractor + include Interactor + delegate :params, + :user, + :edition, + :image_revision, + :issues, + to: :context + + def initialize(params:, user:) + super + end + + def call + Edition.transaction do + context.edition = Edition.lock.find_current(document: params[:document]) + check_for_issues(params[:image]) + + context.image_revision = upload_image(params[:image]) + update_edition + end + end + +private + + def check_for_issues(image_params) + issues = Requirements::ImageUploadChecker.new(image_params).issues + context.fail!(issues: issues) if issues.any? + end + + def upload_image(image_params) + ImageUploadService.new(image_params, edition.revision).call(user) + end + + def update_edition + Versioning::RevisionUpdater.new(edition.revision, user).tap do |updater| + updater.update_image(image_revision, false) + edition.assign_revision(updater.next_revision, user).save! + end + end +end diff --git a/app/interactors/images/destroy.rb b/app/interactors/images/destroy.rb deleted file mode 100644 index 7addeca9f0..0000000000 --- a/app/interactors/images/destroy.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true - -class Images::Destroy - include Interactor - delegate :params, :user, to: :context - - def initialize(params:, user:) - super - end - - def call - Edition.find_and_lock_current(document: params[:document]) do |edition| - image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) - updater = Versioning::RevisionUpdater.new(edition.revision, user) - - updater.remove_image(image_revision) - edition.assign_revision(updater.next_revision, user).save! - - TimelineEntry.create_for_revision(entry_type: :image_deleted, - edition: edition) - PreviewService.new(edition).try_create_preview - - update_context(edition: edition, - image_revision: image_revision, - updater: updater) - end - end - -private - - def update_context(attributes) - attributes.each { |k, v| context[k.to_sym] = v } - end -end diff --git a/app/interactors/images/destroy_interactor.rb b/app/interactors/images/destroy_interactor.rb new file mode 100644 index 0000000000..9de2e03c90 --- /dev/null +++ b/app/interactors/images/destroy_interactor.rb @@ -0,0 +1,45 @@ +# frozen_string_literal: true + +class Images::DestroyInteractor + include Interactor + delegate :params, + :user, + :edition, + :image_revision, + :removed_lead_image, + to: :context + + def initialize(params:, user:) + super + end + + def call + Edition.transaction do + context.edition = Edition.lock.find_current(document: params[:document]) + context.image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) + + updater = remove_image(image_revision) + create_timeline_entry + update_preview + + context.removed_lead_image = updater.removed_lead_image? + end + end + +private + + def remove_image(image_revision) + Versioning::RevisionUpdater.new(edition.revision, user).tap do |updater| + updater.remove_image(image_revision) + edition.assign_revision(updater.next_revision, user).save! + end + end + + def create_timeline_entry + TimelineEntry.create_for_revision(entry_type: :image_deleted, edition: edition) + end + + def update_preview + PreviewService.new(edition).try_create_preview + end +end diff --git a/app/interactors/images/update.rb b/app/interactors/images/update.rb deleted file mode 100644 index 025e7fbcef..0000000000 --- a/app/interactors/images/update.rb +++ /dev/null @@ -1,82 +0,0 @@ -# frozen_string_literal: true - -class Images::Update - include Interactor - delegate :params, :user, to: :context - - def initialize(params:, user:) - super - end - - def call - Edition.find_and_lock_current(document: params[:document]) do |edition| - image_revision = update_image_revision(edition, image_params) - check_issues(edition, image_revision) - - updater = Versioning::RevisionUpdater.new(edition.revision, user) - updater.update_image(image_revision, params[:lead_image] == "on") - - if updater.changed? - create_timeline_entry(edition, updater) - edition.assign_revision(updater.next_revision, user).save! - PreviewService.new(edition).try_create_preview - end - - update_context(edition: edition, - image_revision: image_revision, - updater: updater) - end - end - -private - - def update_image_revision(edition, update_params) - image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) - updater = Versioning::ImageRevisionUpdater.new(image_revision, user) - updater.assign(update_params) - updater.next_revision - end - - def image_params - params.require(:image_revision).permit(:caption, :alt_text, :credit) - end - - def check_issues(edition, image_revision) - checker = Requirements::ImageRevisionChecker.new(image_revision) - issues = checker.pre_preview_metadata_issues - if issues.any? - context.fail!(edition: edition, - image_revision: image_revision, - issues: issues) - end - end - - def upload_image(image_params) - upload_service = ImageUploadService.new(image_params, context.edition.revision) - context.image_revision = upload_service.call(context.user) - end - - def update_edition - updater = Versioning::RevisionUpdater.new(context.edition.revision, - context.user) - - updater.update_image(context.image_revision, false) - context.edition.assign_revision(updater.next_revision, context.user).save! - end - - def create_timeline_entry(edition, updater) - timeline_entry_type = if updater.selected_lead_image? - :lead_image_selected - elsif updater.removed_lead_image? - :lead_image_removed - else - :image_updated - end - - TimelineEntry.create_for_revision(entry_type: timeline_entry_type, edition: edition) - end - - def update_context(attributes) - attributes.each { |k, v| context[k.to_sym] = v } - end -end diff --git a/app/interactors/images/update_crop.rb b/app/interactors/images/update_crop.rb deleted file mode 100644 index d777c77cd1..0000000000 --- a/app/interactors/images/update_crop.rb +++ /dev/null @@ -1,50 +0,0 @@ -# frozen_string_literal: true - -class Images::UpdateCrop - include Interactor - delegate :params, :user, to: :context - - def initialize(params:, user:) - super - end - - def call - Edition.find_and_lock_current(document: params[:document]) do |edition| - image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) - - updater = Versioning::ImageRevisionUpdater.new(image_revision, user) - updater.assign(crop_params) - - if updater.changed? - update_edition(edition, updater.next_revision) - TimelineEntry.create_for_revision(entry_type: :image_updated, - edition: edition) - PreviewService.new(edition).try_create_preview - end - - update_context(edition: edition, image_revision: updater.next_revision) - end - end - -private - - def crop_params - image_aspect_ratio = Image::HEIGHT.to_f / Image::WIDTH - - params - .require(:image_revision) - .permit(:crop_x, :crop_y, :crop_width, :crop_width) - .tap { |p| p[:crop_height] = (p[:crop_width].to_i * image_aspect_ratio).round - end - - def update_edition(edition, image_revision) - updater = Versioning::RevisionUpdater.new(edition.revision, user) - - updater.update_image(image_revision, false) - edition.assign_revision(updater.next_revision, user).save! - end - - def update_context(attributes) - attributes.each { |k, v| context[k.to_sym] = v } - end -end diff --git a/app/interactors/images/update_crop_interactor.rb b/app/interactors/images/update_crop_interactor.rb new file mode 100644 index 0000000000..056cade261 --- /dev/null +++ b/app/interactors/images/update_crop_interactor.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +class Images::UpdateCropInteractor + include Interactor + delegate :params, + :user, + :edition, + :image_revision, + to: :context + + def initialize(params:, user:) + super + end + + def call + Edition.transaction do + context.edition = Edition.lock.find_current(document: params[:document]) + + updater = update_image(find_image_revision(params[:image_id]), crop_params) + + if updater.changed? + create_timeline_entry + update_preview + end + + context.image_revision = updater.next_revision + end + end + +private + + def find_image_revision(image_id) + edition.image_revisions.find_by!(image_id: image_id) + end + + def crop_params + image_aspect_ratio = Image::HEIGHT.to_f / Image::WIDTH + + params + .require(:image_revision) + .permit(:crop_x, :crop_y, :crop_width, :crop_width) + .tap { |p| p[:crop_height] = (p[:crop_width].to_i * image_aspect_ratio).round } + end + + def update_image(image_revision, params) + Versioning::ImageRevisionUpdater.new(image_revision, user).tap do |updater| + updater.assign(params) + update_edition(updater.next_revision) if updater.changed? + end + end + + def update_edition(image_revision) + updater = Versioning::RevisionUpdater.new(edition.revision, user) + updater.update_image(image_revision, false) + edition.assign_revision(updater.next_revision, user).save! + end + + def create_timeline_entry + TimelineEntry.create_for_revision(entry_type: :image_updated, edition: edition) + end + + def update_preview + PreviewService.new(edition).try_create_preview + end +end diff --git a/app/interactors/images/update_interactor.rb b/app/interactors/images/update_interactor.rb new file mode 100644 index 0000000000..335f321d9a --- /dev/null +++ b/app/interactors/images/update_interactor.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +class Images::UpdateInteractor + include Interactor + delegate :params, + :user, + :edition, + :image_revision, + :issues, + :selected_lead_image, + :removed_lead_image, + to: :context + + def initialize(params:, user:) + super + end + + def call + Edition.transaction do + context.edition = Edition.lock.find_current(document: params[:document]) + context.image_revision = update_image_revision(image_params) + + check_for_issues + + updater = update_image(params[:lead_image] == "on") + + context.selected_lead_image = updater.selected_lead_image? + context.removed_lead_image = updater.removed_lead_image? + + if updater.changed? + create_timeline_entry + update_preview + end + end + end + +private + + def update_image_revision(update_params) + image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) + updater = Versioning::ImageRevisionUpdater.new(image_revision, user) + updater.assign(update_params) + updater.next_revision + end + + def image_params + params.require(:image_revision).permit(:caption, :alt_text, :credit) + end + + def check_for_issues + checker = Requirements::ImageRevisionChecker.new(image_revision) + issues = checker.pre_preview_metadata_issues + context.fail!(issues: issues) if issues.any? + end + + def update_image(lead_image) + Versioning::RevisionUpdater.new(edition.revision, user).tap do |updater| + updater.update_image(image_revision, lead_image) + edition.assign_revision(updater.next_revision, user).save! if updater.changed? + end + end + + def upload_image(image_params) + upload_service = ImageUploadService.new(image_params, context.edition.revision) + context.image_revision = upload_service.call(context.user) + end + + def create_timeline_entry + timeline_entry_type = if selected_lead_image + :lead_image_selected + elsif removed_lead_image + :lead_image_removed + else + :image_updated + end + + TimelineEntry.create_for_revision(entry_type: timeline_entry_type, edition: edition) + end + + def update_preview + PreviewService.new(edition).try_create_preview + end +end diff --git a/spec/interactors/images/create_spec.rb b/spec/interactors/images/create_interactor_spec.rb similarity index 80% rename from spec/interactors/images/create_spec.rb rename to spec/interactors/images/create_interactor_spec.rb index ace3183953..68332e7abe 100644 --- a/spec/interactors/images/create_spec.rb +++ b/spec/interactors/images/create_interactor_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.describe Images::Create do +RSpec.describe Images::CreateInteractor do def strong_params(**params) ActionController::Parameters.new(params) end @@ -11,7 +11,7 @@ def strong_params(**params) context "when an image upload has issues" do it "fails with issues" do - result = Images::Create.call( + result = Images::CreateInteractor.call( params: strong_params( document: edition.document.to_param, image: nil, @@ -32,13 +32,13 @@ def strong_params(**params) end it "uploads an image" do - expect { Images::Create.call(params: params, user: user) } + expect { Images::CreateInteractor.call(params: params, user: user) } .to change { Image.count } .by(1) end it "updates the edition revision" do - expect { Images::Create.call(params: params, user: user) } + expect { Images::CreateInteractor.call(params: params, user: user) } .to(change { edition.reload.revision }) end end diff --git a/spec/interactors/images/destroy_spec.rb b/spec/interactors/images/destroy_interactor_spec.rb similarity index 82% rename from spec/interactors/images/destroy_spec.rb rename to spec/interactors/images/destroy_interactor_spec.rb index e392ca42e0..bdd4f3ed98 100644 --- a/spec/interactors/images/destroy_spec.rb +++ b/spec/interactors/images/destroy_interactor_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.describe Images::Destroy do +RSpec.describe Images::DestroyInteractor do def strong_params(**params) ActionController::Parameters.new(params) end @@ -24,7 +24,7 @@ def strong_params(**params) end it "creates a revision without the image revision" do - Images::Destroy.call(params: params, user: user) + Images::DestroyInteractor.call(params: params, user: user) revision = edition.reload.revision expect(revision.image_revisions).not_to include(image_revision) end @@ -33,12 +33,12 @@ def strong_params(**params) expect(TimelineEntry) .to receive(:create_for_revision) .with(entry_type: :image_deleted, edition: edition) - Images::Destroy.call(params: params, user: user) + Images::DestroyInteractor.call(params: params, user: user) end it "creates a preview" do expect(preview_service).to receive(:try_create_preview) - Images::Destroy.call(params: params, user: user) + Images::DestroyInteractor.call(params: params, user: user) end context "when the image does not exist" do @@ -50,7 +50,7 @@ def strong_params(**params) image_id: Image.maximum(:id).to_i + 1, ) - expect { Images::Destroy.call(params: params, user: user) } + expect { Images::DestroyInteractor.call(params: params, user: user) } .to raise_error(ActiveRecord::RecordNotFound) end end diff --git a/spec/interactors/images/update_crop_spec.rb b/spec/interactors/images/update_crop_interactor_spec.rb similarity index 67% rename from spec/interactors/images/update_crop_spec.rb rename to spec/interactors/images/update_crop_interactor_spec.rb index d3b8fd8f7d..7a41ce8ac9 100644 --- a/spec/interactors/images/update_crop_spec.rb +++ b/spec/interactors/images/update_crop_interactor_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.describe Images::UpdateCrop do +RSpec.describe Images::UpdateCropInteractor do def strong_params(**params) ActionController::Parameters.new(params) end @@ -21,14 +21,16 @@ def strong_params(**params) strong_params( document: edition.document.to_param, image_id: image_revision.image_id, - crop_x: image_revision.crop_x + 10, - crop_y: image_revision.crop_y + 10, - crop_width: image_revision.crop_width, + image_revision: { + crop_x: image_revision.crop_x + 10, + crop_y: image_revision.crop_y + 10, + crop_width: image_revision.crop_width, + }, ) end it "creates a new revision" do - expect { Images::UpdateCrop.call(params: params, user: user) } + expect { Images::UpdateCropInteractor.call(params: params, user: user) } .to(change { edition.reload.revision }) end @@ -36,12 +38,12 @@ def strong_params(**params) expect(TimelineEntry) .to receive(:create_for_revision) .with(entry_type: :image_updated, edition: edition) - Images::UpdateCrop.call(params: params, user: user) + Images::UpdateCropInteractor.call(params: params, user: user) end it "creates a preview" do expect(preview_service).to receive(:try_create_preview) - Images::UpdateCrop.call(params: params, user: user) + Images::UpdateCropInteractor.call(params: params, user: user) end end @@ -50,25 +52,27 @@ def strong_params(**params) strong_params( document: edition.document.to_param, image_id: image_revision.image_id, - crop_x: image_revision.crop_x, - crop_y: image_revision.crop_y, - crop_width: image_revision.crop_width, + image_revision: { + crop_x: image_revision.crop_x, + crop_y: image_revision.crop_y, + crop_width: image_revision.crop_width, + }, ) end it "doesn't create a new revision" do - expect { Images::UpdateCrop.call(params: params, user: user) } + expect { Images::UpdateCropInteractor.call(params: params, user: user) } .not_to(change { edition.reload.revision }) end it "doesn't create a timeline entry" do - expect { Images::UpdateCrop.call(params: params, user: user) } + expect { Images::UpdateCropInteractor.call(params: params, user: user) } .not_to change(TimelineEntry, :count) end it "creates a preview" do expect(preview_service).not_to receive(:try_create_preview) - Images::UpdateCrop.call(params: params, user: user) + Images::UpdateCropInteractor.call(params: params, user: user) end end @@ -79,7 +83,7 @@ def strong_params(**params) image_id: Image.maximum(:id).to_i + 1, ) - expect { Images::UpdateCrop.call(params: params, user: user) } + expect { Images::UpdateCropInteractor.call(params: params, user: user) } .to raise_error(ActiveRecord::RecordNotFound) end end diff --git a/spec/interactors/images/update_spec.rb b/spec/interactors/images/update_interactor_spec.rb similarity index 80% rename from spec/interactors/images/update_spec.rb rename to spec/interactors/images/update_interactor_spec.rb index 646596f3e6..ff55b36c09 100644 --- a/spec/interactors/images/update_spec.rb +++ b/spec/interactors/images/update_interactor_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -RSpec.describe Images::Update do +RSpec.describe Images::UpdateInteractor do def strong_params(**params) ActionController::Parameters.new(params) end @@ -25,7 +25,7 @@ def strong_params(**params) context "when an image is updated" do it "creates a new revision" do - expect { Images::Update.call(params: strong_params(success_params), user: user) } + expect { Images::UpdateInteractor.call(params: strong_params(success_params), user: user) } .to(change { edition.reload.revision }) image_revision = edition.image_revisions.first alt_text = success_params[:image_revision][:alt_text] @@ -34,7 +34,7 @@ def strong_params(**params) it "creates a preview" do expect(preview_service).to receive(:try_create_preview) - Images::Update.call(params: strong_params(success_params), user: user) + Images::UpdateInteractor.call(params: strong_params(success_params), user: user) end end @@ -43,7 +43,7 @@ def strong_params(**params) expect(TimelineEntry) .to receive(:create_for_revision) .with(entry_type: :image_updated, edition: edition) - Images::Update.call(params: strong_params(success_params), user: user) + Images::UpdateInteractor.call(params: strong_params(success_params), user: user) end end @@ -53,7 +53,7 @@ def strong_params(**params) end it "sets the edition lead image" do - expect { Images::Update.call(params: params, user: user) } + expect { Images::UpdateInteractor.call(params: params, user: user) } .to change { edition.reload.lead_image_revision } .from(nil) end @@ -62,7 +62,7 @@ def strong_params(**params) expect(TimelineEntry) .to receive(:create_for_revision) .with(entry_type: :lead_image_selected, edition: edition) - Images::Update.call(params: params, user: user) + Images::UpdateInteractor.call(params: params, user: user) end end @@ -73,7 +73,7 @@ def strong_params(**params) end it "removes the edition lead image" do - expect { Images::Update.call(params: params, user: user) } + expect { Images::UpdateInteractor.call(params: params, user: user) } .to change { edition.reload.lead_image_revision } .to(nil) end @@ -82,7 +82,7 @@ def strong_params(**params) expect(TimelineEntry) .to receive(:create_for_revision) .with(entry_type: :lead_image_removed, edition: edition) - Images::Update.call(params: params, user: user) + Images::UpdateInteractor.call(params: params, user: user) end end @@ -93,24 +93,24 @@ def strong_params(**params) end it "doesn't create a timeline entry" do - expect { Images::Update.call(params: params, user: user) } + expect { Images::UpdateInteractor.call(params: params, user: user) } .not_to change(TimelineEntry, :count) end it "doesn't update the editions revision" do - expect { Images::Update.call(params: params, user: user) } + expect { Images::UpdateInteractor.call(params: params, user: user) } .not_to(change { edition.reload.revision }) end it "doesn't preview" do expect(preview_service).not_to receive(:try_create_preview) - Images::Update.call(params: params, user: user) + Images::UpdateInteractor.call(params: params, user: user) end end context "when there are issues" do it "fails with issues" do - result = Images::Update.call( + result = Images::UpdateInteractor.call( params: strong_params( document: edition.document.to_param, image_id: image_revision.image_id, @@ -133,7 +133,7 @@ def strong_params(**params) image_revision: { caption: "Caption" }, ) - expect { Images::Update.call(params: params, user: user) } + expect { Images::UpdateInteractor.call(params: params, user: user) } .to raise_error(ActiveRecord::RecordNotFound) end end From fe020edfa2425652ab169ff2b9d5d86dc779766e Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Tue, 9 Apr 2019 16:38:17 +0100 Subject: [PATCH 4/9] Make interactors more step focused Following a workshop with Ben we've been able to agree on an approach to them that is quite focused on declarative steps. This increases the readability of the call method, but does mean context is set as a side effect of other methods. Hopefully that isn't too confusing. We've kept the delegates to context, they serve the purpose of reducing context verbosity in the class and defining the available properties that can be used when operating on the result. We dropped the initialize with params and user. These were used as a means to formalise the class interface but this is quite repetitive. --- app/controllers/images_controller.rb | 21 ++++--- app/interactors/images/create_interactor.rb | 31 +++++----- app/interactors/images/destroy_interactor.rb | 29 +++++---- .../images/update_crop_interactor.rb | 49 +++++++-------- app/interactors/images/update_interactor.rb | 59 +++++++++---------- 5 files changed, 94 insertions(+), 95 deletions(-) diff --git a/app/controllers/images_controller.rb b/app/controllers/images_controller.rb index 0b22db524a..eda9d6bde0 100644 --- a/app/controllers/images_controller.rb +++ b/app/controllers/images_controller.rb @@ -23,7 +23,7 @@ def create layout: rendering_context, status: :unprocessable_entity else - redirect_to crop_image_path(edition.document, + redirect_to crop_image_path(params[:document], image_revision.image_id, wizard: "upload") end @@ -37,8 +37,8 @@ def crop def update_crop result = Images::UpdateCropInteractor.call(params: params, user: current_user) - edition, image_revision = result.to_h.values_at(:edition, :image_revision) - redirect_to edit_image_path(edition.document, + image_revision = result.image_revision + redirect_to edit_image_path(params[:document], image_revision.image_id, wizard: params[:wizard]) end @@ -72,27 +72,26 @@ def update layout: rendering_context, status: :unprocessable_entity elsif lead_selected - redirect_to document_path(edition.document), + redirect_to document_path(params[:document]), notice: t("documents.show.flashes.lead_image.selected", file: image_revision.filename) elsif lead_removed - redirect_to images_path(edition.document), + redirect_to images_path(params[:document]), notice: t("images.index.flashes.lead_image.removed", file: image_revision.filename) else - redirect_to images_path(edition.document) + redirect_to images_path(params[:document]) end end def destroy result = Images::DestroyInteractor.call(params: params, user: current_user) - edition, image_revision, removed_lead = result.to_h.values_at(:edition, - :image_revision, - :removed_lead_image) + image_revision, removed_lead = result.to_h.values_at(:image_revision, :removed_lead_image) + if removed_lead - redirect_to images_path(edition.document), + redirect_to images_path(params[:document]), notice: t("images.index.flashes.lead_image.deleted", file: image_revision.filename) else - redirect_to images_path(edition.document), + redirect_to images_path(params[:document]), notice: t("images.index.flashes.deleted", file: image_revision.filename) end diff --git a/app/interactors/images/create_interactor.rb b/app/interactors/images/create_interactor.rb index 219de566cc..d3419f3627 100644 --- a/app/interactors/images/create_interactor.rb +++ b/app/interactors/images/create_interactor.rb @@ -9,35 +9,34 @@ class Images::CreateInteractor :issues, to: :context - def initialize(params:, user:) - super - end - def call Edition.transaction do - context.edition = Edition.lock.find_current(document: params[:document]) - check_for_issues(params[:image]) - - context.image_revision = upload_image(params[:image]) + find_and_lock_edition + check_for_issues + create_image_revision update_edition end end private - def check_for_issues(image_params) - issues = Requirements::ImageUploadChecker.new(image_params).issues + def find_and_lock_edition + context.edition = Edition.lock.find_current(document: params[:document]) + end + + def check_for_issues + issues = Requirements::ImageUploadChecker.new(params[:image]).issues context.fail!(issues: issues) if issues.any? end - def upload_image(image_params) - ImageUploadService.new(image_params, edition.revision).call(user) + def create_image_revision + context.image_revision = ImageUploadService.new(params[:image], edition.revision) + .call(user) end def update_edition - Versioning::RevisionUpdater.new(edition.revision, user).tap do |updater| - updater.update_image(image_revision, false) - edition.assign_revision(updater.next_revision, user).save! - end + updater = Versioning::RevisionUpdater.new(edition.revision, user) + updater.update_image(image_revision, false) + edition.assign_revision(updater.next_revision, user).save! end end diff --git a/app/interactors/images/destroy_interactor.rb b/app/interactors/images/destroy_interactor.rb index 9de2e03c90..6c3e4601f4 100644 --- a/app/interactors/images/destroy_interactor.rb +++ b/app/interactors/images/destroy_interactor.rb @@ -9,30 +9,29 @@ class Images::DestroyInteractor :removed_lead_image, to: :context - def initialize(params:, user:) - super - end - def call Edition.transaction do - context.edition = Edition.lock.find_current(document: params[:document]) - context.image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) - - updater = remove_image(image_revision) + find_and_lock_edition + find_and_remove_image create_timeline_entry update_preview - - context.removed_lead_image = updater.removed_lead_image? end end private - def remove_image(image_revision) - Versioning::RevisionUpdater.new(edition.revision, user).tap do |updater| - updater.remove_image(image_revision) - edition.assign_revision(updater.next_revision, user).save! - end + def find_and_lock_edition + context.edition = Edition.lock.find_current(document: params[:document]) + end + + def find_and_remove_image + image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) + updater = Versioning::RevisionUpdater.new(edition.revision, user) + updater.remove_image(image_revision) + edition.assign_revision(updater.next_revision, user).save! + + context.image_revision = image_revision + context.removed_lead_image = updater.removed_lead_image? end def create_timeline_entry diff --git a/app/interactors/images/update_crop_interactor.rb b/app/interactors/images/update_crop_interactor.rb index 056cade261..5d0a8a4899 100644 --- a/app/interactors/images/update_crop_interactor.rb +++ b/app/interactors/images/update_crop_interactor.rb @@ -6,31 +6,34 @@ class Images::UpdateCropInteractor :user, :edition, :image_revision, + :edition_updated, to: :context - def initialize(params:, user:) - super - end - def call Edition.transaction do - context.edition = Edition.lock.find_current(document: params[:document]) + find_and_lock_edition + find_and_update_image - updater = update_image(find_image_revision(params[:image_id]), crop_params) + update_edition - if updater.changed? - create_timeline_entry - update_preview - end - - context.image_revision = updater.next_revision + create_timeline_entry + update_preview end end private - def find_image_revision(image_id) - edition.image_revisions.find_by!(image_id: image_id) + def find_and_lock_edition + context.edition = Edition.lock.find_current(document: params[:document]) + end + + def find_and_update_image + image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) + + updater = Versioning::ImageRevisionUpdater.new(image_revision, user) + updater.assign(crop_params) + + context.image_revision = updater.next_revision end def crop_params @@ -42,24 +45,24 @@ def crop_params .tap { |p| p[:crop_height] = (p[:crop_width].to_i * image_aspect_ratio).round } end - def update_image(image_revision, params) - Versioning::ImageRevisionUpdater.new(image_revision, user).tap do |updater| - updater.assign(params) - update_edition(updater.next_revision) if updater.changed? - end - end - - def update_edition(image_revision) + def update_edition updater = Versioning::RevisionUpdater.new(edition.revision, user) + updater.update_image(image_revision, false) - edition.assign_revision(updater.next_revision, user).save! + edition.assign_revision(updater.next_revision, user).save! if updater.changed? + + context.edition_updated = updater.changed? end def create_timeline_entry + return unless edition_updated + TimelineEntry.create_for_revision(entry_type: :image_updated, edition: edition) end def update_preview + return unless edition_updated + PreviewService.new(edition).try_create_preview end end diff --git a/app/interactors/images/update_interactor.rb b/app/interactors/images/update_interactor.rb index 335f321d9a..0d35de10ac 100644 --- a/app/interactors/images/update_interactor.rb +++ b/app/interactors/images/update_interactor.rb @@ -7,65 +7,62 @@ class Images::UpdateInteractor :edition, :image_revision, :issues, + :edition_updated, :selected_lead_image, :removed_lead_image, to: :context - def initialize(params:, user:) - super - end - def call Edition.transaction do - context.edition = Edition.lock.find_current(document: params[:document]) - context.image_revision = update_image_revision(image_params) + find_and_lock_edition + find_and_update_image check_for_issues + update_edition - updater = update_image(params[:lead_image] == "on") - - context.selected_lead_image = updater.selected_lead_image? - context.removed_lead_image = updater.removed_lead_image? - - if updater.changed? - create_timeline_entry - update_preview - end + create_timeline_entry + update_preview end end private - def update_image_revision(update_params) + def find_and_lock_edition + context.edition = Edition.lock.find_current(document: params[:document]) + end + + def find_and_update_image image_revision = edition.image_revisions.find_by!(image_id: params[:image_id]) + image_params = params.require(:image_revision).permit(:caption, :alt_text, :credit) + updater = Versioning::ImageRevisionUpdater.new(image_revision, user) - updater.assign(update_params) - updater.next_revision - end + updater.assign(image_params) - def image_params - params.require(:image_revision).permit(:caption, :alt_text, :credit) + context.image_revision = updater.next_revision end def check_for_issues checker = Requirements::ImageRevisionChecker.new(image_revision) issues = checker.pre_preview_metadata_issues + context.fail!(issues: issues) if issues.any? end - def update_image(lead_image) - Versioning::RevisionUpdater.new(edition.revision, user).tap do |updater| - updater.update_image(image_revision, lead_image) - edition.assign_revision(updater.next_revision, user).save! if updater.changed? - end - end + def update_edition + is_lead_image = params[:lead_image] == "on" + updater = Versioning::RevisionUpdater.new(edition.revision, user) - def upload_image(image_params) - upload_service = ImageUploadService.new(image_params, context.edition.revision) - context.image_revision = upload_service.call(context.user) + updater.update_image(image_revision, is_lead_image) + edition.assign_revision(updater.next_revision, user).save! if updater.changed? + + context.edition_updated = updater.changed? + context.selected_lead_image = updater.selected_lead_image? + context.removed_lead_image = updater.removed_lead_image? end def create_timeline_entry + return unless edition_updated + timeline_entry_type = if selected_lead_image :lead_image_selected elsif removed_lead_image @@ -78,6 +75,8 @@ def create_timeline_entry end def update_preview + return unless edition_updated + PreviewService.new(edition).try_create_preview end end From e932c6d79088fd8968aac2ea6cc2cc90e807d336 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Fri, 12 Apr 2019 19:21:03 +0100 Subject: [PATCH 5/9] Contacts interactors --- app/controllers/contacts_controller.rb | 30 +-------- app/interactors/contacts/insert_interactor.rb | 64 +++++++++++++++++++ 2 files changed, 66 insertions(+), 28 deletions(-) create mode 100644 app/interactors/contacts/insert_interactor.rb diff --git a/app/controllers/contacts_controller.rb b/app/controllers/contacts_controller.rb index 79904394cf..fc5da000b8 100644 --- a/app/controllers/contacts_controller.rb +++ b/app/controllers/contacts_controller.rb @@ -10,34 +10,8 @@ def search end def insert - Edition.find_and_lock_current(document: params[:document]) do |edition| - redirect_location = edit_document_path(edition.document) + "#body" + Contacts::InsertInteractor.call(params: params, user: current_user) - if params[:contact_id].empty? - redirect_to redirect_location - next - end - - contact_markdown = "[Contact:#{params[:contact_id]}]\n" - revision = edition.revision - - body = revision.contents.fetch("body", "").chomp - updated_body = if body.present? - "#{body}\n\n#{contact_markdown}" - else - contact_markdown - end - - updater = Versioning::RevisionUpdater.new(revision, current_user) - updater.assign(contents: revision.contents.merge("body" => updated_body)) - - if updater.changed? - edition.assign_revision(updater.next_revision, current_user).save! - TimelineEntry.create_for_revision(entry_type: :updated_content, edition: edition) - PreviewService.new(edition).try_create_preview - end - - redirect_to redirect_location - end + redirect_to edit_document_path(params[:document], anchor: "body") end end diff --git a/app/interactors/contacts/insert_interactor.rb b/app/interactors/contacts/insert_interactor.rb new file mode 100644 index 0000000000..e15c7874d0 --- /dev/null +++ b/app/interactors/contacts/insert_interactor.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +class Contacts::InsertInteractor + include Interactor + delegate :params, + :user, + :edition, + :empty_submission, + :edition_updated, + to: :context + + def call + Edition.transaction do + find_and_lock_edition + check_for_submission + + update_edition + + create_timeline_entry + update_preview + end + end + +private + + def find_and_lock_edition + context.edition = Edition.lock.find_current(document: params[:document]) + end + + def check_for_submission + context.fail!(empty_submission: true) if params[:contact_id].empty? + end + + def update_edition + contact_markdown = "[Contact:#{params[:contact_id]}]\n" + revision = edition.revision + + body = revision.contents.fetch("body", "").chomp + updated_body = if body.present? + "#{body}\n\n#{contact_markdown}" + else + contact_markdown + end + + updater = Versioning::RevisionUpdater.new(revision, user) + updater.assign(contents: revision.contents.merge("body" => updated_body)) + + edition.assign_revision(updater.next_revision, user).save! if updater.changed? + context.edition_updated = updater.changed? + end + + def create_timeline_entry + return unless edition_updated + + TimelineEntry.create_for_revision(entry_type: :updated_content, edition: edition) + end + + def update_preview + return unless edition_updated + + PreviewService.new(edition).try_create_preview + end +end + From f083573e373bb6d37b44700752193d6536f940cc Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Fri, 12 Apr 2019 20:10:52 +0100 Subject: [PATCH 6/9] Documents interactors --- app/controllers/documents_controller.rb | 78 ++++++------------- .../documents/destroy_interactor.rb | 32 ++++++++ .../documents/update_interactor.rb | 70 +++++++++++++++++ 3 files changed, 124 insertions(+), 56 deletions(-) create mode 100644 app/interactors/documents/destroy_interactor.rb create mode 100644 app/interactors/documents/update_interactor.rb diff --git a/app/controllers/documents_controller.rb b/app/controllers/documents_controller.rb index 4ee98b8a2f..ec90871989 100644 --- a/app/controllers/documents_controller.rb +++ b/app/controllers/documents_controller.rb @@ -28,53 +28,31 @@ def confirm_delete_draft end def destroy - Edition.find_and_lock_current(document: params[:document]) do |edition| - begin - DeleteDraftService.new(edition.document, current_user).delete - - TimelineEntry.create_for_status_change(entry_type: :draft_discarded, - status: edition.status) - - redirect_to documents_path - rescue GdsApi::BaseError => e - GovukError.notify(e) - redirect_to edition.document, alert_with_description: t("documents.show.flashes.delete_draft_error") - end - end + Documents::DestroyInteractor.call(params: params, user: current_user) + redirect_to documents_path + rescue GdsApi::BaseError => e + GovukError.notify(e) + redirect_to document_path(params[:document]), + alert_with_description: t("documents.show.flashes.delete_draft_error") end def update - Edition.find_and_lock_current(document: params[:document]) do |edition| - updater = Versioning::RevisionUpdater.new(edition.revision, current_user) - updater.assign(update_params(edition.document)) - - add_contact_request = params[:submit] == "add_contact" - @issues = Requirements::EditPageChecker.new(edition, updater.next_revision) - .pre_preview_issues - - if @issues.any? - flash.now["alert_with_items"] = { - "title" => I18n.t!("documents.edit.flashes.requirements"), - "items" => @issues.items, - } - - render :edit, - assigns: { edition: edition, revision: updater.next_revision }, - status: :unprocessable_entity - next - end - - if updater.changed? - edition.assign_revision(updater.next_revision, current_user).save! - TimelineEntry.create_for_revision(entry_type: :updated_content, edition: edition) - PreviewService.new(edition).try_create_preview - end - - if add_contact_request - redirect_to search_contacts_path(edition.document) - else - redirect_to edition.document - end + result = Documents::UpdateInteractor.call(params: params, user: current_user) + edition, revision, issues, = result.to_h.values_at(:edition, :revision, :issues) + + if issues + flash.now["alert_with_items"] = { + "title" => I18n.t!("documents.edit.flashes.requirements"), + "items" => issues.items, + } + + render :edit, + assigns: { edition: edition, revision: revision }, + status: :unprocessable_entity + elsif params[:submit] == "add_contact" + redirect_to search_contacts_path(edition.document) + else + redirect_to edition.document end end @@ -94,16 +72,4 @@ def filter_params per_page: 50, } end - - def update_params(document) - contents_params = document.document_type.contents.map(&:id) - - params.require(:revision) - .permit(:update_type, :change_note, :title, :summary, contents: contents_params) - .tap do |p| - p[:title] = p[:title]&.strip - p[:summary] = p[:summary]&.strip - p[:base_path] = PathGeneratorService.new.path(document, p[:title]) - end - end end diff --git a/app/interactors/documents/destroy_interactor.rb b/app/interactors/documents/destroy_interactor.rb new file mode 100644 index 0000000000..1468ac2790 --- /dev/null +++ b/app/interactors/documents/destroy_interactor.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class Documents::DestroyInteractor + include Interactor + delegate :params, + :user, + :edition, + to: :context + + def call + Edition.transaction do + find_and_lock_edition + discard_draft + create_timeline_entry + end + end + +private + + def find_and_lock_edition + context.edition = Edition.lock.find_current(document: params[:document]) + end + + def discard_draft + DeleteDraftService.new(edition.document, user).delete + end + + def create_timeline_entry + TimelineEntry.create_for_status_change(entry_type: :draft_discarded, + status: edition.status) + end +end diff --git a/app/interactors/documents/update_interactor.rb b/app/interactors/documents/update_interactor.rb new file mode 100644 index 0000000000..5cdf68a58b --- /dev/null +++ b/app/interactors/documents/update_interactor.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +class Documents::UpdateInteractor + include Interactor + delegate :params, + :user, + :edition, + :revision, + :changed, + to: :context + + def call + Edition.transaction do + find_and_lock_edition + update_revision + check_for_issues + + update_edition + create_timeline_entry + update_preview + end + end + +private + + def find_and_lock_edition + context.edition = Edition.lock.find_current(document: params[:document]) + end + + def update_revision + updater = Versioning::RevisionUpdater.new(edition.revision, user) + updater.assign(update_params(edition)) + + context.changed = updater.changed? + context.revision = updater.next_revision + end + + def check_for_issues + issues = Requirements::EditPageChecker.new(edition, revision).pre_preview_issues + context.fail!(issues: issues) if issues.any? + end + + def update_edition + edition.assign_revision(revision, user).save! if changed + end + + def create_timeline_entry + return unless changed + + TimelineEntry.create_for_revision(entry_type: :updated_content, edition: edition) + end + + def update_preview + return unless changed + + PreviewService.new(edition).try_create_preview + end + + def update_params(edition) + contents_params = edition.document_type.contents.map(&:id) + + params.require(:revision) + .permit(:update_type, :change_note, :title, :summary, contents: contents_params) + .tap do |p| + p[:title] = p[:title]&.strip + p[:summary] = p[:summary]&.strip + p[:base_path] = PathGeneratorService.new.path(edition.document, p[:title]) + end + end +end From 64e2bc04b7dd1d9b890f08ab1abc1c5d559d1a04 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Fri, 12 Apr 2019 20:30:00 +0100 Subject: [PATCH 7/9] Editions interactors --- app/controllers/editions_controller.rb | 36 +++-------- app/interactors/editions/create_interactor.rb | 60 +++++++++++++++++++ 2 files changed, 67 insertions(+), 29 deletions(-) create mode 100644 app/interactors/editions/create_interactor.rb diff --git a/app/controllers/editions_controller.rb b/app/controllers/editions_controller.rb index 27e0f457a3..2c570c04e4 100644 --- a/app/controllers/editions_controller.rb +++ b/app/controllers/editions_controller.rb @@ -2,35 +2,13 @@ class EditionsController < ApplicationController def create - Edition.find_and_lock_current(document: params[:document]) do |edition| - unless edition.live? - # FIXME: this shouldn't be an exception but we've not worked out the - # right response - maybe bad request or a redirect with flash? - raise "Can't create a new edition when the current edition is a draft" - end - - edition.update!(current: false) - - next_edition = Edition.find_by( - document: edition.document, - number: edition.number + 1, - ) - - if next_edition - next_edition.resume_discarded(edition, current_user) - - TimelineEntry.create_for_status_change(entry_type: :draft_reset, - status: next_edition.status) - else - next_edition = Edition.create_next_edition(edition, current_user) - - TimelineEntry.create_for_status_change(entry_type: :new_edition, - status: next_edition.status) - end - - PreviewService.new(next_edition).try_create_preview - - redirect_to edit_document_path(next_edition.document) + result = Editions::CreateInteractor.call(params: params, user: current_user) + if result.draft_current_edition + # FIXME: this shouldn't be an exception but we've not worked out the + # right response - maybe bad request or a redirect with flash? + raise "Can't create a new edition when the current edition is a draft" + else + redirect_to edit_document_path(params[:document]) end end end diff --git a/app/interactors/editions/create_interactor.rb b/app/interactors/editions/create_interactor.rb new file mode 100644 index 0000000000..f526f023df --- /dev/null +++ b/app/interactors/editions/create_interactor.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +class Editions::CreateInteractor + include Interactor + delegate :params, + :user, + :live_edition, + :next_edition, + :draft_current_edition, + :resume_discarded, + to: :context + + def call + Edition.transaction do + find_and_lock_live_edition + create_next_edition + create_timeline_entry + update_preview + end + end + +private + + def find_and_lock_live_edition + edition = Edition.lock.find_current(document: params[:document]) + context.fail!(draft_current_edition: true) unless edition.live? + + context.live_edition = edition + end + + def create_next_edition + live_edition.update!(current: false) + + context.resume_discarded = Edition.find_by( + document: live_edition.document, + number: live_edition.number + 1, + ) + + context.next_edition = if resume_discarded + discarded_edition.resume_discarded(live_edition, user) + else + Edition.create_next_edition(live_edition, user) + end + end + + def create_timeline_entry + if resume_discarded + TimelineEntry.create_for_status_change(entry_type: :draft_reset, + status: next_edition.status) + else + TimelineEntry.create_for_status_change(entry_type: :new_edition, + status: next_edition.status) + end + end + + def update_preview + PreviewService.new(live_edition).try_create_preview + end +end + From 5faf0bf8d14a5483b1a69e966c8f07dd69fb0b94 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Mon, 15 Apr 2019 09:45:15 +0100 Subject: [PATCH 8/9] Remove interactor tests for now These aren't currently adding any further value than the feature tests we already have. --- .../images/create_interactor_spec.rb | 46 ------ .../images/destroy_interactor_spec.rb | 58 ------- .../images/update_crop_interactor_spec.rb | 91 ----------- .../images/update_interactor_spec.rb | 141 ------------------ 4 files changed, 336 deletions(-) delete mode 100644 spec/interactors/images/create_interactor_spec.rb delete mode 100644 spec/interactors/images/destroy_interactor_spec.rb delete mode 100644 spec/interactors/images/update_crop_interactor_spec.rb delete mode 100644 spec/interactors/images/update_interactor_spec.rb diff --git a/spec/interactors/images/create_interactor_spec.rb b/spec/interactors/images/create_interactor_spec.rb deleted file mode 100644 index 68332e7abe..0000000000 --- a/spec/interactors/images/create_interactor_spec.rb +++ /dev/null @@ -1,46 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe Images::CreateInteractor do - def strong_params(**params) - ActionController::Parameters.new(params) - end - - describe ".call" do - let(:edition) { create(:edition) } - let(:user) { create(:user) } - - context "when an image upload has issues" do - it "fails with issues" do - result = Images::CreateInteractor.call( - params: strong_params( - document: edition.document.to_param, - image: nil, - ), - user: user, - ) - expect(result).to be_failure - expect(result.issues.any?).to be(true) - end - end - - context "when an image upload doesn't have issues" do - let(:params) do - strong_params( - document: edition.document.to_param, - image: fixture_file_upload("files/960x640.jpg", "image/jpeg"), - ) - end - - it "uploads an image" do - expect { Images::CreateInteractor.call(params: params, user: user) } - .to change { Image.count } - .by(1) - end - - it "updates the edition revision" do - expect { Images::CreateInteractor.call(params: params, user: user) } - .to(change { edition.reload.revision }) - end - end - end -end diff --git a/spec/interactors/images/destroy_interactor_spec.rb b/spec/interactors/images/destroy_interactor_spec.rb deleted file mode 100644 index bdd4f3ed98..0000000000 --- a/spec/interactors/images/destroy_interactor_spec.rb +++ /dev/null @@ -1,58 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe Images::DestroyInteractor do - def strong_params(**params) - ActionController::Parameters.new(params) - end - - describe ".call" do - let(:user) { create(:user) } - let(:image_revision) { create(:image_revision, :on_asset_manager) } - let(:edition) { create(:edition, image_revisions: [image_revision]) } - let(:params) do - strong_params( - document: edition.document.to_param, - image_id: image_revision.image_id, - ) - end - - let(:preview_service) { double(try_create_preview: nil) } - - before do - allow(PreviewService) - .to receive(:new).with(edition).and_return(preview_service) - end - - it "creates a revision without the image revision" do - Images::DestroyInteractor.call(params: params, user: user) - revision = edition.reload.revision - expect(revision.image_revisions).not_to include(image_revision) - end - - it "creates a timeline entry" do - expect(TimelineEntry) - .to receive(:create_for_revision) - .with(entry_type: :image_deleted, edition: edition) - Images::DestroyInteractor.call(params: params, user: user) - end - - it "creates a preview" do - expect(preview_service).to receive(:try_create_preview) - Images::DestroyInteractor.call(params: params, user: user) - end - - context "when the image does not exist" do - let(:edition) { create(:edition) } - - it "raises an ActiveRecord::RecordNotFound error" do - params = strong_params( - document: edition.document.to_param, - image_id: Image.maximum(:id).to_i + 1, - ) - - expect { Images::DestroyInteractor.call(params: params, user: user) } - .to raise_error(ActiveRecord::RecordNotFound) - end - end - end -end diff --git a/spec/interactors/images/update_crop_interactor_spec.rb b/spec/interactors/images/update_crop_interactor_spec.rb deleted file mode 100644 index 7a41ce8ac9..0000000000 --- a/spec/interactors/images/update_crop_interactor_spec.rb +++ /dev/null @@ -1,91 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe Images::UpdateCropInteractor do - def strong_params(**params) - ActionController::Parameters.new(params) - end - - describe ".call" do - let(:user) { create(:user) } - let(:preview_service) { double(try_create_preview: nil) } - let(:image_revision) { create(:image_revision) } - let(:edition) { create(:edition, image_revisions: [image_revision]) } - - before do - allow(PreviewService) - .to receive(:new).with(edition).and_return(preview_service) - end - - context "when the crop has changed" do - let(:params) do - strong_params( - document: edition.document.to_param, - image_id: image_revision.image_id, - image_revision: { - crop_x: image_revision.crop_x + 10, - crop_y: image_revision.crop_y + 10, - crop_width: image_revision.crop_width, - }, - ) - end - - it "creates a new revision" do - expect { Images::UpdateCropInteractor.call(params: params, user: user) } - .to(change { edition.reload.revision }) - end - - it "creates a timeline entry" do - expect(TimelineEntry) - .to receive(:create_for_revision) - .with(entry_type: :image_updated, edition: edition) - Images::UpdateCropInteractor.call(params: params, user: user) - end - - it "creates a preview" do - expect(preview_service).to receive(:try_create_preview) - Images::UpdateCropInteractor.call(params: params, user: user) - end - end - - context "when the crop hasn't changed" do - let(:params) do - strong_params( - document: edition.document.to_param, - image_id: image_revision.image_id, - image_revision: { - crop_x: image_revision.crop_x, - crop_y: image_revision.crop_y, - crop_width: image_revision.crop_width, - }, - ) - end - - it "doesn't create a new revision" do - expect { Images::UpdateCropInteractor.call(params: params, user: user) } - .not_to(change { edition.reload.revision }) - end - - it "doesn't create a timeline entry" do - expect { Images::UpdateCropInteractor.call(params: params, user: user) } - .not_to change(TimelineEntry, :count) - end - - it "creates a preview" do - expect(preview_service).not_to receive(:try_create_preview) - Images::UpdateCropInteractor.call(params: params, user: user) - end - end - - context "when the image does not exist" do - it "raises an ActiveRecord::RecordNotFound error" do - params = strong_params( - document: edition.document.to_param, - image_id: Image.maximum(:id).to_i + 1, - ) - - expect { Images::UpdateCropInteractor.call(params: params, user: user) } - .to raise_error(ActiveRecord::RecordNotFound) - end - end - end -end diff --git a/spec/interactors/images/update_interactor_spec.rb b/spec/interactors/images/update_interactor_spec.rb deleted file mode 100644 index ff55b36c09..0000000000 --- a/spec/interactors/images/update_interactor_spec.rb +++ /dev/null @@ -1,141 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe Images::UpdateInteractor do - def strong_params(**params) - ActionController::Parameters.new(params) - end - - describe ".call" do - let(:user) { create(:user) } - let(:image_revision) { create(:image_revision) } - let(:edition) { create(:edition, image_revisions: [image_revision]) } - let(:preview_service) { double(try_create_preview: nil) } - let(:success_params) do - { - document: edition.document.to_param, - image_id: image_revision.image_id, - image_revision: { alt_text: "alt text" }, - } - end - - before do - allow(PreviewService) - .to receive(:new).with(edition).and_return(preview_service) - end - - context "when an image is updated" do - it "creates a new revision" do - expect { Images::UpdateInteractor.call(params: strong_params(success_params), user: user) } - .to(change { edition.reload.revision }) - image_revision = edition.image_revisions.first - alt_text = success_params[:image_revision][:alt_text] - expect(image_revision.alt_text).to eq(alt_text) - end - - it "creates a preview" do - expect(preview_service).to receive(:try_create_preview) - Images::UpdateInteractor.call(params: strong_params(success_params), user: user) - end - end - - context "when an image is updated without changing lead image" do - it "creates a image_updated timeline entry" do - expect(TimelineEntry) - .to receive(:create_for_revision) - .with(entry_type: :image_updated, edition: edition) - Images::UpdateInteractor.call(params: strong_params(success_params), user: user) - end - end - - context "when a new lead image is selected" do - let(:params) do - strong_params(success_params.merge(lead_image: "on")) - end - - it "sets the edition lead image" do - expect { Images::UpdateInteractor.call(params: params, user: user) } - .to change { edition.reload.lead_image_revision } - .from(nil) - end - - it "creates a lead image selected timeline entry" do - expect(TimelineEntry) - .to receive(:create_for_revision) - .with(entry_type: :lead_image_selected, edition: edition) - Images::UpdateInteractor.call(params: params, user: user) - end - end - - context "when a lead image is removed" do - let(:edition) { create(:edition, lead_image_revision: image_revision) } - let(:params) do - strong_params(success_params.merge(lead_image: nil)) - end - - it "removes the edition lead image" do - expect { Images::UpdateInteractor.call(params: params, user: user) } - .to change { edition.reload.lead_image_revision } - .to(nil) - end - - it "creates a lead image removed timeline entry" do - expect(TimelineEntry) - .to receive(:create_for_revision) - .with(entry_type: :lead_image_removed, edition: edition) - Images::UpdateInteractor.call(params: params, user: user) - end - end - - context "when the image isn't changed" do - let(:image_revision) { create(:image_revision, alt_text: "existing") } - let(:params) do - strong_params(success_params.merge(image_revision: { alt_text: "existing" })) - end - - it "doesn't create a timeline entry" do - expect { Images::UpdateInteractor.call(params: params, user: user) } - .not_to change(TimelineEntry, :count) - end - - it "doesn't update the editions revision" do - expect { Images::UpdateInteractor.call(params: params, user: user) } - .not_to(change { edition.reload.revision }) - end - - it "doesn't preview" do - expect(preview_service).not_to receive(:try_create_preview) - Images::UpdateInteractor.call(params: params, user: user) - end - end - - context "when there are issues" do - it "fails with issues" do - result = Images::UpdateInteractor.call( - params: strong_params( - document: edition.document.to_param, - image_id: image_revision.image_id, - image_revision: { alt_text: "" }, - ), - user: user, - ) - expect(result).to be_failure - expect(result.issues.any?).to be(true) - end - end - - context "when the image does not exist" do - let(:edition) { create(:edition) } - - it "raises an ActiveRecord::RecordNotFound error" do - params = strong_params( - document: edition.document.to_param, - image_id: Image.maximum(:id).to_i + 1, - image_revision: { caption: "Caption" }, - ) - - expect { Images::UpdateInteractor.call(params: params, user: user) } - .to raise_error(ActiveRecord::RecordNotFound) - end - end - end -end From 75f33acd646a0a856acb5136c705f6ba823091bb Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Mon, 15 Apr 2019 09:58:41 +0100 Subject: [PATCH 9/9] Use consistent updated boolean in interactors This is an attempt to bring more consistency to the interactors we have by using the same boolean of `updated` in each of them rather than a variety of `edition_updated` and `changed`. `edition_updated` doesn't make sense for all scenarios - namely that it may be set before the edition is updated. `updated` is rather generic and should be versatile to use consistently to represent that an interactor has caused a mutation. I am a bit worried it might be too generic so I'm in two minds about it. --- app/interactors/contacts/insert_interactor.rb | 8 ++++---- app/interactors/documents/update_interactor.rb | 4 ++-- app/interactors/images/update_crop_interactor.rb | 8 ++++---- app/interactors/images/update_interactor.rb | 8 ++++---- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/app/interactors/contacts/insert_interactor.rb b/app/interactors/contacts/insert_interactor.rb index e15c7874d0..3c3ae0be1c 100644 --- a/app/interactors/contacts/insert_interactor.rb +++ b/app/interactors/contacts/insert_interactor.rb @@ -6,7 +6,7 @@ class Contacts::InsertInteractor :user, :edition, :empty_submission, - :edition_updated, + :updated, to: :context def call @@ -46,17 +46,17 @@ def update_edition updater.assign(contents: revision.contents.merge("body" => updated_body)) edition.assign_revision(updater.next_revision, user).save! if updater.changed? - context.edition_updated = updater.changed? + context.updated = updater.changed? end def create_timeline_entry - return unless edition_updated + return unless updated TimelineEntry.create_for_revision(entry_type: :updated_content, edition: edition) end def update_preview - return unless edition_updated + return unless updated PreviewService.new(edition).try_create_preview end diff --git a/app/interactors/documents/update_interactor.rb b/app/interactors/documents/update_interactor.rb index 5cdf68a58b..f5e25f0d9a 100644 --- a/app/interactors/documents/update_interactor.rb +++ b/app/interactors/documents/update_interactor.rb @@ -6,7 +6,7 @@ class Documents::UpdateInteractor :user, :edition, :revision, - :changed, + :updated, to: :context def call @@ -31,7 +31,7 @@ def update_revision updater = Versioning::RevisionUpdater.new(edition.revision, user) updater.assign(update_params(edition)) - context.changed = updater.changed? + context.updated = updater.changed? context.revision = updater.next_revision end diff --git a/app/interactors/images/update_crop_interactor.rb b/app/interactors/images/update_crop_interactor.rb index 5d0a8a4899..5a4bb2cc0b 100644 --- a/app/interactors/images/update_crop_interactor.rb +++ b/app/interactors/images/update_crop_interactor.rb @@ -6,7 +6,7 @@ class Images::UpdateCropInteractor :user, :edition, :image_revision, - :edition_updated, + :updated, to: :context def call @@ -51,17 +51,17 @@ def update_edition updater.update_image(image_revision, false) edition.assign_revision(updater.next_revision, user).save! if updater.changed? - context.edition_updated = updater.changed? + context.updated = updater.changed? end def create_timeline_entry - return unless edition_updated + return unless updated TimelineEntry.create_for_revision(entry_type: :image_updated, edition: edition) end def update_preview - return unless edition_updated + return unless updated PreviewService.new(edition).try_create_preview end diff --git a/app/interactors/images/update_interactor.rb b/app/interactors/images/update_interactor.rb index 0d35de10ac..a128c2b0c4 100644 --- a/app/interactors/images/update_interactor.rb +++ b/app/interactors/images/update_interactor.rb @@ -7,7 +7,7 @@ class Images::UpdateInteractor :edition, :image_revision, :issues, - :edition_updated, + :updated, :selected_lead_image, :removed_lead_image, to: :context @@ -55,13 +55,13 @@ def update_edition updater.update_image(image_revision, is_lead_image) edition.assign_revision(updater.next_revision, user).save! if updater.changed? - context.edition_updated = updater.changed? + context.updated = updater.changed? context.selected_lead_image = updater.selected_lead_image? context.removed_lead_image = updater.removed_lead_image? end def create_timeline_entry - return unless edition_updated + return unless updated timeline_entry_type = if selected_lead_image :lead_image_selected @@ -75,7 +75,7 @@ def create_timeline_entry end def update_preview - return unless edition_updated + return unless updated PreviewService.new(edition).try_create_preview end