Skip to content

ROX-13123, ROX-13124: Redirect to clusters page when no secured clusters detected#3541

Merged
vjwilson merged 5 commits intomasterfrom
ROX-12931-redirect-to-cluster-page-when-no-clusters-detected
Nov 4, 2022
Merged

ROX-13123, ROX-13124: Redirect to clusters page when no secured clusters detected#3541
vjwilson merged 5 commits intomasterfrom
ROX-12931-redirect-to-cluster-page-when-no-clusters-detected

Conversation

@vjwilson
Copy link
Contributor

@vjwilson vjwilson commented Oct 24, 2022

Description

After much discussion, we have landed on the look in this Jira comment
https://issues.redhat.com/browse/ROX-12931?focusedCommentId=21174780&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-21174780

Simple alert and language, with a link that opens a doc page which will be updated with more appropriate instructions when the 3.73 docs are released.

Checklist

  • Investigated and inspected CI test results (postgres ui-e2e tests passed earlier, now stuck in certificate error)
  • Unit test and regression tests added

Testing Performed

  • Added basic cypress e2e smoke test
  • Manual testing

(overrode comparison to show redirect and Empty State of clusters)
Screen Shot 2022-11-04 at 12 37 19 PM

And here is the link to the docs page
Screen Shot 2022-11-02 at 10 43 27 AM

@openshift-ci
Copy link

openshift-ci bot commented Oct 24, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@vjwilson vjwilson force-pushed the ROX-12931-redirect-to-cluster-page-when-no-clusters-detected branch from 1b4b521 to 6d9b269 Compare November 2, 2022 15:02
@vjwilson vjwilson requested review from dvail and lynnem123 November 2, 2022 15:06
@vjwilson vjwilson marked this pull request as ready for review November 2, 2022 15:06
@ghost
Copy link

ghost commented Nov 2, 2022

Images are ready for the commit at 8c466f1.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.72.x-395-g8c466f166d.

@vjwilson
Copy link
Contributor Author

vjwilson commented Nov 2, 2022

/retest

@vjwilson
Copy link
Contributor Author

vjwilson commented Nov 3, 2022

/test gke-postgres-ui-e2e-tests

Copy link
Contributor

@dvail dvail left a comment

Choose a reason for hiding this comment

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

I had a couple of thoughts/questions, but they might not be applicable to this PR directly.

component="a"
target="_blank"
rel="noopener noreferrer nofollow"
href="https://docs.openshift.com/acs/3.72/installing/install-ocp-operator.html#adding-a-new-cluster-to-rhacs"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this URL something we can determine dynamically, or do we just need to make a note to check this before each release?

Choose a reason for hiding this comment

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

We plan to redirect to a page in the 3.73 doc set once we publish the 3.73 docs. So it's not dynamic. We may want a different approach in future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why plan if redirects work already? #3799

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msugakov The redirect that Lynne is referring to, is a 301 redirect of that doc link to a new improved documentation page about installation, which would not exist until 3.73 is released. The redirect in the documentation means we will not have to change this hard-coded link in the product itself.


// Check for clusters under management
// if none, and user can admin Clusters, redirect to clusters section
// (only applicable in Cloud Services version)
Copy link
Contributor

Choose a reason for hiding this comment

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

By "only applicable in Cloud Services version", does that mean that this situation is only ever possible in the cloud services version? Is there any way an on-prem install could return no clusters?

Related, is the ability to view clusters something that is controlled by SAC? Is it possible a user could have insufficient permissions to view clusters but still be able to do useful work, and this would cause them to get stuck on the clusters page?

Copy link
Contributor Author

@vjwilson vjwilson Nov 4, 2022

Choose a reason for hiding this comment

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

...Is there any way an on-prem install could return no clusters?

While I never underestimate the ability of a user to "bork" their environment, the affordance of encouraging them to connect clusters after they have accidentally unconnected them all, is still helpful even if they are not in the Cloud Services environment. The comment is mainly to explain to folks in the future why this needed to be added at all, since the self-managed installation always adds the cluster where central is installed.

Is it possible a user could have insufficient permissions to view clusters but still be able to do useful work, and this would cause them to get stuck on the clusters page?

The check only happens on page load, so the redirect would only happen on first visit, or if the user triggered a browser refresh. They can always navigate away to do other things. There is not much useful to do until a least one cluster is connected--maybe create custom policies, or integrations--but any user without permissions wouldn't be stuck.

// Check for clusters under management
// if none, and user can admin Clusters, redirect to clusters section
// (only applicable in Cloud Services version)
useQuery<ClusterCountResponse>(CLUSTER_COUNT, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another thought, I wonder if it would be worth adding some text either in the UI or on the docs page that tells the user they need to refresh the ACS UI once a cluster is added. Since Apollo caches the GQL query results, this call will continue to return 0 every time the user navigates to a new page, even after a cluster is added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^ @lmaynard88
Is Dave's suggestion something you can add to the docs?

Choose a reason for hiding this comment

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

Yes, this can be added to the 3.73 doc.

it('should should redirect to the Clusters page', () => {
visitDashboardWithNoClusters();

cy.url().should('contain', `${clustersUrl}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rewrite assertion according to majority pattern (118 versus 3 occurrences) and move into helper function because semantically related to heading assertion there, which is repeated here.

Also removes need to import url here. By the way, it can be exact match, true?

cy.location('pathname').should('eq', clustersUrl);

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 intentionally took all assertions out of the helper function, because this is a one-off test, where the helper function is mainly just to abstract the API mocks and waiting.
(I almost moved everything into test, instead of keeping the mocking in a helper function, but in the end made the subjective call that the setup distracted from the assertions, so that's where I drew the line in this special case.

'p:contains("You have successfully deployed a Red Hat Advanced Cluster Security platform.")'
);

cy.get('h1:contains("Configure the clusters you want to secure.")');
Copy link
Contributor

Choose a reason for hiding this comment

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

Accessibility: how about h2 element here, because Clusters page already has h1 element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, but tricky because the important information is that "you aren't monitoring any clusters yet".
I'll see if I can make it a semantic h2, but with h1 styling.

});
});

describe('when no secured clusters are added yet (only applies to Cloud Service)', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add test to a new file like clustersCloudService.test.js

describe('Clusters Cloud Service', () => {
    it('should redirect from main dashboard when no secured clusters', () => {

Comment on lines +26 to +29
alignItems={{ default: 'alignItemsCenter' }}
justifyContent={{ default: 'justifyContentCenter' }}
className="pf-u-text-align-center"
direction={{ default: 'column' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this equivalent to Bullseye layout element?

https://www.patternfly.org/v4/layouts/bullseye/design-guidelines

Use a Bullseye layout to center content, both vertically and horizontally within a container.

Copy link
Contributor Author

@vjwilson vjwilson Nov 4, 2022

Choose a reason for hiding this comment

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

I tried a Bullseye at first, but required style overrides to get the spacing between the image, text, and button correct. Rather than hack in overrides, I chose to simplify. (The larger UX team recommended this look to Zhenpeng as a variation the Empty State pattern.)

</FlexItem>
<FlexItem className="pf-u-w-66">
<TextContent className="pf-u-mb-xl">
<Text component={TextVariants.h1} data-testid="congratulations">
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Although people debate number of h1 elements on a page for SEO, accessibility day of learning resources included live interview of screen reader user exploring a page for the first time, which included extensive use of heading hierarchy and comments about anything that diverged from semantic hierarchy. In that case skips in levels. It seems likely that multiple h1 would have earned a comment.
  2. is data-testid prop left over? Test asserts on element and content.

pageSize={pageSize}
/>
</div>
{(!fetchingClusters || pollingCount > 0) && currentClusters.length <= 0 && (
Copy link
Contributor

Choose a reason for hiding this comment

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

To follow up excellent improvement to add fetchingClusters and move to a pattern which supports error message if that becomes necessary, something like {content} here and if-else-if-else for assignment makes it clearer to render spinner by default (else in potential future, error message) else if no clusters AddClusterPrompt else CheckboxTable element.

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 started to improve the non-empty flow, but stopped myself, so as not to set a bad example through "scope creep".

Copy link
Contributor

@pedrottimark pedrottimark Nov 4, 2022

Choose a reason for hiding this comment

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

The complicated conditions imply non-obvious behavior change to render neither when fetching.

How about the following, which keeps render neither until after first fetch, but simplifies the conditions:

  • It is hard to see how array length can become less than zero.
  • If length is non-zero (or if you prefer, greater than zero) it seems redundant to require not fetching.
{currentClusters.length === 0 && (!fetchingClusters || pollingCount > 0) && (
    <AddClusterPrompt />
)}
{currentClusters.length !== 0 && (
    <div data-testid="clusters-table" className="h-full w-full">

Copy link
Contributor

Choose a reason for hiding this comment

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

After release 3.73 we can circle back to question by Dave whether no clusters unambiguously implies Cloud Service.

For the moment, no support for cluster auto-upgrade implies Cloud Service. Because it is not impossible that auto-upgrade might become supported in the future, we can follow up about how to get unambiguous indication. Besides this initial conditional rendering, it seems relevant information to render in Clusters and System Health.

https://github.com/stackrox/stackrox/blob/master/ui/apps/platform/src/services/ClustersService.ts#L100-L110

const { hasReadAccess, hasReadWriteAccess, isLoadingPermissions } = usePermissions();

// Check for clusters under management
// if none, and user can admin Clusters, redirect to clusters section
Copy link
Contributor

Choose a reason for hiding this comment

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

The phrase and use can admin Clusters implies a potential timing problem, because the following if statement waits until response from /v1/mypermissions request.

Copy link
Contributor

@pedrottimark pedrottimark left a comment

Choose a reason for hiding this comment

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

Approved with some comments for your consideration. Let’s be careful.

@lynnem123
Copy link

Minor nits for future consideration (if too late now): Change future to present tense. "...new clusters are listed here" instead of "will be."

Also, aren't all clusters listed, not just new ones? So you can add, and at a later time add more, and they all appear here, whether new or not. Should it say "secured clusters are listed here" instead of "new" clusters?

LGTM otherwise.

@vjwilson
Copy link
Contributor Author

vjwilson commented Nov 4, 2022

/retest

1 similar comment
@vjwilson
Copy link
Contributor Author

vjwilson commented Nov 4, 2022

/retest

@openshift-ci
Copy link

openshift-ci bot commented Nov 4, 2022

@vjwilson: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/gke-postgres-ui-e2e-tests 8c466f1 link false /test gke-postgres-ui-e2e-tests

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@vjwilson vjwilson merged commit 5fb3ab5 into master Nov 4, 2022
@vjwilson vjwilson deleted the ROX-12931-redirect-to-cluster-page-when-no-clusters-detected branch November 4, 2022 22:17
rhybrillou pushed a commit that referenced this pull request Nov 7, 2022
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.

5 participants