RuboCop: Style/IdenticalConditionalBranches

auto-corrected, with post review looking for important side effects
of the conditional (one found).

Change-Id: I0fb09793a8645fcac8d81974de9d18a167d2b710
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/278946
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>

.rubocop.common.yml

@@ -229,6 +229,8 @@ Style/HashTransformKeys:
   Severity: error
 Style/HashTransformValues:
   Severity: error
+Style/IdenticalConditionalBranches:
+  Severity: error
 Style/InverseMethods:
   Severity: error
 Style/IfUnlessModifier:

app/controllers/assignments_controller.rb

@@ -483,12 +483,11 @@ class AssignmentsController < ApplicationController
       @request = AssessmentRequest.where(id: params[:id]).first if params[:id].present?
       respond_to do |format|
         if @request.asset.assignment == @assignment && @request.send_reminder!
-          format.html { redirect_to named_context_url(@context, :context_assignment_peer_reviews_url) }
           format.json { render :json => @request }
         else
-          format.html { redirect_to named_context_url(@context, :context_assignment_peer_reviews_url) }
           format.json { render :json => { :errors => { :base => t('errors.reminder_failed', "Reminder failed") } }, :status => :bad_request }
         end
+        format.html { redirect_to named_context_url(@context, :context_assignment_peer_reviews_url) }
       end
     end
   end
@@ -499,12 +498,11 @@ class AssignmentsController < ApplicationController
       @request = AssessmentRequest.where(id: params[:id]).first if params[:id].present?
       respond_to do |format|
         if @request.asset.assignment == @assignment && @request.destroy
-          format.html { redirect_to named_context_url(@context, :context_assignment_peer_reviews_url) }
           format.json { render :json => @request }
         else
-          format.html { redirect_to named_context_url(@context, :context_assignment_peer_reviews_url) }
           format.json { render :json => { :errors => { :base => t('errors.delete_reminder_failed', "Delete failed") } }, :status => :bad_request }
         end
+        format.html { redirect_to named_context_url(@context, :context_assignment_peer_reviews_url) }
       end
     end
   end

app/controllers/eportfolio_entries_controller.rb

@@ -84,12 +84,11 @@ class EportfolioEntriesController < ApplicationController
       end
       respond_to do |format|
         if @entry.update!(entry_params)
-          format.html { redirect_to eportfolio_entry_url(@portfolio, @entry) }
           format.json { render :json => @entry }
         else
-          format.html { redirect_to eportfolio_entry_url(@portfolio, @entry) }
           format.json { render :json => @entry.errors, :status => :bad_request }
         end
+        format.html { redirect_to eportfolio_entry_url(@portfolio, @entry) }
       end
     end
   end

app/controllers/groups_controller.rb

@@ -397,12 +397,11 @@ class GroupsController < ApplicationController
           @group.request_user(@current_user)
           if !@group.grants_right?(@current_user, session, :read)
             render :membership_pending
-            return
           else
             flash[:notice] = t('notices.welcome', "Welcome to the group %{group_name}!", :group_name => @group.name)
             redirect_to named_context_url(@group.context, :context_groups_url)
-            return
           end
+          return
         end
         if params[:leave] && @group.grants_right?(@current_user, :leave)
           membership = @group.membership_for_user(@current_user)

app/models/account.rb

@@ -1754,8 +1754,8 @@ class Account < ActiveRecord::Base
 
   def tabs_available(user = nil, opts = {})
     manage_settings = user && self.grants_right?(user, :manage_account_settings)
+    tabs = []
     if root_account.site_admin?
-      tabs = []
       tabs << { :id => TAB_USERS, :label => t("People"), :css_class => 'users', :href => :account_users_path } if user && self.grants_right?(user, :read_roster)
       tabs << { :id => TAB_PERMISSIONS, :label => t('#account.tab_permissions', "Permissions"), :css_class => 'permissions', :href => :account_permissions_path } if user && self.grants_right?(user, :manage_role_overrides)
       tabs << { :id => TAB_SUB_ACCOUNTS, :label => t('#account.tab_sub_accounts', "Sub-Accounts"), :css_class => 'sub_accounts', :href => :account_sub_accounts_path } if manage_settings
@@ -1764,7 +1764,6 @@ class Account < ActiveRecord::Base
       tabs << { :id => TAB_RELEASE_NOTES, :label => t("Release Notes"), :css_class => "release_notes", :href => :account_release_notes_manage_path } if root_account? && ReleaseNote.enabled? && self.grants_right?(user, :manage_release_notes)
       tabs << { :id => TAB_JOBS, :label => t("#account.tab_jobs", "Jobs"), :css_class => "jobs", :href => :jobs_path, :no_args => true } if root_account? && self.grants_right?(user, :view_jobs)
     else
-      tabs = []
       tabs << { :id => TAB_COURSES, :label => t('#account.tab_courses', "Courses"), :css_class => 'courses', :href => :account_path } if user && self.grants_right?(user, :read_course_list)
       tabs << { :id => TAB_USERS, :label => t("People"), :css_class => 'users', :href => :account_users_path } if user && self.grants_right?(user, :read_roster)
       tabs << { :id => TAB_STATISTICS, :label => t('#account.tab_statistics', "Statistics"), :css_class => 'statistics', :href => :statistics_account_path } if user && self.grants_right?(user, :view_statistics)
@@ -1860,9 +1859,10 @@ class Account < ActiveRecord::Base
     raise "Invalid Service" unless AccountServices.allowable_services[service]
 
     allowed_service_names = (self.allowed_services || "").split(",").compact
+    # rubocop:disable Style/IdenticalConditionalBranches common line needs to happen after the conditional
     if allowed_service_names.count > 0 && !['+', '-'].include?(allowed_service_names[0][0, 1])
-      # This account has a hard-coded list of services, so handle accordingly
       allowed_service_names.reject! { |flag| flag.match("^[+-]?#{service}$") }
+      # This account has a hard-coded list of services, so handle accordingly
       allowed_service_names << service if enable
     else
       allowed_service_names.reject! { |flag| flag.match("^[+-]?#{service}$") }
@@ -1874,6 +1874,7 @@ class Account < ActiveRecord::Base
         allowed_service_names << "-#{service}" if AccountServices.default_allowable_services[service]
       end
     end
+    # rubocop:enable Style/IdenticalConditionalBranches
 
     @allowed_services_hash = nil
     self.allowed_services = allowed_service_names.empty? ? nil : allowed_service_names.join(",")

app/models/context_module.rb

@@ -665,8 +665,6 @@ class ContextModule < ActiveRecord::Base
       added_item.context_module_id = self.id
       added_item.indent = params[:indent] || 0
       added_item.workflow_state = 'unpublished' if added_item.new_record?
-      added_item.save
-      added_item
     when 'context_external_tool', 'external_tool', 'lti/message_handler'
       title = params[:title]
       added_item ||= self.content_tags.build(:context => self.context)
@@ -704,8 +702,6 @@ class ContextModule < ActiveRecord::Base
             context_external_tool: content
           )
       end
-      added_item.save
-      added_item
     when 'context_module_sub_header', 'sub_header'
       title = params[:title]
       added_item ||= self.content_tags.build(:context => self.context)
@@ -720,8 +716,6 @@ class ContextModule < ActiveRecord::Base
       added_item.context_module_id = self.id
       added_item.indent = params[:indent] || 0
       added_item.workflow_state = 'unpublished' if added_item.new_record?
-      added_item.save
-      added_item
     else
       return nil unless item
 
@@ -737,9 +731,9 @@ class ContextModule < ActiveRecord::Base
       added_item.context_module_id = self.id
       added_item.indent = params[:indent] || 0
       added_item.workflow_state = workflow_state if added_item.new_record?
-      added_item.save
-      added_item
     end
+    added_item.save
+    added_item
   end
 
   # specify a 1-based position to insert the items at; leave nil to append to the end of the module

app/models/discussion_topic.rb

@@ -1105,11 +1105,10 @@ class DiscussionTopic < ActiveRecord::Base
 
   def unlink!(type)
     @saved_by = type
+    self.assignment = nil
     if self.discussion_entries.empty?
-      self.assignment = nil
       self.destroy
     else
-      self.assignment = nil
       self.save
     end
     self.child_topics.each { |t| t.unlink!(:assignment) }

app/models/incoming_mail/message_handler.rb

@@ -102,42 +102,37 @@ module IncomingMail
     end
 
     def bounce_message_strings(subject, error)
-      ndr_subject = ""
-      ndr_body = ""
-      case error
-      when IncomingMail::Errors::ReplyToDeletedDiscussion
-        ndr_subject = I18n.t("Undelivered message")
-        ndr_body = I18n.t(<<~TEXT, :subject => subject).gsub(/^ +/, '')
-          The message titled "%{subject}" could not be delivered because the discussion topic has been deleted. If you are trying to contact someone through Canvas you can try logging in to your account and sending them a message using the Inbox tool.
+      ndr_subject = I18n.t("Undelivered message")
+      ndr_body = case error
+                 when IncomingMail::Errors::ReplyToDeletedDiscussion
+                   I18n.t(<<~TEXT, :subject => subject).gsub(/^ +/, '')
+                     The message titled "%{subject}" could not be delivered because the discussion topic has been deleted. If you are trying to contact someone through Canvas you can try logging in to your account and sending them a message using the Inbox tool.
 
-          Thank you,
-          Canvas Support
-        TEXT
-      when IncomingMail::Errors::ReplyToLockedTopic
-        ndr_subject = I18n.t("Undelivered message")
-        ndr_body = I18n.t('lib.incoming_message_processor.locked_topic.body', <<~TEXT, :subject => subject).gsub(/^ +/, '')
-          The message titled "%{subject}" could not be delivered because the discussion topic is locked. If you are trying to contact someone through Canvas you can try logging in to your account and sending them a message using the Inbox tool.
+                     Thank you,
+                     Canvas Support
+                   TEXT
+                 when IncomingMail::Errors::ReplyToLockedTopic
+                   I18n.t('lib.incoming_message_processor.locked_topic.body', <<~TEXT, :subject => subject).gsub(/^ +/, '')
+                     The message titled "%{subject}" could not be delivered because the discussion topic is locked. If you are trying to contact someone through Canvas you can try logging in to your account and sending them a message using the Inbox tool.
 
-          Thank you,
-          Canvas Support
-        TEXT
-      when IncomingMail::Errors::UnknownSender
-        ndr_subject = I18n.t("Undelivered message")
-        ndr_body = I18n.t(<<~TEXT, :subject => subject, :link => I18n.t(:"community.guides_home")).gsub(/^ +/, '')
-          The message you sent with the subject line "%{subject}" was not delivered. To reply to Canvas messages from this email, it must first be a confirmed communication channel in your Canvas profile. Please visit your profile and resend the confirmation email for this email address. You may also contact this person via the Canvas Inbox. For help, please see the Inbox chapter for your user role in the Canvas Guides. [See %{link}].
+                     Thank you,
+                     Canvas Support
+                   TEXT
+                 when IncomingMail::Errors::UnknownSender
+                   I18n.t(<<~TEXT, :subject => subject, :link => I18n.t(:"community.guides_home")).gsub(/^ +/, '')
+                     The message you sent with the subject line "%{subject}" was not delivered. To reply to Canvas messages from this email, it must first be a confirmed communication channel in your Canvas profile. Please visit your profile and resend the confirmation email for this email address. You may also contact this person via the Canvas Inbox. For help, please see the Inbox chapter for your user role in the Canvas Guides. [See %{link}].
 
-          Thank you,
-          Canvas Support
-        TEXT
-      else # including IncomingMessageProcessor::UnknownAddressError
-        ndr_subject = I18n.t("Undelivered message")
-        ndr_body = I18n.t('lib.incoming_message_processor.failure_message.body', <<~TEXT, :subject => subject).gsub(/^ +/, '')
-          The message titled "%{subject}" could not be delivered.  The message was sent to an unknown mailbox address.  If you are trying to contact someone through Canvas you can try logging in to your account and sending them a message using the Inbox tool.
+                     Thank you,
+                     Canvas Support
+                   TEXT
+                 else # including IncomingMessageProcessor::UnknownAddressError
+                   I18n.t('lib.incoming_message_processor.failure_message.body', <<~TEXT, :subject => subject).gsub(/^ +/, '')
+                     The message titled "%{subject}" could not be delivered.  The message was sent to an unknown mailbox address.  If you are trying to contact someone through Canvas you can try logging in to your account and sending them a message using the Inbox tool.
 
-          Thank you,
-          Canvas Support
-        TEXT
-      end
+                     Thank you,
+                     Canvas Support
+                   TEXT
+                 end
 
       [ndr_subject, ndr_body]
     end

app/models/quizzes/quiz.rb

@@ -305,11 +305,10 @@ class Quizzes::Quiz < ActiveRecord::Base
 
   def unlink!(type)
     @saved_by = type
+    self.assignment = nil
     if self.root_entries.empty? && !self.available?
-      self.assignment = nil
       self.destroy
     else
-      self.assignment = nil
       self.save
     end
   end

app/models/submission.rb

@@ -399,13 +399,10 @@ class Submission < ActiveRecord::Base
     if self.submission_type == "online_quiz" && self.workflow_state == "graded"
       # unless it's an auto-graded quiz
       return unless self.workflow_state_before_last_save == "unsubmitted"
-
-      PlannerHelper.complete_planner_override_for_submission(self)
     else
       return unless self.workflow_state == "submitted"
-
-      PlannerHelper.complete_planner_override_for_submission(self)
     end
+    PlannerHelper.complete_planner_override_for_submission(self)
   end
 
   attr_reader :group_broadcast_submission

gems/attachment_fu/lib/attachment_fu/backends/file_system_backend.rb

@@ -74,12 +74,10 @@ module AttachmentFu # :nodoc:
 
       def filename=(value)
         value = sanitize_filename(value)
-        if self.new_record?
-          write_attribute :filename, value
-        else
-          @old_filename = full_filename unless filename.nil? || @old_filename
-          write_attribute :filename, value
+        if !new_record? && !(filename.nil? || @old_filename)
+          @old_filename = full_filename
         end
+        write_attribute :filename, value
       end
 
       def sanitize_filename(filename)

lib/active_support/cache/safe_redis_race_condition.rb

@@ -57,7 +57,6 @@ module ActiveSupport::Cache::SafeRedisRaceCondition
           break
         end
       end
-      entry
     else
       if entry.expired? && (lock_nonce = lock(lock_key, options))
         @safe_redis_internal_options[:lock_nonce] = lock_nonce
@@ -66,8 +65,8 @@ module ActiveSupport::Cache::SafeRedisRaceCondition
       end
       # just return the stale value; someone else is busy
       # regenerating it
-      entry
     end
+    entry
   end
 
   # this is originally defined in ActiveSupport::Cache::Store,

lib/data_fixup/populate_root_account_id_on_user_observers.rb

@@ -32,14 +32,12 @@ module DataFixup::PopulateRootAccountIdOnUserObservers
         # for the unlikely scenario that somehow an equivalent link was made after the deploy but before the fixup finished
         new_id = -1 * retry_count # just in case
         updates = { :root_account_id => new_id, :workflow_state => 'deleted' }
-        shadow&.update(updates)
-        link.update(updates)
       else
         updates = { :root_account_id => root_account_id }
         updates[:workflow_state] = 'deleted' if destroy
-        shadow&.update(updates)
-        link.update(updates)
       end
+      shadow&.update(updates)
+      link.update(updates)
     end
   end
 end

lib/text_helper.rb

@@ -177,14 +177,13 @@ module TextHelper
           current.add_previous_sibling(truncate_elem)
           new_node = Nokogiri::XML::Text.new(new_content.join(' '), doc)
           truncate_elem.add_previous_sibling(new_node)
-          current = truncate_elem
         else
           current = previous
           # why would we do this next line? it just ends up xml escaping stuff
           # current.content = current.content
           current.add_next_sibling(truncate_elem)
-          current = truncate_elem
         end
+        current = truncate_elem
       end
 
       # remove everything else

spec/selenium/grades/gradebook/excuse_assignment_spec.rb

@@ -184,13 +184,12 @@ describe 'Excuse an Assignment' do
             wait_for_ajaximations
             score_values << f('#student_and_assignment_grade').attribute('value')
           end
-          expect(score_values).to eq ['15', 'Excused', '15', '15']
         else
           get "/courses/#{@course.id}/gradebook/"
           wait_for_ajaximations
           score_values = ff('.canvas_1 .slick-row .slick-cell:first-child').map(& :text)
-          expect(score_values).to eq ['15', "Excused", '15', '15']
         end
+        expect(score_values).to eq ['15', 'Excused', '15', '15']
       end
 
       it 'excuses assignments on individual basis', priority: "1", test_id: view == 'srgb' ? 209405 : 209384 do