Use 'update!' to surface save errors

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.

Note that some method calls cannot be changed without further work,
so this disables the Cop instead:

- Admin::WorldwideOrganisationsController#create for some reason
returns a 200 response if the creation failed, and relies on the
view to find and show any errors.

- StatisticsAnnouncement#cancel! uses 'save' to return the boolean
result of the operation.

.rubocop.yml

@@ -81,3 +81,11 @@ Rails/FilePath:
 # it should be removed when we upgrade.
 Style/HashEachMethods:
   Enabled: false
+
+# 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

app/controllers/admin/attachments_controller.rb

@@ -59,7 +59,7 @@ def update_many
   end
 
   def destroy
-    attachment.destroy
+    attachment.destroy!
     attachment_updater
     redirect_to attachable_attachments_path(attachable), notice: "Attachment deleted"
   end
@@ -195,11 +195,13 @@ def handle_duplicate_key_errors_caused_by_double_create_requests(exception)
   end
 
   def save_attachment
-    attachment.save(context: :user_input).tap do |result|
-      if result && attachment.is_a?(HtmlAttachment)
-        Whitehall::PublishingApi.save_draft(attachment)
-      end
+    result = attachment.save(context: :user_input)
+
+    if result && attachment.is_a?(HtmlAttachment)
+      Whitehall::PublishingApi.save_draft(attachment)
     end
+
+    result
   end
 
   def attachment_updater

app/controllers/admin/classification_featurings_controller.rb

@@ -52,11 +52,11 @@ def order
   def destroy
     if featuring_a_document?
       edition = @classification_featuring.edition
-      @classification_featuring.destroy
+      @classification_featuring.destroy!
       flash[:notice] = "#{edition.title} has been unfeatured from #{@classification.name}"
     else
       offsite_link = @classification_featuring.offsite_link
-      @classification_featuring.destroy
+      @classification_featuring.destroy!
       flash[:notice] = "#{offsite_link.title} has been unfeatured from #{@classification.name}"
     end
     redirect_to polymorphic_path([:admin, @classification, :classification_featurings])

app/controllers/admin/contacts_controller.rb

@@ -15,8 +15,7 @@ def edit
   end
 
   def update
-    @contact.update(contact_params)
-    if @contact.save
+    if @contact.update(contact_params)
       handle_show_on_home_page_param
       redirect_to [:admin, @contact.contactable, Contact], notice: %("#{@contact.title}" updated successfully)
     else

app/controllers/admin/document_collection_groups_controller.rb

@@ -34,7 +34,7 @@ def update
   end
 
   def destroy
-    @group.destroy
+    @group.destroy!
     redirect_to admin_document_collection_groups_path(@collection),
                 notice: "'#{@group.heading}' was deleted"
   end

app/controllers/admin/financial_reports_controller.rb

@@ -24,7 +24,7 @@ def update
   end
 
   def destroy
-    @financial_report.destroy
+    @financial_report.destroy!
     redirect_to admin_organisation_financial_reports_path(@organisation), notice: "Deleted Successfully"
   end
 

app/controllers/admin/governments_controller.rb

@@ -42,11 +42,11 @@ def prepare_to_close
   def close
     government = Government.find(params[:id])
 
-    government.update(end_date: Time.zone.today) unless government.end_date
+    government.update!(end_date: Time.zone.today) unless government.end_date
 
     current_active_ministerial_appointments.each do |appointment|
       appointment.ended_at = government.end_date
-      appointment.save(validate: false)
+      appointment.save!(validate: false)
     end
 
     redirect_to edit_admin_government_path(government), notice: "Government closed"

app/controllers/admin/historical_accounts_controller.rb

@@ -30,7 +30,7 @@ def update
   end
 
   def destroy
-    @historical_account.destroy
+    @historical_account.destroy!
     redirect_to admin_person_historical_accounts_url(@person), notice: "Historical account deleted"
   end
 

app/controllers/admin/offsite_links_controller.rb

@@ -28,7 +28,7 @@ def update
   end
 
   def destroy
-    @offsite_link.destroy
+    @offsite_link.destroy!
     flash[:notice] = "#{@offsite_link.title} has been deleted"
     redirect_to offsite_links_path
   end

app/controllers/admin/organisations_controller.rb

@@ -76,7 +76,7 @@ def update
   end
 
   def destroy
-    @organisation.destroy
+    @organisation.destroy!
     redirect_to admin_organisations_path
   end
 

app/controllers/admin/policy_groups_controller.rb

@@ -34,7 +34,7 @@ def update
   def destroy
     policy_group = PolicyGroup.friendly.find(params[:id])
     name = policy_group.name
-    policy_group.destroy
+    policy_group.destroy!
     redirect_to admin_policy_groups_path, notice: %("#{name}" deleted.)
   end
 

app/controllers/admin/promotional_feature_items_controller.rb

