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

Subscription wizard #9496

Conversation

marshmalien
Copy link
Member

SUMMARY

  • Adds subscriptions routes
  • Extends Config context to return an array with two items: config state and config setter
    • Update components that use Config with the new return pattern
  • Move mock config into setupTests.js
  • Return only the routes the user is "authorized" (valid license key) to view

Subscription Details view: /settings/subscription/details

  • This is our standard details view
  • Clicking Edit will send the user to subscription wizard view /settings/subscription/edit
  • Route is not accessible when license type is OPEN

Subscription Add wizard view: /settings/subscription_management

Step 1 - Subscription:

  • If a user does not have a Red Hat Ansible Automation Platform subscription, they can request a trial subscription via the link
  • Toggle between uploading a subscription manifest .zip file or retrieving subscriptions using Red Hat credentials (username and password)
  • Get Subscriptions button fetches subscriptions and displays them in a modal

wizard

subscriptions

Step 2 - Tracking and analytics:

  • Shows two checkboxes to enable User analytics and Automation analytics
  • If the user has previously selected the RH subscription manifest flow, checking the Automation Analytics box will display required RH username and password fields
  • If the user has previously selected the RH username/password flow, they will not see this additional username/password field if Automation Analytics is checked

aa

Step 3 - EULA: https://tower-mockups.testing.ansible.com/patternfly/settings/settings-license-step-03/

  • Submission should show a success message and navigate user to dashboard if this is the initial launch and to the subscription detail view if they are editing the subscription
  • Failed submission should show a wizard form error

ISSUE TYPE

  • Feature

COMPONENT NAME

  • UI

ADDITIONAL INFORMATION

wizard

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@nixocio
Copy link
Contributor

nixocio commented Mar 5, 2021

There are some similarities between this PR, and this one #9159.

@nixocio nixocio self-requested a review March 5, 2021 19:53
@unlikelyzero unlikelyzero added the type:feature prioritized on a feature board label Mar 8, 2021
@unlikelyzero unlikelyzero self-assigned this Mar 8, 2021
@mabashian mabashian self-requested a review March 9, 2021 13:27
@mabashian
Copy link
Member

This UX is sooooooo much better than before 🎉

@mabashian
Copy link
Member

@wenottingham @marshmalien @trahman73 wondering if you all talked through the invalid credentials flow. Right now we're showing this:

Screen Shot 2021-03-09 at 12 28 50 PM

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;
Copy link
Member

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) {
Copy link
Member

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 */}
Copy link
Member

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?

Copy link
Member

@mabashian mabashian left a 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

@wenottingham
Copy link
Contributor

which is our standard error screen. I'm wondering if we should catch the 400/invalid credentials error and show a more elegant message?

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
Copy link
Contributor

Choose a reason for hiding this comment

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

😎

/>
}
>
<FileUpload
Copy link
Contributor

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(
Copy link
Contributor

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.

const { pathname } = useLocation();
const [configState, setConfigState] = useState({});

const { error: configError, request } = useRequest(
Copy link
Contributor

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 = (
Copy link
Contributor

@nixocio nixocio Mar 11, 2021

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.

Copy link
Contributor

@tiagodread tiagodread left a 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">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Button onClick={handleLogout} variant="tertiary">
<Button onClick={handleLogout} variant="tertiary" ouiaId="logout">

</Button>
)}
<Button
id="subscription-wizard-back"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id="subscription-wizard-back"
ouiaId="subscription-wizard-back"
id="subscription-wizard-back"

</Button>
) : (
<Button
id="subscription-wizard-next"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id="subscription-wizard-next"
ouiaId="subscription-wizard-next"
id="subscription-wizard-next"

</Button>
{license_info?.valid_key && (
<Button
id="subscription-wizard-cancel"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
id="subscription-wizard-cancel"
ouiaId="subscription-wizard-cancel"
id="subscription-wizard-cancel"

</p>
<ToggleGroup>
<ToggleGroupItem
text={i18n._(t`Subscription manifest`)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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`)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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 && (
Copy link
Contributor

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

@unlikelyzero
Copy link

@marshmalien can you rebase this please :)

@unlikelyzero
Copy link

unlikelyzero commented Mar 23, 2021

@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

  • Invalid Red Hat credentials could give user more immediate feedback on failure
  • We should make it more clear where the Red Hat username and Log in come from on the second step of the wizard
  • All hyperlinks should have a popout behavior. There are several examples of hyperlinks which use the existing browser page and others which open another tab.
  • EULA is 2019
  • Request subscription button cannot receive tab-focus for a11y
  • On the subscription details the Time remaining is displaying {duration} days
  • Clicking on the Activity Stream from the license settings page then clicking back on the License link on activity stream the user is redirected to the main settings page instead of the license settings page.
  • Rename Trial header

Screen Shot 2021-03-23 at 2 55 25 PM

- Consider re-ordering the dt's within the details view

Screen Shot 2021-03-23 at 2 56 07 PM

- Links to documentation on the wizard should point to ansible 4.0 instead of latest - Should allow users to change the credentials for automation analytics, but still populate from the creds used for subscription in case they are the same - testability - only allow zip? - After applying DELETE request, Username / Password fields are still saved? - What should auditor/normal user see?

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Contributor

@nixocio nixocio left a comment

Choose a reason for hiding this comment

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

Great job @marshmalien.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

Copy link
Contributor

@tiagodread tiagodread left a 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

@softwarefactory-project-zuul
Copy link
Contributor

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.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

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.

@softwarefactory-project-zuul
Copy link
Contributor

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
@softwarefactory-project-zuul
Copy link
Contributor

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.

@softwarefactory-project-zuul
Copy link
Contributor

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.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@unlikelyzero
Copy link

regate

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit f9981c0 into ansible:devel Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants