Filter PoA index endpoint for pending/completed (#98714) #20081

pixiitech · 2025-01-02T20:29:12Z

Summary

This work is behind a feature toggle (flipper): YES
Description Allow sending of status=Pending or status=Completed param to the power_of_attorney_requests endpoint to filter the list
(If bug, how to reproduce) feature
(What is the solution, why is this the solution?) Forward And Filter By Status In POA Request List Endpoint va.gov-team#98714
(Which team do you work for, does your team own the maintenance of this component?) ART, yes
(If introducing a flipper, what is the success criteria being targeted?) ??

Related issue(s)

Link to ticket created in va.gov-team repo OR screenshot of Jira ticket if your team uses Jira
Forward And Filter By Status In POA Request List Endpoint va.gov-team#98714
Link to previous change of the code/bug (if applicable)
Link to epic if not included in ticket

Testing done

New code is covered by unit tests
Describe what the old behavior was prior to the change
power_of_attorney_requests endpoint would return a list of all PoA request without filtering capability
Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing
Run rails accredited_representative_portal:seed in development
accessing the power_of_attorney_requests endpoint with a logged in user with the ?status=Pending should yield 12 PoA requests
accessing the power_of_attorney_requests endpoint with a logged in user with the ?status=Completed should yield 111 PoA requests
If this work is behind a flipper:
- Tests need to be written for both the flipper on and flipper off scenarios. Docs.
- What is the testing plan for rolling out the feature?
- ?

Screenshots

Note: Optional

What areas of the site does it impact?

(Describe what parts of the site are impacted andifcode touched other areas)
Accredited Representative Portal

Acceptance criteria

I fixed|updated|added unit tests and integration tests for each feature (if applicable).
No error nor warning in the console.
Events are being sent to the appropriate logging solution
Documentation has been updated (link to documentation)
No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
Feature/bug has a monitor built into Datadog (if applicable)
If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
I added a screenshot of the developed feature

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

2 Removed obsolete columns: city_bidx, state_bidx, zipcode_bidx 3 Added new columns: claimant_city_ciphertext, claimant_city_bidx, claimant_state_code_ciphertext, claimant_state_code_bidx, claimant_zip_code_ciphertext, claimant_zip_code_bidx 4 Added a new index for claimant_city_bidx, claimant_state_code_bidx, and claimant_zip_code_bidx 5 Added claimant_type column to ar_power_of_attorney_requests

…sion line

ojbucao · 2025-01-06T17:41:02Z

...presentative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb

@@ -19,6 +19,9 @@ module ClaimantTypes
            class_name: 'PowerOfAttorneyRequestResolution',
            inverse_of: :power_of_attorney_request

+    has_many :resolutions,


why are we adding this?

So that the left_joins and joins I used in the scopes below work properly with ActiveRecord

Ok I found a better way. Thanks

nihil2501

Looks good, but I mention some things to solidify the code.

nihil2501 · 2025-01-09T20:29:15Z

...presentative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb

+    scope :pending, -> { left_joins(:resolution).where(resolution: { resolving_type: nil }) }
+    scope :completed, -> { joins(:resolution) }


These scopes will be a useful starting point. I suggest that we change the names to resolved and unresolved instead. In the future, product has the idea that our "Pending" tab should include resolved POA requests for which the effect of the resolution is still processing. Concretely, an accepted POA request will undergo a background process with an upstream Lighthouse API that modifies another database in order to "establish" the POA relationship, thereby granting the representative access to Veteran-centric operations in other software (called VBMS–Veteran Benefits Management System).

Our API can still expose the "Pending" and "Completed" concepts, but the implementation will notably match up to different concepts–"resolved" and "unresolved", and we will thereby be very clear on how the implementation will have to change in work that follows on shortly after.

nihil2501 · 2025-01-09T20:29:33Z

...presentative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb

@@ -31,6 +31,9 @@ module ClaimantTypes

    delegate :poa_code, to: :accredited_individual

+    scope :pending, -> { left_joins(:resolution).where(resolution: { resolving_type: nil }) }


id: nil seems less mysterious.

the linter suggested where.missing(:resolution) ... looks like this is new for 7.1

the linter suggested where.missing(:resolution) ... looks like this is new for 7.1

Oh that's great.

nihil2501 · 2025-01-09T20:48:43Z

...app/controllers/accredited_representative_portal/v0/power_of_attorney_requests_controller.rb

+        return poa_request_scope if params[:status].blank?
+        return poa_request_scope.none unless %w[pending completed].include?(params[:status]&.downcase)
+
+        poa_request_scope.send(params[:status].downcase)


We shouldn't couple user input to the code we call like this. Logically equivalent to what we want, but we should choose a more appropriate expression of that logic.

nihil2501 · 2025-01-09T21:05:41Z

...app/controllers/accredited_representative_portal/v0/power_of_attorney_requests_controller.rb

+        return poa_request_scope if params[:status].blank?
+        return poa_request_scope.none unless %w[pending completed].include?(params[:status]&.downcase)


Let's encode that our API will be kind to 3 values: (absent, pending, and completed), and throw a 400 otherwise. Being really strict with our client will let us keep things tight as we develop. I think we should put this exception rendering behavior as a rescue_from in our somewhat-shared PowerOfAttorneyRequests controller concern, which is where we are currently collecting that behavior so we can make a more principled choice later.

Let's also map from this user input to how we update our relation explicitly. I think the explicit matching should be a case statement that builds on the relation we're building: so we'd start building our relation as rel = poa_request_scope at top of the controller action, then the case branches will have rel = rel.resolved or rel = rel.unresolved or raise BadRequest with some message that the rescue_from could make use of.

And by absent I don't mean the string "absent", I mean the absence of any value.

nihil2501 · 2025-01-09T21:08:13Z

...app/controllers/accredited_representative_portal/v0/power_of_attorney_requests_controller.rb

-        poa_requests = poa_request_scope.includes(includes).limit(100)
+        poa_requests = filtered_poa_requests.includes(includes).limit(100)


Let's inline everything into the controller action rather than have a function extracted. This let's us wait until there is a compelling motivation for indirection and so we don't need to first unwind an existing indirection to achieve another.

ojbucao and others added 27 commits December 30, 2024 17:07

(fix) rerun migrations and update db/schema.rb

672fcff

[ART] Serialize POA form data with POA request

ec98b74

[ART] lint fixes

56ff35f

[ART] Skip spec for POA request representatives migration

a156ad7

[ART] POA request representative migrations

7102a01

[ART] Re-sequence POA request representative migration

4adfce0

[ART] POA request representative

7cf18dc

[ART] lint fixes

1d8e120

[ART] POA request representative refinement

69746b0

[ART] lint fixes

fa23fd0

[ART] POA request resolution validates unique POA request

07f8310

[ART] lint fixes

bb50832

[ART] POA request seed task for dev

4f6e78f

[ART] POA request seed task - refactor, optimize, rm extra validations

6bf3113

[ART] lint and spec fixes

1fb54a4

art/poa-requests/part-8/decisions-endpoint

780ab0c

art/poa-requests/part-8/decisions-endpoint

ce2460b

art/poa-requests/part-8/decisions-endpoint - fixed type

bf412d3

art/poa-requests/part-8/decisions-endpoint - fixed current user

ba79cf3

art/poa-requests/part-8/decisions-endpoint - removed unnecessary deci…

d5568de

…sion line

art/poa-requests/part-8/decisions-endpoint - added common verbiage

fef724d

art/poa-requests/part-8/decisions-endpoint - added uniqueness constraint

c21c469

[ART] lint fixes

733ad0f

[ART] refactor POA request decisions controller

aeebc32

Add Pundit scopes and verification (#98712)

4a3a0b9

linter

ff61d06

ojbucao self-requested a review January 6, 2025 17:39

ojbucao reviewed Jan 6, 2025

View reviewed changes

Prevent DoubleRenderError, add test for ignoring pundit auth

84e0697

nihil2501 and others added 5 commits January 7, 2025 09:30

[ART] Change pundit authorization, only 403 when "unaccredited"

c82e267

[ART] Lint fixes

af562ff

[ART] Refactor POA holder serializer

80b8873

Filter PoA index endpoint for pending/completed (#98714)

7541a8c

Changes per review

7cb8c4e

pixiitech force-pushed the art/98714/filter-poa-list branch from fb7a44b to 7cb8c4e Compare January 9, 2025 20:12

pixiitech changed the base branch from art/poa-requests/decisions to art/serializers/tweak January 9, 2025 20:12

nihil2501 requested changes Jan 9, 2025

View reviewed changes

nihil2501 mentioned this pull request Jan 10, 2025

15: 96890-POA-Request-Search-API #20119

Closed

ojbucao force-pushed the art/serializers/tweak branch 4 times, most recently from fbe92d7 to 43cacea Compare January 16, 2025 21:38

ojbucao deleted the branch department-of-veterans-affairs:art/serializers/tweak January 16, 2025 22:21

ojbucao closed this Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter PoA index endpoint for pending/completed (#98714) #20081

Filter PoA index endpoint for pending/completed (#98714) #20081

pixiitech commented Jan 2, 2025

ojbucao Jan 6, 2025

pixiitech Jan 6, 2025

pixiitech Jan 9, 2025

nihil2501 left a comment

nihil2501 Jan 9, 2025

nihil2501 Jan 9, 2025

pixiitech Jan 13, 2025

nihil2501 Jan 13, 2025

nihil2501 Jan 9, 2025

nihil2501 Jan 9, 2025

nihil2501 Jan 9, 2025

nihil2501 Jan 9, 2025

		scope :pending, -> { left_joins(:resolution).where(resolution: { resolving_type: nil }) }
		scope :completed, -> { joins(:resolution) }

		@@ -31,6 +31,9 @@ module ClaimantTypes

		delegate :poa_code, to: :accredited_individual

		scope :pending, -> { left_joins(:resolution).where(resolution: { resolving_type: nil }) }

		return poa_request_scope if params[:status].blank?
		return poa_request_scope.none unless %w[pending completed].include?(params[:status]&.downcase)

		poa_requests = poa_request_scope.includes(includes).limit(100)
		poa_requests = filtered_poa_requests.includes(includes).limit(100)

Filter PoA index endpoint for pending/completed (#98714) #20081

Filter PoA index endpoint for pending/completed (#98714) #20081

Conversation

pixiitech commented Jan 2, 2025

Summary

Related issue(s)

Testing done

Screenshots

What areas of the site does it impact?

Acceptance criteria

Requested Feedback

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nihil2501 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment