Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge feature/APPEALS-41559 into prodtest/FY24Q4.5.0 (#22889)
* Start feature work tracking PR with a TODO comment * Remove TODO comment for CodeClimate * SeanC/APPEALS-42315 | Create separate Remand table, model and seed data (#22220) * added migration file for adding type column to supplemental claims table * created stub for remand model * added type to supplemental claim factory and new remand trait * added remand sc's to vha seeds * updated migration name and fixed typo * Changed migration to inherit from Caseflow::Mirgration * schema changes * reverting schema * changed the setting of the type column * fixed schema and renamed migration file * fixed migration and schema * updated the remands factory and seeds * fixed comment in schema --------- Co-authored-by: Robert Travis Pierce <robert@roberttravispierce.com> * TYLERB/APPEALS-42455: Display new and migrated Remands in Decision Review Queue tabs (#22206) * Added support for remands to the business_line.rb model and the index.js util file that parses the decision review type filters for the decision review queue. * Updated the business line spec file, the decision reviews controller spec tests, and the reviews spec feature test to support remands. * Added a decision review polymorphic helper to DRY up some of the polymorphic appeal concerns. * Overrode the remand model association to request issues so active record joins will work correctly. * Added another helper for sti polymorphic relationships. Fixed filtering for STI decision review types for remands. Updated tests and added factories for testing. * Code climate fixes and test fixes. * Fixed more test failures. Removed the remand request issues relationship since it was incorrectly implemented. Added a scope to supplemental claims for remands. Fixed an issue where board grant effectuation tasks without request issues were missing from the new task type count method. * Changed the UNION ALL to a UNION in the new task_type_count method to prevent duplicate row counts. Updated tests and fixed an issue where the board grant effectuation tasks were being counted for pending task queries. * JHoang/APPEALS-42318 (#22270) * some preliminary remand & sc model updates * typo * some preliminary hlr checks? * undo seed change * decision issue and sc updates * attempt code climate fixes * another code climate fix * testing changes * useless variable.. * fix conditional in decision issue * fix undefined find_by method * attempt rspec fix * rspec fix attempt #2 * supplemental_claim_spec fix? * remove comma * code climate fix * added comment for transparency on remand creation, attempt fix for remands route * updated specs for decision_issue, hlr_spec, appeal_spec to account for remand creation type * fix lint error * fix lint error #2 --------- Co-authored-by: Jonathan Hoang <trinhjoanthan.hoang@va.gov> * APPEALS-42458 (#22365) * disable edit issues link for remands * add banners explaining that remands are not editable also makes sure that remands are not editable on the edit issues page * TYLERB/APPEALS-44922: Update Generate task report page UI to include Remand in conditions (#22356) * Initial commit. Changed the DecisionReviewType condition filter to a searchable dropdown and added the remands option to it as well. Updated the report page filter parsing to work with the searchable dropdown instead of checkboxes for the decision review type condition filter. * Updated the ReportPage jest test and the report page feature test to work with the decision review type filter as a searchable dropdown instead of checkboxes. * SeanC/APPEALS-42317 | Migrate existing Supplemental Claim Remands to Remand records via script (#22359) * initial verison of update script * remanded migration file * updated sc spec with type * schema * fixed a few issues in the migration * small fix to migration file * moved the type column check into a seperate it block * fixed wrong column in migration file * created a new migration for creating the view * updated the backfill migration file --------- Co-authored-by: Brandon Lee Dorner <Brandondorner@yahoo.com> * J hoang/appeals 44940 (#22428) * initial * some factory and spec updates * change history query update and spec fixes * code climate/lint fix * change @claim_type identifier * fix claim_history_event_spec.rb * test coverage for change history with Remand claim type * spec lint fixes * refactor claim_type filtering to account for remand subtype * business_line_spec fix * decision_reviews_controller_spec update for remand claim type * lint fixes for decision_reviews_controller_spec.rb * comment updates in business_line.rb * renamed mock data in change_history_reporter_spec * trigger GA * refactor claim type filter for less conditional handling * business line spec fix --------- Co-authored-by: Jonathan Hoang <trinhjoanthan.hoang@va.gov> * Initial commit for the brakeman sql injection warning fixes. (#22598) * Updated the business line reporter class to properly display Remands as a seperate type than SupplementalClaims. Also added additional testing logic for the business line reporter for hlr and scs since it was only covering appeals. (#22664) Co-authored-by: Robert Travis Pierce <robert@roberttravispierce.com> * Fixed a bug where the frontend would error due to calling .toLowerCase() on a null or undefined value which could happen sometimes if the api filter options are in an incorrect state. (#22796) Co-authored-by: Robert Travis Pierce <robert@roberttravispierce.com> * Updated the remand migration files to work with the rails 6 upgrade and the changes to Caseflow::Migration. (#22825) * added fix for failing test in decision_reviews_controller_spec (#22829) * feature/APPEALS-34124-43428-29105-28925-33581 - Rails 6.1 upgrade (release) (#22813) (#22817) * 🔧 Assume defaults for `config.action_dispatch.use_cookies_with_metadata` and `config.action_mailer.delivery_job` The following config settings are not backwards compatible: - config.action_dispatch.use_cookies_with_metadata - config.action_mailer.delivery_job Now that Rails 6.0 is stable on production, we can assume their default values going forward. * ✅ Fix flakey spec * 🔧 Assume default for `config.action_dispatch.use_authenticated_cookie_encryption` Since we are making other cookie configuration changes in this PR for Rails 6.0, this is an opportune time to migrate this Rails 5.2 cookie setting to its default value as well. * ⏪️ Restore overrides for `config.action_dispatch.use_authenticated_cookie_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 * ✨ Add new utility module for adding DB indexes concurrently Introduces `Caseflow::Migrations::AddIndexConcurrently` as a replacement for `Caseflow::Migration` for migrations on ActiveRecord 6.0 and beyond, since `Caseflow::Migration` is forever coupled to ActiveRecord 5.1 due to its extensive use on legacy migrations and should be deprecated moving forward. * 🗑️ Deprecate `Caseflow::Migration` * 🔧 Add instructive error message for non-concurrent `add_index` migrations * 🚨 Address linter / codeclimate complaints * ✨ Introduce `SslRedirectExclusionPolicy` To be used in the environment configuration settings for excluding exempt request paths from SSL redirects when `config. force_ssl = true` * ♻️ Replace deprecated controller-level `force_ssl` Replace deprecated controller-level `force_ssl` with equivalent configuration settings in preparation for the Rails 6.1 upgrade. * 🔥 Remove deprecated config setting `config.active_record.sqlite3.represent_boolean_as_integer` This will have no implications for Caseflow, since we are only using the `sqlite3` adapter nominally for the `demo_vacols` database, which is not actually being used in our demo environments (demo environments are deployed as `development` envs). * ⬆️ Update `caseflow-commons` to resolve sub-dependency conflicts Removes unneeded gems `bourbon` and `neat`, which had a sub-dependency conflict on `thor`. * ⬆️ Update rails and other gems as necessary * 🐛 Fix 'uninitialized constant' error when loading app * ⬆️ bin/rails app:update - Apply relevant changes * 🔧 Override default for `config.active_record.has_many_inversing` * 🔧 Assume default for `config.active_storage.track_variants` We're not currently using ActiveStorage in Caseflow, so it is safe to just assume the default here. * 🔧 Override default for `config.active_job.retry_jitter` The default jitter is probably safe, however, I'm not 100% sure that we don't have any jobs that need to be requeued with exact wait times. So we let's override this for now to stay on the safe side. * 🔧 Assume default for `config.active_job.skip_after_callbacks_if_terminated` We're not currently using `throw :abort` within any `before_enqueue`/`before_perform` callbacks on existing Caseflow jobs, so the default should be fine here. For more background, see https://lilyreile.medium.com/rails-6-1-new-framework-defaults-what-they-do-and-how-to-safely-uncomment-them-c546b70f0c5e#4c60 * 🔧 Assume default for `config.action_dispatch.cookies_same_site_protection` This setting controls the `SameSite` optional attribute for the `Set-Cookie` header. `SameSite=Lax` means that the cookie is not sent on cross-site requests, such as on requests to load images or frames, but is sent when a user is navigating to the origin site from an external site (for example, when following a link). This is the default behavior if the SameSite attribute is not specified. `Lax` is currently the default assumed by both Chrome and Edge browsers when this attribute is left unspecified, so assuming this value should be sensible. It allows us to have our cake (blocking CSRF attacks) and eat it too (providing a logged-in experience when users navigate to Caseflow across origins). For more background, see - https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#samesitesamesite-value - https://lilyreile.medium.com/rails-6-1-new-framework-defaults-what-they-do-and-how-to-safely-uncomment-them-c546b70f0c5e#1f15 * 🔧 Assume default for `config.action_controller.urlsafe_csrf_tokens` * 🔧 Assume default for `ActiveSupport.utc_to_local_returns_utc_offset_times` We're not using `ActiveSupport::TimeZone.utc_to_local` anywhere, so the default is safe to assume here. * 🔧 Assume default for `config.action_dispatch.ssl_default_redirect_status` The default is safe to assume. For more background, see https://lilyreile.medium.com/rails-6-1-new-framework-defaults-what-they-do-and-how-to-safely-uncomment-them-c546b70f0c5e#4c3e * 🔧 Assume default for `config.active_record.legacy_connection_handling` The default should be safe to assume here, as we do not do any role or shard switching on database connections. For more background, see https://lilyreile.medium.com/rails-6-1-new-framework-defaults-what-they-do-and-how-to-safely-uncomment-them-c546b70f0c5e#8007 * 🔧 Assume default for `config.action_view.form_with_generates_remote_forms` We don't use the `form_with` helper anywhere, so this behavior change is inconsequential for us, and we can safely assume the new default. * 🔧 Assume default for `config.active_storage.queues.analysis` We do not use ActiveStorage, so the default is safe to assume here. * 🔧 Assume default for `config.active_storage.queues.purge` We do not use ActiveStorage, so the default is safe to assume here. * 🔧 Assume default for `config.action_mailbox.queues.incineration` We don't use ActionMailbox, so the new default is safe to assume here. * 🔧 Assume default for `config.action_mailbox.queues.routing` We do not use ActionMailbox, so the default is safe to assume here. * 🔧 Assume default for `config.action_mailer.deliver_later_queue_name` We're not using `ActionMailer::MessageDelivery #deliver_later` anywhere, so the default is safe to assume. * 🔧 Assume default for `config.action_view.preload_links_header` This flag can be safely uncommented. Browsers that support Link headers will get a performance boost. Browsers that don’t will ignore them. We override in `development` environments to avoid an edge case leading to an HTTP response header overflow. For more background, see https://lilyreile.medium.com/rails-6-1-new-framework-defaults-what-they-do-and-how-to-safely-uncomment-them-c546b70f0c5e#3679 * 🔥 Remove 'new_framework_defaults_6_1.rb' * 🔧 Load defaults for Rails 6.1 * ♻️ Extract constant * ♻️ Migrate to new Rails deprecation config where applicable * ♻️ Push members down now that there is only one subclass * 🩹 Add forgotten disallowed deprecation warning This deprecation warning was addressed by the following PR, but we forgot to add it to the list of disallowed deprecation warnings: #21614 * 💡 Update comment Task `rake routes` has been replaced with `rails routes` * ✅ Update test to account for change to `ActionDispatch::Response#content_type` `ActionDispatch::Response#content_type` now returns the full Content-Type header * 🚨 Exclude 'config.ru' from Rubocop cops * 🚚 Move 'db/etl/migrate' to 'db/etl_migrate' * 🚚 Move 'db/etl/schema.rb' to 'db/etl_schema.rb' * ♻️ Arrange 'database.yml' configs by environment Group DB configs by environment in anticipation of reformatting for Rails 6+ multi-DB configuration. * 🔧 Reformat 'database.yml' to Rails 6+ multi-DB conventions * 🔧 Add etl migration paths to DB config * 🔧 Update DB connection names in 'database_cleaner' config * ♻️ Use new database-specific rake tasks After migrating to the Rails 6+ native multi-database configuration, the behavior of some DB management tasks, such as `rake db:migrate` changed such that they now act on ALL databases and not just the primary database. So we must replace the invocations of these tasks with their new, database-specific counterparts. * ➖ Remove 'multiverse' gem Now that we have fiully transitioned to Rails-native multi-database support, we are no longer reliant on the 'multiverse' gem and can remove it. * 🗃️ Prohibit execution of vacols DB and non-DB-specific rake tasks After transitioning to Rails-native multi-DB support, the behavior of some DB tasks changed such that they will now act on ALL databases and not just the primary database (ex. `rake db:migrate` will now migrate ALL databases). To avoid accidents, we re-define these tasks here to no-op and output a helpful message to redirect developers toward using their new database-specific counterparts instead. * ♻️ Create new environment for GH workflow 'Make-docs-to-webpage' Instead of performing a bunch of hard-to-maintain `sed` gymnastics to modify the existing 'test' environment, let's create a new 'make_docs' environment (based off of 'test') and configure it appropriately for use by the 'Make-docs-to-webpage' GH workflow. * 💚 Remove redundant DB migrations from CI workflow Task `db:schema:load` already loads the checked in schema, so there should be no need to run `db:migrate` afterwards. * 🐛 Fix `spec/mailers/hearing_mailer_spec.rb` - `NoMethodError` Addresses the following error: NoMethodError: undefined method `build_lookup_context' for ActionView::Base:Class * 🐛 Fix `spec/workflows/post_decision_motion_updater_spec.rb` - `FrozenError` Addresses the following error: FrozenError: can't modify frozen Hash: {} * ✅ Add test for `RoSchedulePeriod` * 🐛 Fix `spec/models/schedule_period_spec.rb` - `ActiveRecord::RecordInvalid` Apparently, there were some changes to the inner workings of `ActiveModel::Errors` in Rails 6.1, causing a model to be considered invalid in the case that `errors[:base] == [[]]`. This makes sense, as `[[]]` is not considered "empty". Unfortunately, this was causing `RoSchedulePeriod #validate_spreadsheet` to inadvertently mark the model as invalid upon creation. `HearingSchedule::ValidateRoSpreadsheet #validate` returns an empty array (`[]`) when valid, which gets pushes onto the `RoSchedulePeriod` `errors[:base]` array, resulting in a non-empty array (`[[]]`) and an erroneously invalid disposition. Furthermore, calling `<<` to an `ActiveModel::Errors` message array in order to add an error is a deprecated, so we can take this opportunity to use the new `#add` API to hit two birds with one stone. The change implemented here is not a pure refactoring, however the end-user experience is unchanged in terms of how errors are presented when attempting to upload a spreadsheet with multiple non-conformities. Down the road, we may want to consider moving `HearingSchedule::ValidateRoSpreadsheet` toward using `ActiveModel::Validations` in order to leverage the full `ActiveModel::Errors` API and construct the errors object in the prescribed manner. For more details see - https://api.rubyonrails.org/v6.1.7.7/classes/ActiveModel/Validations.html - https://api.rubyonrails.org/v6.1.7.7/classes/ActiveModel/Errors.html * 🐛 Fix `spec/mailers/hearing_mailer_spec.rb` - `ActionView::Template::Error` * ✅ Fix `spec/models/veteran_spec.rb` * ✅ Fix `spec/sql/ama_cases_sql_spec.rb` Addresses failures such as the below: 0) AMA Cases Tableau data source expected report calculates age and AOD based on person.dob Failure/Error: expect(aod_case["aod_veteran.age"]).to eq("76") expected: "76" got: 0.76e2 * ✅ Fix multiple specs - `Minitest::UnexpectedError` Test helper method `#perform_enqueued_jobs` now wraps exceptions in an `Minitest::UnexpectedError`: https://github.com/rails/rails/blob/914caca2d31bd753f47f9168f2a375921d9e91cc/activejob/lib/active_job/test_helper.rb#L591 So, to assert that a specific exception is raised during execution of the `#perform_enqueued_jobs` block, we must rescue the `Minitest::UnexpectedError` and make the assertion on its error message instead. * ✅ Fix `spec/lib/helpers/association_wrapper_spec.rb` * ✅ Fix `spec/controllers/api/v1/jobs_controller_spec.rb` In Rails 6.1, `ActiveJob #perform_now` was changed to behave as it did once before (at the behest of GitHub), returning the value fo the job instead of true/false. See related GH issue: rails/rails#38040 * 🐛 Fix `spec/controllers/appeals_controller_spec.rb` - `NoMethodError` Addresses error: NoMethodError: undefined method `workflow' for #<CaseSearchResultsForVeteranFileNumber:0x00007f9a030966c8> 0) AppealsController GET appeals when current user is a System Admin when request header does not contain Veteran ID responds with an error Failure/Error: errors: errors.messages[:workflow], NoMethodError: undefined method `workflow' for #<CaseSearchResultsForVeteranFileNumber:0x00007f9a030966c8> # ./app/workflows/case_search_results_base.rb:31:in `search_call' * 🐛 Fix `CaseSearchResultsBase` validations Addresses test failures in `spec/controllers/appeals_controller_spec.rb` similar to the below: AppealsController GET appeals when current user is a System Admin when request header does not contain Veteran ID responds with an error Failure/Error: expect(response_body["errors"][0]["title"]).to eq "Veteran file number missing" expected: "Veteran file number missing" got: nil Using `ActiveModel::Errors` to store error data in an arbitrary format may have been somewhat permissible in the past, but it is an abuse of the object's intended use and is also proving incompatible with the more formalized `ActiveModels::Errors` API in Rails 6.1. In order to preserve the existing response shape of the affected JSON endpoints, we need to move away from the `ActiveModel::Validations` implementation on `CaseSearchResultsBase` (and its descendent classes) to a more bespoke method of performing validations and aggregating errors, since Rails 6.1 `ActiveModel::Errors` is no longer appropriate for our needs here. * ✅ Fix `spec/controllers/application_controller_spec.rb` -- Cache-Control error Addresses the test failure below: ApplicationController no cache headers when toggle set sets Cache-Control etc Failure/Error: expect(response.headers["Cache-Control"]).to eq "no-cache, no-store" expected: "no-cache, no-store" got: "no-store" (compared using ==) # ./spec/controllers/application_controller_spec.rb:59:in `block (4 levels) in <top (required)>' In Rails 6.1, the `no-store` directive is exclusive of any others that are set on the `Cache-Control` header, which makes sense given the specification https://datatracker.ietf.org/doc/html/rfc7234#section-3 This change was implemented in PR rails/rails#39461 Since it no longer makese sense to set both `no-store` and `no-cache` directives, we will only set `no-store` here, as that is the stronger of the two. * 🐛 Fix multiple specs - `ActiveRecord::EagerLoadPolymorphicError` Addresses multiple test failures caused by the error below: QueueConfig.to_hash title when assigned to an org is formatted as expected Failure/Error: tasks.with_assignees.group("assignees.display_name").count(:all).each_pair.map do |option, count| label = self.class.format_option_label(option, count) self.class.filter_option_hash(option, label) end ActiveRecord::EagerLoadPolymorphicError: Cannot eagerly load the polymorphic association :appeal # ./app/models/queue_column.rb:110:in `assignee_options' * 🐛 Fix `spec/models/task_spec.rb` - `update_all` clears query cache In Rails 6.1.7.7, the method `ActiveRecord::Relation #update_all` will now clear any records cached by the calling relation. This was altering the behavior of `Task #cancel_task_and_child_subtasks` and causing the following test failure: Task#cancel_task_and_child_subtasks cancels all tasks and child subtasks Failure/Error: expect(second_level_tasks[0].versions.count).to eq(initial_versions + 2) expected: 3 got: 2 (compared using ==) # ./spec/models/task_spec.rb:368:in `block (3 levels) in <top (required)>' To remedy, we will now cache the necessary Task records in an Array, which can be used for generating PaperTrail versions both before and after the `update_all`. * 🐛 Fix `spec/services/hearings/calendar_service_spec.rb` - template rendering error Addresses the following test failure: Hearings::CalendarService.confirmation_calendar_invite returns appropriate iCalendar event Failure/Error: expect(ical_event.description).to eq(expected_description) expected: "You're scheduled for a virtual hearing with a Veterans Law Judge of the Board of Veterans' Appeals.\...to reschedule or cancel your virtual hearing, contact us by email at bvahearingteamhotline@va.gov\n" got: #<Icalendar::Values::Text("You're scheduled for a virtual hearing with a Veterans Law Judge of the Bo... reschedule or cancel your virtual hearing, contact us by email at bvahearingteamhotline@va.gov\n")> * 🐛 Fix YAML syntax error caused by whitespace in ENV var Address the following error, found during demo deployment: rake aborted! Cannot load database configuration: YAML syntax error occurred while parsing /caseflow/config/database.yml. Please note that YAML must be consistently indented using spaces. Tabs are not allowed. Error: (<unknown>): could not find expected ':' while scanning a simple key at line 49 column 5 * ⬆️ Update `caseflow-commons` dependency to latest ref Removes `bourbon` and `neat` dependencies. --------- Co-authored-by: Robert Travis Pierce <Pierce_Robert@ne.bah.com> Co-authored-by: Sean Craig <110493538+seancva@users.noreply.github.com> Co-authored-by: Tyler Broyles <109369527+TylerBroyles@users.noreply.github.com> Co-authored-by: jonathanh-va <111081469+jonathanh-va@users.noreply.github.com> Co-authored-by: Jonathan Hoang <trinhjoanthan.hoang@va.gov> Co-authored-by: Brandon Lee Dorner <Brandondorner@yahoo.com> Co-authored-by: Clay Sheppard <csheepy@gmail.com> Co-authored-by: Jeremy Croteau <jcroteau@users.noreply.github.com>
- Loading branch information
9 people
authored
Sep 20, 2024
1 parent
96fac0e
commit 7b45f46