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

Updated profile page #233

Merged
merged 32 commits into from
Sep 25, 2018
Merged

Updated profile page #233

merged 32 commits into from
Sep 25, 2018

Conversation

iambibhas
Copy link
Contributor

@iambibhas iambibhas commented Apr 19, 2018

Updated profile page design, with added option to

Also touched upon #189 and #190.

screen shot 2018-04-25 at 4 44 31 pm

@iambibhas
Copy link
Contributor Author

iambibhas commented May 8, 2018

One issue with the removal of external IDs we saw the other day - because twitter auth doesn't return email address, if someone deleted twitter extid, there doesn't seem to be any way to add it again. Twitter auth right now asks for email address after login. Which wont work for an existing account.

We need to add a "add external ID" button on profile page and link to currently logged in account when the user comes back from twitter login.

@jace
Copy link
Member

jace commented May 8, 2018

That should be in this PR: buttons for all login services should be included in an "add external id" section.

@jace
Copy link
Member

jace commented May 8, 2018

The buttons can link to the existing /login/<service> endpoints. Lastuser will add them to the current profile if the user is already logged in. It will also prompt the user to merge accounts if there's an overlap with another account.

@jace
Copy link
Member

jace commented May 8, 2018

Account merger reversal is discussed in #230. It'll become high priority once we include the option of adding external services as people will start accidentally merging accounts.

# XXX: What if, the user is logged in and the "first" email address
# returned by the external ID belongs to another user account in our DB?
# In that case, current_auth.user != useremail.user
# https://github.com/hasgeek/lastuser/blob/profile_page/lastuser_oauth/providers/github.py#L70
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jace need your comment for this part.

# This is possible when user is adding an external ID from the profile page.
# useremail can be None if the "first" email of the external ID doesn't match
# any useremail record.
user = current_auth.user
Copy link
Member

Choose a reason for hiding this comment

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

This else clause is unnecessary. We already handle this.

Copy link
Contributor Author

@iambibhas iambibhas May 31, 2018

Choose a reason for hiding this comment

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

@jace this only works if the email address of the external ID is already in the db and connected to the currently logged in user. But if the external ID returns a completely new email address, it crashes because there is no useremail record for it.

Copy link
Member

Choose a reason for hiding this comment

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

Don't make any changes here. Changes should be in login_service_postcallback, which calls this function.

@@ -171,6 +171,14 @@ def add_email(self, email, primary=False, type=None, private=False):
self.primary_email = useremail
return useremail

def make_email_primary(self, email):
Copy link
Member

Choose a reason for hiding this comment

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

This method doesn't need to exist. self.primary_email is already safe to assign to. Always load the UserEmail object first if you don't have it.

return email
username, domain = email.split('@')
masked_username = username.replace(username[1:], '*' * 4)
masked_domain = domain.replace(domain[1:], '*' * 4)
Copy link
Member

Choose a reason for hiding this comment

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

return '%s****@%s****' % (username[0], domain[0])

# This is possible when user is adding an external ID from the profile page.
# useremail can be None if the "first" email of the external ID doesn't match
# any useremail record.
user = current_auth.user
Copy link
Member

Choose a reason for hiding this comment

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

Don't make any changes here. Changes should be in login_service_postcallback, which calls this function.

@@ -58,7 +59,7 @@ def login():
return set_loginmethod_cookie(render_redirect(get_next_url(session=True), code=303),
'password')
except LoginPasswordResetException:
return render_redirect(url_for('.reset', expired=1, username=loginform.username.data))
return render_redirect(url_for('.reset', nopasswd=1, username=loginform.username.data))
Copy link
Member

Choose a reason for hiding this comment

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

Let's change this to not pass status in request.args, but as a flash message.

@@ -172,6 +173,9 @@ def reset():
if getbool(request.args.get('expired')):
message = _(u"Your password has expired. Please enter your username "
"or email address to request a reset code and set a new password")
elif getbool(request.args.get('nopasswd')):
message = _(u"Your account does not have a password set. Please enter your username "
"or email address to request a reset code and set a new password")
Copy link
Member

Choose a reason for hiding this comment

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

Remove this and just depend on the flash message. Remove the expired check as well unless there's an email that goes out when a password expires (can't use flash messages there).

@iambibhas
Copy link
Contributor Author

screenshot-2018-6-20_my_profile

@jace
Copy link
Member

jace commented Jun 21, 2018

The pending verification text should be a link to where the user can resend a verification link to themselves.

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

Successfully merging this pull request may close these issues.

None yet

3 participants