SolutionGuidance / psm

Welcome to the Medicare/Medicaid Provider Enrollment Screening Portal
http://projectpsm.org/
Other
26 stars 20 forks source link

Issue 949: Collapse profile #997

Closed frankduncan closed 5 years ago

frankduncan commented 6 years ago

This updates the persistence code and how the profile and enrollment are linked, no longer blowing away and rebuilding profiles when saving an enrollment.

The new schema is that for every provider_profile, there exists one active enrollment, now referenced by enrollment_id. For every enrollment, there is a provider_profile referenced by reference_profile_id. There can be multiple enrollments for a given provider_profile, but only one is active and can be approved/duplicated (for editing)/renewed.

The active code is coming in a later PR, but this lays the foundation.

This is related to #949, but also fixes #701

frankduncan commented 6 years ago

This is ready for review, of which will be review phase one, I'm sure.

frankduncan commented 6 years ago

Jenkins, test this please.

frankduncan commented 6 years ago

would you mind tagging that issue in those commits? And expanding their commit messages a bit with why this is useful?

Done!

frankduncan commented 6 years ago

Alright! I believe everything is handled.

Thanks a ton for the review @jasonaowen

frankduncan commented 6 years ago

Because of the timeline, I cherry-picked a commit from the split-enrollment-profile branch into this one, adding the "isActive" flag to UserRequest and Enrollment. I think it makes the UI less confusing when looking at #967, since providers can only update/renew the latest versions of their Enrollments, but it's easy to back out if we think it's not worth it to add.

PaulMorris commented 6 years ago

@jasonaowen I have not actually tested this yet. I was hoping to get to that last week, but since we weren't going to get this in for current demos, I focused on master (and there was plenty to do there). But I am happy to do some testing with this.

frankduncan commented 6 years ago

One of the issues we've uncovered in past testing was that the "My Profiles" page no longer works as expected; this is fixed (by removing the page) in a subsequent PR (right? which one?)

That was actually fixed by enabling the code needed for #967, so, yay?

We also talked last week about merging this and fixing any issues with it going forward; that's not the way I typically like to work, but I agree it's probably the right thing to do here.

That was just to make sure we had it for demos. That time has passed. No worries about merging it when you feel it's ready!

As for testing, I've done a few applications of draft screen one, draft screen two, draft screen three, then submit, approve then renew, deny then edit. Each time making sure the database only had one profile and it showed up on the appropriate screens. But outside of that basic smoke testing along expected success paths, I haven't done much.

jasonaowen commented 6 years ago

Okay! @frankduncan @PaulMorris thank you, that helps me understand where we are.

One of the issues we've uncovered in past testing was that the "My Profiles" page no longer works as expected; this is fixed (by removing the page) in a subsequent PR (right? which one?)

That was actually fixed by enabling the code needed for #967, so, yay?

@frankduncan do you mean that #967 is the code that fixes it? Or that the issue is now fixed in this PR, and I just haven't re-tested that part yet?

On Zulip, @frankduncan asked me for a rough test plan. We should probably record this somewhere in the repo at some point, but for now, here's what I came up with:

Bonus points if we create & approve an application for every type of provider, though I certainly don't expect you to do so, much less write tests about it. In particular, though, we should probably double-check:

frankduncan commented 6 years ago

Or that the issue is now fixed in this PR, and I just haven't re-tested that part yet?

Code in this PR, specifically commit 8adb7527

jasonaowen commented 6 years ago

Jenkins, test this please.

kfogel commented 6 years ago

@frankduncan noted in Zulip chat just now that this should have PR #1003 merged first for safety.