Issue #97853 Add Power of Attorney Request Models, Factories, and Specs #19919

ojbucao · 2024-12-16T22:14:28Z

This PR introduces the core components for the Power of Attorney Request feature, ensuring a complete and testable implementation. Each model is accompanied by its corresponding factory for test data generation and RSpec tests for verification.

Models
• PowerOfAttorneyRequest
• PowerOfAttorneyForm
• PowerOfAttorneyRequestDecision
• PowerOfAttorneyRequestExpiration
• PowerOfAttorneyRequestResolution
Factories
• Added factories for generating test data for the above models.
RSpec Tests
• Added unit tests to validate model behaviors and associations.

github-actions · 2024-12-16T23:13:42Z

	1 Warning
⚠️	This PR changes `287` 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/accredited_representative_portal.gemspec (+1/-0) modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_form.rb (+12/-0) modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_request.rb (+11/-0) modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_request_decision.rb (+8/-0) modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_request_expiration.rb (+5/-0) modules/accredited_representative_portal/app/models/accredited_representative_portal/power_of_attorney_request_resolution.rb (+20/-0) modules/accredited_representative_portal/lib/accredited_representative_portal/engine.rb (+5/-0) modules/accredited_representative_portal/spec/factories/power_of_attorney_decision.rb (+8/-0) modules/accredited_representative_portal/spec/factories/power_of_attorney_expiration.rb (+6/-0) modules/accredited_representative_portal/spec/factories/power_of_attorney_form.rb (+10/-0) modules/accredited_representative_portal/spec/factories/power_of_attorney_request.rb (+7/-0) modules/accredited_representative_portal/spec/factories/power_of_attorney_request_resolution.rb (+37/-0) modules/accredited_representative_portal/spec/models/accredited_representative_portal/power_of_attorney_form_spec.rb (+12/-0) modules/accredited_representative_portal/spec/models/accredited_representative_portal/power_of_attorney_request_decision_spec.rb (+7/-0) modules/accredited_representative_portal/spec/models/accredited_representative_portal/power_of_attorney_request_expiration_spec.rb (+12/-0) modules/accredited_representative_portal/spec/models/accredited_representative_portal/power_of_attorney_request_resolution_spec.rb (+118/-0) modules/accredited_representative_portal/spec/models/accredited_representative_portal/power_of_attorney_request_spec.rb (+8/-0) 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 · 2024-12-17T04:09:04Z

@ojbucao Let's use "Pull Request Stacks" so that these are easy to review. If you're building off of the work of a previous branch branch-1, it makes sense to branch off of that branch branch-2 and then open a PR branch-1 <- branch-2.

nihil2501

we should question use of validates
i think we should consider subbing in DB defined cascading (unfortunately not exposed in add_reference but is exposed in add_foreign_key; sigh) and avoid doing meaningful work in model lifecycle callbacks (sorry, Rails made a mess in these areas)
please get in the habit of making PR stacks. these are hard to review otherwise. GH treats PR stacks conveniently, when stacked PR's base branch merges to main, dependent PR updates its base branch
power_of_attorney_form

nihil2501 · 2024-12-17T04:09:53Z

..._representative_portal/app/models/accredited_representative_portal/power_of_attorney_form.rb

+    validates :city_bidx, presence: true, length: { is: 44 }
+    validates :state_bidx, presence: true, length: { is: 44 }
+    validates :zipcode_bidx, presence: true, length: { is: 44 }


why length?

It's the length of the cryptographic hash that results.

nihil2501 · 2024-12-17T04:14:59Z

..._representative_portal/app/models/accredited_representative_portal/power_of_attorney_form.rb

+    blind_index :zipcode
+
+    # Validations
+    validates :power_of_attorney_request_id, uniqueness: true


See discussion on race conditions when checking uniqueness from ActiveRecord. Since this uniqueness constraint is not something a user can cause by their input, I don't think there is added benefit to implementing this with validations (which are convenient because they can collect issues for feedback for the user, but that's not our situation). Also, the PG adapter surfaces the RecordNotUnique exception, so we'll know what's up.

nihil2501 · 2024-12-17T04:16:28Z

..._representative_portal/app/models/accredited_representative_portal/power_of_attorney_form.rb

+    validates :data_ciphertext, presence: true
+    validates :city_bidx, presence: true, length: { is: 44 }
+    validates :state_bidx, presence: true, length: { is: 44 }
+    validates :zipcode_bidx, presence: true, length: { is: 44 }


None of these are really controlled by user input and there's nowhere they ought to be shown back to the user. If these aren't valid it's a programming bug we caused and these have not null constraints in the DB.

Makes sense. I'll go ahead and remove them.

nihil2501 · 2024-12-17T04:16:50Z

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

+    belongs_to :claimant,
+               class_name: 'UserAccount'
+
+    has_one :form,


power_of_attorney_form

nihil2501 · 2024-12-17T04:16:57Z

..._representative_portal/app/models/accredited_representative_portal/power_of_attorney_form.rb

+  class PowerOfAttorneyForm < ApplicationRecord
+    belongs_to :power_of_attorney_request,
+               class_name: 'AccreditedRepresentativePortal::PowerOfAttorneyRequest',
+               inverse_of: :form


power_of_attorney_form

There is a meaningful semantic difference between poa form and poa request form. The latter doesn't make sense. A poa request will result in a poa form being made. It's marginal but I think leans this way a little.

nihil2501 · 2024-12-17T04:37:32Z

...ive_portal/app/models/accredited_representative_portal/power_of_attorney_request_decision.rb

+    belongs_to :creator,
+               class_name: 'UserAccount'
+
+    has_one :power_of_attorney_request_resolution,


Follow delegated type pattern with mixin. Good example in my sketch PR.

nihil2501 · 2024-12-17T04:43:58Z

...ive_portal/app/models/accredited_representative_portal/power_of_attorney_request_decision.rb

+
+    has_one :power_of_attorney_request_resolution,
+            as: :resolving,
+            inverse_of: :resolving,


didn't look close but does inverse_of work in the polymorphic case? i'll look closer upon refinement of this PR if necessary.

you're right. my bad. removing it.

nihil2501 · 2024-12-17T04:44:07Z

...e_portal/app/models/accredited_representative_portal/power_of_attorney_request_expiration.rb

+  class PowerOfAttorneyRequestExpiration < ApplicationRecord
+    has_one :power_of_attorney_request_resolution,
+            as: :resolving,
+            inverse_of: :resolving,


didn't look close but does inverse_of work in the polymorphic case? i'll look closer upon refinement of this PR if necessary.

nihil2501 · 2024-12-17T04:45:03Z

...e_portal/app/models/accredited_representative_portal/power_of_attorney_request_expiration.rb

+    # Validations
+    validates :id, presence: true


validates :id, presence: true is something i've never seen before. I think it shouldn't be there.

...e_portal/app/models/accredited_representative_portal/power_of_attorney_request_resolution.rb

nihil2501 · 2024-12-17T04:52:12Z

@ojbucao I didn't review the fixtures and model specs really yet.

nihil2501

small changes, then this is ready to go:

resolving not optional
- if there were issues here, maybe this way of creating both objects simultaneously is what we should have?
i don't think we need any of the validates
don't need dependent: :destroy, we can do cascading deletes in the db

but let me know if you think i'm mistaken somewhere

nihil2501 · 2024-12-18T06:21:12Z

...e_portal/app/models/accredited_representative_portal/power_of_attorney_request_resolution.rb

+      'AccreditedRepresentativePortal::PowerOfAttorneyRequestDecision'
+    ].freeze
+
+    delegated_type :resolving, types: RESOLVING_TYPES, dependent: :destroy, optional: true


not optional.

in a follow up commit we can implement all the foreign key cascades and in this case maybe a trigger. let's remove AR's handling of this with dependent

nihil2501 · 2024-12-18T06:52:56Z

...e_portal/app/models/accredited_representative_portal/power_of_attorney_request_resolution.rb

+        has_one(
+          :power_of_attorney_request_resolution,
+          as: :resolving,
+          touch: true


looked up the touch option and it's for updated timestamps which the sub types don't have so we can remove this

nihil2501 · 2024-12-18T06:53:39Z

...e_portal/app/models/accredited_representative_portal/power_of_attorney_request_resolution.rb

+    # Validations
+    validates :resolving_type, presence: true, inclusion: { in: RESOLVING_TYPES, allow_nil: true }
+    validates :resolving_id, presence: true, if: -> { resolving_type.present? }


won't need this since association isn't optional

…POA requests - Override `table_name_prefix` to 'ar_' for models in the Accredited Representative Portal - Add `activerecord` as a development dependency to fix spec issues This ensures Power of Attorney request models in the engine use the correct table prefix and that all specs pass successfully.

nihil2501

LGTM

…cs (#19919) * (feat) Add PowerOfAttorneyRequest model, factory, and specs * (feat) Add PowerOfAttorneyRequestDecision model, factory, and specs * (feat) Add PowerOfAttorneyRequestExpiration model, factory, and specs * (feat) Add PowerOfAttorneyRequestResolution model, factory, and specs * (feat) Add PowerOfAttorneyForm model, factory, and specs * (fix) Rename directory due to misspelling * (fix) Add table name prefix override and ActiveRecord dependency for POA requests - (feat) Override `table_name_prefix` to 'ar_' for models in the Accredited Representative Portal - (feat) Add `activerecord` as a development dependency to fix spec issues This ensures Power of Attorney request models in the engine use the correct table prefix and that all specs pass successfully. * (fix) Apply requested PR changes; all specs passing

ojbucao requested review from nihil2501 and chumpy December 16, 2024 22:14

va-vfs-bot temporarily deployed to art/97853/mvp-poa-reqs-models/main/main December 16, 2024 22:35 Inactive

va-vfs-bot temporarily deployed to art/97853/mvp-poa-reqs-models/main/main December 16, 2024 23:18 Inactive

github-actions bot added the test-failure label Dec 16, 2024

ojbucao added the benefits-accredited-rep-facing label Dec 17, 2024

nihil2501 changed the base branch from master to art/96235/mvp-poa-reqs-migrations December 17, 2024 04:05

va-vfs-bot temporarily deployed to art/97853/mvp-poa-reqs-models/main/main December 17, 2024 04:06 Inactive

nihil2501 changed the base branch from art/96235/mvp-poa-reqs-migrations to master December 17, 2024 04:07

va-vfs-bot temporarily deployed to art/97853/mvp-poa-reqs-models/main/main December 17, 2024 04:15 Inactive

nihil2501 requested changes Dec 17, 2024

View reviewed changes

ojbucao force-pushed the art/97853/mvp-poa-reqs-models branch from 3448bd9 to 1c3df00 Compare December 17, 2024 20:21

github-actions bot added test-passing and removed test-failure labels Dec 17, 2024

va-vfs-bot temporarily deployed to art/97853/mvp-poa-reqs-models/main/main December 17, 2024 20:43 Inactive

ojbucao force-pushed the art/97853/mvp-poa-reqs-models branch from 1c3df00 to 6024452 Compare December 17, 2024 20:46

github-actions bot added test-failure and removed test-passing labels Dec 17, 2024

va-vfs-bot temporarily deployed to art/97853/mvp-poa-reqs-models/main/main December 17, 2024 21:02 Inactive

va-vfs-bot temporarily deployed to art/97853/mvp-poa-reqs-models/main/main December 17, 2024 23:28 Inactive

github-actions bot added test-passing and removed test-failure labels Dec 17, 2024

ojbucao force-pushed the art/97853/mvp-poa-reqs-models branch from 7ec1183 to af04c9a Compare December 18, 2024 00:03

va-vfs-bot temporarily deployed to art/97853/mvp-poa-reqs-models/main/main December 18, 2024 00:05 Inactive

github-actions bot added test-failure and removed test-passing labels Dec 18, 2024

nihil2501 requested changes Dec 18, 2024

View reviewed changes

ojbucao force-pushed the art/97853/mvp-poa-reqs-models branch 2 times, most recently from 3252ccf to 3815598 Compare December 18, 2024 17:53

github-actions bot added test-passing and removed test-failure labels Dec 18, 2024

va-vfs-bot temporarily deployed to art/97853/mvp-poa-reqs-models/main/main December 18, 2024 18:23 Inactive

ojbucao added 8 commits December 18, 2024 10:47

Add PowerOfAttorneyRequest model, factory, and specs

2afd9d7

Add PowerOfAttorneyRequestDecision model, factory, and specs

be763b8

Add PowerOfAttorneyRequestExpiration model, factory, and specs

ec5dac4

Add PowerOfAttorneyRequestResolution model, factory, and specs

85a0c79

Add PowerOfAttorneyForm model, factory, and specs

8dc1655

(fix) Rename directory due to misspelling

f847262

(fix) Apply requested PR changes; all specs passing

f56149d

ojbucao force-pushed the art/97853/mvp-poa-reqs-models branch from 3815598 to f56149d Compare December 19, 2024 00:31

va-vfs-bot temporarily deployed to art/97853/mvp-poa-reqs-models/main/main December 19, 2024 00:33 Inactive

nihil2501 approved these changes Dec 19, 2024

View reviewed changes

ojbucao marked this pull request as ready for review December 19, 2024 00:34

ojbucao requested review from a team as code owners December 19, 2024 00:34

Merge branch 'master' into art/97853/mvp-poa-reqs-models

07134c1

github-actions bot added require-backend-approval ready-for-backend-review labels Dec 19, 2024

va-vfs-bot deployed to art/97853/mvp-poa-reqs-models/main/main December 19, 2024 00:45 View deployment

ojbucao enabled auto-merge (squash) December 19, 2024 01:01

Merge branch 'master' into art/97853/mvp-poa-reqs-models

1f0e8ce

ryan-mcneil approved these changes Dec 19, 2024

View reviewed changes

github-actions bot removed the ready-for-backend-review label Dec 19, 2024

ojbucao merged commit 365ef65 into master Dec 19, 2024
23 checks passed

ojbucao deleted the art/97853/mvp-poa-reqs-models branch December 19, 2024 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #97853 Add Power of Attorney Request Models, Factories, and Specs #19919

Issue #97853 Add Power of Attorney Request Models, Factories, and Specs #19919

ojbucao commented Dec 16, 2024

github-actions bot commented Dec 16, 2024 •

edited

Loading

Files

nihil2501 commented Dec 17, 2024

nihil2501 left a comment •

edited

Loading

nihil2501 Dec 17, 2024

ojbucao Dec 17, 2024

nihil2501 Dec 17, 2024

nihil2501 Dec 17, 2024

ojbucao Dec 17, 2024

nihil2501 Dec 17, 2024

nihil2501 Dec 17, 2024

nihil2501 Dec 17, 2024

nihil2501 Dec 17, 2024

ojbucao Dec 17, 2024

nihil2501 Dec 17, 2024

nihil2501 Dec 17, 2024

nihil2501 commented Dec 17, 2024

nihil2501 left a comment

nihil2501 Dec 18, 2024

nihil2501 Dec 18, 2024

nihil2501 Dec 18, 2024

nihil2501 Dec 18, 2024

nihil2501 left a comment

Issue #97853 Add Power of Attorney Request Models, Factories, and Specs #19919

Issue #97853 Add Power of Attorney Request Models, Factories, and Specs #19919

Conversation

ojbucao commented Dec 16, 2024

github-actions bot commented Dec 16, 2024 • edited Loading

Files

nihil2501 commented Dec 17, 2024

nihil2501 left a comment • edited Loading

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

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

nihil2501 commented Dec 17, 2024

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

nihil2501 left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 16, 2024 •

edited

Loading

nihil2501 left a comment •

edited

Loading