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.

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

[DT-841]Display error from Consent on Sign In #2701

Merged
merged 6 commits into from
Oct 31, 2024
Merged

Conversation

rjohanek
Copy link
Contributor

@rjohanek rjohanek commented Oct 28, 2024

Addresses

Ticket: https://broadworkbench.atlassian.net/browse/DT-841

Summary

  • Pass through error from consent and display to the user
  • Update other error messages to include the description as well (msg field was unused)
  • Convert Alert to tsx so that we can use an optional prop to have an onClose event and an x in the corner of the alert
Screenshot 2024-10-31 at 1 13 20 PM

Have you read Terra's Contributing Guide lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@rjohanek rjohanek requested a review from a team as a code owner October 28, 2024 19:45
@rjohanek rjohanek requested review from snf2ye and s-rubenstein and removed request for a team October 28, 2024 19:45
Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

A suggestion to simplify getting the body:

src/components/SignInButton.tsx Outdated Show resolved Hide resolved
@rjohanek rjohanek requested a review from fboulnois October 29, 2024 17:37
@@ -113,18 +114,34 @@ export const SignInButton = (props: SignInButtonProps) => {
await Metrics.captureEvent(event);
};

const errorStreamToString = async (error: HttpError) => {
const body = await new Response(error.body).json();
return body.message || JSON.stringify(body);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if there is a message return that, else return the whole error

Copy link
Contributor

Choose a reason for hiding this comment

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

What if that is really long? Do we truncate the message?

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 don't believe its truncated, I think it will display the whole thing. Would it be preferable to truncate it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. This is possibly something we can just observe and address if it becomes a problem.

@rjohanek rjohanek requested review from rushtong and cinyecai October 30, 2024 18:35
Copy link
Contributor

@s-rubenstein s-rubenstein left a comment

Choose a reason for hiding this comment

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

Looks reasonable - I think there are a few questions but mine at least isn't blocking.

setErrorDisplay({show: true, title: 'Error', description: errorMessage});
setTimeout(() => {
setErrorDisplay({});
}, 10000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you x out of the error message?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, I don't think we have a close handler on the error message. Would be nice.

Copy link
Contributor

@rushtong rushtong Oct 30, 2024

Choose a reason for hiding this comment

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

I hacked this a little bit ... this code looks ugly (style wise), but works. Modify the alert div around line 221:

        : <div className="dialog-alert">
            <button onClick={() => setErrorDisplay({})}>X</button>
          <Alert
            id="dialog"
            type="danger"
            title={(errorDisplay as ErrorInfo).title}
            description={(errorDisplay as ErrorInfo).description}
          />
        </div>

Copy link
Contributor

@rushtong rushtong Oct 31, 2024

Choose a reason for hiding this comment

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

Spent some more time with this - I think the Alert component is too restrictive. Modifying it to take a conditional close handler might be more work than we want to take on due to how much usage it has. Here is my hacky attempt at a solution that uses import CloseIcon from '@mui/icons-material/Close';

        : <div id={`dialog_alert`} className={`alert-wrapper danger`} style={{border: '1px solid red', borderRadius: '5px'}}>
          <span
            style={{float: 'right', fontWeight: 'bolder', fontSize: 24, cursor: 'pointer'}}
            onClick={() => setErrorDisplay({})}><CloseIcon/></span>
          <h4 id={`dialog_title`} className="alert-title">{(errorDisplay as ErrorInfo).title}</h4>
          <span id={`dialog_description`}
            className="alert-description">{(errorDisplay as ErrorInfo).description}</span>
        </div>

Screenshot 2024-10-31 at 7 34 05 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With some advice from Miguel, I decided to convert the Alert to tsx so I could explicitly define the props and make onClose optional with an undefined default value.

Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

looks good 👍

onClose?: () => void;
}

export const Alert = ({id, type, title, description, onClose}: AlertProps) => {
Copy link

@msilva-broad msilva-broad Oct 31, 2024

Choose a reason for hiding this comment

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

breaking this out to a second line would help readability and match the recommended pattern (including the return type:


export const Alert = (props: AlertProps): React.ReactNode => {
  const { id, type, title, description, onClose } = props;
  //...

Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

👍🏽

@rjohanek rjohanek merged commit 0a1aba2 into develop Oct 31, 2024
9 checks passed
@rjohanek rjohanek deleted the rj/dt-841-sam-error branch October 31, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants