Bug 2018264: Delete Export button doesn't work in topology sidebar (general issue with unknown CSV?) #10457

yozaam · 2021-11-15T05:59:58Z

Fixes:

https://bugzilla.redhat.com/show_bug.cgi?id=2018264
https://issues.redhat.com/browse/OCPBUGSM-36630

Analysis / Root cause:

kind was undefined as it was passed in the props as kindObj

Solution Description:

If kind is undefined, then search for kindObj and also double check if kind is not undefined when accessing its property label

Screen shots / Gifs for design review:

Unit test coverage report:

Test setup:

Steps to Reproduce:

Open Operator Hub and install the "Primer" GitOps Operator
Switch to developer perspective
Import at least one Application to enable the Export button in topology
If not opened yet, open the topology, and click the "Export Application" button
Wait until the export finishs and a new Deployment "primer-export-primer" was shown
Click on surrounding CSV Export "Primer" to open the sidebar
Click in the topology sidebar on Actions > Delete Export

Browser conformance:

Chrome
Firefox
Safari
Edge

openshift-ci · 2021-11-15T06:00:05Z

@yozaam: This pull request references Bugzilla bug 2018264, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

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

Requesting review from QA contact:
/cc @sanketpathak

In response to this:

Bug 2018264: Delete Export button doesn't work in topology sidebar (general issue with unknown CSV?)

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.

yozaam · 2021-11-15T17:32:48Z

/retest-required

jerolimov

First of all, your PR works fine. 😄 But the new prop type confuses me.

This is not what the DeleteModalProps "allows". kind is not optional, it should be defined. And if I search for deleteModal({ to get all code areas that want to open the delete modal, there is the only one that doesn't align with the other one.

(I believe) You can also fix this issue by changing csv-actions.ts to

        deleteModal({
          kind: kindObj,
          resource,
          // ...

Similar to the other deleteModal({ calls.

Can you please check if the other changes here are then required if this caller aka the root cause is fixed? Maybe an additional null check is still worth it!

After this fix, I have still a general concern with the old code. This issue is introduced because the deleteModal AND ALL OTHER functions exported in public/components/modals/index.ts don't define an argument type.

It would be cool if you can check if build, tests, etc. works fine if you define the deleteModal like this:

export const deleteModal = (props: DeleteModalProps) =>
  import('./delete-modal' /* webpackChunkName: "delete-modal" */).then((m) => m.deleteModal(props));

I have just quick checked this and notice multiple errors after adding DeleteModalProps

csv-actions.ts doesn't set the kind prop as mentioned above.
Some callers (csv-actions.ts and packages/operator-lifecycle-manager/src/components/operand/index.tsx) submits a namespace prop to the deleteModal function which is completely ignored! I think we can drop it from the caller code.
DeleteModalProps message prop is optional, only one caller submits this prop.
Some arguments are still missing, that happen because DeleteModalProps includes also & ModalComponentProps & HandlePromiseProps. For this, I would recommend just defining the DeleteModalProps with its own custom props.

export type DeleteModalProps = {
  kind: K8sKind;
  kindObj?: K8sKind;
  resource: any;
  close?: () => void;
  redirectTo?: any;
  message?: JSX.Element;
  cancel?: () => void;
  btnText?: string;
};

And then using these props in the function call as union type, like this:

const DeleteModal = withHandlePromise((props: DeleteModalProps & HandlePromiseProps) => {

ModalComponentProps is not used in this function, so I believe we can remove the import and ignore it as well.

Sorry, much longer than expected for an already working PR 👍 But I really think we should fix this without a new prop (which uses just another name) and I also would like to improve our TS strongly-type check quality here. Wdyt? :)

I also would like to create a tech dept story for the other missing modal props if this works fine for the delete modal.

yozaam · 2021-11-17T08:26:31Z

Thank you @jerolimov
made the changes

jerolimov

Tested this locally and the code works fine for the broken as well as other known resources.

/lgtm
/approve

jerolimov · 2021-11-17T15:16:02Z

/retest

openshift-ci · 2021-11-17T15:17:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, yozaam

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~~ [jerolimov]

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

openshift-bot · 2021-11-17T17:32:39Z

/retest-required