Fix validation in the storage modal mount path #3236

DaoDaoNoCode · 2024-09-20T19:15:12Z

JIRA: https://issues.redhat.com/browse/RHOAIENG-7680

Description

There is something wrong with the logic in the helper text and what error text we should show. I made some changes to that to correct the logic. Also, change the regex to allow a trailing / in the path.

Initial state:

Trailing slash:

Duplicate path:

Empty input:

Invalid value (contains number):

Invalid value (slash is not the trailing slash):

How Has This Been Tested?

Create a cluster storage
Attach it to a workbench
Input in the mouth path field, check if it's allowing a trailing slash
Check all the error states as listed above

Test Impact

Updated the cypress test to check the error cases.

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.

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

DaoDaoNoCode · 2024-09-20T19:18:52Z

@andrewballantyne The bug you mentioned in #3231 (review) is not because of the connection path starts with a preexisting connection path, it's because you have a number 2 in your path, which is not allowed. The code had wrong logic of checking the error cases so it was showing an incorrect error message. The logic is fixed now in this PR.

andrewballantyne · 2024-09-20T19:27:15Z

That logic is likely wrong fwiw -- but I suppose this PR doesn't have to deal with that

codecov · 2024-09-20T20:06:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.08%. Comparing base (a509cf4) to head (15d1363).
Report is 15 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3236   +/-   ##
=======================================
  Coverage   85.07%   85.08%           
=======================================
  Files        1292     1292           
  Lines       28814    28813    -1     
  Branches     7749     7748    -1     
=======================================
+ Hits        24514    24516    +2     
+ Misses       4300     4297    -3

Files with missing lines	Coverage Δ
frontend/src/pages/projects/pvc/MountPathField.tsx	`100.00% <100.00%> (+21.05%)`	⬆️

... and 4 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 a509cf4...15d1363. Read the comment docs.

jpuzz0

Verified locally. Although you did not write the error messages of Must only consist of lower case letters and dashes and Mount folder is already in use for this workbench, I think these both are meant to end with periods.

Worth having @kaedward take a look at all of these screenshots of yours to be sure anyways. I don't consider this blocking.

frontend/src/pages/projects/pvc/MountPathField.tsx

jpuzz0 · 2024-09-23T17:48:00Z

frontend/src/pages/projects/pvc/MountPathField.tsx

@@ -37,8 +37,8 @@ const MountPathField: React.FC<MountPathFieldProps> = ({
          onChange={(e, value) => {
            let error = '';
            if (value.length === 0) {
-              error = 'Required';
-            } else if (!/^[a-z-]+$/.test(value)) {
+              error = 'Enter a path to a model or folder. This path cannot point to a root folder.';


Was this text been provided by UX? Maybe @kaedward could verify this tech content.

Yeah I just moved the old text here https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/projects/pvc/MountPathField.tsx#L55 to above because the helper text render logic was incorrect before.

Gkrumbach07 · 2024-09-25T18:27:04Z

@DaoDaoNoCode it will except a duplicate path if you put a just a trailing slash, but will reject without it or if you add another folder to the end of the trailing slash

DaoDaoNoCode · 2024-09-25T18:33:05Z

@Gkrumbach07 Reading the comment that @andrewballantyne left in the ticket:

I read the issue fast and missed your final remark – I was on a time crunch in the triage meeting and this was one of our last ones we were trying to get through.

I'll update my UX ping and just leave this on the backlog. I didn't know `/something/` was valid and I would think that this is equiv to `/something` anyways.

I think /something is different from /something/, did I understand wrongly? 🤔

andrewballantyne · 2024-09-25T18:39:33Z

Read the last line of the description

The trailing slash is still valid for the notebook object and should not throw an error in the UI. If you manually update the notebook object with a trailing slash it works perfectly fine.

Allow them to add a slash if they want.

Gkrumbach07 · 2024-09-25T18:52:08Z

if

/something is different from /something/

is true then this looks good, otherwise needs to be fixed?

DaoDaoNoCode · 2024-09-25T18:55:00Z

@Gkrumbach07 I tested with both /something and /something/ set as the path and it works well, I think the notebook itself could handle the path. If that's not the case we can refine it later I guess.

Gkrumbach07 · 2024-09-25T19:36:27Z

no it can handle the path. i am saying it allows me to create two pvcs one with /something and the other with /something/... both attached to the same workbench. i tested it and it wont let the notebook start

DaoDaoNoCode · 2024-09-25T19:42:40Z

@Gkrumbach07 My notebook start and running

Gkrumbach07 · 2024-09-25T21:51:16Z

i still cannot replicate it. Are you restarting the workbench pods after you add both PVCs to it

DaoDaoNoCode · 2024-09-25T21:55:34Z

@Gkrumbach07 Hmm, I added it to a stopped notebook, and click toggle the start manually.

Gkrumbach07 · 2024-09-25T22:04:42Z

well i cant get a workbench to spawn at all im pretty sure so something is wrong on my end. I wont hold this PR up then

/lgtm
/approve

openshift-ci · 2024-09-25T22:04:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gkrumbach07, jpuzz0

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

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

pnaik1 · 2024-09-26T07:14:46Z

/retest

openshift-ci bot requested review from lucferbux and mturley September 20, 2024 19:15

DaoDaoNoCode mentioned this pull request Sep 20, 2024

allow for pvc to be added to multiple workbenches #3231

Merged

6 tasks

DaoDaoNoCode force-pushed the jira-rhoaieng-7680 branch from 7320c9e to a5ceb94 Compare September 20, 2024 19:25

jpuzz0 approved these changes Sep 23, 2024

View reviewed changes

openshift-ci bot assigned jpuzz0 Sep 23, 2024

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

DaoDaoNoCode force-pushed the jira-rhoaieng-7680 branch from a5ceb94 to 90dae40 Compare September 23, 2024 20:36

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

Fix validation in the storage modal mount path

15d1363

DaoDaoNoCode force-pushed the jira-rhoaieng-7680 branch from 90dae40 to 15d1363 Compare September 23, 2024 20:39

openshift-ci bot assigned Gkrumbach07 Sep 25, 2024

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

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix validation in the storage modal mount path #3236

Fix validation in the storage modal mount path #3236

DaoDaoNoCode commented Sep 20, 2024 •

edited

Loading

DaoDaoNoCode commented Sep 20, 2024

andrewballantyne commented Sep 20, 2024

codecov bot commented Sep 20, 2024 •

edited

Loading

jpuzz0 left a comment

jpuzz0 Sep 23, 2024

DaoDaoNoCode Sep 23, 2024

Gkrumbach07 commented Sep 25, 2024 •

edited

Loading

DaoDaoNoCode commented Sep 25, 2024

andrewballantyne commented Sep 25, 2024 •

edited

Loading

Gkrumbach07 commented Sep 25, 2024

DaoDaoNoCode commented Sep 25, 2024

Gkrumbach07 commented Sep 25, 2024

DaoDaoNoCode commented Sep 25, 2024

Gkrumbach07 commented Sep 25, 2024

DaoDaoNoCode commented Sep 25, 2024

Gkrumbach07 commented Sep 25, 2024

openshift-ci bot commented Sep 25, 2024

pnaik1 commented Sep 26, 2024

Fix validation in the storage modal mount path #3236

Fix validation in the storage modal mount path #3236

Conversation

DaoDaoNoCode commented Sep 20, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

DaoDaoNoCode commented Sep 20, 2024

andrewballantyne commented Sep 20, 2024

codecov bot commented Sep 20, 2024 • edited Loading

Codecov Report

jpuzz0 left a comment

Choose a reason for hiding this comment

jpuzz0 Sep 23, 2024

Choose a reason for hiding this comment

DaoDaoNoCode Sep 23, 2024

Choose a reason for hiding this comment

Gkrumbach07 commented Sep 25, 2024 • edited Loading

DaoDaoNoCode commented Sep 25, 2024

andrewballantyne commented Sep 25, 2024 • edited Loading

Gkrumbach07 commented Sep 25, 2024

DaoDaoNoCode commented Sep 25, 2024

Gkrumbach07 commented Sep 25, 2024

DaoDaoNoCode commented Sep 25, 2024

Gkrumbach07 commented Sep 25, 2024

DaoDaoNoCode commented Sep 25, 2024

Gkrumbach07 commented Sep 25, 2024

openshift-ci bot commented Sep 25, 2024

pnaik1 commented Sep 26, 2024

DaoDaoNoCode commented Sep 20, 2024 •

edited

Loading

codecov bot commented Sep 20, 2024 •

edited

Loading

Gkrumbach07 commented Sep 25, 2024 •

edited

Loading

andrewballantyne commented Sep 25, 2024 •

edited

Loading