From d19d34162263cc4715a56415212c907f5c120ddb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 17 Mar 2024 21:53:15 +0100 Subject: [PATCH 01/18] Remove unused lib/assets folder We use vendor/assets and app/assets; the purpose of lib/assets isn't that clear, though. According to the Rails guides: > lib/assets is for your own libraries' code that doesn't really fit > into the scope of the application or those libraries which are shared > across applications. So it must be something for companies having several Rails applications, which isn't our case. Furthermore, this text has been removed from the Rails guides for version 7.1, so this folder might be a legacy folder. --- lib/assets/.keep | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 lib/assets/.keep diff --git a/lib/assets/.keep b/lib/assets/.keep deleted file mode 100644 index e69de29bb..000000000 From f8c97b9bb9c26b9f837d48926371d6db7dbb6a4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 17 Mar 2024 22:40:11 +0100 Subject: [PATCH 02/18] Remove monkey-patch of the Numeric class This monkey-patch doesn't seem to be working with Zeitwerk, and we were only using it in one place, so the easiest way to solve the problem is to remove it. Note that, in the process, we're changing the operation so `* 100` appears before the division, so it's consistent with other places where we do similar things (like the `supports_percentage` method in the proposals helper). --- app/helpers/votes_helper.rb | 2 +- app/models/debate.rb | 2 -- lib/numeric.rb | 5 ----- 3 files changed, 1 insertion(+), 8 deletions(-) delete mode 100644 lib/numeric.rb diff --git a/app/helpers/votes_helper.rb b/app/helpers/votes_helper.rb index 8292717e4..9772a8f04 100644 --- a/app/helpers/votes_helper.rb +++ b/app/helpers/votes_helper.rb @@ -1,6 +1,6 @@ module VotesHelper def debate_percentage_of_likes(debate) - debate.likes.percent_of(debate.total_votes) + (debate.likes.to_f * 100 / debate.total_votes).to_i end def votes_percentage(vote, debate) diff --git a/app/models/debate.rb b/app/models/debate.rb index 4316b4777..c4ca74af0 100644 --- a/app/models/debate.rb +++ b/app/models/debate.rb @@ -1,5 +1,3 @@ -require "numeric" - class Debate < ApplicationRecord include Flaggable include Taggable diff --git a/lib/numeric.rb b/lib/numeric.rb deleted file mode 100644 index ab9b212b9..000000000 --- a/lib/numeric.rb +++ /dev/null @@ -1,5 +0,0 @@ -class Numeric - def percent_of(n) - (to_f / n * 100).to_i - end -end From 09eddec663a39af9b9a815d528b51a9d041fb211 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 25 Jul 2023 05:37:42 +0200 Subject: [PATCH 03/18] Remove unnecessary require statements Since we autoload the `lib` folder, there's no need to manually require the files inside it. --- app/controllers/application_controller.rb | 2 -- app/controllers/management/sessions_controller.rb | 2 -- config/initializers/age.rb | 1 - 3 files changed, 5 deletions(-) delete mode 100644 config/initializers/age.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index acd3ba587..76092d526 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,5 +1,3 @@ -require "application_responder" - class ApplicationController < ActionController::Base include TenantVariants include GlobalizeFallbacks diff --git a/app/controllers/management/sessions_controller.rb b/app/controllers/management/sessions_controller.rb index 2a36ded0a..0e4168975 100644 --- a/app/controllers/management/sessions_controller.rb +++ b/app/controllers/management/sessions_controller.rb @@ -1,5 +1,3 @@ -require "manager_authenticator" - class Management::SessionsController < ActionController::Base include TenantVariants include GlobalizeFallbacks diff --git a/config/initializers/age.rb b/config/initializers/age.rb deleted file mode 100644 index d0928527e..000000000 --- a/config/initializers/age.rb +++ /dev/null @@ -1 +0,0 @@ -require "age" From 913b93aea72e49bbcca8f5c6be849485cd5505bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 10 Apr 2024 16:12:40 +0200 Subject: [PATCH 04/18] Fix DocumentParser being included for all objects --- lib/census_api.rb | 3 ++- lib/local_census.rb | 3 ++- lib/remote_census_api.rb | 3 ++- spec/lib/document_parser_spec.rb | 27 ++++++++++++++++++--------- 4 files changed, 24 insertions(+), 12 deletions(-) diff --git a/lib/census_api.rb b/lib/census_api.rb index 09fcbcc87..d290a1bbf 100644 --- a/lib/census_api.rb +++ b/lib/census_api.rb @@ -1,5 +1,6 @@ -include DocumentParser class CensusApi + include DocumentParser + def call(document_type, document_number) response = nil get_document_number_variants(document_type, document_number).each do |variant| diff --git a/lib/local_census.rb b/lib/local_census.rb index 1f6d3f903..dc566f5fb 100644 --- a/lib/local_census.rb +++ b/lib/local_census.rb @@ -1,5 +1,6 @@ -include DocumentParser class LocalCensus + include DocumentParser + def call(document_type, document_number) record = nil get_document_number_variants(document_type, document_number).each do |variant| diff --git a/lib/remote_census_api.rb b/lib/remote_census_api.rb index c5db70365..99ed06445 100644 --- a/lib/remote_census_api.rb +++ b/lib/remote_census_api.rb @@ -1,5 +1,6 @@ -include DocumentParser class RemoteCensusApi + include DocumentParser + def call(document_type, document_number, date_of_birth, postal_code) response = nil get_document_number_variants(document_type, document_number).each do |variant| diff --git a/spec/lib/document_parser_spec.rb b/spec/lib/document_parser_spec.rb index 6162bc9e7..be8b254ae 100644 --- a/spec/lib/document_parser_spec.rb +++ b/spec/lib/document_parser_spec.rb @@ -1,35 +1,44 @@ require "rails_helper" -include DocumentParser describe DocumentParser do + before do + dummy_class = Class.new do + include DocumentParser + end + + stub_const("DummyClass", dummy_class) + end + + let(:dummy) { DummyClass.new } + describe "#get_document_number_variants" do it "returns no variants when document_number is not defined" do - expect(DocumentParser.get_document_number_variants("1", "")).to be_empty + expect(dummy.get_document_number_variants("1", "")).to be_empty end it "trims and cleans up entry" do - expect(DocumentParser.get_document_number_variants(2, " 1 2@ 34")).to eq(["1234"]) + expect(dummy.get_document_number_variants(2, " 1 2@ 34")).to eq(["1234"]) end it "returns only one try for passports & residence cards" do - expect(DocumentParser.get_document_number_variants(2, "1234")).to eq(["1234"]) - expect(DocumentParser.get_document_number_variants(3, "1234")).to eq(["1234"]) + expect(dummy.get_document_number_variants(2, "1234")).to eq(["1234"]) + expect(dummy.get_document_number_variants(3, "1234")).to eq(["1234"]) end it "takes only the last 8 digits for dnis and resicence cards" do - expect(DocumentParser.get_document_number_variants(1, "543212345678")).to eq(["12345678"]) + expect(dummy.get_document_number_variants(1, "543212345678")).to eq(["12345678"]) end it "tries all the dni variants padding with zeroes" do - expect(DocumentParser.get_document_number_variants(1, "0123456")) + expect(dummy.get_document_number_variants(1, "0123456")) .to eq(["123456", "0123456", "00123456"]) - expect(DocumentParser.get_document_number_variants(1, "00123456")) + expect(dummy.get_document_number_variants(1, "00123456")) .to eq(["123456", "0123456", "00123456"]) end it "adds upper and lowercase letter when the letter is present" do - expect(DocumentParser.get_document_number_variants(1, "1234567A")) + expect(dummy.get_document_number_variants(1, "1234567A")) .to eq(%w[1234567 01234567 1234567a 1234567A 01234567a 01234567A]) end end From cb477149c483e9a24d01aa2cf0f39b067674f998 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 17 Mar 2024 21:35:17 +0100 Subject: [PATCH 05/18] Move lib folder inside the app folder The purpose of the lib folder is to have code that doesn't necessary belong in the application but can be shared with other applications. However, we don't have other applications and, if we did, the way to share code between them would be using a gem or even a git submodule. So having both the `app/` and the `lib/` folders is confusing IMHO, and it causes unnecessary problems with autoloading. So we're moving the `lib/` folder to `app/lib/`. Originally, some of these files were in the `app/services/` folder and then they were moved to the `lib/` folder. We're using `app/lib/` instead of `app/services/` so the upgrade is less confusing. There's an exception, though. The `OmniAuth::Strategies::Wordpress` class needs to be available in the Devise initializer. Since this is an initializer and trying to autoload a class here will be problematic when switching to Zeitwerk, we'll keep the `require` clause on top of the Devise initializer in order to load the file and so it will be loaded even if it isn't in the autoload paths anymore. --- .rubocop.yml | 4 ++-- {lib => app/lib}/active_model/dates.rb | 0 .../lib}/active_storage/service/tenant_disk_service.rb | 0 {lib => app/lib}/acts_as_paranoid_aliases.rb | 0 {lib => app/lib}/admin_legislation_sanitizer.rb | 0 {lib => app/lib}/admin_wysiwyg_sanitizer.rb | 0 {lib => app/lib}/age.rb | 0 {lib => app/lib}/application_logger.rb | 0 {lib => app/lib}/application_responder.rb | 0 {lib => app/lib}/asset_finder.rb | 0 {lib => app/lib}/authentication_logger.rb | 0 {lib => app/lib}/census_api.rb | 0 {lib => app/lib}/census_caller.rb | 0 {lib => app/lib}/ckeditor/backend/active_storage.rb | 0 {lib => app/lib}/comment_tree.rb | 0 {lib => app/lib}/consul_form_builder.rb | 0 {lib => app/lib}/document_parser.rb | 0 {lib => app/lib}/email_digest.rb | 0 {lib => app/lib}/evaluation_comment_email.rb | 0 {lib => app/lib}/geozone_stats.rb | 0 {lib => app/lib}/local_census.rb | 0 {lib => app/lib}/manager_authenticator.rb | 0 {lib => app/lib}/markdown_converter.rb | 0 {lib => app/lib}/merged_comment_tree.rb | 0 {lib => app/lib}/omniauth_tenant_setup.rb | 0 {lib => app/lib}/percentage_calculator.rb | 0 {lib => app/lib}/remote_census_api.rb | 0 {lib => app/lib}/remote_translations/caller.rb | 0 .../lib}/remote_translations/microsoft/available_locales.rb | 0 {lib => app/lib}/remote_translations/microsoft/client.rb | 0 .../lib}/remote_translations/microsoft/sentences_parser.rb | 0 {lib => app/lib}/reply_email.rb | 0 {lib => app/lib}/score_calculator.rb | 0 {lib => app/lib}/search_dictionary_selector.rb | 0 {lib => app/lib}/sms_api.rb | 0 {lib => app/lib}/tag_sanitizer.rb | 0 {lib => app/lib}/user_segments.rb | 0 {lib => app/lib}/wysiwyg_sanitizer.rb | 0 config/application.rb | 2 -- config/environments/production.rb | 5 ----- 40 files changed, 2 insertions(+), 9 deletions(-) rename {lib => app/lib}/active_model/dates.rb (100%) rename {lib => app/lib}/active_storage/service/tenant_disk_service.rb (100%) rename {lib => app/lib}/acts_as_paranoid_aliases.rb (100%) rename {lib => app/lib}/admin_legislation_sanitizer.rb (100%) rename {lib => app/lib}/admin_wysiwyg_sanitizer.rb (100%) rename {lib => app/lib}/age.rb (100%) rename {lib => app/lib}/application_logger.rb (100%) rename {lib => app/lib}/application_responder.rb (100%) rename {lib => app/lib}/asset_finder.rb (100%) rename {lib => app/lib}/authentication_logger.rb (100%) rename {lib => app/lib}/census_api.rb (100%) rename {lib => app/lib}/census_caller.rb (100%) rename {lib => app/lib}/ckeditor/backend/active_storage.rb (100%) rename {lib => app/lib}/comment_tree.rb (100%) rename {lib => app/lib}/consul_form_builder.rb (100%) rename {lib => app/lib}/document_parser.rb (100%) rename {lib => app/lib}/email_digest.rb (100%) rename {lib => app/lib}/evaluation_comment_email.rb (100%) rename {lib => app/lib}/geozone_stats.rb (100%) rename {lib => app/lib}/local_census.rb (100%) rename {lib => app/lib}/manager_authenticator.rb (100%) rename {lib => app/lib}/markdown_converter.rb (100%) rename {lib => app/lib}/merged_comment_tree.rb (100%) rename {lib => app/lib}/omniauth_tenant_setup.rb (100%) rename {lib => app/lib}/percentage_calculator.rb (100%) rename {lib => app/lib}/remote_census_api.rb (100%) rename {lib => app/lib}/remote_translations/caller.rb (100%) rename {lib => app/lib}/remote_translations/microsoft/available_locales.rb (100%) rename {lib => app/lib}/remote_translations/microsoft/client.rb (100%) rename {lib => app/lib}/remote_translations/microsoft/sentences_parser.rb (100%) rename {lib => app/lib}/reply_email.rb (100%) rename {lib => app/lib}/score_calculator.rb (100%) rename {lib => app/lib}/search_dictionary_selector.rb (100%) rename {lib => app/lib}/sms_api.rb (100%) rename {lib => app/lib}/tag_sanitizer.rb (100%) rename {lib => app/lib}/user_segments.rb (100%) rename {lib => app/lib}/wysiwyg_sanitizer.rb (100%) diff --git a/.rubocop.yml b/.rubocop.yml index b81f69685..61a117f97 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -10,7 +10,7 @@ AllCops: DisplayStyleGuide: true Exclude: - "db/schema.rb" - - "lib/ckeditor/backend/active_storage.rb" + - "app/lib/ckeditor/backend/active_storage.rb" DisabledByDefault: true Bundler/DuplicatedGem: @@ -480,7 +480,7 @@ Rails/SkipsModelValidations: - update_attribute Exclude: - app/models/tenant.rb - - lib/acts_as_paranoid_aliases.rb + - app/lib/acts_as_paranoid_aliases.rb Rails/TimeZone: Enabled: true diff --git a/lib/active_model/dates.rb b/app/lib/active_model/dates.rb similarity index 100% rename from lib/active_model/dates.rb rename to app/lib/active_model/dates.rb diff --git a/lib/active_storage/service/tenant_disk_service.rb b/app/lib/active_storage/service/tenant_disk_service.rb similarity index 100% rename from lib/active_storage/service/tenant_disk_service.rb rename to app/lib/active_storage/service/tenant_disk_service.rb diff --git a/lib/acts_as_paranoid_aliases.rb b/app/lib/acts_as_paranoid_aliases.rb similarity index 100% rename from lib/acts_as_paranoid_aliases.rb rename to app/lib/acts_as_paranoid_aliases.rb diff --git a/lib/admin_legislation_sanitizer.rb b/app/lib/admin_legislation_sanitizer.rb similarity index 100% rename from lib/admin_legislation_sanitizer.rb rename to app/lib/admin_legislation_sanitizer.rb diff --git a/lib/admin_wysiwyg_sanitizer.rb b/app/lib/admin_wysiwyg_sanitizer.rb similarity index 100% rename from lib/admin_wysiwyg_sanitizer.rb rename to app/lib/admin_wysiwyg_sanitizer.rb diff --git a/lib/age.rb b/app/lib/age.rb similarity index 100% rename from lib/age.rb rename to app/lib/age.rb diff --git a/lib/application_logger.rb b/app/lib/application_logger.rb similarity index 100% rename from lib/application_logger.rb rename to app/lib/application_logger.rb diff --git a/lib/application_responder.rb b/app/lib/application_responder.rb similarity index 100% rename from lib/application_responder.rb rename to app/lib/application_responder.rb diff --git a/lib/asset_finder.rb b/app/lib/asset_finder.rb similarity index 100% rename from lib/asset_finder.rb rename to app/lib/asset_finder.rb diff --git a/lib/authentication_logger.rb b/app/lib/authentication_logger.rb similarity index 100% rename from lib/authentication_logger.rb rename to app/lib/authentication_logger.rb diff --git a/lib/census_api.rb b/app/lib/census_api.rb similarity index 100% rename from lib/census_api.rb rename to app/lib/census_api.rb diff --git a/lib/census_caller.rb b/app/lib/census_caller.rb similarity index 100% rename from lib/census_caller.rb rename to app/lib/census_caller.rb diff --git a/lib/ckeditor/backend/active_storage.rb b/app/lib/ckeditor/backend/active_storage.rb similarity index 100% rename from lib/ckeditor/backend/active_storage.rb rename to app/lib/ckeditor/backend/active_storage.rb diff --git a/lib/comment_tree.rb b/app/lib/comment_tree.rb similarity index 100% rename from lib/comment_tree.rb rename to app/lib/comment_tree.rb diff --git a/lib/consul_form_builder.rb b/app/lib/consul_form_builder.rb similarity index 100% rename from lib/consul_form_builder.rb rename to app/lib/consul_form_builder.rb diff --git a/lib/document_parser.rb b/app/lib/document_parser.rb similarity index 100% rename from lib/document_parser.rb rename to app/lib/document_parser.rb diff --git a/lib/email_digest.rb b/app/lib/email_digest.rb similarity index 100% rename from lib/email_digest.rb rename to app/lib/email_digest.rb diff --git a/lib/evaluation_comment_email.rb b/app/lib/evaluation_comment_email.rb similarity index 100% rename from lib/evaluation_comment_email.rb rename to app/lib/evaluation_comment_email.rb diff --git a/lib/geozone_stats.rb b/app/lib/geozone_stats.rb similarity index 100% rename from lib/geozone_stats.rb rename to app/lib/geozone_stats.rb diff --git a/lib/local_census.rb b/app/lib/local_census.rb similarity index 100% rename from lib/local_census.rb rename to app/lib/local_census.rb diff --git a/lib/manager_authenticator.rb b/app/lib/manager_authenticator.rb similarity index 100% rename from lib/manager_authenticator.rb rename to app/lib/manager_authenticator.rb diff --git a/lib/markdown_converter.rb b/app/lib/markdown_converter.rb similarity index 100% rename from lib/markdown_converter.rb rename to app/lib/markdown_converter.rb diff --git a/lib/merged_comment_tree.rb b/app/lib/merged_comment_tree.rb similarity index 100% rename from lib/merged_comment_tree.rb rename to app/lib/merged_comment_tree.rb diff --git a/lib/omniauth_tenant_setup.rb b/app/lib/omniauth_tenant_setup.rb similarity index 100% rename from lib/omniauth_tenant_setup.rb rename to app/lib/omniauth_tenant_setup.rb diff --git a/lib/percentage_calculator.rb b/app/lib/percentage_calculator.rb similarity index 100% rename from lib/percentage_calculator.rb rename to app/lib/percentage_calculator.rb diff --git a/lib/remote_census_api.rb b/app/lib/remote_census_api.rb similarity index 100% rename from lib/remote_census_api.rb rename to app/lib/remote_census_api.rb diff --git a/lib/remote_translations/caller.rb b/app/lib/remote_translations/caller.rb similarity index 100% rename from lib/remote_translations/caller.rb rename to app/lib/remote_translations/caller.rb diff --git a/lib/remote_translations/microsoft/available_locales.rb b/app/lib/remote_translations/microsoft/available_locales.rb similarity index 100% rename from lib/remote_translations/microsoft/available_locales.rb rename to app/lib/remote_translations/microsoft/available_locales.rb diff --git a/lib/remote_translations/microsoft/client.rb b/app/lib/remote_translations/microsoft/client.rb similarity index 100% rename from lib/remote_translations/microsoft/client.rb rename to app/lib/remote_translations/microsoft/client.rb diff --git a/lib/remote_translations/microsoft/sentences_parser.rb b/app/lib/remote_translations/microsoft/sentences_parser.rb similarity index 100% rename from lib/remote_translations/microsoft/sentences_parser.rb rename to app/lib/remote_translations/microsoft/sentences_parser.rb diff --git a/lib/reply_email.rb b/app/lib/reply_email.rb similarity index 100% rename from lib/reply_email.rb rename to app/lib/reply_email.rb diff --git a/lib/score_calculator.rb b/app/lib/score_calculator.rb similarity index 100% rename from lib/score_calculator.rb rename to app/lib/score_calculator.rb diff --git a/lib/search_dictionary_selector.rb b/app/lib/search_dictionary_selector.rb similarity index 100% rename from lib/search_dictionary_selector.rb rename to app/lib/search_dictionary_selector.rb diff --git a/lib/sms_api.rb b/app/lib/sms_api.rb similarity index 100% rename from lib/sms_api.rb rename to app/lib/sms_api.rb diff --git a/lib/tag_sanitizer.rb b/app/lib/tag_sanitizer.rb similarity index 100% rename from lib/tag_sanitizer.rb rename to app/lib/tag_sanitizer.rb diff --git a/lib/user_segments.rb b/app/lib/user_segments.rb similarity index 100% rename from lib/user_segments.rb rename to app/lib/user_segments.rb diff --git a/lib/wysiwyg_sanitizer.rb b/app/lib/wysiwyg_sanitizer.rb similarity index 100% rename from lib/wysiwyg_sanitizer.rb rename to app/lib/wysiwyg_sanitizer.rb diff --git a/config/application.rb b/config/application.rb index a2cf9bfde..6b6d6c51d 100644 --- a/config/application.rb +++ b/config/application.rb @@ -125,8 +125,6 @@ module Consul config.assets.paths << Rails.root.join("node_modules", "jquery-ui", "themes", "base") config.assets.paths << Rails.root.join("node_modules") - # Add lib to the autoload path - config.autoload_paths << Rails.root.join("lib") config.active_job.queue_adapter = :delayed_job # CONSUL DEMOCRACY specific custom overrides diff --git a/config/environments/production.rb b/config/environments/production.rb index 1b200d1ab..57ba25c25 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -12,11 +12,6 @@ Rails.application.configure do # Rake tasks automatically ignore this option for performance. config.eager_load = true - # Because autoloading is disabled in production environments with Rails 5, - # using autoload_paths will not load needed classes from specified paths. - # The solution to this, is to ask Rails to eager load classes. - config.eager_load_paths += ["#{config.root}/lib"] - # Full error reports are disabled and caching is turned on. config.consider_all_requests_local = false config.action_controller.perform_caching = true From 7d02f0933d421bbc9fd3fdb40a49e21a411323c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 17 Mar 2024 21:30:22 +0100 Subject: [PATCH 06/18] Simplify code to autoload paths This we'll simplify adding these same paths to `eager_load_paths` when switching to Zeitwerk. --- config/application.rb | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/config/application.rb b/config/application.rb index 6b6d6c51d..d8e243a51 100644 --- a/config/application.rb +++ b/config/application.rb @@ -132,12 +132,17 @@ module Consul # * English: https://github.com/consuldemocracy/consuldemocracy/blob/master/CUSTOMIZE_EN.md # * Spanish: https://github.com/consuldemocracy/consuldemocracy/blob/master/CUSTOMIZE_ES.md # - config.autoload_paths << "#{Rails.root}/app/components/custom" - config.autoload_paths << "#{Rails.root}/app/controllers/custom" - config.autoload_paths << "#{Rails.root}/app/graphql/custom" - config.autoload_paths << "#{Rails.root}/app/mailers/custom" - config.autoload_paths << "#{Rails.root}/app/models/custom" - config.autoload_paths << "#{Rails.root}/app/models/custom/concerns" + + [ + "app/components/custom", + "app/controllers/custom", + "app/graphql/custom", + "app/mailers/custom", + "app/models/custom", + "app/models/custom/concerns" + ].each do |path| + config.autoload_paths << Rails.root.join(path) + end config.paths["app/views"].unshift(Rails.root.join("app", "views", "custom")) From e1d9e6f30fa24b14afc28d3169496469a9ac5fdb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 17 Mar 2024 21:57:38 +0100 Subject: [PATCH 07/18] Make it easier to customize files in app/lib Just like we do with pretty much every folder. --- app/lib/custom/.keep | 0 config/application.rb | 1 + 2 files changed, 1 insertion(+) create mode 100644 app/lib/custom/.keep diff --git a/app/lib/custom/.keep b/app/lib/custom/.keep new file mode 100644 index 000000000..e69de29bb diff --git a/config/application.rb b/config/application.rb index d8e243a51..6a80187b4 100644 --- a/config/application.rb +++ b/config/application.rb @@ -137,6 +137,7 @@ module Consul "app/components/custom", "app/controllers/custom", "app/graphql/custom", + "app/lib/custom", "app/mailers/custom", "app/models/custom", "app/models/custom/concerns" From 1a0f4ec65f5c873b45557cf38dd334e0700ff91e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 25 Jul 2023 05:01:30 +0200 Subject: [PATCH 08/18] Follow Zeitwerk conventions in names with acronyms We were getting a few errors when trying out Zeitwerk: ``` expected file lib/sms_api.rb to define constant SmsApi expected file app/components/layout/common_html_attributes_component.rb to define constant Layout::CommonHtmlAttributesComponent ``` In these cases, we aren't using an inflection because we also define the `Verification::SmsController` and a few migrations containing `Html` in their class name, and none of them would work if we defined the inflection. We were also getting an error regarding classes containing WYSIWYG in its name: ``` NameError: uninitialized constant WYSIWYGSanitizer Did you mean? WysiwygSanitizer ``` In this case, adding the acronym is easier, since we never use "Wysiwyg" in the code but we use "WYSIWYG" in many places. --- app/components/layout/common_html_attributes_component.rb | 2 +- app/helpers/layouts_helper.rb | 2 +- app/lib/sms_api.rb | 2 +- app/models/verification/sms.rb | 2 +- config/initializers/inflections.rb | 1 + .../layout/common_html_attributes_component_spec.rb | 4 ++-- 6 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/components/layout/common_html_attributes_component.rb b/app/components/layout/common_html_attributes_component.rb index 0d4d8b741..63f35e0f6 100644 --- a/app/components/layout/common_html_attributes_component.rb +++ b/app/components/layout/common_html_attributes_component.rb @@ -1,4 +1,4 @@ -class Layout::CommonHTMLAttributesComponent < ApplicationComponent +class Layout::CommonHtmlAttributesComponent < ApplicationComponent use_helpers :rtl? private diff --git a/app/helpers/layouts_helper.rb b/app/helpers/layouts_helper.rb index 32ddde240..c602ba983 100644 --- a/app/helpers/layouts_helper.rb +++ b/app/helpers/layouts_helper.rb @@ -11,6 +11,6 @@ module LayoutsHelper end def common_html_attributes - render Layout::CommonHTMLAttributesComponent.new + render Layout::CommonHtmlAttributesComponent.new end end diff --git a/app/lib/sms_api.rb b/app/lib/sms_api.rb index d6373e934..db61fffe3 100644 --- a/app/lib/sms_api.rb +++ b/app/lib/sms_api.rb @@ -1,5 +1,5 @@ require "open-uri" -class SMSApi +class SmsApi attr_accessor :client def initialize diff --git a/app/models/verification/sms.rb b/app/models/verification/sms.rb index 4e8e07f71..ad23dda00 100644 --- a/app/models/verification/sms.rb +++ b/app/models/verification/sms.rb @@ -24,7 +24,7 @@ class Verification::Sms end def send_sms - SMSApi.new.sms_deliver(user.unconfirmed_phone, user.sms_confirmation_code) + SmsApi.new.sms_deliver(user.unconfirmed_phone, user.sms_confirmation_code) end def verified? diff --git a/config/initializers/inflections.rb b/config/initializers/inflections.rb index e95cdbca7..17b2717d6 100644 --- a/config/initializers/inflections.rb +++ b/config/initializers/inflections.rb @@ -18,5 +18,6 @@ ActiveSupport::Inflector.inflections(:en) do |inflect| inflect.plural(/^(\d+)$/i, '\1') inflect.acronym "SDG" + inflect.acronym "WYSIWYG" inflect.irregular "organización", "organizaciones" end diff --git a/spec/components/layout/common_html_attributes_component_spec.rb b/spec/components/layout/common_html_attributes_component_spec.rb index ded22103c..ecdc6187b 100644 --- a/spec/components/layout/common_html_attributes_component_spec.rb +++ b/spec/components/layout/common_html_attributes_component_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" -describe Layout::CommonHTMLAttributesComponent do - let(:component) { Layout::CommonHTMLAttributesComponent.new } +describe Layout::CommonHtmlAttributesComponent do + let(:component) { Layout::CommonHtmlAttributesComponent.new } context "with multitenancy disabled" do before { allow(Rails.application.config).to receive(:multitenancy).and_return(false) } From d8faa4f4d0ae2473260fdb58befef23b00d31e18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 25 Jul 2023 04:59:30 +0200 Subject: [PATCH 09/18] Follow Zeitwerk conventions for file structure Even though we don't load this file with Zeitwerk, we're doing it for consistency. If we tried to load this file using Zeitwerk, without this change we'd get an error: ``` NameError: uninitialized constant OmniauthWordpress ``` --- config/initializers/devise.rb | 2 +- .../strategies/wordpress.rb} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename lib/{omniauth_wordpress.rb => omni_auth/strategies/wordpress.rb} (100%) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 23d2d0a6a..33b1e77cd 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -1,4 +1,4 @@ -require Rails.root.join("lib", "omniauth_wordpress") +require Rails.root.join("lib", "omni_auth", "strategies", "wordpress") # Use this hook to configure devise mailer, warden hooks and so forth. # Many of these configuration options can be set straight in your model. diff --git a/lib/omniauth_wordpress.rb b/lib/omni_auth/strategies/wordpress.rb similarity index 100% rename from lib/omniauth_wordpress.rb rename to lib/omni_auth/strategies/wordpress.rb From 1cf529b134340aaaea890c38af94ba73cc69ca84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 29 Mar 2024 00:59:13 +0100 Subject: [PATCH 10/18] Make Devise find the strategy class automatically Since we're already setting `wordpress_oauth2` using the `option :name` command in the `OmniAuth::Strategies::Wordpress` class, Devise can automatically find the strategy. However, it wasn't working because we were passing a string instead of a symbol. --- config/initializers/devise.rb | 1 - lib/omni_auth/strategies/wordpress.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 33b1e77cd..778c5035e 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -284,7 +284,6 @@ Devise.setup do |config| config.omniauth :wordpress_oauth2, Rails.application.secrets.wordpress_oauth2_key, Rails.application.secrets.wordpress_oauth2_secret, - strategy_class: OmniAuth::Strategies::Wordpress, client_options: { site: Rails.application.secrets.wordpress_oauth2_site }, setup: OmniauthTenantSetup.wordpress_oauth2 diff --git a/lib/omni_auth/strategies/wordpress.rb b/lib/omni_auth/strategies/wordpress.rb index dc0ec1f6c..acc1bfa7e 100644 --- a/lib/omni_auth/strategies/wordpress.rb +++ b/lib/omni_auth/strategies/wordpress.rb @@ -5,7 +5,7 @@ require "omniauth-oauth2" module OmniAuth module Strategies class Wordpress < OmniAuth::Strategies::OAuth2 - option :name, "wordpress_oauth2" + option :name, :wordpress_oauth2 option :client_options, {} From 1af5c18bd71ac11444df1bf39e58f03f369f495c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 29 Mar 2024 00:49:18 +0100 Subject: [PATCH 11/18] Load OmniauthTenantSetup inside a lambda This way we avoid loading the OmniauthTenantSetup in the initializer, which could be problematic when switching to Zeitwerk. --- app/lib/omniauth_tenant_setup.rb | 24 ++++++++---------------- config/initializers/devise.rb | 8 ++++---- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/app/lib/omniauth_tenant_setup.rb b/app/lib/omniauth_tenant_setup.rb index 57d010947..1aef1cad4 100644 --- a/app/lib/omniauth_tenant_setup.rb +++ b/app/lib/omniauth_tenant_setup.rb @@ -1,27 +1,19 @@ module OmniauthTenantSetup class << self - def twitter - ->(env) do - oauth(env, secrets.twitter_key, secrets.twitter_secret) - end + def twitter(env) + oauth(env, secrets.twitter_key, secrets.twitter_secret) end - def facebook - ->(env) do - oauth2(env, secrets.facebook_key, secrets.facebook_secret) - end + def facebook(env) + oauth2(env, secrets.facebook_key, secrets.facebook_secret) end - def google_oauth2 - ->(env) do - oauth2(env, secrets.google_oauth2_key, secrets.google_oauth2_secret) - end + def google_oauth2(env) + oauth2(env, secrets.google_oauth2_key, secrets.google_oauth2_secret) end - def wordpress_oauth2 - ->(env) do - oauth2(env, secrets.wordpress_oauth2_key, secrets.wordpress_oauth2_secret) - end + def wordpress_oauth2(env) + oauth2(env, secrets.wordpress_oauth2_key, secrets.wordpress_oauth2_secret) end private diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 778c5035e..9baa69960 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -270,22 +270,22 @@ Devise.setup do |config| config.omniauth :twitter, Rails.application.secrets.twitter_key, Rails.application.secrets.twitter_secret, - setup: OmniauthTenantSetup.twitter + setup: ->(env) { OmniauthTenantSetup.twitter(env) } config.omniauth :facebook, Rails.application.secrets.facebook_key, Rails.application.secrets.facebook_secret, scope: "email", info_fields: "email,name,verified", - setup: OmniauthTenantSetup.facebook + setup: ->(env) { OmniauthTenantSetup.facebook(env) } config.omniauth :google_oauth2, Rails.application.secrets.google_oauth2_key, Rails.application.secrets.google_oauth2_secret, - setup: OmniauthTenantSetup.google_oauth2 + setup: ->(env) { OmniauthTenantSetup.google_oauth2(env) } config.omniauth :wordpress_oauth2, Rails.application.secrets.wordpress_oauth2_key, Rails.application.secrets.wordpress_oauth2_secret, client_options: { site: Rails.application.secrets.wordpress_oauth2_site }, - setup: OmniauthTenantSetup.wordpress_oauth2 + setup: ->(env) { OmniauthTenantSetup.wordpress_oauth2(env) } # ==> Warden configuration # If you want to use other strategies, that are not supported by Devise, or From 919755f328695b320c4aef4a20dbcaa0a8f7de9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 25 Jul 2023 05:06:13 +0200 Subject: [PATCH 12/18] Use the whole module name for SentencesParser In order for `include SentencesParser` to work with Zeitwerk, we'd have to change the code slightly, so it follows Ruby conventions to resolve constants: ``` module RemoteTranslations::Microsoft class Client include SentencesParser # (...) end end ``` This would mean changing the indentation of the whole file. While we can do that, changing the indentation of a file makes it harder to use commands like `git blame` or `git log` with the file, so we're doing the change the easy way. --- app/lib/remote_translations/microsoft/client.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/lib/remote_translations/microsoft/client.rb b/app/lib/remote_translations/microsoft/client.rb index ad17adccd..81ccff153 100644 --- a/app/lib/remote_translations/microsoft/client.rb +++ b/app/lib/remote_translations/microsoft/client.rb @@ -1,5 +1,5 @@ class RemoteTranslations::Microsoft::Client - include SentencesParser + include RemoteTranslations::Microsoft::SentencesParser CHARACTERS_LIMIT_PER_REQUEST = 5000 PREVENTING_TRANSLATION_KEY = "notranslate".freeze From 004684efe6e76e72e399605e53730fb34e121715 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 29 Mar 2024 00:44:17 +0100 Subject: [PATCH 13/18] Use a string to define the class used by Audited This is possible since Audited 5.2.0, and will make it easier to start using Zeitwerk because it won't reference a constant in an initializer. --- config/initializers/audited.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/audited.rb b/config/initializers/audited.rb index 14de509e8..13e1e0793 100644 --- a/config/initializers/audited.rb +++ b/config/initializers/audited.rb @@ -1,4 +1,4 @@ Audited.config do |config| - config.audit_class = ::Audit + config.audit_class = "::Audit" config.ignored_default_callbacks = [:touch] end From 5f24ee9121177964668463bec527a52ea7f89a61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 25 Jul 2023 04:46:53 +0200 Subject: [PATCH 14/18] Use Zeitwerk instead of the classic autoloader In Rails 6.1, the classic autoloader is deprecated. We were getting an error because we were using `autoload` in the ActiveStorage plugin for CKEditor: expected file app/lib/ckeditor/backend/active_storage.rb to define constant Ckeditor::Backend::ActiveStorage So we're removing the line causing the error. Finally, we can now restore all the tests that that failed sometimes with the classic autoloader and that we modified in commits 2af1fc72f and 8ba37b295. --- app/lib/ckeditor/backend/active_storage.rb | 2 -- config/application.rb | 5 +--- config/environments/development.rb | 1 + spec/system/admin/homepage/homepage_spec.rb | 30 ++++++++++----------- spec/system/admin/widgets/cards_spec.rb | 10 +++---- spec/system/polls/polls_spec.rb | 27 +++++++++---------- 6 files changed, 33 insertions(+), 42 deletions(-) diff --git a/app/lib/ckeditor/backend/active_storage.rb b/app/lib/ckeditor/backend/active_storage.rb index 8ff442443..1f990e041 100644 --- a/app/lib/ckeditor/backend/active_storage.rb +++ b/app/lib/ckeditor/backend/active_storage.rb @@ -67,7 +67,5 @@ module Ckeditor end end end - - autoload :ActiveStorage, "ckeditor/backend/active_storage" end end diff --git a/config/application.rb b/config/application.rb index 6a80187b4..a4cc6006b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -32,10 +32,6 @@ module Consul # in any CONSUL DEMOCRACY installations config.active_support.use_authenticated_message_encryption = false - # Keep using the classic autoloader until we decide how custom classes - # should work with zeitwerk - config.autoloader = :classic - # Don't enable has_many_inversing because it doesn't seem to currently # work with the _count database columns we use for caching purposes config.active_record.has_many_inversing = false @@ -143,6 +139,7 @@ module Consul "app/models/custom/concerns" ].each do |path| config.autoload_paths << Rails.root.join(path) + config.eager_load_paths << Rails.root.join(path) end config.paths["app/views"].unshift(Rails.root.join("app", "views", "custom")) diff --git a/config/environments/development.rb b/config/environments/development.rb index 704a1e63e..df04c5583 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -72,6 +72,7 @@ Rails.application.configure do # Annotate rendered view with file names. # config.action_view.annotate_rendered_view_with_filenames = true + config.eager_load_paths << "#{Rails.root}/spec/mailers/previews" config.action_mailer.preview_path = "#{Rails.root}/spec/mailers/previews" # Limit size of local logs diff --git a/spec/system/admin/homepage/homepage_spec.rb b/spec/system/admin/homepage/homepage_spec.rb index f0f1bec38..274c537bf 100644 --- a/spec/system/admin/homepage/homepage_spec.rb +++ b/spec/system/admin/homepage/homepage_spec.rb @@ -178,16 +178,15 @@ describe "Homepage", :admin do link_text: "Link1 text", link_url: "consul1.dev") - # TODO: uncomment again after switching to zeitwerk - # card2 = create(:widget_card, label: "Card2 label", - # title: "Card2 text", - # description: "Card2 description", - # link_text: "Link2 text", - # link_url: "consul2.dev") + card2 = create(:widget_card, label: "Card2 label", + title: "Card2 text", + description: "Card2 description", + link_text: "Link2 text", + link_url: "consul2.dev") visit root_path - expect(page).to have_css(".card", count: 1) # TODO: change to `count: 2 after switching to zeitwerk + expect(page).to have_css(".card", count: 2) within("#widget_card_#{card1.id}") do expect(page).to have_content("CARD1 LABEL") @@ -198,15 +197,14 @@ describe "Homepage", :admin do expect(page).to have_css("img[alt='#{card1.image.title}']") end - # TODO: uncomment again after switching to zeitwerk - # within("#widget_card_#{card2.id}") do - # expect(page).to have_content("CARD2 LABEL") - # expect(page).to have_content("CARD2 TEXT") - # expect(page).to have_content("Card2 description") - # expect(page).to have_content("Link2 text") - # expect(page).to have_link(href: "consul2.dev") - # expect(page).to have_css("img[alt='#{card2.image.title}']") - # end + within("#widget_card_#{card2.id}") do + expect(page).to have_content("CARD2 LABEL") + expect(page).to have_content("CARD2 TEXT") + expect(page).to have_content("Card2 description") + expect(page).to have_content("Link2 text") + expect(page).to have_link(href: "consul2.dev") + expect(page).to have_css("img[alt='#{card2.image.title}']") + end end scenario "Recomendations" do diff --git a/spec/system/admin/widgets/cards_spec.rb b/spec/system/admin/widgets/cards_spec.rb index 4f5043ae7..d753f1489 100644 --- a/spec/system/admin/widgets/cards_spec.rb +++ b/spec/system/admin/widgets/cards_spec.rb @@ -59,16 +59,14 @@ describe "Cards", :admin do scenario "Show" do card_1 = create(:widget_card, title: "Card homepage large", columns: 8) - # TODO: uncomment after switching to zeitwerk - # card_2 = create(:widget_card, title: "Card homepage medium", columns: 4) - # card_3 = create(:widget_card, title: "Card homepage small", columns: 2) + card_2 = create(:widget_card, title: "Card homepage medium", columns: 4) + card_3 = create(:widget_card, title: "Card homepage small", columns: 2) visit root_path expect(page).to have_css("#widget_card_#{card_1.id}.medium-8") - # TODO: uncomment after switching to zeitwerk - # expect(page).to have_css("#widget_card_#{card_2.id}.medium-4") - # expect(page).to have_css("#widget_card_#{card_3.id}.medium-2") + expect(page).to have_css("#widget_card_#{card_2.id}.medium-4") + expect(page).to have_css("#widget_card_#{card_3.id}.medium-2") end scenario "Edit" do diff --git a/spec/system/polls/polls_spec.rb b/spec/system/polls/polls_spec.rb index bf00eb20a..19dad5252 100644 --- a/spec/system/polls/polls_spec.rb +++ b/spec/system/polls/polls_spec.rb @@ -26,7 +26,7 @@ describe "Polls" do end scenario "Polls can be listed" do - polls = [create(:poll, :with_image)] # TODO: generate a list again after switching to zeitwerk + polls = create_list(:poll, 3, :with_image) visit polls_path @@ -210,24 +210,23 @@ describe "Polls" do expect("Second question").to appear_before("Third question") end - # TODO: uncomment after switching to zeitwerk - # scenario "Buttons to slide through images work back and forth" do - # question = create(:poll_question, :yes_no, poll: poll) - # create(:image, imageable: question.question_answers.last, title: "The no movement") - # create(:image, imageable: question.question_answers.last, title: "No movement planning") + scenario "Buttons to slide through images work back and forth" do + question = create(:poll_question, :yes_no, poll: poll) + create(:image, imageable: question.question_answers.last, title: "The no movement") + create(:image, imageable: question.question_answers.last, title: "No movement planning") - # visit poll_path(poll) + visit poll_path(poll) - # within(".orbit-bullets") do - # find("[data-slide='1']").click + within(".orbit-bullets") do + find("[data-slide='1']").click - # expect(page).to have_css ".is-active[data-slide='1']" + expect(page).to have_css ".is-active[data-slide='1']" - # find("[data-slide='0']").click + find("[data-slide='0']").click - # expect(page).to have_css ".is-active[data-slide='0']" - # end - # end + expect(page).to have_css ".is-active[data-slide='0']" + end + end scenario "Non-logged in users" do create(:poll_question, :yes_no, poll: poll) From 6552e3197d3c07df3dbaa9cc754b139f6a9a6dd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 30 Mar 2024 20:09:16 +0100 Subject: [PATCH 15/18] Use load instead of require_dependency in custom files While using `require_dependency` to load original Consul Democracy code from custom code works with the classic autoloader, this method was never meant to be used this way. With zeitwerk, the code (apparently) works in the test, development and production environments, but there's one important gotcha: changing any `.rb` file in development will require restarting the rails server since the application will crash when reloading. Quoting zeitwerk's author Xavier Noria, whom we've contacted while looking for a solution: > With the classic autoloader, when the Setting constant is autoloaded, > the autoloader searched the autoload paths, found setting.rb in > app/models/custom, and loaded it. With zeitwerk, the autoloader scans > the folders in order and defines an autoload (Module#autoload) in > Object so Ruby autoloads Setting with app/models/custom/settings.rb. > Later, when app/models/setting.rb is found, it's ignored since there's > already an autoload for Setting. > > That means the first file is managed by the autoloaders, while the > second is not. > > So require_dependency worked, but it was pure luck, since the purpose > of require_dependency is forcing the load of files managed by the > autoloaders and, as we've seen, app/models/settings.rb isn't one of > them. > > With your current pattern for custom files, the best solution is using > Kernel#load. So we're using `load` instead of `require_dependency`. Note that, with `load`, we need to add the `.rb` extension to the required file, and we no longer have to convert the Pathname to a string with `to_s`. --- app/models/custom/setting.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/custom/setting.rb b/app/models/custom/setting.rb index d6c6dff6a..d4fa3a51f 100644 --- a/app/models/custom/setting.rb +++ b/app/models/custom/setting.rb @@ -1,4 +1,4 @@ -require_dependency Rails.root.join("app", "models", "setting").to_s +load Rails.root.join("app", "models", "setting.rb") class Setting class << self From 59f84deca16f8ef53c4b4fef6baba999d45bc193 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Tue, 25 Jul 2023 05:27:57 +0200 Subject: [PATCH 16/18] Eager load the test environment like in Rails 7 Quoting from pull request 43508 in the Rails repository [1]: > When you are running test locally, most of the time you run only a > subset, so it's better to load as little code as possible to have a > faster time to first test result. > > But when you are on CI, it's usually much preferable to eager load the > whole application because you will likely need all the code anyway, > and even if the test suite is split across runners, it's preferable to > load all the code to ensure any codefile that may have side effects is > loaded. > > This also ensure that if some autoloaded constants are not properly > tested on CI, at least they'll be loaded and obvious errors (e.g. > SyntaxError) will be caught on CI rather than during deploy. [1] https://github.com/rails/rails/commit/db0ee287eed --- config/environments/test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/environments/test.rb b/config/environments/test.rb index 530286e6c..369978ed2 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -20,7 +20,7 @@ Rails.application.configure do # Do not eager load code on boot. This avoids loading your whole application # just for the purpose of running a single test. If you are using a tool that # preloads Rails for running tests, you may have to set it to true. - config.eager_load = false + config.eager_load = ENV["CI"].present? # Configure public file server for tests with Cache-Control for performance. config.public_file_server.enabled = true From e8195c201db5a5d3f8d5febffa569aee846cb52f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 29 Mar 2024 00:42:48 +0100 Subject: [PATCH 17/18] Avoid warnings during initialization These warnings appear in the logs in the development environment, and, with Rails 7, the application will crash. When running the tests, they would appear in the standard error ouput if we set `config.cache_classes = false` in the test environment but, since that isn't the case, they don't. To reproduce these warnings (or the lack of them), start a Rails console in development and check the log/development.log file. --- config/initializers/acts_as_taggable_on.rb | 8 ++++++-- .../disable_active_storage_uploads.rb | 16 +++++++++------- config/initializers/globalize.rb | 10 ++++++---- config/initializers/vote_extensions.rb | 8 ++++++-- 4 files changed, 27 insertions(+), 15 deletions(-) diff --git a/config/initializers/acts_as_taggable_on.rb b/config/initializers/acts_as_taggable_on.rb index 7dea4aafc..af1045a80 100644 --- a/config/initializers/acts_as_taggable_on.rb +++ b/config/initializers/acts_as_taggable_on.rb @@ -7,6 +7,12 @@ ActsAsTaggableOn.setup do |config| # config.base_class = "ApplicationRecord" end +Rails.application.reloader.to_prepare do + ActsAsTaggableOn::Tag.class_eval do + include Graphqlable + end +end + module ActsAsTaggableOn Tagging.class_eval do after_create :increment_tag_custom_counter @@ -39,8 +45,6 @@ module ActsAsTaggableOn kind == "category" end - include Graphqlable - scope :public_for_api, -> do where( kind: [nil, "category"], diff --git a/config/initializers/disable_active_storage_uploads.rb b/config/initializers/disable_active_storage_uploads.rb index fa5a52a91..f7bf09fb0 100644 --- a/config/initializers/disable_active_storage_uploads.rb +++ b/config/initializers/disable_active_storage_uploads.rb @@ -1,11 +1,13 @@ -ActiveStorage::DirectUploadsController.class_eval do - def create - head :unauthorized +Rails.application.reloader.to_prepare do + ActiveStorage::DirectUploadsController.class_eval do + def create + head :unauthorized + end end -end -ActiveStorage::DiskController.class_eval do - def update - head :unauthorized + ActiveStorage::DiskController.class_eval do + def update + head :unauthorized + end end end diff --git a/config/initializers/globalize.rb b/config/initializers/globalize.rb index 0a63e4d71..9f098a9b0 100644 --- a/config/initializers/globalize.rb +++ b/config/initializers/globalize.rb @@ -1,3 +1,9 @@ +Rails.application.reloader.to_prepare do + Globalize::ActiveRecord::Translation.class_eval do + include SkipValidation + end +end + module Globalize module ActiveRecord module InstanceMethods @@ -9,10 +15,6 @@ module Globalize end end end - - class Translation - include SkipValidation - end end end diff --git a/config/initializers/vote_extensions.rb b/config/initializers/vote_extensions.rb index c70392250..080610432 100644 --- a/config/initializers/vote_extensions.rb +++ b/config/initializers/vote_extensions.rb @@ -1,6 +1,10 @@ -ActsAsVotable::Vote.class_eval do - include Graphqlable +Rails.application.reloader.to_prepare do + ActsAsVotable::Vote.class_eval do + include Graphqlable + end +end +ActsAsVotable::Vote.class_eval do belongs_to :signature belongs_to :budget_investment, foreign_key: "votable_id", class_name: "Budget::Investment" From dbacd7fbfa03e42520e1dd795a0cd0701e68f6db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 11 Apr 2024 18:39:49 +0200 Subject: [PATCH 18/18] Replace byebug with the debug gem included in Ruby Byebug hasn't been maintained for years, and it isn't fully compatible with Zeitwerk [1]. On the other hand, Ruby includes the debug gem since version 3.1.0. We tried to start using at after commit e74eff217, but couldn't do so because our CI was hanging forever in a test related to machine learning, with the message: > DEBUGGER: Attaching after process X fork to child process Y (Note this message appeared with debug 1.6.3 but not with the version we're currently using.) So we're changing the debug gem fork mode in the test so it doesn't hang anymore when running our CI. We tried to change the test so it wouldn't call `Process.fork`, but this required changing the code, and since there are no tests checking machine learning behavior with real scripts, we aren't sure whether these script would keep working after changing the code. [1] Issue 564 in https://github.com/deivid-rodriguez/byebug --- Gemfile | 2 +- Gemfile.lock | 17 +++++++++++++++-- spec/models/machine_learning_spec.rb | 4 ++++ 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index fdd7d9c47..7514d7801 100644 --- a/Gemfile +++ b/Gemfile @@ -64,7 +64,7 @@ gem "wkhtmltopdf-binary", "~> 0.12.6" group :development, :test do gem "bullet", "~> 7.1.6" - gem "byebug", "~> 11.1.3" + gem "debug", "~> 1.9.2" gem "factory_bot_rails", "~> 6.4.3" gem "faker", "~> 3.2.3" gem "i18n-tasks", "~> 0.9.37" diff --git a/Gemfile.lock b/Gemfile.lock index 537e4b816..538c88148 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -106,7 +106,6 @@ GEM bullet (7.1.6) activesupport (>= 3.0.0) uniform_notifier (~> 1.11) - byebug (11.1.3) cancancan (3.5.0) capistrano (3.18.1) airbrussh (>= 1.0.0) @@ -171,6 +170,9 @@ GEM daemons (1.4.1) dalli (3.2.8) date (3.3.4) + debug (1.9.2) + irb (~> 1.10) + reline (>= 0.3.8) delayed_job (4.1.11) activesupport (>= 3.0, < 8.0) delayed_job_active_record (4.1.8) @@ -282,6 +284,10 @@ GEM ruby-vips (>= 2.0.17, < 3) invisible_captcha (2.3.0) rails (>= 5.2) + io-console (0.7.2) + irb (1.12.0) + rdoc + reline (>= 0.4.2) json (2.7.1) jwt (2.7.1) kaminari (1.2.2) @@ -438,6 +444,8 @@ GEM pronto-stylelint (0.10.3) pronto (>= 0.10, < 0.12) rugged (>= 0.24, < 2.0) + psych (5.1.2) + stringio public_suffix (4.0.7) puma (5.6.8) nio4r (~> 2.0) @@ -486,10 +494,14 @@ GEM rainbow (3.1.1) rake (13.1.0) rbtree3 (0.7.1) + rdoc (6.6.3.1) + psych (>= 4.0.0) recipient_interceptor (0.3.1) mail redcarpet (3.6.0) regexp_parser (2.9.0) + reline (0.5.1) + io-console (~> 0.5) request_store (1.6.0) rack (>= 1.4) responders (3.1.1) @@ -615,6 +627,7 @@ GEM net-scp (>= 1.1.2) net-sftp (>= 2.1.2) net-ssh (>= 2.8.0) + stringio (3.1.0) terminal-table (3.0.2) unicode-display_width (>= 1.1.1, < 3) terrapin (0.6.0) @@ -683,7 +696,6 @@ DEPENDENCIES autoprefixer-rails (~> 10.4.16) bing_translator (~> 6.2.0) bullet (~> 7.1.6) - byebug (~> 11.1.3) cancancan (~> 3.5.0) capistrano (~> 3.18.1) capistrano-bundler (~> 2.1.0) @@ -699,6 +711,7 @@ DEPENDENCIES cocoon (~> 1.2.15) daemons (~> 1.4.1) dalli (~> 3.2.8) + debug (~> 1.9.2) delayed_job_active_record (~> 4.1.8) devise (~> 4.9.3) devise-security (~> 0.18.0) diff --git a/spec/models/machine_learning_spec.rb b/spec/models/machine_learning_spec.rb index 1a0dd1735..a004b3fc9 100644 --- a/spec/models/machine_learning_spec.rb +++ b/spec/models/machine_learning_spec.rb @@ -378,6 +378,10 @@ describe MachineLearning do end describe "#run_machine_learning_scripts" do + let!(:original_fork_mode) { DEBUGGER__::CONFIG[:fork_mode] } + before { DEBUGGER__::CONFIG[:fork_mode] = "parent" } + after { DEBUGGER__::CONFIG[:fork_mode] = original_fork_mode } + it "returns true if python script executed correctly" do machine_learning = MachineLearning.new(job)