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

appengine/findit/handlers/flake/triage_flake_analysis.py

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """This module is to handle manual triage of a suspected flake result.
 
 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
 from common.base_handler import Permission
 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
 
   suspect_info = {
       'build_number': suspected_build_number
   }
 
   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
 
 
 class TriageFlakeAnalysis(BaseHandler):
   PERMISSION_LEVEL = Permission.CORP_USER
   LOGIN_REDIRECT_TO_DISTINATION_PAGE_FOR_GET = False
 
   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}}
 
     # As the permission level is CORP_USER, we could assume the current user
     # already logged in.
     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}}
 
   def HandlePost(self):  # pragma: no cover
     return self.HandleGet()