Edit for existing project connection instances #3250

emilys314 · 2024-09-24T19:48:45Z

https://issues.redhat.com/browse/RHOAIENG-11556

Description

Hooks up the edit kebab action to the manageConnectionModal. it will pass in the connection of that row, and pass isEdit. It will then prefill connection data.

The secret stores data in base64, so atob() is used to decode, this is done in the modal itself (which is similar to the data connections). This makes it easier to reference a corresponding connection type, to determine how to decode the value.

If a connection has env var names that do not match up to a connection type field, they will be appended to the bottom as short text fields.

The multi dropdown is stored as a JSON array, and decoded as such.

The "Save" button isn't enabled until the field values have been updated.

Additionally the k8sNameDesc is now intractable in the preview to mimic the actual form.

How Has This Been Tested?

Tested locally.
Just click the kebab menu on an existing connection row and click "Edit". You can update the values and click Save.

To test non matching values, you modify the env var for the field from the connection type, or edit it directly in openshift

Test Impact

A cypress test added to check the edit functionally
Jest tests to test the modal in edit form

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.
@simrandhaliw

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

openshift-ci · 2024-09-24T19:48:49Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2024-09-24T20:31:54Z

Codecov Report

Attention: Patch coverage is 93.26923% with 7 lines in your changes missing coverage. Please review.

Project coverage is 84.90%. Comparing base (ca27d1d) to head (5959ccd).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/concepts/connectionTypes/ConnectionTypeForm.tsx	84.00%	4 Missing ⚠️
frontend/src/concepts/connectionTypes/utils.ts	89.65%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3250      +/-   ##
==========================================
+ Coverage   84.85%   84.90%   +0.04%     
==========================================
  Files        1299     1299              
  Lines       28919    29001      +82     
  Branches     7770     7807      +37     
==========================================
+ Hits        24539    24623      +84     
+ Misses       4380     4378       -2

Files with missing lines	Coverage Δ
...onnectionTypes/fields/ConnectionTypeFormFields.tsx	`100.00% <100.00%> (ø)`
...8sNameDescriptionField/K8sNameDescriptionField.tsx	`100.00% <100.00%> (ø)`
.../k8s/K8sNameDescriptionField/ResourceNameField.tsx	`100.00% <100.00%> (ø)`
...cts/screens/detail/connections/ConnectionsList.tsx	`100.00% <100.00%> (ø)`
...ts/screens/detail/connections/ConnectionsTable.tsx	`100.00% <100.00%> (ø)`
...eens/detail/connections/ManageConnectionsModal.tsx	`93.47% <100.00%> (+4.08%)`	⬆️
.../projects/screens/detail/data-connections/utils.ts	`97.67% <100.00%> (+0.05%)`	⬆️
frontend/src/concepts/connectionTypes/utils.ts	`92.72% <89.65%> (-1.18%)`	⬇️
...rc/concepts/connectionTypes/ConnectionTypeForm.tsx	`88.23% <84.00%> (-0.66%)`	⬇️

... and 5 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca27d1d...5959ccd. Read the comment docs.

christianvogt · 2024-09-24T20:42:04Z

frontend/src/concepts/connectionTypes/ConnectionTypeForm.tsx

@@ -111,7 +113,7 @@ const ConnectionTypeForm: React.FC<Props> = ({
                },
              }
            }
-            onDataChange={setConnectionNameDesc}
+            onDataChange={setConnectionNameDesc ?? (() => undefined)} // onDataChange needs to be truthy to show resource name


That's a strange requirement from K8sNameDescriptionField. Can we update it?

Okay it's been updated. I'm not 100% certain on all the use cases for the component, but i think if you want to old preview behavior, they can still pass in the k8sName.state.immutable = false

christianvogt · 2024-09-24T20:43:49Z

frontend/src/concepts/connectionTypes/fields/ConnectionTypeFormFields.tsx

+      const matching = fields?.find((f) => isConnectionTypeDataField(f) && f.envVar === key);
+      if (!matching) {
+        unmatched.push({
+          type: ConnectionTypeFieldType.ShortText,


we could be smart. If it contains \n make it long text, or if it is true|false make it a boolean. But otherwise I think this works.

christianvogt · 2024-09-24T20:47:18Z

frontend/src/concepts/connectionTypes/utils.ts

+        response[key] = JSON.parse(decodedString);
+      } catch {
+        response[key] = decodedString;


This is expected to be an array correct? If so, perhaps ensure it parsed correctly and fallback to [<decodedString>]?

christianvogt · 2024-09-25T13:09:51Z

frontend/src/concepts/connectionTypes/fields/ConnectionTypeFormFields.tsx

@@ -74,6 +91,7 @@ const ConnectionTypeFormFields: React.FC<Props> = ({
          <React.Fragment key={i}>{renderDataFields(fieldGroup.fields)}</React.Fragment>
        ),
      )}
+      {unmatchedValues.length > 0 && renderDataFields(unmatchedValues)}


@simrandhaliw when editing a connection instance where the env var has no matching field in the connection type, should we section these off with a header and description which informs the user of their situation?

christianvogt · 2024-09-25T13:13:51Z

frontend/src/concepts/connectionTypes/utils.ts

+      matchingField.properties.variant === 'multi'
+    ) {
+      try {
+        response[key] = JSON.parse(decodedString);


Is this overkill what I'm suggesting to ensure that the parsed value conforms to an array of strings?

Suggested change

response[key] = JSON.parse(decodedString);

const parsed = JSON.parse(decodedString);

if (Array.isArray(parsed)) {

response[key] = parsed.map(v => String(v));

} else {

response[key] = [decodedString];

}

emilys314 · 2024-09-25T20:58:19Z

updates to the connection type select

christianvogt · 2024-09-25T21:35:20Z

/lgtm
/approve

openshift-ci · 2024-09-25T21:35:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt

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

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

…tion in edit

emilys314 · 2024-09-26T13:01:57Z

rebasing to run CI again

christianvogt · 2024-09-26T14:30:22Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Sep 24, 2024

emilys314 marked this pull request as ready for review September 24, 2024 20:32

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Sep 24, 2024

openshift-ci bot requested review from lucferbux and pnaik1 September 24, 2024 20:32

christianvogt reviewed Sep 24, 2024

View reviewed changes

christianvogt reviewed Sep 25, 2024

View reviewed changes

emilys314 force-pushed the edit-project-connections-modal branch 2 times, most recently from da72b1d to e763060 Compare September 25, 2024 17:55

openshift-ci bot assigned christianvogt Sep 25, 2024

openshift-ci bot added the lgtm label Sep 25, 2024

openshift-ci bot added the approved label Sep 25, 2024

emilys314 added 4 commits September 26, 2024 09:01

Allow edit of existing project connections

94edcd0

Allow interactive preview of k8s resource name

37e5765

Hide 'connections' from 'data connections' + show disabled type selec…

264152e

…tion in edit

Connection modal typeahead enhancements

5959ccd

emilys314 force-pushed the edit-project-connections-modal branch from 805ace6 to 5959ccd Compare September 26, 2024 13:01

openshift-ci bot removed the lgtm label Sep 26, 2024

openshift-ci bot added the lgtm label Sep 26, 2024

openshift-merge-bot bot merged commit 6ca046e into opendatahub-io:main Sep 26, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit for existing project connection instances #3250

Edit for existing project connection instances #3250

emilys314 commented Sep 24, 2024 •

edited

Loading

openshift-ci bot commented Sep 24, 2024

codecov bot commented Sep 24, 2024 •

edited

Loading

christianvogt Sep 24, 2024

emilys314 Sep 25, 2024

christianvogt Sep 24, 2024

christianvogt Sep 24, 2024 •

edited

Loading

christianvogt Sep 25, 2024

christianvogt Sep 25, 2024

emilys314 commented Sep 25, 2024

christianvogt commented Sep 25, 2024

openshift-ci bot commented Sep 25, 2024

emilys314 commented Sep 26, 2024

christianvogt commented Sep 26, 2024

-        response[key] = JSON.parse(decodedString);
+        const parsed = JSON.parse(decodedString);
+        if (Array.isArray(parsed)) {
+          response[key] = parsed.map(v => String(v));
+        } else {
+          response[key] = [decodedString];
+        }

Edit for existing project connection instances #3250

Edit for existing project connection instances #3250

Conversation

emilys314 commented Sep 24, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Sep 24, 2024

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

christianvogt Sep 24, 2024

Choose a reason for hiding this comment

emilys314 Sep 25, 2024

Choose a reason for hiding this comment

christianvogt Sep 24, 2024

Choose a reason for hiding this comment

christianvogt Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

christianvogt Sep 25, 2024

Choose a reason for hiding this comment

christianvogt Sep 25, 2024

Choose a reason for hiding this comment

emilys314 commented Sep 25, 2024

christianvogt commented Sep 25, 2024

openshift-ci bot commented Sep 25, 2024

emilys314 commented Sep 26, 2024

christianvogt commented Sep 26, 2024

emilys314 commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

christianvogt Sep 24, 2024 •

edited

Loading