Bug 234660 – [perf.webkit.org] Support cc me

• Component: New Bugs
• Status: NEW

[Zhifei Fang]

[perf.webkit.org] Support cc me

[Zhifei Fang]

Created attachment 447931 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 447931 [details]
Patch

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

> Websites/perf.webkit.org/init-database.sql:216
> +    task_cc varchar(256)[] DEFAULT array[]::varchar[],

Please don't use abbreviations like cc. Call it task_watch_list or something.
Are we certain we don't need any options for how to monitor task / task groups?
Otherwise, we should be adding a new table like analysis_task_subscribers or something to normalize the table.

> Websites/perf.webkit.org/init-database.sql:300
> +    testgroup_cc varchar(256)[] DEFAULT array[]::varchar[],

Ditto.

> Websites/perf.webkit.org/public/api/analysis-tasks.php:100
> +        'isMyTask' => $task_row['task_author'] == $current_user,
> +        'amIcc' => array_search($current_user, $cc_list) !== false,

Let's not do this in the backend like this.
This would mean that analysis task entries will never be catchable.
r- because of this.

> Websites/perf.webkit.org/public/include/json-header.php:215
> +function parse_simple_pg_array($array_string) {

I'm very afraid that this function is prone to escaping errors / injection attacks.
We should instead use array_to_json in our SQL query:
https://www.postgresql.org/docs/9.3/functions-json.html

> Websites/perf.webkit.org/public/include/json-header.php:223
> +    for ($i=0; $i<strlen($array_string); $i++) {

Nit: Need space around = and <.

> Websites/perf.webkit.org/public/include/json-header.php:277
> +function to_pg_array($arr) {

Again, I'm very afraid that this function will have escaping errors.

> Websites/perf.webkit.org/public/include/json-header.php:283
> +            $item = str_replace('"', '\\"', $item);

We should use pg_escape_string instead.

We should also use this function in fetch_for_tasks in commit-log-fetcher.php

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:54
> +        // pgsql select statement will get string array like this: {a,b,c}
> +        // pgsql insert or update will need the array like this: {"a","b","c"}
> +        $cc_list = to_pg_array(parse_simple_pg_array($task['task_cc']));

Please remove this comment. It doesn't really add much to the readability of the code.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:108
> +    $group_id = create_test_group_and_build_requests($db, $commit_sets, $task_id, $name, $author, $cc_list, $triggerable_id, $platform_id, $test_id, $repetition_count, $repetition_type, $needs_notification);

This function should take a watch list as an array instead of encoded postgres array.

> Websites/perf.webkit.org/public/privileged-api/update-analysis-task.php:51
> +        if ($data['ccMe']) {
> +            if (array_search($current_user, $cc_list) === false) {
> +                array_push($cc_list, $current_user);
> +                $need_update = true;
> +            }
> +        } else {
> +            $idx = array_search($current_user, $cc_list);
> +            if ($idx !== false) {
> +                array_splice($cc_list, $idx, 1);
> +                $need_update = true;
> +            }
> +        }

Instead of cc-me, we should be specifying the current user name as a part of the API.
Like Bugzilla, I'd imagine people want to be able to add other users to a task / task group as well.

> Websites/perf.webkit.org/public/privileged-api/update-analysis-task.php:79
> +            $db->begin_transaction();
> +            if (!$db->update_row('analysis_test_groups', 'testgroup', array('id' => intval($group['testgroup_id'])), $update_cc)) {
> +                $db->rollback_transaction();
> +                exit_with_error('FailedToUpdateTestGroup', array('id' => $group['testgroup_id'], 'values' => $update_cc));
> +            }
> +            $db->commit_transaction();

We shouldn't have a bunch of small transactions like this.
This would mean changes can be partially committed. r- because of this.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:25
> +        this._isMytask = object.isMyTask;
> +        this._amIcc = object.amIcc;

We should instead determine whether this task belongs to the current user or not here in the frontend side.
But I don't think this even needs to be instance variable. We can determine that in the methods below.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:80
> +    amIcc () { return this._amIcc; }
> +    isMyTask() { return this._isMytask; }

I don't think we want AnalysisTask object to be aware of the concept of current user.
Instead, add a method to return the list of users who are monitoring this task.
r- because of this.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:92
> +    ccMe(cc_switch) { return this._updateRemoteState({ccMe: cc_switch}); }

Nit: use camelCase, not _.
Again, here, we don't want AnalysisTask to be aware of the current user concept directly.
We should specify the name of the observer as an argument instead.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:737
> +        if (!task) return;

Nit: return needs to be on a new line.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:739
> +        if (task.isMyTask()) {

We should probably add a new trivial API which returns the current user name:
e.g. /api/current-user
and fetch that in updateFromSerializedState.
We should then wait for both promises before we start rendering the task.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:746
> +        checkbox.addEventListener('change', () => task.ccMe(checkbox.checked));

We need to schedule to render this component once we updated the status.

[Radar WebKit Bug Importer]

<rdar://problem/87025136>

Comment on attachment 447931 [details]
Patch

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

> Websites/perf.webkit.org/server-tests/resources/test-server.js:54
> +    setCurrentUser(user) {

Nit: new line for '{'

[Ryan Haddad]

rdar://83212502