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.

Sign up for GitHub

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

Jump to bottom

BgsPowerOfAttorney model #13922

Merged
merged 59 commits into from
Apr 25, 2020
Merged

BgsPowerOfAttorney model #13922

merged 59 commits into from
Apr 25, 2020

Conversation

pkarman
Copy link
Contributor

@pkarman pkarman commented Apr 8, 2020 •
edited
Loading

Resolves #13883

Description

Converts the BgsPowerOfAttorney service class to an Active Record model.

There's a lot of files touched in this PR, but most of them are fixing test fixture data to work with this new model.

Testing Plan

  1. Monitor nightly warm caches job for errors.

User Facing Changes

None.

Documentation Updates

  • Topline File Descriptions Added or Updated as Needed

Database Changes

Only for Schema Changes

  • Timestamps (created_at, updated_at) for new tables
  • Column comments updated
  • Query profiling performed (eyeball Rails log, check bullet and fasterer output)
  • Appropriate indexes added (especially for foreign keys, polymorphic columns, and unique constraints)
  • DB schema docs updated with make docs
  • #appeals-schema notified with summary and link to this PR

@pkarman pkarman self-assigned this Apr 8, 2020
@va-bot
Copy link
Collaborator

va-bot commented Apr 8, 2020 •
edited
Loading

2 Warnings
⚠️ This is a Big PR. Try to break this down if possible. Stacked pull requests encourage more detailed and thorough code reviews
⚠️ This PR changes the schema. Please use the PR template checklist.

Generated by 🚫 Danger

@codeclimate
Copy link

codeclimate bot commented Apr 8, 2020 •
edited
Loading

Code Climate has analyzed commit 4cd8d65 and detected 0 issues on this pull request.

View more on Code Climate.

@pkarman
Add bgs_power_of_attorneys table
30074a3
@pkarman
add file_number, clarify comments
cb15b3d
@pkarman
refactor service into model
b28e4ab
@pkarman
neither pid nor fn is guaranteed unique alone but only in combination
034b213
@pkarman
allow null
10da5fb
@pkarman
file_number seems more reliable for POA lookup
e758f18
@pkarman
fix test regressions
7785c4b
@pkarman
fix regression
1d53cae
@pkarman
fix test
03c7428
@pkarman
fix test
f585b04
@pkarman
fix test
8c0cbf7
@pkarman
switch back to preferring PID over file number since only way to diff…
a2451ff 
…erentiate multiple claimants on a single appeal
@pkarman
fix test fixture
d241f22
@pkarman
fix test regression
bdd3665
@pkarman
backcompat for tests
a720427
@pkarman
lint
76a8e40
@pkarman
lint
dee7ee9
@pkarman
lint
b570f0c
@pkarman
lint
68ebba1
@pkarman
ddtrace only in aws envs
d89e85b
@pkarman
fix test data
d2db2fb
@pkarman
fix test data
857786a
@pkarman
find_or_load_by_file_number for softer failure
c48e090
@pkarman
fix test data
309cbe9
@pkarman
fix test data
b607244
@nanotone
Copy link
Contributor

👀 as this will likely dovetail with #14004 although that one needs a slightly different BGS endpoint.

leikkisa
leikkisa reviewed Apr 24, 2020
View reviewed changes
Copy link
Contributor

@leikkisa leikkisa left a comment

Choose a reason for hiding this comment

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

Amazing work - the code for this looks really solid 🥇 (the lack of comments is not from lack of digging!).

I saw that there might still be work being done on tests and I haven't reviewed the tests yet, so I'll take one more look tomorrow. This has been really fun to review, but phew, it's a big one!

app/models/bgs_power_of_attorney.rb Outdated Show resolved Hide resolved
app/models/bgs_power_of_attorney.rb Show resolved Hide resolved
app/models/bgs_power_of_attorney.rb Outdated Show resolved Hide resolved
config/initializers/datadog.rb Show resolved Hide resolved
app/services/external_api/bgs_service.rb Show resolved Hide resolved
app/services/external_api/bgs_service.rb Outdated Show resolved Hide resolved
@pkarman
Copy link
Contributor Author

pkarman commented Apr 24, 2020

Thanks for the review @leikkisa -- I have finished the tests I had in mind. I'm sure CI will find some more regressions, which I hope to iron out today.

leikkisa
leikkisa approved these changes Apr 25, 2020
View reviewed changes
Copy link
Contributor

@leikkisa leikkisa left a comment

Choose a reason for hiding this comment

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

Hey @pkarman, I added some more comments, but I think they're easy fixes, so 👍 overall.

I think something needs to happen with get_poa_from_bgs_poa when it's being used on the less reliable BGS service (it returns a different data structure). Most of my comments are about that. Also it'd be better to use that service as the backup for the "by_participant_ids" version instead of the other way around.

app/services/external_api/bgs_service.rb Outdated Show resolved Hide resolved
lib/fakes/bgs_service.rb Outdated Show resolved Hide resolved
spec/controllers/organizations/tasks_controller_spec.rb Outdated Show resolved Hide resolved
spec/models/bgs_power_of_attorney_spec.rb Show resolved Hide resolved
app/models/bgs_power_of_attorney.rb Show resolved Hide resolved
app/models/bgs_power_of_attorney.rb Outdated Show resolved Hide resolved
app/models/bgs_power_of_attorney.rb Outdated Show resolved Hide resolved
app/mappers/power_of_attorney_mapper.rb Show resolved Hide resolved
@pkarman pkarman mentioned this pull request Apr 25, 2020
Closed
4 tasks
@pkarman
Copy link
Contributor Author

pkarman commented Apr 25, 2020

thanks for that catch @leikkisa ! really helped simplify and clarify the changes here. not to mention working the right way! 😅

@pkarman pkarman added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Apr 25, 2020
@va-bot va-bot merged commit c54a195 into master Apr 25, 2020
@va-bot va-bot deleted the pek-bgs-poa-cache branch April 25, 2020 18:35
@pkarman pkarman mentioned this pull request May 15, 2020
Merged
va-bot pushed a commit that referenced this pull request May 15, 2020
@pkarman
Fix regression with POA missing cache (#14305)
f15e162 
### Description

Fixes regression introduced in #13922 with the advent of the BgsPowerOfAttorney AR model.

We were skipping our local db cache and always making BGS calls, which slowed the job down to take hours instead of minutes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leikkisa leikkisa leikkisa approved these changes

@hschallhorn hschallhorn Awaiting requested review from hschallhorn

@tomas-nava tomas-nava Awaiting requested review from tomas-nava

Assignees

@pkarman pkarman

Labels
Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Power of Attorney Tech Spec
4 participants
@pkarman @va-bot @nanotone @leikkisa