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)
     
     
    Show all issues

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

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

    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)
     
     
    Show all issues

    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)
     
     
     
     
     
     
     
     
    Show all issues

    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)
     
     
     
    Show all issues

    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)
     
     
     
     
     
    Show all issues

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

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

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

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

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

  10. Show all issues

    Remove this blank line.

  11. Show all issues

    No space between the key and :

  12. Show all issues

    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)
     
     
     
    Show all issues

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

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

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

  4. Show all issues

    Get rid of this blank line.

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

    Blank line between these.

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

    Missing trailing period.

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

    "most likely"

    "knowledgeable"

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

    "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)
     
     
     
     
    Show all issues

    No blank line here.

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

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

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

    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)
     
     
     
    Show all issues

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

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

    Blank line between these.

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

    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)
     
     
    Show all issues

    Should use != instead of not ==.

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

    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)
     
     
    Show all issues

    You already have this pulled out into a variable.

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

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

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

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

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

    Alignment problem with now and -.

    Also, too many spaces after -.

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

    Blank line before this.

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

    Instead of a \\, you should use parens.

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

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

  21. Show all issues

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

  22. Show all issues

    Blank line on either side of this.

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

    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. Show all issues

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

  25. Show all issues

    You can use chaining to simplify this:

    this._$targetPeople
        .inlineEditor('startEdit')
        .inlineEditor('appendToFieldValue...')
    
  26. Show all issues

    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. Show all issues

    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. Show all issues

    No trailing blank line in the function.

  29. Show all issues

    Space between if and (.

  30. Show all issues

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues

    No need for these outer parens now.

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

    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)
     
     
     
    Show all issues

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

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

    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. Show all issues

    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)
     
     
     
    Show all issues

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

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

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

  4. 
      
EN
EN
EN
EN
EN
Review request changed
Status:
Completed
Change Summary:
Pushed to dev/suggested-reviewers (0b5d243)