Skip to content

Issue 6481 - UI - When ports that are in use are used to update a DS instance the error message is not helpful #6482

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

Merged
merged 4 commits into from
Jan 14, 2025

Conversation

jchapma
Copy link
Contributor

@jchapma jchapma commented Jan 8, 2025

Bug description:
When updating port values on a DS instance, if the port value is already in use the error message displayed by the UI is not helpful.

Fix description:
Add a UI method that checks if the updated port value is already in use. If it is, disable the save button.

Fixes: #6481

Reviewed by:

…instance the error message is not helpful

Bug description:
When updating port values on a DS instance, if the port value is already in use
the error message displayed by the UI is not helpful.

Fix description:
Add a UI method that checks if the updated port value is already in use.
If it is, disable the save button.

Fixes: 389ds#6481

Reviewed by:
Copy link
Contributor

@mreynolds389 mreynolds389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to move the validation code from the state change function to validateSaveBtn() function, otherwise the NumberInput will not have the validated prop set correctly.

@jchapma
Copy link
Contributor Author

jchapma commented Jan 8, 2025

You need to move the validation code from the state change function to validateSaveBtn() function, otherwise the NumberInput will not have the validated prop set correctly.

Initially I had the validation code in the validateSaveBtn function, then if the user was say decrementing to a port value and came across a port that was in use the -- button was disabled and the user couldn't go any lower. So I thought a better way was to do the validation in minus/plus/change functions, set the errObj for ('nsslapd-port') as true if the port was in use, then check errObj in validateSaveBtn. Or am i missing something ?

@droideck
Copy link
Member

droideck commented Jan 9, 2025

Initially I had the validation code in the validateSaveBtn function, then if the user was say decrementing to a port value and came across a port that was in use the -- button was disabled and the user couldn't go any lower. So I thought a better way was to do the validation in minus/plus/change functions, set the errObj for ('nsslapd-port') as true if the port was in use, then check errObj in validateSaveBtn. Or am i missing something ?

I think validateSaveBtn should disable Save Settings button, and it should not affect plus or minus buttons...

@jchapma
Copy link
Contributor Author

jchapma commented Jan 9, 2025

Initially I had the validation code in the validateSaveBtn function, then if the user was say decrementing to a port value and came across a port that was in use the -- button was disabled and the user couldn't go any lower. So I thought a better way was to do the validation in minus/plus/change functions, set the errObj for ('nsslapd-port') as true if the port was in use, then check errObj in validateSaveBtn. Or am i missing something ?

I think validateSaveBtn should disable Save Settings button, and it should not affect plus or minus buttons...

Ok, let me move the validation back to validateSaveBtn

@mreynolds389
Copy link
Contributor

Almost there :-)

This diff gets the NumberInput working as expected

