[webkit-reviews] review denied: [Bug 234660] [perf.webkit.org] Support cc me : [Attachment 447931] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Zhifei Fang
<zhifei_fang at apple.com>'s request for review:
Bug 234660: [perf.webkit.org] Support cc me
https://bugs.webkit.org/show_bug.cgi?id=234660

Attachment 447931: Patch

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




--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 447931
  --> https://bugs.webkit.org/attachment.cgi?id=447931
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.