feat(topology): add new service in context

[nemesis09]

This PR-

• changes the connector icon when not over a node
• adds a context menu on releasing connector on graph or application group
• adds add-resources menu as the context menu on release of connector on graph or application group
• adds necessary URL params to context menu options for creating connection after save
• is tracked by issue https://issues.redhat.com/browse/ODC-2352

Screen (demo implementation on top of #3918 (add-resources menu) with changes pulled from #3958 (creating the connection post save) to show complete flow) -

[andrewballantyne]

@nemesis09 Could you post a link to your ticket on your PR even if it's a WIP.

/kind feature

[nemesis09]

putting on hold as it needs PR #3918 to be merged first.
/hold

[karthikjeeyar]

className or any property in topology package should not bound to odc specific style/attributes, it should be dynamic. you can pass it via props
cc: @christianvogt

[nemesis09]

I've passed it as a prop to withCreateConnector()
PTAL

[karthikjeeyar]

you could simplify it as

    if (choice) {
      onKebabOptionClick(choice, source);
    }
    return graphActions(target.getController().getElements());
  }

[nemesis09]

Done. PTAL

[karthikjeeyar]

same as above

 if (choice) {
   onKebabOptionClick(choice, source);
 }
 return groupActions(applicationGroup, true);

[nemesis09]

Done. PTAL

[karthikjeeyar]

we should not be able to create knative resources using the drag connector, because knative revision can only be a source, it can never be a target. add isKnativeDisabled param here and remove it from other place.

[nemesis09]

added isKnativeDisabled param

[christianvogt]

We shouldn't need this param. The add forms should properly handle this case by identifying there is a contextSource present and therefore remove knative service option.

[nemesis09]

removing hold as dependent PR #3918 is merged
/hold cancel

[christianvogt]

Topology should not depend on other packages.

[nemesis09]

Removed KebabItem from topology

[christianvogt]

Remove. accept is only for types.

[nemesis09]

done

[christianvogt]

Are you really supporting moving a connector to a new resource?
Please remove. It looks like you're only supporting the creation use case.

[nemesis09]

done

[christianvogt]

Suggested change

	monitor.getOperation() === CREATE_CONNECTOR_OPERATION
	monitor.getItemType() === CREATE_CONNECTOR_DROP_TYPE

[nemesis09]

done

[christianvogt]

Suggested change

	monitor.getOperation() === CREATE_CONNECTOR_OPERATION,
	monitor.getItemType() === CREATE_CONNECTOR_DROP_TYPE

[nemesis09]

done

[christianvogt]

Duplicate code. Move this block up in the function.

[nemesis09]

done

[christianvogt]

We shouldn't need this param. The add forms should properly handle this case by identifying there is a contextSource present and therefore remove knative service option.

[christianvogt]

Why is this here? i see it's copied code. @jeff-phillips-18 why do we need this? Shouldn't we be using the node data directly?

[nemesis09]

we could use node data directly but that would require modifying the groupContextMenu and groupActions files. so I created this object here.
I've made the necessary modifications to the functions and groupContextMenu is directly used now.

[christianvogt]

We cannot change this to use a KebabItem because this is generic code that shouldn't depend on console.
We'll need to find a way to support the console use case without this dependency.
Doesn't this break the storybook sample?
If it doesn't break it then I suppose we can keep it with a TODO to fix later.
But otherwise we need a way to supply a list of react components that go into the context menu.

Maybe we simply add the ability to supply a react element as a choice instead:

choices: ConnectorChoice[] | React.ReactNode;

         {
           React.isValidElement(prompt.choices) ? prompt.choices : 
            prompt.choices.map((c) => (
            <ContextMenuItem
              key={c.label}
              onClick={() => {
                onCreate(prompt.element, prompt.target, prompt.event, c);
              }}
            >
              {c.label}
            </ContextMenuItem>
          ))}

Then in your onCreate function instead of returning the list of actions, you actually build the menu items like we already do elsewhere. You'd be able to use the kebab actions handling of the callback / href without duplicating the code.

[karthikjeeyar]

@nemesis09 Check should be on item, React.isValidElement(prompt.choices?.[0]

[nemesis09]

Done

[christianvogt]

Why is this here? We should already have explicit support for each known type already.

[nemesis09]

It was overlooked while cleanup.
removed it.

[karthikjeeyar]

@nemesis09 Please remove this code

[karthikjeeyar]

remove this block and add the callback to the existing contextMenu

      <ContextMenuItem
        key={option.label}
        component={
          <KebabItem
            option={option}
            onClick={() => (onOptionClick ? onOptionClick(option) : onKebabOptionClick(option))}
          />
        }
      />

[nemesis09]

done

[karthikjeeyar]

While dropping the connector the whole page flickers, @nemesis09 please confirm if there needs to be a followup bug for this.

cc: @christianvogt

[divyanshiGupta]

You can use optional chaining here. Also if the sourceObj is null/undefined you will get a TypeError in line 254 255. I will suggest to do this:

const sourceObj = source.getData()?.resources?.obj || {};
const hrefWithContext = `${option.href}&contextSource=${referenceFor(sourceObj)}/${
        sourceObj?.metadata?.name

[nemesis09]

should we allow creation of resources through this flow if sourceObj is null or undefined. since it will reduce the flow to a simple add-resource flow?

[divyanshiGupta]

In which case the sourceObj will be undefined though? Dragging sourceObj connector and on releasing we will see the menu to create the service.

[nemesis09]

The only time i saw an error was when component did not yet mount. other than that, I think the source will always be there.

[nemesis09]

updated the code with suggested change

[divyanshiGupta]

If you have seen the error probably do not allow redirect to create flow if sourceObj is undefined

[nemesis09]

the new changes takes care of such a scenario, PTAL

[invincibleJai]

@nemesis09 will there be a + icon for a connector within a group, currently it's arrow

[nemesis09]

@christianvogt
I have updated the PR with the changes. PTAL

[andrewballantyne]

@nemesis09 You have a key issue in the console...

[nemesis09]

@nemesis09 You have a key issue in the console...

fixed

[andrewballantyne]

Clicking a group white-screens. TypeError: Cannot read property 'getData' of undefined

[nemesis09]

Clicking a group white-screens. TypeError: Cannot read property 'getData' of undefined

Added checks to fix the issue. PTAL

[andrewballantyne]

Not sure if this is you or @karthikjeeyar but the action menu that appears in the application group sidebar throws an error:

Warning: Failed prop type: Invalid prop actions[3].accessReview of type boolean supplied to ConnectFunction, expected object.

[nemesis09]

Not sure if this is you or @karthikjeeyar but the action menu that appears in the application group sidebar throws an error:

Warning: Failed prop type: Invalid prop actions[3].accessReview of type boolean supplied to ConnectFunction, expected object.

I saw the warning too.
but it does not appear consistently. I got it a few times.
looked into TopologyApplicationPanel, but nothing seemed to be out of place to me.

[christianvogt]

unnecessary change

[nemesis09]

cleaned up

[christianvogt]

This needs a TODO comment for cleanup or a fix now.
We shouldn't have to avoid calling kebabOptionsToMenu. Instead the actions provided should omit including the path when used in this scenario.

[nemesis09]

fixed

[christianvogt]

Same comment as above.

[nemesis09]

fixed

[christianvogt]

This is incorrect, leave it. We don't callback with a ReactElement.
It was a mistake in my pseudo code posted earlier attempting to describe a new solution.

[nemesis09]

fixed

[christianvogt]

/lgtm
/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, karthikjeeyar, nemesis09

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• frontend/packages/dev-console/OWNERS [christianvogt]
• frontend/packages/topology/OWNERS [christianvogt]

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

[nemesis09]

/test analyze

[nemesis09]

/test e2e-gcp-console

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[andrewballantyne]

/hold

Sam is doing investigations into the e2e, they're not likely to pass atm.
#4065

[nemesis09]

/test e2e-gcp-console

[andrewballantyne]

@nemesis09 They are not going to pass right now. PR #4065 is attempting to fix them.

[spadgett]

/hold cancel
/retest