Add a feature to automatically suggest reviewers for a given review request

Review Request #5029 — Created Nov. 26, 2013 and submitted

Information

Review Board
master

Reviewers

Using past review activity, suggest reviewers to a user posting a review request. This will look at:
- the frequency with which users have commented on / reviewed a file in the current review request's diffset.
- The recency of their reviews
- Whether or not they were targetted for review in the reviews they gave.

On the front end, clicking on a name in the suggestions adds to the target people list and pulls them off the suggestions, and hitting cancel puts them back into the suggestions.

Front end:
experimented with the front-end and made sure that everything behaves as I expected:
- clicking on a suggestion when not in inline-edit mode opens the inline editor and adds the suggestion to the list, removing from suggestions list
- clicking on a suggestion when in inline-editor keeps the inline editor open and appends the suggestion, removing it from the suggestions list
- hitting cancel on the people's field throws out the added suggestions from the inline editor and adds them back to the suggestions list.
- opening another field's inline editor while the people's editor is open and has unsubmitted changes from suggestions, then hitting cancel on the other inline editor. This doesn't affect the suggestions list at all, nor the people field's current inline Edit contents. Hitting cancel on the People's inline edit field then behaves as it should (described above)
- Hitting ok on the inline editor after it has been modified by suggestions turns off inline edit mode while reflecting the changes, and keeps the suggestions list unaltered, then turning it on and hitting cancel doesn't add back to the suggestions list the suggestions that were pushed to the editor before the save.

Back-end:
- manually queried and checked expected results (based on frequency, recency and targettedness of comments) for a few individual review requests and made sure that the reported results aligned.

Unit tests pass.


Description From Last Updated

This should be grouped with the standard lib imports (just after 'import re')

daviddavid

It's probably worth bailing out early in the alternative case, to avoid doing a bunch of unnecessary work: if self.get_latest_diffset() …

daviddavid

It might be worth using collections.defaultdict here, so you don't have to do the if rr_id in targets ... else …

daviddavid

You could probably get rid of the blank lines in this block and have it be more compact.

daviddavid

Can you wrap this conditional in () and get rid of the characters? I think it's probably also worth swapping …

daviddavid

Because you've got parentheses here, you don't need the \s

daviddavid

Can you use parens instead of \? Here and below.

daviddavid

Because this is inside [], you don't need the . Alignment is also a little bit weird here.

daviddavid

Remove this blank line.

daviddavid

No space between the key and :

daviddavid

Alignment is off here (indent the second line 1 more space).

daviddavid

Blank line between these.

chipx86chipx86

Missing trailing period.

chipx86chipx86

"most likely" "knowledgeable"

chipx86chipx86

"review request's" We try to be explicit with that, since a "request" is often an HTTPRequest.

chipx86chipx86

No blank line here.

chipx86chipx86

You should call this only once and store it, since it's an otherwise expensive call.

chipx86chipx86

You don't need a \\ when inside parens. I'd suggest rewriting this as: q = ReviewRequest.target_people.field.rel.through.objects q = q.select_related('reviews') q …

chipx86chipx86

The second row of parens should line up with the first.

chipx86chipx86

Blank line between these.

chipx86chipx86

Should be: if (entry[...] not in all_reviews Since we use entry['reviewrequest_reviews'] more than once, it'd be best to pull it …

chipx86chipx86

Should use != instead of not ==.

chipx86chipx86

What's the contents of entry['reviewrequest_reviews']?

chipx86chipx86

You already have this pulled out into a variable.

chipx86chipx86

Can you call this review_id, so it doesn't sound like a full-blown Review object?

chipx86chipx86

No need for these outer parens now.

chipx86chipx86

Comments should have sentence capitalization. Same with the other comments.

chipx86chipx86

Can you add a comment block describing in detail the algorithm you're using?

chipx86chipx86

Kind of a big wall of text. Are there some good parts where you can break this up into separate …

chipx86chipx86

Spacing is a tiny bit off here--one more space before -, and one fewer after.

daviddavid

Alignment problem with now and -. Also, too many spaces after -.

chipx86chipx86

Blank line before this.

chipx86chipx86

Wrap these in (), get rid of \, and fix alignment of the second line.

daviddavid

Instead of a \\, you should use parens.

chipx86chipx86

Instead of hard-coding 5 here, let's have a constant in the class definition. Something like MAX_GUESSED_REVIEWERS.

chipx86chipx86

Alignment issue. The (weight / should be indented one more space.

chipx86chipx86

This might be a bit nicer, and a little faster, by doing: sorted_entries = sorted( weighted.items(), key=itemgetter(1), reverse=True) sorted_users =[ …

chipx86chipx86

Since both of these are unique IDs, it's actually faster to not do this.$(..), and instead do $(..).

chipx86chipx86

Blank line on either side of this.

chipx86chipx86

No leading or trailing blank lines in the function. Do you need to redo the .suggest-user query, or can you …

chipx86chipx86

Get rid of this blank line.

daviddavid

Not sure "Person" needs to be capitalized here. Maybe "the person"

chipx86chipx86

You can use chaining to simplify this: this._$targetPeople .inlineEditor('startEdit') .inlineEditor('appendToFieldValue...')

chipx86chipx86

I saw the review request that adds this function. Is that really needed, or can you just implement that logic …

chipx86chipx86

Should be consistent with the quotes and use single quotes for the delimiter.

chipx86chipx86

Instead of using view in these callbacks, it'd be better to do: cancel: _.bind(function() { // You can make use …

chipx86chipx86

No trailing blank line in the function.

chipx86chipx86

Space between if and (.

chipx86chipx86

No trailing blank line.

chipx86chipx86

Constants should be all capital letters, so something like RECENCY_WEIGHT and TARGET_WEIGHT.

ED edwlee

This can be simply "if entry['user']:"

ED edwlee
EN
EN
david
  1. 
      
  2. reviewboard/reviews/models.py (Diff revision 1)
     
     

    This should be grouped with the standard lib imports (just after 'import re')

  3. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     

    It's probably worth bailing out early in the alternative case, to avoid doing a bunch of unnecessary work:

    if self.get_latest_diffset() is None:
        return []
    
    filediffs = ...
    
  4. reviewboard/reviews/models.py (Diff revision 1)
     
     

    It might be worth using collections.defaultdict here, so you don't have to do the if rr_id in targets ... else ... thing.

  5. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
     
     

    You could probably get rid of the blank lines in this block and have it be more compact.

  6. reviewboard/reviews/models.py (Diff revision 1)
     
     
     

    Can you wrap this conditional in () and get rid of the characters?

    I think it's probably also worth swapping around the 'not's:

    if (rr_id != self.pk and
        entry['reviewrequest__reviews'] not in all_reviews):
    
  7. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     

    Because you've got parentheses here, you don't need the \s

  8. reviewboard/reviews/models.py (Diff revision 1)
     
     
     

    Can you use parens instead of \? Here and below.

  9. reviewboard/reviews/models.py (Diff revision 1)
     
     

    Because this is inside [], you don't need the . Alignment is also a little bit weird here.

  10. Remove this blank line.

  11. No space between the key and :

  12. Alignment is off here (indent the second line 1 more space).

  13. 
      
EN
david
  1. Looking pretty good.

    Did you install django-debug-toolbar? What database queries does this end up doing?

  2. reviewboard/reviews/models.py (Diff revision 2)
     
     
     

    Spacing is a tiny bit off here--one more space before -, and one fewer after.

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

    Wrap these in (), get rid of \, and fix alignment of the second line.

  4. Get rid of this blank line.

  5. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/context.py (Diff revision 2)
     
     
     

    Blank line between these.

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

    Missing trailing period.

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

    "most likely"

    "knowledgeable"

  5. reviewboard/reviews/models.py (Diff revision 2)
     
     

    "review request's"

    We try to be explicit with that, since a "request" is often an HTTPRequest.

  6. reviewboard/reviews/models.py (Diff revision 2)
     
     
     
     

    No blank line here.

  7. reviewboard/reviews/models.py (Diff revision 2)
     
     

    You should call this only once and store it, since it's an otherwise expensive call.

  8. reviewboard/reviews/models.py (Diff revision 2)
     
     
     
     
     

    You don't need a \\ when inside parens.

    I'd suggest rewriting this as:

    q = ReviewRequest.target_people.field.rel.through.objects
    q = q.select_related('reviews')
    q = q.filter(...)
    
  9. reviewboard/reviews/models.py (Diff revision 2)
     
     
     

    The second row of parens should line up with the first.

  10. reviewboard/reviews/models.py (Diff revision 2)
     
     
     

    Blank line between these.

  11. reviewboard/reviews/models.py (Diff revision 2)
     
     

    Should be:

    if (entry[...] not in all_reviews
    

    Since we use entry['reviewrequest_reviews'] more than once, it'd be best to pull it out into a variable.

  12. reviewboard/reviews/models.py (Diff revision 2)
     
     

    Should use != instead of not ==.

  13. reviewboard/reviews/models.py (Diff revision 2)
     
     

    What's the contents of entry['reviewrequest_reviews']?

    1. this is the set of all reviews written on files which are in the diffset of the current review request.

    2. the content of entry['reviewrequest_reviews'] is the pk of a single review.

  14. reviewboard/reviews/models.py (Diff revision 2)
     
     

    You already have this pulled out into a variable.

  15. reviewboard/reviews/models.py (Diff revision 2)
     
     

    Comments should have sentence capitalization. Same with the other comments.

  16. reviewboard/reviews/models.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Can you add a comment block describing in detail the algorithm you're using?

  17. reviewboard/reviews/models.py (Diff revision 2)
     
     
     

    Alignment problem with now and -.

    Also, too many spaces after -.

  18. reviewboard/reviews/models.py (Diff revision 2)
     
     

    Blank line before this.

  19. reviewboard/reviews/models.py (Diff revision 2)
     
     
     

    Instead of a \\, you should use parens.

  20. reviewboard/reviews/models.py (Diff revision 2)
     
     

    Instead of hard-coding 5 here, let's have a constant in the class definition. Something like MAX_GUESSED_REVIEWERS.

  21. Since both of these are unique IDs, it's actually faster to not do this.$(..), and instead do $(..).

  22. Blank line on either side of this.

  23. reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)
     
     
     
     
     
     
     
     

    No leading or trailing blank lines in the function.

    Do you need to redo the .suggest-user query, or can you use allPeople above?

  24. Not sure "Person" needs to be capitalized here. Maybe "the person"

  25. You can use chaining to simplify this:

    this._$targetPeople
        .inlineEditor('startEdit')
        .inlineEditor('appendToFieldValue...')
    
  26. I saw the review request that adds this function. Is that really needed, or can you just implement that logic here? Ideally, it wouldn't need a new function.

  27. Instead of using view in these callbacks, it'd be better to do:

    cancel: _.bind(function() {
        // You can make use of 'this' now.     }, this)
    
  28. No trailing blank line in the function.

  29. Space between if and (.

  30. No trailing blank line.

  31. 
      
EN
EN
chipx86
  1. This looks pretty good! I have a couple small cleanup things to suggest, but otherwise, excellent!

  2. reviewboard/reviews/models.py (Diff revisions 2 - 3)
     
     

    Can you call this review_id, so it doesn't sound like a full-blown Review object?

  3. reviewboard/reviews/models.py (Diff revisions 2 - 3)
     
     

    No need for these outer parens now.

  4. reviewboard/reviews/models.py (Diff revisions 2 - 3)
     
     
     
     
     
     
     
     

    Kind of a big wall of text. Are there some good parts where you can break this up into separate paragraphs? Maybe after the first line, and at "If the user was targetted", and for the last line.

  5. reviewboard/reviews/models.py (Diff revisions 2 - 3)
     
     
     

    Alignment issue. The (weight / should be indented one more space.

  6. reviewboard/reviews/models.py (Diff revisions 2 - 3)
     
     
     
     

    This might be a bit nicer, and a little faster, by doing:

    sorted_entries = sorted(
        weighted.items(),
        key=itemgetter(1),
        reverse=True)
    sorted_users =[
        user
        for user, weight in sorted_entries[:self.MAX_SUGGESTED_REVIEWERS]
    ]
    
  7. Should be consistent with the quotes and use single quotes for the delimiter.

  8. 
      
EN
ED
  1. 
      
  2. reviewboard/reviews/models.py (Diff revision 4)
     
     
     

    Constants should be all capital letters, so something like RECENCY_WEIGHT and TARGET_WEIGHT.

  3. reviewboard/reviews/models.py (Diff revision 4)
     
     

    This can be simply "if entry['user']:"

  4. 
      
EN
EN
EN
EN
EN
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to dev/suggested-reviewers (0b5d243)
Loading...