[NEW] Alert admins when user requires approval & alert users when the account is approved/activated/deactivated by luisfn · Pull Request #7098 · RocketChat/Rocket.Chat

luisfn · 2017-05-26T01:35:34Z

@RocketChat/core

The idea is to send a simple email to all admin users when an user register and needs approval to join. To acomplish this, a new field has been added where the user can fill a reason why he wants to join. The reason can be seen on user info screen and it is removed from user data after receive it's approval

CLAassistant · 2017-05-26T22:12:12Z

All committers have signed the CLA.

- Added method to save reason into user object - Added validation to display field but setting Accounts_ManuallyApproveNewUsers is not working :(

- Modified function placeholders to replace reason as well - Modified email template on accounts.js

- Reason displayed only if setting is active and user is not active - Added reason to getFullUserData

- Added method to save reason into user object - Added validation to display field but setting Accounts_ManuallyApproveNewUsers is not working :(

- Modified function placeholders to replace reason as well - Modified email template on accounts.js

- Reason displayed only if setting is active and user is not active - Added reason to getFullUserData

marceloschmidt · 2017-05-29T16:19:07Z

client/methods/unsetUserReason.js

+		Meteor.users.update(userId, { $unset: { 'reason' : 1 } });
+		return true;
+	}
+});


This method shouldn't exist. First, it is not checking for user permission to unset reason, and second you could unset reason in the same method setUserActiveStatus.

marceloschmidt · 2017-05-29T16:21:07Z

packages/rocketchat-lib/server/models/Users.js

 		this.tryEnsureIndex({ 'active': 1 }, { sparse: 1 });
 		this.tryEnsureIndex({ 'statusConnection': 1 }, { sparse: 1 });
 		this.tryEnsureIndex({ 'type': 1 });
+		this.tryEnsureIndex({ 'reason': 1 });


Do you really need this index? I don't think we'll be doing any query/sorting using reason.

No, just forgot it there. Removing

marceloschmidt · 2017-05-29T16:27:08Z

server/lib/accounts.js

 	}

+	if (!user.active) {
+		user.emails.some((email) => {


Why are you using .some((email) => { if you are not using the email variable and you are also not returning a true value to stop the some? what's the expected result?

No idea, was just folowing an example from another file sending email. Fixing

marceloschmidt · 2017-05-29T16:28:07Z

server/lib/accounts.js

+			const destinations = [];
+
+			RocketChat.models.Roles.findUsersInRole('admin').forEach(function(adminUser) {
+				destinations.push(`${ adminUser.name }<${ adminUser.emails[0].address }>`);


It's possible that an adminUser doesn't have e-mail on adminUser.emails[0].address (for example if the admin only ever logged in using OAuth). You should check if property exists.

marceloschmidt · 2017-05-29T16:31:17Z

server/methods/unsetUserReason.js

+
+		return false;
+	}
+});


Likewise on client side, this method should not exist and its content should be executed inside setUserActiveStatus

# Conflicts: # server/lib/accounts.js

Fixed the issues with the tests

rodrigok · 2018-01-30T19:53:13Z

Hi @luisfn could you fix the conflicts?

graywolf336 · 2018-01-30T22:29:52Z

@rodrigok This was opened in May of last year, we might as well fix the conflicts ourselves as that was a long time ago...

rodrigok · 2018-02-15T20:08:26Z

@graywolf336 yes, lets fix the conflicts our selfs

# Conflicts: # packages/rocketchat-i18n/i18n/en.i18n.json # packages/rocketchat-lib/lib/placeholders.js # packages/rocketchat-lib/server/models/Users.js # packages/rocketchat-ui-flextab/client/tabs/userInfo.html # packages/rocketchat-ui-flextab/client/tabs/userInfo.js

Addressed

graywolf336 · 2018-02-20T20:13:16Z

A few things, and this isn't related to this pull request directly, but whenever a user isn't activated and they try to login successfully (aka correct email and password) the error incorrectly says "invalid user or password" when the error returned via the websockets actually says error-user-is-not-activated. So, I think we need to display the screen again that states they are awaiting user registration.

graywolf336

Works fine when I tested locally. 👍

luisfn added 5 commits May 24, 2017 23:45

Changes for issue #976. Still need some work

d4495b3

Moved email strings to propper indexes on i18n

0255a9e

Created templates for subject and mail body

b90b3b7

Fix typo and code standards

7a93379

Modified to change email to all admins at once

a877603

luisfn changed the title ~~Issue 976~~ [FIX] Send email to admin when user requires approval May 26, 2017

luisfn changed the title ~~[FIX] Send email to admin when user requires approval~~ WIP - [FIX] Send email to admin when user requires approval May 26, 2017

Added reason field into register screen

21dcb15

luisfn added 18 commits May 27, 2017 00:37

- Added error message for invalid reason

8701eb0

- Added method to save reason into user object - Added validation to display field but setting Accounts_ManuallyApproveNewUsers is not working :(

- Changed to use the correct setting

7f5e498

- Modified admin message to contain reason and user name

c92d2b0

- Modified function placeholders to replace reason as well - Modified email template on accounts.js

- Added public key to use Accounts_ManuallyApproveNewUsers on frontend

eae8aa3

- Displaying reason on user info

16a58b5

- Reason displayed only if setting is active and user is not active - Added reason to getFullUserData

Added logic to remove user reason once user gets activated

3925e89

Added reason field into register screen

f001a4c

- Added error message for invalid reason

21c6c73

- Added method to save reason into user object - Added validation to display field but setting Accounts_ManuallyApproveNewUsers is not working :(

- Changed to use the correct setting

fad9497

- Modified admin message to contain reason and user name

17ad773

- Modified function placeholders to replace reason as well - Modified email template on accounts.js

- Added public key to use Accounts_ManuallyApproveNewUsers on frontend

db7d777

- Displaying reason on user info

0f99622

- Reason displayed only if setting is active and user is not active - Added reason to getFullUserData

Added logic to remove user reason once user gets activated

e6fff42

Removing unneeded comma

12988c4

Fix merge

3cf721f

Added conditionals to check and use reason field

7538f00

Trying to fix user creation tests

e7cd5eb

Trying to fix user creation tests(2)

777441e

geekgonecrazy mentioned this pull request May 29, 2017

[FIX] Send email to admins when user need to be approved to join #7090

Closed

marceloschmidt requested changes May 29, 2017

View reviewed changes

Applying last suggestions

c0edb09

luisfn and others added 7 commits June 1, 2017 00:02

Moving reason tests to right file

dfff89e

Moving reason tests to right file (2)

128116d

Fixing test where field was not found

645943a

Rebasing with develop and applied code suggestion

d5f141b

Merge remote-tracking branch 'origin/issue_976' into issue_976

c639292

# Conflicts: # server/lib/accounts.js

Fix the tests

1b564f4

fix error

244cb4c

engelgabriel requested a review from rodrigok January 21, 2018 23:23

engelgabriel assigned rodrigok Jan 21, 2018

engelgabriel added this to the 0.62.0 milestone Jan 21, 2018

RocketChat deleted a comment Feb 17, 2018

Code improvements

a9b004c

rodrigok changed the title ~~WIP - [FIX] Send email to admin when user requires approval~~ [NEW] Alert admins when user requires approval & alert users when the account is activated or deactivated Feb 17, 2018

rodrigok changed the title ~~[NEW] Alert admins when user requires approval & alert users when the account is activated or deactivated~~ [NEW] Alert admins when user requires approval & alert users when the account is activated/deactivated Feb 17, 2018

rodrigok requested a review from graywolf336 February 17, 2018 17:43

Alert by email when an account is approved/activated/deactivated

b2d99d1

rodrigok changed the title ~~[NEW] Alert admins when user requires approval & alert users when the account is activated/deactivated~~ [NEW] Alert admins when user requires approval & alert users when the account is approved/activated/deactivated Feb 17, 2018

graywolf336 approved these changes Feb 20, 2018

View reviewed changes

rodrigok merged commit e899d2d into RocketChat:develop Feb 20, 2018

rodrigok mentioned this pull request Feb 28, 2018

Release 0.62.0 #9935

Merged

rodrigok mentioned this pull request Mar 7, 2018

Suggestion for "Manually Approving Users" #5977

Closed

sampaiodiego mentioned this pull request Aug 2, 2018

Alert admin DM or Email when new user signs up #5945

Closed

Conversation

luisfn commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigok commented Jan 30, 2018

Uh oh!

graywolf336 commented Jan 30, 2018

Uh oh!

rodrigok commented Feb 15, 2018

Uh oh!

graywolf336 commented Feb 20, 2018

Uh oh!

graywolf336 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

luisfn commented May 26, 2017 •

edited

Loading

CLAassistant commented May 26, 2017 •

edited

Loading