Add a feature to automatically suggest reviewers for a given review request
Review Request #5029 — Created Nov. 26, 2013 and submitted
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') |
david | |
It's probably worth bailing out early in the alternative case, to avoid doing a bunch of unnecessary work: if self.get_latest_diffset() … |
david | |
It might be worth using collections.defaultdict here, so you don't have to do the if rr_id in targets ... else … |
david | |
You could probably get rid of the blank lines in this block and have it be more compact. |
david | |
Can you wrap this conditional in () and get rid of the characters? I think it's probably also worth swapping … |
david | |
Because you've got parentheses here, you don't need the \s |
david | |
Can you use parens instead of \? Here and below. |
david | |
Because this is inside [], you don't need the . Alignment is also a little bit weird here. |
david | |
Remove this blank line. |
david | |
No space between the key and : |
david | |
Alignment is off here (indent the second line 1 more space). |
david | |
Blank line between these. |
chipx86 | |
Missing trailing period. |
chipx86 | |
"most likely" "knowledgeable" |
chipx86 | |
"review request's" We try to be explicit with that, since a "request" is often an HTTPRequest. |
chipx86 | |
No blank line here. |
chipx86 | |
You should call this only once and store it, since it's an otherwise expensive call. |
chipx86 | |
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 … |
chipx86 | |
The second row of parens should line up with the first. |
chipx86 | |
Blank line between these. |
chipx86 | |
Should be: if (entry[...] not in all_reviews Since we use entry['reviewrequest_reviews'] more than once, it'd be best to pull it … |
chipx86 | |
Should use != instead of not ==. |
chipx86 | |
What's the contents of entry['reviewrequest_reviews']? |
chipx86 | |
You already have this pulled out into a variable. |
chipx86 | |
Can you call this review_id, so it doesn't sound like a full-blown Review object? |
chipx86 | |
No need for these outer parens now. |
chipx86 | |
Comments should have sentence capitalization. Same with the other comments. |
chipx86 | |
Can you add a comment block describing in detail the algorithm you're using? |
chipx86 | |
Kind of a big wall of text. Are there some good parts where you can break this up into separate … |
chipx86 | |
Spacing is a tiny bit off here--one more space before -, and one fewer after. |
david | |
Alignment problem with now and -. Also, too many spaces after -. |
chipx86 | |
Blank line before this. |
chipx86 | |
Wrap these in (), get rid of \, and fix alignment of the second line. |
david | |
Instead of a \\, you should use parens. |
chipx86 | |
Instead of hard-coding 5 here, let's have a constant in the class definition. Something like MAX_GUESSED_REVIEWERS. |
chipx86 | |
Alignment issue. The (weight / should be indented one more space. |
chipx86 | |
This might be a bit nicer, and a little faster, by doing: sorted_entries = sorted( weighted.items(), key=itemgetter(1), reverse=True) sorted_users =[ … |
chipx86 | |
Since both of these are unique IDs, it's actually faster to not do this.$(..), and instead do $(..). |
chipx86 | |
Blank line on either side of this. |
chipx86 | |
No leading or trailing blank lines in the function. Do you need to redo the .suggest-user query, or can you … |
chipx86 | |
Get rid of this blank line. |
david | |
Not sure "Person" needs to be capitalized here. Maybe "the person" |
chipx86 | |
You can use chaining to simplify this: this._$targetPeople .inlineEditor('startEdit') .inlineEditor('appendToFieldValue...') |
chipx86 | |
I saw the review request that adds this function. Is that really needed, or can you just implement that logic … |
chipx86 | |
Should be consistent with the quotes and use single quotes for the delimiter. |
chipx86 | |
Instead of using view in these callbacks, it'd be better to do: cancel: _.bind(function() { // You can make use … |
chipx86 | |
No trailing blank line in the function. |
chipx86 | |
Space between if and (. |
chipx86 | |
No trailing blank line. |
chipx86 | |
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 |
- Depends On:
-
- Added Files:
-
-
-
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 = ...
-
It might be worth using collections.defaultdict here, so you don't have to do the
if rr_id in targets ... else ...
thing. -
-
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):
-
-
-
-
-
-
-
-
-
-
-
-
-
-
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(...)
-
-
-
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. -
-
-
-
-
-
-
-
-
Instead of hard-coding 5 here, let's have a constant in the class definition. Something like
MAX_GUESSED_REVIEWERS
. -
Since both of these are unique IDs, it's actually faster to not do
this.$(..)
, and instead do$(..)
. -
-
No leading or trailing blank lines in the function.
Do you need to redo the
.suggest-user
query, or can you useallPeople
above? -
-
You can use chaining to simplify this:
this._$targetPeople .inlineEditor('startEdit') .inlineEditor('appendToFieldValue...')
-
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.
-
Instead of using
view
in these callbacks, it'd be better to do:cancel: _.bind(function() { // You can make use of 'this' now. }, this)
-
-
-
-
This looks pretty good! I have a couple small cleanup things to suggest, but otherwise, excellent!
-
-
-
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.
-
-
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] ]
-
- Change Summary:
-
Realized that my TARGET_WEIGHTING was set to 0.5 and I was multiplying so it was actually halving the weight instead of increasing when a user was targetted for review
- Change Summary:
-
Realized that I forgot to change a variable around while I was refactoring things. Fixed this now.