[ARP POA Submission] (#3) Decision Creation Controller Action (#101919)

[pixiitech]

Note: Delete the description statements, complete each step. None are optional, but can be justified as to why they cannot be completed as written. Provide known gaps to testing that may raise the risk of merging to production.

Summary

• This work is behind a feature toggle (flipper): YES/NO
• (Summarize the changes that have been made to the platform)
• (If bug, how to reproduce)
• (What is the solution, why is this the solution?)
• (Which team do you work for, does your team own the maintenance of this component?)
• (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
• ⭐ Lighthouse POA API Integration va.gov-team#101919
• 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
• Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing
• 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)

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?

[github-actions]

1 Warning

⚠️

This PR changes 274 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 config/settings/test.yml (+3/-0) modules/accredited_representative_portal/app/controllers/accredited_representative_portal/v0/power_of_attorney_request_decisions_controller.rb (+10/-16) modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_form_submission.rb (+9/-0) modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb (+10/-0) modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_request_decision.rb (+15/-0) modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_request_resolution.rb (+8/-0) modules/accredited_representative_portal/app/services/accredited_representative_portal/power_of_attorney_request_service/accept.rb (+116/-0) modules/accredited_representative_portal/config/routes.rb (+1/-1) modules/accredited_representative_portal/spec/requests/accredited_representative_portal/v0/power_of_attorney_request_decisions_spec.rb (+61/-24) 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]

Correctness and error handling pivot.

The PRs and their sizes have been very approachable.

[nihil2501]

Ah yes, this is what I'll do for these from now on. Have done values.zip(values).to_h before.

[nihil2501]

Snuck in?

[nihil2501]

Let's revert to letting our model validates declarations carry out this error logic. But we can then map them to errors that are contextual to our Accept service within that service (just reiterating the comment that describes the new strategy).

[nihil2501]

FYI, looks like service raises in the enqueue-failed case so we would never have hit this code.

[nihil2501]

We shouldn't make a second form submission. Rather we should update the one form submission as failing to enqueue.

[pixiitech]

Sorry I left this here from before the API call had to be moved out of the transaction.

[nihil2501]

I think we could encapsulate all 3 of the methods here.

The line of thinking that led me here (and I think there are others) was that attributes isn't a good name, so then I thought of service_payload for accuracy against the current code structure, but then the redundancy of service_ is potentially a smell (I really wouldn't go as far as calling it that though).

I don't think we necessarily need a class for it, but maybe a single entry function that might call helper functions with repetitively prefixed names (e.g. submit and submit_payload), just some thoughts.

The name attributes really does stick out though.

[nihil2501]

This one is a nitpick that doesn't need to be changed—

About naming, I tend to avoid abbreviations unless it gets really dire (poa_request seems like a good convention for local vars for example). Here organization_data comes to mind.

[nihil2501]

PowerOfAttorneyForm#parsed_data is already this method implementation and it is memoized so you can call it repeatedly too.

[nihil2501]

If this sticks around I would actually make it a constant. Fine to do a calculation of another constant like that (and freeze it potentially): TRANSIENT_ERROR_TYPES.

[nihil2501]

Possible that VCR comments on PR stack PR preceding this one will lead to changes here. I'll wait until later to review more closely.

[nihil2501]

• Streamlined service control flow
• Consistent transient error handling
• Resolution & resolving creation encapsulation
• Note about service vs controller separation of concerns

[nihil2501]

Let's finally encapsulate and hide this implementation for co-creating the resolution & resolving:

• Creating the two records in the correct sequence
• Invoking AR's validations in a way that is useful to us
• Wrapping them in a transaction

module AccreditedRepresentativePortal
  class PowerOfAttorneyRequestDecision < ApplicationRecord
    class << self
      def create_acceptance!(**attrs)
        create_with_resolution!(type: Types::ACCEPTANCE, **attrs)
      end

      def create_declination!(**attrs)
        create_with_resolution!(type: Types::DECLINATION, **attrs)
      end

      private

      def create_with_resolution!(creator:, type:, **resolution_attrs)
        PowerOfAttorneyRequestResolution.create_with_resolving!(
          resolving: new(type:, creator:),
          **resolution_attrs
        )
      end
    end
  end
end

module AccreditedRepresentativePortal
  class PowerOfAttorneyRequestResolution < ApplicationRecord
    class << self
      ##
      # Adding this public class method in addition to `create!` because this
      # implementation causes the uniqueness validation to be expressed.
      #
      def create_with_resolving!(resolving:, **attrs)
        transaction do
          resolving.create!
          create!(resolving:, **attrs)
        end
      end
    end
  end
end

[nihil2501]

Also note the use of keyword args to mimic AR model interface.

[nihil2501]

Only downside I see for exposing #accept! and #decline on PowerOfAttorneyRequest is that these verbs could potentially lead someone to believe that this performs all the business logic that it needs to, but it doesn't. We lifted these verbs from the model layer to the service layer where it would be appropriate to handle all the business logic.

On this point I am mostly indifferent though.

[pixiitech]

are mark_accepted! and mark_declined! more descriptive?

[nihil2501]

@pixiitech I think that's a good idea. I've seen mark_* plenty of times.

This makes me think of what methods ActiveRecord::Enum exposes e.g. accepted! and tempts to do that...

Up to you!

[nihil2501]

Unexpected errors are transient and should be handled accordingly (blow up and roll back the resolution & resolving).

[nihil2501]

A form submission wouldn't have been created if we're in this rescue. This use of memoization is creating and then instantly deleting this record. Can we create a more streamlined flow? I think using local variables is potentially the exact control flow we want, while memoized instance variables is a complicating departure from a direct control flow.

[nihil2501]

We should have straightforward control flow that is based around two operations:

• Make a failed submission on a failure
• Make a successful submission on a success

[nihil2501]

I think we can get away with this for now, but I suspect the correct layered design is as follows:

Service surfaces a variety of kinds of errors that express what went wrong semantically. The service understands the internals of what went wrong but is happy to obfuscate those internal details that are too much information for the service's caller to need to know about.

This controller is a caller of the service. It should have the responsibility of interpreting the variety of kinds of (semantic) errors that the service exposes and then it decides what response should be returned from the controller.

For now:
If the responses returned from the controller look principled from the outside world, but the design doesn't follow above, that is fine; we can punt on such a refactoring until later.

[nihil2501]

LGTM