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

eFolder Rails 6.1 Rebase Alec k/appeals 46558 #1636

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

AKeyframe
Copy link
Contributor

Resolves APPEALS-43567
This is a revisit of the work done for APPEALS-28929 - updating eFolder to 6.1 PR

Description

🔴 Warning: Merge & deploy only after APPEALS-43566 has been merged to master. 🔴

Updates Rails to 6.1

  • pg gem updated for postgres runtime
  • Fixed depreciated initialization autoloaded constants
  • Neat / Bourbon removal (required for rails 6.1 upgrade)

Acceptance Criteria

  • See documentation produced in APPEALS-28920
  • Perform steps in Rails Upgrade Guide 1.3.1 through 1.5
  • Address broken tests (if any)
  • Fix breaking changes introduced by Rails or updated dependencies

Testing Plan

Regression testing via UAT

🔴 Warning: Merge & deploy only after APPEALS-43566 has been merged to master. 🔴

Copy link
Contributor

@jcroteau jcroteau left a comment

Choose a reason for hiding this comment

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

Looking good. Just a few minor changes needed. Let me know if you'd like to go over any of these together...

@@ -231,7 +230,7 @@ GEM
bigdecimal
rexml
crass (1.0.6)
d3-rails (5.9.2)
d3-rails (7.8.5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally, I would say we could omit this update to d3-rails (though I realize it is being pulled in via the caseflow-commons update).

However, it doesn't appear that d3 is being used at all in eFolder, so I guess it shouldn't hurt anything to update it 🤷

config.ru Outdated Show resolved Hide resolved
config/application.rb Outdated Show resolved Hide resolved
config/application.rb Show resolved Hide resolved
config/application.rb Show resolved Hide resolved
config/application.rb Outdated Show resolved Hide resolved
config/application.rb Outdated Show resolved Hide resolved
config/environments/development.rb Outdated Show resolved Hide resolved
config/environments/test.rb Outdated Show resolved Hide resolved
@jcroteau jcroteau self-requested a review July 23, 2024 19:46
jcroteau
jcroteau previously approved these changes Jul 23, 2024
@jcroteau jcroteau self-requested a review July 23, 2024 19:46
@jcroteau
Copy link
Contributor

🎉 Thanks for addressing my concerns above!

I'll make my final review/approval once department-of-veterans-affairs/caseflow-commons#225 gets merged and the associated ref is updated in Gemfile/Gemfile.lock.

@jcroteau
Copy link
Contributor

📝 Note to self / other reviewers

The following test failures are also present on other eFolder builds; They are not related to the changes in this PR:

Failures:

  1) Backend Error Flows When VBMS returns an error Download with VBMS connection error
     Failure/Error: expect(page).to have_css ".usa-alert-heading", text: "We are having trouble connecting to VBMS"
       expected to find visible css ".usa-alert-heading" with text "We are having trouble connecting to VBMS" but there were no matches. Also found "We could not complete the search for this Veteran ID", which matched the selector but not all filters. 
     # ./spec/features/backend_error_flows_spec.rb:75:in `block (4 levels) in <top (required)>'
     # ./spec/features/backend_error_flows_spec.rb:70:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:37:in `block (2 levels) in <top (required)>'

  2) Backend Error Flows When VVA returns an error Download with VVA connection error
     Failure/Error: expect(page).to have_css ".usa-alert-heading", text: "We are having trouble connecting to VVA"
       expected to find visible css ".usa-alert-heading" with text "We are having trouble connecting to VVA" but there were no matches. Also found "We could not complete the search for this Veteran ID", which matched the selector but not all filters. 
     # ./spec/features/backend_error_flows_spec.rb:97:in `block (4 levels) in <top (required)>'
     # ./spec/features/backend_error_flows_spec.rb:92:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:37:in `block (2 levels) in <top (required)>'

  3) React Downloads Creating a download
     Failure/Error: V2::DownloadManifestJob.perform_later(self, RequestStore[:current_user])
     
       (V2::DownloadManifestJob (class)).perform_later(#<ManifestSource id: 1, manifest_id: 1, status: "initialized", name: "VBMS", fetched_at: nil, created_at: "2024-07-23 19:28:50.908649000 +0000", updated_at: "2024-07-23 19:28:50.908649000 +0000">, #<User id: 1, css_id: "123123", station_id: "116", email: "[email protected]", created_at: "2024-07-23 1...23 19:28:50.735062114 +0000", vva_coachmarks_view_count: 0, participant_id: nil, last_login_at: nil>)
           expected: 2 times with any arguments
           received: 3 times with arguments: (#<ManifestSource id: 1, manifest_id: 1, status: "initialized", name: "VBMS", fetched_at: nil, created_at: "2024-07-23 19:28:50.908649000 +0000", updated_at: "2024-07-23 19:28:50.908649000 +0000">, #<User id: 1, css_id: "123123", station_id: "116", email: "[email protected]", created_at: "2024-07-23 1...23 19:28:50.735062114 +0000", vva_coachmarks_view_count: 0, participant_id: nil, last_login_at: nil>)
     # ./app/models/manifest_source.rb:22:in `start!'
     # ./app/models/manifest.rb:34:in `start!'
     # ./app/controllers/api/v2/manifests_controller.rb:16:in `refresh'
     # ------------------
     # --- Caused by: ---
     # Capybara::CapybaraError:
     #   Your application server raised an error - It has been raised in your test code because Capybara.raise_server_errors == true
     #   ./spec/spec_helper.rb:37:in `block (2 levels) in <top (required)>'

Finished in 1 minute 33.23 seconds (files took 3.36 seconds to load)
262 examples, 3 failures, 4 pending

Failed examples:

rspec ./spec/features/backend_error_flows_spec.rb:69 # Backend Error Flows When VBMS returns an error Download with VBMS connection error
rspec ./spec/features/backend_error_flows_spec.rb:91 # Backend Error Flows When VVA returns an error Download with VVA connection error
rspec ./spec/features/react_download_spec.rb:65 # React Downloads Creating a download

@AKeyframe AKeyframe marked this pull request as ready for review July 26, 2024 16:21
jcroteau
jcroteau previously approved these changes Jul 26, 2024
Copy link
Contributor

@jcroteau jcroteau left a comment

Choose a reason for hiding this comment

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

🎈

AKeyframe and others added 11 commits August 13, 2024 12:35
Removes `bourbon` and `neat` dependencies.
During assets precompile in a 'production' environment, we encountered the following error:

  Uglifier::Error: Unexpected token: name (compare1).
  To use ES6 syntax, harmony mode must be enabled with Uglifier.new(:harmony => true).

Per the `uglifier` README:

  UglifyJS only works with ES5. If you need to compress ES6, `ruby-terser` is a better option.

Looking at the Caseflow git history for comparison, it looks like the `uglifier` gem was
removed in favor of using Webpack to perform JS compression via the `UglifyjsWebpackPlugin`.
Later, the `UglifyjsWebpackPlugin` was removed when Webpack v4 incorporated the
`TerserWebpackPlugin` out-of-the-box:
https://github.com/department-of-veterans-affairs/caseflow-efolder/blob/9853eaeb98692099f1e62435de9a4dc08292fa53/client/yarn.lock#L6119

It appears that there may need to be some additional configuration added to the
`webpack.config.js` file in order to leverage the Terser plugin:
https://v4.webpack.js.org/plugins/terser-webpack-plugin/

However, the Caseflow `webpack.config.js` does not include the Terser configuration at this time,
and so, in keeping parity with Caseflow, we will omit this configuration in eFolder as well
and leave it as a future exercise should it be necessary to enact JS compression.
…okie_encryption` and `config.action_dispatch.use_cookies_with_metadata`

While testing in PreProd, we discovered that, without these cookie config overrides,
re-authentication was broken -- after logging out, a user could not log back in.

Since the default settings are still optional going forward, we can restore these
overrides and devise a solution to migrate cookies later.

For more details, see Jira story APPEALS-54897:

https://jira.devops.va.gov/browse/APPEALS-54897
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants