diff --git a/reviewboard/diffviewer/commit_utils.py b/reviewboard/diffviewer/commit_utils.py index 723da1fd5678a26d0169689e8192d40215b36f84..22b339a708811ac92864831f19f15eaa1a15aa5b 100644 --- a/reviewboard/diffviewer/commit_utils.py +++ b/reviewboard/diffviewer/commit_utils.py @@ -6,6 +6,8 @@ import base64 import json from itertools import chain +from django.utils.six.moves import zip + from reviewboard.scmtools.core import PRE_CREATION, UNKNOWN @@ -214,3 +216,171 @@ def update_validation_info(validation_info, commit_id, parent_id, filediffs): } return validation_info + + +class CommitHistoryDiffEntry(object): + """An entry in a commit history diff.""" + + COMMIT_ADDED = 'added' + COMMIT_REMOVED = 'removed' + COMMIT_MODIFIED = 'modified' + COMMIT_UNMODIFIED = 'unmodified' + + entry_types = ( + COMMIT_ADDED, + COMMIT_REMOVED, + COMMIT_MODIFIED, + COMMIT_UNMODIFIED, + ) + + def __init__(self, entry_type, old_commit=None, new_commit=None): + """Initialize the CommitHistoryDiffEntry object. + + Args: + entry_type (unicode): + The commit type. This must be one of the values in + :py:attr:`entry_types`. + + old_commit (reviewboard.diffviewer.models.diffcommit.DiffCommit, + optional): + The old commit. This is required if the commit type is one of: + + * :py:data:`COMMIT_REMOVED` + * :py:data:`COMMIT_MODIFIED` + * :py:data:`COMMIT_UNMODIFIED` + + new_commit (reviewboard.diffviewer.models.diffcommit.DiffCommit, + optional): + The new commit. This is required if the commit type is one of: + + * :py:data:`COMMIT_ADDED` + * :py:data:`COMMIT_MODIFIED` + * :py:data:`COMMIT_UNMODIFIED` + + Raises: + ValueError: + The value of ``entry_type`` was invalid or the wrong commits + were specified. + """ + if entry_type not in self.entry_types: + raise ValueError('Invalid entry_type: "%s"' % entry_type) + + if not old_commit and entry_type != self.COMMIT_ADDED: + raise ValueError('old_commit required for given commit type.') + + if not new_commit and entry_type != self.COMMIT_REMOVED: + raise ValueError('new_commit required for given commit type') + + self.entry_type = entry_type + self.old_commit = old_commit + self.new_commit = new_commit + + def serialize(self): + """Serialize the entry to a dictionary. + + Returns: + dict: + A dictionary of the serialized information. + """ + result = { + 'entry_type': self.entry_type, + } + + if self.new_commit: + result['new_commit_id'] = self.new_commit.commit_id + + if self.old_commit: + result['old_commit_id'] = self.old_commit.commit_id + + return result + + def __eq__(self, other): + """Compare two entries for equality. + + Two entries are equal if and only if their attributes match. + + Args: + other (CommitHistoryDiffEntry): + The entry to compare against. + + Returns: + bool: + Whether or not this entry and the other entry are equal. + """ + return (self.entry_type == other.entry_type, + self.old_commit == other.old_commit, + self.new_commit == other.new_commit) + + def __ne__(self, other): + """Compare two entries for inequality. + + Two entries are not equal if and only if any of their attributes don't + match. + + Args: + other (CommitHistoryDiffEntry): + The entry to compare against. + + Returns: + bool: + Whether or not this entry and the other entry are not equal. + """ + return not self == other + + def __repr__(self): + """Return a string representation of the entry. + + Returns: + unicode: + A string representation of the entry. + """ + return ( + '' + % (self.entry_type, self.old_commit, self.new_commit) + ) + + +def diff_histories(old_history, new_history): + """Yield the entries in the diff between the old and new histories. + + Args: + old_history (list of reviewboard.diffviewer.models.diffcommit. + DiffCommit): + The list of commits from a previous + :py:class:`~reviewboard.diffviewer.models.diffset.DiffSet`. + + new_history (list of reviewboard.diffviewer.models.diffcommit. + DiffCommit): + The list of commits from the new + :py:class:`~reviewboard.diffviewer.models.diffset.DiffSet`. + + Yields: + CommitHistoryDiffEntry: + The history entries. + """ + i = 0 + + # This is not quite the same as ``enumerate(...)`` because if we run out + # of history, ``i`` will not be incremented. + + for old_commit, new_commit in zip(old_history, new_history): + if old_commit.commit_id != new_commit.commit_id: + break + + yield CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_UNMODIFIED, + old_commit=old_commit, + new_commit=new_commit) + + i += 1 + + for old_commit in old_history[i:]: + yield CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_REMOVED, + old_commit=old_commit) + + for new_commit in new_history[i:]: + yield CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_ADDED, + new_commit=new_commit) diff --git a/reviewboard/diffviewer/tests/test_commit_utils.py b/reviewboard/diffviewer/tests/test_commit_utils.py index 1eb348a2a0c33240a36ad8af7f155def7d2f4719..cc5ed53ab2d96482aff150ff011fe0d980461198 100644 --- a/reviewboard/diffviewer/tests/test_commit_utils.py +++ b/reviewboard/diffviewer/tests/test_commit_utils.py @@ -4,8 +4,11 @@ from __future__ import unicode_literals from kgb import SpyAgency -from reviewboard.diffviewer.commit_utils import (exclude_ancestor_filediffs, +from reviewboard.diffviewer.commit_utils import (CommitHistoryDiffEntry, + diff_histories, + exclude_ancestor_filediffs, get_file_exists_in_history) +from reviewboard.diffviewer.models import DiffCommit from reviewboard.diffviewer.tests.test_diffutils import \ BaseFileDiffAncestorTests from reviewboard.scmtools.core import UNKNOWN @@ -454,3 +457,235 @@ class ExcludeAncestorFileDiffsTests(BaseFileDiffAncestorTests): } self.assertEqual(expected, set(result)) + + +class DiffHistoriesTests(TestCase): + """Unit tests for reviewboard.diffviewer.commit_utils.diff_histories.""" + + def test_diff_histories_identical(self): + """Testing diff_histories with identical histories""" + new_history = old_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r1'), + DiffCommit(commit_id='r2'), + ] + + expected_result = [ + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_UNMODIFIED, + old_commit=old_history[0], + new_commit=new_history[0]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_UNMODIFIED, + old_commit=old_history[1], + new_commit=new_history[1]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_UNMODIFIED, + old_commit=old_history[2], + new_commit=new_history[2]), + ] + + self.assertEqual(list(diff_histories(old_history, new_history)), + expected_result) + + def test_diff_histories_added(self): + """Testing diff_histories with a new history that adds commits at the + end of the history + """ + old_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r1'), + ] + + new_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r1'), + DiffCommit(commit_id='r2'), + ] + + expected_result = [ + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_UNMODIFIED, + old_commit=old_history[0], + new_commit=new_history[0]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_UNMODIFIED, + old_commit=old_history[1], + new_commit=new_history[1]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_ADDED, + new_commit=new_history[2]), + ] + + self.assertEqual(list(diff_histories(old_history, new_history)), + expected_result) + + def test_diff_histories_removed(self): + """Testing diff_histories with a new history that removes commits""" + old_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r1'), + DiffCommit(commit_id='r2'), + ] + + new_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r1'), + ] + + expected_result = [ + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_UNMODIFIED, + old_commit=old_history[0], + new_commit=new_history[0]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_UNMODIFIED, + old_commit=old_history[1], + new_commit=new_history[1]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_REMOVED, + old_commit=old_history[2]), + ] + + self.assertEqual(list(diff_histories(old_history, new_history)), + expected_result) + + def test_diff_histories_added_start(self): + """Testing diff_histories with a new history that adds commits at the + start of the history + """ + old_history = [ + DiffCommit(commit_id='r1'), + DiffCommit(commit_id='r2'), + ] + + new_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r1'), + DiffCommit(commit_id='r2'), + ] + + expected_result = [ + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_REMOVED, + old_commit=old_history[0]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_REMOVED, + old_commit=old_history[1]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_ADDED, + new_commit=new_history[0]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_ADDED, + new_commit=new_history[1]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_ADDED, + new_commit=new_history[2]), + ] + + self.assertEqual(list(diff_histories(old_history, new_history)), + expected_result) + + def test_diff_histories_removed_start(self): + """Testing diff_histories with a new history that removes commits at + the start of the history + """ + old_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r1'), + DiffCommit(commit_id='r2'), + ] + + new_history = [ + DiffCommit(commit_id='r1'), + DiffCommit(commit_id='r2'), + ] + + expected_result = [ + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_REMOVED, + old_commit=old_history[0]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_REMOVED, + old_commit=old_history[1]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_REMOVED, + old_commit=old_history[2]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_ADDED, + new_commit=new_history[0]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_ADDED, + new_commit=new_history[1]), + ] + + self.assertEqual(list(diff_histories(old_history, new_history)), + expected_result) + + def test_diff_histories_addedd_middle(self): + """Testing diff_histories with a new history that adds commits in the + middle of the history + """ + old_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r2'), + ] + + new_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r1'), + DiffCommit(commit_id='r2'), + ] + + expected_result = [ + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_UNMODIFIED, + old_commit=old_history[0], + new_commit=new_history[0]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_REMOVED, + old_commit=old_history[1]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_ADDED, + new_commit=new_history[1]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_ADDED, + new_commit=new_history[2]), + ] + + self.assertEqual(list(diff_histories(old_history, new_history)), + expected_result) + + def test_diff_histories_removed_middle(self): + """Testing diff_histories with a new history that removes commits in + the middle of the history + """ + old_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r1'), + DiffCommit(commit_id='r2'), + ] + + new_history = [ + DiffCommit(commit_id='r0'), + DiffCommit(commit_id='r2'), + ] + + expected_result = [ + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_UNMODIFIED, + old_commit=old_history[0], + new_commit=new_history[0]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_REMOVED, + old_commit=old_history[1]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_REMOVED, + old_commit=old_history[2]), + CommitHistoryDiffEntry( + entry_type=CommitHistoryDiffEntry.COMMIT_ADDED, + new_commit=new_history[1]), + ] + + self.assertEqual(list(diff_histories(old_history, new_history)), + expected_result) diff --git a/reviewboard/diffviewer/views.py b/reviewboard/diffviewer/views.py index 38a103ecef68fa873affda4c79b6e3d0f24ae80d..074429ed5ed01786662959cdf0fbf6d26ab8edb2 100644 --- a/reviewboard/diffviewer/views.py +++ b/reviewboard/diffviewer/views.py @@ -27,10 +27,11 @@ from pygments import highlight from pygments.formatters import HtmlFormatter from pygments.lexers import get_lexer_by_name +from reviewboard.diffviewer.commit_utils import diff_histories from reviewboard.diffviewer.diffutils import (get_diff_files, get_enable_highlighting) from reviewboard.diffviewer.errors import PatchError, UserVisibleError -from reviewboard.diffviewer.models import DiffSet, FileDiff +from reviewboard.diffviewer.models import DiffCommit, DiffSet, FileDiff from reviewboard.diffviewer.renderers import (get_diff_renderer, get_diff_renderer_class) from reviewboard.scmtools.errors import FileNotFoundError @@ -201,6 +202,7 @@ class DiffViewerView(TemplateView): diff_context = { 'commits': None, + 'commit_history_diff': None, 'filename_patterns': list(filename_patterns), 'revision': { 'revision': diffset.revision, @@ -226,10 +228,35 @@ class DiffViewerView(TemplateView): page.previous_page_number() if diffset.commit_count > 0: - diff_context['commits'] = list(diffset.commits.values( - 'author_name', - 'commit_id', - 'commit_message')) + diffset_pks = [diffset.pk] + + if interdiffset: + diffset_pks.append(interdiffset.pk) + + commits_by_diffset_id = {} + commits = DiffCommit.objects.filter(diffset_id__in=diffset_pks) + + for commit in commits: + commits_by_diffset_id.setdefault(commit.diffset_id, []).append( + commit) + + if interdiffset: + diff_context['commit_history_diff'] = [ + entry.serialize() + for entry in diff_histories( + commits_by_diffset_id[diffset.pk], + commits_by_diffset_id[interdiffset.pk]) + ] + + diff_context['commits'] = [ + { + 'author_name': commit.author_name, + 'commit_id': commit.commit_id, + 'commit_message': commit.commit_message, + } + for pk in commits_by_diffset_id + for commit in commits_by_diffset_id[pk] + ] context = dict({ 'diff_context': diff_context, diff --git a/reviewboard/reviews/tests/test_builtin_fields.py b/reviewboard/reviews/tests/test_builtin_fields.py index 289c6bda809d1838809470df947d2e2d394c0ddb..f33605ebf652da2da877f925073b217a939799e5 100644 --- a/reviewboard/reviews/tests/test_builtin_fields.py +++ b/reviewboard/reviews/tests/test_builtin_fields.py @@ -159,7 +159,9 @@ class CommitListFieldTests(TestCase): result = field.render_value(field.load_value(review_request)) self.assertInHTML('', result) - self.assertInHTML('SummaryAuthor', result) + self.assertInHTML( + 'SummaryAuthor', + result) self.assertInHTML( '' ' ' @@ -206,7 +208,7 @@ class CommitListFieldTests(TestCase): ' ' '', result) - self.assertInHTML('Summary', result) + self.assertInHTML('Summary', result) self.assertInHTML( '' ' ' @@ -260,8 +262,7 @@ class CommitListFieldTests(TestCase): result) self.assertInHTML( '' - ' ' - ' Summary' + ' Summary' ' Author' '', result) diff --git a/reviewboard/static/rb/css/pages/review-request.less b/reviewboard/static/rb/css/pages/review-request.less index b72c86f01d3812073ab86c98d77013baf3ae9b6d..69dd9618581774627c75eb7164f848de6f5bf5db 100644 --- a/reviewboard/static/rb/css/pages/review-request.less +++ b/reviewboard/static/rb/css/pages/review-request.less @@ -88,8 +88,22 @@ display: table !important; width: 100%; - &.changed .marker { - width: 0; + &.changed { + .marker { + font-family: @textarea-font-family; + font-size: @textarea-font-size; + font-weight: bold; + text-align: center; + width: 0; + } + + .new-value .marker { + background: @diff-insert-linenum-color; + } + + .old-value .marker { + background: @diff-delete-linenum-color; + } } a { diff --git a/reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js b/reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js new file mode 100644 index 0000000000000000000000000000000000000000..7d4077dccaf716055aaa02eb69b9d982cdc400c4 --- /dev/null +++ b/reviewboard/static/rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js @@ -0,0 +1,6 @@ +/** + * A collection of CommitsHistoryDiffEntries. + */ +RB.CommitHistoryDiffEntryCollection = Backbone.Collection.extend({ + model: RB.CommitHistoryDiffEntry, +}); diff --git a/reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js b/reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js new file mode 100644 index 0000000000000000000000000000000000000000..1bf22173abd9e27c36a03df6fdbce493f48853f9 --- /dev/null +++ b/reviewboard/static/rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js @@ -0,0 +1,72 @@ +(function() { + + +/** + * A history entry in a diff between two sets of commits. + * + * Attributes: + * entryType (string): + * The type of entry. + * + * newCommitID (string): + * The commit ID of the new commit in the entry (if any). + * + * oldCommitID (string): + * The commit ID of the old commit in the entry (if any). + */ +RB.CommitHistoryDiffEntry = Backbone.Model.extend({ + defaults: { + entryType: null, + newCommitID: null, + oldCommitID: null, + }, + + /** + * Parse the response. + * + * Args: + * rsp (object): + * The raw response from the server. + * + * Returns: + * object: + * The parsed attributes. + */ + parse(rsp) { + return { + entryType: rsp.entry_type, + newCommitID: rsp.new_commit_id, + oldCommitID: rsp.old_commit_id, + }; + }, + + /** + * Return the symbol representing this entry. + * + * Returns: + * string: + * The symbol representing this entry. + */ + getSymbol() { + const entryType = this.get('entryType'); + return RB.CommitHistoryDiffEntry.HISTORY_SYMBOL_MAP[entryType] || null; + }, +}, { + ADDED: 'added', + REMOVED: 'removed', + MODIFIED: 'modified', + UNMODIFIED: 'unmodified', +}); + + +const symbolMap = {}; + +symbolMap[RB.CommitHistoryDiffEntry.ADDED] = '+'; +symbolMap[RB.CommitHistoryDiffEntry.REMOVED] = '-'; +symbolMap[RB.CommitHistoryDiffEntry.MODIFIED] = '~'; +symbolMap[RB.CommitHistoryDiffEntry.UNMODIFIED] = ' '; + +RB.CommitHistoryDiffEntry.HISTORY_SYMBOL_MAP = symbolMap; + + +})(); diff --git a/reviewboard/static/rb/js/diffviewer/models/diffCommitListModel.es6.js b/reviewboard/static/rb/js/diffviewer/models/diffCommitListModel.es6.js index d8ff9388e2dcae3c03514486ca6833863246f8ef..2384ce80d834bc2994f6d9170874e566747cf468 100644 --- a/reviewboard/static/rb/js/diffviewer/models/diffCommitListModel.es6.js +++ b/reviewboard/static/rb/js/diffviewer/models/diffCommitListModel.es6.js @@ -5,14 +5,25 @@ * commits (RB.DiffCommitCollection): * The commits the view is rendering. * - * isInterdiff (boolean): - * Whether or not an interdiff is being rendered. + * historyDiff (boolean): + * The collection of history diff entries when displaying an interdiff. */ RB.DiffCommitList = Backbone.Model.extend({ defaults() { return { commits: new RB.DiffCommitCollection(), - isInterdiff: false, + historyDiff: new RB.CommitHistoryDiffEntryCollection(), }; }, + + /** + * Return whether or not an interdiff is being rendered. + * + * Returns: + * boolean: + * Whether or not an interdiff is being rendered. + */ + isInterdiff() { + return !this.get('historyDiff').isEmpty(); + }, }); diff --git a/reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js b/reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js index d114d9d22869d76bc5b0a0e7e5a29c145c7db93f..1030deac8691c88118b28ff1a98e13befb08f2df 100644 --- a/reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js +++ b/reviewboard/static/rb/js/diffviewer/views/diffCommitListView.es6.js @@ -9,7 +9,12 @@ const itemTemplate = _.template(dedent` - + + <% if (showHistorySymbol) { %> + + <%- historyDiffEntry.getSymbol() %> + + <% } %> <% if (showExpandCollapse) { %> <% if (commit.hasSummary()) { %> @@ -22,18 +27,24 @@ const itemTemplate = _.template(dedent` <% } %> <% } %> - 
<%- commit.get('summary') %>
- <%- commit.get('authorName') %> + class="value"<% } %>> + 
<%- commit.get('summary') %>
+ + class="value"<% } %>> + <%- commit.get('authorName') %> + `); const headerTemplate = _.template(dedent` - <% if (showExpandCollapse) { %> + <% if (showHistorySymbol) { %> <% } %> - <%- summaryText %> + colspan="2"<% } %>> + <%- summaryText %> + <%- authorText %> @@ -42,6 +53,9 @@ const headerTemplate = _.template(dedent` const tableTemplate = _.template(dedent` + <% if (showHistorySymbol) { %> + + <% } %> <% if (showExpandCollapse) { %> <% } %> @@ -76,41 +90,82 @@ RB.DiffCommitListView = Backbone.View.extend({ * This view, for chaining. */ render() { - let $content; - - if (this.model.get('isInterdiff')) { - /* - * TODO: We should display the difference between the two sets of - * commits. - */ - $content = $('

') - .text(gettext('Interdiff commit listings not supported.')); - } else { - const commits = this.model.get('commits'); - const showExpandCollapse = commits.some( - commit => commit.hasSummary()); - - $content = - $(tableTemplate({ - showExpandCollapse: showExpandCollapse, - })) - .append(headerTemplate({ + const commits = this.model.get('commits'); + const isInterdiff = this.model.isInterdiff(); + + const commonContext = { + showExpandCollapse: commits.some(commit => commit.hasSummary()), + showHistorySymbol: isInterdiff, + }; + + const $content = $(tableTemplate(commonContext)) + .toggleClass('changed', isInterdiff) + .append(headerTemplate(_.extend( + { authorText: gettext('Author'), - showExpandCollapse: showExpandCollapse, summaryText: gettext('Summary'), - })); - - const $tbody = $('

'); - - commits.each(commit => $tbody.append(itemTemplate({ - expandText: gettext('Expand commit message.'), - commit: commit, - showExpandCollapse: showExpandCollapse, - }))); - - $content.append($tbody); + }, + commonContext + ))); + + const $tbody = $(''); + + commonContext.expandText = gettext('Expand commit message.'); + + if (isInterdiff) { + this.model.get('historyDiff').each(historyDiffEntry => { + const entryType = historyDiffEntry.get('entryType'); + + let key; + let rowClass; + + switch (entryType) { + case RB.CommitHistoryDiffEntry.ADDED: + rowClass = 'new-value'; + key = 'newCommitID'; + break; + + case RB.CommitHistoryDiffEntry.REMOVED: + rowClass = 'old-value'; + key = 'oldCommitID'; + break; + + case RB.CommitHistoryDiffEntry.UNMODIFIED: + case RB.CommitHistoryDiffEntry.MODIFIED: + key = 'newCommitID'; + break; + + default: + console.error('Invalid history entry type: %s', + entryType); + break; + } + + const commit = commits.findWhere({ + commitID: historyDiffEntry.get(key), + }); + + $tbody.append(itemTemplate(_.extend( + { + commit: commit, + historyDiffEntry: historyDiffEntry, + rowClass: rowClass, + }, + commonContext + ))); + }); + } else { + commonContext.rowClass = ''; + commits.each(commit => $tbody.append(itemTemplate(_.extend( + { + commit: commit, + }, + commonContext + )))); } + $content.append($tbody); + this.$el .empty() .append($content); diff --git a/reviewboard/static/rb/js/diffviewer/views/tests/diffCommitListViewTests.es6.js b/reviewboard/static/rb/js/diffviewer/views/tests/diffCommitListViewTests.es6.js index c8b8e594a1c80ddc1c00a88a1706f0bdb3b62664..c2db0b99b1ed600d853f87da0af3c557e595c5f8 100644 --- a/reviewboard/static/rb/js/diffviewer/views/tests/diffCommitListViewTests.es6.js +++ b/reviewboard/static/rb/js/diffviewer/views/tests/diffCommitListViewTests.es6.js @@ -4,19 +4,28 @@ suite('rb/diffviewer/views/DiffCommitListView', function() { /* * If there is an expand/collapse column, row values start at the - * second column instead of the first. + * second column instead of the first and likewise for a history entry + * column. If both are present, values start at the third column. */ - const startIndex = options.haveExpandCollapse ? 1 : 0; + const startIndex = ((options.haveExpandCollapse ? 1 : 0) + + (options.haveHistory ? 1 : 0)); + + const linkIndex = options.haveHistory ? 1 : 0; for (let i = 0; i < $rows.length; i++) { const $row = $rows.eq(i).find('td'); - const $link = $row.eq(0).find('a'); const rowOptions = options.rowOptions[i]; const values = rowOptions.values; + expect($row.length).toEqual(values.length + startIndex); + + if (options.haveHistory) { + expect($row.eq(0).text().trim()).toEqual(rowOptions.historySymbol.trim()); + } + if (options.haveExpandCollapse) { - expect($row.length).toEqual(values.length + 1); + const $link = $row.eq(linkIndex).find('a'); if (rowOptions.haveExpandCollapse) { expect($link.length).toEqual(1); @@ -32,15 +41,11 @@ suite('rb/diffviewer/views/DiffCommitListView', function() { expect($span.attr('title')) .toEqual(gettext('Expand commit message.')); } - } else { - expect($link.find('a').length).toEqual(0); } - } else { - expect($row.length).toEqual(values.length); } for (let j = 0; j < values; j++) { - expect($row.eq(startIndex + j).text()).toEqual(values[j]); + expect($row.eq(startIndex + j).text().trim()).toEqual(values[j]); } } } @@ -78,6 +83,7 @@ suite('rb/diffviewer/views/DiffCommitListView', function() { model = new RB.DiffCommitList({ commits, + historyDiff: new RB.CommitHistoryDiffEntryCollection(), isInterdiff: false, }); }); @@ -92,8 +98,8 @@ suite('rb/diffviewer/views/DiffCommitListView', function() { const $table = $container.find('table'); const $cols = $table.find('thead th'); expect($cols.length).toEqual(2); - expect($cols.eq(0).text()).toEqual(gettext('Summary')); - expect($cols.eq(1).text()).toEqual(gettext('Author')); + expect($cols.eq(0).text().trim()).toEqual(gettext('Summary')); + expect($cols.eq(1).text().trim()).toEqual(gettext('Author')); testRows($table.find('tbody tr'), { haveExpandCollapse: false, @@ -119,11 +125,10 @@ suite('rb/diffviewer/views/DiffCommitListView', function() { const $table = $container.find('table'); const $cols = $table.find('thead th'); - expect($cols.length).toEqual(3); + expect($cols.length).toEqual(2); - expect($cols.eq(0).text()).toEqual(''); - expect($cols.eq(1).text()).toEqual(gettext('Summary')); - expect($cols.eq(2).text()).toEqual(gettext('Author')); + expect($cols.eq(0).text().trim()).toEqual(gettext('Summary')); + expect($cols.eq(1).text().trim()).toEqual(gettext('Author')); testRows($table.find('tbody tr'), { haveExpandCollapse: true, @@ -152,8 +157,8 @@ suite('rb/diffviewer/views/DiffCommitListView', function() { let $cols = $table.find('thead th'); expect($cols.length).toEqual(2); - expect($cols.eq(0).text()).toEqual(gettext('Summary')); - expect($cols.eq(1).text()).toEqual(gettext('Author')); + expect($cols.eq(0).text().trim()).toEqual(gettext('Summary')); + expect($cols.eq(1).text().trim()).toEqual(gettext('Author')); testRows($table.find('tbody tr'), { haveExpandCollapse: false, @@ -174,10 +179,9 @@ suite('rb/diffviewer/views/DiffCommitListView', function() { $table = $container.find('table'); $cols = $table.find('thead th'); - expect($cols.length).toEqual(3); - expect($cols.eq(0).text()).toEqual(''); - expect($cols.eq(1).text()).toEqual(gettext('Summary')); - expect($cols.eq(2).text()).toEqual(gettext('Author')); + expect($cols.length).toEqual(2); + expect($cols.eq(0).text().trim()).toEqual(gettext('Summary')); + expect($cols.eq(1).text().trim()).toEqual(gettext('Author')); testRows($table.find('tbody tr'), { haveExpandCollapse: true, @@ -192,26 +196,51 @@ suite('rb/diffviewer/views/DiffCommitListView', function() { }); it('Interdiff', function() { - model.set('isInterdiff', true); + model.get('historyDiff').reset([ + { + entry_type: RB.CommitHistoryDiffEntry.REMOVED, + old_commit_id: 'r0', + }, + { + entry_type: RB.CommitHistoryDiffEntry.ADDED, + new_commit_id: 'r1', + } + ], {parse: true}); + view = new RB.DiffCommitListView({ model: model, el: $container, }); view.render(); - expect($container.find('table').length).toEqual(0); - const $p = $container.find('p'); + const $table = $container.find('table'); + const $cols = $table.find('thead th'); + expect($cols.length).toEqual(3); + expect($cols.eq(0).text().trim()).toEqual(gettext('')); + expect($cols.eq(1).text().trim()).toEqual(gettext('Summary')); + expect($cols.eq(2).text().trim()).toEqual(gettext('Author')); - expect($p.length).toEqual(1); - expect($p.text()).toEqual( - 'Interdiff commit listings not supported.'); + testRows($table.find('tbody tr'), { + haveExpandCollapse: false, + haveHistory: true, + rowOptions: [ + { + historySymbol: '-', + values: ['Commit message 1', 'Example Author'], + }, + { + historySymbol: '+', + values: ['Commit message 2', 'Example Author'], + }, + ], + }); }); }); describe('Event handlers', function() { let model; - beforeEach(() => { + beforeEach(function() { let commits = new RB.DiffCommitCollection([ { commit_id: 'r0', @@ -232,7 +261,7 @@ suite('rb/diffviewer/views/DiffCommitListView', function() { }); }); - it('Expand/collapse', () => { + it('Expand/collapse', function() { view = new RB.DiffCommitListView({ model: model, el: $container, @@ -243,10 +272,9 @@ suite('rb/diffviewer/views/DiffCommitListView', function() { const $table = $container.find('table'); const $cols = $table.find('thead th'); - expect($cols.length).toEqual(3); - expect($cols.eq(0).text()).toEqual(''); - expect($cols.eq(1).text()).toEqual(gettext('Summary')); - expect($cols.eq(2).text()).toEqual(gettext('Author')); + expect($cols.length).toEqual(2); + expect($cols.eq(0).text().trim()).toEqual(gettext('Summary')); + expect($cols.eq(1).text().trim()).toEqual(gettext('Author')); testRows($table.find('tbody tr'), { haveExpandCollapse: true, diff --git a/reviewboard/static/rb/js/pages/models/diffViewerPageModel.es6.js b/reviewboard/static/rb/js/pages/models/diffViewerPageModel.es6.js index d4b12a04f2fdcdff7a7539f935456eaf38154775..582c0c200550e1b6b25cdfda793aac8ca5625011 100644 --- a/reviewboard/static/rb/js/pages/models/diffViewerPageModel.es6.js +++ b/reviewboard/static/rb/js/pages/models/diffViewerPageModel.es6.js @@ -33,6 +33,7 @@ RB.DiffViewerPage = RB.ReviewablePage.extend({ constructor() { this.commentsHint = new RB.DiffCommentsHint(); this.commits = new RB.DiffCommitCollection(); + this.commitHistoryDiff = new RB.CommitHistoryDiffEntryCollection(); this.files = new RB.DiffFileCollection(); this.pagination = new RB.Pagination(); this.revision = new RB.DiffRevision(); @@ -163,7 +164,17 @@ RB.DiffViewerPage = RB.ReviewablePage.extend({ this.revision.set(this.revision.parse(rsp.revision)); } + if (rsp.commit_history_diff) { + this.commitHistoryDiff.reset(rsp.commit_history_diff, + {parse: true}); + } + if (rsp.commits) { + /* + * The RB.DiffCommitListView listens for the reset event on the + * commits collection to trigger a render, so it must be updated + * **after** the commit history is updated. + */ this.commits.reset(rsp.commits, {parse: true}); } diff --git a/reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js b/reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js index 3b0d8b014c1692026702c3f6ac58923fc7d0dd26..5646efb31de5c7cb61091b340908f51a6a21e1bd 100644 --- a/reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js +++ b/reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js @@ -188,7 +188,10 @@ RB.DiffViewerPageView = RB.ReviewablePageView.extend({ this._commitListView = new RB.DiffCommitListView({ el: $('#diff_commit_list').find('.commit-list-container'), - model: diffCommitList, + model: new RB.DiffCommitList({ + commits: this.model.commits, + historyDiff: this.model.commitHistoryDiff, + }), }); this._commitListView.render(); } diff --git a/reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js b/reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js index 637b5119b90a3be78b10809093819bfa6d55ec46..acd7b5393c1064006337513a756e8778cb6e7440 100644 --- a/reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js +++ b/reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js @@ -834,7 +834,7 @@ suite('rb/pages/views/DiffViewerPageView', function() { }, { author_name: 'Author Name', - commit_id: 'r123', + commit_id: 'r125', commit_message: 'Commit message 3', }, ], @@ -865,11 +865,28 @@ suite('rb/pages/views/DiffViewerPageView', function() { it('Initial render (with interdiff)', function() { page.revision.set('interdiffRevision', 456); + page.commitHistoryDiff.reset([ + { + entry_type: RB.CommitHistoryDiffEntry.REMOVED, + old_commit_id: 'r123', + }, + { + entry_type: RB.CommitHistoryDiffEntry.ADDED, + new_commit_id: 'r124', + }, + { + entry_type: RB.CommitHistoryDiffEntry.ADDED, + new_commit_id: 'r125', + }, + ], { + parse: true, + }); pageView.render(); - expect($commitList.find('table').length).toBe(0); - expect($commitList.find('p').length).toBe(1); + const $table = $commitList.find('table'); + expect($table.length).toBe(1); + expect($table.find('tbody tr').length).toBe(3); }); it('Subsequent render (without interdiff)', function() { @@ -916,6 +933,39 @@ suite('rb/pages/views/DiffViewerPageView', function() { it('Subsequent render (with interdiff)', function() { pageView.render(); + const rspPayload = { + diff_context: { + revision: { + revision: 2, + interdiff_revision: 3, + is_interdiff: true, + }, + commits: [ + { + author_name: 'Author Name', + commit_id: 'r124', + commit_message: 'Commit message', + }, + { + author_name: 'Author Name', + commit_id: 'r125', + commit_message: 'Commit message', + }, + ], + commit_history_diff: [ + { + entry_type: RB.CommitHistoryDiffEntry.REMOVED, + old_commit_id: 'r124', + }, + { + entry_type: RB.CommitHistoryDiffEntry.ADDED, + new_commit_id: 'r125', + }, + ], + }, + }; + + spyOn($, 'ajax').and.callFake(function(url) { expect(url) .toBe('/api/review-requests/123/diff-context/' + @@ -923,16 +973,7 @@ suite('rb/pages/views/DiffViewerPageView', function() { return { done(cb) { - cb({ - diff_context: { - revision: { - revision: 2, - interdiff_revision: 3, - is_interdiff: true, - }, - commits: [], - }, - }); + cb(rspPayload); }, }; }); @@ -944,8 +985,9 @@ suite('rb/pages/views/DiffViewerPageView', function() { expect($.ajax).toHaveBeenCalled(); - expect($commitList.find('table').length).toBe(0); - expect($commitList.find('p').length).toBe(1); + const $table = $commitList.find('table'); + expect($table.length).toBe(1); + expect($table.find('tbody tr').length).toBe(2); }); }); }); diff --git a/reviewboard/staticbundles.py b/reviewboard/staticbundles.py index db7efc9c79d5f8dba6aaf568f0b36b3dfcd2f6b3..89ab8d31d54e5d40ef64542efd8afa56911abc01 100644 --- a/reviewboard/staticbundles.py +++ b/reviewboard/staticbundles.py @@ -292,6 +292,7 @@ PIPELINE_JAVASCRIPT = dict({ 'rb/js/views/markdownReviewableView.es6.js', 'rb/js/views/uploadDiffView.js', 'rb/js/views/updateDiffView.js', + 'rb/js/diffviewer/models/commitHistoryDiffEntry.es6.js', 'rb/js/diffviewer/models/diffCommentBlockModel.es6.js', 'rb/js/diffviewer/models/diffCommentsHintModel.js', 'rb/js/diffviewer/models/diffCommitListModel.es6.js', @@ -300,6 +301,7 @@ PIPELINE_JAVASCRIPT = dict({ 'rb/js/diffviewer/models/diffReviewableModel.es6.js', 'rb/js/diffviewer/models/diffRevisionModel.es6.js', 'rb/js/diffviewer/models/paginationModel.js', + 'rb/js/diffviewer/collections/commitHistoryDiffEntryCollection.es6.js', 'rb/js/diffviewer/collections/diffCommitCollection.es6.js', 'rb/js/diffviewer/collections/diffFileCollection.js', 'rb/js/diffviewer/collections/diffReviewableCollection.es6.js', diff --git a/reviewboard/templates/reviews/commit_list_field.html b/reviewboard/templates/reviews/commit_list_field.html index ef9c55cf09ebcd024f683981f3abeb1cf22ff8a0..9ee3badb5e8f2136a173ff47290e30aebaa92f09 100644 --- a/reviewboard/templates/reviews/commit_list_field.html +++ b/reviewboard/templates/reviews/commit_list_field.html @@ -16,10 +16,7 @@ identically. -{% if to_expand %} - -{% endif %} - + {% trans "Summary" %} {% if include_author_name %} {% endif %}
{% trans "Summary" %}{% trans "Author" %}