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

Original claims bug fixes #4104

Merged
merged 18 commits into from
Apr 7, 2020
Merged

Original claims bug fixes #4104

merged 18 commits into from
Apr 7, 2020

Conversation

SMLuthi
Copy link
Contributor

@SMLuthi SMLuthi commented Apr 6, 2020

Description of change

Add extra gating to ITF controller that verifies the user has a valid birls_id. This is to account for an extreme, but possible, edge case.

Fix multiple instances of broken raising ValidationErrors. These errors were expecting a specific object but the instances in questions did not contain what is required... so it was throwing a 500 error instead. These have been replaced with more generic UnprocessableEntity errors that are more flexible.

The add person call in in_progress_form_controller will now raise an error if the add_person call responds with anything but ok.

The clear_cache logic has been removed due to a possible race condition when calling in_progress_form and, subsequently, the itf controller. It was verified that the proxy add was being made successfully but the user object did not reflect this change. Its hard to debug the exact reason but a potential issue is that the subsequent call to find_profile did not contain the new ids. Therefore, the call in the mvi model has been changed to, instead, assign the new ids to the profile directly and recache the profile afterwards. As a bonus, this will avoid making, yet, another call to MVI.

Testing

spec tests
Will be tested in staging with staging user accounts missing the ids

Acceptance Criteria (Definition of Done)

Unique to this PR

  • Add extra gating to ITF controller
  • Fix broken ValidationErrors errors
  • Replace cache destruction with recaching and updating the profile ids directly.
  • Add throwing an error if the add_person call to mvi fails in in_progress_forms controller

Applies to all PRs

  • Swagger docs have been updated, if applicable
  • Provide link to originating GitHub issue, or connected to it via ZenHub
  • Does not contain any sensitive information (i.e. PII/credentials/internal URLs/etc., in logging, hardcoded, or in specs)
  • Provide which alerts would indicate a problem with this functionality (if applicable)

@jholton
Copy link
Contributor

jholton commented Apr 7, 2020

a potential issue is that the subsequent call to find_profile did not contain the new ids.

Do they appear in results from find_profile later?
Is the idea/theory that there is some kind of lag between submitting the user changes to MVI and the MVI service subsequently returning the changes in find_profile requests?

app/models/mvi.rb Outdated Show resolved Hide resolved
app/models/mvi.rb Outdated Show resolved Hide resolved
lib/mvi/responses/add_parser.rb Outdated Show resolved Hide resolved
spec/models/mvi_spec.rb Outdated Show resolved Hide resolved
lib/mvi/service.rb Outdated Show resolved Hide resolved
SMLuthi and others added 3 commits April 7, 2020 09:45
Co-Authored-By: Johnny Holton <johnny@oddball.io>
Co-Authored-By: Johnny Holton <johnny@oddball.io>
Co-Authored-By: Johnny Holton <johnny@oddball.io>
@SMLuthi
Copy link
Contributor Author

SMLuthi commented Apr 7, 2020

@jholton So the whole test case was strange. The proxy add would be called from the SiP call that starts of the form, we verified that MVI received the request and created the ids, but the ITF call to vets-api immediately following SiP would fail due to the user not having the proper authorization (ie. missing the id's). We'd restart the form and then everything would go off without a hitch. The ids had been generated in the profile successfully at that point. I did a bunch of investigating around why it could be that way but I honestly couldnt find a good reason... On paper, the entire process looked perfect since it all was happening sequentially. Redis isnt async so the changes to destroy the cache should have propagated immediately forcing vets-api to make another call to MVI immediately. In the end, I figured that maybe MVI isnt reflecting its own changes immediately and that may be the issue (though its kind of a stretch). Either way, making these changes are good as it removes an extra find profile call we'll be forcing the api to make.

@va-vfs-bot va-vfs-bot temporarily deployed to original_claims_bug_fixes/master April 7, 2020 17:13 Inactive
@va-vfs-bot va-vfs-bot temporarily deployed to original_claims_bug_fixes/master April 7, 2020 17:21 Inactive
Copy link
Contributor

@jholton jholton left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@SarahJaine SarahJaine left a comment

Choose a reason for hiding this comment

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

Excellent, thanks for this work! 🎉

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.

None yet

5 participants