Skip to content
This repository has been archived by the owner on Feb 23, 2019. It is now read-only.

Use real UUID for obfuscating username #216

Merged
merged 3 commits into from Apr 16, 2018

Conversation

yli-sugarcrm
Copy link
Contributor

Closes #215

Copy link
Contributor

@rhoggSugarcrm rhoggSugarcrm left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for your first PR to Thorn @yli-sugarcrm!

Left some nitpicky comments, but otherwise looks great!

Please send back to me when you've addressed these.

tests/index.js Outdated
'[a-fA-F0-9]{4}',
'4{1}[a-fA-F0-9]{3}',
'[89abAB]{1}[a-fA-F0-9]{3}',
'[a-fA-F0-9]{12}'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please place a trailing comma at the end of this line.

For future reference, you can run gulp lint before submitting a PR to make sure your code is style-guide compliant.

tests/index.js Outdated
let uuidv4Format = [
'[a-fA-F0-9]{8}',
'[a-fA-F0-9]{4}',
'4{1}[a-fA-F0-9]{3}',
Copy link
Contributor

Choose a reason for hiding this comment

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

We can delete any {1}'s as they're redundant.

tests/index.js Outdated
'[a-fA-F0-9]{8}',
'[a-fA-F0-9]{4}',
'4{1}[a-fA-F0-9]{3}',
'[89abAB]{1}[a-fA-F0-9]{3}',
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarification: the 89AB is the variant part of the RFC 4122 UUID. Per the spec:

Msb0  Msb1  Msb2  Description
    0     x     x    Reserved, NCS backward compatibility.
    1     0     x    The variant specified in this document.
    1     1     0    Reserved, Microsoft Corporation backward compatibility
    1     1     1    Reserved for future definition.

8, 9, A, and B are the only hexadecimal digits that start with 10 in binary; anything higher than that starts with 11 and anything lower than that starts with 01 or 00.

@coveralls
Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage increased (+0.01%) to 95.977% when pulling 6546ce9 on yli-sugarcrm:uuid-username-suffix into 895c508 on sugarcrm:master.

@rhoggSugarcrm rhoggSugarcrm dismissed their stale review September 29, 2017 17:34

Yuyi addressed my comments

Copy link
Contributor

@rhoggSugarcrm rhoggSugarcrm left a comment

Choose a reason for hiding this comment

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

Thanks @yli-sugarcrm!

@jcsmorais
Copy link
Contributor

Username have a maximum length of 60 chars, if we're now using UUID's (which take more than half-of that, 36 chars) should we add something to the documentation to explicitly say these should be under 24 chars?

@coveralls
Copy link

coveralls commented Oct 10, 2017

Coverage Status

Coverage increased (+0.01%) to 95.977% when pulling 41c9502 on yli-sugarcrm:uuid-username-suffix into 9a43d47 on sugarcrm:master.

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.

None yet

4 participants