[Findit] Show build analysis references in UI for Findit Cross-platform auto-triage

appengine/findit/handlers/triage_analysis.py

Index: appengine/findit/handlers/triage_analysis.py
diff --git a/appengine/findit/handlers/triage_analysis.py b/appengine/findit/handlers/triage_analysis.py
index bbc1256c5c322d0ec22a6c22d6cae1eea8be10cb..5ea80bd1f3fe0e2b01f30d7b1d9e70eabe7da917 100644
--- a/appengine/findit/handlers/triage_analysis.py
+++ b/appengine/findit/handlers/triage_analysis.py
@@ -48,6 +48,7 @@ def _GenPotentialCulpritTupleList(analysis):
   # Iterates through the failures, tests, and suspected_cls, appending potential
   # (step_name, test_name, revision) and (step_name, revision) culprit tuples to
   # the list.
+

[lijeffrey, 2016/06/27 20:38:22]

nit: delete this empty line

[josiahk, 2016/06/28 22:59:23]

Done.

   for failure in analysis.result['failures']:
     if failure.get('tests'):
       for test in failure['tests']:
@@ -90,7 +91,8 @@ def _DoAnalysesMatch(analysis_1, analysis_2):
           sorted(potential_culprit_tuple_list_2))
 
 
-def _AppendTriageHistoryRecord(analysis, is_correct, user_name):
+def _AppendTriageHistoryRecord(
+    analysis, is_correct, user_name, is_duplicate=False):
   """Appends a triage history record to the given analysis.
 
   Args:
@@ -98,10 +100,15 @@ def _AppendTriageHistoryRecord(analysis, is_correct, user_name):
     is_correct: True if the history record should indicate a correct judgement,
         otherwise False.
     user_name: The user_name of the person to include in the triage record.
+    is_duplicate: True if the history record should indicate that this triage is
+        a duplicate, otherwise False.
   """
   if is_correct:
     if analysis.suspected_cls:
-      analysis.result_status = result_status.FOUND_CORRECT
+      if is_duplicate:
+        analysis.result_status = result_status.FOUND_CORRECT_DUPLICATE
+      else:
+        analysis.result_status = result_status.FOUND_CORRECT
       analysis.culprit_cls = analysis.suspected_cls
     else:
       analysis.result_status = result_status.NOT_FOUND_CORRECT
@@ -109,7 +116,10 @@ def _AppendTriageHistoryRecord(analysis, is_correct, user_name):
   else:
     analysis.culprit_cls = None
     if analysis.suspected_cls:
-      analysis.result_status = result_status.FOUND_INCORRECT
+      if is_duplicate:
+        analysis.result_status = result_status.FOUND_INCORRECT_DUPLICATE
+      else:
+        analysis.result_status = result_status.FOUND_INCORRECT
     else:
       analysis.result_status = result_status.NOT_FOUND_INCORRECT
 
@@ -133,6 +143,10 @@ def _UpdateAnalysisResultStatus(
   if not analysis or not analysis.completed:
     return False, None
 
+  analysis.triage_reference_analysis_master_name = None

[lijeffrey, 2016/06/27 20:38:22]

so what are these for, and why are we setting them to None here?

[chanli, 2016/06/27 22:27:56]

+1

[josiahk, 2016/06/28 22:59:23]

When another "first-cause" build analysis is triaged, and this build analysis is
marked as a duplicate from that other "first-cause" build analysis, these are
the variables that hold the reference back to that "first-cause" build analysis.

It's possible that someone could then manually re-triage this build analysis, in
which case this build analysis is no longer a duplicate, and we want to erase
the reference to the no-longer-relevant "first-cause" build_analysis, which is
what these lines are doing.

Maybe it would be a good idea for me to move these lines into
_AppendTriageHistoryRecord(), because that is where other properties of the
analysis get updated.

[lijeffrey, 2016/06/29 00:48:23]

you don't need to initialize them to None, if they're not explicitly set they
should default to None so these lines are unnecessary unless you specifically
want to overwrite any previously set values

[josiahk, 2016/06/29 22:40:00]

Yes, I want to potentially overwrite previously set values. There is a
possibility they might be set to something other than None (see lines 202-207).
Thanks!

[lijeffrey, 2016/06/30 01:09:46]

It appears with what you have triage duplicate results which will do the writing
is always called after this function. If that's indeed the case then setting
these to None here would just be overwritten anyway too, making them unnecessary

[josiahk, 2016/06/30 22:34:56]

_UpdateAnalysisResultStatus() operates on a single original build analysis that
the user directly triages.
_TriageDuplicateResults() operates on potentially many build analyses that match
that first single original build analysis.

Since _TriageDuplicateResults() doesn't operate on the original build analysis,
the None's don't get overwritten right away.

I moved this code to inside _AppendTriageHistoryRecord(), and added a
conditional and a comment; hopefully the code is more self-explanatory now!

+  analysis.triage_reference_analysis_builder_name = None
+  analysis.triage_reference_analysis_build_number = None
+
   _AppendTriageHistoryRecord(analysis, is_correct, user_name)
 
   return True, analysis
@@ -175,16 +189,26 @@ def _GetDuplicateAnalyses(original_analysis):
 
   # Further filter potential duplicates and return them.
   return [analysis for analysis in analysis_results if
+          analysis.completed and
+          analysis.result and
           _DoAnalysesMatch(original_analysis, analysis) and
-          original_analysis.key is not analysis.key and
-          analysis.completed]
+          original_analysis.key is not analysis.key]
 
 
 def _TriageDuplicateResults(original_analysis, is_correct, user_name=None):
   matching_analyses = _GetDuplicateAnalyses(original_analysis)
 
   for analysis in matching_analyses:
-    _AppendTriageHistoryRecord(analysis, is_correct, user_name)
+    analysis.triage_reference_analysis_master_name = (
+        original_analysis.master_name)
+    analysis.triage_reference_analysis_builder_name = (
+        original_analysis.builder_name)
+    analysis.triage_reference_analysis_build_number = (
+        original_analysis.build_number)
+    _AppendTriageHistoryRecord(analysis, is_correct, user_name,
+                               is_duplicate=True)
+
+  return len(matching_analyses)

[lijeffrey, 2016/06/27 20:38:22]

It looks like you're returning the number of matching analyses that were found.
I would rename your method to something like _GetTriageDuplicateResultsCount or
something to make it more clear that this method returns a count and not just
performs actions

[josiahk, 2016/06/28 22:59:23]

Renamed to _TriageAndCountDuplicateResults()

 
 
 class TriageAnalysis(BaseHandler):
@@ -211,6 +235,9 @@ class TriageAnalysis(BaseHandler):
     user_name = users.get_current_user().email().split('@')[0]
     success, original_analysis = _UpdateAnalysisResultStatus(
         master_name, builder_name, build_number, is_correct, user_name)
+    num_duplicate_analyses = 0
     if success:
-      _TriageDuplicateResults(original_analysis, is_correct, user_name)
-    return {'data': {'success': success}}
+      num_duplicate_analyses = _TriageDuplicateResults(
+          original_analysis, is_correct, user_name)
+    return {'data': {'success': success,
+                     'num_duplicate_analyses': num_duplicate_analyses}}