Skip to content

Commit

Permalink
Move code to delete pages to pages controller
Browse files Browse the repository at this point in the history
Refactor the forms delete confirmation controller to deal with forms
only, and move the code for deleting pages to the pages controller; it
is more idiomatic Rails to have the destroy and delete actions for a
model in the controller for that model.
  • Loading branch information
lfdebrux committed Dec 9, 2024
1 parent dc07edb commit caba6b4
Show file tree
Hide file tree
Showing 7 changed files with 140 additions and 88 deletions.
32 changes: 5 additions & 27 deletions app/controllers/forms/delete_confirmation_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,7 @@ def destroy

if @delete_confirmation_input.valid?
if @delete_confirmation_input.confirmed?
if params[:page_id].present?
delete_page(current_form, @page)
else
delete_form(current_form)
end
delete_form(current_form)
else
redirect_to @back_url
end
Expand Down Expand Up @@ -52,18 +48,10 @@ def previous_page(id)
def load_page_variables
@delete_confirmation_input = DeleteConfirmationInput.new

if params[:page_id].present?
@page = PageRepository.find(page_id: params[:page_id], form_id: current_form.id)
@url = destroy_page_path(current_form, @page.id)
@confirm_deletion_legend = t("forms_delete_confirmation_input.confirm_deletion_page")
@item_name = @page.question_text
@back_url = edit_question_path(current_form, @page.id)
else
@url = destroy_form_path(current_form)
@confirm_deletion_legend = t("forms_delete_confirmation_input.confirm_deletion")
@item_name = current_form.name
@back_url = form_path(current_form)
end
@url = destroy_form_path(current_form)
@confirm_deletion_legend = t("forms_delete_confirmation_input.confirm_deletion")
@item_name = current_form.name
@back_url = form_path(current_form)
end

def delete_form(form)
Expand All @@ -75,15 +63,5 @@ def delete_form(form)
raise StandardError, "Deletion unsuccessful"
end
end

def delete_page(form, page)
success_url = form_pages_path(form)

if PageRepository.destroy(page)
redirect_to success_url, status: :see_other, success: "Successfully deleted ‘#{page.question_text}’"
else
raise StandardError, "Deletion unsuccessful"
end
end
end
end
44 changes: 44 additions & 0 deletions app/controllers/pages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,40 @@ def index
render :index, locals: { current_form: }
end

def delete
@page = PageRepository.find(page_id: params[:page_id], form_id: current_form.id)
@url = destroy_page_path(current_form, @page.id)
@confirm_deletion_legend = t("forms_delete_confirmation_input.confirm_deletion_page")
@item_name = @page.question_text
@back_url = edit_question_path(current_form, @page.id)

@delete_confirmation_input = Forms::DeleteConfirmationInput.new
end

def destroy
@page = PageRepository.find(page_id: params[:page_id], form_id: current_form.id)
@url = destroy_page_path(current_form, @page.id)
@confirm_deletion_legend = t("forms_delete_confirmation_input.confirm_deletion_page")
@item_name = @page.question_text
@back_url = edit_question_path(current_form, @page.id)

@delete_confirmation_input = Forms::DeleteConfirmationInput.new(
params.require(:forms_delete_confirmation_input).permit(:confirm),
)
if @delete_confirmation_input.valid?
if @delete_confirmation_input.confirmed?
delete_page(current_form, @page)
else
redirect_to @back_url
end
else
render :delete
end
rescue StandardError
flash[:message] = "Deletion unsuccessful"
redirect_to @back_url
end

def start_new_question
clear_draft_questions_data
redirect_to type_of_answer_create_path(form_id: current_form.id)
Expand Down Expand Up @@ -70,4 +104,14 @@ def setup_draft_question_for_existing_page
end
edit_draft_question
end

def delete_page(form, page)
success_url = form_pages_path(form)

if PageRepository.destroy(page)
redirect_to success_url, status: :see_other, success: "Successfully deleted ‘#{page.question_text}’"
else
raise StandardError, "Deletion unsuccessful"
end
end
end
10 changes: 10 additions & 0 deletions app/views/pages/delete.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<% set_page_title(title_with_error_prefix(@confirm_deletion_legend, @delete_confirmation_input.errors.any?)) %>
<% content_for :back_link, govuk_back_link_to(@back_url) %>

<%= render(
@delete_confirmation_input,
url: @url,
caption_text: @item_name,
legend_text: @confirm_deletion_legend,
hint_text: nil,
) %>
4 changes: 2 additions & 2 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@
end

scope "/delete" do
get "/" => "forms/delete_confirmation#delete", as: :delete_page
delete "/" => "forms/delete_confirmation#destroy", as: :destroy_page
get "/" => "pages#delete", as: :delete_page
delete "/" => "pages#destroy", as: :destroy_page
end
end
end
Expand Down
52 changes: 0 additions & 52 deletions spec/requests/forms/delete_confirmation_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,30 +35,6 @@
end
end
end

describe "Given a valid page" do
before do
ActiveResource::HttpMock.respond_to do |mock|
mock.get "/api/v1/forms/2", headers, form.to_json, 200
end

allow(PageRepository).to receive(:find).and_return(page)

get delete_page_path(form_id: form.id, page_id: page.id)
end

it "Reads the form through the page repository" do
expect(PageRepository).to have_received(:find)
end

context "when current user is not in group for form the page is in" do
let(:membership) { nil }

it "returns an error" do
expect(response).to have_http_status :forbidden
end
end
end
end

describe "#destroy" do
Expand Down Expand Up @@ -113,33 +89,5 @@
expect(form).not_to have_been_deleted
end
end

describe "Given a valid page" do
before do
allow(PageRepository).to receive_messages(find: page, destroy: true)

delete destroy_page_path(form_id: form.id, page_id: page.id, forms_delete_confirmation_input: { confirm: "yes" })
end

it "destroys the page through the page repository" do
expect(PageRepository).to have_received(:destroy)
end

it "redirects you to the page" do
expect(response).to redirect_to(form_pages_path(form.id))
end

context "when current user is not in group for form" do
let(:membership) { nil }

it "returns an error" do
expect(response).to have_http_status :forbidden
end

it "does not call destroy through the page repository" do
expect(PageRepository).not_to have_received(:destroy)
end
end
end
end
end
43 changes: 36 additions & 7 deletions spec/requests/pages_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@
let(:form_response) { build :form, id: 2 }

let(:group) { create(:group, organisation: standard_user.organisation) }
let(:membership) { create :membership, group:, user: standard_user }

before do
Membership.create!(group_id: group.id, user: standard_user, added_by: standard_user)
membership
login_as_standard_user
end

Expand Down Expand Up @@ -86,8 +87,8 @@
end
end

describe "Deleting an existing page" do
describe "Given a valid page" do
describe "#delete" do
describe "given a valid page" do
let(:page) do
Page.new({
id: 1,
Expand All @@ -113,13 +114,25 @@
end

it "renders the delete page template" do
expect(response).to render_template("forms/delete_confirmation/delete")
expect(response).to render_template("pages/delete")
end

it "reads the form through the page repository" do
expect(PageRepository).to have_received(:find)
end

context "when current user is not in group for form the page is in" do
let(:membership) { nil }

it "returns an error" do
expect(response).to have_http_status :forbidden
end
end
end
end

describe "Destroying an existing page" do
describe "Given a valid page" do
describe "#destroy" do
describe "given a valid page" do
let(:page) do
Page.new({
id: 1,
Expand Down Expand Up @@ -149,9 +162,25 @@
delete destroy_page_path(form_id: 2, page_id: 1, forms_delete_confirmation_input: { confirm: "yes" })
end

it "Redirects you to the page index screen" do
it "redirects you to the page index screen" do
expect(response).to redirect_to(form_pages_path)
end

it "destroys the page through the page repository" do
expect(PageRepository).to have_received(:destroy)
end

context "when current user is not in group for form" do
let(:membership) { nil }

it "returns an error" do
expect(response).to have_http_status :forbidden
end

it "does not call destroy through the page repository" do
expect(PageRepository).not_to have_received(:destroy)
end
end
end
end

Expand Down
43 changes: 43 additions & 0 deletions spec/views/pages/delete.html.erb_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
require "rails_helper"

RSpec.describe "pages/delete" do
before do
assign(:back_url, edit_question_path(form_id: 1, page_id: 1))
assign(:confirm_deletion_legend, "Are you sure you want to delete this page?")
assign(:delete_confirmation_input, Forms::DeleteConfirmationInput.new)
assign(:item_name, "What’s your name?")
assign(:url, destroy_page_path(form_id: 1, page_id: 1))

render
end

it "has a page title" do
expect(view.content_for(:title)).to include "Are you sure you want to delete this page?"
end

it "has a heading" do
expect(rendered).to have_css "h1", text: "Are you sure you want to delete this page?"
end

it "has a heading caption with the question text" do
expect(rendered).to have_css ".govuk-caption-l", text: "What’s your name?"
end

it "has a back link to the edit question page" do
expect(view.content_for(:back_link)).to have_link "Back", href: "/forms/1/pages/1/edit/question"
end

it "has a delete confirmation input to confirm deletion of the page" do
expect(rendered).to render_template "input_objects/forms/_delete_confirmation_input"
end

describe "delete confirmation input" do
it "posts to the destroy action" do
expect(rendered).to have_element "form", action: "/forms/1/pages/1/delete", method: "post"
end

it "does not have a hint" do
expect(rendered).not_to have_css ".govuk-hint"
end
end
end

0 comments on commit caba6b4

Please sign in to comment.