Created attachment 358119 [details] Patch

Comment on attachment 358119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358119&action=review > Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:46 > + if (array_key_exists('needsNotification', $data)) { > + $db->update_row('analysis_test_groups', 'testgroup', array('id' => $test_group_id), So what happens if the user who created the test group and the user who added more build requests were different? Our simplistic model of having a single author per test group may no longer be valid... > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:403 > + <test-group-form id="add-build-request-form">Add</test-group-form> It seems a bit unintuitive that "Add" adds as many iterations as specified for retry... > Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:561 > + groupPane.listenToAction('bisectTestGroup', (testGroup, commitSets, repetitionCount, notifyOnCompletion) => this._bisectCurrentTestGroup(testGroup, commitSets, repetitionCount, notifyOnCompletion));groupPane.listenToAction('retryTestGroup', (testGroup, repetitionCount, notifyOnCompletion) => this._retryCurrentTestGroup(testGroup, repetitionCount, notifyOnCompletion)); You have a duplicate code for retryTestGroup. Remove that.

Comment on attachment 358119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358119&action=review >> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:46 >> + $db->update_row('analysis_test_groups', 'testgroup', array('id' => $test_group_id), > > So what happens if the user who created the test group and the user who added more build requests were different? > Our simplistic model of having a single author per test group may no longer be valid... You are right. Per this commit, what will happen is the author is not changed, meaning that the people who increase the build request may not receive the notification...

(In reply to dewei_zhu from comment #3) > Comment on attachment 358119 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=358119&action=review > > >> Websites/perf.webkit.org/public/privileged-api/add-build-requests.php:46 > >> + $db->update_row('analysis_test_groups', 'testgroup', array('id' => $test_group_id), > > > > So what happens if the user who created the test group and the user who added more build requests were different? > > Our simplistic model of having a single author per test group may no longer be valid... > > You are right. Per this commit, what will happen is the author is not > changed, meaning that the people who increase the build request may not > receive the notification... This along with the other feature where you're trying to add the ability to notify authors of other test groups, I'm more and more convinced that we need some kind of a watch list for a test group / analysis task.