-
-
Notifications
You must be signed in to change notification settings - Fork 662
Fix Server URL field to be editable #3256
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: develop
Are you sure you want to change the base?
Conversation
ajhollid
left a comment
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.
Thanks for your contribution thusfar!
A couple of changes needed on the frontend, noted in review.
There is a bigger issue on the server side, which isn't caused by your PR but will require changes to your PR once resolved, so we may as well do it all here.
await editMonitorBodyValidation.validateAsync(req.body);
const editedMonitor = await this.monitorService.editMonitor({ teamId, monitorId, body: req.body });
The body is validated, but then the original unvalidated body is passed to the monitor service.
We should pass the validated body along:
const validatedBody = await editMonitorBodyValidation.validateAsync(req.body);
const editedMonitor = await this.monitorService.editMonitor({ teamId, monitorId, body: validatedBody });
And that will require updating the validation schema to allow the URL field, which is not currently allowed.
The reason the PR works currently is that the body is effectively not validated; if we did validate it properly, it would fail to update the URL as the value would be stripped.
We should be good to go after that 👍
| /> | ||
| )} | ||
| {isEditMode && | ||
| generalSettingsConfig.showUrl && |
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.
You can remove the showUrl condition, that is not used on the the config page.
| )} | ||
| {isEditMode && | ||
| generalSettingsConfig.showUrl && | ||
| watchedUrl !== existingMonitor?.url && ( |
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.
This causes the alert to show and then hide when the page loads, as the watchedUrl does not equal the existingMonitor.url initially but does once loaded.
| {isEditMode && | ||
| generalSettingsConfig.showUrl && | ||
| watchedUrl !== existingMonitor?.url && ( | ||
| <Alert severity="warning">{t("urlUpdateWarning")}</Alert> |
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.
Pleaes put this string under pages.createMonitor.form.general.url.alert in en.json
Describe your changes
Fixes inability to edit the "Server URL" field.
Write your issue number after "Fixes "
Fixes #3100
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.