OCPBUGS-26566: Page fails to return to the Secrets list after clicking 'Cancel' on any Secret creation page #13504

cyril-ui-developer · 2024-01-12T14:02:50Z

Before:

After:

openshift-ci-robot · 2024-01-12T14:02:55Z

@cyril-ui-developer: This pull request references Jira Issue OCPBUGS-26566, which is invalid:

expected the bug to target the "4.16.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Before:

After:

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 openshift-eng/jira-lifecycle-plugin repository.

cyril-ui-developer · 2024-01-12T14:05:33Z

/jira refresh

openshift-ci-robot · 2024-01-12T14:05:39Z

@cyril-ui-developer: This pull request references Jira Issue OCPBUGS-26566, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @XiyunZhao

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

sg00dwin · 2024-01-12T15:34:13Z

/lgtm

TheRealJon

Removing the onCancel prop from this component changes the API. I'm not sure this is necessary to fix the bug. It might cause regressions if it's being used anywhere. Also, if this prop is going to be removed, the component prop types need to be updated as well.

TheRealJon · 2024-01-24T15:03:08Z

frontend/public/components/secrets/create-secret.tsx

@@ -272,7 +273,7 @@ export const SecretFormWrapper: React.FC<BaseEditSecretProps_> = (props) => {
        errorMessage={error || ''}
        inProgress={inProgress}
        submitText={t('public~Create')}
-        cancel={props.onCancel || defaultCancel}
+        cancel={cancel}


We probably need to keep the original logic using props.onCancel.

We probably need to keep the original logic using props.onCancel.

Please can you explain why?

TheRealJon · 2024-01-24T15:05:03Z

frontend/public/components/secrets/create-secret.tsx

-                type="button"
-                variant="secondary"
-                id="cancel"
-                onClick={onCancel || defaultCancel}


cyril-ui-developer · 2024-01-26T12:51:29Z

secrets/create-secret.tsx

@TheRealJon Yes, removing onCancel is not needed to fix the issue but it is not adding any value too. I removed because it is an optional props, and I doubt if this could ever cause any regression.
However we can clean up the other places where it is being used later.

TheRealJon · 2024-02-01T20:52:10Z

@TheRealJon Yes, removing onCancel is not needed to fix the issue but it is not adding any value too. I removed because it is an optional props, and I doubt if this could ever cause any regression. However we can clean up the other places where it is being used later.

If we know the onCancel prop of the SecretFormWrapper component is unused, I think it's okay to remove it. We also need to remove it from BaseEditSecretProps_.

cyril-ui-developer · 2024-02-01T20:58:06Z

@TheRealJon Yes, removing onCancel is not needed to fix the issue but it is not adding any value too. I removed because it is an optional props, and I doubt if this could ever cause any regression. However we can clean up the other places where it is being used later.

If we know the onCancel prop of the SecretFormWrapper component is unused, I think it's okay to remove it. We also need to remove it from BaseEditSecretProps_.

Yes, I agree. But should be a separate bug or tech-debt stuff since it would require verifying every place where it is used.

TheRealJon · 2024-02-01T21:11:26Z

Yes, I agree. But it should be a separate bug or tech-debt stuff since it would require verifying every place where it is used.

Could you clarify? The changes here and here are removing support for the onCancel prop in the SecretFormWrapper component. If this needs to be addressed as a tech debt issue, those changes must be removed from this PR. Otherwise, we have to do due diligence to remove it from that component's API as part of this PR.

cyril-ui-developer · 2024-02-02T14:38:59Z

Yes, I agree. But it should be a separate bug or tech-debt stuff since it would require verifying every place where it is used.

Could you clarify? The changes here and here are removing support for the onCancel prop in the SecretFormWrapper component. If this needs to be addressed as a tech debt issue, those changes must be removed from this PR. Otherwise, we have to do due diligence to remove it from that component's API as part of this PR.

Never mind, it seems it is a small change to remove the onCancel props.

TheRealJon

This could cause a regression in dev console.

TheRealJon · 2024-02-02T17:55:27Z

frontend/packages/dev-console/src/components/import/CreateSecretModal.tsx

@@ -24,7 +24,6 @@ const CreateSecretModal: React.FC<Props> = ({ close, namespace, save, secretType

  return (
    <SecretFormWrapper
-      onCancel={close}


This seems to have more impact than intended. Now clicking cancel in this dev console secret modal will not close the modal. I think we need to retain support for the optional onCancel prop in the SecretFormWrapper component since the API is being used here.

This seems to have more impact than intended. Now clicking cancel in this dev console secret modal will not close the modal. I think we need to retain support for the optional onCancel prop in the SecretFormWrapper component since the API is being used here.

Hmm, I traced where SecretFormWrapper is used in CreateSecretModal, and I couldn't find any instance that is passing in the onCancel props. Are you referring to the "Add secret to workload" in dev-console, or something else?

IIUC, the original behavior of the SecretFormWrapper component uses the optional onCancel callback prop when the cancel button is clicked. Dev console appears to use the SecretFormWrapper component in a modal, and uses this onCancel prop to close the modal. So, from what I can tell, removing onCancel from SecretFormWrapper may cause a regression in the dev console. I think it's beyond the scope of this bug to track down the usage of this specific prop and remove it. We should leave it as it was since there does seem to be some risk of regression.

IIUC, the original behavior of the SecretFormWrapper component uses the optional onCancel callback prop when the cancel button is clicked. Dev console appears to use the SecretFormWrapper component in a modal, and uses this onCancel prop to close the modal. So, from what I can tell, removing onCancel from SecretFormWrapper may cause a regression in the dev console. I think it's beyond the scope of this bug to track down the usage of this specific prop and remove it. We should leave it as it was since there does seem to be some risk of regression.

@TheRealJon Updated! PTAL.

cyril-ui-developer · 2024-02-07T11:15:23Z

/retest

TheRealJon

/lgtm

openshift-ci · 2024-02-07T15:41:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cyril-ui-developer, sg00dwin, TheRealJon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [TheRealJon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-02-07T17:59:02Z

@cyril-ui-developer: all tests passed!

Full PR test history. Your PR dashboard.

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.

openshift-ci-robot · 2024-02-07T18:03:19Z

@cyril-ui-developer: Jira Issue OCPBUGS-26566: All pull requests linked via external trackers have merged:

openshift/console#13504

Jira Issue OCPBUGS-26566 has been moved to the MODIFIED state.

In response to this:

Before:

After:

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-bot · 2024-02-07T22:35:38Z

[ART PR BUILD NOTIFIER]

This PR has been included in build openshift-enterprise-console-container-v4.16.0-202402072112.p0.ga9730e2.assembly.stream.el8 for distgit openshift-enterprise-console.
All builds following this will include this PR.

openshift-merge-robot · 2024-02-08T15:31:56Z

Fix included in accepted release 4.16.0-0.nightly-2024-02-08-073857

rhamilto · 2024-02-22T13:45:00Z

/cherry-pick release-4.15

openshift-cherrypick-robot · 2024-02-22T13:45:44Z

@rhamilto: new pull request created: #13630

In response to this:

/cherry-pick release-4.15

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.

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jan 12, 2024

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jan 12, 2024

openshift-ci bot requested review from XiyunZhao, jhadvig and kyoto January 12, 2024 14:06

openshift-ci bot added the component/core Related to console core functionality label Jan 12, 2024

openshift-ci bot assigned sg00dwin Jan 12, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2024

TheRealJon requested changes Jan 24, 2024

View reviewed changes

cyril-ui-developer force-pushed the page-fail-return-secret-list branch from 3974f3e to 6c79de7 Compare February 2, 2024 14:41

openshift-ci bot added component/dev-console Related to dev-console and removed lgtm Indicates that a PR is ready to be merged. labels Feb 2, 2024

cyril-ui-developer force-pushed the page-fail-return-secret-list branch 2 times, most recently from 7c3a185 to 77d66e5 Compare February 2, 2024 14:48

TheRealJon requested changes Feb 2, 2024

View reviewed changes

cyril-ui-developer force-pushed the page-fail-return-secret-list branch from 77d66e5 to 2c897d8 Compare February 5, 2024 17:54

page-fail-return-secret-list

50a6af6

cyril-ui-developer force-pushed the page-fail-return-secret-list branch from 2c897d8 to 50a6af6 Compare February 5, 2024 17:58

TheRealJon approved these changes Feb 7, 2024

View reviewed changes

openshift-ci bot assigned TheRealJon Feb 7, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2024

openshift-merge-bot bot merged commit a9730e2 into openshift:master Feb 7, 2024
6 checks passed

openshift-cherrypick-robot mentioned this pull request Feb 22, 2024

[release-4.15] OCPBUGS-29845: Page fails to return to the Secrets list after clicking 'Cancel' on any Secret creation page #13630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-26566: Page fails to return to the Secrets list after clicking 'Cancel' on any Secret creation page #13504

OCPBUGS-26566: Page fails to return to the Secrets list after clicking 'Cancel' on any Secret creation page #13504

cyril-ui-developer commented Jan 12, 2024

openshift-ci-robot commented Jan 12, 2024

Before:

After:

cyril-ui-developer commented Jan 12, 2024

openshift-ci-robot commented Jan 12, 2024

sg00dwin commented Jan 12, 2024

TheRealJon left a comment

TheRealJon Jan 24, 2024

cyril-ui-developer Jan 24, 2024

TheRealJon Jan 24, 2024

cyril-ui-developer commented Jan 26, 2024

TheRealJon commented Feb 1, 2024

cyril-ui-developer commented Feb 1, 2024

TheRealJon commented Feb 1, 2024 •

edited

cyril-ui-developer commented Feb 2, 2024

TheRealJon left a comment

TheRealJon Feb 2, 2024

cyril-ui-developer Feb 2, 2024

TheRealJon Feb 5, 2024

cyril-ui-developer Feb 5, 2024

cyril-ui-developer commented Feb 7, 2024

TheRealJon left a comment

openshift-ci bot commented Feb 7, 2024

openshift-ci bot commented Feb 7, 2024

openshift-ci-robot commented Feb 7, 2024

Before:

After:

openshift-bot commented Feb 7, 2024

openshift-merge-robot commented Feb 8, 2024

rhamilto commented Feb 22, 2024

openshift-cherrypick-robot commented Feb 22, 2024

OCPBUGS-26566: Page fails to return to the Secrets list after clicking 'Cancel' on any Secret creation page #13504

OCPBUGS-26566: Page fails to return to the Secrets list after clicking 'Cancel' on any Secret creation page #13504

Conversation

cyril-ui-developer commented Jan 12, 2024

Before:

After:

openshift-ci-robot commented Jan 12, 2024

Before:

After:

cyril-ui-developer commented Jan 12, 2024

openshift-ci-robot commented Jan 12, 2024

sg00dwin commented Jan 12, 2024

TheRealJon left a comment

Choose a reason for hiding this comment

TheRealJon Jan 24, 2024

Choose a reason for hiding this comment

cyril-ui-developer Jan 24, 2024

Choose a reason for hiding this comment

TheRealJon Jan 24, 2024

Choose a reason for hiding this comment

cyril-ui-developer commented Jan 26, 2024

TheRealJon commented Feb 1, 2024

cyril-ui-developer commented Feb 1, 2024

TheRealJon commented Feb 1, 2024 • edited

cyril-ui-developer commented Feb 2, 2024

TheRealJon left a comment

Choose a reason for hiding this comment

TheRealJon Feb 2, 2024

Choose a reason for hiding this comment

cyril-ui-developer Feb 2, 2024

Choose a reason for hiding this comment

TheRealJon Feb 5, 2024

Choose a reason for hiding this comment

cyril-ui-developer Feb 5, 2024

Choose a reason for hiding this comment

cyril-ui-developer commented Feb 7, 2024

TheRealJon left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 7, 2024

openshift-ci bot commented Feb 7, 2024

openshift-ci-robot commented Feb 7, 2024

Before:

After:

openshift-bot commented Feb 7, 2024

openshift-merge-robot commented Feb 8, 2024

rhamilto commented Feb 22, 2024

openshift-cherrypick-robot commented Feb 22, 2024

TheRealJon commented Feb 1, 2024 •

edited