Skip to content
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

Require URL contains valid protocol in MDM settings end user migration form #19124

Merged
merged 1 commit into from
May 20, 2024

Conversation

gillespi314
Copy link
Contributor

Issue #18566

@gillespi314 gillespi314 requested a review from a team as a code owner May 17, 2024 19:41
@gillespi314
Copy link
Contributor Author

gillespi314 commented May 17, 2024

@RachelElysia, I think the issue found by QAWolf may stem from the changes made to frontend URL validation in this PR. The options for the new validator library are a bit confusing, but looking into the source, it seems that require_protocol must be set in order to check that a protocol is actually present. The protocols and require_valid_protocol only test validity of the protocol prefix if one is detected. Otherwise it considers "example.com" to be ok.

I haven't checked whether the issue might also be present on other pages. In the interest of time, I just imported the new validator library directly on this page so that I could set the additional options required for the validator to check for the presence of a protocol.

Since you are more familiar with the changes more generally, would you mind taking a look at the usage in other places to confirm it works as expected?

Copy link
Member

@roperzh roperzh left a comment

Choose a reason for hiding this comment

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

I wonder if there are any other places in the UI that need the protocol explicitly defined?

@gillespi314 gillespi314 merged commit 8dd3c70 into main May 20, 2024
9 checks passed
@gillespi314 gillespi314 deleted the 18556-mdm-webhook-url-validation-ui branch May 20, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants