-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Subscription wizard #9496
Subscription wizard #9496
Conversation
Build succeeded.
|
There are some similarities between this PR, and this one #9159. |
This UX is sooooooo much better than before 🎉 |
@wenottingham @marshmalien @trahman73 wondering if you all talked through the invalid credentials flow. Right now we're showing this: which is our standard error screen. I'm wondering if we should catch the 400/invalid credentials error and show a more elegant message? |
@@ -32,15 +32,15 @@ const SplitLayout = styled(PageSection)` | |||
`; | |||
const Card = styled(_Card)` | |||
display: inline-block; | |||
break-inside: avoid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏 this bug has bothered me for a while. Thank you!
}, | ||
]; | ||
|
||
function secondsToDays(seconds) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this would fit well in https://github.com/ansible/awx/blob/devel/awx/ui_next/src/util/dates.jsx. Might make it easier to test out a bunch of scenarios.
</> | ||
)} | ||
<Flex alignItems={{ default: 'alignItemsCenter' }}> | ||
{/* TODO: Add AA dashboard image */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this todo still valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments but this lgtm
Cc @rooftopcellist @ryanpetrello I think we return 400 with a description in the error message for all issues talking to RHSM. We'd need to catch all the subcases (401 - invalid username/password; 500 - RHSM blew up; whatever we return for a connection error, etc) if we did this. I don't know how complex that would be. |
}); | ||
|
||
return ( | ||
<Flex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😎
/> | ||
} | ||
> | ||
<FileUpload |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to specify the file type?
label={i18n._(t`Expires on`)} | ||
value={ | ||
license_info.license_date && | ||
formatDateStringUTC( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to show UTC time here? Majority of dates on the UI are formatDateString
. If we want UTC, perhaps add a note.
awx/ui_next/src/contexts/Config.jsx
Outdated
const { pathname } = useLocation(); | ||
const [configState, setConfigState] = useState({}); | ||
|
||
const { error: configError, request } = useRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to display isLoading?
@@ -178,6 +153,23 @@ function AppContainer({ i18n, navRouteConfig = [], children }) { | |||
/> | |||
); | |||
|
|||
const simpleHeader = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the simpleHeader
is being rendering for a moment after the user logged in. Then header
is being displayed. Having issues to add a .gif.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to add ouiaId
on WizardNavItem ?
<PageHeaderTools> | ||
<PageHeaderToolsGroup> | ||
<PageHeaderToolsItem> | ||
<Button onClick={handleLogout} variant="tertiary"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Button onClick={handleLogout} variant="tertiary"> | |
<Button onClick={handleLogout} variant="tertiary" ouiaId="logout"> |
</Button> | ||
)} | ||
<Button | ||
id="subscription-wizard-back" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id="subscription-wizard-back" | |
ouiaId="subscription-wizard-back" | |
id="subscription-wizard-back" |
</Button> | ||
) : ( | ||
<Button | ||
id="subscription-wizard-next" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id="subscription-wizard-next" | |
ouiaId="subscription-wizard-next" | |
id="subscription-wizard-next" |
</Button> | ||
{license_info?.valid_key && ( | ||
<Button | ||
id="subscription-wizard-cancel" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id="subscription-wizard-cancel" | |
ouiaId="subscription-wizard-cancel" | |
id="subscription-wizard-cancel" |
</p> | ||
<ToggleGroup> | ||
<ToggleGroupItem | ||
text={i18n._(t`Subscription manifest`)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text={i18n._(t`Subscription manifest`)} | |
text={i18n._(t`Subscription manifest`)} | |
ouiaId="subscription-manifest" | |
aria-label={i18n._(t`Subscription manifest`)} |
onChange={() => setIsSelected('uploadManifest')} | ||
/> | ||
<ToggleGroupItem | ||
text={i18n._(t`Username / password`)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text={i18n._(t`Username / password`)} | |
text={i18n._(t`Username / password`)} | |
ouiaId="username-password" | |
aria-label={i18n._(t`Username / password`)} |
labelIcon={ | ||
<Popover | ||
content={i18n._(t`Select the playbook to be executed by | ||
{custom_virtualenvs && custom_virtualenvs.length > 1 && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We removed custom_virtualenvs
fields from displayed on the UI
@marshmalien can you rebase this please :) |
@marshmalien , we completed a bug bash for this PR and here the result. Please make sure you sync with @tiagodread to go over what can be addressed post-merge Results
|
d5f9bb5
to
e266c08
Compare
Build succeeded.
|
e266c08
to
0e4d0c3
Compare
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @marshmalien.
0e4d0c3
to
e19bd35
Compare
Build succeeded.
|
e19bd35
to
18ee3ff
Compare
Build succeeded.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Checked that:
- apply license with a manifest file
- apply the license with credentials
- if the options for analytics and insights is working
- not navigate directly to a different route when no license was applied
- auditor or normal users can not apply license
- the user can edit the subscription from settings
Added e2e tests on 6130
18ee3ff
to
dc69375
Compare
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Build succeeded.
|
8b12459
to
dc69375
Compare
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
dc69375
to
f260601
Compare
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
1 similar comment
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
1f54c33
to
476cfd7
Compare
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
476cfd7
to
440bdee
Compare
Build succeeded.
|
regate |
Build succeeded (gate pipeline).
|
SUMMARY
Subscription Details view:
/settings/subscription/details
/settings/subscription/edit
Subscription Add wizard view:
/settings/subscription_management
Step 1 - Subscription:
Step 2 - Tracking and analytics:
Step 3 - EULA: https://tower-mockups.testing.ansible.com/patternfly/settings/settings-license-step-03/
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION