From 3b151bf45df7a418ec4b5d77138969c6bac4a38d Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 13:48:33 -0400 Subject: [PATCH 01/16] Initial rubocop config No code changes have been made at this time. Config is not final. https://mitlibraries.atlassian.net/browse/TCO-45 https://mitlibraries.atlassian.net/browse/TCO-46 https://mitlibraries.atlassian.net/browse/TCO-60 --- .rubocop.yml | 33 +++++++++++++++++++++++++++++++++ Gemfile | 11 +++++++---- Gemfile.lock | 11 +++++++++++ Makefile | 9 ++++++++- 4 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 .rubocop.yml diff --git a/.rubocop.yml b/.rubocop.yml new file mode 100644 index 0000000..6d2b9fc --- /dev/null +++ b/.rubocop.yml @@ -0,0 +1,33 @@ +require: + - rubocop-capybara + - rubocop-graphql + - rubocop-minitest + - rubocop-performance + - rubocop-rails + +AllCops: + TargetRubyVersion: 3.2 + NewCops: enable + Exclude: + - "db/**/*" + - "config/**/*" + - "bin/**" + - 'node_modules/**/*' + - 'tmp/**/*' + - 'vendor/**/*' + - '.git/**/*' + - 'Gemfile' + - 'Rakefile' + - 'config.ru' + +Metrics/BlockLength: + Exclude: + - "test/**/*" + +Metrics/ClassLength: + Exclude: + - "test/**/*" + +Metrics/MethodLength: + Exclude: + - "test/**/*.rb" diff --git a/Gemfile b/Gemfile index 39ba4b6..abc2c57 100644 --- a/Gemfile +++ b/Gemfile @@ -89,9 +89,12 @@ group :development do gem 'annotate' # RuboCop is a Ruby static code analyzer (a.k.a. linter) and code formatter. - gem 'rubocop' - gem 'rubocop-capybara' - gem 'rubocop-rails' + gem 'rubocop', require: false + gem 'rubocop-capybara', require: false + gem 'rubocop-graphql', require: false + gem 'rubocop-minitest', require: false + gem 'rubocop-performance', require: false + gem 'rubocop-rails', require: false # Use console on exceptions pages [https://github.com/rails/web-console] gem 'web-console' @@ -112,4 +115,4 @@ group :test do gem 'webmock' end -gem "administrate", "~> 0.20.1" +gem 'administrate', '~> 0.20.1' diff --git a/Gemfile.lock b/Gemfile.lock index edfc3aa..5fca592 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -358,6 +358,14 @@ GEM parser (>= 3.3.1.0) rubocop-capybara (2.21.0) rubocop (~> 1.41) + rubocop-graphql (1.5.4) + rubocop (>= 1.50, < 2) + rubocop-minitest (0.35.1) + rubocop (>= 1.61, < 2.0) + rubocop-ast (>= 1.31.1, < 2.0) + rubocop-performance (1.21.1) + rubocop (>= 1.48.1, < 2.0) + rubocop-ast (>= 1.31.1, < 2.0) rubocop-rails (2.25.1) activesupport (>= 4.2.0) rack (>= 1.1) @@ -477,6 +485,9 @@ DEPENDENCIES rails (~> 7.1.2) rubocop rubocop-capybara + rubocop-graphql + rubocop-minitest + rubocop-performance rubocop-rails selenium-webdriver simplecov diff --git a/Makefile b/Makefile index 050945f..ca21f57 100644 --- a/Makefile +++ b/Makefile @@ -45,4 +45,11 @@ outdated: # List outdated dependencies # Code quality and safety commands #################################### -# coming soon! +lint: + bundle exec rubocop + +lint-models: + bundle exec rubocop app/models + +lint-controllers: + bundle exec rubocop app/controllers From 7867b90056733c865e4406d2c7fc40a021f4c7a2 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 13:50:41 -0400 Subject: [PATCH 02/16] Rubocop safe model cleanup `bundle exec rubocop -a app/models/` --- app/models/ability.rb | 1 + app/models/detector/journal.rb | 2 +- app/models/lookup_doi.rb | 6 ++++-- app/models/lookup_isbn.rb | 2 +- app/models/lookup_issn.rb | 4 ++-- app/models/lookup_pmid.rb | 6 +++--- app/models/search_event.rb | 2 +- app/models/standard_identifiers.rb | 2 +- app/models/user.rb | 2 +- 9 files changed, 15 insertions(+), 12 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 407b086..95b74b3 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -11,6 +11,7 @@ def initialize(user) # Rules will go here. return unless user.admin? + can :manage, :all end end diff --git a/app/models/detector/journal.rb b/app/models/detector/journal.rb index 61962ae..ebcf50b 100644 --- a/app/models/detector/journal.rb +++ b/app/models/detector/journal.rb @@ -39,7 +39,7 @@ def self.full_term_match(phrase) # # @return [Set of Detector::Journal] A set of ActiveRecord Detector::Journal relations. def self.partial_term_match(phrase) - Journal.all.map { |journal| journal if phrase.downcase.include?(journal.name) }.compact + Journal.all.select { |journal| phrase.downcase.include?(journal.name) } end private diff --git a/app/models/lookup_doi.rb b/app/models/lookup_doi.rb index bf41e55..7e8eba1 100644 --- a/app/models/lookup_doi.rb +++ b/app/models/lookup_doi.rb @@ -38,8 +38,10 @@ def fetch(doi) if resp.status == 200 JSON.parse(resp.to_s) else - Rails.logger.debug("Fact lookup error. DOI #{doi} detected but unpaywall returned no data or otherwise errored") - Rails.logger.debug("URL: #{url(doi)}") + Rails.logger.debug do + "Fact lookup error. DOI #{doi} detected but unpaywall returned no data or otherwise errored" + end + Rails.logger.debug { "URL: #{url(doi)}" } 'Error' end end diff --git a/app/models/lookup_isbn.rb b/app/models/lookup_isbn.rb index b7792a4..cc34efc 100644 --- a/app/models/lookup_isbn.rb +++ b/app/models/lookup_isbn.rb @@ -42,7 +42,7 @@ def parse_response(url) JSON.parse(resp.to_s) else Rails.logger.debug('Fact lookup error: openlibrary returned no data') - Rails.logger.debug("URL: #{url}") + Rails.logger.debug { "URL: #{url}" } 'Error' end end diff --git a/app/models/lookup_issn.rb b/app/models/lookup_issn.rb index a70d6cc..f7dcbd3 100644 --- a/app/models/lookup_issn.rb +++ b/app/models/lookup_issn.rb @@ -31,8 +31,8 @@ def fetch(issn) if resp.status == 200 JSON.parse(resp.to_s) else - Rails.logger.debug("ISSN Lookup error. ISSN #{issn} detected but crossref returned no data") - Rails.logger.debug("URL: #{url(issn)}") + Rails.logger.debug { "ISSN Lookup error. ISSN #{issn} detected but crossref returned no data" } + Rails.logger.debug { "URL: #{url(issn)}" } 'Error' end end diff --git a/app/models/lookup_pmid.rb b/app/models/lookup_pmid.rb index 7a9444f..90df4ad 100644 --- a/app/models/lookup_pmid.rb +++ b/app/models/lookup_pmid.rb @@ -12,7 +12,7 @@ def info(pmid) if metadata.reject { |_k, v| v.empty? }.present? metadata else - Rails.logger.debug("Fact lookup error. PMID #{pmid} detected but ncbi returned no data") + Rails.logger.debug { "Fact lookup error. PMID #{pmid} detected but ncbi returned no data" } nil end end @@ -37,8 +37,8 @@ def fetch(pmid) if resp.status == 200 Nokogiri::XML(resp.to_s) else - Rails.logger.debug("Fact lookup error. PMID #{pmid} detected but ncbi an error status") - Rails.logger.debug("URL: #{url(pmid)}") + Rails.logger.debug { "Fact lookup error. PMID #{pmid} detected but ncbi an error status" } + Rails.logger.debug { "URL: #{url(pmid)}" } 'Error' end end diff --git a/app/models/search_event.rb b/app/models/search_event.rb index 8fe5a5f..7042b8f 100644 --- a/app/models/search_event.rb +++ b/app/models/search_event.rb @@ -21,5 +21,5 @@ class SearchEvent < ApplicationRecord # # @param month [DateTime] A DateTime object within the `month` to be filtered. # @return [Array] All SearchEvents for the supplied `month`. - scope :single_month, ->(month) { where(created_at: month.beginning_of_month..month.end_of_month) } + scope :single_month, ->(month) { where(created_at: month.all_month) } end diff --git a/app/models/standard_identifiers.rb b/app/models/standard_identifiers.rb index 0de7f4c..b8fd3ed 100644 --- a/app/models/standard_identifiers.rb +++ b/app/models/standard_identifiers.rb @@ -55,7 +55,7 @@ def strip_invalid_issns # An example calculation is shared at # https://en.wikipedia.org/wiki/International_Standard_Serial_Number#Code_format def validate_issn(candidate) - digits = candidate.gsub('-', '').chars[..6] + digits = candidate.delete('-')[..6].chars check_digit = candidate.last.downcase sum = 0 diff --git a/app/models/user.rb b/app/models/user.rb index a2126ce..12e5ff1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -33,7 +33,7 @@ def self.from_omniauth(auth) email = auth.extra.raw_info.email end - User.where(uid: uid).first_or_create do |user| + User.where(uid:).first_or_create do |user| user.uid = uid user.email = email end From 12fdeb43336bb61761f5e9a1b5450d4bc8245334 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 13:52:12 -0400 Subject: [PATCH 03/16] Rubocop safeish model cleanup `bundle exec rubocop -A app/models/` --- app/models/ability.rb | 2 +- app/models/concerns/fake_auth_config.rb | 2 ++ app/models/lookup_isbn.rb | 2 +- app/models/metrics/algorithms.rb | 2 +- app/models/user.rb | 2 ++ 5 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 95b74b3..998253a 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -7,7 +7,7 @@ class Ability # See the wiki for details: # https://github.com/CanCanCommunity/cancancan/blob/develop/docs/define_check_abilities.md def initialize(user) - return unless user.present? + return if user.blank? # Rules will go here. return unless user.admin? diff --git a/app/models/concerns/fake_auth_config.rb b/app/models/concerns/fake_auth_config.rb index eeda3fb..809c7f0 100644 --- a/app/models/concerns/fake_auth_config.rb +++ b/app/models/concerns/fake_auth_config.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module FakeAuthConfig # Used in an initializer to determine if the application is configured and allowed to use fake authentication. def self.fake_auth_enabled? diff --git a/app/models/lookup_isbn.rb b/app/models/lookup_isbn.rb index cc34efc..d17f5d1 100644 --- a/app/models/lookup_isbn.rb +++ b/app/models/lookup_isbn.rb @@ -26,7 +26,7 @@ def fetch_isbn(isbn) def fetch_authors(isbn_json) return unless isbn_json['authors'] - authors = isbn_json['authors'].map { |a| a['key'] } + authors = isbn_json['authors'].pluck('key') author_names = authors.map do |author| url = [base_url, author, '.json'].join json = parse_response(url) diff --git a/app/models/metrics/algorithms.rb b/app/models/metrics/algorithms.rb index 82535ab..2ba9b24 100644 --- a/app/models/metrics/algorithms.rb +++ b/app/models/metrics/algorithms.rb @@ -41,7 +41,7 @@ def generate(month = nil) matches = if month.present? count_matches(SearchEvent.single_month(month).includes(:term)) else - count_matches(SearchEvent.all.includes(:term)) + count_matches(SearchEvent.includes(:term)) end Metrics::Algorithms.create(month:, doi: matches[:doi], issn: matches[:issn], isbn: matches[:isbn], pmid: matches[:pmid], journal_exact: matches[:journal_exact], diff --git a/app/models/user.rb b/app/models/user.rb index 12e5ff1..99d9d99 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # == Schema Information # # Table name: users From d6296c5a64d6292a2afcc8289456d9222bf37c2f Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 15:26:08 -0400 Subject: [PATCH 04/16] Rubocop safe controller cleanup `bundle exec rubocop -a app/controllers/` --- app/controllers/admin/application_controller.rb | 4 ++-- .../admin/detector/suggested_resources_controller.rb | 10 +++++----- app/controllers/graphql_controller.rb | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/controllers/admin/application_controller.rb b/app/controllers/admin/application_controller.rb index 8449926..2852401 100644 --- a/app/controllers/admin/application_controller.rb +++ b/app/controllers/admin/application_controller.rb @@ -13,7 +13,7 @@ class ApplicationController < Administrate::ApplicationController def authorize_user return if authorize_action?(resource_name, action_name) - + redirect_to root_path, alert: 'Not authorized' end @@ -23,7 +23,7 @@ def authorize_action?(resource, action) def require_user return if current_user - + redirect_to root_path, alert: 'Please sign in to continue' end diff --git a/app/controllers/admin/detector/suggested_resources_controller.rb b/app/controllers/admin/detector/suggested_resources_controller.rb index 9cebe16..599c6f6 100644 --- a/app/controllers/admin/detector/suggested_resources_controller.rb +++ b/app/controllers/admin/detector/suggested_resources_controller.rb @@ -8,7 +8,7 @@ class SuggestedResourcesController < Admin::ApplicationController # super # send_foo_updated_email(requested_resource) # end - + # Override this method to specify custom lookup behavior. # This will be used to set the resource for the `show`, `edit`, and `update` # actions. @@ -16,9 +16,9 @@ class SuggestedResourcesController < Admin::ApplicationController # def find_resource(param) # Foo.find_by!(slug: param) # end - + # The result of this lookup will be available as `requested_resource` - + # Override this if you have certain roles that require a subset # this will be used to set the records shown on the `index` action. # @@ -29,7 +29,7 @@ class SuggestedResourcesController < Admin::ApplicationController # resource_class.with_less_stuff # end # end - + # Override `resource_params` if you want to transform the submitted # data before it's persisted. For example, the following would turn all # empty values into nil values. It uses other APIs such as `resource_class` @@ -40,7 +40,7 @@ class SuggestedResourcesController < Admin::ApplicationController # permit(dashboard.permitted_attributes(action_name)). # transform_values { |value| value == "" ? nil : value } # end - + # See https://administrate-demo.herokuapp.com/customizing_controller_actions # for more information end diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index 923c431..8d629d8 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -48,6 +48,6 @@ def handle_error_in_development(e) logger.error e.message logger.error e.backtrace.join("\n") - render json: { errors: [{ message: e.message, backtrace: e.backtrace }], data: {} }, status: 500 + render json: { errors: [{ message: e.message, backtrace: e.backtrace }], data: {} }, status: :internal_server_error end end From d60c56266981e11f82d57194e535f18c48f729eb Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 15:27:09 -0400 Subject: [PATCH 05/16] Rubocop safeish controller cleanup `bundle exec rubocop -A app/controllers/` --- .../admin/application_controller.rb | 2 ++ .../suggested_resources_controller.rb | 2 ++ .../admin/search_events_controller.rb | 2 ++ app/controllers/admin/terms_controller.rb | 2 ++ app/controllers/admin/users_controller.rb | 2 ++ app/controllers/application_controller.rb | 2 ++ app/controllers/static_controller.rb | 2 ++ .../users/omniauth_callbacks_controller.rb | 30 +++++++++++-------- 8 files changed, 31 insertions(+), 13 deletions(-) diff --git a/app/controllers/admin/application_controller.rb b/app/controllers/admin/application_controller.rb index 2852401..dbc907e 100644 --- a/app/controllers/admin/application_controller.rb +++ b/app/controllers/admin/application_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # All Administrate controllers inherit from this # `Administrate::ApplicationController`, making it the ideal place to put # authentication logic or other before_actions. diff --git a/app/controllers/admin/detector/suggested_resources_controller.rb b/app/controllers/admin/detector/suggested_resources_controller.rb index 599c6f6..9e794d9 100644 --- a/app/controllers/admin/detector/suggested_resources_controller.rb +++ b/app/controllers/admin/detector/suggested_resources_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Admin module Detector class SuggestedResourcesController < Admin::ApplicationController diff --git a/app/controllers/admin/search_events_controller.rb b/app/controllers/admin/search_events_controller.rb index de79091..cd2b0de 100644 --- a/app/controllers/admin/search_events_controller.rb +++ b/app/controllers/admin/search_events_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Admin class SearchEventsController < Admin::ApplicationController # Overwrite any of the RESTful controller actions to implement custom behavior diff --git a/app/controllers/admin/terms_controller.rb b/app/controllers/admin/terms_controller.rb index 9f2780c..778a926 100644 --- a/app/controllers/admin/terms_controller.rb +++ b/app/controllers/admin/terms_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Admin class TermsController < Admin::ApplicationController # Overwrite any of the RESTful controller actions to implement custom behavior diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 2c4ab7d..8465b41 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Admin class UsersController < Admin::ApplicationController # Overwrite any of the RESTful controller actions to implement custom behavior diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 4bb3e9c..41e9036 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class ApplicationController < ActionController::Base helper Mitlibraries::Theme::Engine.helpers diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb index 4d46687..c41a44b 100644 --- a/app/controllers/static_controller.rb +++ b/app/controllers/static_controller.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class StaticController < ApplicationController def index; end end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index d822729..a08cb73 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -1,21 +1,25 @@ +# frozen_string_literal: true + # Handles authentication response from Omniauth. See # [the Devise docs](https://www.rubydoc.info/gems/devise_token_auth/DeviseTokenAuth/OmniauthCallbacksController) for # additional information about this controller. -class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController - include FakeAuthConfig +module Users + class OmniauthCallbacksController < Devise::OmniauthCallbacksController + include FakeAuthConfig - def openid_connect - @user = User.from_omniauth(request.env['omniauth.auth']) - sign_in_and_redirect @user, event: :authentication - flash[:notice] = "Welcome, #{@user.email}!" - end + def openid_connect + @user = User.from_omniauth(request.env['omniauth.auth']) + sign_in_and_redirect @user, event: :authentication + flash[:notice] = "Welcome, #{@user.email}!" + end - # Developer authentication is used in local dev and PR builds. - def developer - raise 'Invalid Authentication' unless FakeAuthConfig.fake_auth_enabled? + # Developer authentication is used in local dev and PR builds. + def developer + raise 'Invalid Authentication' unless FakeAuthConfig.fake_auth_enabled? - @user = User.from_omniauth(request.env['omniauth.auth']) - sign_in_and_redirect @user, event: :authentication - flash[:notice] = "Welcome, #{@user.email}!" + @user = User.from_omniauth(request.env['omniauth.auth']) + sign_in_and_redirect @user, event: :authentication + flash[:notice] = "Welcome, #{@user.email}!" + end end end From 1d7a24a158da995c4dbe4d8f4d20416a4a0adb14 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 15:32:04 -0400 Subject: [PATCH 06/16] Rubocop safe graphql cleanup `bundle exec rubocop -a app/graphql/` --- app/graphql/tacos_schema.rb | 6 ++-- app/graphql/types/details_type.rb | 12 ++++---- app/graphql/types/query_type.rb | 30 +++++++++++-------- app/graphql/types/search_event_type.rb | 8 ++--- .../types/standard_identifiers_type.rb | 2 +- app/graphql/types/term_type.rb | 6 ++-- 6 files changed, 34 insertions(+), 30 deletions(-) diff --git a/app/graphql/tacos_schema.rb b/app/graphql/tacos_schema.rb index c195e17..31e661c 100644 --- a/app/graphql/tacos_schema.rb +++ b/app/graphql/tacos_schema.rb @@ -16,7 +16,7 @@ def self.type_error(err, context) end # Union and Interface Resolution - def self.resolve_type(abstract_type, obj, ctx) + def self.resolve_type(_abstract_type, _obj, _ctx) # TODO: Implement this method # to return the correct GraphQL object type for `obj` raise(GraphQL::RequiredImplementationMissingError) @@ -28,13 +28,13 @@ def self.resolve_type(abstract_type, obj, ctx) # Relay-style Object Identification: # Return a string UUID for `object` - def self.id_from_object(object, type_definition, query_ctx) + def self.id_from_object(object, _type_definition, _query_ctx) # For example, use Rails' GlobalID library (https://github.com/rails/globalid): object.to_gid_param end # Given a string UUID, find the object - def self.object_from_id(global_id, query_ctx) + def self.object_from_id(global_id, _query_ctx) # For example, use Rails' GlobalID library (https://github.com/rails/globalid): GlobalID.find(global_id) end diff --git a/app/graphql/types/details_type.rb b/app/graphql/types/details_type.rb index b4d0a5a..32eb554 100644 --- a/app/graphql/types/details_type.rb +++ b/app/graphql/types/details_type.rb @@ -2,17 +2,17 @@ module Types class DetailsType < Types::BaseObject - field :title, String field :authors, [String] - field :date, String - field :publisher, String - field :oa, Boolean - field :oa_status, String field :best_oa_location, String + field :date, String + field :doi, String field :issns, [String] field :journal_name, String - field :doi, String field :link_resolver_url, String + field :oa, Boolean + field :oa_status, String + field :publisher, String + field :title, String def issns @object[:journal_issns]&.split(',') diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb index f77e73f..d25f33c 100644 --- a/app/graphql/types/query_type.rb +++ b/app/graphql/types/query_type.rb @@ -6,19 +6,11 @@ class QueryType < Types::BaseObject argument :id, ID, required: true, description: 'ID of the object.' end - def node(id:) - context.schema.object_from_id(id, context) - end - field :nodes, [Types::NodeType, { null: true }], null: true, description: 'Fetches a list of objects given a list of IDs.' do argument :ids, [ID], required: true, description: 'IDs of the objects.' end - def nodes(ids:) - ids.map { |id| context.schema.object_from_id(id, context) } - end - # Add root-level fields here. # They will be entry points for queries on your schema. @@ -28,16 +20,28 @@ def nodes(ids:) argument :source_system, String, required: true end - def log_search_event(search_term:, source_system:) - term = Term.create_or_find_by!(phrase: search_term) - term.search_events.create!(source: source_system) - end - field :lookup_term, TermType, null: true, description: 'Lookup a term to return information about it (bypasses logging)' do argument :search_term, String, required: true end + def node(id:) + context.schema.object_from_id(id, context) + end + + def node(id:) + context.schema.object_from_id(id, context) + end + + def nodes(ids:) + ids.map { |id| context.schema.object_from_id(id, context) } + end + + def log_search_event(search_term:, source_system:) + term = Term.create_or_find_by!(phrase: search_term) + term.search_events.create!(source: source_system) + end + def lookup_term(search_term:) term = Term.find_by(phrase: search_term) end diff --git a/app/graphql/types/search_event_type.rb b/app/graphql/types/search_event_type.rb index 437e9e3..7660e05 100644 --- a/app/graphql/types/search_event_type.rb +++ b/app/graphql/types/search_event_type.rb @@ -2,13 +2,13 @@ module Types class SearchEventType < Types::BaseObject - field :id, ID, null: false - field :term_id, Integer - field :source, String field :created_at, GraphQL::Types::ISO8601DateTime, null: false - field :updated_at, GraphQL::Types::ISO8601DateTime, null: false + field :id, ID, null: false field :phrase, String + field :source, String field :standard_identifiers, [StandardIdentifiersType] + field :term_id, Integer + field :updated_at, GraphQL::Types::ISO8601DateTime, null: false def phrase @object.term.phrase diff --git a/app/graphql/types/standard_identifiers_type.rb b/app/graphql/types/standard_identifiers_type.rb index b9776d6..3d1d5d4 100644 --- a/app/graphql/types/standard_identifiers_type.rb +++ b/app/graphql/types/standard_identifiers_type.rb @@ -2,9 +2,9 @@ module Types class StandardIdentifiersType < Types::BaseObject + field :details, DetailsType field :kind, String, null: false field :value, String, null: false - field :details, DetailsType # details does external lookups and should only be run if the fields # have been explicitly requested diff --git a/app/graphql/types/term_type.rb b/app/graphql/types/term_type.rb index 742d1e3..18f7e9c 100644 --- a/app/graphql/types/term_type.rb +++ b/app/graphql/types/term_type.rb @@ -2,13 +2,13 @@ module Types class TermType < Types::BaseObject - field :id, ID, null: false field :created_at, GraphQL::Types::ISO8601DateTime, null: false - field :updated_at, GraphQL::Types::ISO8601DateTime, null: false - field :phrase, String, null: false + field :id, ID, null: false field :occurence_count, Integer + field :phrase, String, null: false field :search_events, [SearchEventType], null: false field :standard_identifiers, [StandardIdentifiersType] + field :updated_at, GraphQL::Types::ISO8601DateTime, null: false def occurence_count @object.search_events.count From 08e5c6fa7a4451158d6f6524e2b9b9c660ff1eaf Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 15:35:03 -0400 Subject: [PATCH 07/16] Rubocop safeish graphql cleanup `bundle exec rubocop -A app/graphql/` Note: reverted change to tacos_schema.rb that removed an example of how to do custom error handling that felt useful to keep in place even if it doesn't do anything yet. --- app/graphql/types/mutation_type.rb | 2 ++ app/graphql/types/query_type.rb | 2 +- app/graphql/types/search_event_type.rb | 6 ++---- app/graphql/types/term_type.rb | 6 ++---- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 1138619..8bda052 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module Types class MutationType < Types::BaseObject end diff --git a/app/graphql/types/query_type.rb b/app/graphql/types/query_type.rb index d25f33c..cd88fce 100644 --- a/app/graphql/types/query_type.rb +++ b/app/graphql/types/query_type.rb @@ -43,7 +43,7 @@ def log_search_event(search_term:, source_system:) end def lookup_term(search_term:) - term = Term.find_by(phrase: search_term) + Term.find_by(phrase: search_term) end end end diff --git a/app/graphql/types/search_event_type.rb b/app/graphql/types/search_event_type.rb index 7660e05..d1d0d32 100644 --- a/app/graphql/types/search_event_type.rb +++ b/app/graphql/types/search_event_type.rb @@ -15,11 +15,9 @@ def phrase end def standard_identifiers - ids = [] - StandardIdentifiers.new(@object.term.phrase).identifiers.each do |identifier| - ids << { kind: identifier.first, value: identifier.last } + StandardIdentifiers.new(@object.term.phrase).identifiers.map do |identifier| + { kind: identifier.first, value: identifier.last } end - ids end end end diff --git a/app/graphql/types/term_type.rb b/app/graphql/types/term_type.rb index 18f7e9c..28b5243 100644 --- a/app/graphql/types/term_type.rb +++ b/app/graphql/types/term_type.rb @@ -15,11 +15,9 @@ def occurence_count end def standard_identifiers - ids = [] - StandardIdentifiers.new(@object.phrase).identifiers.each do |identifier| - ids << { kind: identifier.first, value: identifier.last } + StandardIdentifiers.new(@object.phrase).identifiers.map do |identifier| + { kind: identifier.first, value: identifier.last } end - ids end end end From 6ea4511a6f78fedf76cc6459d338df73211acbca Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 15:36:47 -0400 Subject: [PATCH 08/16] Rubocop safe test cleanup `bundle exec rubocop -a test/` --- test/controllers/graphql_controller_test.rb | 6 ++ test/integration/admin_dashboard_test.rb | 7 ++- test/integration/authentication_test.rb | 3 + test/models/detector/journal_test.rb | 16 +++-- .../detector/suggested_resource_test.rb | 44 ++++++++------ test/models/lookup_doi_test.rb | 5 +- test/models/lookup_isbn_test.rb | 5 +- test/models/lookup_issn_test.rb | 5 +- test/models/lookup_pmid_test.rb | 5 +- test/models/metrics/algorithms_test.rb | 58 +++++++++++-------- test/models/search_event_test.rb | 20 ++++--- test/models/standard_identifiers_test.rb | 16 +++++ test/models/term_test.rb | 4 +- test/models/user_test.rb | 23 +++++--- test/tasks/suggested_resource_rake_test.rb | 10 ++-- test/test_helper.rb | 2 +- 16 files changed, 153 insertions(+), 76 deletions(-) diff --git a/test/controllers/graphql_controller_test.rb b/test/controllers/graphql_controller_test.rb index 60ae020..613de09 100644 --- a/test/controllers/graphql_controller_test.rb +++ b/test/controllers/graphql_controller_test.rb @@ -12,6 +12,7 @@ class GraphqlControllerTest < ActionDispatch::IntegrationTest updatedAt } }' } + assert_equal(200, response.status) json = JSON.parse(response.body) term_id = Term.last.id @@ -32,6 +33,7 @@ class GraphqlControllerTest < ActionDispatch::IntegrationTest updatedAt } }' } + assert_equal(200, response.status) assert_equal Term.count, (initial_term_count + 1) assert_equal 'range life', Term.last.phrase @@ -47,6 +49,7 @@ class GraphqlControllerTest < ActionDispatch::IntegrationTest updatedAt } }' } + assert_equal(200, response.status) assert_equal Term.count, initial_term_count end @@ -62,6 +65,7 @@ class GraphqlControllerTest < ActionDispatch::IntegrationTest }' } json = JSON.parse(response.body) + assert_equal('doi', json['data']['logSearchEvent']['standardIdentifiers'].first['kind']) assert_equal('10.1038/nphys1170', json['data']['logSearchEvent']['standardIdentifiers'].first['value']) end @@ -74,6 +78,7 @@ class GraphqlControllerTest < ActionDispatch::IntegrationTest }' } json = JSON.parse(response.body) + assert_equal('10.1038/nphys1170', json['data']['logSearchEvent']['phrase']) end @@ -95,6 +100,7 @@ class GraphqlControllerTest < ActionDispatch::IntegrationTest }' } json = JSON.parse(response.body) + assert_equal('Measured measurement', json['data']['logSearchEvent']['standardIdentifiers'].first['details']['title']) assert_equal('https://mit.primo.exlibrisgroup.com/discovery/openurl?institution=01MIT_INST&rfr_id=info:sid/mit.tacos.api&vid=01MIT_INST:MIT&rft.atitle=Measured measurement&rft.date=&rft.genre=journal-article&rft.jtitle=Nature Physics&rft_id=info:doi/10.1038/nphys1170', diff --git a/test/integration/admin_dashboard_test.rb b/test/integration/admin_dashboard_test.rb index ba934c7..d31a5c4 100644 --- a/test/integration/admin_dashboard_test.rb +++ b/test/integration/admin_dashboard_test.rb @@ -11,8 +11,10 @@ def teardown test 'admin area redirects to root with prompt to sign in if not already signed in' do get '/admin' + assert_response :redirect follow_redirect! + assert_equal '/', path assert_select 'div.alert', text: 'Please sign in to continue', count: 1 end @@ -20,8 +22,10 @@ def teardown test 'authenticated users without admin status still cannot access admin area' do mock_auth(users(:basic)) get '/admin' + assert_response :redirect follow_redirect! + assert_equal '/', path assert_select 'div.alert', text: 'Not authorized', count: 1 end @@ -29,7 +33,8 @@ def teardown test 'admin users can access admin area' do mock_auth(users(:admin)) get '/admin' - assert_response 200 + + assert_response :ok assert_equal '/admin', path end end diff --git a/test/integration/authentication_test.rb b/test/integration/authentication_test.rb index 1d501e6..a0ceff7 100644 --- a/test/integration/authentication_test.rb +++ b/test/integration/authentication_test.rb @@ -24,6 +24,7 @@ def silence_omniauth get '/users/auth/openid_connect/callback' follow_redirect! end + assert_response :success assert_equal(user_count, User.count) end @@ -36,6 +37,7 @@ def silence_omniauth user_count = User.count get '/users/auth/openid_connect/callback' follow_redirect! + assert_response :success assert_equal(user_count + 1, User.count) end @@ -44,6 +46,7 @@ def silence_omniauth user_count = User.count mock_auth(users(:valid)) follow_redirect! + assert_response :success assert_equal(user_count, User.count) end diff --git a/test/models/detector/journal_test.rb b/test/models/detector/journal_test.rb index 06828c8..ccfba12 100644 --- a/test/models/detector/journal_test.rb +++ b/test/models/detector/journal_test.rb @@ -18,7 +18,7 @@ class JournalTest < ActiveSupport::TestCase expected = detector_journals('the_new_england_journal_of_medicine') actual = Detector::Journal.full_term_match('the new england journal of medicine') - assert actual.count == 1 + assert_equal 1, actual.count assert_equal(expected, actual.first) end @@ -26,30 +26,34 @@ class JournalTest < ActiveSupport::TestCase expected = detector_journals('the_new_england_journal_of_medicine') actual = Detector::Journal.full_term_match('The New England Journal of Medicine') - assert actual.count == 1 + assert_equal 1, actual.count assert_equal(expected, actual.first) end test 'exact match within longer term returns no matches' do actual = Detector::Journal.full_term_match('The New England Journal of Medicine, 1999') - assert actual.count.zero? + + assert_predicate actual.count, :zero? end test 'phrase match within longer term returns matches' do actual = Detector::Journal.partial_term_match('words and stuff The New England Journal of Medicine, 1999') - assert actual.count == 1 + + assert_equal 1, actual.count end test 'multple matches can happen with phrase matching within longer terms' do actual = Detector::Journal.partial_term_match('words and stuff Nature medicine, 1999') - assert actual.count == 2 + + assert_equal 2, actual.count end test 'mixed titles are downcased when saved' do mixed_case = 'ThIs Is A tItLe' actual = Detector::Journal.create(name: mixed_case) actual.reload - refute_equal(mixed_case, actual.name) + + assert_not_equal(mixed_case, actual.name) assert_equal(mixed_case.downcase, actual.name) end end diff --git a/test/models/detector/suggested_resource_test.rb b/test/models/detector/suggested_resource_test.rb index 620b35b..57031b9 100644 --- a/test/models/detector/suggested_resource_test.rb +++ b/test/models/detector/suggested_resource_test.rb @@ -25,99 +25,105 @@ class SuggestedResourceTest < ActiveSupport::TestCase new_resource = Detector::SuggestedResource.create(resource) - assert new_resource.fingerprint == 'latest our resource' + assert_equal 'latest our resource', new_resource.fingerprint end test 'fingerprints are recalculated on save' do resource = detector_suggested_resources('jstor') - refute resource.fingerprint == 'A brand new phrase' + + assert_not_equal resource.fingerprint, 'A brand new phrase' resource.phrase = 'This is a brand new phrase' resource.save resource.reload - assert resource.fingerprint == 'a brand is new phrase this' + assert_equal 'a brand is new phrase this', resource.fingerprint end test 'generating fingerprints does not alter the phrase' do resource = detector_suggested_resources('jstor') benchmark = 'This is an updated phrase! ' - refute resource.phrase == benchmark + assert_not_equal resource.phrase, benchmark resource.phrase = benchmark resource.save resource.reload - assert resource.phrase == benchmark + assert_equal resource.phrase, benchmark end test 'fingerprints strip extra spaces' do resource = detector_suggested_resources('jstor') - refute resource.fingerprint == 'i need space' + + assert_not_equal resource.fingerprint, 'i need space' resource.phrase = ' i need space ' resource.save resource.reload - assert resource.fingerprint == 'i need space' + assert_equal 'i need space', resource.fingerprint end test 'fingerprints are coerced to lowercase' do resource = detector_suggested_resources('jstor') - refute resource.fingerprint == 'ftw intercapping' + + assert_not_equal resource.fingerprint, 'ftw intercapping' resource.phrase = 'InterCapping FTW' resource.save resource.reload - assert resource.fingerprint == 'ftw intercapping' + assert_equal 'ftw intercapping', resource.fingerprint end test 'fingerprints remove punctuation and symbols' do resource = detector_suggested_resources('jstor') - refute resource.fingerprint == 'bullets perfect phrase punctuation quoted symbols' + + assert_not_equal resource.fingerprint, 'bullets perfect phrase punctuation quoted symbols' resource.phrase = 'symbols™ + punctuation: * bullets! - "quoted phrase" (perfect) ¥€$' resource.save resource.reload - assert resource.fingerprint == 'bullets perfect phrase punctuation quoted symbols' + assert_equal 'bullets perfect phrase punctuation quoted symbols', resource.fingerprint end test 'fingerprints coerce characters to ASCII' do resource = { title: 'A wide range of characters', url: 'https://example.org', - phrase: 'а а̀ а̂ а̄ ӓ б в г ґ д ђ ѓ е ѐ е̄ е̂ ё є ж з з́ ѕ и і ї ꙇ ѝ и̂ ӣ й ј к л љ м н њ о о̀ о̂ ō ӧ п р с с́'\ - ' т ћ ќ у у̀ у̂ ӯ ў ӱ ф х ц ч џ ш щ ꙏ ъ ъ̀ ы ь ѣ э ю ю̀ я' + phrase: 'а а̀ а̂ а̄ ӓ б в г ґ д ђ ѓ е ѐ е̄ е̂ ё є ж з з́ ѕ и і ї ꙇ ѝ и̂ ӣ й ј к л љ м н њ о о̀ о̂ ō ӧ п р с с́ ' \ + 'т ћ ќ у у̀ у̂ ӯ ў ӱ ф х ц ч џ ш щ ꙏ ъ ъ̀ ы ь ѣ э ю ю̀ я' } new_resource = Detector::SuggestedResource.create(resource) - assert new_resource.fingerprint == 'a b ch d dj dz dzh e f g gh gj i ia ie io iu j k kh kj l lj m n nj o p r s '\ - 'sh shch t ts tsh u v y yi z zh' + assert_equal 'a b ch d dj dz dzh e f g gh gj i ia ie io iu j k kh kj l lj m n nj o p r s ' \ + 'sh shch t ts tsh u v y yi z zh', new_resource.fingerprint end test 'fingerprints remove repeated words' do resource = detector_suggested_resources('jstor') - refute resource.fingerprint == 'double' + + assert_not_equal resource.fingerprint, 'double' resource.phrase = 'double double' resource.save resource.reload - assert resource.fingerprint == 'double' + assert_equal 'double', resource.fingerprint end test 'fingerprints sort words alphabetically' do resource = detector_suggested_resources('jstor') - refute resource.fingerprint == 'delta gamma' + + assert_not_equal resource.fingerprint, 'delta gamma' resource.phrase = 'gamma delta' resource.save resource.reload - assert resource.fingerprint == 'delta gamma' + assert_equal 'delta gamma', resource.fingerprint end end end diff --git a/test/models/lookup_doi_test.rb b/test/models/lookup_doi_test.rb index 796932d..20de97d 100644 --- a/test/models/lookup_doi_test.rb +++ b/test/models/lookup_doi_test.rb @@ -9,8 +9,9 @@ class LookupDoiTest < ActiveSupport::TestCase expected_keys = %i[genre title date publisher oa oa_status best_oa_location journal_issns journal_name link_resolver_url] + expected_keys.each do |key| - assert(metadata.keys.include?(key)) + assert_includes(metadata.keys, key) end end end @@ -20,6 +21,7 @@ class LookupDoiTest < ActiveSupport::TestCase metadata = LookupDoi.new.info('10.1038/d41586-023-03497-2') expected_url = 'https://mit.primo.exlibrisgroup.com/discovery/openurl?institution=01MIT_INST&rfr_id=info:sid/mit.tacos.api&vid=01MIT_INST:MIT&rft.atitle=Flashy molecules decode a polymer’s lengthening chain&rft.date=&rft.genre=journal-article&rft.jtitle=Nature&rft_id=info:doi/10.1038/d41586-023-03497-2' + assert_equal(expected_url, metadata[:link_resolver_url]) end end @@ -27,6 +29,7 @@ class LookupDoiTest < ActiveSupport::TestCase test 'non 200 responses' do VCR.use_cassette('doi not found') do metadata = LookupDoi.new.info('123456') + assert_nil(metadata) end end diff --git a/test/models/lookup_isbn_test.rb b/test/models/lookup_isbn_test.rb index 23b2a4c..d48dd6a 100644 --- a/test/models/lookup_isbn_test.rb +++ b/test/models/lookup_isbn_test.rb @@ -8,8 +8,9 @@ class LookupIsbnTest < ActiveSupport::TestCase metadata = LookupIsbn.new.info('978-0-08-102133-0') expected_keys = %i[title date publisher authors link_resolver_url] + expected_keys.each do |key| - assert(metadata.keys.include?(key)) + assert_includes(metadata.keys, key) end end end @@ -19,6 +20,7 @@ class LookupIsbnTest < ActiveSupport::TestCase metadata = LookupIsbn.new.info('978-0-08-102133-0') expected_url = 'https://mit.primo.exlibrisgroup.com/discovery/openurl?institution=01MIT_INST&rfr_id=info:sid/mit.tacos.api&vid=01MIT_INST:MIT&rft.isbn=978-0-08-102133-0' + assert_equal(expected_url, metadata[:link_resolver_url]) end end @@ -26,6 +28,7 @@ class LookupIsbnTest < ActiveSupport::TestCase test 'non 200 responses' do VCR.use_cassette('isbn not found') do metadata = LookupIsbn.new.info('asdf') + assert_nil(metadata) end end diff --git a/test/models/lookup_issn_test.rb b/test/models/lookup_issn_test.rb index 7cbd819..945c4cf 100644 --- a/test/models/lookup_issn_test.rb +++ b/test/models/lookup_issn_test.rb @@ -8,8 +8,9 @@ class LookupIssnTest < ActiveSupport::TestCase metadata = LookupIssn.new.info('1078-8956') expected_keys = %i[publisher journal_issns journal_name link_resolver_url] + expected_keys.each do |key| - assert(metadata.keys.include?(key)) + assert_includes(metadata.keys, key) end end end @@ -19,6 +20,7 @@ class LookupIssnTest < ActiveSupport::TestCase metadata = LookupIssn.new.info('1078-8956') expected_url = 'https://mit.primo.exlibrisgroup.com/discovery/openurl?institution=01MIT_INST&rfr_id=info:sid/mit.tacos.api&vid=01MIT_INST:MIT&rft.issn=1078-8956' + assert_equal(expected_url, metadata[:link_resolver_url]) end end @@ -26,6 +28,7 @@ class LookupIssnTest < ActiveSupport::TestCase test 'non 200 responses' do VCR.use_cassette('issn not found') do metadata = LookupIssn.new.info('asdf') + assert_nil(metadata) end end diff --git a/test/models/lookup_pmid_test.rb b/test/models/lookup_pmid_test.rb index 5da3557..086c2fe 100644 --- a/test/models/lookup_pmid_test.rb +++ b/test/models/lookup_pmid_test.rb @@ -8,8 +8,9 @@ class LookupPmidTest < ActiveSupport::TestCase metadata = LookupPmid.new.info('37953305') expected_keys = %i[title date journal_volume doi journal_name link_resolver_url] + expected_keys.each do |key| - assert(metadata.keys.include?(key)) + assert_includes(metadata.keys, key) end end end @@ -19,6 +20,7 @@ class LookupPmidTest < ActiveSupport::TestCase metadata = LookupPmid.new.info('37953305') expected_url = "https://mit.primo.exlibrisgroup.com/discovery/openurl?institution=01MIT_INST&rfr_id=info:sid/mit.tacos.api&vid=01MIT_INST:MIT&rft.atitle=Flashy molecules decode a polymer's lengthening chain.&rft.date=2023&rft.jtitle=Nature&rft_id=info:doi/10.1038/d41586-023-03497-2" + assert_equal(expected_url, metadata[:link_resolver_url]) end end @@ -26,6 +28,7 @@ class LookupPmidTest < ActiveSupport::TestCase test 'non 200 responses' do VCR.use_cassette('pmid not found') do metadata = LookupPmid.new.info('asdf') + assert_nil(metadata) end end diff --git a/test/models/metrics/algorithms_test.rb b/test/models/metrics/algorithms_test.rb index 314647c..279a081 100644 --- a/test/models/metrics/algorithms_test.rb +++ b/test/models/metrics/algorithms_test.rb @@ -21,32 +21,38 @@ class Algorithms < ActiveSupport::TestCase # Monthlies test 'dois counts are included in monthly aggregation' do aggregate = Metrics::Algorithms.new.generate(DateTime.now) - assert aggregate.doi == 1 + + assert_equal 1, aggregate.doi end test 'issns counts are included in monthly aggregation' do aggregate = Metrics::Algorithms.new.generate(DateTime.now) - assert aggregate.issn == 1 + + assert_equal 1, aggregate.issn end test 'isbns counts are included in monthly aggregation' do aggregate = Metrics::Algorithms.new.generate(DateTime.now) - assert aggregate.isbn == 1 + + assert_equal 1, aggregate.isbn end test 'pmids counts are included in monthly aggregation' do aggregate = Metrics::Algorithms.new.generate(DateTime.now) - assert aggregate.pmid == 1 + + assert_equal 1, aggregate.pmid end test 'journal exact counts are included in monthly aggregation' do aggregate = Metrics::Algorithms.new.generate(DateTime.now) - assert aggregate.journal_exact == 1 + + assert_equal 1, aggregate.journal_exact end test 'unmatched counts are included are included in monthly aggregation' do aggregate = Metrics::Algorithms.new.generate(DateTime.now) - assert aggregate.unmatched == 2 + + assert_equal 2, aggregate.unmatched end test 'creating lots of searchevents leads to correct data for monthly' do @@ -80,42 +86,48 @@ class Algorithms < ActiveSupport::TestCase aggregate = Metrics::Algorithms.new.generate(DateTime.now) - assert doi_expected_count == aggregate.doi - assert issn_expected_count == aggregate.issn - assert isbn_expected_count == aggregate.isbn - assert pmid_expected_count == aggregate.pmid - assert unmatched_expected_count == aggregate.unmatched + assert_equal doi_expected_count, aggregate.doi + assert_equal issn_expected_count, aggregate.issn + assert_equal isbn_expected_count, aggregate.isbn + assert_equal pmid_expected_count, aggregate.pmid + assert_equal unmatched_expected_count, aggregate.unmatched end # Total test 'dois counts are included in total aggregation' do aggregate = Metrics::Algorithms.new.generate - assert aggregate.doi == 1 + + assert_equal 1, aggregate.doi end test 'issns counts are included in total aggregation' do aggregate = Metrics::Algorithms.new.generate - assert aggregate.issn == 1 + + assert_equal 1, aggregate.issn end test 'isbns counts are included in total aggregation' do aggregate = Metrics::Algorithms.new.generate - assert aggregate.isbn == 1 + + assert_equal 1, aggregate.isbn end test 'pmids counts are included in total aggregation' do aggregate = Metrics::Algorithms.new.generate - assert aggregate.pmid == 2 + + assert_equal 2, aggregate.pmid end test 'journal exact counts are included in total aggregation' do aggregate = Metrics::Algorithms.new.generate - assert aggregate.journal_exact == 2 + + assert_equal 2, aggregate.journal_exact end test 'unmatched counts are included are included in total aggregation' do aggregate = Metrics::Algorithms.new.generate - assert aggregate.unmatched == 2 + + assert_equal 2, aggregate.unmatched end test 'creating lots of searchevents leads to correct data for total' do @@ -154,11 +166,11 @@ class Algorithms < ActiveSupport::TestCase aggregate = Metrics::Algorithms.new.generate - assert doi_expected_count == aggregate.doi - assert issn_expected_count == aggregate.issn - assert isbn_expected_count == aggregate.isbn - assert pmid_expected_count == aggregate.pmid - assert journal_exact_count == aggregate.journal_exact - assert unmatched_expected_count == aggregate.unmatched + assert_equal doi_expected_count, aggregate.doi + assert_equal issn_expected_count, aggregate.issn + assert_equal isbn_expected_count, aggregate.isbn + assert_equal pmid_expected_count, aggregate.pmid + assert_equal journal_exact_count, aggregate.journal_exact + assert_equal unmatched_expected_count, aggregate.unmatched end end diff --git a/test/models/search_event_test.rb b/test/models/search_event_test.rb index ec921d2..5f98cf2 100644 --- a/test/models/search_event_test.rb +++ b/test/models/search_event_test.rb @@ -15,27 +15,31 @@ class SearchEventTest < ActiveSupport::TestCase test 'term is required' do s = search_events('timdex_cool') - assert(s.valid?) + + assert_predicate(s, :valid?) s.term = nil - refute(s.valid?) + + assert_not_predicate(s, :valid?) end test 'source is required' do s = search_events('timdex_cool') - assert(s.valid?) + + assert_predicate(s, :valid?) s.source = nil - refute(s.valid?) + + assert_not_predicate(s, :valid?) end test 'monthly scope returns requested month of SearchEvents' do - assert SearchEvent.all.include?(search_events(:current_month_pmid)) - assert SearchEvent.single_month(Time.now).include?(search_events(:current_month_pmid)) + assert_includes SearchEvent.all, search_events(:current_month_pmid) + assert_includes SearchEvent.single_month(Time.now), search_events(:current_month_pmid) end test 'monthly scope does not return SearchEvents outside the requested month' do - assert SearchEvent.all.include?(search_events(:old_month_pmid)) - refute SearchEvent.single_month(Time.now).include?(search_events(:old_month_pmid)) + assert_includes SearchEvent.all, search_events(:old_month_pmid) + assert_not_includes SearchEvent.single_month(Time.now), search_events(:old_month_pmid) end end diff --git a/test/models/standard_identifiers_test.rb b/test/models/standard_identifiers_test.rb index cdf8154..24df967 100644 --- a/test/models/standard_identifiers_test.rb +++ b/test/models/standard_identifiers_test.rb @@ -17,6 +17,7 @@ class StandardIdentifiersTest < ActiveSupport::TestCase samples.each do |isbn| actual = StandardIdentifiers.new(isbn).identifiers + assert_equal(isbn, actual[:isbn]) end end @@ -28,6 +29,7 @@ class StandardIdentifiersTest < ActiveSupport::TestCase samples.each do |isbn| actual = StandardIdentifiers.new(isbn).identifiers + assert_equal(isbn, actual[:isbn]) end end @@ -37,6 +39,7 @@ class StandardIdentifiersTest < ActiveSupport::TestCase samples.each do |notisbn| actual = StandardIdentifiers.new(notisbn).identifiers + assert_nil(actual[:isbn]) end end @@ -49,12 +52,14 @@ class StandardIdentifiersTest < ActiveSupport::TestCase samples.each do |notisbn| actual = StandardIdentifiers.new(notisbn).identifiers + assert_nil(actual[:isbn]) end end test 'ISSNs detected in a string' do actual = StandardIdentifiers.new('test 0250-6335 test').identifiers + assert_equal('0250-6335', actual[:issn]) end @@ -63,6 +68,7 @@ class StandardIdentifiersTest < ActiveSupport::TestCase samples.each do |issn| actual = StandardIdentifiers.new(issn).identifiers + assert_equal(issn, actual[:issn]) end end @@ -72,12 +78,14 @@ class StandardIdentifiersTest < ActiveSupport::TestCase samples.each do |notissn| actual = StandardIdentifiers.new(notissn).identifiers + assert_nil(actual[:issn]) end end test 'ISSNs need boundaries' do actual = StandardIdentifiers.new('12345-5678 1234-56789').identifiers + assert_nil(actual[:issn]) end @@ -109,6 +117,7 @@ class StandardIdentifiersTest < ActiveSupport::TestCase ] samples.each do |notissn| actual = StandardIdentifiers.new(notissn).identifiers + assert_nil(actual[:issn]) end end @@ -122,6 +131,7 @@ class StandardIdentifiersTest < ActiveSupport::TestCase ] samples.each do |issn| actual = StandardIdentifiers.new(issn).identifiers + assert_equal(issn, actual[:issn]) end end @@ -129,6 +139,7 @@ class StandardIdentifiersTest < ActiveSupport::TestCase test 'doi detected in string' do actual = StandardIdentifiers.new('"Quantum tomography: Measured measurement", Markus Aspelmeyer, nature physics "\ "January 2009, Volume 5, No 1, pp11-12; [ doi:10.1038/nphys1170 ]').identifiers + assert_equal('10.1038/nphys1170', actual[:doi]) end @@ -138,6 +149,7 @@ class StandardIdentifiersTest < ActiveSupport::TestCase samples.each do |doi| actual = StandardIdentifiers.new(doi).identifiers + assert_equal(doi, actual[:doi]) end end @@ -147,12 +159,14 @@ class StandardIdentifiersTest < ActiveSupport::TestCase samples.each do |notdoi| actual = StandardIdentifiers.new(notdoi).identifiers + assert_nil(actual[:notdoi]) end end test 'pmid detected in string' do actual = StandardIdentifiers.new('Citation and stuff PMID: 35648703 more stuff.').identifiers + assert_equal('PMID: 35648703', actual[:pmid]) end @@ -161,6 +175,7 @@ class StandardIdentifiersTest < ActiveSupport::TestCase samples.each do |pmid| actual = StandardIdentifiers.new(pmid).identifiers + assert_equal(pmid, actual[:pmid]) end end @@ -170,6 +185,7 @@ class StandardIdentifiersTest < ActiveSupport::TestCase samples.each do |notpmid| actual = StandardIdentifiers.new(notpmid).identifiers + assert_nil(actual[:pmid]) end end diff --git a/test/models/term_test.rb b/test/models/term_test.rb index dad33f7..5f09fc4 100644 --- a/test/models/term_test.rb +++ b/test/models/term_test.rb @@ -37,7 +37,7 @@ class TermTest < ActiveSupport::TestCase term.destroy assert_equal((term_pre_count - 1), Term.count) - assert(SearchEvent.count < event_pre_count) + assert_operator(SearchEvent.count, :<, event_pre_count) end test 'destroying a SearchEvent does not delete the Term' do @@ -52,6 +52,6 @@ class TermTest < ActiveSupport::TestCase t.reload assert_equal(events_count - 1, t.search_events.count) - assert(t.valid?) + assert_predicate(t, :valid?) end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 9697d86..5b7a9d1 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -14,38 +14,45 @@ class UserTest < ActiveSupport::TestCase test 'user valid with uid and email' do user = users(:valid) - assert user.uid.present? - assert user.email.present? - assert user.valid? + + assert_predicate user.uid, :present? + assert_predicate user.email, :present? + assert_predicate user, :valid? end test 'user invalid without uid' do user = users(:valid) - assert user.valid? + + assert_predicate user, :valid? user.uid = nil user.save + assert_not user.valid? end test 'user invalid without email' do user = users(:valid) - assert user.valid? + + assert_predicate user, :valid? user.email = nil user.save + assert_not user.valid? end test 'admin user is valid' do user = users(:admin) - assert user.admin? - assert user.valid? + + assert_predicate user, :admin? + assert_predicate user, :valid? end test 'non-admin user is valid' do user = users(:basic) + assert_not user.admin? - assert user.valid? + assert_predicate user, :valid? end end diff --git a/test/tasks/suggested_resource_rake_test.rb b/test/tasks/suggested_resource_rake_test.rb index c1efd9e..51e5b6a 100644 --- a/test/tasks/suggested_resource_rake_test.rb +++ b/test/tasks/suggested_resource_rake_test.rb @@ -16,8 +16,9 @@ def setup remote_file = 'http://static.lndo.site/suggested_resources.csv' Rake::Task['suggested_resources:reload'].invoke(remote_file) end - refute_equal records_before, Detector::SuggestedResource.count - refute_equal first_record_before, Detector::SuggestedResource.first + + assert_not_equal records_before, Detector::SuggestedResource.count + assert_not_equal first_record_before, Detector::SuggestedResource.first end test 'reload task errors without a file argument' do @@ -29,7 +30,7 @@ def setup test 'reload errors on a local file' do error = assert_raises(ArgumentError) do - local_file = Rails.root.join('test', 'fixtures', 'files', 'suggested_resources.csv').to_s + local_file = Rails.root.join('test/fixtures/files/suggested_resources.csv').to_s Rake::Task['suggested_resources:reload'].invoke(local_file) end assert_equal 'Local files are not supported yet', error.message @@ -63,6 +64,7 @@ def setup remote_file = 'http://static.lndo.site/suggested_resources_extra.csv' Rake::Task['suggested_resources:reload'].invoke(remote_file) end - refute_equal records_before, Detector::SuggestedResource.count + + assert_not_equal records_before, Detector::SuggestedResource.count end end diff --git a/test/test_helper.rb b/test/test_helper.rb index ba0bc18..2207e14 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -28,7 +28,7 @@ class TestCase SimpleCov.command_name "#{SimpleCov.command_name}-#{worker}" end - parallelize_teardown do |worker| + parallelize_teardown do |_worker| SimpleCov.result end From 3ef6f5316094a6a062e991d410f8f5328e4fa266 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 15:38:13 -0400 Subject: [PATCH 09/16] Rubocop safeish test cleanup `bundle exec rubocop -A test/` --- test/controllers/graphql_controller_test.rb | 12 ++++++------ test/integration/admin_dashboard_test.rb | 2 ++ test/integration/authentication_test.rb | 2 ++ test/models/fake_auth_test.rb | 16 +++++++++------- test/models/search_event_test.rb | 4 ++-- test/models/user_test.rb | 2 ++ 6 files changed, 23 insertions(+), 15 deletions(-) diff --git a/test/controllers/graphql_controller_test.rb b/test/controllers/graphql_controller_test.rb index 613de09..2647775 100644 --- a/test/controllers/graphql_controller_test.rb +++ b/test/controllers/graphql_controller_test.rb @@ -14,13 +14,13 @@ class GraphqlControllerTest < ActionDispatch::IntegrationTest }' } assert_equal(200, response.status) - json = JSON.parse(response.body) + json = response.parsed_body term_id = Term.last.id assert_equal 'bento', json['data']['logSearchEvent']['source'] assert_equal term_id, json['data']['logSearchEvent']['termId'] - assert_equal Date.today, json['data']['logSearchEvent']['createdAt'].to_date - assert_equal Date.today, json['data']['logSearchEvent']['updatedAt'].to_date + assert_equal Time.zone.today, json['data']['logSearchEvent']['createdAt'].to_date + assert_equal Time.zone.today, json['data']['logSearchEvent']['updatedAt'].to_date end test 'search event query creates a new term if one does not exist' do @@ -64,7 +64,7 @@ class GraphqlControllerTest < ActionDispatch::IntegrationTest } }' } - json = JSON.parse(response.body) + json = response.parsed_body assert_equal('doi', json['data']['logSearchEvent']['standardIdentifiers'].first['kind']) assert_equal('10.1038/nphys1170', json['data']['logSearchEvent']['standardIdentifiers'].first['value']) @@ -77,7 +77,7 @@ class GraphqlControllerTest < ActionDispatch::IntegrationTest } }' } - json = JSON.parse(response.body) + json = response.parsed_body assert_equal('10.1038/nphys1170', json['data']['logSearchEvent']['phrase']) end @@ -99,7 +99,7 @@ class GraphqlControllerTest < ActionDispatch::IntegrationTest } }' } - json = JSON.parse(response.body) + json = response.parsed_body assert_equal('Measured measurement', json['data']['logSearchEvent']['standardIdentifiers'].first['details']['title']) diff --git a/test/integration/admin_dashboard_test.rb b/test/integration/admin_dashboard_test.rb index d31a5c4..2f91b81 100644 --- a/test/integration/admin_dashboard_test.rb +++ b/test/integration/admin_dashboard_test.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'test_helper' class AdminDashboardTest < ActionDispatch::IntegrationTest diff --git a/test/integration/authentication_test.rb b/test/integration/authentication_test.rb index a0ceff7..0a095b7 100644 --- a/test/integration/authentication_test.rb +++ b/test/integration/authentication_test.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'test_helper' class AuthenticationTest < ActionDispatch::IntegrationTest diff --git a/test/models/fake_auth_test.rb b/test/models/fake_auth_test.rb index d1f9a26..e846fc7 100644 --- a/test/models/fake_auth_test.rb +++ b/test/models/fake_auth_test.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'test_helper' class FakeAuthTest < ActiveSupport::TestCase @@ -7,7 +9,7 @@ class FakeAuthTest < ActiveSupport::TestCase ClimateControl.modify( FAKE_AUTH_ENABLED: 'false' ) do - assert_equal(false, FakeAuthConfig.fake_auth_enabled?) + assert_not_predicate(FakeAuthConfig, :fake_auth_enabled?) end end @@ -16,7 +18,7 @@ class FakeAuthTest < ActiveSupport::TestCase FAKE_AUTH_ENABLED: 'false', HEROKU_APP_NAME: 'tacos-api-pipeline-pr-1' ) do - assert_equal(false, FakeAuthConfig.fake_auth_enabled?) + assert_not_predicate(FakeAuthConfig, :fake_auth_enabled?) end end @@ -25,33 +27,33 @@ class FakeAuthTest < ActiveSupport::TestCase FAKE_AUTH_ENABLED: 'true', HEROKU_APP_NAME: 'tacos-api-pipeline-pr-1' ) do - assert_equal(true, FakeAuthConfig.fake_auth_enabled?) + assert_predicate(FakeAuthConfig, :fake_auth_enabled?) end ClimateControl.modify( FAKE_AUTH_ENABLED: 'true', HEROKU_APP_NAME: 'tacos-api-pipeline-pr-500' ) do - assert_equal(true, FakeAuthConfig.fake_auth_enabled?) + assert_predicate(FakeAuthConfig, :fake_auth_enabled?) end end test 'fakeauth enabled no HEROKU_APP_NAME' do ClimateControl.modify FAKE_AUTH_ENABLED: 'true' do - assert_equal(false, FakeAuthConfig.fake_auth_enabled?) + assert_not_predicate(FakeAuthConfig, :fake_auth_enabled?) end end test 'fakeauth enabled production app name' do ClimateControl.modify FAKE_AUTH_ENABLED: 'true', HEROKU_APP_NAME: 'tacos-prod' do - assert_equal(false, FakeAuthConfig.fake_auth_enabled?) + assert_not_predicate(FakeAuthConfig, :fake_auth_enabled?) end end test 'fakeauth enabled staging app name' do ClimateControl.modify FAKE_AUTH_ENABLED: 'true', HEROKU_APP_NAME: 'tacos-stage' do - assert_equal(false, FakeAuthConfig.fake_auth_enabled?) + assert_not_predicate(FakeAuthConfig, :fake_auth_enabled?) end end end diff --git a/test/models/search_event_test.rb b/test/models/search_event_test.rb index 5f98cf2..7e67007 100644 --- a/test/models/search_event_test.rb +++ b/test/models/search_event_test.rb @@ -35,11 +35,11 @@ class SearchEventTest < ActiveSupport::TestCase test 'monthly scope returns requested month of SearchEvents' do assert_includes SearchEvent.all, search_events(:current_month_pmid) - assert_includes SearchEvent.single_month(Time.now), search_events(:current_month_pmid) + assert_includes SearchEvent.single_month(Time.zone.now), search_events(:current_month_pmid) end test 'monthly scope does not return SearchEvents outside the requested month' do assert_includes SearchEvent.all, search_events(:old_month_pmid) - assert_not_includes SearchEvent.single_month(Time.now), search_events(:old_month_pmid) + assert_not_includes SearchEvent.single_month(Time.zone.now), search_events(:old_month_pmid) end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 5b7a9d1..f554370 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + # == Schema Information # # Table name: users From 829401175a97706900c0affa417a5b41b65c0abd Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 15:40:04 -0400 Subject: [PATCH 10/16] Rubocop safeish task cleanup `bundle exec rubocop -A lib/tasks/` --- lib/tasks/journals.rake | 4 ++-- lib/tasks/search_event_loader.rake | 4 ++-- lib/tasks/suggested_resources.rake | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/tasks/journals.rake b/lib/tasks/journals.rake index 75cd577..0466ed2 100644 --- a/lib/tasks/journals.rake +++ b/lib/tasks/journals.rake @@ -16,7 +16,7 @@ namespace :journals do # In development, use your own. If used in production, use a team Moira list. desc 'Harvest from Open Alex' task :openalex_harvester, %i[email] => :environment do |_task, args| - raise ArgumentError.new, 'Email is required' unless args.email.present? + raise ArgumentError.new, 'Email is required' if args.email.blank? base_url = 'https://api.openalex.org/sources?filter=is_core:true' next_cursor = '*' @@ -81,7 +81,7 @@ namespace :journals do # @param path [String] local file path or URI to a JSON file to load desc 'Load from OpenAlex harvest' task :openalex_loader, %i[file] => :environment do |_task, args| - raise ArgumentError.new, 'File is required' unless args.file.present? + raise ArgumentError.new, 'File is required' if args.file.blank? # does the file look like a path or a URI if URI(args.file).scheme diff --git a/lib/tasks/search_event_loader.rake b/lib/tasks/search_event_loader.rake index 88a80a1..d57b0a5 100644 --- a/lib/tasks/search_event_loader.rake +++ b/lib/tasks/search_event_loader.rake @@ -18,8 +18,8 @@ namespace :search_events do # @param source [String] source name to load the data under desc 'Load search_events from csv' task :csv_loader, %i[path source] => :environment do |_task, args| - raise ArgumentError.new, 'Path is required' unless args.path.present? - raise ArgumentError.new, 'Source is required' unless args.source.present? + raise ArgumentError.new, 'Path is required' if args.path.blank? + raise ArgumentError.new, 'Source is required' if args.source.blank? Rails.logger.info("Loading data from #{args.path}") diff --git a/lib/tasks/suggested_resources.rake b/lib/tasks/suggested_resources.rake index 9c0272f..d444272 100644 --- a/lib/tasks/suggested_resources.rake +++ b/lib/tasks/suggested_resources.rake @@ -6,7 +6,7 @@ namespace :suggested_resources do # we do need a way to import records from a CSV file. desc 'Replace all Suggested Resources from CSV' task :reload, [:addr] => :environment do |_task, args| - raise ArgumentError.new, 'URL is required' unless args.addr.present? + raise ArgumentError.new, 'URL is required' if args.addr.blank? raise ArgumentError.new, 'Local files are not supported yet' unless URI(args.addr).scheme From 0a3e132686b19411cf0d01b5c3bbaf6b72688500 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 15:59:17 -0400 Subject: [PATCH 11/16] Rubocop misc safe cleanup Note: removed changed to `suggested_resource.rake` as it caused a test failure --- .../detector/suggested_resource_dashboard.rb | 14 +++++++------- app/dashboards/search_event_dashboard.rb | 4 ++-- app/dashboards/term_dashboard.rb | 4 ++-- app/dashboards/user_dashboard.rb | 4 ++-- app/mailers/application_mailer.rb | 4 ++-- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/app/dashboards/detector/suggested_resource_dashboard.rb b/app/dashboards/detector/suggested_resource_dashboard.rb index 7fd3240..08bcade 100644 --- a/app/dashboards/detector/suggested_resource_dashboard.rb +++ b/app/dashboards/detector/suggested_resource_dashboard.rb @@ -1,4 +1,4 @@ -require "administrate/base_dashboard" +require 'administrate/base_dashboard' module Detector class Detector::SuggestedResourceDashboard < Administrate::BaseDashboard @@ -15,9 +15,9 @@ class Detector::SuggestedResourceDashboard < Administrate::BaseDashboard title: Field::String, url: Field::String, created_at: Field::DateTime, - updated_at: Field::DateTime, + updated_at: Field::DateTime }.freeze - + # COLLECTION_ATTRIBUTES # an array of attributes that will be displayed on the model's index page. # @@ -29,7 +29,7 @@ class Detector::SuggestedResourceDashboard < Administrate::BaseDashboard phrase title ].freeze - + # SHOW_PAGE_ATTRIBUTES # an array of attributes that will be displayed on the model's show page. SHOW_PAGE_ATTRIBUTES = %i[ @@ -41,7 +41,7 @@ class Detector::SuggestedResourceDashboard < Administrate::BaseDashboard created_at updated_at ].freeze - + # FORM_ATTRIBUTES # an array of attributes that will be displayed # on the model's form (`new` and `edit`) pages. @@ -50,7 +50,7 @@ class Detector::SuggestedResourceDashboard < Administrate::BaseDashboard title url ].freeze - + # COLLECTION_FILTERS # a hash that defines filters that can be used while searching via the search # field of the dashboard. @@ -62,7 +62,7 @@ class Detector::SuggestedResourceDashboard < Administrate::BaseDashboard # open: ->(resources) { resources.where(open: true) } # }.freeze COLLECTION_FILTERS = {}.freeze - + # Overwrite this method to customize how suggested resources are displayed # across all pages of the admin dashboard. # diff --git a/app/dashboards/search_event_dashboard.rb b/app/dashboards/search_event_dashboard.rb index cd4e096..8e9237e 100644 --- a/app/dashboards/search_event_dashboard.rb +++ b/app/dashboards/search_event_dashboard.rb @@ -1,4 +1,4 @@ -require "administrate/base_dashboard" +require 'administrate/base_dashboard' class SearchEventDashboard < Administrate::BaseDashboard # ATTRIBUTE_TYPES @@ -12,7 +12,7 @@ class SearchEventDashboard < Administrate::BaseDashboard source: Field::String, term: Field::BelongsTo, created_at: Field::DateTime, - updated_at: Field::DateTime, + updated_at: Field::DateTime }.freeze # COLLECTION_ATTRIBUTES diff --git a/app/dashboards/term_dashboard.rb b/app/dashboards/term_dashboard.rb index b75dbff..457e141 100644 --- a/app/dashboards/term_dashboard.rb +++ b/app/dashboards/term_dashboard.rb @@ -1,4 +1,4 @@ -require "administrate/base_dashboard" +require 'administrate/base_dashboard' class TermDashboard < Administrate::BaseDashboard # ATTRIBUTE_TYPES @@ -12,7 +12,7 @@ class TermDashboard < Administrate::BaseDashboard phrase: Field::String, search_events: Field::HasMany, created_at: Field::DateTime, - updated_at: Field::DateTime, + updated_at: Field::DateTime }.freeze # COLLECTION_ATTRIBUTES diff --git a/app/dashboards/user_dashboard.rb b/app/dashboards/user_dashboard.rb index 7fe9659..2069c7f 100644 --- a/app/dashboards/user_dashboard.rb +++ b/app/dashboards/user_dashboard.rb @@ -1,4 +1,4 @@ -require "administrate/base_dashboard" +require 'administrate/base_dashboard' class UserDashboard < Administrate::BaseDashboard # ATTRIBUTE_TYPES @@ -13,7 +13,7 @@ class UserDashboard < Administrate::BaseDashboard email: Field::String, uid: Field::String, created_at: Field::DateTime, - updated_at: Field::DateTime, + updated_at: Field::DateTime }.freeze # COLLECTION_ATTRIBUTES diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 3c34c81..286b223 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -1,4 +1,4 @@ class ApplicationMailer < ActionMailer::Base - default from: "from@example.com" - layout "mailer" + default from: 'from@example.com' + layout 'mailer' end From cdc2ed87799dc2fab8936fad44e41e07db5b28ec Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 16:02:31 -0400 Subject: [PATCH 12/16] Rubocop misc safeish cleanup `bundle exec rubocop -A` Notes: auto cleanup on `app/dashboards/detector/suggested_resource_dashboard.rb` was incorrect as the initial code seemed a bit odd. I believe my manual fix is correct. Removed autofix for tacos_schema.rb as it removed a good example of how to handle errors --- app/channels/application_cable/channel.rb | 2 ++ app/channels/application_cable/connection.rb | 2 ++ app/dashboards/detector/suggested_resource_dashboard.rb | 4 +++- app/dashboards/search_event_dashboard.rb | 2 ++ app/dashboards/term_dashboard.rb | 2 ++ app/dashboards/user_dashboard.rb | 2 ++ app/helpers/application_helper.rb | 2 ++ app/jobs/application_job.rb | 2 ++ app/mailers/application_mailer.rb | 2 ++ 9 files changed, 19 insertions(+), 1 deletion(-) diff --git a/app/channels/application_cable/channel.rb b/app/channels/application_cable/channel.rb index d672697..9aec230 100644 --- a/app/channels/application_cable/channel.rb +++ b/app/channels/application_cable/channel.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module ApplicationCable class Channel < ActionCable::Channel::Base end diff --git a/app/channels/application_cable/connection.rb b/app/channels/application_cable/connection.rb index 0ff5442..8d6c2a1 100644 --- a/app/channels/application_cable/connection.rb +++ b/app/channels/application_cable/connection.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module ApplicationCable class Connection < ActionCable::Connection::Base end diff --git a/app/dashboards/detector/suggested_resource_dashboard.rb b/app/dashboards/detector/suggested_resource_dashboard.rb index 08bcade..83c6f6a 100644 --- a/app/dashboards/detector/suggested_resource_dashboard.rb +++ b/app/dashboards/detector/suggested_resource_dashboard.rb @@ -1,7 +1,9 @@ +# frozen_string_literal: true + require 'administrate/base_dashboard' module Detector - class Detector::SuggestedResourceDashboard < Administrate::BaseDashboard + class SuggestedResourceDashboard < Administrate::BaseDashboard # ATTRIBUTE_TYPES # a hash that describes the type of each of the model's fields. # diff --git a/app/dashboards/search_event_dashboard.rb b/app/dashboards/search_event_dashboard.rb index 8e9237e..922d58a 100644 --- a/app/dashboards/search_event_dashboard.rb +++ b/app/dashboards/search_event_dashboard.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'administrate/base_dashboard' class SearchEventDashboard < Administrate::BaseDashboard diff --git a/app/dashboards/term_dashboard.rb b/app/dashboards/term_dashboard.rb index 457e141..c8d1090 100644 --- a/app/dashboards/term_dashboard.rb +++ b/app/dashboards/term_dashboard.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'administrate/base_dashboard' class TermDashboard < Administrate::BaseDashboard diff --git a/app/dashboards/user_dashboard.rb b/app/dashboards/user_dashboard.rb index 2069c7f..001d81e 100644 --- a/app/dashboards/user_dashboard.rb +++ b/app/dashboards/user_dashboard.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require 'administrate/base_dashboard' class UserDashboard < Administrate::BaseDashboard diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index de6be79..15b06f0 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -1,2 +1,4 @@ +# frozen_string_literal: true + module ApplicationHelper end diff --git a/app/jobs/application_job.rb b/app/jobs/application_job.rb index d394c3d..bef3959 100644 --- a/app/jobs/application_job.rb +++ b/app/jobs/application_job.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class ApplicationJob < ActiveJob::Base # Automatically retry jobs that encountered a deadlock # retry_on ActiveRecord::Deadlocked diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 286b223..d84cb6e 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class ApplicationMailer < ActionMailer::Base default from: 'from@example.com' layout 'mailer' From 5e639412a27c9465f77ce85de9462a3094c6c4ed Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 16:13:01 -0400 Subject: [PATCH 13/16] Rubocop: add todolist We have a fair amount of not-autofixable code we have already accepted into the repository. Rather than failing on the same code each time we run the linters, we can note which code is known to be not following our standards and only fail the linters on changed code. `bundle exec rubocop --auto-gen-config` is the way to do that. --- .rubocop.yml | 2 + .rubocop_todo.yml | 126 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) create mode 100644 .rubocop_todo.yml diff --git a/.rubocop.yml b/.rubocop.yml index 6d2b9fc..c389bc6 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,3 +1,5 @@ +inherit_from: .rubocop_todo.yml + require: - rubocop-capybara - rubocop-graphql diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml new file mode 100644 index 0000000..012d781 --- /dev/null +++ b/.rubocop_todo.yml @@ -0,0 +1,126 @@ +# This configuration was generated by +# `rubocop --auto-gen-config` +# on 2024-08-12 20:10:38 UTC using RuboCop version 1.65.0. +# The point is for the user to remove these configuration records +# one by one as the offenses are removed from the code base. +# Note that changes in the inspected code, or installation of new +# versions of RuboCop, may require this file to be generated again. + +# Offense count: 3 +GraphQL/ArgumentDescription: + Exclude: + - 'app/graphql/types/query_type.rb' + +# Offense count: 28 +GraphQL/FieldDescription: + Exclude: + - 'app/graphql/types/details_type.rb' + - 'app/graphql/types/search_event_type.rb' + - 'app/graphql/types/standard_identifiers_type.rb' + - 'app/graphql/types/term_type.rb' + +# Offense count: 1 +# Configuration parameters: Include. +# Include: **/graphql/**/*_schema.rb +GraphQL/MaxComplexitySchema: + Exclude: + - 'app/graphql/tacos_schema.rb' + +# Offense count: 1 +# Configuration parameters: Include. +# Include: **/graphql/**/*_schema.rb +GraphQL/MaxDepthSchema: + Exclude: + - 'app/graphql/tacos_schema.rb' + +# Offense count: 7 +GraphQL/ObjectDescription: + Exclude: + - 'app/graphql/types/details_type.rb' + - 'app/graphql/types/mutation_type.rb' + - 'app/graphql/types/node_type.rb' + - 'app/graphql/types/query_type.rb' + - 'app/graphql/types/search_event_type.rb' + - 'app/graphql/types/standard_identifiers_type.rb' + - 'app/graphql/types/term_type.rb' + +# Offense count: 1 +Lint/DuplicateMethods: + Exclude: + - 'app/graphql/types/query_type.rb' + +# Offense count: 1 +# This cop supports unsafe autocorrection (--autocorrect-all). +# Configuration parameters: AutoCorrect. +Lint/UselessMethodDefinition: + Exclude: + - 'app/graphql/tacos_schema.rb' + +# Offense count: 1 +# Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes. +Metrics/AbcSize: + Max: 18 + +# Offense count: 2 +# Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. +# AllowedMethods: refine +Metrics/BlockLength: + Max: 60 + +# Offense count: 4 +# Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns. +Metrics/MethodLength: + Max: 16 + +# Offense count: 1 +Minitest/AssertRaisesCompoundBody: + Exclude: + - 'test/tasks/suggested_resource_rake_test.rb' + +# Offense count: 2 +Minitest/MultipleAssertions: + Max: 5 + +# Offense count: 1 +# Configuration parameters: MinNameLength, AllowNamesEndingInNumbers, AllowedNames, ForbiddenNames. +# AllowedNames: as, at, by, cc, db, id, if, in, io, ip, of, on, os, pp, to +Naming/MethodParameterName: + Exclude: + - 'app/controllers/graphql_controller.rb' + +# Offense count: 6 +# Configuration parameters: EnforcedStyle, CheckMethodNames, CheckSymbols, AllowedIdentifiers, AllowedPatterns. +# SupportedStyles: snake_case, normalcase, non_integer +# AllowedIdentifiers: capture3, iso8601, rfc1123_date, rfc822, rfc2822, rfc3339, x86_64 +Naming/VariableNumber: + Exclude: + - 'test/models/metrics/algorithms_test.rb' + +# Offense count: 1 +# This cop supports safe autocorrection (--autocorrect). +Performance/StringReplacement: + Exclude: + - 'lib/tasks/suggested_resources.rake' + +# Offense count: 3 +Rails/I18nLocaleTexts: + Exclude: + - 'app/controllers/admin/application_controller.rb' + - 'app/controllers/application_controller.rb' + +# Offense count: 1 +Security/Open: + Exclude: + - 'lib/tasks/journals.rake' + +# Offense count: 25 +# Configuration parameters: AllowedConstants. +Style/Documentation: + Enabled: false + +# Offense count: 6 +# This cop supports safe autocorrection (--autocorrect). +# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, AllowedPatterns. +# URISchemes: http, https +Layout/LineLength: + Max: 310 From db8b42bf909a0b8b08503a0fda28e2e400f85a3f Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 16:15:05 -0400 Subject: [PATCH 14/16] Update .rubocop_todo.yml Adds a few things the autogen config misses. Until we fix these, the autogen config will be annoying as it will keep losing these exclusions. --- .rubocop_todo.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 012d781..6eefae6 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -36,6 +36,18 @@ GraphQL/MaxDepthSchema: # Offense count: 7 GraphQL/ObjectDescription: Exclude: + - 'app/graphql/mutations/base_mutation.rb' + - 'app/graphql/tacos_schema.rb' + - 'app/graphql/types/base_argument.rb' + - 'app/graphql/types/base_connection.rb' + - 'app/graphql/types/base_edge.rb' + - 'app/graphql/types/base_enum.rb' + - 'app/graphql/types/base_field.rb' + - 'app/graphql/types/base_input_object.rb' + - 'app/graphql/types/base_interface.rb' + - 'app/graphql/types/base_object.rb' + - 'app/graphql/types/base_scalar.rb' + - 'app/graphql/types/base_union.rb' - 'app/graphql/types/details_type.rb' - 'app/graphql/types/mutation_type.rb' - 'app/graphql/types/node_type.rb' From 0e91ff57054a402e5c7da66a305e68d54c7f6ef4 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 16:17:00 -0400 Subject: [PATCH 15/16] Rubocop cleanup: Security/Open --- .rubocop_todo.yml | 5 ----- lib/tasks/journals.rake | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 6eefae6..a829bdc 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -120,11 +120,6 @@ Rails/I18nLocaleTexts: - 'app/controllers/admin/application_controller.rb' - 'app/controllers/application_controller.rb' -# Offense count: 1 -Security/Open: - Exclude: - - 'lib/tasks/journals.rake' - # Offense count: 25 # Configuration parameters: AllowedConstants. Style/Documentation: diff --git a/lib/tasks/journals.rake b/lib/tasks/journals.rake index 0466ed2..eff5bc8 100644 --- a/lib/tasks/journals.rake +++ b/lib/tasks/journals.rake @@ -86,7 +86,7 @@ namespace :journals do # does the file look like a path or a URI if URI(args.file).scheme Rails.logger.info("Loading data from remote file #{args.file}") - data = URI.open(args.file, 'rb', &:read) + data = URI.parse(args.file).open('rb', &:read) else Rails.logger.info("Loading data from local file #{args.file}") data = File.read(args.file) From f2b0e7d6387f7ebde2073bec884e8c3a1b0197a3 Mon Sep 17 00:00:00 2001 From: Jeremy Prevost Date: Mon, 12 Aug 2024 16:28:15 -0400 Subject: [PATCH 16/16] Rubocop cleanup: Naming/MethodParameterName --- .rubocop_todo.yml | 7 ------- app/controllers/graphql_controller.rb | 8 ++++---- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index a829bdc..dfecc96 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -93,13 +93,6 @@ Minitest/AssertRaisesCompoundBody: Minitest/MultipleAssertions: Max: 5 -# Offense count: 1 -# Configuration parameters: MinNameLength, AllowNamesEndingInNumbers, AllowedNames, ForbiddenNames. -# AllowedNames: as, at, by, cc, db, id, if, in, io, ip, of, on, os, pp, to -Naming/MethodParameterName: - Exclude: - - 'app/controllers/graphql_controller.rb' - # Offense count: 6 # Configuration parameters: EnforcedStyle, CheckMethodNames, CheckSymbols, AllowedIdentifiers, AllowedPatterns. # SupportedStyles: snake_case, normalcase, non_integer diff --git a/app/controllers/graphql_controller.rb b/app/controllers/graphql_controller.rb index 8d629d8..951a568 100644 --- a/app/controllers/graphql_controller.rb +++ b/app/controllers/graphql_controller.rb @@ -44,10 +44,10 @@ def prepare_variables(variables_param) end end - def handle_error_in_development(e) - logger.error e.message - logger.error e.backtrace.join("\n") + def handle_error_in_development(error) + logger.error error.message + logger.error error.backtrace.join("\n") - render json: { errors: [{ message: e.message, backtrace: e.backtrace }], data: {} }, status: :internal_server_error + render json: { errors: [{ message: error.message, backtrace: error.backtrace }], data: {} }, status: :internal_server_error end end