-
Notifications
You must be signed in to change notification settings - Fork 66
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
Dbex user identifiers status check #14858
Conversation
Includes the mapping of required Form526 user identifier data generated by the Users::Form526UserIdentifiersStatusService in the user profile returned from the main user endpoint. The front end will use this information to display to the user any identifiers we don't have in our system, so they can pass that information on to the Contact Center. Adds a feature flag, form_526_required_identifiers_in_user_object, and gates the presence of this data in the profile.
Updates four JSON schema validation files to relfect the addition of the form526_required_identifier_presence key to the users profile metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM
Removes the front end feature flags I initially implemented here alongside the back end changes in department-of-veterans-affairs/vets-api#14858. I decided it would simplify the code and release rollout to simply check if the new missing identifiers metadata was present (meaning the back end feature toggle was on), and make the component rendering dependent on that instead of checking a flag that is unecessary in this case.
Forgot to pass the user arg to this feature flag. We want to allow this level of control so we could release this change progressively if we wanted to
@jerekshoe recall we spoke about this some months ago, apologies for the delay in revisiting it my team deprioritized this fix for a while. I figured you may be the best person to look at this but feel free to delegate. The PR description should make things clear enough but feel free to ping me if it makes more sense to hop on a call at some point. One open I question I have is if we want to/it's even possible to do testing on this staging. You may recall I asked for your help in learning how to modify some of the XML for mock data users to remove SSN, birth date etc. which is how I was able to test this locally. Is it possible to do something along those lines in staging or is it probably best just to open the feature gate for a small amount of users to start in production? Thanks! |
The user argument doesn't seem to work properly when enabling and disabling Flippers in specs. Confident this is working properly in the actual codebase though.
Prefer unless statement and early return to if statment in this case
describe 'participant_id validation' do | ||
context 'when participant_id is missing' do | ||
it "returns a hash with 'participant_id' marked false" do | ||
allow(user).to receive(:participant_id).and_return(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗣️ Have you tried a different approach that didn't require stubbing out the messages? For example:
RSpec.describe Users::Form526UserIdentifiersStatusService do
describe '#call' do
subject(:call_service) { described_class.call(user) }
describe 'participant_id validation' do
let(:user) { build(:user, participant_id:) }
context 'when participant_id is missing' do
let(:participant_id) { nil }
it "returns a hash with 'participant_id' marked false" do
result = call_service
expect(result['participant_id']).to be_falsey
end
end
context 'when participant_id is present' do
let(:participant_id) { '8675309' }
it "returns a hash with 'participant_id' marked true" do
result = call_service
expect(result['participant_id']).to be_truthy
end
end
end
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good suggestion, this needs to go out soon but I'll keep this in mind for my next specs!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for addressing the feedback. I left one additional comment about the specs, but I don't want to hold you up, so feel free to ignore if you don't feel it needs to be addressed.
#25428) * Implement 526 missing identifiers warning component Creates a basic component and unit tests to tell the user they are missing certain identifiers we require before starting the Form 526 flow, and dynamically renders which identifiers are missing so they can share that information with a contact center This is intended to replace all of the the inconsistent and inaccurate message components in src/applications/disability-benefits/all-claims/containers/MissingServices.jsx * Refactor Missing526Identifiers containerm tests Cleans up the formatting and rendering code for Missing526Identifiers to make it more logically organized. Addresses problems with tests that seemed to be working before; ended up having to use a regexp match on the container content for my message paragraph; using typical React Testing Library functions such as getByText is problematic and causes confusing failures because it doesn't support web components, which va-alert is (it's part of the VA component library) * Add Google Analytics logging to message display Adds a recordEvent call to log the appearance of the error message and dynamically includes which data was missing. May consider a better way to include the list of missing identifiers in the analytics log so it is easier to query on in the future. Fix typo on participantId key in test and prop types * Refactor Form526 to render new error messages Replaces the existng Missing Services component renders in Form526EZApp.jsx with the new Missing526Identifiers component. This was done to standardize the messsaging during these situations and to provide the user with exactly which data is missing to share with the contact center. Adds default claims arg to the test page in Form526EZApp.unit.spec.jsx. These styles of tests are now deprecated in our system, but I kept my new tests in the same style for consistency. * Refactor list formatting for older browser compatibility Adds a new utility function to our claims utilities for formatting a list as a human readable string with the correct commas and use of 'and'. Replaces my initial implementation of this in the Missing526Identifiers endpoint which used the Intl.ListFormat function: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat As this is a relatively new feature of JavaScript not supported by older browsers (I was told Safari 9 for example, which many veterans are still on, frequently raises errors when we use these features), it's better to use the more vanilla, classic JavaScript approach * Feature gate new and old messaging behavior Adds back the existing messages behavior I had removed in an earlier commit and wraps the new and old behavior in a feature flag. * Add empty string safeguard to title formatter The default argument to this titleLowerCase function is an empty string, however the destructuring below it will leave the firstChar undefined and would error if we tried to lower case it * Refactor Alert to use React children pattern * Improve language in Missing526Identifiers spec * Remove feature flag toggles Removes the front end feature flags I initially implemented here alongside the back end changes in department-of-veterans-affairs/vets-api#14858. I decided it would simplify the code and release rollout to simply check if the new missing identifiers metadata was present (meaning the back end feature toggle was on), and make the component rendering dependent on that instead of checking a flag that is unecessary in this case. * Revert accidental removal of important docstring from existing code * Add slot attr to h2 in VA alert * Use existing utility for formatting lists I wasn't aware of this readable list utility when I made my own, so deleting my implementation and relying on this existing helper. * Fix docstring typo * PR Feedback: Add v3 uswds styling tag to alert message Adds the uswds tag to the alert poriton of the new More Info Needed error message for styling consistency
* Resolve bundle-audit error (#15704) * Change rack-cors version to fix bundle-audit error * Add ignore flag to bundle audit * Restore rack cors version 2.0.1 * Update bundle-audit * Fix Gemfile.lock (#15700) * remove x86_64-darwin-22 using bundle lock --remove-platform command * add ruby using bundle lock --add-platform command * Dbex user identifiers status check (#14858) * Include Form526 user identifier presence mapping in user profile Includes the mapping of required Form526 user identifier data generated by the Users::Form526UserIdentifiersStatusService in the user profile returned from the main user endpoint. The front end will use this information to display to the user any identifiers we don't have in our system, so they can pass that information on to the Contact Center. Adds a feature flag, form_526_required_identifiers_in_user_object, and gates the presence of this data in the profile. * Update User LOA JSON Schema validations for 526 identifiers Updates four JSON schema validation files to relfect the addition of the form526_required_identifier_presence key to the users profile metadata * Add user arg to feature flags Forgot to pass the user arg to this feature flag. We want to allow this level of control so we could release this change progressively if we wanted to * Satisfy outstanding Rubocop warning to use anonymous positional arguments forwarding * Fix Flipper disable in profile test The user argument doesn't seem to work properly when enabling and disabling Flippers in specs. Confident this is working properly in the actual codebase though. * Address style suggestion in metadata conditional Prefer unless statement and early return to if statment in this case --------- Co-authored-by: Eric Boehs <[email protected]> * Adds count of records written to log message (#15705) * Use LOA2 instead of LOA3 for datebox timestamp logic (#15655) * revert(vaos): removed clinic stop code logging (#15693) va.gov-team#71272 * Filter Unwanted Errors from Retrying (Dependents Forms) (#15545) * Add filters from exception cause * Use sidekiq skip mechanism * Use common method to submit backup * Move filter handling to method * Params for filter method * Lint issues * Fix icn as instance var * Missed 686c param * Move salvage form to top of rescue block * Same for other job * Remove code checks * updated cve file in build workflow --------- Co-authored-by: Holden Hinkle <[email protected]> Co-authored-by: Nathan Burgess <[email protected]> Co-authored-by: Eric Boehs <[email protected]> Co-authored-by: gia-lexa <[email protected]> Co-authored-by: Eric Tillberg <[email protected]> Co-authored-by: AJ Magdub <[email protected]> Co-authored-by: Tyler Fink <[email protected]> Co-authored-by: RachalCassity <[email protected]>
#25428) * Implement 526 missing identifiers warning component Creates a basic component and unit tests to tell the user they are missing certain identifiers we require before starting the Form 526 flow, and dynamically renders which identifiers are missing so they can share that information with a contact center This is intended to replace all of the the inconsistent and inaccurate message components in src/applications/disability-benefits/all-claims/containers/MissingServices.jsx * Refactor Missing526Identifiers containerm tests Cleans up the formatting and rendering code for Missing526Identifiers to make it more logically organized. Addresses problems with tests that seemed to be working before; ended up having to use a regexp match on the container content for my message paragraph; using typical React Testing Library functions such as getByText is problematic and causes confusing failures because it doesn't support web components, which va-alert is (it's part of the VA component library) * Add Google Analytics logging to message display Adds a recordEvent call to log the appearance of the error message and dynamically includes which data was missing. May consider a better way to include the list of missing identifiers in the analytics log so it is easier to query on in the future. Fix typo on participantId key in test and prop types * Refactor Form526 to render new error messages Replaces the existng Missing Services component renders in Form526EZApp.jsx with the new Missing526Identifiers component. This was done to standardize the messsaging during these situations and to provide the user with exactly which data is missing to share with the contact center. Adds default claims arg to the test page in Form526EZApp.unit.spec.jsx. These styles of tests are now deprecated in our system, but I kept my new tests in the same style for consistency. * Refactor list formatting for older browser compatibility Adds a new utility function to our claims utilities for formatting a list as a human readable string with the correct commas and use of 'and'. Replaces my initial implementation of this in the Missing526Identifiers endpoint which used the Intl.ListFormat function: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat As this is a relatively new feature of JavaScript not supported by older browsers (I was told Safari 9 for example, which many veterans are still on, frequently raises errors when we use these features), it's better to use the more vanilla, classic JavaScript approach * Feature gate new and old messaging behavior Adds back the existing messages behavior I had removed in an earlier commit and wraps the new and old behavior in a feature flag. * Add empty string safeguard to title formatter The default argument to this titleLowerCase function is an empty string, however the destructuring below it will leave the firstChar undefined and would error if we tried to lower case it * Refactor Alert to use React children pattern * Improve language in Missing526Identifiers spec * Remove feature flag toggles Removes the front end feature flags I initially implemented here alongside the back end changes in department-of-veterans-affairs/vets-api#14858. I decided it would simplify the code and release rollout to simply check if the new missing identifiers metadata was present (meaning the back end feature toggle was on), and make the component rendering dependent on that instead of checking a flag that is unecessary in this case. * Revert accidental removal of important docstring from existing code * Add slot attr to h2 in VA alert * Use existing utility for formatting lists I wasn't aware of this readable list utility when I made my own, so deleting my implementation and relying on this existing helper. * Fix docstring typo * PR Feedback: Add v3 uswds styling tag to alert message Adds the uswds tag to the alert poriton of the new More Info Needed error message for styling consistency
#25428) * Implement 526 missing identifiers warning component Creates a basic component and unit tests to tell the user they are missing certain identifiers we require before starting the Form 526 flow, and dynamically renders which identifiers are missing so they can share that information with a contact center This is intended to replace all of the the inconsistent and inaccurate message components in src/applications/disability-benefits/all-claims/containers/MissingServices.jsx * Refactor Missing526Identifiers containerm tests Cleans up the formatting and rendering code for Missing526Identifiers to make it more logically organized. Addresses problems with tests that seemed to be working before; ended up having to use a regexp match on the container content for my message paragraph; using typical React Testing Library functions such as getByText is problematic and causes confusing failures because it doesn't support web components, which va-alert is (it's part of the VA component library) * Add Google Analytics logging to message display Adds a recordEvent call to log the appearance of the error message and dynamically includes which data was missing. May consider a better way to include the list of missing identifiers in the analytics log so it is easier to query on in the future. Fix typo on participantId key in test and prop types * Refactor Form526 to render new error messages Replaces the existng Missing Services component renders in Form526EZApp.jsx with the new Missing526Identifiers component. This was done to standardize the messsaging during these situations and to provide the user with exactly which data is missing to share with the contact center. Adds default claims arg to the test page in Form526EZApp.unit.spec.jsx. These styles of tests are now deprecated in our system, but I kept my new tests in the same style for consistency. * Refactor list formatting for older browser compatibility Adds a new utility function to our claims utilities for formatting a list as a human readable string with the correct commas and use of 'and'. Replaces my initial implementation of this in the Missing526Identifiers endpoint which used the Intl.ListFormat function: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat As this is a relatively new feature of JavaScript not supported by older browsers (I was told Safari 9 for example, which many veterans are still on, frequently raises errors when we use these features), it's better to use the more vanilla, classic JavaScript approach * Feature gate new and old messaging behavior Adds back the existing messages behavior I had removed in an earlier commit and wraps the new and old behavior in a feature flag. * Add empty string safeguard to title formatter The default argument to this titleLowerCase function is an empty string, however the destructuring below it will leave the firstChar undefined and would error if we tried to lower case it * Refactor Alert to use React children pattern * Improve language in Missing526Identifiers spec * Remove feature flag toggles Removes the front end feature flags I initially implemented here alongside the back end changes in department-of-veterans-affairs/vets-api#14858. I decided it would simplify the code and release rollout to simply check if the new missing identifiers metadata was present (meaning the back end feature toggle was on), and make the component rendering dependent on that instead of checking a flag that is unecessary in this case. * Revert accidental removal of important docstring from existing code * Add slot attr to h2 in VA alert * Use existing utility for formatting lists I wasn't aware of this readable list utility when I made my own, so deleting my implementation and relying on this existing helper. * Fix docstring typo * PR Feedback: Add v3 uswds styling tag to alert message Adds the uswds tag to the alert poriton of the new More Info Needed error message for styling consistency
* merged previous work into v2 app * Burials * Merge fixes / redirect / flipper / Intro page functional React * attempt to fix infinite loop * conditional required field changes / relies on pattern changes * add return statements for specs * fixed duplicates / fixed number validation for plotAllowancePartOne * schema changes * Set DataDog log telemetrySampleRate to 100 (#28207) * [21-0966] Implement prefill and read-only veteran information page (#28193) * Implement prefill for form 21-0966 * Implement hasVeteranPrefill function to check for valid prefill data for the veteran flow * Update pages to use strong tags * Implement read only veteran summary page based on prefill data * Implement prefill for form 21-0966 * Implement hasVeteranPrefill function to check for valid prefill data for the veteran flow * Update pages to use strong tags * Implement read only veteran summary page based on prefill data * Update initialize helper function to retain prefill data * Added page not found for unknown URLs in medical records (#28148) * 76825 poa requests table (#28190) * Create POST for declining POA request * Create POST for accepting POA request * Refactor to combine accept and decline using action prop * Create POA Requests Table * Integrate table into POARequests page with a shared mockPOARequests * Fix 'View all' link on widget to allow navigation to table * Add unit tests to fully test table content * Add e2e check for table existing * Fix View all Link jest test * Refactor fetches to two methods to give the caller a more constrained interface * Refactor status check in functional way to enhance readability and maintainability * Move isActionable outside of react component * Schema changes (#28189) * init * schema changes * Update list loop aria labels (#28161) * 526: Implement more accurate "We're missing some information" messages (#25428) * Implement 526 missing identifiers warning component Creates a basic component and unit tests to tell the user they are missing certain identifiers we require before starting the Form 526 flow, and dynamically renders which identifiers are missing so they can share that information with a contact center This is intended to replace all of the the inconsistent and inaccurate message components in src/applications/disability-benefits/all-claims/containers/MissingServices.jsx * Refactor Missing526Identifiers containerm tests Cleans up the formatting and rendering code for Missing526Identifiers to make it more logically organized. Addresses problems with tests that seemed to be working before; ended up having to use a regexp match on the container content for my message paragraph; using typical React Testing Library functions such as getByText is problematic and causes confusing failures because it doesn't support web components, which va-alert is (it's part of the VA component library) * Add Google Analytics logging to message display Adds a recordEvent call to log the appearance of the error message and dynamically includes which data was missing. May consider a better way to include the list of missing identifiers in the analytics log so it is easier to query on in the future. Fix typo on participantId key in test and prop types * Refactor Form526 to render new error messages Replaces the existng Missing Services component renders in Form526EZApp.jsx with the new Missing526Identifiers component. This was done to standardize the messsaging during these situations and to provide the user with exactly which data is missing to share with the contact center. Adds default claims arg to the test page in Form526EZApp.unit.spec.jsx. These styles of tests are now deprecated in our system, but I kept my new tests in the same style for consistency. * Refactor list formatting for older browser compatibility Adds a new utility function to our claims utilities for formatting a list as a human readable string with the correct commas and use of 'and'. Replaces my initial implementation of this in the Missing526Identifiers endpoint which used the Intl.ListFormat function: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/ListFormat As this is a relatively new feature of JavaScript not supported by older browsers (I was told Safari 9 for example, which many veterans are still on, frequently raises errors when we use these features), it's better to use the more vanilla, classic JavaScript approach * Feature gate new and old messaging behavior Adds back the existing messages behavior I had removed in an earlier commit and wraps the new and old behavior in a feature flag. * Add empty string safeguard to title formatter The default argument to this titleLowerCase function is an empty string, however the destructuring below it will leave the firstChar undefined and would error if we tried to lower case it * Refactor Alert to use React children pattern * Improve language in Missing526Identifiers spec * Remove feature flag toggles Removes the front end feature flags I initially implemented here alongside the back end changes in department-of-veterans-affairs/vets-api#14858. I decided it would simplify the code and release rollout to simply check if the new missing identifiers metadata was present (meaning the back end feature toggle was on), and make the component rendering dependent on that instead of checking a flag that is unecessary in this case. * Revert accidental removal of important docstring from existing code * Add slot attr to h2 in VA alert * Use existing utility for formatting lists I wasn't aware of this readable list utility when I made my own, so deleting my implementation and relying on this existing helper. * Fix docstring typo * PR Feedback: Add v3 uswds styling tag to alert message Adds the uswds tag to the alert poriton of the new More Info Needed error message for styling consistency * Mhv 52717 implement visibility restrictions to see mr automated test (#27579) * MHV-52717 Implement visibility restrictions to see MR automated test * MHV-52717 Implement visibility restrictions to see MR automated test * Implement visibility restrictions to see MR automated test. * Add TXT Download for consolidated report (all domains/Blue Button) Update feature toggles * Implement visibility restrictions to see MR automated test. --------- Co-authored-by: Aaron Read <[email protected]> * Mhv 54452 thread with 6 more messages (#28200) * add: verify accordions focus * add: thread-accordions focus verification * add: comment assertion * upd: add wait to test * upd: thread-response.json * add: if else statement * upd: thread-accordion.cypress.spec.js * upd: fix failed test * upd: fix loadSingleThread method * upd: fix failed test --------- Co-authored-by: Aaron Read <[email protected]> * Add profile loading guards * new form version * required field changes * Intro page InProgress component * redirect specs, remove allowance specs * unit tests for chapters 1-5 * labels and page arrangment change * added version to payload * reference V2 * fixes known accessibility issues * remove v1 specs * Add v2 constant * review page tweaks and accessibility * maximal-test.json * remove burial date warning * review page finalization * more CAIA feedback changes * statement of truth change * added feature flag back after cleanup on main * Confirmation page / cleanup * overflow json data * additonal evidence section * typo fix * test data changes * cypress specs * button up / documentation update started * confirmation page finalization * clean up * documentation continued * more docs * spec cleanup * removed moment from submit * temporarily remove v1 schema test * fix failing specs * use version 0 * remove schema test not applicable because not all components used in the form originate from vets-json-schema * classnames * pre-review items: cypress axe added, fdc page added, fdc prop added * additional unit tests * remove unused logic * Intro page link changes * test coverage updates * Team QA fixes: round 1 * Team QA fixes: round 2 * Update README.md * added sme feedback fields to locationOfDeath * Team QA fixes: round 2 (cont.) * adds phoneSchema and emailSchema * added missing email label * Team QA fixes: round 3 * Team QA fixes: round 3 cont. * disable conditional requirement * fixed custom validation for burial date * fix specs * min, max, ovr .json updates * removes broken test --------- Co-authored-by: Dakota Larson <[email protected]> Co-authored-by: Rob Garrison <[email protected]> Co-authored-by: Nick Sprinkle <[email protected]> Co-authored-by: Carlos Felix <[email protected]> Co-authored-by: William Phelps <[email protected]> Co-authored-by: Todd Rizzolo <[email protected]> Co-authored-by: Nathan Burgess <[email protected]> Co-authored-by: Anal Gomes <[email protected]> Co-authored-by: Aaron Read <[email protected]> Co-authored-by: fazilqa <[email protected]> Co-authored-by: Dakota Larson <[email protected]> Co-authored-by: Evan Smith <[email protected]> Co-authored-by: Thomas Blackwell <[email protected]>
Summary
Overview
Implements the back end portion of a highly-requested fix to confusing and misleading error messages Users encounter when attempting to fill out Form 526. This has been a significant source of frustration for Veterans, as the message blocks the User from proceeding and prompts them to reach out to a Contact Center, but only provides a limited and in some cases completely inaccurate indication of what the problem is.
A recent round of Medalia research made this need even more clear.
The changes are placed behind a feature gate as we may want to release this progressively. The front end is implemented here
Here is a demo of the UX for the page before and after the changes:
Screen.Recording.2024-01-02.at.5.57.15.PM.mov
Changes
Adds a new set of Form 526-related metadata to the main user profile JSON object returned from the user endpoint (
/v0/user
) that drives that portion of the VA website.The data is nested under the "claims" portion of the profile and lists a handful of User identifier fields we require to be present for a User to proceed with filling out Form 526, and indicates whether we have them or not:
The changes are hidden behind a feature flag to allow us to safely test this in staging.
(Note, an earlier version of these changes was PRed over the summer, but this fix was deprioritized. I've elected to start with a fresh branch for various reasons I can expand on )
The Problem
The front end logic for Form526 prevents a user from starting their claim if any of the following data is missing in the User object we have stored for them:
Currently, when some of this data is missing, we stop them from proceeding with the form, and display one of a number of different messages like the one included at the top of this PR description
After tracing through some convulted code in the front and back end (link to findings report here), I discovered these messages aren't even really accurate, aren't standardized and in some cases don't tell the User anything useful at all.
The Solution
As implemented in the front end PR for this fix, the front end will use the more detailed metadata added in this PR to display a single standard message along with the exact list of what is missing, so they can pass that information along to the contact center.
Which team do you work for, does your team own the maintenance of this component?: I am on the DBex team, working specifically to improve submission success rates on the 526ez form. This work will only affect the 526ez flow, which is maintained by myself and my team.
(If using a flipper, what is the end date of the flipper being required/success criteria being targeted) Unknown, my team is still new and working out what our release strategy will be, but I imagine after a couple weeks of having these improved messages live we will feel comfortable enabling them for all users.
Related issue(s)
Testing done
Testing this locally is a little challenging, as we need to muck with the mock data for a user and even then I could only figure out how to remove a handful of the data we are checking for (figured out how to delete the BIRLS ID, SSN and birth date for a User).
For the other cases, automated tests in the front and back end give me a good amount of confidence this works as intended.
What areas of the site does it impact?
This should only affect the Form 526 workflow.
Acceptance criteria
Requested Feedback
N/A