Conversation
|
Skipping CI for Draft Pull Request. |
WIP Add e2e test WIP WIP WIP
1b4b521 to
6d9b269
Compare
|
Images are ready for the commit at 8c466f1. To use with deploy scripts, first |
|
/retest |
|
/test gke-postgres-ui-e2e-tests |
dvail
left a comment
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
Is this URL something we can determine dynamically, or do we just need to make a note to check this before each release?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
...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, { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
^ @lmaynard88
Is Dave's suggestion something you can add to the docs?
There was a problem hiding this comment.
Yes, this can be added to the 3.73 doc.
| it('should should redirect to the Clusters page', () => { | ||
| visitDashboardWithNoClusters(); | ||
|
|
||
| cy.url().should('contain', `${clustersUrl}`); |
There was a problem hiding this comment.
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);There was a problem hiding this comment.
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.")'); |
There was a problem hiding this comment.
Accessibility: how about h2 element here, because Clusters page already has h1 element?
There was a problem hiding this comment.
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)', () => { |
There was a problem hiding this comment.
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', () => {| alignItems={{ default: 'alignItemsCenter' }} | ||
| justifyContent={{ default: 'justifyContentCenter' }} | ||
| className="pf-u-text-align-center" | ||
| direction={{ default: 'column' }} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
- Although people debate number of
h1elements 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 multipleh1would have earned a comment. - is
data-testidprop left over? Test asserts on element and content.
| pageSize={pageSize} | ||
| /> | ||
| </div> | ||
| {(!fetchingClusters || pollingCount > 0) && currentClusters.length <= 0 && ( |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I started to improve the non-empty flow, but stopped myself, so as not to set a bad example through "scope creep".
There was a problem hiding this comment.
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">There was a problem hiding this comment.
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.
| const { hasReadAccess, hasReadWriteAccess, isLoadingPermissions } = usePermissions(); | ||
|
|
||
| // Check for clusters under management | ||
| // if none, and user can admin Clusters, redirect to clusters section |
There was a problem hiding this comment.
The phrase and use can admin Clusters implies a potential timing problem, because the following if statement waits until response from /v1/mypermissions request.
pedrottimark
left a comment
There was a problem hiding this comment.
Approved with some comments for your consideration. Let’s be careful.
|
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. |
|
/retest |
1 similar comment
|
/retest |
|
@vjwilson: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
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
Testing Performed
(overrode comparison to show redirect and Empty State of clusters)

And here is the link to the docs page
