From 1fa5cb9ce3444c1e7d5ec624238b865f8e520f6e Mon Sep 17 00:00:00 2001
From: "ilevy@chromium.org"
 <ilevy@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Wed, 5 Dec 2012 04:04:42 +0000
Subject: [PATCH] Add presubmit check to verify issue is not closed.

This can come up when CQ closed an issue but the user's
local branch is still tied to the issue.

BUG=161702


Review URL: https://chromiumcodereview.appspot.com/11348122

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@171153 0039d316-1c4b-4281-b951-d872f2087c98
---
 presubmit_canned_checks.py  | 33 ++++++++++++++++++++++++++++++---
 tests/presubmit_unittest.py | 22 +++++++++++++++++++++-
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py
index af88a39b1..e6384fa0c 100644
--- a/presubmit_canned_checks.py
+++ b/presubmit_canned_checks.py
@@ -791,17 +791,40 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
   return []
 
 
+def _GetRietveldIssueProps(input_api, messages):
+  """Gets the issue properties from rietveld."""
+  issue = input_api.change.issue
+  if issue and input_api.rietveld:
+    return input_api.rietveld.get_issue_properties(
+        issue=int(issue), messages=messages)
+
+
+def CheckIssueNotClosed(input_api, output_api):
+  """Verifies issue is not closed.
+
+  We should not be working with a closed review. CQ and dcommit set this bit,
+  so it is a pretty good indicator of whether an issue has been committed.
+  """
+  issue_props = _GetRietveldIssueProps(input_api=input_api, messages=False)
+  if issue_props and issue_props['closed']:
+    return [output_api.PresubmitError(
+        'Issue %s is closed.  If this issue was already used for a commit,\n'
+        'please reset the issue number associated with this branch with:\n'
+        'git cl issue 0\n' % issue_props['issue']
+        )]
+  return []
+
+
 def _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed=False):
   """Return the owner and reviewers of a change, if any.
 
   If approval_needed is True, only reviewers who have approved the change
   will be returned.
   """
-  if not input_api.change.issue:
+  issue_props = _GetRietveldIssueProps(input_api, True)
+  if not issue_props:
     return None, None
 
-  issue_props = input_api.rietveld.get_issue_properties(
-      int(input_api.change.issue), True)
   if not approval_needed:
     return issue_props['owner_email'], set(issue_props['reviewers'])
 
@@ -940,6 +963,10 @@ def PanProjectChecks(input_api, output_api,
     results.extend(input_api.canned_checks.CheckOwners(
         input_api, output_api, source_file_filter=None))
 
+  snapshot("checking review not closed")
+  results.extend(input_api.canned_checks.CheckIssueNotClosed(
+      input_api, output_api))
+
   snapshot("checking long lines")
   results.extend(input_api.canned_checks.CheckLongLines(
       input_api, output_api, source_file_filter=sources))
diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index cf449dcc8..49f538693 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -1507,6 +1507,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
       'CheckLongLines', 'CheckTreeIsOpen', 'PanProjectChecks',
       'CheckLicense',
       'CheckOwners',
+      'CheckIssueNotClosed',
       'CheckRietveldTryJobExecution',
       'CheckSingletonInHeaders',
       'CheckSvnModifiedDirectories',
@@ -2253,7 +2254,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
 
       if issue:
         input_api.rietveld.get_issue_properties(
-            int(input_api.change.issue), True).AndReturn(rietveld_response)
+            issue=int(input_api.change.issue), messages=True).AndReturn(
+                rietveld_response)
         people.add(owner_email)
 
       fake_db.directories_not_covered_by(set(['foo/xyz.cc']),
@@ -2394,6 +2396,24 @@ class CannedChecksUnittest(PresubmitTestsBase):
                            is_committing=False,
                            uncovered_dirs=set())
 
+  def CheckIssueClosedBase(self, closed):
+    input_api = self.MockInputApi(
+        presubmit.Change('', '', None, None, 1, 0, None), False)
+    input_api.rietveld.get_issue_properties(
+        issue=int(input_api.change.issue), messages=False).AndReturn(
+            {'closed': closed, 'issue': 1})
+    self.mox.ReplayAll()
+    return presubmit_canned_checks.CheckIssueNotClosed(
+        input_api, presubmit.OutputApi)
+
+  def testIssueOpen(self):
+    self.assertEqual([], self.CheckIssueClosedBase(False))
+
+  def testIssueClosed(self):
+    results = self.CheckIssueClosedBase(True)
+    self.assertEqual(len(results), 1)
+    self.assertTrue(results[0].fatal)
+
   def testCannedRunUnitTests(self):
     change = presubmit.Change(
         'foo1', 'description1', self.fake_root_dir, None, 0, 0, None)