Add initial review request UI for commit histories.

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

brennie
Review Board
dvcs
6816, 6815
39bc55a...
reviewboard
chipx86, mike_conley, smacleod

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.
Loading file attachments...

  • 0
  • 0
  • 28
  • 0
  • 28
Description From Last Updated
  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
  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)
     
     
     

    This would be good for the docstring.

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

    This would be good info in the docstring.

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

    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)
     
     

    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)
     
     
     
     
     
     

    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)
     
     

    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)
     
     
     

    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)
     
     
     
     
     
     
     
     

    Selectors should be in alphabetical order.

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

    Here too.

  11. One space indentation.

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

    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
  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
  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. 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
  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)
     
     
    Col: 9
     E303 too many blank lines (2)
    
  3. reviewboard/reviews/builtin_fields.py (Diff revision 5)
     
     
    Col: 9
     E303 too many blank lines (2)
    
  4. 
      
brennie
  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
  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. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     

    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.

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

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

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

    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.

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

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

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

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

  7. reviewboard/reviews/views.py (Diff revision 7)
     
     

    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)
      
  8. Ideally, we'd reference some consatnts for any colors. Here and below.

  9. Needs localization.

  10. No spaces inside the {{}}

  11. Needs localization.

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

  13. 
      
brennie
  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)
     
     

    Add a blank line after this.

  3. 
      
brennie
  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: Closed (submitted)

Change Summary:

Pushed to dvcs (241be47)
Loading...