[Findit] Add feedback button for manual triage.

[Findit] Add feedback button for manual triage.

How to go to feedback page.
1. Open the dashboard: https://findit-for-me.googleplex.com/crash/fracas-dashboard
2. Click the signature of a crash, the page will be redirected to the feedback page of this crash.

Sheriff can triage 4 results findit provided:
regression_range,
suspected_cls,
suspected_project,
suspected_components

BUG=615267
Committed: https://chromium.googlesource.com/infra/infra/+/882c793e6442828e2c70c2289215cb155674c13d

[Sharu Jiang, 2016-06-21 20:37:55]

Patchset #1 (id:1) has been deleted

[Sharu Jiang, 2016-06-21 20:38:02]

Patchset #1 (id:20001) has been deleted

[Sharu Jiang, 2016-06-22 00:17:37]

Patchset #1 (id:40001) has been deleted

[Sharu Jiang, 2016-06-28 23:15:37]

Patchset #1 (id:60001) has been deleted

[Sharu Jiang, 2016-06-28 23:33:12]

Description was changed from

==========
Merge branch 'dashboard' into feedback-all


Merge branch 'show-result' into feedback-all


FREEZE.unindexed


Update.


Merge branch 'only-dashboard' into dashboard


Fix bugs.


fix a bug.


Fix bugs.


Fix bug.


simplify.


Merge branch 'only-dashboard' into dashboard


Fix tests.


Add tests.


Add tests.


Add features.


Merge branch 'only-dashboard' into dashboard


FREEZE.unindexed


Add time_util


Add dashboard test.


Fix nits.


update dashboard.


Add real culprit input.


Fix nits.


Merge branch 'master' of https://chromium.googlesource.com/infra/infra into
dashboard


experiments.


Modify handlers.


add one more.


intermediate.


intermediate.


Fix nits.


Fix nits.


Fix nits.


Fix nits.


Fix pylints.


fetch DEPS from buildspec/


intermediate


Fetch DEPS from buildspec/ instead of trunk for chrome official builds.

BUG=605356

patch from issue 1914653003 at patchset 80001
(http://crrev.com/1914653003#ps80001)

Merge branch 'master' of https://chromium.googlesource.com/infra/infra into
buildspec


change to googleplex app.


Fix nits.
==========

to

==========
[Findit] Add feedback button for manual triage.

How to go to feedback page.
1. Open the dashboard:
https://findit-for-me.googleplex.com/crash/fracas-dashboard
2. Click the signature of a crash, the page will be redirected to the feedback
page of this crash.

Sheriff can triage 4 results findit provided:
regression_range,
suspected_cls,
suspected_project,
suspected_components

BUG=615267
==========

[Sharu Jiang, 2016-06-29 02:35:46]

Patchset #1 (id:80001) has been deleted

[Sharu Jiang, 2016-06-29 02:36:07]

PTAL :)

[Sharu Jiang, 2016-06-29 22:43:33]

katesonia@chromium.org changed reviewers:
+ mbarbella@google.com, stgao@chromium.org

[Sharu Jiang, 2016-06-29 22:43:33]

PTAL :)

[Sharu Jiang, 2016-06-29 22:43:47]

Patchset #1 (id:100001) has been deleted

[stgao, 2016-06-30 01:28:02]

Some initial comments, will take a further look at the template.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
File appengine/findit/handlers/crash/test/triage_fracas_analysis_test.py
(right):

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
appengine/findit/handlers/crash/test/triage_fracas_analysis_test.py:1: #
Copyright 2015 The Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
File appengine/findit/handlers/crash/triage_fracas_analysis.py (right):

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
appengine/findit/handlers/crash/triage_fracas_analysis.py:21: def
_UpdateAnalysis(key, user_name, update):
Var name `update` is not clear.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
appengine/findit/handlers/crash/triage_fracas_analysis.py:28: if 'triage_status'
in key:
Will there be multiple 'triage_status'?

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
appengine/findit/handlers/crash/triage_fracas_analysis.py:41: 'triage_status':
triage_status.TRIAGE_STATUS_TO_DESCRIPTION[status],
I guess we won't change the description for a status, but it seems better to
save the status instead.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
appengine/findit/handlers/crash/triage_fracas_analysis.py:56: def
HandleGet(self):  # pragma: no cover
This could be removed if the JS is a post.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/crash_analysis.py (right):

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/crash_analysis.py:83: triage_history =
ndb.JsonProperty(indexed=False, default=[])
We'd better not "hard-code" [] as the default value.

This could cause similar issue as
http://stackoverflow.com/questions/9887180/the-value-of-an-empty-list-in-func...

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/crash_analysis.py:119: except Exception:  # pragma:
no cover.
Why there will be exceptions here? What kinds of exceptions?

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/model...
File appengine/findit/model/triage_status.py (right):

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/model...
appengine/findit/model/triage_status.py:13: TRIAGED_INCORRECT:
'Triaged-Incorrect',
Should we make it shorter "correct", "incorrect", and "unsure"?

[stgao, 2016-06-30 17:16:41]

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
File appengine/findit/templates/crash/fracas_result_feedback.html (right):

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:20: }
How about putting these to the common.css as they seems shared from the other
template?

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:72: var
suspected_project = '{{suspected_project}}'
style nit: no ending ";".

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:78: culprit_cls[i]
= culprit_cls[i]['url'];
Why this is needed?

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:147:
$(".triaged[result_property='" + result +
"']").addClass('triage').removeClass('triaged');
style: use single quote ['] in JS code, but double quote ["] in html code.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:156: alert('Failed
to update datastore. Please refresh and try again.');
nit: it is unnecessary to mention datastore here.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:193:
$("form.not-display[result_property='" + result +
"']").removeClass('not-display');
Just an idea: can we use element id instead?

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:271: <div
class="triage correct" result_property="regression_range">Correct<img
src="https://www.gstatic.com/images/icons/material/system/1x/sentiment_satisfied_black_24dp.png"/></div>
I like the icon.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:274: <form
class="not-display" result_property="regression_range">
Why a form is needed? This form is not used to submitted the data, right?
Same as below.

[Martin Barbella, 2016-06-30 23:16:42]

mbarbella@chromium.org changed reviewers:
+ mbarbella@chromium.org

[Martin Barbella, 2016-06-30 23:16:42]

Deferring to stgao's review here, but I'll try to take a look at the next patch
set.

[Sharu Jiang, 2016-07-01 22:05:49]

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
File appengine/findit/handlers/crash/test/triage_fracas_analysis_test.py
(right):

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
appengine/findit/handlers/crash/test/triage_fracas_analysis_test.py:1: #
Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/06/30 01:28:02, stgao wrote:
> 2016

Done.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
File appengine/findit/handlers/crash/triage_fracas_analysis.py (right):

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
appengine/findit/handlers/crash/triage_fracas_analysis.py:21: def
_UpdateAnalysis(key, user_name, update):
On 2016/06/30 01:28:02, stgao wrote:
> Var name `update` is not clear.

Changed to update_data.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
appengine/findit/handlers/crash/triage_fracas_analysis.py:28: if 'triage_status'
in key:
On 2016/06/30 01:28:02, stgao wrote:
> Will there be multiple 'triage_status'?

We only click one button at a time, so it will only have one triage_status

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
appengine/findit/handlers/crash/triage_fracas_analysis.py:41: 'triage_status':
triage_status.TRIAGE_STATUS_TO_DESCRIPTION[status],
On 2016/06/30 01:28:02, stgao wrote:
> I guess we won't change the description for a status, but it seems better to
> save the status instead.

Acknowledged.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/handl...
appengine/findit/handlers/crash/triage_fracas_analysis.py:56: def
HandleGet(self):  # pragma: no cover
On 2016/06/30 01:28:02, stgao wrote:
> This could be removed if the JS is a post.

Done.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/model...
File appengine/findit/model/crash/crash_analysis.py (right):

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/crash_analysis.py:83: triage_history =
ndb.JsonProperty(indexed=False, default=[])
On 2016/06/30 01:28:02, stgao wrote:
> We'd better not "hard-code" [] as the default value.
> 
> This could cause similar issue as
>
http://stackoverflow.com/questions/9887180/the-value-of-an-empty-list-in-func...

Done.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/model...
appengine/findit/model/crash/crash_analysis.py:119: except Exception:  # pragma:
no cover.
On 2016/06/30 01:28:02, stgao wrote:
> Why there will be exceptions here? What kinds of exceptions?

Just want to return False somehow, changed it to if nothing changed, it will
return False :)

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/model...
File appengine/findit/model/triage_status.py (right):

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/model...
appengine/findit/model/triage_status.py:13: TRIAGED_INCORRECT:
'Triaged-Incorrect',
On 2016/06/30 01:28:02, stgao wrote:
> Should we make it shorter "correct", "incorrect", and "unsure"?

Done.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
File appengine/findit/templates/crash/fracas_result_feedback.html (right):

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:20: }
On 2016/06/30 17:16:40, stgao wrote:
> How about putting these to the common.css as they seems shared from the other
> template?

Done.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:72: var
suspected_project = '{{suspected_project}}'
On 2016/06/30 17:16:40, stgao wrote:
> style nit: no ending ";".

Done.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:78: culprit_cls[i]
= culprit_cls[i]['url'];
On 2016/06/30 17:16:40, stgao wrote:
> Why this is needed?

culprit_cls has a lot of information, like 'author', 'score', 'reason'...
But I only want to show the url in the textarea.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:147:
$(".triaged[result_property='" + result +
"']").addClass('triage').removeClass('triaged');
On 2016/06/30 17:16:40, stgao wrote:
> style: use single quote ['] in JS code, but double quote ["] in html code.

Done.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:156: alert('Failed
to update datastore. Please refresh and try again.');
On 2016/06/30 17:16:40, stgao wrote:
> nit: it is unnecessary to mention datastore here.

Done.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:193:
$("form.not-display[result_property='" + result +
"']").removeClass('not-display');
On 2016/06/30 17:16:40, stgao wrote:
> Just an idea: can we use element id instead?

Done.

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:271: <div
class="triage correct" result_property="regression_range">Correct<img
src="https://www.gstatic.com/images/icons/material/system/1x/sentiment_satisfied_black_24dp.png"/></div>
On 2016/06/30 17:16:40, stgao wrote:
> I like the icon.

Thanks :)

https://codereview.chromium.org/2074273002/diff/120001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:274: <form
class="not-display" result_property="regression_range">
On 2016/06/30 17:16:40, stgao wrote:
> Why a form is needed? This form is not used to submitted the data, right?
> Same as below.

Done.

[Sharu Jiang, 2016-07-01 22:05:58]

Patchset #2 (id:140001) has been deleted

[stgao, 2016-07-09 01:20:53]

lgtm with nits.

https://codereview.chromium.org/2074273002/diff/160001/appengine/findit/handl...
File appengine/findit/handlers/crash/triage_fracas_analysis.py (right):

https://codereview.chromium.org/2074273002/diff/160001/appengine/findit/handl...
appengine/findit/handlers/crash/triage_fracas_analysis.py:59: update_data =
self.request.params.get('update')
should we change the param name in the request too?

https://codereview.chromium.org/2074273002/diff/160001/appengine/findit/templ...
File appengine/findit/templates/crash/fracas_result_feedback.html (right):

https://codereview.chromium.org/2074273002/diff/160001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:174:
$('.triage').click(triageAnalysisResult);
Are we still updating the triage history per-click of the
correct/incorrect/unsure?

From the UI, it looks more like that the user should fill in all feedback of all
fields and then click the "Save" button to save them in one request.

https://codereview.chromium.org/2074273002/diff/160001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:176: if
(triage_status == 1) {
Should we add a comment to explain which scenario this is?
Same for below.

https://codereview.chromium.org/2074273002/diff/160001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:333: {{component}}
Why some suspected component is an empty string?

https://findit-for-me.googleplex.com/crash/fracas-result-feedback?key=ahpzfmd...

[Sharu Jiang, 2016-07-11 18:37:46]

https://codereview.chromium.org/2074273002/diff/160001/appengine/findit/templ...
File appengine/findit/templates/crash/fracas_result_feedback.html (right):

https://codereview.chromium.org/2074273002/diff/160001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:174:
$('.triage').click(triageAnalysisResult);
On 2016/07/09 01:20:52, stgao wrote:
> Are we still updating the triage history per-click of the
> correct/incorrect/unsure?
> 
> From the UI, it looks more like that the user should fill in all feedback of
all
> fields and then click the "Save" button to save them in one request.

The current design is, we update the triage history per-click, but save all the
culprit results and note using Save button, because I think it tooks some time
for sheriffs to enter the culprit results and note, and they may not click the
save button if they just click 'correct' for all the results.

https://codereview.chromium.org/2074273002/diff/160001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:176: if
(triage_status == 1) {
On 2016/07/09 01:20:52, stgao wrote:
> Should we add a comment to explain which scenario this is?
> Same for below.

Done.

https://codereview.chromium.org/2074273002/diff/160001/appengine/findit/templ...
appengine/findit/templates/crash/fracas_result_feedback.html:333: {{component}}
On 2016/07/09 01:20:52, stgao wrote:
> Why some suspected component is an empty string?
> 
>
https://findit-for-me.googleplex.com/crash/fracas-result-feedback?key=ahpzfmd...

I will fix this in another cl, it's in the component classifier, I should filter
those empty results there.

[Sharu Jiang, 2016-07-12 18:03:19]

The CQ bit was checked by katesonia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 18:03:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 18:18:41]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 18:18:42]

Dry run: Try jobs failed on following builders:
  Infra Linux Trusty 64 Tester on master.tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Linux%20Trusty%...)

[Sharu Jiang, 2016-07-12 22:59:30]

The CQ bit was checked by katesonia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 22:59:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 23:13:51]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 23:13:52]

Dry run: This issue passed the CQ dry run.

[Sharu Jiang, 2016-07-12 23:18:05]

The CQ bit was checked by katesonia@chromium.org

[Sharu Jiang, 2016-07-12 23:18:06]

The patchset sent to the CQ was uploaded after l-g-t-m from stgao@chromium.org
Link to the patchset: https://codereview.chromium.org/2074273002/#ps200001
(title: "Rebase and fix tests.")

[commit-bot: I haz the power, 2016-07-12 23:18:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-12 23:20:49]

Description was changed from

==========
[Findit] Add feedback button for manual triage.

How to go to feedback page.
1. Open the dashboard:
https://findit-for-me.googleplex.com/crash/fracas-dashboard
2. Click the signature of a crash, the page will be redirected to the feedback
page of this crash.

Sheriff can triage 4 results findit provided:
regression_range,
suspected_cls,
suspected_project,
suspected_components

BUG=615267
==========

to

==========
[Findit] Add feedback button for manual triage.

How to go to feedback page.
1. Open the dashboard:
https://findit-for-me.googleplex.com/crash/fracas-dashboard
2. Click the signature of a crash, the page will be redirected to the feedback
page of this crash.

Sheriff can triage 4 results findit provided:
regression_range,
suspected_cls,
suspected_project,
suspected_components

BUG=615267

Committed:
https://chromium.googlesource.com/infra/infra/+/882c793e6442828e2c70c2289215c...
==========

[commit-bot: I haz the power, 2016-07-12 23:20:51]

Committed patchset #4 (id:200001) as
https://chromium.googlesource.com/infra/infra/+/882c793e6442828e2c70c2289215c...