MozReview: WIP: Review Page Redesign: Move commits table above the main content area (Bug 1309964) draft
authorDavid Walsh <dwalsh@mozilla.com>
Fri, 14 Oct 2016 16:10:54 -0500
changeset 9704 a3ea7d2390f68325db7d1ab9c7b15015222b82f7
parent 9691 7a7c9ac9ee684e3c2d9309cc7c99171c8aa1bff2
child 9705 46e83379c27b21a1366769700e45602ae8fcb35e
push id1279
push userbmo:dwalsh@mozilla.com
push dateFri, 14 Oct 2016 21:11:28 +0000
bugs1309964
MozReview: WIP: Review Page Redesign: Move commits table above the main content area (Bug 1309964) MozReview-Commit-ID: KM0AyJQa31D
pylib/mozreview/mozreview/extension.py
pylib/mozreview/mozreview/fields.py
pylib/mozreview/mozreview/hooks.py
pylib/mozreview/mozreview/static/mozreview/css/commits.less
pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-reviews.less
pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less
pylib/mozreview/mozreview/static/mozreview/js/review.js
pylib/mozreview/mozreview/templates/mozreview/commits-requests.html
pylib/mozreview/mozreview/templates/mozreview/commits.html
--- a/pylib/mozreview/mozreview/extension.py
+++ b/pylib/mozreview/mozreview/extension.py
@@ -54,16 +54,17 @@ from mozreview.fields import (
     PullCommitField,
     TryField,
 )
 from mozreview.file_diff_reviewer.resources import (
     file_diff_reviewer_resource,
 )
 from mozreview.hooks import (
     MozReviewApprovalHook,
+    MozReviewCommitTableHook,
 )
 from mozreview.hostingservice.bmo_bugtracker import (
     BMOBugTracker,
 )
 from mozreview.hostingservice.hmo_repository import (
     HMORepository,
 )
 from mozreview.ldap.resources import (
@@ -310,16 +311,20 @@ class MozReviewExtension(Extension):
                      'mozreview/user-data.html')
         TemplateHook(self, 'base-after-content',
                      'mozreview/repository.html')
         TemplateHook(self, 'base-after-content',
                      'mozreview/user_review_flag.html',
                      apply_to=review_request_url_names)
 
         ReviewRequestFieldsHook(self, 'main', [CommitsListField])
+        MozReviewCommitTableHook(self, 'base-before-content',
+                     'mozreview/commits.html',
+                     apply_to=review_request_url_names)
+
         # This forces the Commits field to be the top item.
         main_fieldset.field_classes.insert(0,
                                            main_fieldset.field_classes.pop())
 
         # The above hack forced Commits at the top, but the rest of these
         # fields are fine below the Description.
         ReviewRequestFieldsHook(self, 'main', [CombinedReviewersField])
         ReviewRequestFieldsHook(self, 'main', [TryField])
--- a/pylib/mozreview/mozreview/fields.py
+++ b/pylib/mozreview/mozreview/fields.py
@@ -94,54 +94,17 @@ class CommitsListField(CommitDataBackedF
 
     def has_value_changed(self, old_value, new_value):
         # Just to be safe, we de-serialize the json and compare values
         if old_value is not None and new_value is not None:
             return json.loads(old_value) != json.loads(new_value)
         return old_value != new_value
 
     def should_render(self, value):
-        return is_pushed(self.review_request_details)
-
-    def as_html(self):
-        user = self.request.user
-        parent = get_parent_rr(self.review_request_details.get_review_request())
-        parent_details = parent.get_draft(user) or parent
-
-        # If a user can view the parent draft they should also have
-        # permission to view every child. We check if the child is
-        # accessible anyways in case it has been restricted for other
-        # reasons.
-        children_details = [
-            child for child in gen_child_rrs(parent_details, user=user)
-            if child.is_accessible_by(user)]
-
-        autoland_requests = AutolandRequest.objects.filter(
-            review_request_id=parent.id).order_by('-autoland_id')
-
-        repo_urls = set()
-        latest_autoland_requests = []
-
-
-        # We would like to fetch the latest AutolandRequest for each
-        # different repository.
-        for request in autoland_requests:
-            if request.repository_url in repo_urls:
-                continue
-
-            repo_urls.add(request.repository_url)
-            latest_autoland_requests.append(request)
-
-        return get_template('mozreview/commits.html').render(Context({
-            'review_request_details': self.review_request_details,
-            'parent_details': parent_details,
-            'children_details': children_details,
-            'latest_autoland_requests': latest_autoland_requests,
-            'user': user
-        }))
+        return False
 
     def render_change_entry_html(self, info):
         old_value = json.loads(info.get('old', ['[]'])[0])
         old_commits = [c for c, r in old_value]
 
         new_value = json.loads(info.get('new', ['[]'])[0])
         new_commits = [c for c, r in new_value]
 
--- a/pylib/mozreview/mozreview/hooks.py
+++ b/pylib/mozreview/mozreview/hooks.py
@@ -1,32 +1,100 @@
 from __future__ import unicode_literals
 
 import logging
 
-from reviewboard.extensions.hooks import ReviewRequestApprovalHook
+from django.template.loader import Context, get_template
+from django.utils.translation import ugettext as _
 
+from reviewboard.extensions.hooks import (
+    ReviewRequestApprovalHook,
+    ReviewRequestFieldsHook,
+    TemplateHook
+)
+
+from mozreview.autoland.models import AutolandEventLogEntry, AutolandRequest
 from mozreview.extra_data import (
     COMMIT_ID_KEY,
     fetch_commit_data,
     gen_child_rrs,
+    get_parent_rr,
     is_parent,
     is_pushed,
 )
 from mozreview.models import (
     get_profile,
 )
 from mozreview.review_helpers import (
     has_valid_shipit,
     has_l3_shipit,
 )
 
 
 logger = logging.getLogger(__name__)
 
+class MozReviewCommitTableHook(TemplateHook):
+
+    def get_extra_context(self, request, context):
+
+        user = request.user
+        review_request_details = context['review_request_details']
+
+        parent = get_parent_rr(review_request_details.get_review_request())
+        parent_details = parent.get_draft(user) or parent
+
+        # If a user can view the parent draft they should also have
+        # permission to view every child. We check if the child is
+        # accessible anyways in case it has been restricted for other
+        # reasons.
+        children_details = [
+            child for child in gen_child_rrs(parent_details, user=user)
+            if child.is_accessible_by(user)]
+
+        try:
+            current_child_index = children_details.index(review_request_details)
+            current_child_num = current_child_index + 1
+        except:
+            current_child_num = current_child_index = None
+            pass
+
+        next_child = prev_child = None
+        if(current_child_index != None):
+            if(current_child_index + 1 < len(children_details)):
+                next_child = children_details[current_child_index + 1]
+            if(current_child_index != 0):
+                prev_child = children_details[current_child_index - 1]
+
+        autoland_requests = AutolandRequest.objects.filter(
+            review_request_id=parent.id).order_by('-autoland_id')
+
+        repo_urls = set()
+        latest_autoland_requests = []
+
+        commit_data = fetch_commit_data(review_request_details)
+        commits_json = commit_data.get_for(review_request_details, 'p2rb.commits')
+
+        # We would like to fetch the latest AutolandRequest for each
+        # different repository.
+        for request in autoland_requests:
+            if request.repository_url in repo_urls:
+                continue
+
+            repo_urls.add(request.repository_url)
+            latest_autoland_requests.append(request)
+
+        return {'review_request_details': review_request_details,
+            'parent_details': parent_details,
+            'children_details': children_details,
+            'num_children': len(children_details),
+            'current_child_num': current_child_num,
+            'next_child': next_child,
+            'prev_child': prev_child,
+            'latest_autoland_requests': latest_autoland_requests,
+            'user': user}
 
 class MozReviewApprovalHook(ReviewRequestApprovalHook):
     """Calculates landing approval for review requests.
 
     This hook allows us to control the `approved` and `approval_failure`
     fields on review request model instances, and Web API results
     associated with them. By calculating landing approval and returning
     it here we have a nice way to distribute this decision throughout
--- a/pylib/mozreview/mozreview/static/mozreview/css/commits.less
+++ b/pylib/mozreview/mozreview/static/mozreview/css/commits.less
@@ -31,24 +31,34 @@
   border-radius: 50%;
   height: 15px;
   width: 15px;
   border: 1px solid #3E3E3E;
   cursor: default;
   margin-top: -1px;
 }
 
+#mozreview-review-header {
+    text-transform: uppercase;
+    color: #777;
+    display: block;
+    font-weight: normal;
+}
+
 #mozreview-child-requests {
   width: 100%;
   table-layout: fixed;
   border-spacing: 0px;
   margin-top:10px;
   min-width: 653px;
-  border-top: 1px solid #C1C1C1;
-  border-right: 1px solid #C1C1C1;
+  border-top: 1px solid #EEE;
+  border-right: 1px solid #EEE;
+  margin-bottom: 20px;
+  border: 1px solid #CCC;
+  font-size: 110%;
 
   tr, td {
     text-align: left;
   }
 
   .truncate_text {
     white-space: nowrap;
     overflow: hidden;
@@ -108,56 +118,69 @@
     &.review-cleared{
       color: grey;
     }
     &.review-cleared::after {
       content: " (r? cleared)";
     }
   }
 
-  .status {
-    width: 90px;
-    white-space: nowrap;
+  tbody > tr, tbody > tr:hover {
+    background: #F9F9F9;
   }
 
-  tbody > tr {
+  #mozreview-child-requests-nav-row {
     background: #fff;
+    font-size: 90%;
+
+    td {
+      padding: 7px;
+      position: relative;
+      text-align: center;
+    }
+
+    a[rel="next"] {
+      position: absolute;
+      right: 10px;
+    }
+
+    a[rel="previous"] {
+      position: absolute;
+      left: 10px;
+    }
   }
 
   .mozreview_commit_summary {
     font-weight: bold;
     text-decoration: none;
 
     &:hover, &:active, &:focus {
       text-decoration: underline;
     }
   }
 
   .diffstat-insert {
     color: green;
   }
 
   .diffstat-delete {
-    color: #a02222;
-  }
-
-  & > tbody > tr[current="true"],
-  & > tbody > tr:hover {
-    background-color: #eee;
+    color: #A02222;
   }
 
  td,
  th {
    padding: 5px;
-   border-left: 1px solid #C1C1C1;
-   border-bottom: 1px solid #C1C1C1;
+   border-left: 1px solid #EEE;
+   border-bottom: 1px solid #EEE;
  }
 
  & > thead > tr {
-   background: @moz-blue-dark;
+   background: #EEE;
+   line-height: 1.5;
+   font-size: 16px;
  }
 
  & > tbody td > a.commit_sha {
    font-family: monospace;
  }
 
  .reviewers {
    .inline-editor-form {
@@ -168,17 +191,26 @@
    }
    .buttons {
      white-space: nowrap;
    }
  }
 
  /* This was copied from the rb dashboard less file.*/
   .status {
+    width: auto;
     text-align: center;
+    background: #EEE;
+    font-weight: bold;
+    padding: 3px 10px;
+    display: inline-block;
+
+    &.approval {
+      background: #AEFF60;
+    }
   }
 
   .no-approval {
     cursor: default;
   }
 
   .approval {
     cursor: default;
@@ -188,18 +220,16 @@
   .approval-issues {
     background: #fff4B0;
     position: relative;
   }
 
   .issue-count {
     text-decoration: none;
     color: inherit;
-    display: block;
-    padding: 10px 0;
    }
 
   .shipit-count {
     background-image: url("../shipit_bg.png");
     background-position: top left;
     background-repeat: repeat-x;
     background-color: #6bc810;
     border: 1px solid @approval-border-color;
--- a/pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-reviews.less
+++ b/pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-reviews.less
@@ -1,31 +1,33 @@
 @import (reference) 'mozilla-theme-common.less';
 
+/* Maximum with for reviews content */
+#review_request .box-container, #reviews, #mozreview-review-header, #mozreview-child-requests {
+  max-width: 1200px;
+  margin-left: auto;
+  margin-right: auto;
+}
+
 /* Review Requests */
 .review-request {
   .main {
-    background: #fff;
+    background: #FFF;
     border: 0;
     border-radius: 0;
   }
 
   .content .field-container > pre {
     font-family: inherit;
     border: 0;
     padding: 0;
     font-size: 90%;
   }
 }
 
-.reviewable-page #mozreview-child-requests > thead > tr > .status,
-.reviewable-page #mozreview-child-requests > tbody > tr > .status {
-  min-width: 86px;
-}
-
 /* Review Banners */
 .banner {
   border-bottom: 0;
 }
 
 /* Review Comments */
 .box, .box-inner {
   .border-radius(0);
--- a/pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less
+++ b/pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less
@@ -1,13 +1,14 @@
 @import (reference) 'mozilla-theme-common.less';
 
 body {
   background: @moz-blue-light;
-  font-size: 13px;
+  font-family: arial;
+  font-size: 14px;
   padding-top: 8px;
 }
 
 /* Logo and Title */
 #headerbar {
   height: 18px;
 
   #title a {
--- a/pylib/mozreview/mozreview/static/mozreview/js/review.js
+++ b/pylib/mozreview/mozreview/static/mozreview/js/review.js
@@ -14,22 +14,29 @@
   // Change string of "Review" button to be a verb so people better
   // understand what clicking it does.
   $("#review-link").text("Finish Review...");
 
   if (MozReview.isParent) {
     $('#review_request_extra').prepend(MRParents.parentWarning);
   }
 
+  // Show all commits when link is clicked
+  $('#mozreview-all-commits').on('click', function(e) {
+    e.preventDefault();
+    $('#mozreview-child-requests tr[hidden]').removeAttr('hidden');
+    $(this).hide();
+  });
+
   var reviewRequest = RB.PageManager.getPage().reviewRequest;
   RB.apiCall({
     type: 'GET',
     prefix: reviewRequest.get('sitePrefix'),
     noActivityIndicator: true,
-    url: '/api/review-requests/'+reviewRequest.get('id')+'/reviews/' +
+    url: '/api/review-requests/' + reviewRequest.get('id') + '/reviews/' +
          '?max-results=200',
     success: function(data) {
       _.forEach(data.reviews, function(item) {
         var flag = item.extra_data['p2rb.review_flag'];
         var flagDesc = '';
         var reviewText = $('#review'+item.id+' .body');
         switch(flag){
           case ' ':
@@ -48,17 +55,17 @@
     }
   });
 
   // Tooltips for landable and "r?" cells
   $('#mozreview-child-requests .help-tooltip, #mozreview-child-requests tbody .status').each(function() {
      var $element = $(this);
      var text = $element.attr('title');
 
-     if(!text) return;
+     if (!text) return;
 
      $element.attr('title', '');
 
      // Draw the tooltip title and text
      var $tip = $('<div></div>').attr('class', 'review-tooltip').appendTo($element);
      $('<div></div>').attr('class', 'review-tooltip-text').text(text).appendTo($tip);
   });
 
--- a/pylib/mozreview/mozreview/templates/mozreview/commits-requests.html
+++ b/pylib/mozreview/mozreview/templates/mozreview/commits-requests.html
@@ -10,44 +10,79 @@ by both the commits template and the com
 <table id="mozreview-child-requests">
   <thead>
   <tr>
     <th class="commits">{% trans "Commit" %}</th>
     {% comment "TODO: show this column when the commit author will be available" %}
     <th class="submitter">{% trans "Submitter" %}</th>
     {% endcomment %}
     <th class="reviewers">{% trans "Reviewers" %}</th>
-    <th class="status">
-      {% trans "Landable" %}
-      <div class="help-icon help-tooltip" title="{% trans "Hover over each commit's status to view landable status" %}"><span>?</span></div>
-    </th>
   </tr>
   </thead>
+
+  {% if parent_details.get_review_request.id != review_request_details.get_review_request.id and num_children > 1 %}
+  <tr id="mozreview-child-requests-nav-row">
+      <td colspan="2">
+        {% if prev_child %}
+          <a href="{{prev_child.get_review_request.get_absolute_url}}" rel="previous">&lt;&lt; Previous Commit</a>
+        {% endif %}
+        Viewing commit {{current_child_num}} of {{num_children}}. <a href="javascript:;" id="mozreview-all-commits">View All Commits</a>
+        {% if next_child %}
+          <a href="{{next_child.get_review_request.get_absolute_url}}" rel="next">Next Commit &gt;&gt;</a>
+        {% endif %}
+      </td>
+  </tr>
+  {% endif %}
+
+
   {% for child_details in children_details %}
-  <tr {% if child_details.get_review_request.id = review_request_details.get_review_request.id %}current="true"{% endif %}>
+  <tr
+    {% if child_details.get_review_request.id = review_request_details.get_review_request.id %}
+      current="true"
+    {% else %}
+      {% if parent_details.get_review_request.id != review_request_details.get_review_request.id %}
+        hidden
+      {% endif %}
+    {% endif %}>
 
   <td class="commits">
     <div class="truncate_text">
-        <a class="mozreview_commit_summary" title="See diff for commit {{child_details|commit_id|slice:":12"}}"
-        href="{{child_details.get_review_request.get_absolute_url}}diff/#index_header">
-          {{ child_details.summary }}
-        </a>
-        <ul>
-          <li>
-            <a href="{{child_details.get_review_request.repository.path}}/raw-rev/{{child_details|commit_id|slice:":12"}}">
-              {% trans "View Raw" %}
-            </a>
-          </li><li>
-            <a href="{{child_details.get_review_request.get_absolute_url}}" title="{{ child_details.summary}}">
-                {% trans "View Reviews" %}
-            </a>
-        </li><li class="diffstat" title="{% trans "Lines inserted / deleted" %}">
-            {{ child_details.get_review_request|diffstat_text:user }}
-          </li>
-        </ul>
+      {% if child_details.get_review_request.issue_open_count > 0 %}
+        <div class="status approval-issues" title="{{child_details.get_review_request.issue_open_count}} open issues">
+          <a class="issue-count" href="{{child_details.get_review_request.get_absolute_url}}#issue-summary">
+            ! {{child_details.get_review_request.issue_open_count}}
+          </a>
+        </div>
+      {% elif child_details.get_review_request.approved %}
+        <div class="status approval" title="Approved For Landing - You have at least one valid ship it!">
+          r+
+        </div>
+      {% else %}
+        <div class="status no-approval" title="{{child_details.get_review_request.approval_failure}}">
+          r?
+        </div>
+      {% endif %}
+
+      <a class="mozreview_commit_summary" title="See diff for commit {{child_details|commit_id|slice:":12"}}"
+      href="{{child_details.get_review_request.get_absolute_url}}diff/#index_header">
+        {{ child_details.summary }}
+      </a>
+      <ul style="margin-left: 40px;font-size: inherit;">
+        <li>
+          <a href="{{child_details.get_review_request.repository.path}}/raw-rev/{{child_details|commit_id|slice:":12"}}">
+            {% trans "View Raw" %}
+          </a>
+        </li><li>
+          <a href="{{child_details.get_review_request.get_absolute_url}}" title="{{ child_details.summary}}">
+              {% trans "View Reviews" %}
+          </a>
+      </li><li class="diffstat" title="{% trans "Lines inserted / deleted" %}">
+          {{ child_details.get_review_request|diffstat_text:user }}
+        </li>
+      </ul>
     </div>
   </td>
 
     {% comment "TODO: show this column when the commit author will be available" %}
       <td>{{ child_details.submitter }}</td>
     {% endcomment %}
 
     <td class="reviewers">
@@ -58,28 +93,11 @@ by both the commits template and the com
           {% if child_details|isDraft %}
             <span class="reviewer-name">{{ reviewer }}</span>
           {% else %}
             <span class="reviewer-name {% if status.ship_it %}reviewer-ship-it{% endif %} {{status.review_flag|review_flag_class}}">{{ reviewer }}</span>
           {% endif %}
         {% endfor %}
       </span>
     </td>
-
-    {% if child_details.get_review_request.issue_open_count > 0 %}
-      <td class="status approval-issues" title="{{child_details.get_review_request.issue_open_count}} open issues">
-        <a class="issue-count" href="{{child_details.get_review_request.get_absolute_url}}#issue-summary">
-          ! {{child_details.get_review_request.issue_open_count}}
-        </a>
-      </td>
-    {% elif child_details.get_review_request.approved %}
-      <td class="status approval" title="Approved For Landing - You have at least one valid ship it!">
-        r+
-      </td>
-    {% else %}
-      <td class="status no-approval" title="{{child_details.get_review_request.approval_failure}}">
-        r?
-      </td>
-    {% endif %}
-
   </tr>
   {% endfor %}
 </table>
--- a/pylib/mozreview/mozreview/templates/mozreview/commits.html
+++ b/pylib/mozreview/mozreview/templates/mozreview/commits.html
@@ -16,21 +16,35 @@ This is the template for the "Commits" l
 </div>
 
 <div id="mozreview-data"
   data-parent-review-id="{{ parent_details.get_review_request.id }}"
   data-selected-review-id="{{ review_request_details.get_review_request.id }}"
 ></div>
 
 <div id="mozreview-request-series">
-  <div id="mozreview-parent-request">
-    <a href="{{parent_details.get_review_request.get_absolute_url}}">Review Summary</a>
-    <a href="{{parent_details.get_review_request.get_absolute_url}}diff/#index_header">Squashed Diff</a>
+  <div id="mozreview-review-header">
+      {% if child_details.get_review_request.id = review_request_details.get_review_request.id %}
+        Review Summary
+      {% else %}
+        <a href="{{parent_details.get_review_request.get_absolute_url}}">Review Summary</a>
+      {% endif %}
+      &bull;
+      {% if review_request_details.get_review_request.bugs_closed %}
+      <a href="https://bugzilla.mozilla.org/show_bug.cgi?id={{review_request_details.get_review_request.bugs_closed}}">Bug #{{review_request_details.get_review_request.bugs_closed}}</a>
+      &bull;
+      {% endif %}
+      {{parent_details.get_review_request.repository}}
+      {% if parent_details.get_review_request.id = review_request_details.get_review_request.id %}
+        &bull; <a href="{{parent_details.get_review_request.get_absolute_url}}diff/#index_header">Squashed Diff</a>
+      {% endif %}
   </div>
+
   {% include 'mozreview/commits-requests.html' %}
+
   {% if latest_autoland_requests %}
   <div id="ci-actions">
     {% for autoland_request in latest_autoland_requests %}
       {% if not forloop.first %}
       <div class="action-separator"></div>
       {% endif %}
 
       {% if autoland_request.last_known_status == 'P' %}