MozReview: WIP: Review Page Redesign: Move commits table above the main content area (
Bug 1309964)
MozReview-Commit-ID: KM0AyJQa31D
--- 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"><< 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 >></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 %}
+ •
+ {% 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>
+ •
+ {% endif %}
+ {{parent_details.get_review_request.repository}}
+ {% if parent_details.get_review_request.id = review_request_details.get_review_request.id %}
+ • <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' %}