Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

AlecK/APPEALS-44509 - (6.1 Base) #1671

Open
wants to merge 14 commits into
base: AlecK/APPEALS-46558
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ gem "aws-sdk-sqs"
gem "aws-sdk-ec2"
gem "bgs", git: "https://github.com/department-of-veterans-affairs/ruby-bgs.git", ref: "a2e055b5a52bd1e2bb8c2b3b8d5820b1a404cd3d"
gem "bootsnap", require: false
gem "caseflow", git: "https://github.com/department-of-veterans-affairs/caseflow-commons", ref: "9bd3635fbd8094d25160669f38d8699e2f1d7a98"
gem "caseflow", git: "https://github.com/department-of-veterans-affairs/caseflow-commons", branch: "AlecK/Zeitwerk" #ref: "9bd3635fbd8094d25160669f38d8699e2f1d7a98"
gem "coffee-rails", "> 4.1.0"
gem "connect_vbms", git: "https://github.com/department-of-veterans-affairs/connect_vbms.git", branch: "master"
gem "connect_vva", git: "https://github.com/department-of-veterans-affairs/connect_vva.git", ref: "dfd1aeb2605c1f237f520bcdc41b059202e8944d"
Expand Down
4 changes: 2 additions & 2 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ GIT

GIT
remote: https://github.com/department-of-veterans-affairs/caseflow-commons
revision: 9bd3635fbd8094d25160669f38d8699e2f1d7a98
ref: 9bd3635fbd8094d25160669f38d8699e2f1d7a98
revision: 298c0a10c67920ad98251c0a9f3bd37abf408c35
branch: AlecK/Zeitwerk
specs:
caseflow (0.4.8)
aws-sdk-s3
Expand Down
2 changes: 1 addition & 1 deletion app/exceptions/bgs_errors.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
module BGS
module BGSErrors
class InvalidUsername < StandardError; end
class InvalidStation < StandardError; end
class InvalidApplication < StandardError; end
Expand Down
31 changes: 16 additions & 15 deletions app/services/external_api/vbms_service.rb
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
require "vbms"

class RailsVBMSLogger
def log(event, data)
case event
when :request
if data[:response_code] != 200
name = data[:request].class.name.split("::").last

Rails.logger.error(
"VBMS HTTP Error #{data[:response_code]}\n" \
"VBMS #{name} Response #{data[:response_body]}"
)



class ExternalApi::VBMSService
class RailsVBMSLogger
def log(event, data)
case event
when :request
if data[:response_code] != 200
name = data[:request].class.name.split("::").last

Rails.logger.error(
"VBMS HTTP Error #{data[:response_code]}\n" \
"VBMS #{name} Response #{data[:response_body]}"
)
end
end
end
end
end

class ExternalApi::VBMSService
def self.fetch_documents_for(download)
request = VBMS::Requests::ListDocuments.new(download.file_number)

Expand Down
2 changes: 1 addition & 1 deletion app/services/external_api/vva_service.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
require "vva"


# Thin interface to talk to Virtual VA
class ExternalApi::VVAService
Expand Down
26 changes: 22 additions & 4 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ module CaseflowEfolder
class Application < Rails::Application
# Initialize configuration defaults for originally generated Rails version.
config.load_defaults 6.1
config.autoloader = :classic

# Configuration for the application, engines, and railties goes here.
#
Expand Down Expand Up @@ -122,9 +121,28 @@ class Application < Rails::Application
# eFolder Specific configs
#---------------------------------------------------------------------------------------
config.download_filepath = Rails.root + "tmp/files"
config.autoload_paths += Dir[Rails.root + 'app/jobs']
config.autoload_paths << Rails.root.join('lib')
config.autoload_paths << Rails.root.join('lib/scripts')

Copy link
Contributor

@jcroteau jcroteau Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move the Zeitwerk inflector settings here (before config.autoload_paths) and delete the initializer, as was done for Caseflow in this commit.

This is a precautionary move, to circumvent potential sequencing issues when autoloading inflected constants in the autoload_once_paths (via the once autoloader).

We are not currently autloading anything with the once autoloader, but if down the road we need to, this will be one less gotcha we'll have to deal with.

For more details on this issue, see rails/rails#45568

config.autoload_paths += [
"#{root}/lib",
]

config.eager_load_paths += [
"#{root}/lib",
]

# A collapse statement will remove the need for a namespace based on the direcotry given.
# Tasks::Support::ModuleOrClassName becomes ModuleOrClassName with the below statements.
Rails.autoloaders.main.collapse(
"app/jobs/middleware",
"#{root}/lib/tasks",
"#{root}/lib/tasks/support"
)

Rails.autoloaders.main.ignore(
"#{root}/lib/assets",
"#{root}/lib/pdfs",
"#{root}/lib/scripts"
)

# Currently the Caseflow client makes calls to get document content directly
# from eFolder Express to reduce load on Caseflow. Since Caseflow and eFolder
Expand Down
4 changes: 3 additions & 1 deletion config/initializers/bgs.rb
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
BGSService = (!BaseController.dependencies_faked? ? ExternalApi::BGSService : Fakes::BGSService)
Rails.application.reloader.to_prepare do
BGSService = (!BaseController.dependencies_faked? ? ExternalApi::BGSService : Fakes::BGSService)
end
7 changes: 4 additions & 3 deletions config/initializers/ruby_claim_evidence_api.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
# frozen_string_literal: true

VeteranFileFetcher = ExternalApi::VeteranFileFetcher
.new(use_canned_api_responses: BaseController.dependencies_faked_for_CEAPI?, logger: Rails.logger)
Rails.application.reloader.to_prepare do
VeteranFileFetcher = ExternalApi::VeteranFileFetcher
.new(use_canned_api_responses: BaseController.dependencies_faked_for_CEAPI?, logger: Rails.logger)
end
4 changes: 3 additions & 1 deletion config/initializers/s3.rb
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
S3Service = (Rails.application.config.s3_enabled ? Caseflow::S3Service : Caseflow::Fakes::S3Service)
Rails.application.reloader.to_prepare do
S3Service = (Rails.application.config.s3_enabled ? Caseflow::S3Service : Caseflow::Fakes::S3Service)
end
2 changes: 1 addition & 1 deletion config/initializers/shoryuken.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
require "#{Rails.root}/app/jobs/middleware/job_metrics_service_metric_middleware"


# set up default exponential backoff parameters
ActiveJob::QueueAdapters::ShoryukenAdapter::JobWrapper
Expand Down
4 changes: 3 additions & 1 deletion config/initializers/vbms.rb
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
VBMSService = (!BaseController.dependencies_faked? ? ExternalApi::VBMSService : Fakes::VBMSService)
Rails.application.reloader.to_prepare do
VBMSService = (!BaseController.dependencies_faked? ? ExternalApi::VBMSService : Fakes::VBMSService)
end
4 changes: 3 additions & 1 deletion config/initializers/vva.rb
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
VVAService = (!BaseController.dependencies_faked? ? ExternalApi::VVAService : Fakes::VVAService)
Rails.application.reloader.to_prepare do
VVAService = (!BaseController.dependencies_faked? ? ExternalApi::VVAService : Fakes::VVAService)
end
29 changes: 29 additions & 0 deletions config/initializers/zeitwerk.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@

# Zeitwerk has specific requirements for auto/eager loading. See below links for more details
# https://guides.rubyonrails.org/classic_to_zeitwerk_howto.html
# https://github.com/fxn/zeitwerk

# To sumarize Zeitwerk requires several conditions be met when attempting to auto/eager load
# - The top level namespace must reflect the auto/eager-loadpath / filename. The auto/eager-load
# path will be different from the relative path. See https://github.com/fxn/zeitwerk#file-structure
# - Zeitwerk expects 1 constant per file, anything not under that constant won't be loaded
# - Acronyms aren't implicit. You can use them but you'll need to tell zeitwerk. (More details below)
# - The constant name and file name MUST match, or zeitwerk needs to be told otherwise.
# So file_name.rb should be `module or class FileName` not `FileNames``.
# - Namespaces need to be unique. Depending on the auto/eager-load path, files with the same
# name but located in different directories can cause issues. To avoid this try and name files uniquely as well.

# Use the command and test below to ensure zeitwerk compliance.
# bin/rails zeitwerk:check
# Alternatively an rspec test has been added, check CI output for details

Rails.autoloaders.each do |autoloader|
# "file_name" => Expected Module or Class name.
autoloader.inflector.inflect(
"bgs_errors" => "BGSErrors",
"bgs_service" => "BGSService",
"poa_mapper" => "POAMapper",
"vbms_service" => "VBMSService",
"vva_service" => "VVAService"
)
end
2 changes: 1 addition & 1 deletion lib/fakes/bgs_service.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
require 'bgs_errors'


class Fakes::BGSService
include ActiveModel::Model
Expand Down
75 changes: 40 additions & 35 deletions lib/fakes/test_auth_strategy.rb
Original file line number Diff line number Diff line change
@@ -1,43 +1,48 @@
require "omniauth/strategies/developer"
require "omniauth/form"

class EfolderAuthForm < OmniAuth::Form
def hidden_field(name, value)
@html << "\n<input type='hidden' name='#{name}' value='#{value}' />"
self
end

def header(title, header_info)
@html << <<-HTML
<!DOCTYPE html>
<html lang="en">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<title>#{title}</title>
#{css}
#{header_info}
</head>
<body>
<h1>#{title}</h1>
<form method='post' #{"action='#{options[:url]}' " if options[:url]}noValidate='noValidate'>
HTML
self
end
end
module Fakes
module TestAuthStrategy
class EfolderAuthForm < OmniAuth::Form
def hidden_field(name, value)
@html << "\n<input type='hidden' name='#{name}' value='#{value}' />"
self
end

class OmniAuth::Strategies::TestAuthStrategy < OmniAuth::Strategies::Developer
# custom form rendering
def request_phase
form = EfolderAuthForm.new(title: "Test VA Saml", url: callback_path)
form.hidden_field :username, session["login"]["username"]
form.hidden_field :station_id, session["login"]["station_id"]
form.button "Fake PIV Login"
form.to_response
end
def header(title, header_info)
@html << <<-HTML
<!DOCTYPE html>
<html lang="en">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<title>#{title}</title>
#{css}
#{header_info}
</head>
<body>
<h1>#{title}</h1>
<form method='post' #{"action='#{options[:url]}' " if options[:url]}noValidate='noValidate'>
HTML
self
end
end

class OmniAuth::Strategies::TestAuthStrategy < OmniAuth::Strategies::Developer
# custom form rendering
def request_phase
form = EfolderAuthForm.new(title: "Test VA Saml", url: callback_path)
form.hidden_field :username, session["login"]["username"]
form.hidden_field :station_id, session["login"]["station_id"]
form.button "Fake PIV Login"
form.to_response
end

def auth_hash
hash = super
hash.uid = hash["info"]["css_id"]
hash
def auth_hash
hash = super
hash.uid = hash["info"]["css_id"]
hash
end
end
end
end
2 changes: 1 addition & 1 deletion lib/scripts/current_user_load_test.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
class CurrentUserLoadTests
class CurrentUserLoadTest
def initialize(iterations = 100)
@iterations = iterations
end
Expand Down
9 changes: 9 additions & 0 deletions spec/initializers/zeitwerk_check_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# frozen_string_literal: true
require "rails_helper"

# See config/initializers/zeitwerk.rb if this fails
RSpec.describe "Zeitwerk Compliance Check" do
it "Eager loads all files without errors" do
expect { Rails.application.eager_load! }.not_to raise_error
end
end
Comment on lines +1 to +9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For conformity w/ Caseflow, let's move this file to spec/config/zeitwerk_spec.rb.

Loading