Apply Rails/SaveBang rubocop rule

Having exceptions is better than having silent bugs.

There are a few methods I've kept the same way they were.

The `RelatedContentScore#score_with_opposite` method is a bit peculiar:
it creates scores for both itself and the opposite related content,
which means the opposite related content will try to create the same
scores as well.

We've already got a test to check `Budget::Ballot#add_investment` when
creating a line fails ("Edge case voting a non-elegible investment").

Finally, the method `User#send_oauth_confirmation_instructions` doesn't
update the record when the email address isn't already present, leading
to the test "Try to register with the email of an already existing user,
when an unconfirmed email was provided by oauth" fo fail if we raise an
exception for an invalid user. That's because updating a user's email
doesn't update the database automatically, but instead a confirmation
email is sent.

There are also a few false positives for classes which don't have bang
methods (like the GraphQL classes) or destroying attachments.

For these reasons, I'm adding the rule with a "Refactor" severity,
meaning it's a rule we can break if necessary.
This commit is contained in:
Javi Martín
2019-10-20 03:54:56 +02:00
parent 777fb55399
commit 7ca55c44e0
167 changed files with 645 additions and 645 deletions

View File

@@ -41,7 +41,7 @@ class Admin::AdminNotificationsController < Admin::BaseController
def destroy
@admin_notification = AdminNotification.find(params[:id])
@admin_notification.destroy
@admin_notification.destroy!
notice = t("admin.admin_notifications.delete_success")
redirect_to admin_admin_notifications_path, notice: notice

View File

@@ -14,7 +14,7 @@ class Admin::AdministratorsController < Admin::BaseController
def create
@administrator.user_id = params[:user_id]
@administrator.save
@administrator.save!
redirect_to admin_administrators_path
end

View File

@@ -31,7 +31,7 @@ class Admin::BannersController < Admin::BaseController
end
def destroy
@banner.destroy
@banner.destroy!
redirect_to admin_banners_path
end

View File

@@ -38,7 +38,7 @@ class Admin::BudgetGroupsController < Admin::BaseController
if @group.headings.any?
redirect_to groups_index, alert: t("admin.budget_groups.destroy.unable_notice")
else
@group.destroy
@group.destroy!
redirect_to groups_index, notice: t("admin.budget_groups.destroy.success_notice")
end
end

View File

@@ -37,7 +37,7 @@ class Admin::BudgetHeadingsController < Admin::BaseController
def destroy
if @heading.can_be_deleted?
@heading.destroy
@heading.destroy!
redirect_to headings_index, notice: t("admin.budget_headings.destroy.success_notice")
else
redirect_to headings_index, alert: t("admin.budget_headings.destroy.unable_notice")

View File

@@ -62,7 +62,7 @@ class Admin::BudgetInvestmentsController < Admin::BaseController
def toggle_selection
@investment.toggle :selected
@investment.save
@investment.save!
load_investments
end
@@ -127,7 +127,7 @@ class Admin::BudgetInvestmentsController < Admin::BaseController
def load_ballot
query = Budget::Ballot.where(user: current_user, budget: @budget)
@ballot = @budget.balloting? ? query.first_or_create : query.first_or_initialize
@ballot = @budget.balloting? ? query.first_or_create! : query.first_or_initialize
end
def parse_valuation_filters

View File

@@ -66,7 +66,7 @@ class Admin::BudgetsController < Admin::BaseController
elsif @budget.poll.present?
redirect_to admin_budgets_path, alert: t("admin.budgets.destroy.unable_notice_polls")
else
@budget.destroy
@budget.destroy!
redirect_to admin_budgets_path, notice: t("admin.budgets.destroy.success_notice")
end
end

View File

@@ -15,7 +15,7 @@ class Admin::Dashboard::AdministratorTasksController < Admin::Dashboard::BaseCon
def update
authorize! :update, administrator_task
administrator_task.update(user: current_user, executed_at: Time.current)
administrator_task.update!(user: current_user, executed_at: Time.current)
redirect_to admin_dashboard_administrator_tasks_path,
{ flash: { notice: t("admin.dashboard.administrator_tasks.update.success") }}
end

View File

@@ -34,7 +34,7 @@ class Admin::GeozonesController < Admin::BaseController
def destroy
if @geozone.safe_to_destroy?
@geozone.destroy
@geozone.destroy!
redirect_to admin_geozones_path, notice: t("admin.geozones.delete.success")
else
redirect_to admin_geozones_path, flash: { error: t("admin.geozones.delete.error") }

View File

@@ -32,7 +32,7 @@ class Admin::Legislation::DraftVersionsController < Admin::Legislation::BaseCont
end
def destroy
@draft_version.destroy
@draft_version.destroy!
notice = t("admin.legislation.draft_versions.destroy.notice")
redirect_to admin_legislation_process_draft_versions_path, notice: notice
end

View File

@@ -46,7 +46,7 @@ class Admin::Legislation::ProcessesController < Admin::Legislation::BaseControll
end
def destroy
@process.destroy
@process.destroy!
notice = t("admin.legislation.processes.destroy.notice")
redirect_to admin_legislation_processes_path, notice: notice
end
@@ -93,7 +93,7 @@ class Admin::Legislation::ProcessesController < Admin::Legislation::BaseControll
def set_tag_list
@process.set_tag_list_on(:customs, process_params[:custom_list])
@process.save
@process.save!
end
def resource

View File

@@ -33,7 +33,7 @@ class Admin::Legislation::QuestionsController < Admin::Legislation::BaseControll
end
def destroy
@question.destroy
@question.destroy!
notice = t("admin.legislation.questions.destroy.notice")
redirect_to admin_legislation_process_questions_path, notice: notice
end

View File

@@ -26,7 +26,7 @@ class Admin::LocalCensusRecordsController < Admin::BaseController
end
def destroy
@local_census_record.destroy
@local_census_record.destroy!
redirect_to admin_local_census_records_path,
notice: t("admin.local_census_records.destroy.notice")
end

View File

@@ -14,13 +14,13 @@ class Admin::ManagersController < Admin::BaseController
def create
@manager.user_id = params[:user_id]
@manager.save
@manager.save!
redirect_to admin_managers_path
end
def destroy
@manager.destroy
@manager.destroy!
redirect_to admin_managers_path
end
end

View File

@@ -34,7 +34,7 @@ class Admin::MilestoneStatusesController < Admin::BaseController
end
def destroy
@status.destroy
@status.destroy!
redirect_to admin_milestone_statuses_path,
notice: t("admin.statuses.delete.notice")
end

View File

@@ -14,13 +14,13 @@ class Admin::ModeratorsController < Admin::BaseController
def create
@moderator.user_id = params[:user_id]
@moderator.save
@moderator.save!
redirect_to admin_moderators_path
end
def destroy
@moderator.destroy
@moderator.destroy!
redirect_to admin_moderators_path
end
end

View File

@@ -39,7 +39,7 @@ class Admin::NewslettersController < Admin::BaseController
def destroy
@newsletter = Newsletter.find(params[:id])
@newsletter.destroy
@newsletter.destroy!
redirect_to admin_newsletters_path, notice: t("admin.newsletters.delete_success")
end
@@ -49,7 +49,7 @@ class Admin::NewslettersController < Admin::BaseController
if @newsletter.valid?
@newsletter.delay.deliver
@newsletter.update(sent_at: Time.current)
@newsletter.update!(sent_at: Time.current)
flash[:notice] = t("admin.newsletters.send_success")
else
flash[:error] = t("admin.segment_recipient.invalid_recipients_segment")

View File

@@ -14,7 +14,7 @@ class Admin::OfficialsController < Admin::BaseController
def update
@user = User.find(params[:id])
@user.update(user_params)
@user.update!(user_params)
redirect_to admin_officials_path, notice: t("admin.officials.flash.official_updated")
end

View File

@@ -29,7 +29,7 @@ class Admin::Poll::BoothAssignmentsController < Admin::Poll::BaseController
@booth_assignment = ::Poll::BoothAssignment.new(poll: @poll,
booth: @booth)
@booth_assignment.save
@booth_assignment.save!
respond_to do |format|
format.js { render layout: false }
@@ -41,7 +41,7 @@ class Admin::Poll::BoothAssignmentsController < Admin::Poll::BaseController
@booth = Poll::Booth.find(booth_assignment_params[:booth_id])
@booth_assignment = ::Poll::BoothAssignment.find(params[:id])
@booth_assignment.destroy
@booth_assignment.destroy!
respond_to do |format|
format.js { render layout: false }

View File

@@ -20,13 +20,13 @@ class Admin::Poll::OfficersController < Admin::Poll::BaseController
def create
@officer.user_id = params[:user_id]
@officer.save
@officer.save!
redirect_to admin_officers_path
end
def destroy
@officer.destroy
@officer.destroy!
redirect_to admin_officers_path
end

View File

