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

[ARP POA Submission] (#5) Update POA Form Submission Scopes (#101919) #21155

Merged
merged 2 commits into from
Mar 7, 2025
Merged

[ARP POA Submission] (#5) Update POA Form Submission Scopes (#101919) #21155

merged 2 commits into from
Mar 7, 2025

Conversation

nihil2501
Copy link
Contributor

No description provided.

@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/update-poa-form-submission-scopes.oren/main/main March 7, 2025 15:15 Inactive
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 7, 2025 •
edited
Loading

1 Warning
⚠️ This PR changes 211 LoC (not counting whitespace/newlines).

In order to ensure each PR receives the proper attention it deserves, we recommend not exceeding
200. Expect some delays getting reviews.

File Summary

Files

  • modules/accredited_representative_portal/app/controllers/accredited_representative_portal/v0/power_of_attorney_requests_controller.rb (+17/-6)

  • modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_form_submission.rb (+13/-1)

  • modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb (+60/-9)

  • modules/accredited_representative_portal/app/models/accredited_representative_portal/user_account_accredited_individual.rb (+1/-1)

  • modules/accredited_representative_portal/app/serializers/accredited_representative_portal/power_of_attorney_request_serializer.rb (+8/-5)

  • modules/accredited_representative_portal/app/serializers/accredited_representative_portal/power_of_attorney_request_serializer/individual_power_of_attorney_holder_serializer.rb (+0/-17)

  • modules/accredited_representative_portal/lib/tasks/seed.rake (+0/-0)

  • modules/accredited_representative_portal/spec/factories/power_of_attorney_request.rb (+16/-0)

  • modules/accredited_representative_portal/spec/lib/tasks/staging_seed_spec.rb (+2/-2)

  • modules/accredited_representative_portal/spec/models/accredited_representative_portal/representative_user_account_spec.rb (+1/-1)

  • modules/accredited_representative_portal/spec/models/accredited_representative_portal/user_account_accredited_individual_spec.rb (+1/-1)

  • modules/accredited_representative_portal/spec/requests/accredited_representative_portal/v0/power_of_attorney_requests_spec.rb (+46/-3)

    Note: We exclude files matching the following when considering PR size:

    *.csv, *.json, *.tsv, *.txt, *.md, Gemfile.lock, app/swagger, modules/mobile/docs, spec/fixtures/, spec/support/vcr_cassettes/, modules/mobile/spec/support/vcr_cassettes/, db/seeds, modules/vaos/app/docs, modules/meb_api/app/docs, modules/appeals_api/app/swagger/, *.bru, *.pdf

Big PRs are difficult to review, often become stale, and cause delays.

Generated by 🚫 Danger

@nihil2501 nihil2501 mentioned this pull request Mar 7, 2025
Closed
9 tasks
@nihil2501 nihil2501 changed the title Convert poa request status scopes/predicates [ARP POA Submission] (#5) Update POA Form Submission Scopes (#101919) Mar 7, 2025
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/update-poa-form-submission-scopes.oren/main/main March 7, 2025 15:33 Inactive
@rjohnson2011
Copy link
Contributor

Can you resolve the linting errors please?

Base automatically changed from art/101919/form-submission-check-job to master March 7, 2025 20:07
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/update-poa-form-submission-scopes.oren/main/main March 7, 2025 20:08 Inactive
@pixiitech @nihil2501
Convert poa request status scopes/predicates
464b3aa 
This is so that these internal methods match status params in
the index endpoint.

- unprocessed -> pending
- resolved -> processed

They have also been modified to count requests with a PoA form
submission in an unsuccessful state as pending.
@nihil2501 nihil2501 force-pushed the art/101919/update-poa-form-submission-scopes.oren branch from 533709f to 464b3aa Compare March 7, 2025 21:07
@nihil2501 nihil2501 requested review from chumpy and ojbucao March 7, 2025 21:08
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/update-poa-form-submission-scopes.oren/main/main March 7, 2025 21:08 Inactive
ojbucao
ojbucao previously approved these changes Mar 7, 2025
View reviewed changes
chumpy
chumpy previously approved these changes Mar 7, 2025
View reviewed changes
@nihil2501 nihil2501 marked this pull request as ready for review March 7, 2025 21:20
@nihil2501 nihil2501 requested review from a team as code owners March 7, 2025 21:20
rjohnson2011
rjohnson2011 reviewed Mar 7, 2025
View reviewed changes
Copy link
Contributor

@rjohnson2011 rjohnson2011 left a comment

Choose a reason for hiding this comment

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

Can you either address why the comment blocks are necessary or remove them please?

...presentative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb Outdated
Comment on lines 146 to 151
# `processed` and `not_processed` are the logical negation of one another,
# but this isn't enforced structurally in the code. An application of De
# Morgan's law is evident here. `invert_where` is a possibility to pull
# this off too, but it's not so usable because it inverts conditions that
# were chained prior.
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the comment blocks in the file? That is not something we generally commit into the repo and the parts like An application of De Morgan's law is evident here. are unclear.

...presentative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb Outdated
Comment on lines 110 to 116
##
# This `concerning` block puts up some flashing lights around this
# complexity. It potentially wants to coexist directly with other model
# functionality or at a higher business logic layer, but extra care might be
# needed to pull that off without making a mess. This block is especially
# narrow--it only defines two scopes and no instance methods for example.
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary?

...presentative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb Outdated
Comment on lines 118 to 122
##
# The 3x `LEFT OUTER JOIN`s with very particular join conditions make
# expressing both of the `processed` and `not_processed` relations easy
# to express with very simple `WHERE` conditions.
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question

...presentative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb Outdated
Comment on lines 154 to 157
##
# Must be resolved, and either the resolution is not an acceptance, or if
# it is, there must be a form submission that succeeded.
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too

...presentative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb Outdated
Comment on lines 169 to 172
##
# Must be unresolved, or the resolution is an acceptance and there also
# must not be a form submission that succeeded.
#
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

@nihil2501 nihil2501 dismissed stale reviews from chumpy and ojbucao via f641d0a March 7, 2025 21:58
@rjohnson2011 rjohnson2011 self-requested a review March 7, 2025 21:59
rjohnson2011
rjohnson2011 previously approved these changes Mar 7, 2025
View reviewed changes
Copy link
Contributor

@rjohnson2011 rjohnson2011 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing those!

@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/update-poa-form-submission-scopes.oren/main/main March 7, 2025 21:59 Inactive
@nihil2501 nihil2501 force-pushed the art/101919/update-poa-form-submission-scopes.oren branch from f641d0a to 2d34edc Compare March 7, 2025 22:00
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/update-poa-form-submission-scopes.oren/main/main March 7, 2025 22:08 Inactive
@nihil2501 nihil2501 requested a review from rjohnson2011 March 7, 2025 22:45
@nihil2501 nihil2501 merged commit 41e772b into master Mar 7, 2025
27 of 29 checks passed
@nihil2501 nihil2501 deleted the art/101919/update-poa-form-submission-scopes.oren branch March 7, 2025 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chumpy chumpy chumpy approved these changes

@ojbucao ojbucao ojbucao approved these changes

@rjohnson2011 rjohnson2011 rjohnson2011 approved these changes

Assignees
No one assigned
Labels
benefits-accredited-rep-facing require-backend-approval test-passing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nihil2501 @rjohnson2011 @chumpy @ojbucao @va-vfs-bot @pixiitech