Bug 1867864: restrict multiple selections of a revision in traffic modal #6286

nemesis09 · 2020-08-11T04:10:24Z

Fixes:
https://issues.redhat.com/browse/ODC-3696

Analysis / Root cause:
a revision can be assigned traffic multiple times in traffic distribution modal

Solution Description:
restrict multiple selection of the same revision in the traffic modal by removing the already selected revisions from dropdown options

Screens:

Test Coverage:

Browser Conformance:

Firefox
Chrome
Safari
Edge

nemesis09 · 2020-08-11T04:14:33Z

/assign @invincibleJai

nemesis09 · 2020-08-11T04:17:52Z

/kind bug

openshift-ci-robot · 2020-08-11T04:32:56Z

@nemesis09: This pull request references Bugzilla bug 1867864, 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 release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1867864: restrict multiple selections of a revision in traffic modal

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 · 2020-08-11T10:57:34Z

@nemesis09: This pull request references Bugzilla bug 1867864, which is valid.

3 validation(s) were run on this bug

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

In response to this:

Bug 1867864: restrict multiple selections of a revision in traffic modal

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.

invincibleJai · 2020-08-11T12:16:20Z

...kages/knative-plugin/src/components/traffic-splitting/TrafficModalRevisionsDropdownField.tsx

+import { DropdownField } from '@console/shared';
+
+type TrafficModalRevisionsDropdownFieldProps = {
+  revisionItems: any;


do we know type for it

It is an object of type {string(revisionName): string(revisionName)}
Should I define a type for it in utils to use?

There is already a type RevisionItems in utils, got overlooked
updated it with type

invincibleJai · 2020-08-11T12:27:40Z

@nemesis09 we need to disable add row as well is no revisions left with disableAddRow

nemesis09 · 2020-08-11T13:11:32Z

@nemesis09 we need to disable add row as well is no revisions left with disableAddRow

done

nemesis09 · 2020-08-11T14:48:19Z

/retest

invincibleJai · 2020-08-12T16:14:30Z

it works fine , just one if same revision added traffic twice via cli then in modal i can see add enabled and dropdown is empty

invincibleJai · 2020-08-12T16:20:39Z

...kages/knative-plugin/src/components/traffic-splitting/TrafficModalRevisionsDropdownField.tsx

+}) => {
+  const [field] = useField(name);
+  const dropdownItems =
+    !field.value || Object.values(revisionItems).includes(field.value)


nit: can we it as below

Suggested change

!field.value || Object.values(revisionItems).includes(field.value)

Object.values(revisionItems).includes(field.value)

this will allow empty value to be shown in dropdown resulting in a field with blank title

am not sure m why will that be the case?

invincibleJai · 2020-08-12T16:37:45Z

...es/knative-plugin/src/components/traffic-splitting/__tests__/TrafficSplittingFields.spec.tsx

  });

-  it('should not disable delete row button for more than one values', () => {
-    const wrapper = shallow(<TrafficSplittingFields {...formProps} />);
+  it('should not disable delete row button or add row button if number of values if more than one but less than total number of revisions', () => {


Typo:

Suggested change

it('should not disable delete row button or add row button if number of values if more than one but less than total number of revisions', () => {

it('should not disable delete row button or add row button if number of values is more than one but less than total number of revisions', () => {

invincibleJai · 2020-08-12T16:40:03Z

...es/knative-plugin/src/components/traffic-splitting/__tests__/TrafficSplittingFields.spec.tsx

+        values={{
+          trafficSplitting: mockTrafficData
+            .filter((_, index) => index < 2)
+            .map((trafficElement) => ({ ...trafficElement, percent: 50 })),
+        }}


IMO below will be simpler as it's mock data

Suggested change

values={{

trafficSplitting: mockTrafficData

.filter((_, index) => index < 2)

.map((trafficElement) => ({ ...trafficElement, percent: 50 })),

}}

values={[

{ percent: 50, tag: 'tag-1', revisionName: 'overlayimage-fdqsf' },

{ percent: 50, tag: 'tag-2', revisionName: 'overlayimage-tkvz5' },

]}

invincibleJai · 2020-08-12T16:40:38Z

...es/knative-plugin/src/components/traffic-splitting/__tests__/TrafficSplittingFields.spec.tsx

+        .find(TrafficModalRevisionsDropdownField)
+        .first()
+        .props().revisionItems['overlayimage-fdqsf'],
+    ).toBeFalsy();


nit : can stick to

Suggested change

).toBeFalsy();

).toBe(false);

karthikjeeyar · 2020-08-12T16:23:57Z

...kages/knative-plugin/src/components/traffic-splitting/TrafficModalRevisionsDropdownField.tsx

+  const dropdownItems =
+    !field.value || Object.values(revisionItems).includes(field.value)
+      ? revisionItems
+      : set(cloneDeep(revisionItems), [field.value], field.value);


nit: we could simply do this and remove lodash imports.

Suggested change

: set(cloneDeep(revisionItems), [field.value], field.value);

: {...revisionItems, [field.value] : field.value};

karthikjeeyar · 2020-08-12T16:26:46Z

frontend/packages/knative-plugin/src/components/traffic-splitting/TrafficSplittingFields.tsx

+  const selectedRevisions: string[] = values.trafficSplitting.reduce((acc, currentValue) => {
+    acc.push(currentValue.revisionName);
+    return acc;
+  }, []);


Suggested change

const selectedRevisions: string[] = values.trafficSplitting.reduce((acc, currentValue) => {

acc.push(currentValue.revisionName);

return acc;

}, []);

const selectedRevisions: string[] = values.trafficSplitting.map((traffic) => traffic.revisionName);

nemesis09 · 2020-08-13T02:35:40Z

it works fine , just one if same revision added traffic twice via cli then in modal i can see add enabled and dropdown is empty

fixed

nemesis09 · 2020-08-13T09:22:54Z

/test e2e-gcp-console

invincibleJai · 2020-08-17T12:38:09Z

/lgtm
/approve

Verified the changes, seems to work as expected

openshift-bot · 2020-08-17T22:53:33Z

/retest