Update Rubocop version and enforce all cops (#617)

* Update Rubocop (and plugins) to latest versions * Autocorrect safe cops One rule was disabled since we don't want to alter already performed migrations. * Autocorrect unsafe cops Special care is needed for `tag.rb` around line 190. The autofix did not produce a valid result, I had to put extra curly braces around that line as we want to push an object to the array. * Remove unnecessary Unique validation RuboCop TODO They probably got unnecessary due to the update of RuboCop, though I haven't looked into their changelog to confirm.
MaMpf-HD · Apr 17, 2024 · dfdad5d · dfdad5d
1 parent 10de4ac
commit dfdad5d
Show file tree

Hide file tree

Showing 13 changed files with 62 additions and 64 deletions.
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -484,17 +484,25 @@ GEM
       rspec-mocks (~> 3.11)
       rspec-support (~> 3.11)
     rspec-support (3.12.0)
-    rubocop (1.63.0)
+    rubocop (1.63.1)
       json (~> 2.3)
       language_server-protocol (>= 3.17.0)
       parallel (~> 1.10)
       parser (>= 3.3.0.2)
+      parser (>= 3.3.0.2)
       rainbow (>= 2.2.2, < 4.0)
       regexp_parser (>= 1.8, < 3.0)
       rexml (>= 3.2.5, < 4.0)
       rubocop-ast (>= 1.31.1, < 2.0)
+      rubocop-ast (>= 1.31.1, < 2.0)
       ruby-progressbar (~> 1.7)
       unicode-display_width (>= 2.4.0, < 3.0)
+    rubocop-ast (1.31.2)
+      parser (>= 3.3.0.4)
+    rubocop-performance (1.21.0)
+      rubocop (>= 1.48.1, < 2.0)
+      rubocop-ast (>= 1.31.1, < 2.0)
+    rubocop-rails (2.24.1)
     rubocop-ast (1.31.2)
       parser (>= 3.3.0.4)
     rubocop-performance (1.21.0)
@@ -505,6 +513,7 @@ GEM
       rack (>= 1.1)
       rubocop (>= 1.33.0, < 2.0)
       rubocop-ast (>= 1.31.1, < 2.0)
+      rubocop-ast (>= 1.31.1, < 2.0)
     ruby-graphviz (1.2.5)
       rexml
     ruby-progressbar (1.13.0)

diff --git a/app/controllers/commontator/comments_controller.rb b/app/controllers/commontator/comments_controller.rb
@@ -156,7 +156,7 @@ def upvote
 
     # PUT /comments/1/downvote
     def downvote
-      security_transgression_unless(@comment.can_be_voted_on_by?(@commontator_user) && \
+      security_transgression_unless(@comment.can_be_voted_on_by?(@commontator_user) &&
                                     @comment.thread.config.comment_voting.to_sym == :ld)
 
       @comment.downvote_from(@commontator_user)

diff --git a/app/controllers/readers_controller.rb b/app/controllers/readers_controller.rb
@@ -21,9 +21,8 @@ def update_all
                           .map(&:commontator_thread)
     existing_readers = Reader.where(user: current_user, thread: threads)
     missing_thread_ids = threads.map(&:id) - existing_readers.pluck(:thread_id)
-    new_readers = []
-    missing_thread_ids.each do |t|
-      new_readers << Reader.new(thread_id: t, user: current_user)
+    new_readers = missing_thread_ids.map do |t|
+      Reader.new(thread_id: t, user: current_user)
     end
     Reader.import new_readers
     Reader.where(user: current_user, thread: threads).touch_all

diff --git a/app/helpers/media_helper.rb b/app/helpers/media_helper.rb
@@ -147,6 +147,6 @@ def edit_or_show_medium_path(medium)
   def external_link_description_not_empty(medium)
     # Uses link display name if not empty, otherwise falls back to the
     # link url itself.
-    (medium.external_link_description.presence || medium.external_reference_link)
+    medium.external_link_description.presence || medium.external_reference_link
   end
 end
diff --git a/app/models/manuscript.rb b/app/models/manuscript.rb
@@ -148,19 +148,16 @@ def create_or_update_chapter_items!
     attrs = [:medium_id, :pdf_destination, :section_id, :sort, :page, :description, :ref_number,
              :position, :quarantine]
     item_details = items.pluck(*attrs).map { |i| attrs.zip(i).to_h }
-    contents = []
-    @chapters.each do |c|
-      contents.push(
-        { medium_id: @medium.id,
-          pdf_destination: c["destination"],
-          section_id: nil,
-          sort: "chapter",
-          page: c["page"].to_i,
-          description: c["description"],
-          ref_number: c["label"],
-          position: nil,
-          quarantine: nil }
-      )
+    contents = @chapters.map do |c|
+      { medium_id: @medium.id,
+        pdf_destination: c["destination"],
+        section_id: nil,
+        sort: "chapter",
+        page: c["page"].to_i,
+        description: c["description"],
+        ref_number: c["label"],
+        position: nil,
+        quarantine: nil }
     end
     create_or_update_items!(contents, item_details, item_destinations,
                             item_id_map)
@@ -176,21 +173,18 @@ def create_or_update_section_items!
     attrs = [:medium_id, :pdf_destination, :section_id, :sort, :page, :description, :ref_number,
              :position, :quarantine]
     item_details = items.pluck(*attrs).map { |i| attrs.zip(i).to_h }
-    contents = []
     # NOTE: that sections get a position -1 in order to place them ahead
     # of all content items within themseleves in #script_items_by_position
-    @sections.each do |s|
-      contents.push(
-        { medium_id: @medium.id,
-          pdf_destination: s["destination"],
-          section_id: s["mampf_section"].id,
-          sort: "section",
-          page: s["page"].to_i,
-          description: s["description"],
-          ref_number: s["label"],
-          position: -1,
-          quarantine: nil }
-      )
+    contents = @sections.map do |s|
+      { medium_id: @medium.id,
+        pdf_destination: s["destination"],
+        section_id: s["mampf_section"].id,
+        sort: "section",
+        page: s["page"].to_i,
+        description: s["description"],
+        ref_number: s["label"],
+        position: -1,
+        quarantine: nil }
     end
     create_or_update_items!(contents, item_details, item_destinations,
                             item_id_map)
@@ -208,22 +202,19 @@ def create_or_update_content_items!(filter_boxes)
     attrs = [:medium_id, :pdf_destination, :section_id, :sort, :page, :description, :ref_number,
              :position, :hidden, :quarantine]
     item_details = items.pluck(*attrs).map { |i| attrs.zip(i).to_h }
-    contents = []
-    @content.each do |c|
-      contents.push(
-        { medium_id: @medium.id,
-          pdf_destination: c["destination"],
-          section_id: @sections.find do |s|
-                        c["section"] == s["section"]
-                      end ["mampf_section"]&.id,
-          sort: Item.internal_sort(c["sort"]),
-          page: c["page"].to_i,
-          description: c["description"],
-          ref_number: c["label"],
-          position: c["counter"],
-          hidden: filter_boxes[c["counter"]].third == false,
-          quarantine: nil }
-      )
+    contents = @content.map do |c|
+      { medium_id: @medium.id,
+        pdf_destination: c["destination"],
+        section_id: @sections.find do |s|
+                      c["section"] == s["section"]
+                    end ["mampf_section"]&.id,
+        sort: Item.internal_sort(c["sort"]),
+        page: c["page"].to_i,
+        description: c["description"],
+        ref_number: c["label"],
+        position: c["counter"],
+        hidden: filter_boxes[c["counter"]].third == false,
+        quarantine: nil }
     end
     create_or_update_items!(contents, item_details, item_destinations,
                             item_id_map)

diff --git a/app/models/medium_publisher.rb b/app/models/medium_publisher.rb
@@ -128,13 +128,12 @@ def realize_optional_stuff!
     # to the medium's teachable's media_scope
     def create_notifications!
       @medium.teachable&.media_scope&.touch
-      notifications = []
       @medium.teachable.media_scope.users.touch_all
-      @medium.teachable.media_scope.users.each do |u|
-        notifications << Notification.new(recipient: u,
-                                          notifiable_id: @medium.id,
-                                          notifiable_type: "Medium",
-                                          action: "create")
+      notifications = @medium.teachable.media_scope.users.map do |u|
+        Notification.new(recipient: u,
+                         notifiable_id: @medium.id,
+                         notifiable_type: "Medium",
+                         action: "create")
       end
       Notification.import notifications
     end

diff --git a/app/models/notion.rb b/app/models/notion.rb
@@ -2,7 +2,7 @@ class Notion < ApplicationRecord
   belongs_to :tag, optional: true, touch: true
   belongs_to :aliased_tag, class_name: "Tag", optional: true, touch: true
 
-  validates :title, uniqueness: { scope: :locale } # rubocop:todo Rails/UniqueValidationWithoutIndex
+  validates :title, uniqueness: { scope: :locale }
   validates :title, presence: true
   validate :presence_of_tag, if: :persisted?
 

diff --git a/app/models/quiz.rb b/app/models/quiz.rb
@@ -115,7 +115,7 @@ def preselected_hide_solution(vertex_id, crosses)
 
   def questions
     ids = quiz_graph&.vertices&.values&.select { |v| v[:type] == "Question" }
-                    &.map { |v| v[:id] }
+                    &.pluck(:id)
     Question.where(id: ids)
   end
 

diff --git a/app/models/referral.rb b/app/models/referral.rb
@@ -33,7 +33,7 @@ def vtt_time_span
 
   # provide metadata for vtt file
   def vtt_properties
-    link = (item.link.presence || item.medium_link)
+    link = item.link.presence || item.medium_link
     # at the moment, relations between items can be only of the form
     # script <-> video, which means that between them there will be at most
     # one script, one manuscript and one video

diff --git a/app/models/submission.rb b/app/models/submission.rb
@@ -63,7 +63,7 @@ def correction_size
   end
 
   def preceding_tutorial(user)
-    assignment.previous&.map { |a| a.tutorial(user) }&.compact&.first
+    assignment.previous&.filter_map { |a| a.tutorial(user) }&.first
   end
 
   def invited_users

diff --git a/app/models/tag.rb b/app/models/tag.rb
@@ -184,13 +184,11 @@ def self.select_by_title_except(excluded_tags)
   # converts the subgraph of all tags of distance <= 2 to the given marked tag
   # into a cytoscape array representing this subgraph
   def self.to_cytoscape(tags, marked_tag, highlight_related_tags: true)
-    result = []
     # add vertices
-    tags.each do |t|
-      result.push(data: t.cytoscape_vertex(marked_tag,
-                                           highlight_related_tags:
-                                             highlight_related_tags))
+    result = tags.map do |t|
+      { data: t.cytoscape_vertex(marked_tag, highlight_related_tags: highlight_related_tags) }
     end
+
     # add edges
     edges = []
     tags.each do |t|

diff --git a/app/models/term.rb b/app/models/term.rb
@@ -4,7 +4,7 @@ class Term < ApplicationRecord
   has_many :lectures
 
   # season can only be SS/WS, and there can be only one of this type each year
-  validates :season, presence: true, # rubocop:todo Rails/UniqueValidationWithoutIndex
+  validates :season, presence: true,
                      inclusion: { in: ["SS", "WS"] },
                      uniqueness: { scope: :year }
   # a year >=2000 needs to be present

diff --git a/db/migrate/20201002094500_change_submission_foreign_keys.rb b/db/migrate/20201002094500_change_submission_foreign_keys.rb
@@ -1,4 +1,5 @@
 # rubocop:disable Rails/
+# rubocop:disable Lint/SymbolConversion
 class ChangeSubmissionForeignKeys < ActiveRecord::Migration[6.0]
   def up
     remove_index :user_submission_joins,
@@ -33,3 +34,4 @@ def id_to_uuid(table_name, relation_name, relation_class)
   end
 end
 # rubocop:enable Rails/
+# rubocop:enable Lint/SymbolConversion