Chromium

codereview/models.py

@@ -158,6 +158,11 @@ def num_messages(self, val):
   def patchsets(self):
     return PatchSet.query(ancestor=self.key).order(Issue.created)
 
+  def most_recent_patchset_key(self):
+    query = PatchSet.query(ancestor=self.key).order(-Issue.created)
+    ps_key = query.get(keys_only=True)
+    return ps_key
+
   @property
   def messages(self):
     return Message.query(ancestor=self.key).order(Message.date)
@@ -273,7 +278,16 @@ def calculate_updates_for(self, *msgs):
     updates_for_set = set(self.updates_for)
     approval_dict = {r: None for r in self.reviewers}
     self.num_messages = 0
-    old_messages = Message.query(Message.draft == False, ancestor=self.key).order(Message.date)
+    old_messages = Message.query(
+        Message.draft == False,
+        ancestor=self.key).order(Message.date)
+    # We cannot put this condition in the query because:
+    # (a) auto_generated == False does not return legacy messages
+    # and (b) auto_generated != True conflicts with sorting.
+    old_messages = [
+      msg for msg in old_messages if not msg.auto_generated]
+
+
     for msg in itertools.chain(old_messages, msgs):
       if self._original_subject is None:
         self._original_subject = msg.subject
@@ -509,6 +523,8 @@ def status(self):
       return 'success'
     if self.result == self.SKIPPED:
       return 'skipped'
+    elif self.result == self.EXCEPTION:
+      return 'exception'
     elif self.result in self.FAIL:
       return 'failure'
     elif self.result == self.TRYPENDING:
@@ -673,6 +689,11 @@ class Message(ndb.Model):
   issue_was_closed = ndb.BooleanProperty(default=False)
   # If message came in through email, we might not count "lgtm"
   was_inbound_email = ndb.BooleanProperty(default=False)
+  # Whether this Message was auto generated in response to an action the system
+  # would like to log. Eg: Checking CQ checkbox or changing reviewers.
+  auto_generated = ndb.BooleanProperty(default=False)
+  # Patchset that the user was responding to.
+  patchset_key = ndb.KeyProperty(PatchSet)
 
   _approval = None
   _disapproval = None

codereview/revert_patchset.py

@@ -40,14 +40,15 @@
 
 
 def _get_revert_description(request, revert_reason, reviewers, original_issue,
-                            original_patch_num):
+                            original_patch_num, original_patchset_id):
   """Creates and returns a description for the revert CL."""
   revert_description = []
   # Contain link to original CL.
   original_issue_link = request.build_absolute_uri(
       reverse('codereview.views.show', args=[original_issue.key.id()]))
-  revert_description.append('Revert of %s (patchset #%s of %s)' % (
-      original_issue.subject, original_patch_num, original_issue_link))
+  revert_description.append('Revert of %s (patchset #%s id:%s of %s)' % (
+      original_issue.subject, original_patch_num, original_patchset_id,
+      original_issue_link))
   # Display the reason for reverting.
   revert_description.append('')  # Extra new line to separate sections.
   revert_description.append('Reason for revert:')
@@ -131,6 +132,7 @@ def revert_patchset(request):
   original_issue = request.issue
   original_patch_num = len(list(original_issue.patchsets))
   original_patchset = request.patchset
+  original_patchset_id = original_patchset.key.id()
   original_patches = original_patchset.patches
 
   # Make sure the original issue is closed.
@@ -167,7 +169,8 @@ def revert_patchset(request):
   revert_reason = request.POST['revert_reason']
   revert_cq = request.POST['revert_cq'] == '1'
   description = _get_revert_description(
-      request, revert_reason, reviewers, original_issue, original_patch_num)
+      request, revert_reason, reviewers, original_issue, original_patch_num,
+      original_patchset_id)
   issue = models.Issue(subject=subject,
                        description=description,
                        project=original_issue.project,
@@ -274,10 +277,10 @@ def revert_patchset(request):
   revert_issue_link = request.build_absolute_uri(
       reverse('codereview.views.show', args=[issue.key.id()]))
   revert_message = (
-      'A revert of this CL (patchset #%s) has been created in %s by %s.\n\n'
-      'The reason for reverting is: %s.' % (
-          original_patch_num, revert_issue_link, request.user.email(),
-          revert_reason))
+      'A revert of this CL (patchset #%s id:%s) has been created in %s by '
+      '%s.\n\nThe reason for reverting is: %s.' % (
+          original_patch_num, original_patchset_id, revert_issue_link,
+          request.user.email(), revert_reason))
   views.make_message(request, original_issue, revert_message,
                      send_mail=True).put()
   # Notify the revert issue recipients.

codereview/views.py

@@ -1523,10 +1523,19 @@ def show(request):
     if last_patchset.patches:
       first_patch = last_patchset.patches[0]
   messages = []
+  generated_messages = []
   has_draft_message = False
+  # Keep track of the last non-generated message.
+  message_index = -1
+  last_user_message_index = -1
   for msg in request.issue.messages:
+    if msg.auto_generated:
+      generated_messages.append(msg)
     if not msg.draft:
       messages.append(msg)
+      message_index += 1
+      if not msg.auto_generated:
+        last_user_message_index = message_index
     elif msg.draft and request.user and msg.sender == request.user.email():
       has_draft_message = True
   num_patchsets = len(patchsets)
@@ -1562,6 +1571,8 @@ def show(request):
     'issue': request.issue,
     'last_patchset': last_patchset,
     'messages': messages,
+    'generated_messages': generated_messages,
+    'last_user_message_index': last_user_message_index,
     'num_patchsets': num_patchsets,
     'patchsets': patchsets,
     'src_url': src_url,
@@ -1643,7 +1654,8 @@ def _log_reviewers_if_changed(request, orig_reviewers, new_reviewers):
     reviewers_msg += '+ %s\n' % ', '.join(sorted(additions_set))
   if removals_set:
     reviewers_msg += '- %s\n' % ', '.join(sorted(removals_set))
-  make_message(request, request.issue, reviewers_msg, send_mail=False).put()
+  make_message(request, request.issue, reviewers_msg, send_mail=False,
+               auto_generated=True).put()
 
 
 @deco.issue_editor_required
@@ -1667,7 +1679,12 @@ def edit(request):
                              'closed': issue.closed,
                              'private': issue.private,
                              })
-    return respond(request, 'edit.html', {'issue': issue, 'form': form})
+    return respond(request, 'edit.html', {
+        'issue': issue,
+        'form': form,
+        'offer_delete': (issue.owner == request.user
+                         or auth_utils.is_current_user_admin())
+        })
 
   form = EditLocalBaseForm(request.POST)
 
@@ -1763,7 +1780,8 @@ def delete_patchset(request):
       break
   delete_msg = 'Patchset #%s (id:%s) has been deleted' % (
       patchset_num, request.patchset.key.id())
-  make_message(request, request.issue, delete_msg, send_mail=False).put()
+  make_message(request, request.issue, delete_msg, send_mail=False,
+               auto_generated=True).put()
   # Delete the patchset.
   request.patchset.nuke()
   return HttpResponseRedirect(reverse(show, args=[request.issue.key.id()]))
@@ -2020,6 +2038,8 @@ def _issue_as_dict(issue, messages, request=None):
         'text': m.text,
         'approval': m.approval,
         'disapproval': m.disapproval,
+        'auto_generated': m.auto_generated,
+        'patchset': m.patchset_key.id() if m.patchset_key else None,
       }
       for m in models.Message.query(ancestor=issue.key)),
       key=lambda x: x['date'])
@@ -2746,7 +2766,9 @@ def _get_mail_template(request, issue, full_diff=False):
   template = 'mails/comment.txt'
   if request.user == issue.owner:
     query = models.Message.query(
-        models.Message.sender == request.user.email(), ancestor=issue.key)
+        models.Message.sender == request.user.email(),
+        models.Message.auto_generated != True,
+        ancestor=issue.key)
     if query.count(1) == 0:
       template = 'mails/review.txt'
       files, patch = _get_affected_files(issue, full_diff)
@@ -2831,14 +2853,15 @@ def publish(request):
     return respond(request, 'publish.html', {'form': form, 'issue': issue})
 
   _log_reviewers_if_changed(request=request, orig_reviewers=issue.reviewers,
-                            new_reviewers=reviewers)
+      new_reviewers=reviewers)
   issue.reviewers = reviewers
   issue.cc = cc
   if form.cleaned_data['commit'] and not issue.closed:
     issue.commit = True
     commit_checked_msg = 'The CQ bit was checked by %s' % (
         request.user.email().lower())
-    make_message(request, issue, commit_checked_msg, send_mail=False).put()
+    make_message(request, issue, commit_checked_msg, send_mail=False,
+                 auto_generated=True).put()
   if not form.cleaned_data.get('message_only', False):
     tbd, comments = _get_draft_comments(request, issue)
   else:
@@ -3014,7 +3037,7 @@ def _get_modified_counts(issue):
 
 
 def make_message(request, issue, message, comments=None, send_mail=False,
-                  draft=None, in_reply_to=None):
+                  draft=None, in_reply_to=None, auto_generated=False):
   """Helper to create a Message instance and optionally send an email."""
   attach_patch = request.POST.get("attach_patch") == "yes"
   template, context = _get_mail_template(request, issue, full_diff=attach_patch)
@@ -3055,7 +3078,8 @@ def make_message(request, issue, message, comments=None, send_mail=False,
                          recipients=reply_to,
                          text=text,
                          parent=issue.key,
-                         issue_was_closed=issue.closed)
+                         issue_was_closed=issue.closed,
+                         auto_generated=auto_generated)
   else:
     msg = draft
     msg.subject = subject
@@ -3064,7 +3088,11 @@ def make_message(request, issue, message, comments=None, send_mail=False,
     msg.draft = False
     msg.date = datetime.datetime.now()
     msg.issue_was_closed = issue.closed
-  issue.calculate_updates_for(msg)
+    msg.auto_generated = auto_generated
+  if not auto_generated:
+    issue.calculate_updates_for(msg)
+
+  msg.patchset_key = issue.most_recent_patchset_key()
 
   if in_reply_to:
     try:
@@ -4129,8 +4157,8 @@ def process_issue(
 
   lgtms = sum(
       m.sender == user and
-      m.find('lgtm', owner_allowed=True) and
-      not m.find('no lgtm', owner_allowed=True)
+      m.find(models.Message.LGTM_RE, owner_allowed=True) and
+      not m.find(models.Message.NOT_LGTM_RE, owner_allowed=True)
       for m in messages)
 
   # TODO(maruel): Check for the base username part, e.g.:

codereview/views_chromium.py

@@ -46,7 +46,7 @@
 from codereview import views
 
 
-# This is the number of patches to lint in each task run.  It has 10 minutes 
+# This is the number of patches to lint in each task run.  It has 10 minutes
 # to run.  Linting on large files can take ~10s per file.
 LINT_BATCH_SIZE = 50
 
@@ -253,7 +253,7 @@ def inner_handle(reason, base_url, timestamp, packet, result, properties):
       requester = users.User(properties['requester'])
     except users.UserNotFoundError:
       pass
-  
+
   def tx_try_job_result():
     if try_job_key:
       try_obj = ndb.Key(urlsafe=try_job_key).get()
@@ -439,7 +439,7 @@ def edit_flags(request):
     action = 'checked' if request.issue.commit else 'unchecked'
     commit_checked_msg = 'The CQ bit was %s by %s' % (action, user_email)
     views.make_message(request, request.issue, commit_checked_msg,
-                       send_mail=False).put()
+                       send_mail=False, auto_generated=True).put()
     request.issue.put()
 
   if 'builders' in request.POST:

index.yaml

@@ -58,6 +58,13 @@ indexes:
   - name: modified
     direction: desc
 
+- kind: Message
+  ancestor: yes
+  properties:
+  - name: sender
+  - name: auto_generated
+
+
 # AUTOGENERATED
 
 # This index.yaml is automatically updated whenever the dev_appserver
@@ -325,6 +332,13 @@ indexes:
   - name: date
     direction: desc
 
+- kind: Message
+  ancestor: yes
+  properties:
+  - name: auto_generated
+  - name: draft
+  - name: date
+
 - kind: Message
   ancestor: yes
   properties:

static/script.js

@@ -768,31 +768,55 @@ function M_switchInlineComment(cid, lineno, side) {
 }
 
 /**
- * Used to expand all comments, hiding the preview and showing the comment.
+ * Used to expand all visible comments, hiding the preview and showing the
+ * comment.
  * @param {String} prefix The level of the comment -- one of
  *                        ('cl', 'file', 'inline')
  * @param {Integer} num_comments The number of comments to show
  */
-function M_showAllComments(prefix, num_comments) {
+function M_expandAllVisibleComments(prefix, num_comments) {
   for (var i = 0; i < num_comments; i++) {
     M_hideElement(prefix + "-preview-" + i);
     M_showElement(prefix + "-comment-" + i);
   }
 }
 
 /**
- * Used to collpase all comments, showing the preview and hiding the comment.
+ * Used to collapse all visible comments, showing the preview and hiding the
+ * comment.
  * @param {String} prefix The level of the comment -- one of
  *                        ('cl', 'file', 'inline')
  * @param {Integer} num_comments The number of comments to hide
  */
-function M_hideAllComments(prefix, num_comments) {
+function M_collapseAllVisibleComments(prefix, num_comments) {
   for (var i = 0; i < num_comments; i++) {
     M_showElement(prefix + "-preview-" + i);
     M_hideElement(prefix + "-comment-" + i);
   }
 }
 
+/**
+ * Used to show all auto_generated comments.
+ * @param {Integer} num_comments The total number of comments to loop through
+ */
+function M_showGeneratedComments(num_comments) {
+  for (var i = 0; i < num_comments; i++) {
+    // The top level msg div starts at index 1.
+    M_showElement("generated-msg" + (i+1));
+  }
+}
+
+/**
+ * Used to hide all auto_generated comments.
+ * @param {Integer} num_comments The total number of comments to loop through
+ */
+function M_hideGeneratedComments(num_comments) {
+  for (var i = 0; i < num_comments; i++) {
+    // The top level msg div starts at index 1.
+    M_hideElement("generated-msg" + (i+1));
+  }
+}
+
 // Common methods for comment handling (changelist.html, file.html,
 // comment_form.html)
 

static/styles.css

@@ -799,6 +799,12 @@ span.visualtab {
   margin-bottom: .8em;
 }
 
+/* auto-generated messages have a light grey background and the default
+   border */
+.message.generated .header {
+  background-color: #EEE;
+}
+
 /* Approval messages have light green header background and dark green border */
 .message.approval .header {
   background-color: #7FFF7F;
@@ -894,6 +900,10 @@ ul.errorlist li {
   padding: 1px 0;
 }
 
+.build-status-color-exception {
+  background-color: rgb(224, 176, 255);
+}
+
 .build-status-color-failure {
   background-color: rgb(233, 128, 128);
 }
@@ -974,4 +984,10 @@ input:checked ~ span .if_checked {
   display: block;
   font-weight: bold;
 }
-
+
+#issue-description {
+  font-family: monospace;
+  font-size: 12px;
+  margin-left: 15px;
+  white-space: pre-wrap;
+}