Eamils for user actions #2137

DawoudIO · 2017-03-19T19:54:59Z

Emails
Apis

Closes #614 #1200
Subset of #2042

DawoudIO · 2017-03-20T04:42:07Z

Ready for review

crossan007

Some minor things that won't block the merge. #2158

crossan007 · 2017-03-25T15:25:45Z

src/ChurchCRM/Emails/users/AccountDeletedEmail.php

+    protected $password;
+
+    public function __construct($user, $password) {
+        $this->password = $password;


Why does this need password?

we're informing of a deleted account.

was this a copy/paste from the new account template?

Not blocking merge - tracking in #2158

crossan007 · 2017-03-25T15:26:07Z

src/ChurchCRM/Emails/users/AccountDeletedEmail.php

+    protected function buildMessageBody()
+    {
+        $msg = array();
+        array_push($msg, gettext("Your CRM Account was Deleted, if you think this is an error, please contact your admin"));


Not blocking merge for this, but it'd be nice to list the admin contact. Opened separate issue #2157

ya we need a html email wrapper around all these emails that has all the info.

crossan007 · 2017-03-25T15:26:43Z

src/ChurchCRM/Emails/users/BaseUserEmail.php

+
+    protected function buildMessageHeader()
+    {
+        return gettext("Dear") ." " . $this->user->getFullName();


This should be configurable, much like #1658 / #1673.

MVP - not blocking merge - see issue #2158

The call to $this->user->getFullName(); returns the user's salutation as well, which makes for an awkward email salutation:

I.e."

Dear Crossan, Mr. Charles

done now SystemConfig::getValue('sDear')

crossan007 · 2017-03-25T15:27:03Z

src/ChurchCRM/Emails/users/BaseUserEmail.php

+
+    protected function getLink()
+    {
+        return SystemURLs::getURL() . "Login.php?username=" . $this->user->getUserName();


hmm..... I like the convenience of pre-populating their username field.

However, It's not common (in my experience), are we opening up any vulnerabilities by allowing that parameter in the Login page?

all that it does is prefill the username...

crossan007 · 2017-03-25T15:27:20Z

src/ChurchCRM/Emails/users/NewAccountEmail.php

+
+    protected function getSubSubject()
+    {
+        return gettext("Your CRM Account");


"ChurchCRM"

crossan007 · 2017-03-25T15:27:31Z

src/ChurchCRM/Emails/users/NewAccountEmail.php

+        array_push($msg, gettext("A ChurchCRM account was created for you:"));
+        array_push($msg, "<a href='" . $this->getLink() . "'>" . gettext("Login"). "</a>");
+        array_push($msg, gettext('Username') . ": " . $this->user->getUserName());
+        array_push($msg, gettext('New Password') . ": " . $this->password);


Should probably just be "Password"

Let's reserve the use of "New Password" for instances where we are changing the password

Not blocking merge for this - tracking in #2158

crossan007 · 2017-03-25T15:28:16Z

src/ChurchCRM/Emails/users/ResetPasswordEmail.php

+
+    protected function getSubSubject()
+    {
+        return gettext("Your CRM New Password");


"Your New ChurchCRM Password"

Not blocking merge for this - #2158

crossan007 · 2017-03-25T15:29:58Z

src/api/routes/users.php

+
+    $this->post('/{userId:[0-9]+}/password/reset', function ($request, $response, $args) {
+        if (!$_SESSION['user']->isAdmin()) {
+            return $response->withStatus(401);


I think this should be an exception with a more descriptive error message since this will be displayed to the user via window.CRM.DisplayErrorMessage() as a result of $(document).ajaxError in https://github.com/ChurchCRM/CRM/blob/master/src/Include/Header-function.php#L153

See:
https://github.com/ChurchCRM/CRM/blob/master/src/api/routes/database.php#L18\

Not blocking merge for this - See #2163

crossan007 · 2017-03-25T15:30:09Z

src/api/routes/users.php

+        }
+        $user = UserQuery::create()->findPk($args['userId']);
+        if (!is_null($user)) {
+            $password = SystemConfig::getValue('sDefault_Pass');


this makes me cringe.

Not blocking merge - Let's explore in #2160

* based on feedback from #2137 (#2168) * send email when the user is locked * Apply fixes from StyleCI (#2175) * fix 1049

* added User APIs * added updatePassword * moved to ORM * setSearchLimit moved to ORM * Moved to ORM/APIs Moved List user to orm moved table to data-table moved delete user to model and APIs moved Reset failed login user to model and APIs moved reset passwrod user to model and APIs * added delete user null check * cleanup of Sunday School Session Settings removeing from the session to just reading the object * removed extra ; * isPasswordValid & hasReachedMaxFailedLogin * using hasReachedMaxFailedLogin * added missing SystemConfig * usr_UserName matching db * moved to new Logoff.php page * moved to Logoff.php * moved to Logoff.php * Refac - Moved logout to new page - moved password check into password - cleanned if bocks - cleanned browser check * renamed method to isLocked * added updatePasswordDefault * ORM and Select 2 * Apply fixes from StyleCI (#2050) * created hashPassword * removed unused user settings for mail * support passing username as a login.php param * fixed bug with isPasswordValid * cleanup * more function from person * User password reset email with password * fixed extra } * Apply fixes from StyleCI (#2132) * added a msg vs reload the page * fixed password reset ajax call * moved to ORM & sending emails * new system user emails all working * send an email when user is created * hashPassword is now public * sending emails for reset and unlock * Apply fixes from StyleCI (#2135) * send an email on account delete * added login link and username * extract password to a var * based on feedback from #2137 (#2168) * added emails when a user is locked... * moved lockedEmail to the correct block * Apply fixes from StyleCI (#2177) * remvoed extra user delete * removed extra title

Eamils for user actions

6f3288c

DawoudIO added this to the 2.7.0 milestone Mar 19, 2017

DawoudIO added the In Review label Mar 19, 2017

DawoudIO added 3 commits March 19, 2017 21:39

import cleanup

670f3cf

added users.php

f4a636a

added account/password methods

2da1dbf

DawoudIO requested a review from crossan007 March 20, 2017 04:41

revert major reformat

0553b7a

DawoudIO self-assigned this Mar 20, 2017

DawoudIO and others added 2 commits March 23, 2017 21:54

Merge branch 'develop' into feature/user-api-email

11e133e

Merge branch 'develop' into feature/user-api-email

ee2477d

crossan007 approved these changes Mar 25, 2017

View reviewed changes

crossan007 merged commit e7f7bc6 into develop Mar 25, 2017

crossan007 deleted the feature/user-api-email branch March 25, 2017 15:35

crossan007 removed the In Review label Mar 25, 2017

DawoudIO added a commit that referenced this pull request Mar 25, 2017

based on feedback from #2137

2b48077

DawoudIO mentioned this pull request Mar 25, 2017

Users email cleanup #2168

Merged

DawoudIO added a commit that referenced this pull request Mar 25, 2017

based on feedback from #2137 (#2168)

b4a2b26

DawoudIO added a commit that referenced this pull request Mar 25, 2017

based on feedback from #2137 (#2168)

2a1cae0

DawoudIO mentioned this pull request Mar 25, 2017

User Email v2 #2173

Closed

DawoudIO added a commit that referenced this pull request Mar 25, 2017

Send email when user is locked (#2176)

4318e3c

* based on feedback from #2137 (#2168) * send email when the user is locked * Apply fixes from StyleCI (#2175) * fix 1049

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eamils for user actions #2137

Eamils for user actions #2137

DawoudIO commented Mar 19, 2017 •

edited

Loading

DawoudIO commented Mar 20, 2017

crossan007 left a comment

crossan007 Mar 25, 2017

crossan007 Mar 25, 2017

crossan007 Mar 25, 2017

DawoudIO Mar 25, 2017

crossan007 Mar 25, 2017

DawoudIO Mar 25, 2017

crossan007 Mar 25, 2017

DawoudIO Mar 25, 2017

crossan007 Mar 25, 2017

DawoudIO Mar 25, 2017

crossan007 Mar 25, 2017

DawoudIO Mar 25, 2017

crossan007 Mar 25, 2017

DawoudIO Mar 25, 2017

crossan007 Mar 25, 2017

crossan007 Mar 25, 2017

Eamils for user actions #2137

Eamils for user actions #2137

Conversation

DawoudIO commented Mar 19, 2017 • edited Loading

DawoudIO commented Mar 20, 2017

crossan007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DawoudIO commented Mar 19, 2017 •

edited

Loading