Fix bug on hearing details page where email error messages weren't being displayed #15044

ferristseng · 2020-08-21T22:03:22Z

Resolves #14990

Description

Update server to throw if there are validation errors (update => update!)
Update server to handle setting the representative email to null
- Updated server to not send down virtual hearing notification alerts if only the representative email was updated
Small fixes to remove PropType warnings
Add tests for email validations on the hearing details page

Acceptance Criteria

Ensure validations display when modifying any email address on the hearing details page for a virtual hearing

Testing Plan

Login as a Hearing Coordinator
Go to the hearing details page for a virtual hearing
Try changing representative email to 123456
Try deleting the representative email
Try changing veteran email and deleting the representative email
Try changing the representative timezone and then deleting the representative email (the timezone should revert)
Try making both the veteran email and representative email invalid

User Facing Changes

va-bot · 2020-08-21T22:07:05Z

	2 Warnings
⚠️	This is a Big PR. Try to break this down if possible. Stacked pull requests encourage more detailed and thorough code reviews
⚠️	This PR modifies React components — consider adding/updating corresponding Storybook file

Generated by 🚫 Danger

codeclimate · 2020-08-21T22:07:55Z

Code Climate has analyzed commit d98fe9a and detected 0 issues on this pull request.

View more on Code Climate.

…lated

…ges made to the virtual hearing

…e actually doing

…to null

…ative timezone back to the initial value

…deo-virtual hearing

…page

…ng modal is being displayed

ferristseng · 2020-08-25T14:35:54Z

client/app/hearings/components/HearingConversion.jsx

@@ -148,7 +148,7 @@ export const HearingConversion = ({
            <div className={classNames('usa-width-one-half', { [noMaxWidth]: true })} >
              <Timezone
                errorMessage={errors?.representativeTz}
-                required={virtualHearing?.representativeEmail}
+                required={Boolean(virtualHearing?.representativeEmail)}


Fixes a console PropType warning

Thank you so much for fixing this!

ferristseng · 2020-08-25T14:36:10Z

client/app/hearings/components/VirtualHearings/Fields.jsx

@@ -55,7 +56,7 @@ export const VirtualHearingFields = ({
        <div className="usa-width-one-third">
          <Timezone
            errorMessage={errors?.representativeTz}
-            required={virtualHearing?.representativeEmail}
+            required={Boolean(virtualHearing?.representativeEmail)}


Fixes a console PropType warning

ferristseng · 2020-08-25T14:36:50Z

client/app/hearings/components/VirtualHearingModal.jsx

        <p>
-          <strong>Representative Email</strong>
+          <strong>POA/Representative Email</strong>


Makes this label match the pop-up that shows for formerly central virtual hearings

ferristseng · 2020-08-25T14:39:22Z

app/models/hearings/forms/hearing_update_form.rb

@@ -15,7 +15,7 @@ def update_hearing
  end

  def hearing_updated?
-    super || advance_on_docket_motion_attributes&.present?
+    super || advance_on_docket_motion_attributes.present?


Small fix so this returns false instead of nil if advance_on_docket_motion_attributes == nil

sahalliburton

Logic looks great! Just mentioned a few things I think we should address, thanks!

sahalliburton · 2020-08-25T15:37:03Z

client/app/hearings/contexts/HearingsFormContext.jsx

-  hearing: { ...state.hearing, ...value },
-  formsUpdated: !isEmpty(deepDiff(state.initialHearing, { ...state.hearing, ...value }))
-});
+const setUpdated = (state, value) => {


Refactoring this way doesnt change the logic at all, I think we should leave this as is

IMO the refactor makes it a bit easier to understand what's going on. Previously, the expression which represents the merged new hearing, { ...state.hearing, ...value }, was repeated twice. I think the intention of the call to deepDiff is to determine if there are changes between the new hearing and the initial hearing, which I think the refactor captures a bit better.

Refactoring this way also adds mutation to the function because instead of using the splat operator to assign the formsUpdated value, you are first creating an object and then assigning the value. I really think we should leave this as is because it follows functional programming guidelines of not-mutating

I generally agree with not-mutating, but I don't see the downside here because we are mutating a local that's declared and instantiated inside the function. I could see it being a bigger problem if we were mutating the field of an argument that was being passed in.

One alternative way of doing this could be:

const setUpdated = (state, value) => { const newHearing = { ...state.hearing, ...value }; return { ...state, hearing: newHearing, formsUpdated: !isEmpty(deepDiff(newState.initialHearing, newHearing)) }; };

That refactor is better because it does not mutate. While I don't agree that this makes the code easier to read IMO it actually makes it more difficult, this does follow guidelines so I will concede

sahalliburton · 2020-08-25T15:40:59Z

client/app/hearings/utils.js

@@ -63,9 +63,15 @@ export const deepDiff = (firstObj, secondObj) => {
      const secondVal = secondObj[key];

      if (_.isEqual(firstVal, secondVal)) {
-        result[key] = null;
+        delete result[key];


Can we refactor this to be a pure function? I am concerned about using delete because it can lead to unintended side-effects that are hard to debug due to JavaScripts asynchronous data access.

Ok, removed the calls to delete! Please take a look again!

LGTM thanks!

sahalliburton

LGTM

ferristseng added 3 commits August 21, 2020 15:38

use update! instead of update for error handling

35aeac1

removed call to compact to support updating emails to nil

8863470

update deepDiff to support changing values to null

494f0b5

ferristseng added the Team: Tango 💃 label Aug 21, 2020

ferristseng added 15 commits August 24, 2020 14:01

update deepDiff to handle comparisons with null fields better

172816f

slight refactoring to make it clearer how formsUpdated is being calcu…

76f04da

…lated

add backend logic that accounts for when to show alerts based on chan…

37f1cbc

…ges made to the virtual hearing

try to make it a little bit clearer what these *_sent_flag methods ar…

d5adc7e

…e actually doing

small refactor so method returns false instead of nil

fa94551

do not open virtual hearing modal if representative email was edited …

ce55e75

…to null

when changing representative email to null, also change the represent…

5b8cf1c

…ative timezone back to the initial value

fix a messaging change on the virtual hearing modal when editing a vi…

7a7eeac

…deo-virtual hearing

resolve line length lint warning

59fcf2a

coerce string into boolean

513f66b

update snapshots

c9184e0

provide default value for fetch

c77bece

commit linting changes

0f4ae82

add new tests for representative email validation on hearing details …

e6d6357

…page

update logic to only rethrow validation errors when the virtual heari…

6e173c8

…ng modal is being displayed

ferristseng commented Aug 25, 2020

View reviewed changes

ferristseng changed the title ~~[WIP] Fix bug on hearing details page where email error messages weren't being displayed~~ Fix bug on hearing details page where email error messages weren't being displayed Aug 25, 2020

ferristseng requested review from a team and rubaiyat22 and removed request for a team and rubaiyat22 August 25, 2020 14:47

ferristseng requested a review from sahalliburton August 25, 2020 14:47

sahalliburton suggested changes Aug 25, 2020

View reviewed changes

ferristseng added 2 commits August 25, 2020 13:42

remove calls to delete

45ebeef

Merge branch 'master' into ftseng-hearing-details-email-validation

d943588

sahalliburton approved these changes Aug 25, 2020

View reviewed changes

refactor to avoid mutation

4c82a19

ferristseng added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Aug 25, 2020

fixing some linting issues

d98fe9a

va-bot merged commit 9afb417 into master Aug 25, 2020

va-bot deleted the ftseng-hearing-details-email-validation branch August 25, 2020 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug on hearing details page where email error messages weren't being displayed #15044

Fix bug on hearing details page where email error messages weren't being displayed #15044

ferristseng commented Aug 21, 2020 •

edited

Loading

va-bot commented Aug 21, 2020 •

edited

Loading

codeclimate bot commented Aug 21, 2020 •

edited

Loading

ferristseng Aug 25, 2020

sahalliburton Aug 25, 2020

ferristseng Aug 25, 2020

ferristseng Aug 25, 2020

ferristseng Aug 25, 2020

sahalliburton left a comment

sahalliburton Aug 25, 2020

ferristseng Aug 25, 2020

sahalliburton Aug 25, 2020

ferristseng Aug 25, 2020

sahalliburton Aug 25, 2020

sahalliburton Aug 25, 2020

ferristseng Aug 25, 2020

sahalliburton Aug 25, 2020

sahalliburton left a comment

Fix bug on hearing details page where email error messages weren't being displayed #15044

Fix bug on hearing details page where email error messages weren't being displayed #15044

Conversation

ferristseng commented Aug 21, 2020 • edited Loading

Description

Acceptance Criteria

Testing Plan

User Facing Changes

va-bot commented Aug 21, 2020 • edited Loading

codeclimate bot commented Aug 21, 2020 • 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

sahalliburton 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

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

sahalliburton left a comment

Choose a reason for hiding this comment

ferristseng commented Aug 21, 2020 •

edited

Loading

va-bot commented Aug 21, 2020 •

edited

Loading

codeclimate bot commented Aug 21, 2020 •

edited

Loading