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

[MM-53799] Stop autocompleting while the user is typing https:// #3202

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

devinbinnie
Copy link
Member

Summary

In circumstances where the user stopped typing, or was slow typing the beginning of their server URL, the URL validation would kick in and erroneously mistake the beginning of a URL (ie. any substring of https://) as a hostname, and try to add https:// for the user. This would cause some confusion and frustration for users trying to type.

This PR fixes the validation to recognize any leading substring of https:// as an invalid URL before it tries to call it a hostname. This stops the user from having to backspace and correct themselves while typing, and should still server to help people typing without the leading https://. Additionally, I added a QoL fix to stop the trailing / from appearing while you were typing a host name, so that if you do stop, you don't have to backspace the / first to keep typing the name.

Ticket Link

https://mattermost.atlassian.net/browse/MM-53799

Stop autocompleting while the user is typing `https://`

@devinbinnie devinbinnie added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Nov 11, 2024
@devinbinnie devinbinnie added this to the v5.11.0 milestone Nov 11, 2024
@devinbinnie devinbinnie requested review from hmhealey, yasserfaraazkhan and a team November 11, 2024 20:40
httpUrl = url.replace(/^((.+):\/\/)?/, 'https://');
} else {
// Otherwise add HTTPS for them
} else if (!url.match(/^(h|ht|htt|http|https):?\/?\/?$/)) {
Copy link
Member

Choose a reason for hiding this comment

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

Not a big fan of regexes. It's probably easier to write a quick sub-string prefix matching function that does this. 0/5

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say this is less code though, because then i'd have to check all of the above cases for substrings and it would be a mess of ORs. I usually try to avoid regexes as well these days, but I think for this it makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could flip the check around? Something like 'https://'.startsWith(url) seems like it would work, although we'd have to check both HTTP and HTTPS separately

Copy link
Member

Choose a reason for hiding this comment

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

I meant a generic function, not a mess of ORs ;) But looks like Harrison's suggestion would work as well. Checking for both http and https doesn't sound like too much overhead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Harrison's solution sounds very clean so yes I will implement that instead.

src/app/serverViewState.test.js Show resolved Hide resolved
@yasserfaraazkhan yasserfaraazkhan added the Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests label Nov 12, 2024
Copy link

Here are the test results below:

Test Summary for Linux on commit 8fb3ac4

The following known failed tests have been fixed on Linux:
- menu/view MM-T820 should open Developer Tools For Application Wrapper for main window

Test Summary for macOS on commit 8fb3ac4

New failed tests found on macOS:

  • LongServerName MM-T4050 Long server name

The following known failed tests have been fixed on macOS:
- popup MM-T2827_1 should be able to select all in popup windows

@github-actions github-actions bot removed the Run Desktop E2E Tests This label will trigger the workflow that runs e2e automation tests label Nov 12, 2024
Copy link
Contributor

@yasserfaraazkhan yasserfaraazkhan left a comment

Choose a reason for hiding this comment

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

might be a rare cases, that can occur when typing

  • typing Capital HTTPS, changes it into lower and adds https
Screenshot 2024-11-12 at 12 41 37 PM Screenshot 2024-11-12 at 12 43 58 PM
  • typing https and appending special charecters like " , ' will quickly parse it and append https. (for other special charecters it doent do this)
image

Verified creating, editing server url that when typing https the input field wont append additional https unless user types some other word

@devinbinnie
Copy link
Member Author

might be a rare cases, that can occur when typing

  • typing Capital HTTPS, changes it into lower and adds https

Will make a fix for this.

@devinbinnie devinbinnie requested a review from agnivade November 13, 2024 16:47
@devinbinnie devinbinnie removed the 3: QA Review Requires review by a QA tester label Nov 13, 2024
Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

Thanks!

@devinbinnie devinbinnie added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Nov 13, 2024
@devinbinnie devinbinnie enabled auto-merge (squash) November 13, 2024 16:56
@devinbinnie devinbinnie merged commit 453965c into master Nov 13, 2024
19 checks passed
@devinbinnie devinbinnie deleted the MM-53799 branch November 13, 2024 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants