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

Auto-generate username if none provided during registration #470

Merged
merged 3 commits into from
May 31, 2018

Conversation

APwhitehat
Copy link
Contributor

@APwhitehat APwhitehat commented May 26, 2018

Auto-generate username if none provided during registration.
Reserve numeric user IDs.

fixes #400.
Signed-off-by: Anant Prakash <anantprakashjsr@gmail.com>

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Tiny things, and then of course converting the goroutine to just using a postgres sequence as was discussed out-of-band.

@@ -403,6 +406,18 @@ func Register(
sessionID = util.RandomString(sessionIDLength)
}

// Don't allow numeric username. But it's impractical to reserve numerics outside the range of int.
Copy link
Member

Choose a reason for hiding this comment

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

"numeric username" -> "numeric usernames"

Also do you mean by "it's impractical to reserve numerics outside the range of int"? Just that we don't need to worry about > MaxInt?

JSON: jsonerror.InvalidUsername("Numeric user IDs are reserved"),
}
}
// auto generate a numeric username if r.Username is empty
Copy link
Member

Choose a reason for hiding this comment

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

"auto" -> "Auto"

@APwhitehat APwhitehat force-pushed the auto_generate_numericids branch from b478f4b to 1f668b3 Compare May 30, 2018 16:39
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

one typo. Tested and everything else looks good 👍

sessionID = util.RandomString(sessionIDLength)
}

// Don't allow numeric usernames less than MAX_INT64.
Copy link
Member

Choose a reason for hiding this comment

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

greater than? :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No ?
We are using numbers less than int_max 64, so only disallow numbers less than the same.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, for users trying to register a username. Thought this was referencing generated usernames.

if r.Username == "" {
id, err := accountDB.GetNewNumericLocalpart(req.Context())
if err != nil {
return jsonerror.InternalServerError()
Copy link
Member

Choose a reason for hiding this comment

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

We use return httputil.LogThenError(req, err) so that it includes an appropriate error message in the response.

@anoadragon453
Copy link
Member

@APwhitehat Could you rebase on master please?

@APwhitehat
Copy link
Contributor Author

APwhitehat commented May 31, 2018

aye aye

edit: Sorry, I have a habit of rebasing instead of merge. :P

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.

Registration: Allow registration without a username
3 participants