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

fix cognitive complexity #1736

Open
wants to merge 36 commits into
base: current-user-mgmt
Choose a base branch
from

Conversation

SanthiParakal133
Copy link
Contributor

Description

Fix cognitive complexity

Acceptance Criteria

  • Code compiles correctly

Testing Plan

  1. Go to ...

User Facing Changes

  • Screenshots of UI changes added to PR & Original Issue
BEFORE AFTER

Code Documentation Updates

  • Add or update code comments at the top of the class, module, and/or component.

Database Changes

Only for Schema Changes

  • Timestamps (created_at, updated_at) for new tables
  • Column comments updated
  • Verify that migrate:rollback works as desired (change supported functions)
  • Query profiling performed (eyeball Rails log, check bullet and fasterer output)
  • Appropriate indexes added (especially for foreign keys, polymorphic columns, unique constraints, and Rails scopes)
  • Add your indexes safely (see Caseflow::Migrations
  • DB schema docs updated with make docs (after running make migrate)
  • #appeals-schema notified with summary and link to this PR
  • Any non-obvious semantics or logic useful for interpreting database data is documented at Caseflow Data Model and Dictionary

Integrations: Adding endpoints for external APIs

  • Check that Caseflow's external API code for the endpoint matches the code in the relevant integration repo
    • Request: Service name, method name, input field names
    • Response: Check expected data structure
  • Update Fakes
  • Integrations impacting functionality are tested in Caseflow UAT

tradesmanhelix and others added 30 commits October 8, 2024 15:04
* Add Pre File Fetch Sensitivity Check

- Verify user and veteran sensitivity levels are compatible.

- Add specs and supporting services needed to perform sensitivity
level checks.

* Implement Banner for Unauthorized Vet Access

* Restore Old Error Message Logic
* Gate VBMS Methods with Sensitivity Checks

Validate user access to veteran before allowing fetch of veteran
data.

* Remove Unneeded allow Directives in Spec
* Remove Unneeded Sensitivity Checks

Checks are already handled by the sensitive_record method.

* Fix Non-Forbidden Banner Styling
…le (#1667)

* Pass Sensitivity Check Feature Toggle to UI

* Restore Manifest Sensitivity Logic
* Fix User Missing for BGS Sensitivity Check

- Update the manifests_controller's refresh method to use the
  find_or_create_by_user method to find a manifest. This will
  ensure that the user is set correctly for BGS calls.

- Move SensitivityLevelCheckFailure logic to the base_controller.

* Fix Misc. Issues

- Move rescue_from for BGS errors into the API V1 controller so
  its existing standard error rescue doesn't catch this exception.

- Improve manifests_controller request spec with sensitivity check
  logic.
- Use new method to check user/veteran sensitivity compatibility
  in the V2 ApplicationController.

- This will prevent the old "use BGS error to verify access" logic
  from running.

Co-authored-by: cacevesva <[email protected]>
* Send Veteran Number in Restart Request

* Update Link rel Param
- Remove recently-added frontend logic for setting veteran ID in
  refresh request as it is unreliable in the way it sets the
  veteran ID.

- Update manifests_controller to set the veteran ID using the
  manifest as this is much more reliable.
- Gem now returns the JSON body of a HTTP response, so our
  response parsing code needed to be updated to handle the new
  format.

- Update the VBMS service to alert us of any API responses that
  can't be parsed so we can troubleshoot them.
- Since the SaveFilesInS3 job is spawned by another job, it does
  not have access to RequestStore[:current_user] which is needed
  for verifying veteran/user sensitivity compatibility.

- This PR also fixes several Rubocop violations in various files.
* Adjusted UI_EXPIRY_HOURS based on deploy environment

* Adjust API HOURS
- Sort output by sensitivity level.

- Display total result count for each level.
* Removed send_user feature flag, combined with use_ce_api

* Updated vbms service spec

* Updated manifest spec for uat expiration hours

* send user feature toggle combined with use_ce_api

* Combined with ce_api feature toggle

* Update failing rspecs

* Remove feature flag from method, wrapped method with all ce api calls

* wrap ce_api related sensitivity changes

* Fix failing specs, reverted to prior code outside of feature flag

* update manifest expiry hours to not change in test + non prod

* remove pry, reverted to previous test case

* If user is blank, return

* update front end error handling

* Linting

* Revert changes
* Change type_description to mapping

* Updated rspecs to handle edge case
* pass user info to ceapi

* Update specs

* update branch name

* fix rspec

* update claim_evidence_request method

* change ref to branch

* revert x86

---------

Co-authored-by: youfoundmanesh <[email protected]>
youfoundmanesh and others added 6 commits October 10, 2024 11:09
* Add CE API Error Handler Class

* Update VBMSService and SensitivityChecker Classes with Error Handling

* Update VBMSService with CE API Error Handling

* Fix Bad Method Signature

* Fix More Spec Failures
- Restore code we know is good.

- Wrap in begin/rescue blocks and handle errors there.

- Add feature toggle to ApplicationController response so frontend
  will display banners correctly.
…-affairs/caseflow-efolder into Deepak/fix-code-climate
@SanthiParakal133 SanthiParakal133 marked this pull request as ready for review November 6, 2024 21:06
@youfoundmanesh youfoundmanesh force-pushed the current-user-mgmt branch 2 times, most recently from 518200c to 05a41c0 Compare November 15, 2024 16:41
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.

4 participants