From 03d209275380664143fe79a04b82d3f1d5cd60ff Mon Sep 17 00:00:00 2001 From: Saagar Arya <51128536+skarya22@users.noreply.github.com> Date: Thu, 11 Jan 2024 06:12:47 -0800 Subject: [PATCH] [Issue Tracker] Issue Change Notifications (#8885) Fixed issue where the assignee dropdown did not show the current assignee Emails users who were added as watching When a comment is added, it adds it to the email to everyone watching --- modules/issue_tracker/jsx/IssueForm.js | 2 +- modules/issue_tracker/php/edit.class.inc | 139 ++++++++++++++---- .../test/issue_tracker_test_plan.md | 1 + .../email/issue_assigned_modified.tpl | 14 ++ smarty/templates/email/issue_change.tpl | 5 +- 5 files changed, 133 insertions(+), 28 deletions(-) create mode 100644 smarty/templates/email/issue_assigned_modified.tpl diff --git a/modules/issue_tracker/jsx/IssueForm.js b/modules/issue_tracker/jsx/IssueForm.js index 90b7a972617..13748be7fd9 100644 --- a/modules/issue_tracker/jsx/IssueForm.js +++ b/modules/issue_tracker/jsx/IssueForm.js @@ -26,7 +26,7 @@ class IssueForm extends Component { super(props); this.state = { - Data: [], + Data: {}, formData: {}, submissionResult: null, errorMessage: null, diff --git a/modules/issue_tracker/php/edit.class.inc b/modules/issue_tracker/php/edit.class.inc index 3c79e1d65f0..b4705d6f839 100644 --- a/modules/issue_tracker/php/edit.class.inc +++ b/modules/issue_tracker/php/edit.class.inc @@ -222,6 +222,10 @@ class Edit extends \NDB_Page implements ETagCalculator $issueData['attachments'] = $attachments; $issueData['whoami'] = $user->getUsername(); $issueData['othersWatching'] = array_keys($othersWatching); + $issueData['assignee'] = $db->pselectOne( + "SELECT assignee FROM issues WHERE issueID=:issueID", + ['issueID' => $issueID] + ); // We need to unescape the string here: // React is escaping the string in the template @@ -370,7 +374,12 @@ class Edit extends \NDB_Page implements ETagCalculator $watching = $db->pselect( "SELECT Real_name, UserID from users WHERE UserID IN - (SELECT UserID FROM issues_watching WHERE issueID=:issueID)", + (SELECT userID from issues_watching + WHERE issueID=:issueID) + AND UserID NOT IN + (SELECT assignee FROM issues + WHERE issueID=:issueID + AND assignee IS NOT NULL)", ['issueID' => $issueID] ); @@ -454,6 +463,11 @@ class Edit extends \NDB_Page implements ETagCalculator $issueValues['lastUpdatedBy'] = $user->getUsername(); + $assignee = $db->pselectOne( + "SELECT assignee FROM issues WHERE issueID=:ID", + ['ID' => $issueID] + ); + $validatedInput = $this->validateInput($validateValues, $user); if (!is_array($validatedInput)) { // Error exists. return $validatedInput; @@ -504,18 +518,6 @@ class Edit extends \NDB_Page implements ETagCalculator } } - // Adding new assignee to watching - if (isset($issueValues['assignee'])) { - $nowWatching = [ - 'userID' => $issueValues['assignee'], - 'issueID' => $issueID, - ]; - $db->replace('issues_watching', $nowWatching); - - // sending email - $this->emailUser($issueID, $issueValues['assignee'], '', $user); - } - // Adding others from multiselect to watching table. if (isset($values['othersWatching'])) { @@ -542,19 +544,34 @@ class Edit extends \NDB_Page implements ETagCalculator ) { continue; } - $this->emailUser($issueID, '', $usersWatching, $user); + $this->emailUser( + $issueID, + '', + $usersWatching, + $user, + 'false', + $values + ); } } } // Add editor to the watching table unless they don't want to be added. - if (isset($values['watching']) && $values['watching'] == 'Yes') { + if (isset($values['watching']) + && $values['watching'] == 'Yes' + && (!isset($issueValues['assignee']) + || $issueValues['assignee'] !== $user->getUsername()) + ) { $nowWatching = [ 'userID' => $user->getUsername(), 'issueID' => $issueID, ]; $db->replace('issues_watching', $nowWatching); - } else if (isset($values['watching']) && $values['watching'] == 'No') { + } else if (isset($values['watching']) + && $values['watching'] == 'No' + && (!isset($issueValues['assignee']) + || $issueValues['assignee'] !== $user->getUsername()) + ) { $db->delete( 'issues_watching', [ @@ -563,6 +580,46 @@ class Edit extends \NDB_Page implements ETagCalculator ] ); } + // Adding new assignee to watching + if (isset($issueValues['assignee']) + && $issueValues['assignee'] !== $assignee + ) { + $nowWatching = [ + 'userID' => $issueValues['assignee'], + 'issueID' => $issueID + ]; + $db->replace('issues_watching', $nowWatching); + // sending email + $this->emailUser( + $issueID, + $issueValues['assignee'], + isset($usersWatching) ? $usersWatching : '', + $user, + 'false', + $values + ); + } else if (isset($issueValues['assignee']) + && $issueValues['assignee'] === $assignee + ) { + // sending email + $this->emailUser( + $issueID, + $issueValues['assignee'], + isset($usersWatching) ? $usersWatching : '', + $user, + 'true', + $values + ); + } else { + $this->emailUser( + $issueID, + '', + isset($usersWatching) ? $usersWatching : '', + $user, + 'false', + $values + ); + } return new \LORIS\Http\Response\JsonResponse( ['issueID' => $issueID] ); @@ -575,11 +632,13 @@ class Edit extends \NDB_Page implements ETagCalculator * @param string $changed_assignee changed assignee * @param string $changed_watcher changed watcher * @param \User $user the user requesting the change + * @param string $new_assignee_tag boolean of whether it is a new assignee + * @param array $values the values the user entered in the form * * @return void */ function emailUser(int $issueID, string $changed_assignee, - string $changed_watcher, \User $user + string $changed_watcher, \User $user, string $new_assignee_tag, array $values ) { $db = $this->loris->getDatabaseConnection(); $baseurl = \NDB_Factory::singleton()->settings()->getBaseURL(); @@ -596,17 +655,36 @@ class Edit extends \NDB_Page implements ETagCalculator $msg_data['issueID'] = $issueID; $msg_data['currentUser'] = $user->getUsername(); $msg_data['title'] = $title; + $msg_data['comment'] = $values['comment']; - if (isset($changed_assignee)) { + if (isset($changed_assignee) && $new_assignee_tag == 'false') { $issueChangeEmailsAssignee = $db->pselect( "SELECT - u.Email AS Email, - u.First_name AS firstname - FROM - users u - WHERE - u.UserID = :assignee - AND u.UserID != :currentUser", + u.Email AS Email, + u.First_name AS firstname + FROM + users u + WHERE + u.UserID = :assignee + AND u.UserID != :currentUser", + [ + 'assignee' => $changed_assignee, + 'currentUser' => $user->getUserName(), + ] + ); + if (isset($issueChangeEmailsAssignee[0])) { + $msg_data['firstname'] = $issueChangeEmailsAssignee[0]['firstname']; + \Email::send( + $issueChangeEmailsAssignee[0]['Email'], + 'issue_assigned.tpl', + $msg_data + ); + } + } else if (isset($changed_assignee) && $new_assignee_tag === 'true') { + $issueChangeEmailsAssignee = $db->pselect( + "SELECT u.Email as Email, u.First_name as firstname " . + "FROM users u WHERE u.UserID=:assignee + AND u.UserID<>:currentUser", [ 'assignee' => $changed_assignee, 'currentUser' => $user->getUserName(), @@ -618,7 +696,7 @@ class Edit extends \NDB_Page implements ETagCalculator \Email::send( $issueChangeEmailsAssignee[0]['Email'], - 'issue_assigned.tpl', + 'issue_assigned_modified.tpl', $msg_data ); } @@ -634,11 +712,13 @@ class Edit extends \NDB_Page implements ETagCalculator WHERE w.issueID = :issueID AND u.UserID = :watcher + AND u.UserID != :assignee AND u.UserID != :currentUser", [ 'issueID' => $issueID, 'watcher' => $changed_watcher, 'currentUser' => $user->getUserName(), + 'assignee' => $changed_assignee, ] ); @@ -829,6 +909,10 @@ class Edit extends \NDB_Page implements ETagCalculator function updateHistory(array $values, int $issueID, \User $user) { $db = $this->loris->getDatabaseConnection(); + $originalAssignee = $db->pselectOne( + 'SELECT assignee FROM issues where issueID = :issueID', + ['issueID' => $issueID] + ); foreach ($values as $key => $value) { // centerID is allowed to be NULL if (!empty($value) || $key === 'centerID') { @@ -838,6 +922,9 @@ class Edit extends \NDB_Page implements ETagCalculator 'issueID' => $issueID, 'addedBy' => $user->getUsername(), ]; + if ($key === 'assignee' && $originalAssignee === $value) { + continue; + } $db->unsafeInsert('issues_history', $changedValues); } } diff --git a/modules/issue_tracker/test/issue_tracker_test_plan.md b/modules/issue_tracker/test/issue_tracker_test_plan.md index 3aef36d8cb1..2f56604eed3 100644 --- a/modules/issue_tracker/test/issue_tracker_test_plan.md +++ b/modules/issue_tracker/test/issue_tracker_test_plan.md @@ -40,6 +40,7 @@ 10. Test if users assigned to issues can upload attachments. 11. Test if users can delete their own uploaded attachments. 12. Test if user assigned to issue cannot delete attachments of issue owner. +13. Test that emails are sent to users that are watching the issue. ## Permissions [Automation Testing] 1. Remove `access_all_profiles` permission. diff --git a/smarty/templates/email/issue_assigned_modified.tpl b/smarty/templates/email/issue_assigned_modified.tpl new file mode 100644 index 00000000000..8fa7792263a --- /dev/null +++ b/smarty/templates/email/issue_assigned_modified.tpl @@ -0,0 +1,14 @@ +Subject: Issue Assigned - # {$issueID} +{$firstname}, + +The issue "{$title}" that is assigned to you has been modified. + +{if $comment !== "null"} + {$currentUser} commented: "{$comment}" + +{/if} +Please see the issue here: {$url} + +Thank you, + +LORIS Team \ No newline at end of file diff --git a/smarty/templates/email/issue_change.tpl b/smarty/templates/email/issue_change.tpl index 8a8b53a9188..9559382c328 100644 --- a/smarty/templates/email/issue_change.tpl +++ b/smarty/templates/email/issue_change.tpl @@ -1,9 +1,12 @@ Subject: Change to Issue # - {$issueID} - {$firstname}, {$currentUser} has updated an issue "{$title}" you are watching. +{if $comment !== "null"} + {$currentUser} commented: "{$comment}" +{/if} + Please view the changes here: {$url} Thank you,