Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Fix manual input token #3945

Merged
merged 3 commits into from
Dec 22, 2016
Merged

Fix manual input token #3945

merged 3 commits into from
Dec 22, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Dec 22, 2016

No description provided.

@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 22, 2016
</div>
);
}

onChangeToken = (event, token) => {
const validToken = /[a-zA-Z0-9]{4}-?[a-zA-Z0-9]{4}-?[a-zA-Z0-9]{4}-?[a-zA-Z0-9]{4}/.test(token);
const validToken = /[a-zA-Z0-9]{4}(-)?[a-zA-Z0-9]{4}(-)?[a-zA-Z0-9]{4}(-)?[a-zA-Z0-9]{4}/.test(token);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simplified to this:

/([a-z0-9]{4}(-)?){4}/i

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I can't reproduce why -? -> (-)?.

/([a-z0-9]{4}-?){4}/i.test('2M28-4gXT-X2qW-bAa') // false
/([a-z0-9]{4}-?){4}/i.test('2M28-4gXT-X2qW-bAaS') // true

Copy link
Contributor

Choose a reason for hiding this comment

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

trimming the input might be worth, as users often copy & paste adjacent spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can read and understand it as-is so the bug we introduced jumped out immediately. Since I can see no benefit in crunching apart from making me straining my brain & eyes, I'm not going to crush it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be at least /^[a-zA-Z0-9]{4}(-)?[a-zA-Z0-9]{4}(-)?[a-zA-Z0-9]{4}(-)?[a-zA-Z0-9]{4}$/

@ngotchac
Copy link
Contributor

1232M28-4gXT-X2qW-bAaz is still valid

@jacogr
Copy link
Contributor Author

jacogr commented Dec 22, 2016

The whole approach of having optional -'s is anyway questionable as logged. The output from parity ui new-token has the dashes, that is what we verify. If users copy from authcodes and play techy, they should know to add the -'s

@derhuerst
Copy link
Contributor

derhuerst commented Dec 22, 2016

@jacogr Not sure if you're responding to @ngotchac. afaict @ngotchac wants to add ^ and $ to the regex. His point is that the token may have invalid characters and still be valid right now.

@ngotchac
Copy link
Contributor

The thing is that the token input but the user gets modified anyway via token.replace(/[^a-zA-Z0-9]/g, ''). IMO, the check should be whether the transformed token is valid or not (ie. being 16 chars long). That's how it's stored in localstorage too.

@jacogr
Copy link
Contributor Author

jacogr commented Dec 22, 2016

We added the optional -'s due to an issue logged. My point is that issue is invalid since if you copy from authcodes, you should expect things to not work as you expected. We never direct any users there neither do we direct them to copy from storage. The ^ & $ is valid though, updated.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 85.756% when pulling 5859b8d on jg-fix-manual-token into be75cbf on master.

@jacogr jacogr changed the title Fix manual input token (invalid regex) Fix manual input token Dec 22, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 85.765% when pulling 5859b8d on jg-fix-manual-token into be75cbf on master.

@ngotchac ngotchac added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 22, 2016
@jacogr jacogr merged commit 203b419 into master Dec 22, 2016
@jacogr jacogr deleted the jg-fix-manual-token branch December 22, 2016 16:22
jacogr added a commit that referenced this pull request Jan 3, 2017
jacogr added a commit that referenced this pull request Jan 4, 2017
* Cleanups & tests for #3945

* Externalise icons as per PR comments
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants