Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Standardize errors about localpart structure #2351

Merged
merged 2 commits into from
Dec 14, 2018

Conversation

turt2live
Copy link
Member

Fixes element-hq/element-web#5833

This also includes changing some Jira references that aren't searchable anymore, and a thing to replace the spinner on the SetMxidDialog as per element-hq/element-web#5833 (comment)

@turt2live turt2live requested a review from a team December 13, 2018 23:06
@turt2live turt2live force-pushed the travis/fix-username-requirements branch from a493025 to 4fdcafb Compare December 13, 2018 23:17
Fixes element-hq/element-web#5833

This also includes changing some Jira references that aren't searchable anymore, and a thing to replace the spinner on the SetMxidDialog as per element-hq/element-web#5833 (comment)
@turt2live turt2live force-pushed the travis/fix-username-requirements branch from 4fdcafb to 8592e76 Compare December 13, 2018 23:45
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Thanks, the change looks good. 😁

Please investigate the CI failure before merging... E2E tests seem to fail on the first step. Is the test checking from some of this text?

@turt2live
Copy link
Member Author

Looks like the regex is failing on the login page and that's causing it to not continue normally.

Going to have to spend some time figuring this one out though:

SAFE_LOCALPART_REGEX.test("lowercase")
true
!SAFE_LOCALPART_REGEX.test("lowercase")
true

@jryans
Copy link
Collaborator

jryans commented Dec 14, 2018

Going to have to spend some time figuring this one out though:

SAFE_LOCALPART_REGEX.test("lowercase")
true
!SAFE_LOCALPART_REGEX.test("lowercase")
true

Yikes... I guess parens do the right thing:

!(SAFE_LOCALPART_REGEX.test("lowercase"))
false

but yeah... that's pretty strange. 😰

@jryans
Copy link
Collaborator

jryans commented Dec 14, 2018

Oh wait, the test method is stateful somehow...?

SAFE_LOCALPART_REGEX.test("lowercase")
true
SAFE_LOCALPART_REGEX.test("lowercase")
false
SAFE_LOCALPART_REGEX.test("lowercase")
true
SAFE_LOCALPART_REGEX.test("lowercase")
false

@jryans
Copy link
Collaborator

jryans commented Dec 14, 2018

Ah, it's the global flag on the regex. If you remove that, it works:

const SAFE_LOCALPART_REGEX = /^[a-z0-9=_\-./]+$/;
undefined
SAFE_LOCALPART_REGEX.test("lowercase")
true
SAFE_LOCALPART_REGEX.test("lowercase")
true
SAFE_LOCALPART_REGEX.test("lowercase")
true

Regular expression objects are stateful, and the global flag interferes badly with the expression. A valid call can cause it to act like a flip flop instead of a stateless test.
@turt2live
Copy link
Member Author

Huh, I didn't realize that the expression actually stores state (and that the global flag interferes that badly). Thanks for taking a look into it - I've committed the change.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants