Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Installed rubocop-performance gem and made necessary adjustments #3288

Merged
merged 7 commits into from
Mar 23, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@
#
# Try to place any new Cops under their relevant section and in alphabetical order

require:
# - rubocop-rails
# - rubocop-rspec
- rubocop-performance

AllCops:
# Show the name of the cops being voilated in the feedback
DisplayCopNames: true
Expand Down
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ For more detailed explanation, please refer to this video : https://www.youtube.
- Cleaned up `spec/rails_helper.rb` and `spec/spec_helper.rb`
- Simplified the `spec/support/capybara.rb` helper to work with the latest version of Capybara and use its built in headless Chrome driver

### Added Rubocop performance gem
- Installed rubocop-performance gem and made suggested changes

### Bug Fixes


## v4.0.2

### Added
Expand Down
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,9 @@ group :ci, :development do
# RuboCop rules for detecting and autocorrecting undecorated strings for i18n
# (gettext and rails-i18n)
gem 'rubocop-i18n'

# Performance checks by Rubocop
gem 'rubocop-performance', require: false
end

group :development do
Expand Down
132 changes: 68 additions & 64 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,63 +1,63 @@
GEM
remote: https://rubygems.org/
specs:
actioncable (6.1.7.2)
actionpack (= 6.1.7.2)
activesupport (= 6.1.7.2)
actioncable (6.1.7.3)
actionpack (= 6.1.7.3)
activesupport (= 6.1.7.3)
nio4r (~> 2.0)
websocket-driver (>= 0.6.1)
actionmailbox (6.1.7.2)
actionpack (= 6.1.7.2)
activejob (= 6.1.7.2)
activerecord (= 6.1.7.2)
activestorage (= 6.1.7.2)
activesupport (= 6.1.7.2)
actionmailbox (6.1.7.3)
actionpack (= 6.1.7.3)
activejob (= 6.1.7.3)
activerecord (= 6.1.7.3)
activestorage (= 6.1.7.3)
activesupport (= 6.1.7.3)
mail (>= 2.7.1)
actionmailer (6.1.7.2)
actionpack (= 6.1.7.2)
actionview (= 6.1.7.2)
activejob (= 6.1.7.2)
activesupport (= 6.1.7.2)
actionmailer (6.1.7.3)
actionpack (= 6.1.7.3)
actionview (= 6.1.7.3)
activejob (= 6.1.7.3)
activesupport (= 6.1.7.3)
mail (~> 2.5, >= 2.5.4)
rails-dom-testing (~> 2.0)
actionpack (6.1.7.2)
actionview (= 6.1.7.2)
activesupport (= 6.1.7.2)
actionpack (6.1.7.3)
actionview (= 6.1.7.3)
activesupport (= 6.1.7.3)
rack (~> 2.0, >= 2.0.9)
rack-test (>= 0.6.3)
rails-dom-testing (~> 2.0)
rails-html-sanitizer (~> 1.0, >= 1.2.0)
actiontext (6.1.7.2)
actionpack (= 6.1.7.2)
activerecord (= 6.1.7.2)
activestorage (= 6.1.7.2)
activesupport (= 6.1.7.2)
actiontext (6.1.7.3)
actionpack (= 6.1.7.3)
activerecord (= 6.1.7.3)
activestorage (= 6.1.7.3)
activesupport (= 6.1.7.3)
nokogiri (>= 1.8.5)
actionview (6.1.7.2)
activesupport (= 6.1.7.2)
actionview (6.1.7.3)
activesupport (= 6.1.7.3)
builder (~> 3.1)
erubi (~> 1.4)
rails-dom-testing (~> 2.0)
rails-html-sanitizer (~> 1.1, >= 1.2.0)
activejob (6.1.7.2)
activesupport (= 6.1.7.2)
activejob (6.1.7.3)
activesupport (= 6.1.7.3)
globalid (>= 0.3.6)
activemodel (6.1.7.2)
activesupport (= 6.1.7.2)
activerecord (6.1.7.2)
activemodel (= 6.1.7.2)
activesupport (= 6.1.7.2)
activemodel (6.1.7.3)
activesupport (= 6.1.7.3)
activerecord (6.1.7.3)
activemodel (= 6.1.7.3)
activesupport (= 6.1.7.3)
activerecord_json_validator (2.1.3)
activerecord (>= 4.2.0, < 8)
json_schemer (~> 0.2.18)
activestorage (6.1.7.2)
actionpack (= 6.1.7.2)
activejob (= 6.1.7.2)
activerecord (= 6.1.7.2)
activesupport (= 6.1.7.2)
activestorage (6.1.7.3)
actionpack (= 6.1.7.3)
activejob (= 6.1.7.3)
activerecord (= 6.1.7.3)
activesupport (= 6.1.7.3)
marcel (~> 1.0)
mini_mime (>= 1.1.0)
activesupport (6.1.7.2)
activesupport (6.1.7.3)
concurrent-ruby (~> 1.0, >= 1.0.2)
i18n (>= 1.6, < 2)
minitest (>= 5.1)
Expand Down Expand Up @@ -134,9 +134,9 @@ GEM
no_proxy_fix
octokit (~> 5.0)
terminal-table (>= 1, < 4)
database_cleaner (2.0.1)
database_cleaner-active_record (~> 2.0.0)
database_cleaner-active_record (2.0.1)
database_cleaner (2.0.2)
database_cleaner-active_record (>= 2, < 3)
database_cleaner-active_record (2.1.0)
activerecord (>= 5.a)
database_cleaner-core (~> 2.0.0)
database_cleaner-core (2.0.1)
Expand Down Expand Up @@ -207,7 +207,7 @@ GEM
locale (>= 2.0.5)
prime
text (>= 1.3.0)
git (1.17.2)
git (1.18.0)
addressable (~> 2.8)
rchardet (~> 1.8)
globalid (1.1.0)
Expand Down Expand Up @@ -351,27 +351,27 @@ GEM
pundit-matchers (1.8.4)
rspec-rails (>= 3.0.0)
racc (1.6.2)
rack (2.2.6.3)
rack (2.2.6.4)
rack-mini-profiler (3.0.0)
rack (>= 1.2.0)
rack-protection (3.0.5)
rack
rack-test (2.0.2)
rack-test (2.1.0)
rack (>= 1.3)
rails (6.1.7.2)
actioncable (= 6.1.7.2)
actionmailbox (= 6.1.7.2)
actionmailer (= 6.1.7.2)
actionpack (= 6.1.7.2)
actiontext (= 6.1.7.2)
actionview (= 6.1.7.2)
activejob (= 6.1.7.2)
activemodel (= 6.1.7.2)
activerecord (= 6.1.7.2)
activestorage (= 6.1.7.2)
activesupport (= 6.1.7.2)
rails (6.1.7.3)
actioncable (= 6.1.7.3)
actionmailbox (= 6.1.7.3)
actionmailer (= 6.1.7.3)
actionpack (= 6.1.7.3)
actiontext (= 6.1.7.3)
actionview (= 6.1.7.3)
activejob (= 6.1.7.3)
activemodel (= 6.1.7.3)
activerecord (= 6.1.7.3)
activestorage (= 6.1.7.3)
activesupport (= 6.1.7.3)
bundler (>= 1.15.0)
railties (= 6.1.7.2)
railties (= 6.1.7.3)
sprockets-rails (>= 2.0.0)
rails-controller-testing (1.0.5)
actionpack (>= 5.0.1.rc1)
Expand All @@ -382,9 +382,9 @@ GEM
nokogiri (>= 1.6)
rails-html-sanitizer (1.5.0)
loofah (~> 2.19, >= 2.19.1)
railties (6.1.7.2)
actionpack (= 6.1.7.2)
activesupport (= 6.1.7.2)
railties (6.1.7.3)
actionpack (= 6.1.7.3)
activesupport (= 6.1.7.3)
method_source
rake (>= 12.2)
thor (~> 1.0)
Expand All @@ -409,7 +409,7 @@ GEM
rspec-expectations (3.12.2)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.12.0)
rspec-mocks (3.12.3)
rspec-mocks (3.12.4)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.12.0)
rspec-rails (6.0.1)
Expand All @@ -421,7 +421,7 @@ GEM
rspec-mocks (~> 3.11)
rspec-support (~> 3.11)
rspec-support (3.12.0)
rubocop (1.48.0)
rubocop (1.48.1)
json (~> 2.3)
parallel (~> 1.10)
parser (>= 3.2.0.0)
Expand All @@ -435,6 +435,9 @@ GEM
parser (>= 3.2.1.0)
rubocop-i18n (3.0.0)
rubocop (~> 1.0)
rubocop-performance (1.16.0)
rubocop (>= 1.7.0, < 2.0)
rubocop-ast (>= 0.4.0)
ruby-progressbar (1.13.0)
ruby2_keywords (0.0.5)
ruby_dig (0.0.2)
Expand Down Expand Up @@ -486,7 +489,7 @@ GEM
unicode-display_width (2.4.2)
uniform_notifier (1.16.0)
uri_template (0.7.0)
version_gem (1.1.1)
version_gem (1.1.2)
warden (1.2.9)
rack (>= 2.0.9)
web-console (4.2.0)
Expand Down Expand Up @@ -582,6 +585,7 @@ DEPENDENCIES
rspec-rails
rubocop
rubocop-i18n
rubocop-performance
shoulda
spring
spring-commands-rspec
Expand All @@ -598,7 +602,7 @@ DEPENDENCIES
yard-tomdoc

RUBY VERSION
ruby 3.0.5p211
ruby 3.0.4p208

BUNDLED WITH
2.4.6
2.4.8
4 changes: 2 additions & 2 deletions app/controllers/answers_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,8 @@ def permitted_params
# rubocop:enable Metrics/AbcSize

def check_answered(section, q_array, all_answers)
n_qs = section.questions.select { |question| q_array.include?(question.id) }.length
n_ans = all_answers.select { |ans| q_array.include?(ans.question.id) and ans.answered? }.length
n_qs = section.questions.count { |question| q_array.include?(question.id) }
n_ans = all_answers.count { |ans| q_array.include?(ans.question.id) and ans.answered? }
[n_qs, n_ans]
end
end
2 changes: 1 addition & 1 deletion app/controllers/api/v0/plans_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ def index
if params['updated_after'].present? || params['updated_before'].present?
@plans = @plans.where(updated_at: dates_to_range(params, 'updated_after', 'updated_before'))
end
if params['remove_tests'].present? && params['remove_tests'].downcase == 'true'
if params['remove_tests'].present? && params['remove_tests'].casecmp('true').zero?
@plans = @plans.where.not(visibility: Plan.visibilities[:is_test])
end
# filter on funder (dmptemplate_id)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v0/statistics_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ def plans
raise Pundit::NotAuthorizedError unless Api::V0::StatisticsPolicy.new(@user, :statistics).plans?

@org_plans = @user.org.plans
if params['remove_tests'].present? && params['remove_tests'].downcase == 'true'
if params['remove_tests'].present? && params['remove_tests'].casecmp('true').zero?
@org_plans = @org_plans.where.not(visibility: Plan.visibilities[:is_test])
end
if params['start_date'].present? || params['end_date'].present?
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/v1/plans_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def plan_exists?(json:)

# Get the Plan's owner
def determine_owner(client:, plan:)
contact = plan.contributors.select(&:data_curation?).first
contact = plan.contributors.find(&:data_curation?)
# Use the contact if it was sent in and has an affiliation defined
return contact if contact.present? && contact.org.present?

Expand Down
4 changes: 2 additions & 2 deletions app/controllers/concerns/conditional_user_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ module ConditionalUserMailer
#
# Returns Boolean
def deliver_if(key:, recipients: [], &block)
return false unless block_given?
return false unless block

Array(recipients).each do |recipient|
email_hash = recipient.get_preferences('email').with_indifferent_access
# Violation of rubocop's DoubleNegation check
# preference_value = !!email_hash.dig(*key.to_s.split("."))
preference_value = email_hash.dig(*key.to_s.split('.'))
block.call(recipient) if preference_value
yield(recipient) if preference_value
end

true
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/org_admin/plans_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def download_plans
raise Pundit::NotAuthorizedError unless current_user.present? && current_user.can_org_admin?

org = current_user.org
file_name = org.name.gsub(/ /, '_')
file_name = org.name.tr(' ', '_')
.gsub(/[.;,]/, '')
header_cols = [
_('Project title').to_s,
Expand Down
8 changes: 4 additions & 4 deletions app/controllers/org_admin/templates_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class TemplatesController < ApplicationController
def index
authorize Template
templates = Template.latest_version.where(customization_of: nil)
published = templates.select { |t| t.published? || t.draft? }.length
published = templates.count { |t| t.published? || t.draft? }

@orgs = Org.includes(:identifiers).managed
@title = _('All Templates')
Expand All @@ -40,7 +40,7 @@ def organisational
authorize Template
templates = Template.latest_version_per_org(current_user.org.id)
.where(customization_of: nil, org_id: current_user.org.id)
published = templates.select { |t| t.published? || t.draft? }.length
published = templates.count { |t| t.published? || t.draft? }

@orgs = current_user.can_super_admin? ? Org.includes(:identifiers).all : nil
@title = if current_user.can_super_admin?
Expand Down Expand Up @@ -78,7 +78,7 @@ def customisable
customizations = customizations.select do |t|
funder_template_families.include?(t.customization_of)
end
published = customizations.select { |t| t.published? || t.draft? }.length
published = customizations.count { |t| t.published? || t.draft? }

@orgs = current_user.can_super_admin? ? Org.includes(:identifiers).all : []
@title = _('Customizable Templates')
Expand Down Expand Up @@ -328,7 +328,7 @@ def template_export
@formatting = Settings::Template::DEFAULT_SETTINGS[:formatting]

begin
safe_title = @template.title.gsub(/[^a-zA-Z\d\s]/, '').gsub(/ /, '_')
safe_title = @template.title.gsub(/[^a-zA-Z\d\s]/, '').tr(' ', '_')
file_name = "#{safe_title}_v#{@template.version}"
respond_to do |format|
format.docx do
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/plan_exports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def file_name
# Sanitize bad characters and replace spaces with underscores
ret = @plan.title
ret = ret.strip.gsub(/\s+/, '_')
ret = ret.gsub(/"/, '')
ret = ret.delete('"')
ret = ActiveStorage::Filename.new(ret).sanitized
# limit the filename length to 100 chars. Windows systems have a MAX_PATH allowance
# of 255 characters, so this should provide enough of the title to allow the user
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/plans_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ def edit
.find(params[:id])
authorize plan
phase_id = params[:phase_id].to_i
phase = plan.template.phases.select { |p| p.id == phase_id }.first
phase = plan.template.phases.find { |p| p.id == phase_id }
raise ActiveRecord::RecordNotFound if phase.nil?

guidance_groups = GuidanceGroup.where(published: true, id: plan.guidance_group_ids)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/public_pages_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def template_export
@formatting = Settings::Template::DEFAULT_SETTINGS[:formatting]

begin
file_name = @template.title.gsub(/[^a-zA-Z\d\s]/, '').gsub(/ /, '_')
file_name = @template.title.gsub(/[^a-zA-Z\d\s]/, '').tr(' ', '_')
file_name = "#{file_name}_v#{@template.version}"
respond_to do |format|
format.docx do
Expand Down
Loading