Skip to content

Commit

Permalink
Use 'update!' to surface save errors
Browse files Browse the repository at this point in the history
Previously we switched from 'update_attribute' to 'update' to fix
linting issues, but this carries a risk of silently failing to save
changes, since 'update' will run validations. This switches all uses
of 'update' to 'update!' to ensure we become aware of any issues in
case they are not covered by the tests.
  • Loading branch information
Ben Thorner committed May 27, 2020
1 parent 2d6b614 commit ac607b4
Show file tree
Hide file tree
Showing 12 changed files with 24 additions and 17 deletions.
8 changes: 8 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,11 @@ inherit_gem:
inherit_mode:
merge:
- Exclude

# We seldom check the return value of 'update' to see if
# the operation was successful. Since we assume success, we
# should raise an exception if this is not the case.
#
# We intend to add this to rubocop-govuk.
Rails/SaveBang:
Enabled: true
2 changes: 1 addition & 1 deletion app/models/link.rb
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def self.build(params)

def make_missing
self.url = nil
save
save!
end

private
Expand Down
2 changes: 1 addition & 1 deletion lib/tasks/import/covid19_mhclg.rake
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace :import do
end

SERVICE_MAPPINGS.each do |type, codes|
service_interaction = ServiceInteraction.find_or_create_by(
service_interaction = ServiceInteraction.find_or_create_by!(
service: Service.find_by!(lgsl_code: codes[:lgsl]),
interaction: Interaction.find_by!(lgil_code: codes[:lgil]),
)
Expand Down
2 changes: 1 addition & 1 deletion lib/tasks/import/covid19_theyhelpyou.rake
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace :import do
end

puts "Fetching mappings for type #{type}, LGSL=#{codes[:lgsl]} LGIL=#{codes[:lgil]}"
service_interaction = ServiceInteraction.find_or_create_by(
service_interaction = ServiceInteraction.find_or_create_by!(
service: Service.find_by!(lgsl_code: codes[:lgsl]),
interaction: Interaction.find_by!(lgil_code: codes[:lgil]),
)
Expand Down
2 changes: 1 addition & 1 deletion spec/features/links/index_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
feature "The broken links page" do
before do
User.create(email: "[email protected]", name: "Test User", permissions: %w[signin])
User.create!(email: "[email protected]", name: "Test User", permissions: %w[signin])

@service = create(:service, :all_tiers)
@service_interaction = create(:service_interaction, service: @service)
Expand Down
2 changes: 1 addition & 1 deletion spec/features/links/links_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
feature "The links for a local authority" do
before do
User.create(email: "[email protected]", name: "Test User", permissions: %w[signin])
User.create!(email: "[email protected]", name: "Test User", permissions: %w[signin])
@time = Timecop.freeze("2016-07-14 11:34:09 +0100")
@local_authority = create(:local_authority, status: "ok", link_last_checked: @time - (60 * 60))
@service = create(:service)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
feature "The local authorities index page" do
before do
User.create(email: "[email protected]", name: "Test User", permissions: %w[signin])
User.create!(email: "[email protected]", name: "Test User", permissions: %w[signin])

@angus = create(:local_authority, name: "Angus")
@zorro = create(:local_authority, name: "Zorro Council", broken_link_count: 1)
Expand Down
6 changes: 3 additions & 3 deletions spec/features/local_authorities/local_authority_show_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
let!(:local_authority) { create(:district_council) }

before do
User.create(email: "[email protected]", name: "Test User", permissions: %w[signin])
User.create!(email: "[email protected]", name: "Test User", permissions: %w[signin])
visit local_authority_path(local_authority_slug: local_authority.slug)
end

Expand All @@ -26,7 +26,7 @@

it "displays 'No link'" do
local_authority.homepage_url = nil
local_authority.save
local_authority.save!
visit local_authority_path(local_authority_slug: local_authority.slug)
within(:css, ".page-title") do
expect(page).to have_content("No link")
Expand All @@ -35,7 +35,7 @@

it "does not display 'Link not checked'" do
local_authority.homepage_url = nil
local_authority.save
local_authority.save!
visit local_authority_path(local_authority_slug: local_authority.slug)
within(:css, ".page-title") do
expect(page).not_to have_content("Link not checked")
Expand Down
2 changes: 1 addition & 1 deletion spec/features/services/service_index_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
feature "The services index page" do
before do
User.create(email: "[email protected]", name: "Test User", permissions: %w[signin])
User.create!(email: "[email protected]", name: "Test User", permissions: %w[signin])

@aardvark = create(:service, label: "Aardvark Wardens")
@zebra = create(:service, label: "Zebra Fouling", broken_link_count: 1)
Expand Down
2 changes: 1 addition & 1 deletion spec/features/services/service_show_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
feature "The services show page" do
before do
User.create(email: "[email protected]", name: "Test User", permissions: %w[signin])
User.create!(email: "[email protected]", name: "Test User", permissions: %w[signin])

@service = create(:service, :all_tiers)
service_interaction = create(:service_interaction, service: @service)
Expand Down
7 changes: 3 additions & 4 deletions spec/lib/local-links-manager/link_resolver_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
end

it "returns nil if no service interaction exists" do
service_interaction.destroy
service_interaction.destroy!

expect(link_resolver.resolve).to be_nil
end
Expand Down Expand Up @@ -69,8 +69,7 @@
end

it "returns the link that is not for LGIL 8 if its LGIL is higher than 8" do
interaction2.update(lgil_code: 9)

interaction2.update!(lgil_code: 9)
expect(link_resolver.resolve).to eq(link2)
end
end
Expand All @@ -85,7 +84,7 @@
end

it "returns the link if it is for LGIL 8" do
link.interaction.update(lgil_code: 8)
link.interaction.update!(lgil_code: 8)

expect(link_resolver.resolve).to eq(link)
end
Expand Down
4 changes: 2 additions & 2 deletions spec/requests/api/link_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@
end

it "responds without link details for unsupported lgsl and lgil combination" do
link.destroy
service_interaction.destroy
link.destroy!
service_interaction.destroy!

get "/api/link?authority_slug=blackburn&lgsl=2&lgil=4"

Expand Down

0 comments on commit ac607b4

Please sign in to comment.