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

Webui bz report offline #5075

Merged
merged 2 commits into from
Aug 29, 2023
Merged

Conversation

rvykydal
Copy link
Contributor

@rvykydal rvykydal commented Aug 24, 2023

Addresses https://issues.redhat.com/browse/INSTALLER-3637
TODO:

Video:
https://rvykydal.fedorapeople.org/webui/error_handling/webui-bz-report-offline.webm

To check locally you can apply updates image in your live installation of some reasonably recent image in terminal by running:

sudo /usr/libexec/anaconda/apply-updates https://rvykydal.fedorapeople.org/webui/error_handling/updates.offline.im

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Looks great. you 're right about the testing that it might be hard.

@@ -127,7 +129,9 @@ export const BZReportModal = ({
/>
<FormHelperText isHidden={false}>
<HelperText>
<HelperTextItem>{_("Reporting an issue will send information over the network. Plese review and edit the attached log to remove any sensitive information.")}</HelperTextItem>
{isConnected
? <HelperTextItem> {_("Reporting an issue will send information over the network. Plese review and edit the attached log to remove any sensitive information.")} </HelperTextItem>
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Plese/Please

Copy link
Contributor

Choose a reason for hiding this comment

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

oops this exists upstream already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

Regarding the icon you mentioned from https://react-icons.github.io/react-icons/search?q=offline this is a whole new librbary and not Patternfly. So I think we should stick to Patternfly available icons.
If some icons is missing there, report that upstream https://github.com/patternfly/patternfly-react/issues/

@rvykydal
Copy link
Contributor Author

rvykydal commented Aug 24, 2023

Regarding the icon you mentioned from https://react-icons.github.io/react-icons/search?q=offline this is a whole new librbary and not Patternfly. So I think we should stick to Patternfly available icons. If some icons is missing there, report that upstream https://github.com/patternfly/patternfly-react/issues/

You are right, I was blind, it is among PF icons (https://www.patternfly.org/design-foundations/icons/).

@rvykydal
Copy link
Contributor Author

/kickstart-test --waive webui-only

@rvykydal
Copy link
Contributor Author

rvykydal commented Aug 25, 2023

I would suggest to add the tests in a separate issue. We need to think about alternatives:

  • mock at the level of Anaconda (add Connected setter to API)
  • mock at the level of dasbus
  • mock at the level of cockpit dbus handling
  • add ability to test offline installer to our CI
  • test via OpenQA
  • ... ?
    -> tracked in https://issues.redhat.com/browse/INSTALLER-3658

@AdamWill
Copy link
Contributor

Fedora bug report for Beta FE purposes: https://bugzilla.redhat.com/show_bug.cgi?id=2234964

@rvykydal
Copy link
Contributor Author

/kickstart-test --waive webui-only

@rvykydal rvykydal requested a review from M4rtinK August 29, 2023 07:02
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.

Looks good to me, thanks! :)

<HelperTextItem>{_("Reporting an issue will send information over the network. Plese review and edit the attached log to remove any sensitive information.")}</HelperTextItem>
{isConnected
? <HelperTextItem> {_("Reporting an issue will send information over the network. Please review and edit the attached log to remove any sensitive information.")} </HelperTextItem>
: <HelperTextItem icon={<DisconnectedIcon />}> {_("Network not available. Configure the network in the top bar menu to report the issue.")} </HelperTextItem>}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is good for now in the Fedora Workstation environment, but we should take a note to handle this somehow better for other environments. Eq. boot.iso currently does not have a top bar & other Live spins could have different mechanisms for this.

@rvykydal rvykydal merged commit 0bb1bfd into rhinstaller:master Aug 29, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants