Fix 41: Create/Delete Provider Types #983

frankduncan · 2018-07-23T03:15:57Z

This grew to more than just #41, as it appeared the entire provider type editing UI was incomplete.

Test by adding, editing, and deleting provider types from the admin interface. Additional check to see if agreements and license options are added to enrollments.

PaulMorris

This is a big improvement, thanks @frankduncan ! There's a lot here. Everything worked as expected when I tried it out. (Adding, editing, deleting provider types, adding licenses to them, and seeing the licenses show up on a new application of the new type.) I had a few minor comments. Given the size, it could probably use another look by another pair of eyes, particularly some of the Java code changes and the unit tests. Thanks for all the code improvements and the solid test coverage.

PaulMorris · 2018-07-25T17:02:13Z

psm-app/cms-web/WebContent/WEB-INF/pages/admin/service_admin_view_provider_type.jsp

+                        <div class="col2">
+                          <c:forEach var="licenseType" items="${licenseTypes}">
+                            <div class="row">
+                              <span>${licenseType.description}</span>


You can omit the <span> here, as it's not doing anything.

PaulMorris · 2018-07-25T17:08:39Z

psm-app/cms-web/WebContent/WEB-INF/pages/admin/service_admin_edit_provider_type.jsp

@@ -62,6 +62,32 @@
                          </div>
                        </div>
                      </div>
+                      <div class="tableHeader"><span>Applicable Licenses</span></div>


Do you need the span here? I see they exist in similar places elsewhere in this file.

There is a css specifically for these, yeah. I am definitely wary of the different style interactions, so I just copy paste from other sections in the page when I'm doing "make a new section that looks like that one please."

Ah, gotcha, makes sense.

PaulMorris · 2018-07-25T17:12:48Z

psm-app/cms-web/WebContent/WEB-INF/pages/admin/service_admin_edit_provider_type.jsp

+                                  <c:forEach var="licenseType" items="${allLicenseTypes}">
+                                    <option
+                                      value="${licenseType.code}"
+                                      ${licenseType.code eq selectedLicenseType.code ? 'selected' : ''}


Optional suggestion: use "==" instead of "eq" because it's less cryptic and easier for JPS novices to understand.

PaulMorris · 2018-07-25T17:14:27Z

psm-app/cms-web/WebContent/css/style.css

@@ -1274,7 +1274,7 @@ input.date{
  width:160px;
 }

-.tableData table .remove{
+.remove{


PaulMorris · 2018-07-25T17:50:02Z

...b/WebContent/WEB-INF/pages/admin/includes/service_admin_create_edit_provider_type_common.jsp

+                </c:forEach>
+                <c:forEach var="doc" items="${remainingAgreements}">
+                  <div class="row">
+                    <input id="remaining_provider_agreement_${doc.id}" type="checkbox" name="providerAgreements" value="${doc.id}"/>


These lines are a little long. I see they are already that way, but if you feel inclined to split them up on several lines while we're here, feel free.

PaulMorris · 2018-07-25T21:08:29Z

...ntegration-tests/src/test/java/gov/medicaid/features/enrollment/ui/OrganizationInfoPage.java

@@ -105,7 +103,6 @@ public void setContactEmail(String contactEmail) {
    }

    public void checkForFeinError() throws Exception {
-        assertThat($(".errorInfo > ._15_fein").getText())
-                .contains(ORGANIZATION_FEIN_ERROR_MESSAGE);
+        checkForFormError("_15_fein", ORGANIZATION_FEIN_ERROR_MESSAGE);


PaulMorris · 2018-07-25T21:48:50Z

...b/WebContent/WEB-INF/pages/admin/includes/service_admin_create_edit_provider_type_common.jsp

-            <span class="floatL"><b>:</b></span>
-            <form:input id="createEditProviderTypeProviderType" path="description" cssClass="text"/>
+          <label for="editProviderTypeProviderType">Provider Type</label>
+          <span class="floatL"><b>:</b></span>


Let's take out this line to avoid the unnecessary colon (see #376).

PaulMorris · 2018-07-25T22:06:13Z

...b/WebContent/WEB-INF/pages/admin/includes/service_admin_create_edit_provider_type_common.jsp

+            <c:forEach var="selectedLicenseCode" items="${selectedLicenseCodes}">
+              <div class="providerTypeLicenseRow">
+                <label>
+                  <div class="providerTypeLicenseRowLabel">Applicable License:</div>


Let's remove the colon here (see #376).

PaulMorris · 2018-07-25T22:07:21Z

...b/WebContent/WEB-INF/pages/admin/includes/service_admin_create_edit_provider_type_common.jsp

+</form:form>
+<div id="licenseTemplate" class="providerTypeLicenseRow">
+  <label>
+    <div class="providerTypeLicenseRowLabel">Applicable License:</div>


Let's remove the colon here (see #376).

PaulMorris · 2018-07-25T22:23:07Z

psm-app/cms-web/WebContent/css/style.css

+.providerTypeLicenseRow .remove {
+  float:left;
+  margin-top:10px;
+}


Hmmm, this brings up the BEM approach again. Would these work without the parent .providerTypeLicenseRow ? Maybe with more specific class names? If so, then it would be better to avoid the inheritance. And for the label one it would need its own class. But I won't insist if you want to stick with this approach.

Hmmmm. I'm not sure if these would work without that.

As some context, the css for this gave me a lot of issues, because this page brought in a lot of css intended for the enrollment application screens. When I brought in selects and the remove button, things just looked bad until I started overriding some of those general css (for instance, all selects in a newEnrollmentPanel are float:left). I attempted a more organized, architected approach, but every time I played around with that, things went really weird, really fast, so I just kind of kept throwing styles at it until it looked right. I surmised that doing it the right way and rebuilding the page intelligently would involve a bit too much work.

The consequence is that if you remove the newEnrollmentPanel class from the top level div, none of these are even necessary anymore, much less useful as anything other than a bandaid for this specific case, so having them be reusable via BEM might not be a win.

However! I'm very welcome to doing that in the interim if you think it's the right move with this context.

I'm still somewhat unused to the BEM approach. This would be replaced by .providerTypeLicenseRow__remove and that class added to the remove span? And then similarly with other items that I did specifically for this page?

I hear you on the CSS issues... These kinds of difficulties are the reason I'm interested in moving towards a more BEM-style approach (since they usually involve unwanted inheritance and needing greater and greater selector specificity to override existing styles with new ones).

We haven't adopted the full BEM naming convention (but maybe we should...). I've just been trying to avoid inheritance when creating new styles, when possible and straightforward, and using unique class names instead. But it's an uphill battle, to say the least, given the current size and state of our CSS file.

I'm still getting used to BEM, but I think .providerTypeLicenseRow__remove is the right idea, if we say that the row is a block and the remove button is an element that belongs to that block and will only appear in it (so we have a block__element class). (If we wanted to say that the remove button is its own block, and could appear in other contexts, then we'd just have a block-only name, something like .remove-button. If we were starting from scratch that probably would be the way to go, but since we're not, I think it would be best to just go with the block__element class.)

Let me look a little closer at this particular case and see what makes sense. Curious about what removing the newEnrollmentPanel would do. More in a bit.

frankduncan · 2018-07-26T10:15:26Z

Thanks for the review @PaulMorris ! I did some discussion inline before I make some of the changes you were requesting. I'm also requesting a quick review from @jasonaowen as a secondary sanity check.

PaulMorris · 2018-07-26T18:59:21Z

Per discussion on zulip, I've added some commits for the CSS part. I'm +1 to merge pending @jasonaowen taking a look.

jasonaowen

Overall the code looks good, but for a few small (non-blocking) concerns below. Testing now!

Also, I'm sorry for introducing merge conflicts, @frankduncan - I should have prioritized reviewing this over making further changes.

jasonaowen · 2018-08-21T20:02:58Z

psm-app/services/src/main/java/gov/medicaid/services/ProviderTypeService.java

+    /**
+     * This method gets a provider type by its description. If not found, returns null.
+     *
+     * @param string - the description of the provider type to retrieve


Typo: string should be description.

jasonaowen · 2018-08-21T20:03:32Z

psm-app/services/src/main/java/gov/medicaid/services/ProviderTypeService.java

+     *
+     * @param string - the description of the provider type to retrieve
+     * @return - the requested provider type
+     * @throws PortalServiceException - If there are any errors during the execution of this method


This method doesn't declare PortalServiceException as an exception it throws, so this @throws line should be deleted.

jasonaowen · 2018-08-21T21:08:37Z

...siness-process/src/test/groovy/gov/medicaid/services/impl/ProviderTypeServiceBeanTest.groovy

+        pt.code == '00'
+    }
+
+    def "Get ProviderType then update"() {


I'm having trouble understanding this test. What do you want it to do? Would you mind splitting out a given: section for test setup so that the when: section can contain only the bit you want to test? Does it need to be broken out into multiple different tests?

Take a look now, I think it's clearer.

jasonaowen · 2018-08-24T19:49:53Z

@frankduncan this is pretty amazing! Awesome job.

In particular, I like the logic around disabling deletion if there are existing enrollments with that provider type or if there are provider type settings.

Is there a way to add organizational provider types, or only individual?

There is, I think, follow-up work to fully complete the feature of "adding and editing provider types", but this PR is probably enough to resolve #41. Can you help me figure out the remaining steps necessary, and then we can open issues?

Allow admins to configure the information collection screens required for a provider type (I feel like we talked about this, and agreed how bad the current approach is, but I don't remember / can't find an issue for it)
Split up provider type settings, and expose admin UI for configuring each (see PRs Extract ProviderType-AgreementDocument link #86 and Extract ProviderType-LicenseType link #99 for earlier examples)
Remove need for gov.medicaid.domain.model.ProviderType enum #946, as you previously opened - thank you

frankduncan · 2018-08-28T20:30:42Z

I've opened #1060, #1061, and #1062 for these issues. Yay!

jasonaowen

Thank you, @frankduncan! Those follow-up issues are perfect, and this looks good to me.

Having two entity graphs for completeness is unneeded by the application and blocking the ability to easily edit provider types. To fetch eagerly when getting a single providertype requires the agreements and license type to be sets.

Consolodating them all in ProviderTypeService makes further enhancements cleaner.

Issue #41: Fix or remove provider type creation/deletion

Issue #41: Fix or remove provider type creation/deletion When re-enabling editing, license types were getting lost. This adds a UI to keep/add them.

Provider Types must have unique descriptions, and this adds the UI for that enforcement. Issue #41: Fix or remove provider type creation/deletion

Issue #41: Fix or remove provider type creation/deletion

kfogel added the review label Jul 23, 2018

frankduncan force-pushed the 41-Fix-Provider-Types-Interface branch 2 times, most recently from 04e41bf to 9eb058c Compare July 23, 2018 04:55

frankduncan changed the title ~~Fix 41: Create/Delete Provider Types~~ Fix 41: Create/Delete Provider Types - WIP Jul 23, 2018

frankduncan force-pushed the 41-Fix-Provider-Types-Interface branch from 2036122 to d2fd69d Compare July 24, 2018 14:55

frankduncan changed the title ~~Fix 41: Create/Delete Provider Types - WIP~~ Fix 41: Create/Delete Provider Types Jul 24, 2018

frankduncan force-pushed the 41-Fix-Provider-Types-Interface branch 2 times, most recently from fd94e0a to e428ee1 Compare July 25, 2018 16:02

PaulMorris self-requested a review July 25, 2018 21:17

PaulMorris requested changes Jul 25, 2018

View reviewed changes

frankduncan force-pushed the 41-Fix-Provider-Types-Interface branch 2 times, most recently from 6c4e4bd to eebb454 Compare July 26, 2018 10:10

frankduncan requested a review from jasonaowen July 26, 2018 10:15

PaulMorris approved these changes Jul 26, 2018

View reviewed changes

jasonaowen reviewed Aug 24, 2018

View reviewed changes

frankduncan force-pushed the 41-Fix-Provider-Types-Interface branch 3 times, most recently from 923c986 to 6d09acd Compare August 28, 2018 19:57

frankduncan force-pushed the 41-Fix-Provider-Types-Interface branch 2 times, most recently from 75879c5 to d66e16e Compare August 28, 2018 20:36

jasonaowen approved these changes Aug 28, 2018

View reviewed changes

Frank Duncan added 5 commits August 28, 2018 16:15

Update provider type entity graph.

eff0b6a

Having two entity graphs for completeness is unneeded by the application and blocking the ability to easily edit provider types. To fetch eagerly when getting a single providertype requires the agreements and license type to be sets.

Refactor ProviderType calls from LookupService

423569c

Consolodating them all in ProviderTypeService makes further enhancements cleaner.

Add LicenseTypes to ProviderType View

62e33b2

Issue #41: Reenable provider type editing.

12c0717

Issue #41: Fix or remove provider type creation/deletion

Issue #41: Add provider type - license type selection

588e7aa

Issue #41: Fix or remove provider type creation/deletion When re-enabling editing, license types were getting lost. This adds a UI to keep/add them.

Frank Duncan added 7 commits August 28, 2018 16:15

Refactor admin ProviderType viewing.

ffc359d

Add error handling to provider type edit.

787e03e

Provider Types must have unique descriptions, and this adds the UI for that enforcement. Issue #41: Fix or remove provider type creation/deletion

Refactor provider type edit into common include.

a392685

Issue #41: Fix add provider type

146f1a7

Issue #41: Fix or remove provider type creation/deletion

Fix #41: Fix delete provider type

0313f36

Issue #41: Fix or remove provider type creation/deletion

Add tests for ProviderType admin interface.

0e934e1

Refactor tests to use general method

6bdf723

frankduncan force-pushed the 41-Fix-Provider-Types-Interface branch from d66e16e to 6bdf723 Compare August 28, 2018 21:27

frankduncan merged commit 06698f1 into master Aug 29, 2018

frankduncan deleted the 41-Fix-Provider-Types-Interface branch August 29, 2018 10:01

kfogel removed the review label Aug 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 41: Create/Delete Provider Types #983

Fix 41: Create/Delete Provider Types #983

frankduncan commented Jul 23, 2018 •

edited

Loading

PaulMorris left a comment •

edited

Loading

PaulMorris Jul 25, 2018

PaulMorris Jul 25, 2018

frankduncan Jul 26, 2018

PaulMorris Jul 26, 2018

PaulMorris Jul 25, 2018

PaulMorris Jul 25, 2018

PaulMorris Jul 25, 2018

PaulMorris Jul 25, 2018

PaulMorris Jul 25, 2018

PaulMorris Jul 25, 2018

PaulMorris Jul 25, 2018

PaulMorris Jul 25, 2018

frankduncan Jul 26, 2018

PaulMorris Jul 26, 2018 •

edited

Loading

frankduncan commented Jul 26, 2018

PaulMorris commented Jul 26, 2018

jasonaowen left a comment

jasonaowen Aug 21, 2018

jasonaowen Aug 21, 2018

jasonaowen Aug 21, 2018

frankduncan Aug 28, 2018

jasonaowen commented Aug 24, 2018 •

edited

Loading

frankduncan commented Aug 28, 2018

jasonaowen left a comment

Fix 41: Create/Delete Provider Types #983

Fix 41: Create/Delete Provider Types #983

Conversation

frankduncan commented Jul 23, 2018 • edited Loading

PaulMorris 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

Choose a reason for hiding this comment

PaulMorris Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

frankduncan commented Jul 26, 2018

PaulMorris commented Jul 26, 2018

jasonaowen 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

jasonaowen commented Aug 24, 2018 • edited Loading

frankduncan commented Aug 28, 2018

jasonaowen left a comment

Choose a reason for hiding this comment

frankduncan commented Jul 23, 2018 •

edited

Loading

PaulMorris left a comment •

edited

Loading

PaulMorris Jul 26, 2018 •

edited

Loading

jasonaowen commented Aug 24, 2018 •

edited

Loading