@@ -63,7 +63,7 @@ class Admin::Poll::PollsController < Admin::Poll::BaseController
if ::Poll::Voter.where(poll: @poll).any?
redirect_to admin_poll_path(@poll), alert: t("admin.polls.destroy.unable_notice")
else
@poll.destroy
@poll.destroy!
redirect_to admin_polls_path, notice: t("admin.polls.destroy.success_notice")
end

View File

@@ -22,7 +22,7 @@ class Admin::Poll::Questions::Answers::ImagesController < Admin::Poll::BaseContr
def destroy
@image = ::Image.find(params[:id])
@image.destroy
@image.destroy!
respond_to do |format|
format.js { render layout: false }

View File

@@ -30,7 +30,7 @@ class Admin::Poll::ShiftsController < Admin::Poll::BaseController
alert = t("admin.poll_shifts.flash.unable_to_destroy")
redirect_to new_admin_booth_shift_path(@booth), alert: alert
else
@shift.destroy
@shift.destroy!
notice = t("admin.poll_shifts.flash.destroy")
redirect_to new_admin_booth_shift_path(@booth), notice: notice
end

View File

@@ -22,7 +22,7 @@ class Admin::SettingsController < Admin::BaseController
def update
@setting = Setting.find(params[:id])
@setting.update(settings_params)
@setting.update!(settings_params)
redirect_to request_referer, notice: t("admin.settings.flash.updated")
end
@@ -39,7 +39,7 @@ class Admin::SettingsController < Admin::BaseController
mime_type_values = content_type_params.keys.map do |content_type|
Setting.mime_types[group][content_type]
end
setting.update value: mime_type_values.join(" ")
setting.update! value: mime_type_values.join(" ")
redirect_to admin_settings_path, notice: t("admin.settings.flash.updated")
end

View File

@@ -44,7 +44,7 @@ class Admin::SiteCustomization::ContentBlocksController < Admin::SiteCustomizati
if is_heading_content_block?(params[:site_customization_content_block][:name])
heading_content_block = new_heading_content_block
if heading_content_block.save
@content_block.destroy
@content_block.destroy!
notice = t("admin.site_customization.content_blocks.create.notice")
redirect_to admin_site_customization_content_blocks_path, notice: notice
else
@@ -61,13 +61,13 @@ class Admin::SiteCustomization::ContentBlocksController < Admin::SiteCustomizati
end
def destroy
@content_block.destroy
@content_block.destroy!
notice = t("admin.site_customization.content_blocks.destroy.notice")
redirect_to admin_site_customization_content_blocks_path, notice: notice
end
def delete_heading_content_block
Budget::ContentBlock.find(params[:id]).destroy
Budget::ContentBlock.find(params[:id]).destroy!
notice = t("admin.site_customization.content_blocks.destroy.notice")
redirect_to admin_site_customization_content_blocks_path, notice: notice
end
@@ -101,7 +101,7 @@ class Admin::SiteCustomization::ContentBlocksController < Admin::SiteCustomizati
@content_block.locale = params[:locale]
@content_block.body = params[:body]
if @content_block.save
heading_content_block.destroy
heading_content_block.destroy!
notice = t("admin.site_customization.content_blocks.update.notice")
redirect_to admin_site_customization_content_blocks_path, notice: notice
else

View File

@@ -21,7 +21,7 @@ class Admin::SiteCustomization::DocumentsController < Admin::SiteCustomization::
def destroy
@document = Document.find(params[:id])
@document.destroy
@document.destroy!
notice = t("admin.documents.destroy.success_notice")
redirect_to admin_site_customization_documents_path, notice: notice

View File

@@ -17,9 +17,9 @@ class Admin::SiteCustomization::InformationTextsController < Admin::SiteCustomiz
if value == t(content[:id], locale: locale) || value.match(/translation missing/)
next
else
text = I18nContent.find_or_create_by(key: content[:id])
text = I18nContent.find_or_create_by!(key: content[:id])
Globalize.locale = locale
text.update(value: value)
text.update!(value: value)
end
end
end

View File

@@ -27,7 +27,7 @@ class Admin::SiteCustomization::PagesController < Admin::SiteCustomization::Base
end
def destroy
@page.destroy
@page.destroy!
notice = t("admin.site_customization.pages.destroy.notice")
redirect_to admin_site_customization_pages_path, notice: notice
end

View File

@@ -9,12 +9,12 @@ class Admin::TagsController < Admin::BaseController
end
def create
Tag.category.create(tag_params)
Tag.category.create!(tag_params)
redirect_to admin_tags_path
end
def destroy
@tag.destroy
@tag.destroy!
redirect_to admin_tags_path
end

View File

