[Findit] Using master flake analysis key for updating triage results

appengine/findit/handlers/flake/triage_flake_analysis.py

Index: appengine/findit/handlers/flake/triage_flake_analysis.py
diff --git a/appengine/findit/handlers/flake/triage_flake_analysis.py b/appengine/findit/handlers/flake/triage_flake_analysis.py
index 7670f080a359edc29d8e71ce41350e6f2ac801f4..5228d2c1a71f140f9651f67832e759c76952e6a0 100644
--- a/appengine/findit/handlers/flake/triage_flake_analysis.py
+++ b/appengine/findit/handlers/flake/triage_flake_analysis.py
@@ -7,6 +7,7 @@
 This handler will mark the suspected flake result as correct or incorrect.
 """
 
+from google.appengine.ext import ndb
 from google.appengine.api import users
 
 from common.base_handler import BaseHandler
@@ -15,11 +16,8 @@ from model.flake.master_flake_analysis import MasterFlakeAnalysis
 
 
 def _UpdateSuspectedFlakeAnalysis(
-    master_name, builder_name, build_number, step_name, test_name,
-    version_number, suspected_build_number, triage_result, user_name):
-  master_flake_analysis = MasterFlakeAnalysis.GetVersion(
-      master_name, builder_name, build_number, step_name, test_name,
-      version_number)
+    key_urlsafe, suspected_build_number, triage_result, user_name):
+  master_flake_analysis = ndb.Key(urlsafe=key_urlsafe).get()
 
   if not master_flake_analysis:  # pragma: no cover
     return False
@@ -29,7 +27,8 @@ def _UpdateSuspectedFlakeAnalysis(
   }
 
   master_flake_analysis.UpdateTriageResult(

[stgao, 2016/12/08 00:14:12]

Do we assert that the analysis is completed and have a finding?

[lijeffrey, 2016/12/08 01:23:46]

Added for robustness, but it shouldn't be necessary. There should be no
suspected flake build number if the analysis is not completed, and with no build
number there's no way to triage the result and thus no way to reach this code.

-      triage_result, suspect_info, user_name, version_number)
+      triage_result, suspect_info, user_name,
+      master_flake_analysis.version_number)
   master_flake_analysis.put()
   return True
 
@@ -40,14 +39,11 @@ class TriageFlakeAnalysis(BaseHandler):
 
   def HandleGet(self):  # pragma: no cover
     """Sets the manual triage result for the suspected flake analysis."""
-    flake_info = self.request.get('flake_info')
-    (master_name, builder_name, build_number, step_name, test_name,
-     version_number, suspected_build_number) = flake_info.split('/')
+    key_urlsafe = self.request.get('key').strip()
+    suspected_build_number = self.request.get('suspected_build_number').strip()

[chanli, 2016/12/07 22:22:52]

I think suspected_build_number is actually
master_flake_analysis.suspected_flake_build_number? Do we need to have this
parameter?

[stgao, 2016/12/08 00:14:12]

+1

[lijeffrey, 2016/12/08 01:23:46]

Done.

     triage_result = self.request.get('triage_result')
 
-    if not (master_name and builder_name and build_number and step_name and
-            test_name and version_number and suspected_build_number and
-            str(triage_result)):
+    if not (key_urlsafe and suspected_build_number and str(triage_result)):

[stgao, 2016/12/08 00:14:12]

Should we test "triage_result is not None" instead?

[lijeffrey, 2016/12/08 01:23:46]

Done.

       # All fields needed for getting master_flake_analysis must be provided in
       # order to update triage results.
       return {'data': {'success': False}}
@@ -57,9 +53,7 @@ class TriageFlakeAnalysis(BaseHandler):
     user_name = users.get_current_user().email().split('@')[0]
 
     success = _UpdateSuspectedFlakeAnalysis(
-        master_name, builder_name, build_number, step_name, test_name,
-        int(version_number), suspected_build_number, int(triage_result),
-        user_name)
+        key_urlsafe, suspected_build_number, int(triage_result), user_name)
 
     return {'data': {'success': success}}