Feature/email settings change accounts ui #1770

brylie · 2016-10-12T11:01:47Z

Closes #1725
Closes #1727
Closes #1719
Closes #1742

Changes

conditionally change Accounts Templates links and features based on email settings
- showForgotPasswordLink
- showResendVerificationEmailLink
move login_verify.js to users module
- fix startup.js import
- refactor and lint file
- remove call to loginAttemptVerifier setting from AutoForm
fix env.MAIL_URL template string to remove newlines

Rationale

We probably only need to set the loginAttemptVerifier on startup, and update the function to handle our use cases:

email settings enabled
email settings not enabled

bajiat · 2016-10-13T10:18:25Z

@marla-singer Can you mentor and review?

marla-singer · 2016-10-13T10:19:14Z

@bajiat Yes

marla-singer · 2016-10-13T11:25:27Z

@brylie A suggestion received from @jykae about admin verification.
There are checking if user is in role admin, not updating verified if it is not really verified.
Use Roles.userIsInRole(userId, ['admin']); in this part

platform/core/helper_functions/login_verify.js

Line 4 in 24a1f16

// Find user email & callback check if it's verified

jykae · 2016-10-13T11:33:29Z

settings/server/methods.js

@@ -9,6 +9,24 @@ import { loginAttemptVerifier } from '/core/helper_functions/login_verify';
 import { Settings } from '../collection';

 Meteor.methods({
+  configureSmtpSettings (settings) {


Remember to call this function on startup to set MAIL_URL env variable if process is killed and restarted.

We are not currently doing that. Is it an issue right now?

marla-singer · 2016-10-13T13:22:06Z

users/server/login_verify.js

+    }
+
+    // If email is verified and parameters.allowed is true, user login is allowed
+    if (verified && parameters.allowed) {


Can't understand why do you check verified and parameters.allowed? If user has verified as true and allowed as false, why login not allowed?

@marla-singer I am not sure what parameters.allowed means. Where does it come from? It was part of the code before I started working here.

@brylie I dont' know too. But looks very strange

marla-singer · 2016-10-13T13:37:56Z

@brylie Don't forget to add condition for showForgotPasswordLink and showResendVerificationEmailLink. Now it isn't so.

brylie · 2016-10-13T13:43:55Z

Thanks @marla-singer. Right now, I am not even able to get verification emails to send, so need to work on the links after sending works.

brylie · 2016-10-13T13:54:28Z

I am getting a server console error when creating a new user, with email validation enabled:

Exception in setTimeout callback: Error: connect ECONNREFUSED 104.130.177.23:25
     at Object.Future.wait (/home/brylie/.meteor/packages/meteor-tool/.1.4.1_1.139xb76++os.linux.x86_64+web.browser+web.cordova/mt-os.linux.x86_64/dev_bundle/server-lib/node_modules/fibers/future.js:449:15)
     at smtpSend (packages/email/email.js:89:1)
     at Object.Email.send (packages/email/email.js:192:1)
     at AccountsServer.Accounts.sendVerificationEmail (packages/accounts-password/password_server.js:802:9)
     at [object Object].sendRegistrationEmailVerification (users/server/methods.js:34:16)
     at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1711:12)
     at packages/ddp-server/livedata_server.js:1624:18
     at [object Object]._.extend.withValue (packages/meteor/dynamics_nodejs.js:56:1)
     at Server.apply (packages/ddp-server/livedata_server.js:1623:45)
     at Server.call (packages/ddp-server/livedata_server.js:1566:17)
     - - - - -
     at Object.exports._errnoException (util.js:907:11)
     at exports._exceptionWithHostPort (util.js:930:20)
     at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1077:14)

This may be related to the Mailgun SMTP port. I have tried:

25
465
587

brylie · 2016-10-13T14:19:48Z

The above console error may be related to the email from_address.

http://stackoverflow.com/questions/32104047/meteor-cant-send-emails-sendererror-mail-from-command-failed-501-syntax-er

However, since I am not calling Email.send directly, I am not sure how to properly set the from_address for Accounts.sendVerificationEmail().

jykae · 2016-10-13T14:21:02Z

@brylie 587 did work for Mailgun when I worked with it earlier.
http://blog.mailgun.com/25-465-587-what-port-should-i-use/

marla-singer · 2016-10-13T14:27:25Z

@brylie Sending emails work! I used the free russian smtp server and got email 👍 But it was appeared in spam

jykae · 2016-10-13T14:29:40Z

Looks like you found fix, but anyway FYI, https://github.com/meteor/meteor/blob/87681c8f166641c6c3e34958032a5a070aa2d11a/packages/accounts-password/password_server.js#L740

marla-singer · 2016-10-13T14:38:30Z

@brylie "Email address for sending emails" isn't updated in users emails.
For instance, the first values was old_mail@mail.com, the second one is new_mail@mail.com
After changing I created another user, but got email from old_mail@mail.com.

marla-singer · 2016-10-13T14:42:13Z

Try to login as un-exist user.

Found: Message with text "Interval server error" and server console error Exception while invoking method 'login' TypeError: Cannot read property '_id' of undefined

Expected: Message with text "Login forbidden"

brylie · 2016-10-13T14:44:44Z

Try to login as un-exist user.

Found: Message with text "Interval server error" and console error Exception while invoking method 'login' TypeError: Cannot read property '_id' of undefined

Expected: Message with text "Login forbidden"

@marla-singer please file the last comment as a new issue. It may not be in the scope of this issue.

brylie · 2016-10-13T14:45:22Z

"Email address for sending emails" isn't updated in users emails.

That might be fixed in commit a5f5b4c

marla-singer · 2016-10-13T14:46:21Z

please file the last comment as a new issue. It may not be in the scope of this issue

We have message with text "Login forbidden" on nightly and staging but don't in this branch

brylie · 2016-10-13T14:50:18Z

I get login forbidden when I try logging in as a non-existant user.

marla-singer · 2016-10-13T14:51:47Z

Okay, maybe something's wrong on my side

brylie · 2016-10-13T14:52:50Z

Cool. I am not sure what it might be, but will gladly fix it if it is a bug I've introduced.

marla-singer · 2016-10-13T15:00:33Z

Great! Commit a5f5b4c fixed problem

jykae · 2016-10-13T15:01:46Z

@brylie AccountsTemplates might need init after setting them, if we change them.
meteor-useraccounts/core#159

brylie · 2016-10-13T15:02:04Z

Note: we cannot configure AccountsTemplates settings after they have been initialized:

Update mail configuration: Error: Configuration options must be set before AccountsTemplates.init!

We have to figure out a different solution, as part of another PR.

bajiat · 2016-10-13T15:03:05Z

We will not fix the links for this release. We must have email enabled
anyway for our production version.

jykae · 2016-10-13T15:04:09Z

yep, agreed

marla-singer · 2016-10-13T15:12:58Z

@brylie "Verification email lost?" works, "Reset password" works.
These links didn't disappear when I disabled mail settings, but let's create another issue for that
Merge?

bajiat · 2016-10-13T15:14:20Z

Yes, we need to create another issue for that. And can't fix it for
Mindtrek. Is everything else working now?

brylie · 2016-10-14T06:46:55Z

I opened an upstream support request, asking how to change AccountsTemplates configuration after it has been initialized:

meteor-useraccounts/core#722

brylie added 2 commits October 12, 2016 13:58

Disable forgot password link/route

7dedff9

Disable resend verification email link

cce13fb

brylie added the in progress label Oct 12, 2016

brylie added this to the Sprint 33 milestone Oct 12, 2016

brylie added 3 commits October 12, 2016 14:36

Split updateMailConfiguration into multiple methods

aaf36a0

Cleanup console log; add comments

ecce54b

Cleanup unneeded route configuration

70230ee

brylie assigned marla-singer Oct 13, 2016

jykae reviewed Oct 13, 2016

View reviewed changes

brylie added 12 commits October 13, 2016 15:20

Confirm password

fd3b19e

Remove call to loginAttemptVerifier

79d936e

Remove validateAdminUserEmails

3731990

Remove loginAttemptVerifier import

3a86a6b

Simplify countUsers method

ad61dab

Move file

06e65a8

Change loginAttemptVerifier import

101b8da

Refactor and lint

4107bf0

Remove unused method

3d9f0d8

Remove unused method

3595170

Add admin role check

12ddb93

Get user details from parameters.user object

1f6489f

marla-singer reviewed Oct 13, 2016

View reviewed changes

brylie added 2 commits October 13, 2016 16:27

Fix SMTP MAIL_URL template string (remove newlines)

b5ce486

Lint

2a3a417

Set from address so mail can send

a5f5b4c

brylie added 2 commits October 13, 2016 17:42

Enable/disable accounts emails based on settings

3378032

sendVerificationEmail disabled by default

c2194b0

brylie added 2 commits October 13, 2016 17:59

Add notes about functions not working

cf3706f

Set email verification and password reset to true

bd79fba

brylie added the Ready for review label Oct 13, 2016

marla-singer merged commit d463e0f into develop Oct 13, 2016

marla-singer removed in progress Ready for review labels Oct 13, 2016

marla-singer deleted the feature/email-settings-change-accounts-ui branch October 13, 2016 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/email settings change accounts ui #1770

Feature/email settings change accounts ui #1770

brylie commented Oct 12, 2016 •

edited

bajiat commented Oct 13, 2016

marla-singer commented Oct 13, 2016

marla-singer commented Oct 13, 2016

jykae Oct 13, 2016

brylie Oct 13, 2016

marla-singer Oct 13, 2016 •

edited

brylie Oct 13, 2016 •

edited

marla-singer Oct 13, 2016

marla-singer commented Oct 13, 2016

brylie commented Oct 13, 2016

brylie commented Oct 13, 2016 •

edited

brylie commented Oct 13, 2016

jykae commented Oct 13, 2016

marla-singer commented Oct 13, 2016

jykae commented Oct 13, 2016

marla-singer commented Oct 13, 2016 •

edited

marla-singer commented Oct 13, 2016 •

edited

brylie commented Oct 13, 2016 •

edited

brylie commented Oct 13, 2016

marla-singer commented Oct 13, 2016 •

edited

brylie commented Oct 13, 2016 •

edited

marla-singer commented Oct 13, 2016

brylie commented Oct 13, 2016

marla-singer commented Oct 13, 2016

jykae commented Oct 13, 2016

brylie commented Oct 13, 2016

bajiat commented Oct 13, 2016 •

edited

jykae commented Oct 13, 2016

marla-singer commented Oct 13, 2016 •

edited

bajiat commented Oct 13, 2016 •

edited

brylie commented Oct 14, 2016

Feature/email settings change accounts ui #1770

Feature/email settings change accounts ui #1770

Conversation

brylie commented Oct 12, 2016 • edited

Changes

Rationale

bajiat commented Oct 13, 2016

marla-singer commented Oct 13, 2016

marla-singer commented Oct 13, 2016

jykae Oct 13, 2016

Choose a reason for hiding this comment

brylie Oct 13, 2016

Choose a reason for hiding this comment

marla-singer Oct 13, 2016 • edited

Choose a reason for hiding this comment

brylie Oct 13, 2016 • edited

Choose a reason for hiding this comment

marla-singer Oct 13, 2016

Choose a reason for hiding this comment

marla-singer commented Oct 13, 2016

brylie commented Oct 13, 2016

brylie commented Oct 13, 2016 • edited

brylie commented Oct 13, 2016

jykae commented Oct 13, 2016

marla-singer commented Oct 13, 2016

jykae commented Oct 13, 2016

marla-singer commented Oct 13, 2016 • edited

marla-singer commented Oct 13, 2016 • edited

brylie commented Oct 13, 2016 • edited

brylie commented Oct 13, 2016

marla-singer commented Oct 13, 2016 • edited

brylie commented Oct 13, 2016 • edited

marla-singer commented Oct 13, 2016

brylie commented Oct 13, 2016

marla-singer commented Oct 13, 2016

jykae commented Oct 13, 2016

brylie commented Oct 13, 2016

bajiat commented Oct 13, 2016 • edited

jykae commented Oct 13, 2016

marla-singer commented Oct 13, 2016 • edited

bajiat commented Oct 13, 2016 • edited

brylie commented Oct 14, 2016

brylie commented Oct 12, 2016 •

edited

marla-singer Oct 13, 2016 •

edited

brylie Oct 13, 2016 •

edited

brylie commented Oct 13, 2016 •

edited

marla-singer commented Oct 13, 2016 •

edited

marla-singer commented Oct 13, 2016 •

edited

brylie commented Oct 13, 2016 •

edited

marla-singer commented Oct 13, 2016 •

edited

brylie commented Oct 13, 2016 •

edited

bajiat commented Oct 13, 2016 •

edited

marla-singer commented Oct 13, 2016 •

edited

bajiat commented Oct 13, 2016 •

edited