@@ -20,7 +20,7 @@ class Admin::TrackersController < Admin::BaseController
def create
@tracker = Tracker.new(tracker_params)
@tracker.save
@tracker.save!
redirect_to admin_trackers_path
end
@@ -40,7 +40,7 @@ class Admin::TrackersController < Admin::BaseController
end
def destroy
@tracker.destroy
@tracker.destroy!
redirect_to admin_trackers_path
end

View File

@@ -38,7 +38,7 @@ class Admin::ValuatorGroupsController < Admin::BaseController
def destroy
@group = ValuatorGroup.find(params[:id])
@group.destroy
@group.destroy!
notice = t("flash.actions.destroy.valuator_group")
redirect_to [:admin, :valuator_groups], notice: notice
end

View File

@@ -18,7 +18,7 @@ class Admin::ValuatorsController < Admin::BaseController
def create
@valuator = Valuator.new(valuator_params)
@valuator.save
@valuator.save!
redirect_to admin_valuators_path
end
@@ -39,7 +39,7 @@ class Admin::ValuatorsController < Admin::BaseController
end
def destroy
@valuator.destroy
@valuator.destroy!
redirect_to admin_valuators_path
end

View File

@@ -34,7 +34,7 @@ class Admin::Widget::CardsController < Admin::BaseController
def destroy
@card = ::Widget::Card.find(params[:id])
@card.destroy
@card.destroy!
redirect_to_customization_page_cards_or_homepage
end

View File

@@ -2,7 +2,7 @@ class Admin::Widget::FeedsController < Admin::BaseController
def update
@feed = ::Widget::Feed.find(params[:id])
@feed.update(feed_params)
@feed.update!(feed_params)
head :ok
end

View File

@@ -26,7 +26,7 @@ module Budgets
load_heading
load_map
@line.destroy
@line.destroy!
load_investments
end
@@ -41,7 +41,7 @@ module Budgets
end
def load_ballot
@ballot = Budget::Ballot.where(user: current_user, budget: @budget).first_or_create
@ballot = Budget::Ballot.where(user: current_user, budget: @budget).first_or_create!
end
def load_investment

View File

@@ -20,7 +20,7 @@ module Budgets
def load_ballot
query = Budget::Ballot.where(user: current_user, budget: @budget)
@ballot = @budget.balloting? ? query.first_or_create : query.first_or_initialize
@ballot = @budget.balloting? ? query.first_or_create! : query.first_or_initialize
end
def store_referer

View File

@@ -86,7 +86,7 @@ module Budgets
end
def destroy
@investment.destroy
@investment.destroy!
redirect_to user_path(current_user, filter: "budget_investments"), notice: t("flash.actions.destroy.budget_investment")
end
@@ -143,7 +143,7 @@ module Budgets
def load_ballot
query = Budget::Ballot.where(user: current_user, budget: @budget)
@ballot = @budget.balloting? ? query.first_or_create : query.first_or_initialize
@ballot = @budget.balloting? ? query.first_or_create! : query.first_or_initialize
end
def load_heading

View File

@@ -17,7 +17,7 @@ class Dashboard::ActionsController < Dashboard::BaseController
@dashboard_executed_action = Dashboard::ExecutedAction.new(source_params)
if @dashboard_executed_action.save
Dashboard::AdministratorTask.create(source: @dashboard_executed_action)
Dashboard::AdministratorTask.create!(source: @dashboard_executed_action)
redirect_to progress_proposal_dashboard_path(proposal.to_param),
{ flash: { info: t("dashboard.create_request.success") }}
@@ -38,7 +38,7 @@ class Dashboard::ActionsController < Dashboard::BaseController
def unexecute
authorize! :dashboard, proposal
Dashboard::ExecutedAction.where(proposal: proposal, action: dashboard_action).first.destroy
Dashboard::ExecutedAction.where(proposal: proposal, action: dashboard_action).first.destroy!
redirect_to request.referer
end

View File

@@ -42,7 +42,7 @@ class Dashboard::PollsController < Dashboard::BaseController
if ::Poll::Voter.where(poll: poll).any?
redirect_to proposal_dashboard_polls_path(proposal), alert: t("dashboard.polls.poll.unable_notice")
else
poll.destroy
poll.destroy!
redirect_to proposal_dashboard_polls_path(proposal), notice: t("dashboard.polls.poll.success_notice")
end

View File

@@ -3,14 +3,14 @@ class FollowsController < ApplicationController
load_and_authorize_resource
def create
@follow = Follow.create(user: current_user, followable: find_followable)
@follow = Follow.create!(user: current_user, followable: find_followable)
flash.now[:notice] = t("shared.followable.#{followable_translation_key(@follow.followable)}.create.notice")
render :refresh_follow_button
end
def destroy
@follow = Follow.find(params[:id])
@follow.destroy
@follow.destroy!
flash.now[:notice] = t("shared.followable.#{followable_translation_key(@follow.followable)}.destroy.notice")
render :refresh_follow_button
end

View File

@@ -11,7 +11,7 @@ class Legislation::AnswersController < Legislation::BaseController
def create
if @process.debate_phase.open?
@answer.user = current_user
@answer.save
@answer.save!
track_event
respond_to do |format|
format.js

View File

@@ -7,7 +7,7 @@ class Polls::AnswersController < ApplicationController
def create
@question = Poll::Question.find_by(id: params[:id])
if @question.votation_type.open? && !check_question_answer_exist
@question.question_answers.create(
@question.question_answers.create!(
title: params[:answer],
given_order: @question.question_answers.maximum(:given_order).to_i + 1,
hidden: false

View File

@@ -7,7 +7,7 @@ class RelatedContentsController < ApplicationController
if relationable_object && related_object
if relationable_object.url != related_object.url
RelatedContent.create(parent_relationable: @relationable, child_relationable: @related, author: current_user)
RelatedContent.create!(parent_relationable: @relationable, child_relationable: @related, author: current_user)
flash[:success] = t("related_content.success")
else

View File

@@ -6,7 +6,7 @@ class RemoteTranslationsController < ApplicationController
def create
@remote_translations.each do |remote_translation|
RemoteTranslation.create(remote_translation) unless translations_enqueued?(remote_translation)
RemoteTranslation.create!(remote_translation) unless translations_enqueued?(remote_translation)
end
redirect_to request.referer, notice: t("remote_translations.create.enqueue_remote_translation")
end

View File

@@ -40,7 +40,7 @@ class TopicsController < ApplicationController
end
def destroy
@topic.destroy
@topic.destroy!
redirect_to community_path(@community), notice: I18n.t("flash.actions.destroy.topic")
end

View File

@@ -35,7 +35,7 @@ class Tracking::MilestonesController < Tracking::BaseController
end
def destroy
@milestone.destroy
@milestone.destroy!
redirect_to milestoneable_path, notice: t("tracking.milestones.delete.notice")
end

View File

@@ -33,7 +33,7 @@ class Tracking::ProgressBarsController < Tracking::BaseController
end
def destroy
@progress_bar.destroy
@progress_bar.destroy!
redirect_to progress_bars_index, notice: t("tracking.progress_bars.delete.notice")
end

View File

@@ -9,7 +9,7 @@ class Users::ConfirmationsController < Devise::ConfirmationsController
resource.assign_attributes(resource_params)
if resource.valid? # password is set correctly
resource.save
resource.save!
set_official_position if resource.has_official_email?
resource.confirm
set_flash_message(:notice, :confirmed) if is_flashing_format?

View File

@@ -31,7 +31,7 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController
@user = current_user || identity.user || User.first_or_initialize_for_oauth(auth)
if save_user
identity.update(user: @user)
identity.update!(user: @user)
sign_in_and_redirect @user, event: :authentication
set_flash_message(:notice, :success, kind: provider.to_s.capitalize) if is_navigational_format?
else

View File

@@ -6,7 +6,7 @@ class Verification::EmailController < ApplicationController
def show
if Verification::Email.find(current_user, params[:email_verification_token])
current_user.update(verified_at: Time.current)
current_user.update!(verified_at: Time.current)
redirect_to account_path, notice: t("verification.email.show.flash.success")
else
redirect_to verified_user_path, alert: t("verification.email.show.alert.failure")

View File

@@ -29,7 +29,7 @@ class Verification::LetterController < ApplicationController
def update
@letter = Verification::Letter.new(letter_params.merge(user: current_user, verify: true))
if @letter.valid?
current_user.update(verified_at: Time.current)
current_user.update!(verified_at: Time.current)
redirect_to account_path, notice: t("verification.letter.update.flash.success")
else
Lock.increase_tries(@letter.user) if @letter.user

View File

@@ -27,7 +27,7 @@ class Verification::SmsController < ApplicationController
def update
@sms = Verification::Sms.new(sms_params.merge(user: current_user))
if @sms.verified?
current_user.update(confirmed_phone: current_user.unconfirmed_phone)
current_user.update!(confirmed_phone: current_user.unconfirmed_phone)
ahoy.track(:level_2_user, user_id: current_user.id) rescue nil
if VerifiedUser.phone?(current_user)