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

Use Critical error dialog with context #4942

Merged
merged 11 commits into from Aug 9, 2023

Conversation

rvykydal
Copy link
Contributor

@rvykydal rvykydal commented Jul 24, 2023

The draft is adding some context for the user to Critical Error dialog addressing https://issues.redhat.com/browse/INSTALLER-3562. I am opening it to check if the direction and approach makes sense at all (I feel there already must be a common way how to do such things.). It is a very early draft, don't miss the notes below.

The goal here is to inform user more than the exception text says. In this PR I add:

  • context: what was the acction performed when the error ocured
  • hint: (more optional) - what can user do (usually not much in case of critical errors?)

Notes:

  • The wording / presentation of the messages is to be discussed (like using just normal phrases). For example: "Initialize storage model" -> "Reading information about disks." (closer to user pov). Or "While reading information about disks this error appeared: "Error: TEST DBUS EXCEPTION". This should not happen ..."
  • In the PR there are both the new onCritFail and existing onAddErrorNotification (Just exception message in notification). I think in most cases we want to replace the notification with Critical Error dialog (needs to be audited place by place). Here I am replacing it just in progress component as an example.
  • There is a rough stab on passing the handler to Actions.
  • We would like to add also some information for the developer to find the error origin easily, like source file reference if it is possible to do in automated way (the user context messages are translated so they might be hard to find).
  • And add a link to BZ reporting to the dialog but it is another independent issue: UPDATE: this is done later below in this PR

Screenshot from 2023-07-24 10-09-54

ui/webui/src/components/app.jsx Outdated Show resolved Hide resolved
ui/webui/src/components/Error.jsx Outdated Show resolved Hide resolved
@rvykydal rvykydal force-pushed the webui-error-dialog-context branch 2 times, most recently from 755d4e0 to 528b0e7 Compare July 26, 2023 09:49
@rvykydal
Copy link
Contributor Author

I've added BZ report link to the dialog. All the layout and UI is TBD, I just wanted to demonstrate what information would be available at this point.
Action: and Hint: are added in the source code, Error: is the exception message, Report: points to the generated link (can be seen in href or the alert window.
Screenshot from 2023-07-26 11-42-07
Href does not seem to work in the dialog, click to look at the URL:
Screenshot from 2023-07-26 11-42-33

BZ form opened with the link (component, summary, details):
Screenshot from 2023-07-26 11-57-15
snip
Screenshot from 2023-07-26 11-57-30

@rvykydal rvykydal requested a review from garrett July 26, 2023 11:32
@rvykydal
Copy link
Contributor Author

Added onCritFail to installation progress:
Screenshot from 2023-07-27 10-57-37

Previous state:
Screenshot from 2023-07-27 10-55-12

@garrett
Copy link

garrett commented Jul 27, 2023

So, a few points:

  1. The reporting action shouldn't be a link, but a button.
  2. We should have a way to preview the data that would be sent.
  3. We probably should let someone know that this will send information over the network.
  4. There should be an entry in the context of the error to report what was happening, and the text would be send over to the full report.

Something like this:


Fatal bug report (1)


Fatal bug report (2)

Clicking on the preview action would toggle the modal dialog into a preview mode, where the information is read-only, in a scrollable viewport. There would be a way to toggle back to edit to be able to correct typos.

(I'm not certain of the toggle as an action button in the modal. I'm still exploring ideas. But I wanted to share what I have so far and the reasoning behind it.)


Fatal bug report (4)

When sending the report, the button should have a spinner. All input fields and buttons should be in a disabled state while sending.

It would use the progress variant of the button: https://www.patternfly.org/v4/components/button#progress-indicators


Fatal bug report (3)

When done, there should be a confirmation prompt with a link to the bug.


Looking forward to your feedback!

@garrett
Copy link

garrett commented Jul 27, 2023

Also: That dark grey background ("backdrop") should be transparent; you should be able to see the screen behind the modal, similar to the design doc @ https://www.patternfly.org/v4/components/modal/design-guidelines#center-aligned-modal-default

@garrett
Copy link

garrett commented Jul 27, 2023

If we cannot collect information from within Anaconda, then we could have an extremely reduced functionality one that still provides a preview with notice about sending information over the network (good for privacy info, especially in the EU), where it's basically this:

Fatal bug report_ preview (2)

The button would be a link that opens a new window for more information on Bugzilla (or whatever other issue tracker). Additionally, when that window is opened, we should still display "thanks" modal. However, since it already opened the issue report on Bugzilla, we'd remove the link from it on this version.

Fatal bug report_ reported

(I made the quit in this specific modal primary as it's the only option. It should still be secondary in the other modal.)

@M4rtinK
Copy link
Contributor

M4rtinK commented Jul 27, 2023

@garrett Thanks for the mockups! :) I think both look good, but given the available time and tooling, I think we can only implement number 2 for F39. Number 1 would require automated error reporting tooling that is jut not there right not function wise right now. :P

@garrett
Copy link

garrett commented Jul 27, 2023

For the previews, we might want to consider making the modal larger (especially vertically), as there might be a lot of information, such as logs. Browsing through that in a tiny scrollable area isn't very ideal. 😉 So the mockups might not be so accurate in terms of size.

@rvykydal
Copy link
Contributor Author

@garrett thank you for the mockups!

@rvykydal rvykydal force-pushed the webui-error-dialog-context branch 2 times, most recently from ebbe5aa to f301be4 Compare July 28, 2023 13:45
@rvykydal
Copy link
Contributor Author

rvykydal commented Jul 31, 2023

@garrett @M4rtinK: new update, again with layout to be refined and improved. Should demonstrate what we want (are able) to communicate in the dialog and check the UX approach:

  • Detail of the error.
  • The error / exception itself (should be perhaps less prominent).
  • Information about log.
    • I think we will use output of journalctl -a generated by the handler (or /tmp/syslog which seems to be pretty close to it but in live environment journal will maybe have more info).
    • I guess we want the user to review the log, but maybe having another button opening a bigger window would be more appropriate. Like:
      • under "Log attachment" have instructions about attaching log later in BZ and about review, then buttons [Send issue to Bugzilla] [Review log attachment] [Reboot] where the Review button will open simple window to edit / save / close the log.
      • or have the log review as next step / dialog after [Send issue to Bugzilla]
      • and we might also want a checkbox like "I will review and attach the log" but that seems rather heavy on user?
    • Do we want the log reviewing (and even attaching it) so mandatory (or upfront) so that we risk user rather not reporting the issue? Perhaps yes because issue without log is not of much use for us. I am pretty sure wording and UI around the log attachment can be done better then in my draft.
    • The name of the log file to be attached is communicated only in the Bugzilla form, in the Details field.
    • The code for generating and saving of reviewed journal output is TBD if we want it.
    • The "hint" context property in the code of the PR does not make much sense with this version and can be removed.
      Screenshot from 2023-08-01 12-17-43

Prefiled BZ report now also with log attachment mentioned:
Screenshot from 2023-08-01 12-19-13

@rvykydal
Copy link
Contributor Author

rvykydal commented Aug 1, 2023

Also: That dark grey background ("backdrop") should be transparent; you should be able to see the screen behind the modal, similar to the design doc @ https://www.patternfly.org/v4/components/modal/design-guidelines#center-aligned-modal-default

This is not a matter of Backdrop but of how we display the dialog - either dialog or the background:

. I'll look at it: 1a319b8

@rvykydal
Copy link
Contributor Author

rvykydal commented Aug 1, 2023

/boot-iso --webui

}
});
taskProxy.addEventListener("Failed", () => {
setStatus("danger");
});
taskProxy.addEventListener("Stopped", () => {
taskProxy.Finish().catch(onAddErrorNotification);
taskProxy.Finish().catch(onCritFail({
context: cockpit.format(_("Installing the system: $0"), refStatusMessage.current),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you use the 'message' directly here? I guess I am missing something?

Copy link
Contributor Author

@rvykydal rvykydal Aug 1, 2023

Choose a reason for hiding this comment

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

Message is not available here. I didn't want to use statusMessage because I would have to add it to the hook dependencies and the hook (getting tasks, registering handlers, starting the queue) would be called on every message change? So I thought useRef is suitable here.

@garrett
Copy link

garrett commented Aug 1, 2023

The contents of the modal should be more like this:

Fatal bug report_ preview (6)

But it should be a larger modal to provide enough space in the text entry, so it'd look more like this within Anaconda:

Fatal bug report_ preview (7)

The changes are basically to minimize the content outside of the scrollable area and making the scrollable area larger. Please also note the names of labels (including button labels) and use of iconography (the reprot issue does not have an icon, until submission happens, then it has a spinner and is disabled (as is quit and reboot) while submitting).

@rvykydal
Copy link
Contributor Author

rvykydal commented Aug 2, 2023

@garrett I like the labels, error description and details updates - now definitely more human readable. I am going to update the PR, and also build iso so you can play with it live, especially with the flow with following BZ link opened.

Regarding the button icons and log review I have some questions and notes.

  1. Iconography of button for reporting:

Please also note the names of labels (including button labels) and use of iconography (the reprot issue does not have an icon, until submission happens, then it has a spinner and is disabled (as is quit and reboot) while submitting).

I was using the button for sending report proposed in the alternative solution by you (opening the BZ report) above. We do just open the BZ report prefiled url in the browser with window.open so I am not sure we can have enough information / events to be able to update button with proper icons (in progress, done), ie: the user submitted the form in bugzilla (you can see part of the form in the screenshot above #4942 (comment)), the user attached the log - he has to do it manually based on the instructions in the BZ form (see Note: below) and our hint in the description (see Details:, the /tmp/webui.log is the file reviewed by the user in the previous step).
attach

So here I would rather go just with the simple stateless button. This way the user can even try again if he for some reason already closed the window with the BZ report form.

  1. Attaching the log is an action that happens later based on the instructions in BZ report form (generic for Fedora component) and our prefiled Details so it is even kind of optional, although we definitely want to have the logs. That is why I was thinking about having another button [Review log attachment] opening another modal for editing the file as large as we need. But maybe it is better to enforce a review (and then attaching) a bit by having it upfront.

@garrett
Copy link

garrett commented Aug 2, 2023

I like the labels, error description and details updates - now definitely more human readable.

👍

I am going to update the PR, and also build iso so you can play with it live, especially with the flow with following BZ link opened.

Awesome! Thank you!

I was using the button for sending report proposed in the alternative solution by you (opening the BZ report) above.

Haha, whoops. I see that above. I didn't see it in the Penpot file when I redid it and was context shifting from dev work on Cockpit (PatternFly stuff, mainly around browser compatibility and CSS), as it was on a separate page and I was basing the newer dialogs from the page with the interactive ones. Oops!

So here I would rather go just with the simple stateless button.

That would only work if it has immediate actions... which I guess it does?

(That is, using rel="noopener" target="_blank" , like so: <a href="https://link.to.site/to/report/a/bug?with=values&you=are+reporting" rel="noopener" target="_blank">Report it!</a> — but this would have to be a link that is styled like a button. Otherwise, if it's a button (which it will likely be with PF React, then you need to use window.open() with a target and such. https://developer.mozilla.org/en-US/docs/Web/API/Window/open window.open(site, target, windowFeatures) where target is also _blank and features would include noopener or noreferrer.)

This way the user can even try again if he for some reason already closed the window with the BZ report form.

Normally dialogs close, but I guess it makes sense to do this here. They'll need an option to reboot, which is in the modal, and if they accidentally close the window, like you're saying, then they need a way to get back. Since we don't have the (X) modal and there's nothing else to do other than report or quit/reboot, then it does make sense to treat this modal differently.

(In other words: I agree; this makes sense.)


I wonder if we really should use the word "Bugzilla" though. I was probably wrong suggesting that a while back. The icon probably still makes sense though, as it would open a new window.

Here's a revised mockup that changes the title, drops "Bugzilla", and has the external icon (nothing else changed):

Fatal bug report_ simple, details, larger

@rvykydal rvykydal force-pushed the webui-error-dialog-context branch 3 times, most recently from b56d44a to fc5d722 Compare August 8, 2023 12:19
Copy link
Contributor

@M4rtinK M4rtinK left a comment

Choose a reason for hiding this comment

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

Still looks good to me. :)

@KKoukiou
Copy link
Contributor

KKoukiou commented Aug 8, 2023

Split two commits #5027 as these can already be merged. To get this a bit leaner.

@rvykydal
Copy link
Contributor Author

rvykydal commented Aug 9, 2023

/kickstart-test --waive webui-only

…g is read

The reviewed log is dumped to the checked file on the button click.
@rvykydal
Copy link
Contributor Author

rvykydal commented Aug 9, 2023

/kickstart-test --waive webui-only

<Button
variant="primary"
isLoading={preparingReport}
isDisabled={preparingReport}
Copy link
Contributor

Choose a reason for hiding this comment

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

You should disable this button instead of adding sleep in the test. So isDisabled={!content || preparingReport}

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'll open a followup PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rvykydal rvykydal merged commit 37ffa72 into rhinstaller:master Aug 9, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants