Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix delete all routes journey #1774

Merged
merged 4 commits into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion app/services/condition_repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,15 @@ def save!(record)
def destroy(record)
condition = Api::V1::ConditionResource.new(record.attributes, true)
condition.prefix_options = record.prefix_options
condition.destroy # rubocop:disable Rails/SaveBang

begin
condition.destroy # rubocop:disable Rails/SaveBang
rescue ActiveResource::ResourceNotFound
# ActiveRecord::Persistence#destroy doesn't raise an error
# if record has already been destroyed, let's emulate that
end

record
end
end
end
10 changes: 9 additions & 1 deletion app/services/form_repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,15 @@ def archive!(record)

def destroy(record)
form = Api::V1::FormResource.new(record.attributes, true)
form.destroy # rubocop:disable Rails/SaveBang

begin
form.destroy # rubocop:disable Rails/SaveBang
rescue ActiveResource::ResourceNotFound
# ActiveRecord::Persistence#destroy doesn't raise an error
# if record has already been destroyed, let's emulate that
end

record
end

def pages(record)
Expand Down
10 changes: 9 additions & 1 deletion app/services/page_repository.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,15 @@ def save!(record)
def destroy(record)
page = Api::V1::PageResource.new(record.attributes, true)
page.prefix_options = record.prefix_options
page.destroy # rubocop:disable Rails/SaveBang

begin
page.destroy # rubocop:disable Rails/SaveBang
rescue ActiveResource::ResourceNotFound
# ActiveRecord::Persistence#destroy doesn't raise an error
# if record has already been destroyed, let's emulate that
end

record
end

def move_page(record, direction)
Expand Down
20 changes: 18 additions & 2 deletions spec/requests/pages/routes_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@
end

describe "#destroy" do
let(:condition) { build :condition, routing_page_id: selected_page.id, check_page_id: selected_page.id, goto_page_id: pages.last.id, answer_value: "Option 1" }
let(:condition) { build :condition, id: 1, routing_page_id: selected_page.id, check_page_id: selected_page.id, goto_page_id: pages.last.id, answer_value: "Option 1" }
let(:secondary_skip_page) { form.pages[2] }
let(:secondary_skip) { build :condition, routing_page_id: secondary_skip_page.id, check_page_id: selected_page.id, goto_page_id: pages[3].id }
let(:secondary_skip) { build :condition, id: 2, routing_page_id: secondary_skip_page.id, check_page_id: selected_page.id, goto_page_id: pages[3].id }

before do
allow(PageRepository).to receive(:find).with(page_id: "101", form_id: 1).and_return(selected_page)
Expand All @@ -84,6 +84,22 @@
expect(ConditionRepository).to receive(:destroy).with(have_attributes(id: secondary_skip.id))
delete destroy_routes_path(form_id: form.id, page_id: selected_page.id, pages_routes_delete_confirmation_input: { confirm: "yes" })
end

context "but one of the routes is already deleted" do
before do
# forms-api may choose to delete the secondary skip when the condition is deleted
allow(ConditionRepository).to receive(:destroy).and_call_original
ActiveResource::HttpMock.respond_to do |mock|
mock.delete "/api/v1/forms/#{form.id}/pages/#{selected_page.id}/conditions/#{condition.id}", delete_headers, nil, 204
mock.delete "/api/v1/forms/#{form.id}/pages/#{secondary_skip_page.id}/conditions/#{secondary_skip.id}", delete_headers, nil, 404
end
end

it "does not render an error page" do
delete destroy_routes_path(form_id: form.id, page_id: selected_page.id, pages_routes_delete_confirmation_input: { confirm: "yes" })
expect(response).not_to be_client_error
end
end
end

context "when given invalid params" do
Expand Down
31 changes: 31 additions & 0 deletions spec/services/condition_repository_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,5 +72,36 @@
described_class.destroy(condition)
expect(Api::V1::ConditionResource.new(id: 4, form_id: 1, page_id: 2)).to have_been_deleted
end

it "returns the deleted condition" do
condition = described_class.find(condition_id: 4, form_id: 1, page_id: 2)
expect(described_class.destroy(condition)).to eq condition
end

context "when the condition has already been deleted" do
it "does not raise an error" do
condition = described_class.find(condition_id: 4, form_id: 1, page_id: 2)
described_class.destroy(condition)

ActiveResource::HttpMock.respond_to do |mock|
mock.delete "/api/v1/forms/1/pages/2/conditions/4", delete_headers, nil, 404
end

expect {
described_class.destroy(condition)
}.not_to raise_error
end

it "returns the deleted condition" do
condition = described_class.find(condition_id: 4, form_id: 1, page_id: 2)
described_class.destroy(condition)

ActiveResource::HttpMock.respond_to do |mock|
mock.delete "/api/v1/forms/1/pages/2/conditions/4", delete_headers, nil, 404
end

expect(described_class.destroy(condition)).to eq condition
end
end
end
end
31 changes: 31 additions & 0 deletions spec/services/form_repository_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,37 @@
described_class.destroy(form)
expect(Api::V1::FormResource.new(id: 2)).to have_been_deleted
end

it "returns the deleted form" do
form = described_class.find(form_id: 2)
expect(described_class.destroy(form)).to eq form
end

context "when the form has already been deleted" do
it "does not raise an error" do
form = described_class.find(form_id: 2)
described_class.destroy(form)

ActiveResource::HttpMock.respond_to do |mock|
mock.delete "/api/v1/forms/2", delete_headers, nil, 404
end

expect {
described_class.destroy(form)
}.not_to raise_error
end

it "returns the deleted form" do
form = described_class.find(form_id: 2)
described_class.destroy(form)

ActiveResource::HttpMock.respond_to do |mock|
mock.delete "/api/v1/forms/2", delete_headers, nil, 404
end

expect(described_class.destroy(form)).to eq form
end
end
end

describe "#pages" do
Expand Down
31 changes: 31 additions & 0 deletions spec/services/page_repository_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,37 @@
described_class.destroy(page)
expect(Api::V1::PageResource.new(id: 2, form_id: 1)).to have_been_deleted
end

it "returns the deleted page" do
page = described_class.find(page_id: 2, form_id: 1)
expect(described_class.destroy(page)).to eq page
end

context "when the page has already been deleted" do
it "does not raise an error" do
page = described_class.find(page_id: 2, form_id: 1)
described_class.destroy(page)

ActiveResource::HttpMock.respond_to do |mock|
mock.delete "/api/v1/forms/1/pages/2", delete_headers, nil, 404
end

expect {
described_class.destroy(page)
}.not_to raise_error
end

it "returns the deleted page" do
page = described_class.find(page_id: 2, form_id: 1)
described_class.destroy(page)

ActiveResource::HttpMock.respond_to do |mock|
mock.delete "/api/v1/forms/1/pages/2", delete_headers, nil, 404
end

expect(described_class.destroy(page)).to eq page
end
end
end

describe "#move_page" do
Expand Down