Github username taken #1865

jykae · 2016-11-02T15:02:48Z

jykae · 2016-11-02T15:04:02Z

works, but I clean it tomorrow

brylie · 2016-11-03T08:12:37Z

users/client/lib/router.js

+// Redirect to profile page if user doesn't have username
+// Eg. logged in with Github & username already taken
+const redirectToProfile = function () {
+  if (Meteor.userId() && !Meteor.user().username) {


Would the following work here instead?

if (Meteor.user() && Meteor.user().username !== undefined) { ... };

or, for example:

// Get user, if logged in - otherwise `null` const user = Meteor.user(); if (user && user.username != undefined) { ... };

The !== undefined is more explicit

since we are already relying on Meteor.user(), why introduce Meteor.userId()?

E.g. keep the code consistent by using only one Meteor.method()

storing the user object, or undefined, in a temporary variable might also reduce the noise here

only use Meteor.. once

only one function call

Meteor.user() should return null if user is not logged in

@brylie everything else I agree here but that explicitly saying "!== undefined", it should be common knowledge for JavaScript developer that it checks case "undefined", imho.

brylie · 2016-11-03T08:14:54Z

This is a nice improvement, and is generally very readable. 😎

marla-singer · 2016-11-03T08:39:03Z

@jykae Form allows me update profile without username

marla-singer · 2016-11-03T08:39:47Z

@jykae And can't navigate to Catalog and Add api page via navbar

brylie · 2016-11-03T08:40:52Z

@jykae we should also notify the user why they have been redirected to the profile page, if this is not already happening. Otherwise, it would be confusing:

why does it keep showing me the profile page:question:

-- confused user 😕

marla-singer · 2016-11-03T08:42:03Z

@brylie These has sAlert

brylie · 2016-11-03T08:43:40Z

Ah, I need to set my username, before I can add my "Gift Requests API" 🎁 ❗

-- santa claus 🎅

marla-singer · 2016-11-03T08:45:56Z

@brylie In PR #1859 I suggest to add popup like we have on Reset password. Maybe create another issue to improve an user usability?

brylie · 2016-11-03T08:51:43Z

Cool, it looks like this PR is headed in the right direction. ✈️

jykae · 2016-11-03T10:24:40Z

@marla-singer this logic setting for username if it is already taken has been like this for about year, as it was designed to function like this.
This PR was basically meant to fix just redirect as route for profile has been changed when we have improved settings, and some cleanup for router code.

If we need rethink usability add new issue.

marla-singer · 2016-11-03T10:35:10Z

@jykae Okay, put aside popup and user usability. But form allows user to update information with empty value

jykae · 2016-11-03T10:36:20Z

@marla-singer yep, I'm checking that one, good find 👍

jykae · 2016-11-03T10:43:34Z

Interesting we have separate template for username that does not seem to be included anywhere https://github.com/apinf/platform/tree/develop/users/client/account/username O_o

jykae · 2016-11-03T11:00:57Z

Also one other find, username also has different requirements on registration vs. profile form

brylie · 2016-11-03T11:08:06Z

Cool. Set optional: false in UserSchema.

jykae · 2016-11-03T11:27:40Z

@brylie I really hope it would be that simple. That solution provides "Internal server error" already in Github registration/login phase, when trying to create user, as username is required.

jykae · 2016-11-03T12:27:24Z

@marla-singer now it doesn't allow empty username, but shows "Please set username" 2 times on first redirect to profile. :) I can't see where this bug hides..

jykae · 2016-11-03T13:25:01Z

@marla-singer ready for review

thanks @brylie for bug-catching eyeglasses 8-)

marla-singer · 2016-11-03T13:57:53Z

@jykae I'll merge both branch on local and test

marla-singer · 2016-11-03T14:14:19Z

@jykae Detected problem and can't understand why.

Close all tab with site
Go to site again

Found from my side when router to site in first time:

Empty main page, only navbar
After refresh page, it's appeared for logined user
After refresh page, it is not so for anonymous user
After log in or navigate to Catalog, main page is appeared for anonymous user

Do you have something like this?

jykae · 2016-11-04T07:22:28Z

@marla-singer sorry, moved this.next outside if-statement, what about now?

brylie · 2016-11-04T07:27:53Z

users/client/lib/router.js

@@ -35,16 +42,38 @@ Router.route('/settings/profile', {
  template: 'profile',
 });

+// Redirect to profile page if user doesn't have username
+// Eg. logged in with Github & username already taken
+const checkUsername = function () {


This is an improvement in the function name. However, lets be just slightly more explicit, with something like:

ensureUsernameExists

ensureUserHasUsername

Using 'ensure' means that we are 'making sure' the user has a username, and is consistent with the ensureSignedIn function name.

brylie · 2016-11-04T07:31:00Z

users/client/profile/profile.js

@@ -1,5 +1,21 @@
 AutoForm.hooks({
  updateProfile: {
+    before: {
+      update (user) {


Can't we do this easier by making username: { optional: false } in the user profile schema?

jykae · 2016-11-04T08:49:50Z

@brylie Different simplified approach, took away unnecessary complexity, cleaned up, ready for review.

If user does not have username, redirect to profile

9a5ebb3

jykae added the in progress label Nov 2, 2016

Lint, cleanup

2feed54

jykae added the Ready for review label Nov 3, 2016

brylie reviewed Nov 3, 2016

View reviewed changes

jykae mentioned this pull request Nov 3, 2016

Cases for local & github registration #1859

Merged

brylie self-assigned this Nov 3, 2016

brylie removed the Ready for review label Nov 3, 2016

brylie assigned marla-singer and unassigned brylie Nov 3, 2016

More human friendly & descriptive style

d418786

Ville Jyrkkä added 3 commits November 3, 2016 14:19

Check username, move info stuff to router

4a22052

info user, add comments

fd9756a

patch wounds

9a25abd

change to redirect, don't return on signout

5abebe8

workaround Iron Router bug, rename function as requested

6d72f34

jykae added the Ready for review label Nov 3, 2016

Move this.next outside if-statement

38b86f1

brylie reviewed Nov 4, 2016

View reviewed changes

Ville Jyrkkä added 8 commits November 4, 2016 09:31

Refactor function name

5f82371

Deprecate unused template

a9d8aa2

simplify

f999961

Username required, lint

70ee649

Add prefix, if clashes with existing username

8c5ac5b

Router hack not needed

15b8704

Move autoform

f164b2f

lint

7e93be9

jykae force-pushed the bugfix/github-username-taken branch from f97c258 to 7e93be9 Compare November 4, 2016 08:42

Solve conflict with accounts_hooks

bab0bce

jykae force-pushed the bugfix/github-username-taken branch from 2ed2154 to bab0bce Compare November 4, 2016 08:46

brylie merged commit 0f1127e into develop Nov 4, 2016

brylie deleted the bugfix/github-username-taken branch November 4, 2016 10:10

brylie removed in progress Ready for review labels Nov 4, 2016

brylie self-assigned this Nov 4, 2016

jykae added this to the Sprint 34 milestone Nov 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Github username taken #1865

Github username taken #1865

jykae commented Nov 2, 2016

jykae commented Nov 2, 2016

brylie Nov 3, 2016 •

edited

jykae Nov 3, 2016 •

edited

brylie commented Nov 3, 2016

marla-singer commented Nov 3, 2016

marla-singer commented Nov 3, 2016 •

edited

brylie commented Nov 3, 2016 •

edited

marla-singer commented Nov 3, 2016

brylie commented Nov 3, 2016 •

edited

marla-singer commented Nov 3, 2016

brylie commented Nov 3, 2016 •

edited

jykae commented Nov 3, 2016

marla-singer commented Nov 3, 2016

jykae commented Nov 3, 2016

jykae commented Nov 3, 2016

jykae commented Nov 3, 2016

brylie commented Nov 3, 2016

jykae commented Nov 3, 2016

jykae commented Nov 3, 2016

jykae commented Nov 3, 2016 •

edited

marla-singer commented Nov 3, 2016

marla-singer commented Nov 3, 2016 •

edited

jykae commented Nov 4, 2016

brylie Nov 4, 2016

brylie Nov 4, 2016 •

edited

jykae commented Nov 4, 2016

Github username taken #1865

Github username taken #1865

Conversation

jykae commented Nov 2, 2016

jykae commented Nov 2, 2016

brylie Nov 3, 2016 • edited

Choose a reason for hiding this comment

jykae Nov 3, 2016 • edited

Choose a reason for hiding this comment

brylie commented Nov 3, 2016

marla-singer commented Nov 3, 2016

marla-singer commented Nov 3, 2016 • edited

brylie commented Nov 3, 2016 • edited

marla-singer commented Nov 3, 2016

brylie commented Nov 3, 2016 • edited

marla-singer commented Nov 3, 2016

brylie commented Nov 3, 2016 • edited

jykae commented Nov 3, 2016

marla-singer commented Nov 3, 2016

jykae commented Nov 3, 2016

jykae commented Nov 3, 2016

jykae commented Nov 3, 2016

brylie commented Nov 3, 2016

jykae commented Nov 3, 2016

jykae commented Nov 3, 2016

jykae commented Nov 3, 2016 • edited

marla-singer commented Nov 3, 2016

marla-singer commented Nov 3, 2016 • edited

jykae commented Nov 4, 2016

brylie Nov 4, 2016

Choose a reason for hiding this comment

brylie Nov 4, 2016 • edited

Choose a reason for hiding this comment

jykae commented Nov 4, 2016

brylie Nov 3, 2016 •

edited

jykae Nov 3, 2016 •

edited

marla-singer commented Nov 3, 2016 •

edited

brylie commented Nov 3, 2016 •

edited

brylie commented Nov 3, 2016 •

edited

brylie commented Nov 3, 2016 •

edited

jykae commented Nov 3, 2016 •

edited

marla-singer commented Nov 3, 2016 •

edited

brylie Nov 4, 2016 •

edited