Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: phase 1 of supporting multiple email addresses #4977

Merged
merged 1 commit into from
Jul 20, 2017

Conversation

tgxworld
Copy link
Contributor

No description provided.

@discoursebot
Copy link

You've signed the CLA, tgxworld. Thank you! This pull request is ready for review.

@tgxworld tgxworld force-pushed the LeoMcA-alternate-emails-phase-1 branch 9 times, most recently from 1d35b91 to ba39643 Compare July 18, 2017 07:46
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/additional-email-support/59847/6

@tgxworld
Copy link
Contributor Author

Main ruby tests are passing but our plugin tests are broken.

@@ -32,7 +32,7 @@ class UserExists < StandardError; end
def user_doesnt_already_exist
@email_already_exists = false
return if email.blank?
u = User.find_by("email = ?", Email.downcase(email))
u = User.find_by_email(Email.downcase(email))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we doing Email.downcase two times ? (multiple instances of this in the commit)

https://github.com/discourse/discourse/blob/master/app/models/user.rb#L235

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! 👍

@@ -98,11 +98,9 @@ def self.create_invite_by_email(email, invited_by, opts=nil)
group_ids = opts[:group_ids]
send_email = opts[:send_email].nil? ? true : opts[:send_email]
custom_message = opts[:custom_message]

lower_email = Email.downcase(email)
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 be needed? (see other comments)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for the subsequent queries

@@ -71,7 +71,7 @@ def self.confirm(token)
end

if user
return User.find_by(email: Email.downcase(user.email)) if Invite.redeem_from_email(user.email).present?
return User.find_by_email(Email.downcase(user.email)) if Invite.redeem_from_email(user.email).present?
Copy link
Contributor

Choose a reason for hiding this comment

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

Email.downcase probably not needed here

@tgxworld tgxworld force-pushed the LeoMcA-alternate-emails-phase-1 branch 5 times, most recently from 3c7f60c to e7a5eb7 Compare July 20, 2017 02:13
@tgxworld tgxworld force-pushed the LeoMcA-alternate-emails-phase-1 branch from e7a5eb7 to d0b027d Compare July 20, 2017 02:34
@tgxworld tgxworld merged commit a56e98b into discourse:master Jul 20, 2017
@tgxworld tgxworld deleted the LeoMcA-alternate-emails-phase-1 branch July 20, 2017 02:35
@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/the-github-linkback-plugin/66081/7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants