-
Notifications
You must be signed in to change notification settings - Fork 76
feat(FR-1220): Add duplicate check logic for session and service names #3937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
---|---|---|---|
🔴 | Statements | 4.52% (-0.01% 🔻) |
492/10883 |
🔴 | Branches | 4.02% (-0.01% 🔻) |
306/7616 |
🔴 | Functions | 2.63% (-0% 🔻) |
91/3462 |
🔴 | Lines | 4.48% (-0.01% 🔻) |
477/10646 |
Show new covered files 🐣
St.❔ |
File | Statements | Branches | Functions | Lines |
---|---|---|---|---|---|
🔴 | ... / useValidateSessionName.tsx |
0% | 0% | 0% | 0% |
🔴 | ... / useValidateServiceName.tsx |
0% | 0% | 0% | 0% |
Test suite run success
159 tests passing in 15 suites.
Report generated by 🧪jest coverage report action from 0a257b9
53dd8e6
to
a99992d
Compare
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has required the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like a good idea to make it possible to tell from the PR title what the PR fixes. Example. Add name duplication check before creating session and service
resources/i18n/en.json
Outdated
"ServiceCreated": "Model service {{ name }} has been created successfully.", | ||
"ServiceDelegatedFrom": "Model service is created by {{ createdUser }} but the session ownership will be delegated to {{ sessionOwner }}", | ||
"ServiceEndpoint": "Service Endpoint", | ||
"ServiceInfo": "Service Info", | ||
"ServiceName": "Service Name", | ||
"ServiceName": "Service name", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use uppercase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified this because everywhere else uses 'service name'. so, can i change all instances to uppercase for consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're using uppercase in the service form, so I think it's good to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. you mean fix only that instance into uppercase? i'll fix that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix here. @hummingbbird
a99992d
to
dec2fe2
Compare
I did it because of the character limit. I'll try to edit it like that😃 |
dec2fe2
to
f4a30fe
Compare
f4a30fe
to
66fb7ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make custom hooks for validation rules of session and service names
react/src/components/ComputeSessionNodeItems/EditableSessionName.tsx
Outdated
Show resolved
Hide resolved
f157499
to
330e05a
Compare
330e05a
to
85ce962
Compare
resources/i18n/en.json
Outdated
"ServiceCreated": "Model service {{ name }} has been created successfully.", | ||
"ServiceDelegatedFrom": "Model service is created by {{ createdUser }} but the session ownership will be delegated to {{ sessionOwner }}", | ||
"ServiceEndpoint": "Service Endpoint", | ||
"ServiceInfo": "Service Info", | ||
"ServiceName": "Service Name", | ||
"ServiceName": "Service name", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix here.
85ce962
to
7cf9a49
Compare
…e to service and session name input
7cf9a49
to
b775ea5
Compare
resources/i18n/en.json
Outdated
"ServiceCreated": "Model service {{ name }} has been created successfully.", | ||
"ServiceDelegatedFrom": "Model service is created by {{ createdUser }} but the session ownership will be delegated to {{ sessionOwner }}", | ||
"ServiceEndpoint": "Service Endpoint", | ||
"ServiceInfo": "Service Info", | ||
"ServiceName": "Service Name", | ||
"ServiceName": "Service name", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix here. @hummingbbird
7297c6f
to
9485f3e
Compare
9485f3e
to
96a761c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please re-request. review after reviewing my commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
96a761c
to
1503ba8
Compare
1503ba8
to
9637f37
Compare
6738bae
to
a769c8d
Compare
a769c8d
to
0a257b9
Compare
Resolves #3920 (FR-1220)
description
update validation messages for session/service names.
Modify ambiguous expressions
update the validation logic to clarify ambiguous expressions.
add validation logic for duplicated name
add a function to check for duplicate names using an existing GraphQL query.
to reviewers
Although many files were changed, most of them are i18n files, so I committed them all at once.