[16] ART: Filter PoA results list

[pixiitech]

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?

[nihil2501]

Final tweak

[nihil2501]

Let's represent this enum with a module defined in this controller:

module Statuses
  ALL = [
    PENDING = 'pending',
    COMPLETED = 'completed'
  ].freeze
end

And then represent exhaustively matching against it with a case statement:

rel = poa_request_scope
status = params[:status].presence

rel =
  case status
  when Statuses::PENDING
    rel.unresolved
  when Statuses::COMPLETED
    rel.resolved
  when NilClass
    rel
  else
    # Throw 400 for unexpected, non-blank statuses
    raise ActionController::BadRequest.new(<<~MSG.squish)
      Invalid status parameter.
      Must be one of (#{Statuses::ALL.join(', ')})
    MSG
  end

Note:

• Use of ActionController::BadRequest for now, which mirrors how we're using other native Rails controller exceptions for now in our semi-shared exception handling
• Even stricter about inputs (no downcase or other normalization), since we shouldn't present clients a loose API contract

[pixiitech]

These were measures I added to prevent extra frustration if the frontend accidentally sent a lowercase version but that's ok. I still think we should ignore an empty status string (this rewrite will throw a 400) Nevermind I see the use of presence

[nihil2501]

ActionController::BadRequest

[nihil2501]

This would need to handle ActionController::BadRequest now since we switched to raising that.

[nihil2501]

A couple additional things.

[nihil2501]

should be :bad_request after we reflect the changed exception type in our exception rendering.

[nihil2501]

We shouldn't need these. I think we make our test suite run with transaction-oriented database cleaning on a per-spec-example basis and I think the let! is defeating that idea. Kind of just guessing, but I tried changing to let and causing the creation at the top of each spec example and I didn't need the delete_all's. There are very few delete_all's in the spec suite. For now, let's use let and cause the creation at the top of each example.

[nihil2501]

LGTM

The base branch was changed.

[stevenjcumming]

I mentioned this in another PR https://github.com/department-of-veterans-affairs/vets-api/pull/20321/files

Please remove the Statuses module and use the constants directly

 PENDING = 'pending'.freeze
 COMPLETED = 'completed'.freeze
 ALL = [PENDING, COMPLETED].freeze

[pixiitech]

I mentioned this in another PR https://github.com/department-of-veterans-affairs/vets-api/pull/20321/files

Please remove the Statuses module and use the constants directly

 PENDING = 'pending'.freeze
 COMPLETED = 'completed'.freeze
 ALL = [PENDING, COMPLETED].freeze

Linter is making me remove the .freeze on the first two lines

Rebased with master and pushed

[stevenjcumming]

I mentioned this in another PR https://github.com/department-of-veterans-affairs/vets-api/pull/20321/files
Please remove the Statuses module and use the constants directly

 PENDING = 'pending'.freeze
 COMPLETED = 'completed'.freeze
 ALL = [PENDING, COMPLETED].freeze

Linter is making me remove the .freeze on the first two lines

Rebased with master and pushed

Sorry, old habit die hard.. no need to freeze

[nihil2501]

@stevenjcumming @pixiitech Is it a requirement to remove the namespacing of our related constants? This is a pattern I have used and seen in many many places and indeed would encourage elsewhere within our module. This seems like more of a stylistic suggestion.

Without the scoping I'd at least ask for the variable name to now reflect the repeated constant name prefix STATUS_ or something, since otherwise the name ALL is not a sufficiently explanatory constant name.

TL;DR: can we merge this with the original namespacing?

[stevenjcumming]

@stevenjcumming @pixiitech Is it a requirement to remove the namespacing of our related constants? This is a pattern I have used and seen in many many places and indeed would encourage elsewhere within our module. This seems like more of a stylistic suggestion.

Without the scoping I'd at least ask for the variable name to now reflect the repeated constant name prefix STATUS_ or something, since otherwise the name ALL is not a sufficiently explanatory constant name.

TL;DR: can we merge this with the original namespacing?

@nihil2501 It is stylistic. I'm not a fan of it, unless it's used in outside the class (no issue with that).

Prefixes are fine, you can call is STATUSES = [], or use a hash, etc. I meant to just illustrate my intent not specifically suggest that, apologies for the confusion.

can we merge this with the original namespacing?
=> I really don't like it, but I know you have a deadline so I'll approve it if you feel strongly about it

[pixiitech]

@stevenjcumming @pixiitech Is it a requirement to remove the namespacing of our related constants? This is a pattern I have used and seen in many many places and indeed would encourage elsewhere within our module. This seems like more of a stylistic suggestion.
Without the scoping I'd at least ask for the variable name to now reflect the repeated constant name prefix STATUS_ or something, since otherwise the name ALL is not a sufficiently explanatory constant name.
TL;DR: can we merge this with the original namespacing?

@nihil2501 It is stylistic. I'm not a fan of it, unless it's used in outside the class (no issue with that).

Prefixes are fine, you can call is STATUSES = [], or use a hash, etc. I meant to just illustrate my intent not specifically suggest that, apologies for the confusion.

can we merge this with the original namespacing? => I really don't like it, but I know you have a deadline so I'll approve it if you feel strongly about it

dropped last commit and rebased

[nihil2501]

@stevenjcumming I think I see the perspective about internal versus external. Of the other options I'd probably prefer:

STATUSES = {
  PENDING: 'pending',
  COMPLETED: 'completed'
}

STATUSES.values # to get all of them

In any event, this code is going to be modified more and more heavily to an encapsulated module for doing all the things that one needs to do for searching: defaulting, query building, invalidity, etc. So here I just wanted to suggest by example that that kind of encapsulation is forthcoming.