Display commit lists for review requests created with history

Review Request #10094 — Created July 18, 2018 and submitted

brennie
Review Board
release-4.0.x
10099, 10082
10125
9d9e132...
reviewboard

The CommitListField renders the summaries (and authors if they differ
from the review request submitter) of the commits that make up a
multi-commit review request. This field has been added to a new
ExtraFieldSet, which exists below the MainFieldSet. This is due to
the RB.ReviewRequestEditorView expecting the last field in the main
field set to be an editable text field and working around this
limitation was more complex and convoluted than adding a new fieldset.

The JS RB.ReviewRequestFields.CommitListFieldView is responsible for
toggling between the full commit message (if it is longer than a single
line) and the summary.

  • Ran unit tests.
  • Ran JS tests.
  • Uploaded review requests with multiple commits and saw the correct
    commits displayed in the field and in change descriptions.
  • Uploaded a plain review request and did not see the new field on
    the review request page or change description box.
Loading file attachments...

  • 0
  • 0
  • 48
  • 3
  • 51
Description From Last Updated
Checks run (1 failed, 1 failed with error)
flake8 failed.
JSHint internal error.

flake8

brennie
Review request changed

Summary:

-WIP
+Display commit lists for review requests created with history

Description:

~  

WIP

  ~

The CommitListField renders the summaries (and authors if they differ

  + from the review request submitter) of the commits that make up a
  + multi-commit review request. This field has been added to a new
  + ExtraFieldSet, which exists below the MainFieldSet. This is due to
  + the RB.ReviewRequestEditorView expecting the last field in the main
  + field set to be an editable text field and working around this
  + limitation was more complex and convoluted than adding a new fieldset.

  +
  +

The JS RB.ReviewRequestFields.CommitListFieldView is responsible for

  + toggling between the full commit message (if it is longer than a single
  + line) and the summary.

Testing Done:

  +
  • Ran unit tests.
  +
  • Uploaded review requests with multiple commits and saw the correct
    commits displayed in the field and in change descriptions.
  +
  • TODO: Add unit tests for the field.

Depends On:

+10099 - Correctly return line counts in the FileDiffCollectionMixin

Commit:

-af8a4800e4076aa05c36f4b489023be474aab363
+bec2c2299fc198e5674b709d1a041b3e0f1064dc

Diff:

Revision 2 (+561 -20)

Show changes

Added Files:

Checks run (1 failed, 1 failed with error)

flake8 failed.
JSHint internal error.

flake8

brennie
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
brennie
chipx86
  1. 
      
  2. Unit tests for the field should probably be added (mentioned as a "TODO" in your Testing Done).

    Did your test run include JavaScript unit tests, and review requests without history support?

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

    I'm switching more code over to this (I have a pending change I need to split up and put up for review), but for render_to_string() you'll want to use the new compat function we have, which will help us migrate to newer versions of Django soon.

    from djblets.util.compat.django.template.loader import render_to_string
    
    ...
    
    render_to_string(template_name=...,
                     context={...},
                     request=request)
    

    Passing in request gives you a RequestContext. If not passed in, you get a Context.

    (It does support passing in those objects themselves, but a dictionary is more future-proof.)

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

    We do this both here and in should_render(), but we do it differently in each. We should be consistent, or we should have a private helper that wraps it.

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

    Typo: "rquest" -> "request"

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

    Rather than loop through the commits multiple times, how about:

    include_author_name = not submitter_name
    should_expand = {}
    show_expand_collapse = False
    
    for commit in commits:
        commit_should_expand = (commit.summary.strip() != commit.commit_message.strip())
        should_expand[commit.pk] = commit_should_expand
    
        if commit_should_expand:
            show_expand_collapse = True
    
        if not include_author_name and commit.author_name != submitter_name:
            include_author_name = True
    

    Potentially, you could also get rid of show_expand_collapse by turning should_expand into a set of ones to expand. It's then easy to check against that for both individual commits and as a larger "are there any commits that should expand". Then we're simply looking at:

    include_author_name = not submitter_name
    should_expand = set()
    
    for commit in commits:
        if (commit.summary.strip() != commit.commit_message.strip()):
            should_expand.add(commit.pk)
    
        if not include_author_name and commit.author_name != submitter_name:
            include_author_name = True
    
  7. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     

    This sounds like a boolean, but it's a dictionary. Maybe call it commits_to_expand?

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

    Let's make this a private method on the class, so it's not redefined every time this method is called.

    Alternatively, do this instead:

    return {
        key: set(
            {
                'author': commit.author_name,
                'summary': commit.summary,
            }
            for commit in commits_by_diffset_id[info[key]]
        )
        for key in ('old', 'new')
    }
    
  9. reviewboard/reviews/builtin_fields.py (Diff revision 7)
     
     

    The annoyance with {} for both sets and dicts is that this looks pretty weird at first glance. Thinking we should use set() explicitly here to differentiate from the dict.

    1. It was supposed to be a list 😄

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

    Missing trailing comma.

  11. reviewboard/reviews/detail.py (Diff revision 7)
     
     
     

    We should probably explicitly check that diffset_id is not None here.

  12. Missing a docstring.

  13. Typo: "creatd" -> "created"

  14. reviewboard/reviews/models/review_request_draft.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    We can define this once in the base class instead of in each subclass by doing:

    if not self.get_review_request().created_with_history:
        return None
    
    if not hasattr(self, '_commit_messages'):
        self._commit_messages = list(
            self.get_latest_diffset().commits
            .values_list('commit_message', flat=True)
    
    return self._commit_messages
    
    1. This gets removed in a subsequent patch so I'm not super worried about it.

  15. Typo: "creatd" -> "created"

  16. Unwanted blank line.

  17. Alphabetical order.

  18. Can you define a constant for this?

  19. We should either have a constant already for the font size, or you should be able to inherit.

  20. Can you define a constant for this?

  21. We should have constants for this already, since we have the same styling elsewhere. If not, it should be added and used both here and in the file list.

  22. Blank line here.

  23. This is missing the mobile rule in #review-request-main. Really, we should add a mixin that's used both here and there that sets up this styling and the mobile styling. Probably want to sneak in the position: relative and display: block as well, I'd think.

  24. Looks like you got some whiteout on your '.

  25. Typo: "statis" -> "status"

  26. Typo: "Collpase" -> "Collapse"

  27. You can do this.$('.commit-message') instead.

  28. Can you start the first parameter on the function call line? That or turn this into a dictionary. Looks a bit out of place the way it is.

  29. Typo: "colum" -> "column"

  30. Instead of - and +, can you use fa fa-plus and fa fa-minus? That's what we use for other collapse/expand controls.

    1. Indeed, which this is not. This is the add/remove line marker. This currently does not have expand controls.

    2. Ahh, right. Ignore me!

  31. 
      
brennie
brennie
Review request changed

Change Summary:

Addressed issues.

Testing Done:

   
  • Ran unit tests.
~  
  • Uploaded review requests with multiple commits and saw the correct
    commits displayed in the field and in change descriptions.
~  
  • TODO: Add unit tests for the field.
  ~
  • Ran JS tests.
  ~
  • Uploaded review requests with multiple commits and saw the correct
    commits displayed in the field and in change descriptions.
  +
  • Uploaded a plain review request and did not see the new field on
    the review request page or change description box.

Commit:

-3c6cb257bbd6f60e47975c79e703aab80b50f345
+78a47edf09cf309867f61e51d8b8b24e1ccc1de9

Diff:

Revision 9 (+1208 -22)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/reviews/builtin_fields.py (Diff revision 10)
     
     

    This should be the new full type path.

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

    You should use keyword arguments for these calls to render_to_string(), to ensure compatibility going forward.

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

    Can you put the start of the tuple on the second line?

  5. "Backbone", not "Bckbone".

  6. Alphabetical order.

  7. 
      
brennie
chipx86
  1. 
      
  2. reviewboard/reviews/tests/test_builtin_fields.py (Diff revision 11)
     
     
     
     
     
     

    RequestFactory() should be defined in setUp and not setUpClass, since it keeps state between requests and can introduce subtle failures.

  3. Should be Model Attributes.

  4. Should be jQuery.Event.

  5. Should be jQuery.Event.

  6. Should be jQuery.Event.

    1. No it shouldn't :).

  7. 
      
brennie
chipx86
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (6b29797)
Loading...