Bug 1798404: Set target-port as the first port when route target-port is empty in edit forms

[divyanshiGupta]

Jira Issue - https://issues.redhat.com/browse/ODC-2905

Fix-

Edit flows are currently broken because of this issue - https://issues.redhat.com/browse/ODC-2853
PR for the fix - #4087

For testing this PR locally the above PR is needed.

[divyanshiGupta]

/kind bug

[openshift-ci-robot]

@divyanshiGupta: This pull request references Bugzilla bug 1798404, 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.

Details

In response to this:

Bug 1798404: Set target-port as the first port when route target-port is empty

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.

[christianvogt]

You've got 2 form fields now resetting the value of initialValues.route.targetPort creating a race condition.

My suggestion would be to track first load vs subsequent loads. The value can be stored in a ref:
const canUpdateTargetPort = React.useRef(!initialValues.route.targetPort)

You'd need to do this in each form field that uses this value.

Then once first load is complete do canUpdateTargetPort.current = true.

[divyanshiGupta]

So in this file initialValues.route.targetPort is getting re-initialized only once in line 97 whenever we have a new searchTerm. No other field resets its value, everywhere else values.route.targetPort is set and that does not affect initialValues.route.targetPort. Did you find any use case where you saw the race condition? I might be missing something.

[christianvogt]

My mistake. I mistook the other file as being in the same flow. But it's for the 2nd radio option so it wouldn't be an issue.

Changing the initial value just seems a bit odd to me even though it's working.

Did you consider just checking if the target port is found in the set of ports? And only if it isn't use the first available port? Would there be an issue with doing this? That way we don't have to mutate the initial value.

The side effect here is if they change the target port for one image search, then search again and that same target port is available, but not first in the list; it would keep that selection. Not sure if that's ok though.

Another thought is to check if the field is touched or not and if not use the initial value. Immediately after checking you can force set the field touched so that we don't use the initial value ever again.

Lastly if clearing the initial value as you are doing is the only thing that works. You can probably do this immediately after assigning the target port and not in a useEffect to simplify things.

[divyanshiGupta]

So I went through a few examples where it was needed to change the initial values and hence we have enableReinitialize which when set to true lets us change the initial values. Anyways I'll go through the other approaches you suggested.

Another thought is to check if the field is touched or not and if not use the initial value. Immediately after checking you can force set the field touched so that we don't use the initial value ever again.

Will try this one and see if it works.

[rohitkrai03]

@divyanshiGupta This combined with my suggestion above should work as a workaround here -

  React.useEffect(() => {
    if (touched.searchTerm && initialValues.searchTerm !== values.searchTerm) {
      const targetPort: ContainerPort = _.head(values.isi.ports);
      targetPort && setFieldValue('route.targetPort', makePortName(targetPort));
    }
  }, [
    touched.searchTerm,
    setFieldValue,
    values.isi.ports,
    initialValues.searchTerm,
    values.searchTerm,
  ]);

The only issue I found with this approach was that if a user starts to update the searchTerm after searching for an image before, the target port resets to head. I can't think of a scenario where this would cause any real problems for user though. If a user is changing the searchTerm, he intents to update the container image which should reset the ports.

[rohitkrai03]

/assign

[rohitkrai03]

Suggested change

	const targetPort = !initialValues.route.targetPort && _.head(ports);
	const targetPort = (!initialValues.route.targetPort || touched.searchTerm) && _.head(ports);

[rohitkrai03]

@divyanshiGupta This combined with my suggestion above should work as a workaround here -

  React.useEffect(() => {
    if (touched.searchTerm && initialValues.searchTerm !== values.searchTerm) {
      const targetPort: ContainerPort = _.head(values.isi.ports);
      targetPort && setFieldValue('route.targetPort', makePortName(targetPort));
    }
  }, [
    touched.searchTerm,
    setFieldValue,
    values.isi.ports,
    initialValues.searchTerm,
    values.searchTerm,
  ]);

The only issue I found with this approach was that if a user starts to update the searchTerm after searching for an image before, the target port resets to head. I can't think of a scenario where this would cause any real problems for user though. If a user is changing the searchTerm, he intents to update the container image which should reset the ports.

[spadgett]

/bugzilla refresh

[openshift-ci-robot]

@spadgett: This pull request references Bugzilla bug 1798404, which is invalid:

• expected the bug to target the "4.5.0" release, but it targets "4.4.0" instead

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

Details

In response to this:

/bugzilla 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 kubernetes/test-infra repository.

[rohitkrai03]

@divyanshiGupta This PR - #4187 replaced InputSearchField with InputField. In that process it lost the ability to update touched.searchTerm since it's handling the onBlur itself. So, now the logic of relying on the touched.searchTerm is broken unless we fix the issue mentioned above. You can fix it by calling setFieldTouched in the onBlur handler but that would mean that even when the form loads for the first time it will set touched to true. So my suggestion is to change the passing of handleSearch to onBlur like this -

onBlur={() => {
  handleSearch();
  setFieldTouched('searchTerm', true);
}}

[rohitkrai03]

Everything works as expected after applying the above fix for external image registry edit flow.

[rohitkrai03]

While changing the imagestream if there are more than one ports and user changes target port explicitly in the route advanced options, the target port is reset to the first port in the list of available ports.

[rohitkrai03]

Maybe add another check for targetPort not being touched by user. Then only it should update the value.

[divyanshiGupta]

/bugzilla refresh

[openshift-ci-robot]

@divyanshiGupta: This pull request references Bugzilla bug 1798404, which is valid.

3 validation(s) were run on this bug

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

Details

In response to this:

/bugzilla 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 kubernetes/test-infra repository.

[rohitkrai03]

/approve

[karthikjeeyar]

verified locally, works fine
/lgtm

[karthikjeeyar]

/retest

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: divyanshiGupta, karthikjeeyar, rohitkrai03

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 [rohitkrai03]

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

[divyanshiGupta]

/cherry-pick release-4.4

[openshift-cherrypick-robot]

@divyanshiGupta: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-4.4

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-bot]

/retest

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

[openshift-bot]

/retest

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

[openshift-ci-robot]

@divyanshiGupta: All pull requests linked via external trackers have merged. Bugzilla bug 1798404 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1798404: Set target-port as the first port when route target-port is empty in edit forms

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-cherrypick-robot]

@divyanshiGupta: new pull request created: #4561

Details

In response to this:

/cherry-pick release-4.4

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.