List connected notebooks for each connection table row #3256

jeff-phillips-18 · 2024-09-25T20:13:16Z

Description

Adds connected notebooks to the connection table rows.
Lists connected notebooks when deleting a connection.

How Has This Been Tested?

Tested manually.
Added test scenarios

Test Impact

Test the connection table to verify the related notebooks show up in each row
Test the delete connection modal to verify the related notebooks are listed.

Screen shots

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.

/cc @simrandhaliw

christianvogt · 2024-09-25T21:43:14Z

fyi @emilys314

codecov · 2024-09-26T12:28:19Z

Codecov Report

Attention: Patch coverage is 51.35135% with 36 lines in your changes missing coverage. Please review.

Project coverage is 84.89%. Comparing base (a0d40fd) to head (033e04d).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
frontend/src/concepts/design/utils.ts	14.81%	23 Missing ⚠️
frontend/src/concepts/design/TypedObjectIcon.tsx	41.17%	10 Missing ⚠️
...eens/detail/connections/ConnectionsDeleteModal.tsx	86.66%	2 Missing ⚠️
.../screens/detail/connections/ConnectedResources.tsx	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3256      +/-   ##
==========================================
- Coverage   84.96%   84.89%   -0.08%     
==========================================
  Files        1302     1306       +4     
  Lines       29101    29173      +72     
  Branches     7828     7859      +31     
==========================================
+ Hits        24727    24766      +39     
- Misses       4374     4407      +33

Files with missing lines	Coverage Δ
frontend/src/images/icons/ModelsIcon.ts	`100.00% <100.00%> (ø)`
...pages/projects/notebook/ConnectedNotebookNames.tsx	`94.11% <ø> (ø)`
.../src/pages/projects/notebook/useNotebooksStates.ts	`100.00% <100.00%> (ø)`
...src/pages/projects/notebook/useRelatedNotebooks.ts	`93.87% <100.00%> (ø)`
frontend/src/pages/projects/pvc/DeletePVCModal.tsx	`85.18% <ø> (ø)`
...cts/screens/detail/connections/ConnectionsList.tsx	`100.00% <ø> (ø)`
...ts/screens/detail/connections/ConnectionsTable.tsx	`100.00% <100.00%> (ø)`
...screens/detail/connections/ConnectionsTableRow.tsx	`96.15% <ø> (ø)`
...jects/screens/detail/connections/ResourceLabel.tsx	`100.00% <100.00%> (ø)`
...ail/data-connections/DeleteDataConnectionModal.tsx	`79.41% <ø> (ø)`
... and 9 more

... 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 a0d40fd...033e04d. Read the comment docs.

emilys314 · 2024-09-26T16:03:28Z

frontend/src/pages/projects/screens/detail/connections/__tests__/ConnectionsTable.spec.tsx

@@ -35,5 +48,30 @@ describe('ConnectionsTable', () => {
    expect(screen.getByText('desc1')).toBeTruthy();
    expect(screen.queryByText('s3')).toBeFalsy();
    expect(screen.getByText('S3 Buckets')).toBeTruthy();
+    expect(screen.getByText('S3 Buckets')).toBeTruthy();


Duplicate line added. We can remove line 51

emilys314 · 2024-09-26T16:12:46Z

frontend/src/pages/projects/screens/detail/connections/ConnectedResources.tsx

+  if (!connectedNotebooks.length) {
+    return '-';
+  }
+
+  if (!notebooksLoaded) {
+    return <Spinner size="sm" />;
+  }


I don't know if the <Spinner /> will ever return? If it's not loaded, the connectedNotbooks will always be [], so it would always return '-' while loading?

So I think these 2 need to be switched around?

emilys314 · 2024-09-26T16:23:36Z

frontend/src/pages/projects/screens/detail/connections/ConnectionsDeleteModal.tsx

+                        key={notebook.metadata.name}
+                        data-testid="connections-delete-notebooks-item"
+                      >
+                        {getDisplayNameFromK8sResource(notebook)}


Looking at the mocks, I believe we need to append " (Running)" to the display name, if notebook.running === true

emilys314 · 2024-09-27T13:29:37Z

/lgtm

christianvogt · 2024-09-27T13:59:36Z

/approve

openshift-ci · 2024-09-27T13:59:42Z

[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

openshift-ci bot requested a review from simrandhaliw September 25, 2024 20:13

jeff-phillips-18 force-pushed the connected-workbenches branch from f23a25c to 04fc681 Compare September 25, 2024 20:14

jeff-phillips-18 requested review from christianvogt and emilys314 September 25, 2024 20:31

jeff-phillips-18 force-pushed the connected-workbenches branch from 04fc681 to b3682d7 Compare September 26, 2024 11:46

emilys314 reviewed Sep 26, 2024

View reviewed changes

jeff-phillips-18 force-pushed the connected-workbenches branch from b3682d7 to ced3d3d Compare September 27, 2024 11:40

List connected notebooks for each connection table row

033e04d

jeff-phillips-18 force-pushed the connected-workbenches branch from ced3d3d to 033e04d Compare September 27, 2024 12:46

openshift-ci bot assigned emilys314 Sep 27, 2024

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

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

openshift-merge-bot bot merged commit 947c09b into opendatahub-io:main Sep 27, 2024
8 checks passed

jeff-phillips-18 deleted the connected-workbenches branch September 27, 2024 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List connected notebooks for each connection table row #3256

List connected notebooks for each connection table row #3256

jeff-phillips-18 commented Sep 25, 2024 •

edited

Loading

christianvogt commented Sep 25, 2024

codecov bot commented Sep 26, 2024 •

edited

Loading

emilys314 Sep 26, 2024

emilys314 Sep 26, 2024

emilys314 Sep 26, 2024

emilys314 commented Sep 27, 2024

christianvogt commented Sep 27, 2024

openshift-ci bot commented Sep 27, 2024

List connected notebooks for each connection table row #3256

List connected notebooks for each connection table row #3256

Conversation

jeff-phillips-18 commented Sep 25, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Screen shots

Request review criteria:

christianvogt commented Sep 25, 2024

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

emilys314 Sep 26, 2024

Choose a reason for hiding this comment

emilys314 Sep 26, 2024

Choose a reason for hiding this comment

emilys314 Sep 26, 2024

Choose a reason for hiding this comment

emilys314 commented Sep 27, 2024

christianvogt commented Sep 27, 2024

openshift-ci bot commented Sep 27, 2024

jeff-phillips-18 commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading