Add initial review request UI for commit histories.

Review Request #6931 — Created Feb. 10, 2015 and submitted

Information

Review Board
dvcs
39bc55a...

Reviewers

Review requests that contain commit history will now show the list of
commits on the current diff revision. The changes to the commits in
diff revisions are now reflected in change descriptions.

The BuiltinLocalFieldMixin has been modified to allow subclasses to
override its default behaviour (where the review_request_details are
replaced if it is a draft and the field does not exist on the draft).
This was changed because the new DiffCommitListField needs to display
the draft commits and the default behaviour would prevent that.

Tested the following manually:

  • Created a review request with commit history. The commit list field
    was displayed in the draft view.
  • Published the review request. The commit list field was displayed in
    the public review request.
  • Uploaded a new diff with history. The draft view reflected the
    changes in the commit list field.
  • Published the review request. The change description reflected the
    changes to the commits on the review request.
  • Published the review request without uploading a new diff. The commit
    list field did not change and it did not appear in the change
    description.
  • Updated the review request using a new diff with history. The change
    description was rendered properly.
  • Updated the review request using a squashed diff. The commit list
    field disappeared from the review request and the change description
    showed the removed commits.
  • Updated the review request using a a diff with history. THe commit
    list field re-appeared in the review request and the chagne
    description showed the added commits.

Description From Last Updated

Hmm. It would be really nice to have a single list of commits: Commit Change Author Add foo Updated ... …

daviddavid

<p>That is pretty great. I love it.</p>

chipx86chipx86

This would be good for the docstring.

chipx86chipx86

This would be good info in the docstring.

chipx86chipx86

We need to return something else if this doesn't match. (Okay, technically, in Python, we don't, but it'd best to …

chipx86chipx86

You can reduce the verbosity of the code (and reduce lookups) by starting off with a local variable for the …

chipx86chipx86

To avoid an extra query, can we fetch both up-front into a list, and then fetch out of that list? …

chipx86chipx86

Ideally, we'd pull out of the locals, but I know we don't have that. Maybe add a # TODO for …

chipx86chipx86

Since the indentation is weird, how about pulling out the list into a variable first, then iterating that?

chipx86chipx86

Selectors should be in alphabetical order.

chipx86chipx86

Here too.

chipx86chipx86

One space indentation.

chipx86chipx86

Template tags and HTML tags should be indented independently of each other, with indentation for template tags happening inside the …

chipx86chipx86

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

Col: 9 E303 too many blank lines (2)

reviewbotreviewbot

This will evaluate by pulling in all matches, deserializing them, and then checking the resulting list for truthiness. Instead, add …

chipx86chipx86

"ID"

chipx86chipx86

You can use .exists() here as well. Basically, it will do a SELECT 1 FROM ... LIMIT 1, which is …

chipx86chipx86

This seems a bit oddly formatted. I'd have expected the pep8 checker to yell about it. How about: commit_list = …

chipx86chipx86

Small thing, but just for readability, can you move the j < len(...) onto the next line so there's only …

chipx86chipx86

You can simplify this just a bit with a generator expression to a dictionary.

chipx86chipx86

If there's 20 diffsets, we'll make 20 queries here, each with a join. How about doing: diff_commits = DiffCommit.objects.filter(diffset__in=diffsets) for …

chipx86chipx86

Ideally, we'd reference some consatnts for any colors. Here and below.

chipx86chipx86

Needs localization.

chipx86chipx86

No spaces inside the {{}}

chipx86chipx86

Needs localization.

chipx86chipx86

No spaces inside the {{}}. Here and below.

chipx86chipx86

Add a blank line after this.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/diffviewer/view_diff.html
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
brennie
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/builtin_fields.py (Diff revision 2)
     
     
     
    Show all issues

    This would be good for the docstring.

  3. reviewboard/reviews/builtin_fields.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    This would be good info in the docstring.

  4. reviewboard/reviews/builtin_fields.py (Diff revision 2)
     
     
    Show all issues

    We need to return something else if this doesn't match.

    (Okay, technically, in Python, we don't, but it'd best to be explicit.)

  5. reviewboard/reviews/builtin_fields.py (Diff revision 2)
     
     
    Show all issues

    You can reduce the verbosity of the code (and reduce lookups) by starting off with a local variable for the dictionary, populating it, and then setting that into changedesc after.

  6. reviewboard/reviews/builtin_fields.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues

    To avoid an extra query, can we fetch both up-front into a list, and then fetch out of that list? (Database queries are expensive and, historically, the cause of most of our performance issues on large servers, so I try to point this out whenever possible :)

  7. reviewboard/reviews/builtin_fields.py (Diff revision 2)
     
     
    Show all issues

    Ideally, we'd pull out of the locals, but I know we don't have that. Maybe add a # TODO for that, and an entry in Asana?

  8. reviewboard/reviews/builtin_fields.py (Diff revision 2)
     
     
     
    Show all issues

    Since the indentation is weird, how about pulling out the list into a variable first, then iterating that?

  9. reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
     
     
     
     
     
     
     
     
    Show all issues

    Selectors should be in alphabetical order.

  10. reviewboard/static/rb/css/pages/reviews.less (Diff revision 2)
     
     
     
     
     
     
     
     
    Show all issues

    Here too.

  11. Show all issues

    One space indentation.

  12. reviewboard/templates/reviews/boxes/commit_list.html (Diff revision 2)
     
     
     
     
     
     
     
     
     
    Show all issues

    Template tags and HTML tags should be indented independently of each other, with indentation for template tags happening inside the {% ... %}, like:

    {% for ... %}
    {%  if ... %}
    

    The template language has its own set of indentation, and the formatted HTML is its own set of indentation.

  13. 
      
brennie
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/resources/draft_diff_commit.py
        reviewboard/webapi/server_info.py
        reviewboard/webapi/resources/filediff.py
        reviewboard/webapi/tests/test_draft_diff_commit.py
        reviewboard/webapi/resources/draft_diff.py
        reviewboard/webapi/tests/test_diff_commit.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/forms.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_filediff.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/diff.py
        reviewboard/webapi/resources/diff_commit.py
        reviewboard/diffviewer/forms.py
        reviewboard/webapi/resources/draft_filediff.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/testing/testcase.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/resources/draft_diff_commit.py
        reviewboard/webapi/server_info.py
        reviewboard/webapi/resources/filediff.py
        reviewboard/webapi/tests/test_draft_diff_commit.py
        reviewboard/webapi/resources/draft_diff.py
        reviewboard/webapi/tests/test_diff_commit.py
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/forms.py
        reviewboard/webapi/tests/urls.py
        reviewboard/webapi/tests/test_filediff.py
        reviewboard/webapi/tests/mimetypes.py
        reviewboard/webapi/resources/diff.py
        reviewboard/webapi/resources/diff_commit.py
        reviewboard/diffviewer/forms.py
        reviewboard/webapi/resources/draft_filediff.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
david
  1. 
      
  2. Show all issues

    Hmm. It would be really nice to have a single list of commits:

    Commit Change Author
    Add foo Updated ...
    Add bar Updated ...
    Call foo and bar in main Updated ...
    Add shebang to run.py Updated ...
    Add gitignore for pyc files Updated ...
    Add baz module Added ...
    1. The current revision has support for unmodified, added, and removed commits. It does not currently support detecting modified commits.

  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. reviewboard/reviews/builtin_fields.py (Diff revision 5)
     
     
    Show all issues
    Col: 9
     E303 too many blank lines (2)
    
  3. reviewboard/reviews/builtin_fields.py (Diff revision 5)
     
     
    Show all issues
    Col: 9
     E303 too many blank lines (2)
    
  4. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
brennie
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/templates/reviews/boxes/commit_list_change.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/templates/reviews/boxes/commit_list_change.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
chipx86
  1. 
      
  2. Show all issues

    <p>That is pretty great. I love it.</p>

  3. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     
    Show all issues

    This will evaluate by pulling in all matches, deserializing them, and then checking the resulting list for truthiness.

    Instead, add .exists() to the end of that query.

  4. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     
    Show all issues

    "ID"

  5. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     
    Show all issues

    You can use .exists() here as well. Basically, it will do a SELECT 1 FROM ... LIMIT 1, which is faster even than counting.

  6. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     
     
     
    Show all issues

    This seems a bit oddly formatted. I'd have expected the pep8 checker to yell about it. How about:

    commit_list = list(
        DiffCommit.objects
            .select_related('diffset')
            .filter(...))
    
    1. As it turns out, this code you suggested is a PEP8 violation on the .selected_realted('diffset') line:

      E131 continutation line unaligned for hanging indent.

  7. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     
     
    Show all issues

    Small thing, but just for readability, can you move the j < len(...) onto the next line so there's only one expression per line?

  8. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    You can simplify this just a bit with a generator expression to a dictionary.

  9. reviewboard/reviews/views.py (Diff revision 7)
     
     
    Show all issues

    If there's 20 diffsets, we'll make 20 queries here, each with a join.

    How about doing:

    diff_commits = DiffCommit.objects.filter(diffset__in=diffsets)
    
    for diff_commit in diff_commits:
        diff_commits_by_id.setdefault(
            diff_commit.diffset_id, []).append(diff_commit)
    
    1. Is there any benefit in using setdefault over collections.defaultdict, i.e.:

      from collections import defaultdict
      
      # ...
      
      diff_commits = DiffCommit.objects.filter(diffset__in=diffsets)
      diff_commits_by_id = defaultdict(list)
      
      for diff_commit in diff_commits:
          diff_commits_by_id[diff_commit.id].append(diff_commit)
      
  10. Show all issues

    Ideally, we'd reference some consatnts for any colors. Here and below.

  11. Show all issues

    Needs localization.

  12. Show all issues

    No spaces inside the {{}}

  13. Show all issues

    Needs localization.

  14. Show all issues

    No spaces inside the {{}}. Here and below.

  15. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/templates/reviews/boxes/commit_list_change.html
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/templates/reviews/boxes/commit_list_change.html
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/builtin_fields.py (Diff revision 8)
     
     
    Show all issues

    Add a blank line after this.

  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/templates/reviews/boxes/commit_list_change.html
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/builtin_fields.py
        reviewboard/reviews/views.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/commit_list.html
        reviewboard/templates/reviews/boxes/commit_list_change.html
        reviewboard/static/rb/css/defs.less
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to dvcs (241be47)