diff --git a/src/cockpit/389-console/src/lib/server/settings.jsx b/src/cockpit/389-console/src/lib/server/settings.jsx
index 5de56dac6..568fd1a53 100644
--- a/src/cockpit/389-console/src/lib/server/settings.jsx
+++ b/src/cockpit/389-console/src/lib/server/settings.jsx
@@ -311,6 +311,10 @@ async validateSaveBtn(nav_tab, attr, value) {
                         const portInUse = await is_port_in_use(portValue);
                         if (portInUse) {
                             disableSaveBtn = true;
+                            if (value !== this.state['_' + attr]) {
+                                // New value, but it's already being used
+                                valueErr = true;
+                            }
                         }
                     } catch (error) {
                         console.error("Error checking port:", error);

@mreynolds389
Copy link
Contributor

Sorry another issue... I also noticed that the "minus" buttons are disabled for port and secure port. That should be fixed as well. Thanks!

@jchapma
Copy link
Contributor Author

jchapma commented Jan 10, 2025

Sorry another issue... I also noticed that the "minus" buttons are disabled for port and secure port. That should be fixed as well. Thanks!

The issue of the minus button being disabled existed before I started this work, I had a look at it yesterday and it seems the values of nsslapd-port and nsslapd-secureport are undefined at render time and only defined after loadConfig() is called in componentDidMount(). Let me have another look at it.

@mreynolds389
Copy link
Contributor

Sorry another issue... I also noticed that the "minus" buttons are disabled for port and secure port. That should be fixed as well. Thanks!

The issue of the minus button being disabled existed before I started this work, I had a look at it yesterday and it seems the values of nsslapd-port and nsslapd-secureport are undefined at render time and only defined after loadConfig() is called in componentDidMount(). Let me have another look at it.

Sorry I was not saying it was your fault, but might as well fix it while you are working on that code.

@jchapma
Copy link
Contributor Author

jchapma commented Jan 10, 2025

Sorry another issue... I also noticed that the "minus" buttons are disabled for port and secure port. That should be fixed as well. Thanks!

The issue of the minus button being disabled existed before I started this work, I had a look at it yesterday and it seems the values of nsslapd-port and nsslapd-secureport are undefined at render time and only defined after loadConfig() is called in componentDidMount(). Let me have another look at it.

Sorry I was not saying it was your fault, but might as well fix it while you are working on that code.

Ye I knew you meant fix it while I was working on that area of code, thanks anyway though.

I had a look around and found a few more disabled "minus" button issues, how about I create an issue for these and fix them all together ?

Server
Server Settings -> General Settings -> LDAP Port, LDAPS Port,
Server Settings -> Disk Monitoring -> Disk Monitoring Threshold, Disk Monitoring Grace Period
Tuning and Limits -> Idle Connection Timeout, I/O Block Timeout
Logging -> Access Log Settings -> Maximum Number Of Logs
Logging -> Audit Log Settings -> Maximum Number Of Logs
Logging -> Audit Fail Log Settings -> Maximum Number Of Logs
Logging -> Error Log Settings -> Maximum Number Of Logs
Logging -> Security Log Settings -> Maximum Number Of Logs

Database
Global Database Configuration -> Limits -> ID List Scan Limit
Global Database Configuration -> NDN Cache -> Normalized DN Cache Max Size
Global Database Configuration -> Advanced Settings -> Database Max DBs

Copy link
Contributor

@mreynolds389 mreynolds389 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, since there are other places where the minus button is the same just open a new ticket then. Thanks, and ack!

Copy link
Member

@droideck droideck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@jchapma jchapma merged commit 7adcd8a into 389ds:main Jan 14, 2025
197 of 199 checks passed
jchapma added a commit that referenced this pull request Jan 14, 2025
…instance the error message is not helpful (#6482)

Bug description:
When updating port values on a DS instance, if the port value is already in use
the error message displayed by the UI is not helpful.

Fix description:
Add a UI method that checks if the updated port value is already in use.
If it is, disable the save button.

Fixes: #6481

Reviewed by: @mreynolds389, @droideck (Thank you)
jchapma added a commit that referenced this pull request Jan 14, 2025
…instance the error message is not helpful (#6482)

Bug description:
When updating port values on a DS instance, if the port value is already in use
the error message displayed by the UI is not helpful.

Fix description:
Add a UI method that checks if the updated port value is already in use.
If it is, disable the save button.

Fixes: #6481

Reviewed by: @mreynolds389, @droideck (Thank you)
jchapma added a commit that referenced this pull request Jan 14, 2025
…instance the error message is not helpful (#6482)

Bug description:
When updating port values on a DS instance, if the port value is already in use
the error message displayed by the UI is not helpful.

Fix description:
Add a UI method that checks if the updated port value is already in use.
If it is, disable the save button.

Fixes: #6481

Reviewed by: @mreynolds389, @droideck (Thank you)
jchapma added a commit that referenced this pull request Jan 14, 2025
…instance the error message is not helpful (#6482)

Bug description:
When updating port values on a DS instance, if the port value is already in use
the error message displayed by the UI is not helpful.

Fix description:
Add a UI method that checks if the updated port value is already in use.
If it is, disable the save button.

Fixes: #6481

Reviewed by: @mreynolds389, @droideck (Thank you)
@jchapma jchapma deleted the ui-ports branch April 2, 2025 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI - When ports that are in use are used to update a DS instance the error message is not helpful
3 participants