[Webkit-unassigned] [Bug 184340] Added sending notification feature when test group finishes.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=184340

--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 341254
  --> https://bugs.webkit.org/attachment.cgi?id=341254
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341254&action=review

> Websites/perf.webkit.org/init-database.sql:282
> +    testgroup_has_pending_notification boolean NOT NULL DEFAULT FALSE,

As we discussed in person, I think we should call this needs_notification instead
since has_pending_notification sounds as though we've already got a notification to send
when in reality this boolean indicates that we must send a notification WHEN all build requests complete.

> Websites/perf.webkit.org/public/api/test-groups.php:16
> +    if (count($path) > 0 && $path[0] == 'all-with-pending-notification') {

I think ?notifications-pending=true will do.

> Websites/perf.webkit.org/public/api/test-groups.php:24
> +        $test_group_id_list = array();
> +        foreach($test_groups as $group)
> +            array_push($test_group_id_list, $group['testgroup_id']);

Why don't we move this code into fetch_requests_for_groups
so that we can call int() on each test group ID to make sure they're not random strings.

> Websites/perf.webkit.org/public/api/test-groups.php:26
> +        if (!count($test_group_id_list))

You need { ~ } here.

> Websites/perf.webkit.org/tools/js/email-notifier.js:4
> +class EmailNotifier {

Since this class doesn't directly send an email but rather notifies an external service, we shouldn't call this EmailNotifier.
How about AnalysisResultsNotifier?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180525/2ad7b0cb/attachment.html>