Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 22 commits into from
Aug 25, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
35aeac1
use update! instead of update for error handling
ferristseng Aug 21, 2020
8863470
removed call to compact to support updating emails to nil
ferristseng Aug 21, 2020
494f0b5
update deepDiff to support changing values to null
ferristseng Aug 21, 2020
172816f
update deepDiff to handle comparisons with null fields better
ferristseng Aug 24, 2020
76f04da
slight refactoring to make it clearer how formsUpdated is being calcu…
ferristseng Aug 24, 2020
37f1cbc
add backend logic that accounts for when to show alerts based on chan…
ferristseng Aug 24, 2020
d5adc7e
try to make it a little bit clearer what these *_sent_flag methods ar…
ferristseng Aug 24, 2020
fa94551
small refactor so method returns false instead of nil
ferristseng Aug 24, 2020
ce55e75
do not open virtual hearing modal if representative email was edited …
ferristseng Aug 24, 2020
5b8cf1c
when changing representative email to null, also change the represent…
ferristseng Aug 24, 2020
7a7eeac
fix a messaging change on the virtual hearing modal when editing a vi…
ferristseng Aug 24, 2020
59fcf2a
resolve line length lint warning
ferristseng Aug 24, 2020
513f66b
coerce string into boolean
ferristseng Aug 24, 2020
c9184e0
update snapshots
ferristseng Aug 24, 2020
c77bece
provide default value for fetch
ferristseng Aug 24, 2020
0f4ae82
commit linting changes
ferristseng Aug 24, 2020
e6d6357
add new tests for representative email validation on hearing details …
ferristseng Aug 24, 2020
6e173c8
update logic to only rethrow validation errors when the virtual heari…
ferristseng Aug 24, 2020
45ebeef
remove calls to `delete`
ferristseng Aug 25, 2020
d943588
Merge branch 'master' into ftseng-hearing-details-email-validation
ferristseng Aug 25, 2020
4c82a19
refactor to avoid mutation
ferristseng Aug 25, 2020
d98fe9a
fixing some linting issues
ferristseng Aug 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions app/models/hearings/forms/base_hearing_update_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ def update
hearing.reload

start_async_job
add_virtual_hearing_alert

add_virtual_hearing_alert if show_virtual_hearing_progress_alerts?
end
end

Expand Down Expand Up @@ -73,7 +74,11 @@ def show_update_alert?
return false if hearing.virtual? && (hearing_updates.dig(:scheduled_time).present? ||
hearing_updates.dig(:scheduled_for).present?)

hearing_updated?
hearing_updated? || (virtual_hearing_updates.present? && !show_virtual_hearing_progress_alerts?)
end

def show_virtual_hearing_progress_alerts?
[appellant_email_sent_flag, representative_email_sent_flag, judge_email_sent_flag].any?(false)
end

def should_create_or_update_virtual_hearing?
Expand Down Expand Up @@ -155,16 +160,18 @@ def updates_requiring_email?

# Send appellant email if cancelling, updating time or updating either appellant email or appellant timezone
def appellant_email_sent_flag
!(updates_requiring_email? ||
virtual_hearing_attributes&.key?(:appellant_email) ||
virtual_hearing_attributes&.key?(:appellant_tz))
should_send_email = updates_requiring_email? ||
virtual_hearing_attributes&.key?(:appellant_email) ||
virtual_hearing_attributes&.key?(:appellant_tz)
!should_send_email
end

# Send rep email if cancelling, updating time or updating either rep email or rep timezone
def representative_email_sent_flag
!(updates_requiring_email? ||
virtual_hearing_attributes&.key?(:representative_email) ||
virtual_hearing_attributes&.key?(:representative_tz))
should_send_email = updates_requiring_email? ||
virtual_hearing_attributes&.fetch(:representative_email, nil).present? ||
virtual_hearing_attributes&.key?(:representative_tz)
!should_send_email
end

# also returns false if the judge id is present or true if the virtual hearing is being cancelled
Expand Down Expand Up @@ -199,7 +206,7 @@ def virtual_hearing_updates

sanitize_updated_emails if virtual_hearing_attributes.present?

updates = (virtual_hearing_attributes || {}).compact.merge(emails_sent_updates)
updates = (virtual_hearing_attributes || {}).merge(emails_sent_updates)

if judge_id.present?
updates[:judge_email] = hearing.judge&.email
Expand Down Expand Up @@ -230,11 +237,11 @@ def create_or_update_virtual_hearing
# Handle the status toggle of the virtual hearing
if virtual_hearing_cancelled?
# Update the virtual hearings
virtual_hearing.update(virtual_hearing_updates)
virtual_hearing.update!(virtual_hearing_updates)

DataDogService.increment_counter(metric_name: "cancelled_virtual_hearing.successful", **updated_metric_info)
elsif !virtual_hearing_created?
virtual_hearing.update(virtual_hearing_updates)
virtual_hearing.update!(virtual_hearing_updates)
virtual_hearing.establishment.restart!
DataDogService.increment_counter(metric_name: "updated_virtual_hearing.successful", **updated_metric_info)
else
Expand Down
2 changes: 1 addition & 1 deletion app/models/hearings/forms/hearing_update_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

private
Expand Down
16 changes: 14 additions & 2 deletions client/app/hearings/components/Details.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,10 @@ const HearingDetails = (props) => {
const virtual = hearing.isVirtual || hearing.wasVirtual || converting;
const noEmail = !hearing.virtualHearing?.appellantEmail;
const noRepTimezone = !hearing.virtualHearing?.representativeTz && hearing.virtualHearing?.representativeEmail;
const emailUpdated = editedEmails?.appellantEmailEdited || editedEmails?.representativeEmailEdited;
const emailUpdated = (
editedEmails?.appellantEmailEdited ||
(editedEmails?.representativeEmailEdited && hearing.virtualHearing?.representativeEmail)
);
const timezoneUpdated = editedEmails?.representativeTzEdited || editedEmails?.appellantTzEdited;
const errors = noEmail || (noRepTimezone && hearing.readableRequestType !== 'Video');

Expand Down Expand Up @@ -174,7 +177,15 @@ const HearingDetails = (props) => {

// email validations should be thrown inline
if (code === 1002) {
if (hearing?.readableRequestType === 'Video' && !userUseFullPageVideoToVirtual) {
// API errors from the server need to be bubbled up to the VirtualHearingModal so it can
// update the email components with the validation error messages.
const changingFromVideoToVirtualWithModalFlow = (
hearing?.readableRequestType === 'Video' &&
!hearing.isVirtual &&
!userUseFullPageVideoToVirtual
);

if (changingFromVideoToVirtualWithModalFlow) {
// 1002 is returned with an invalid email. rethrow respError, then re-catch it in VirtualHearingModal
throw respError;
} else {
Expand Down Expand Up @@ -262,6 +273,7 @@ const HearingDetails = (props) => {
/>
<DetailsForm
hearing={hearing}
initialHearing={initialHearing}
update={updateHearing}
convertHearing={convertHearing}
errors={virtualHearingErrors}
Expand Down
5 changes: 3 additions & 2 deletions client/app/hearings/components/VirtualHearingModal.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,10 @@ export const ReadOnlyEmails = ({
{virtualHearing.appellantEmail}
</p>
)}
{(representativeEmailEdited || showAllEmails) && (
{(virtualHearing.representativeEmail &&
(representativeEmailEdited || showAllEmails)) && (
<p>
<strong>Representative Email</strong>
<strong>POA/Representative Email</strong>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<br />
{virtualHearing.representativeEmail}
</p>
Expand Down
16 changes: 14 additions & 2 deletions client/app/hearings/components/VirtualHearings/Fields.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const VirtualHearingFields = ({
requestType,
defaultAppellantTz,
defaultRepresentativeTz,
initialRepresentativeTz,
virtualHearing,
readOnly,
update,
Expand Down Expand Up @@ -55,7 +56,7 @@ export const VirtualHearingFields = ({
<div className="usa-width-one-third">
<Timezone
errorMessage={errors?.representativeTz}
required={virtualHearing?.representativeEmail}
required={Boolean(virtualHearing?.representativeEmail)}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes a console PropType warning

value={virtualHearing?.representativeTz || defaultRepresentativeTz}
onChange={(representativeTz) => update('virtualHearing', { representativeTz })}
readOnly={readOnly || !virtualHearing?.representativeEmail}
Expand All @@ -72,7 +73,17 @@ export const VirtualHearingFields = ({
emailType="representativeEmail"
email={virtualHearing?.representativeEmail}
error={errors?.representativeEmail}
update={update}
update={(key, value) => {
// Switch the representative timezone back to the initial value if the
// representative email is changed to null. This should prevent `deepDiff``
// from trying to send any changes to the representative timezone if the
// representative email is being removed.
if (!value.representativeEmail) {
value.representativeTz = initialRepresentativeTz;
}

update(key, value);
}}
/>
</div>
</div>
Expand Down Expand Up @@ -114,4 +125,5 @@ VirtualHearingFields.propTypes = {
errors: PropTypes.object,
defaultAppellantTz: PropTypes.string,
defaultRepresentativeTz: PropTypes.string,
initialRepresentativeTz: PropTypes.string,
};
2 changes: 1 addition & 1 deletion client/app/hearings/components/VirtualHearings/RepresentativeSection.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export const RepresentativeSection = ({
<div className={classNames('usa-width-one-half', { [noMaxWidth]: true })}>
<Timezone
errorMessage={errors?.representativeTz}
required={virtualHearing?.representativeEmail}
required={Boolean(virtualHearing?.representativeEmail)}
value={virtualHearing?.representativeTz}
onChange={(representativeTz) =>
update('virtualHearing', { representativeTz })
Expand Down
5 changes: 5 additions & 0 deletions client/app/hearings/components/details/DetailsForm.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import TextareaField from '../../../components/TextareaField';
const DetailsForm = (props) => {
const {
hearing,
initialHearing,
update,
isLegacy,
openVirtualHearingModal,
Expand Down Expand Up @@ -115,6 +116,7 @@ const DetailsForm = (props) => {
<VirtualHearingForm
errors={errors}
hearing={hearing}
initialHearing={initialHearing}
readOnly={readOnly}
virtualHearing={hearing?.virtualHearing}
update={update}
Expand Down Expand Up @@ -147,6 +149,9 @@ DetailsForm.propTypes = {
wasVirtual: PropTypes.bool,
isVirtual: PropTypes.bool,
}),
initialHearing: PropTypes.shape({
virtualHearing: PropTypes.object
}),
isLegacy: PropTypes.bool,
openVirtualHearingModal: PropTypes.func,
readOnly: PropTypes.bool,
Expand Down
6 changes: 5 additions & 1 deletion client/app/hearings/components/details/VirtualHearingForm.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { getAppellantTitleForHearing } from '../../utils';
import { VirtualHearingFields } from '../VirtualHearings/Fields';

export const VirtualHearingForm = (
{ hearing, virtualHearing, readOnly, update, errors }
{ hearing, initialHearing, virtualHearing, readOnly, update, errors }
) => {
if (!hearing?.isVirtual && !hearing?.wasVirtual) {
return null;
Expand Down Expand Up @@ -53,6 +53,7 @@ export const VirtualHearingForm = (
time={hearing.scheduledTimeString}
requestType={hearing.readableRequestType}
defaultAppellantTz={hearing?.appellantTz}
initialRepresentativeTz={initialHearing?.virtualHearing?.representativeTz}
defaultRepresentativeTz={hearing?.representativeTz}
/>
)}
Expand All @@ -70,6 +71,9 @@ VirtualHearingForm.propTypes = {
wasVirtual: PropTypes.bool,
isVirtual: PropTypes.bool
}),
initialHearing: PropTypes.shape({
virtualHearing: PropTypes.object
}),
readOnly: PropTypes.bool,
virtualHearing: PropTypes.shape({
appellantEmail: PropTypes.string,
Expand Down
46 changes: 27 additions & 19 deletions client/app/hearings/contexts/HearingsFormContext.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,15 @@ const formatHearing = (hearing) => ({
});

export const SET_UPDATED = 'setUpdated';
const setUpdated = (state, value) => ({
...state,
hearing: { ...state.hearing, ...value },
formsUpdated: !isEmpty(deepDiff(state.initialHearing, { ...state.hearing, ...value }))
});
const setUpdated = (state, value) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))
  };
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const newHearing = { ...state.hearing, ...value };

return {
...state,
hearing: newHearing,
formsUpdated: !isEmpty(deepDiff(state.initialHearing, newHearing))
};
};

// Full reset of everything.
export const RESET_HEARING = 'reset';
Expand All @@ -62,22 +66,26 @@ const reset = (state, hearing) => ({
// Resets only the `virtualHearing` field, and should preserve all other fields.
export const RESET_VIRTUAL_HEARING = 'resetVirtualHearing';
const resetVirtualHearing = (state, virtualHearing) => {
const newHearing = {
...state.hearing,
virtualHearing: {
...(state.hearing?.virtualHearing || {}),
...virtualHearing
}
};
const newInitialHearing = {
...state.initialHearing,
virtualHearing: {
...(state.initialHearing?.virtualHearing || {}),
...virtualHearing
}
};

return {
...state,
initialHearing: {
...state.initialHearing,
virtualHearing: {
...(state.initialHearing?.virtualHearing || {}),
...virtualHearing
}
},
hearing: {
...state.hearing,
virtualHearing: {
...(state.hearing?.virtualHearing || {}),
...virtualHearing
}
}
initialHearing: newInitialHearing,
hearing: newHearing,
formsUpdated: !isEmpty(deepDiff(newInitialHearing, newHearing))
};
};

Expand Down
14 changes: 8 additions & 6 deletions client/app/hearings/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,13 @@ export const deepDiff = (firstObj, secondObj) => {
(result, firstVal, key) => {
const secondVal = secondObj[key];

if (_.isEqual(firstVal, secondVal)) {
result[key] = null;
} else if (_.isObject(firstVal) && _.isObject(secondVal)) {
result[key] = deepDiff(firstVal, secondVal);
} else {
if (_.isObject(firstVal) && _.isObject(secondVal)) {
const nestedDiff = deepDiff(firstVal, secondVal);

if (nestedDiff && !_.isEmpty(nestedDiff)) {
result[key] = nestedDiff;
}
} else if (!_.isEqual(firstVal, secondVal)) {
result[key] = secondVal;
}

Expand All @@ -75,7 +77,7 @@ export const deepDiff = (firstObj, secondObj) => {
{}
);

return _.pickBy(changedObject, (val) => val !== null);
return changedObject;
};

export const filterCurrentIssues = (issues) =>
Expand Down
5 changes: 3 additions & 2 deletions client/test/app/hearings/components/VirtualHearings/__snapshots__/Fields.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15321,7 +15321,7 @@ exports[`Fields Matches snapshot with default props 1`] = `
name="representativeTz"
onChange={[Function]}
readOnly={false}
required="[email protected]"
required={true}
time="08:15"
value={null}
>
Expand Down Expand Up @@ -16098,7 +16098,7 @@ exports[`Fields Matches snapshot with default props 1`] = `
}
placeholder="Select a timezone"
readOnly={false}
required="[email protected]"
required={true}
strongLabel={true}
styling={
Object {
Expand Down Expand Up @@ -30334,6 +30334,7 @@ exports[`Fields Matches snapshot with default props 1`] = `
email="[email protected]"
emailType="representativeEmail"
label="POA/Representative Email"
update={[Function]}
>
<TextField
className={
Expand Down
4 changes: 2 additions & 2 deletions .../app/hearings/components/VirtualHearings/__snapshots__/RepresentativeSection.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ SAN FRANCISCO, CA 94103
name="POA/Representative Timezone"
onChange={[Function]}
readOnly={false}
required="[email protected]"
required={true}
time="06:00"
value={null}
>
Expand Down Expand Up @@ -930,7 +930,7 @@ SAN FRANCISCO, CA 94103
}
placeholder="Select a timezone"
readOnly={false}
required="[email protected]"
required={true}
strongLabel={true}
styling={
Object {
Expand Down
Loading