@@ -9,7 +9,7 @@ def new
   end
 
   def create
-    @promotional_feature_item = @promotional_feature.promotional_feature_items.create(promotional_feature_item_params)
+    @promotional_feature_item = @promotional_feature.promotional_feature_items.build(promotional_feature_item_params)
     if @promotional_feature_item.save
       redirect_to_feature "Feature item added."
     else
@@ -30,7 +30,7 @@ def update
   end
 
   def destroy
-    @promotional_feature_item.destroy
+    @promotional_feature_item.destroy!
     redirect_to_feature "Feature item deleted."
   end
 

app/controllers/admin/promotional_features_controller.rb

@@ -34,7 +34,7 @@ def update
   end
 
   def destroy
-    @promotional_feature.destroy
+    @promotional_feature.destroy!
     redirect_to [:admin, @organisation, PromotionalFeature], notice: "Promotional feature deleted."
   end
 

app/controllers/admin/role_appointments_controller.rb

@@ -29,7 +29,7 @@ def update
 
   def destroy
     if @role_appointment.destroyable?
-      @role_appointment.destroy
+      @role_appointment.destroy!
       redirect_to edit_admin_role_path(@role_appointment.role), notice: "Appointment has been deleted"
     else
       flash.now[:alert] = "Appointment can not be deleted"

app/controllers/admin/social_media_accounts_controller.rb

@@ -14,8 +14,7 @@ def new
   def edit; end
 
   def update
-    @social_media_account.update(social_media_account_params)
-    if @social_media_account.save
+    if @social_media_account.update(social_media_account_params)
       redirect_to [:admin, @socialable, SocialMediaAccount],
                   notice: "#{@social_media_account.service_name} account updated successfully"
     else

app/controllers/admin/take_part_pages_controller.rb

@@ -36,7 +36,7 @@ def update
 
   def destroy
     @take_part_page = TakePartPage.friendly.find(params[:id])
-    @take_part_page.destroy
+    @take_part_page.destroy!
     redirect_to [:admin, TakePartPage], notice: %(Take part page "#{@take_part_page.title}" deleted!)
   end
 

app/controllers/admin/worldwide_offices_controller.rb

@@ -16,8 +16,7 @@ def edit
 
   def update
     worldwide_office_params[:service_ids] ||= []
-    @worldwide_office.update(worldwide_office_params)
-    if @worldwide_office.save
+    if @worldwide_office.update(worldwide_office_params)
       handle_show_on_home_page_param
       redirect_to [:admin, @worldwide_organisation, WorldwideOffice]
     else

app/controllers/admin/worldwide_organisations_controller.rb

@@ -14,7 +14,7 @@ def new
   end
 
   def create
-    @worldwide_organisation = WorldwideOrganisation.create(worldwide_organisation_params)
+    @worldwide_organisation = WorldwideOrganisation.create(worldwide_organisation_params) # rubocop:disable Rails/SaveBang
     respond_with :admin, @worldwide_organisation
   end
 
@@ -24,7 +24,7 @@ def edit
   end
 
   def update
-    @worldwide_organisation.update(worldwide_organisation_params)
+    @worldwide_organisation.update!(worldwide_organisation_params)
     respond_with :admin, @worldwide_organisation
   end
 
@@ -44,7 +44,7 @@ def set_main_office
   end
 
   def destroy
-    @worldwide_organisation.destroy
+    @worldwide_organisation.destroy!
     respond_with :admin, @worldwide_organisation
   end
 

app/models/classification_relation.rb

@@ -48,7 +48,7 @@ def create_inverse_relation
 
   def destroy_inverse_relation
     if inverse_relation.present?
-      inverse_relation.destroy
+      inverse_relation.destroy!
     end
   end
 end

app/models/consultation_participation.rb

@@ -31,7 +31,7 @@ def has_postal_address?
   def destroy_form_if_required
     if has_response_form? &&
         ConsultationParticipation.where(consultation_response_form_id: consultation_response_form.id).empty?
-      consultation_response_form.destroy
+      consultation_response_form.destroy!
     end
   end
 

app/models/consultation_response_form.rb

@@ -14,7 +14,7 @@ class ConsultationResponseForm < ApplicationRecord
 
   def destroy_consultation_response_form_data_if_required
     unless ConsultationResponseForm.where(consultation_response_form_data_id: consultation_response_form_data.id).any?
-      consultation_response_form_data.destroy
+      consultation_response_form_data.destroy!
     end
   end
 end

app/models/contact_type.rb

@@ -7,13 +7,13 @@ def self.find_by_name(name)
     all.detect { |type| type.name == name }
   end
 
-  General = create(
+  General = create!(
     id: 1, name: "General contact",
   )
-  FOI = create(
+  FOI = create!(
     id: 2, name: "Freedom of Information contact",
   )
-  Media = create(
+  Media = create!(
     id: 3, name: "Media contact",
   )
 end

app/models/corporate_information_page_type.rb

@@ -34,67 +34,67 @@ def display_type_key
     slug.tr("-", "_")
   end
 
-  PersonalInformationCharter = create(
+  PersonalInformationCharter = create!(
     id: 1, slug: "personal-information-charter", menu_heading: :other,
   )
-  PublicationScheme = create(
+  PublicationScheme = create!(
     id: 2, slug: "publication-scheme", menu_heading: :other,
   )
-  ComplaintsProcedure = create(
+  ComplaintsProcedure = create!(
     id: 3, slug: "complaints-procedure", menu_heading: :our_information,
   )
-  TermsOfReference = create(
+  TermsOfReference = create!(
     id: 4, slug: "terms-of-reference", menu_heading: :our_information,
   )
-  OurGovernance = create(
+  OurGovernance = create!(
     id: 5, slug: "our-governance", menu_heading: :our_information,
   )
-  Statistics = create(
+  Statistics = create!(
     id: 6, slug: "statistics", menu_heading: :our_information,
   )
-  Procurement = create(
+  Procurement = create!(
     id: 7, slug: "procurement", menu_heading: :jobs_and_contracts,
   )
-  Recruitment = create(
+  Recruitment = create!(
     id: 8, slug: "recruitment", menu_heading: :jobs_and_contracts,
   )
-  OurEnergyUse = create(
+  OurEnergyUse = create!(
     id: 9, slug: "our-energy-use", menu_heading: :our_information,
   )
-  Membership = create(
+  Membership = create!(
     id: 10, slug: "membership", menu_heading: :our_information,
   )
-  WelshLanguageScheme = create(
+  WelshLanguageScheme = create!(
     id: 11, slug: "welsh-language-scheme", menu_heading: :other,
   )
-  EqualityAndDiversity = create(
+  EqualityAndDiversity = create!(
     id: 12, slug: "equality-and-diversity", menu_heading: :our_information,
   )
-  PetitionsAndCampaigns = create(
+  PetitionsAndCampaigns = create!(
     id: 13, slug: "petitions-and-campaigns", menu_heading: :our_information,
   )
-  Research = create(
+  Research = create!(
     id: 14, slug: "research", menu_heading: :our_information,
   )
-  OfficeAccessAndOpeningTimes = create(
+  OfficeAccessAndOpeningTimes = create!(
     id: 15, slug: "access-and-opening", menu_heading: :our_information,
   )
-  StaffNewsAndInformation = create(
+  StaffNewsAndInformation = create!(
     id: 16, slug: "staff-update", menu_heading: :other,
   )
-  MediaEnquiries = create(
+  MediaEnquiries = create!(
     id: 17, slug: "media-enquiries", menu_heading: :our_information,
   )
-  SocialMediaUse = create(
+  SocialMediaUse = create!(
     id: 18, slug: "social-media-use", menu_heading: :other,
   )
-  AboutOurServices = create(
+  AboutOurServices = create!(
     id: 19, slug: "about-our-services", menu_heading: :other,
   )
-  AboutUs = create(
+  AboutUs = create!(
     id: 20, slug: "about", menu_heading: :other,
   )
-  AccessibleDocumentsPolicy = create(
+  AccessibleDocumentsPolicy = create!(
     id: 21, slug: "accessible-documents-policy", menu_heading: :our_information,
   )
 

app/models/document.rb

@@ -91,7 +91,7 @@ def update_slug_if_possible(new_title)
 
     candidate_slug = normalize_friendly_id(new_title)
     unless candidate_slug == slug
-      update(sluggable_string: new_title)
+      update!(sluggable_string: new_title)
     end
   end
 

app/models/edition/active_editors.rb

@@ -10,7 +10,7 @@ def active_edition_openings
   end
 
   def open_for_editing_as(editor)
-    recent_edition_openings.create(editor: editor)
+    recent_edition_openings.create!(editor: editor)
   rescue ActiveRecord::RecordNotUnique
     recent_edition_openings.where(editor_id: editor).find_each do |r|
       r.update!(created_at: Time.zone.now)

app/models/edition/audit_trail.rb

@@ -75,7 +75,7 @@ def publication_audit_entry
 
   def record_create
     user = Edition::AuditTrail.whodunnit
-    versions.create event: "create", user: user, state: state
+    versions.create! event: "create", user: user, state: state
     alert!(user)
   end
 

app/models/edition/images.rb

@@ -8,7 +8,7 @@ def process_associations_after_save(edition)
         if image.invalid?
           Rails.logger.warn "Ignoring errors on saving image for edition with id #{edition.id}: #{image.errors.full_messages.join(', ')}"
         end
-        image.save(validate: false)
+        image.save!(validate: false)
       end
     end
   end