From 4a7a0a71fadb5f926200f3ade5835dc7ece4af01 Mon Sep 17 00:00:00 2001 From: Saagar Arya Date: Mon, 18 Sep 2023 12:11:49 -0400 Subject: [PATCH 1/4] [Issue Tracker] Notifications --- modules/issue_tracker/jsx/IssueForm.js | 2 +- modules/issue_tracker/php/edit.class.inc | 124 +++++++++++++++--- .../test/issue_tracker_test_plan.md | 1 + .../email/issue_assigned_modified.tpl | 15 +++ smarty/templates/email/issue_change.tpl | 4 + 5 files changed, 126 insertions(+), 20 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 0e3a0680d52..22e7927ff11 100644 --- a/modules/issue_tracker/php/edit.class.inc +++ b/modules/issue_tracker/php/edit.class.inc @@ -202,6 +202,10 @@ class Edit extends \NDB_Page implements ETagCalculator $issueData['attachments'] = $attachments; $issueData['whoami'] = $user->getUsername(); $issueData['othersWatching'] = $this->getWatching($issueID); + $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 @@ -347,7 +351,12 @@ class Edit extends \NDB_Page implements ETagCalculator $db = $this->loris->getDatabaseConnection(); $watching = $db->pselect( - "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] ); @@ -431,6 +440,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; @@ -481,18 +495,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'])) { @@ -519,19 +521,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', [ @@ -540,6 +557,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] ); @@ -552,11 +609,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(); @@ -573,8 +632,9 @@ 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, @@ -598,7 +658,26 @@ class Edit extends \NDB_Page implements ETagCalculator '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(), + ] + ); + + if (isset($issueChangeEmailsAssignee[0])) { + $msg_data['firstname'] = $issueChangeEmailsAssignee[0]['firstname']; + + \Email::send( + $issueChangeEmailsAssignee[0]['Email'], + 'issue_assigned_modified.tpl', + $msg_data + ); + } } $issue_change_emails = $db->pselect( @@ -806,6 +885,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') { @@ -815,6 +898,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..7528e3204c3 --- /dev/null +++ b/smarty/templates/email/issue_assigned_modified.tpl @@ -0,0 +1,15 @@ +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..a400149e803 100644 --- a/smarty/templates/email/issue_change.tpl +++ b/smarty/templates/email/issue_change.tpl @@ -4,6 +4,10 @@ Subject: Change to Issue # - {$issueID} {$currentUser} has updated an issue "{$title}" you are watching. +{if $comment !== "null"} + {$currentUser} commented: "{$comment}" +{/if} + Please view the changes here: {$url} Thank you, From 44d59a6b12d3b6d9a5df9c8a11a277720cea3e71 Mon Sep 17 00:00:00 2001 From: Saagar Arya Date: Mon, 18 Sep 2023 12:20:37 -0400 Subject: [PATCH 2/4] Fix spacing in php --- modules/issue_tracker/php/edit.class.inc | 55 ++++++++++++------------ 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/modules/issue_tracker/php/edit.class.inc b/modules/issue_tracker/php/edit.class.inc index 22e7927ff11..7f5fbd43750 100644 --- a/modules/issue_tracker/php/edit.class.inc +++ b/modules/issue_tracker/php/edit.class.inc @@ -637,47 +637,46 @@ class Edit extends \NDB_Page implements ETagCalculator 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(), - ] + } + } 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(), + ] + ); + + if (isset($issueChangeEmailsAssignee[0])) { + $msg_data['firstname'] = $issueChangeEmailsAssignee[0]['firstname']; + + \Email::send( + $issueChangeEmailsAssignee[0]['Email'], + 'issue_assigned_modified.tpl', + $msg_data ); - - if (isset($issueChangeEmailsAssignee[0])) { - $msg_data['firstname'] = $issueChangeEmailsAssignee[0]['firstname']; - - \Email::send( - $issueChangeEmailsAssignee[0]['Email'], - 'issue_assigned_modified.tpl', - $msg_data - ); - } + } } $issue_change_emails = $db->pselect( From c43711855e7ad16cfaa5c6c1f0968938767a91e5 Mon Sep 17 00:00:00 2001 From: Saagar Arya Date: Mon, 18 Sep 2023 12:39:08 -0400 Subject: [PATCH 3/4] Remove assignee from emails to watchers --- modules/issue_tracker/php/edit.class.inc | 2 ++ smarty/templates/email/issue_assigned_modified.tpl | 1 - smarty/templates/email/issue_change.tpl | 1 - 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/issue_tracker/php/edit.class.inc b/modules/issue_tracker/php/edit.class.inc index 7f5fbd43750..2a08795629c 100644 --- a/modules/issue_tracker/php/edit.class.inc +++ b/modules/issue_tracker/php/edit.class.inc @@ -689,11 +689,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, ] ); diff --git a/smarty/templates/email/issue_assigned_modified.tpl b/smarty/templates/email/issue_assigned_modified.tpl index 7528e3204c3..8fa7792263a 100644 --- a/smarty/templates/email/issue_assigned_modified.tpl +++ b/smarty/templates/email/issue_assigned_modified.tpl @@ -1,5 +1,4 @@ Subject: Issue Assigned - # {$issueID} - {$firstname}, The issue "{$title}" that is assigned to you has been modified. diff --git a/smarty/templates/email/issue_change.tpl b/smarty/templates/email/issue_change.tpl index a400149e803..9559382c328 100644 --- a/smarty/templates/email/issue_change.tpl +++ b/smarty/templates/email/issue_change.tpl @@ -1,5 +1,4 @@ Subject: Change to Issue # - {$issueID} - {$firstname}, {$currentUser} has updated an issue "{$title}" you are watching. From 665a8272cc1760a35e496e1bc2015b8844522740 Mon Sep 17 00:00:00 2001 From: Saagar Arya Date: Tue, 19 Sep 2023 14:28:51 -0400 Subject: [PATCH 4/4] Fix php formatting --- modules/issue_tracker/php/edit.class.inc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/issue_tracker/php/edit.class.inc b/modules/issue_tracker/php/edit.class.inc index 2a08795629c..6a39a7cb7b3 100644 --- a/modules/issue_tracker/php/edit.class.inc +++ b/modules/issue_tracker/php/edit.class.inc @@ -545,9 +545,9 @@ class Edit extends \NDB_Page implements ETagCalculator ]; $db->replace('issues_watching', $nowWatching); } else if (isset($values['watching']) - && $values['watching'] == 'No' - && (!isset($issueValues['assignee']) - || $issueValues['assignee'] !== $user->getUsername()) + && $values['watching'] == 'No' + && (!isset($issueValues['assignee']) + || $issueValues['assignee'] !== $user->getUsername()) ) { $db->delete( 'issues_watching',