Closed Bug 1194913 Opened 9 years ago Closed 8 years ago

MozReview's diff view should show the commit's author

Categories

(MozReview Graveyard :: Review Board: Extension, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: botond, Assigned: botond)

Details

Attachments

(5 files, 1 obsolete file)

Currently, MozReview's diff view for a commit doesn't show who the commit's author is, only who pushed the patch to review; the two may not be the same.

For example, this commit [1] was authored by Markus Stange, and pushed to review by me, but Markus' name doesn't appear anywhere in the diff view. It should.

[1] https://reviewboard.mozilla.org/r/16191/diff/1#index_header
Will this be a problem once anybody can push to MozReview? Commit authors mean nothing from a security and trust perspective. And this seems like more complexity for marginal gain. I prefer we avoid doing this if possible.
(In reply to Gregory Szorc [:gps] from comment #1)
> Will this be a problem once anybody can push to MozReview? Commit authors
> mean nothing from a security and trust perspective. 

I'm not sure I follow. What do you mean by "once anybody can push to MozReview"?

> And this seems like more
> complexity for marginal gain. I prefer we avoid doing this if possible.

Showing the author of a commit, on a page where you're viewing the contents of the commit, seems like pretty basic functionality to me. 

Also, I think it's important and useful for a patch reviewer to know who wrote the patch.
Currently, only people with LDAP accounts can push to MozReview. I have a goal to enable *anyone* to push to MozReview.

Also, the changeset author isn't necessarily the actual author. The field is easily spoofed. The authenticated person that added the changeset to MozReview is the only thing you can trust.
(In reply to Gregory Szorc [:gps] from comment #3)
> Currently, only people with LDAP accounts can push to MozReview. I have a
> goal to enable *anyone* to push to MozReview.

Ok, I see. In my case, though, I didn't push another person's commit because that other person didn't have access to MozReview; I pushed it because I was pushing the entire patch series for a bug, and different patches in the series had different authors. I believe this is a fairly common use case.

> Also, the changeset author isn't necessarily the actual author. The field is
> easily spoofed. The authenticated person that added the changeset to
> MozReview is the only thing you can trust.

I understand that. The use case I'm trying to address is not one of someone deliberately trying to spoof authorship, but merely the reviewer knowing who wrote each patch in a patch series, under an assumption of good faith (i.e. no spoofing) from the pusher.
Priority: -- → P3
[WIP] Bug 1194913 - Show the commit's author for child review requests
This is a WIP patch for showing the commit's author as an entry in the "Information" section of a child review request. Not quite working yet.
Product: Developer Services → MozReview
Component: General → Review Board: Extension
Going to try and finish this off.
Assignee: nobody → botond
Attachment #8664447 - Attachment is obsolete: true
Rebased the patch to apply to latest v-c-t trunk. I haven't made another other changes/fixes yet, so it's still (presumably) not working.
Comment on attachment 8732468 [details]
MozReview Request: reviewboard: show the commit's author for child review requests (bug 1194913); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41203/diff/1-2/
Comment on attachment 8732468 [details]
MozReview Request: reviewboard: show the commit's author for child review requests (bug 1194913); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41203/diff/2-3/
Attachment #8732468 - Attachment description: MozReview Request: [WIP] Bug 1194913 - Show the comit's author for child review requests → MozReview Request: Bug 1194913 - Show the comit's author for child review requests
The updated patch now seems to be working, but it needs a test (and/or existing tests updated).
Attachment #8732468 - Attachment description: MozReview Request: Bug 1194913 - Show the comit's author for child review requests → MozReview Request: Bug 1194913 - Show the comit's author for child review requests. r=smacleod
Attachment #8732468 - Flags: review?(smacleod)
Comment on attachment 8732468 [details]
MozReview Request: reviewboard: show the commit's author for child review requests (bug 1194913); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41203/diff/3-4/
Updated patch to adjust existing tests. The existing tests dump the extra_data in many places, so we have coverage for the presence of the 'author' field there. Posted for review.
Attachment #8732468 - Flags: review?(smacleod)
Comment on attachment 8732468 [details]
MozReview Request: reviewboard: show the commit's author for child review requests (bug 1194913); r=smacleod

https://reviewboard.mozilla.org/r/41203/#review39515

::: hgext/reviewboard/hgrb/proto.py:308
(Diff revision 4)
>          else:
>              reviewers = []
>              requal_reviewers = []
>          commits['individual'].append({
>              'id': node,
> +            'author': ctx.user(),

It seems strange to me that the tests use `test` as the user rather than something of the form `First Last <email@example.com>`...

:gps, do you foresee any possible problems we could hit with weird formats of username `ctx.user()` might spit out?

:botond, could you manually test this new field and behaviour with some typical usernames (and atypical possibly?) that you'd find in a commit. Maybe posting some screenshots of how it renders as well would be helpful

::: pylib/mozreview/mozreview/extension.py:274
(Diff revision 4)
>          # We want pull to appear first as it is the more robust way of
>          # retrieving changesets.
> +        ReviewRequestFieldsHook(self, 'info', [CommitAuthorField])

Your new line should go above the comment referencing the other two fields.

::: pylib/mozreview/mozreview/fields.py:47
(Diff revision 4)
>      extra_data fields on ReviewRequest and ReviewRequestDraft.
>      """

Can you add in a comment here that unlike built-in extra data fields the values will not be automatically copied for `draft_extra_data` to `extra_data` and the field id should be added to `DRAFTED_COMMIT_DATA_KEYS` if that behaviour is desired?

::: pylib/mozreview/mozreview/fields.py:212
(Diff revision 4)
>                  'commit_id': commit_id,
>                  'repo_path': repo_path,
>          }))
>  
>  
> +class CommitAuthorField(CommitDataBackedField):

So as it is this will display nothing beside the author label for any review request created before this change. I wonder if we should do something different in this case?

::: pylib/mozreview/mozreview/fields.py:213
(Diff revision 4)
>                  'repo_path': repo_path,
>          }))
>  
>  
> +class CommitAuthorField(CommitDataBackedField):
> +    """Field for the author of the commit being reviewed"""

"Field for the author of the review request's commit"

::: pylib/mozreview/mozreview/fields.py:216
(Diff revision 4)
>  
> +class CommitAuthorField(CommitDataBackedField):
> +    """Field for the author of the commit being reviewed"""
> +    field_id = AUTHOR_KEY
> +    label = _("Author")
> +    

Whitespace.
(In reply to Steven MacLeod [:smacleod] from comment #15)
> Comment on attachment 8732468 [details]
> MozReview Request: Bug 1194913 - Show the comit's author for child review
> requests. r=smacleod
> 
> https://reviewboard.mozilla.org/r/41203/#review39515
> 
> ::: hgext/reviewboard/hgrb/proto.py:308
> (Diff revision 4)
> >          else:
> >              reviewers = []
> >              requal_reviewers = []
> >          commits['individual'].append({
> >              'id': node,
> > +            'author': ctx.user(),
> 
> It seems strange to me that the tests use `test` as the user rather than
> something of the form `First Last <email@example.com>`...
> 
> :gps, do you foresee any possible problems we could hit with weird formats
> of username `ctx.user()` might spit out?

(Please reply in RB)
Flags: needinfo?(gps)
https://reviewboard.mozilla.org/r/41203/#review39515

> It seems strange to me that the tests use `test` as the user rather than something of the form `First Last <email@example.com>`...
> 
> :gps, do you foresee any possible problems we could hit with weird formats of username `ctx.user()` might spit out?
> 
> :botond, could you manually test this new field and behaviour with some typical usernames (and atypical possibly?) that you'd find in a commit. Maybe posting some screenshots of how it renders as well would be helpful

The default test user comes from the Mercurial test harness. We could consider changing it, but it will invalidate hashes throughout tests.
https://reviewboard.mozilla.org/r/41203/#review39695

::: hgext/reviewboard/hgrb/proto.py:308
(Diff revision 4)
>          else:
>              reviewers = []
>              requal_reviewers = []
>          commits['individual'].append({
>              'id': node,
> +            'author': ctx.user(),

This needs to be `encoding.fromlocal(ctx.user())`. `ctx.user()` and `ctx.description()` return `mercurial.encoding.localstr` instances that behave like str but are converted to the encoding Mercurial is configured with (should be utf-8 on all our servers). `encoding.fromlocal()` returns the raw bytes, which we prefer to operate on.
This is a screenshot showing how the author field is rendered in the information box.
This screenshot shows the rendering when a commit has two authors (this is rare in mozilla-central history, but it does happen).
This screenshots shows the rendering when the author field doesn't contain an email. Nothing special to see here, just posting it for completeness.
https://reviewboard.mozilla.org/r/41203/#review39515

> The default test user comes from the Mercurial test harness. We could consider changing it, but it will invalidate hashes throughout tests.

Please see the attached screenshots showing the rendering in various scenarios.

> So as it is this will display nothing beside the author label for any review request created before this change. I wonder if we should do something different in this case?

I amended the logic in should_render() to not render if the contents of the field are empty. Does that seem reasonable?
Comment on attachment 8732468 [details]
MozReview Request: reviewboard: show the commit's author for child review requests (bug 1194913); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41203/diff/4-5/
Attachment #8732468 - Attachment description: MozReview Request: Bug 1194913 - Show the comit's author for child review requests. r=smacleod → MozReview Request: reviewboard: show the commit's author for child review requests (bug 1194913); r=smacleod
Attachment #8732468 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/41203/#review39515

> Please see the attached screenshots showing the rendering in various scenarios.

rendering looks good :D. BTW, if you upload screenshots to a review request, rather than bugzilla, users can draw regions over the images and comment on them (and if you update the image attachment, it will allow you to diff between the versions).

> I amended the logic in should_render() to not render if the contents of the field are empty. Does that seem reasonable?

That was my first instinct as well, sounds good to me!
Comment on attachment 8732468 [details]
MozReview Request: reviewboard: show the commit's author for child review requests (bug 1194913); r=smacleod

https://reviewboard.mozilla.org/r/41203/#review40239

This looks good to me, just a couple of really small nits. Thanks for taking care of it :D

::: pylib/mozreview/mozreview/fields.py:48
(Diff revision 5)
>  
>      This Field class will emulate the behavior of normal review
>      request fields but stores its data in CommitData.extra_data
>      and CommitData.draft_extra_data instead of the built-in
>      extra_data fields on ReviewRequest and ReviewRequestDraft.
> +    

You have trailing whitespace on this line.

::: pylib/mozreview/mozreview/fields.py:224
(Diff revision 5)
> +        # requests different constituent commits can have different authors.
> +        return not is_parent(self.review_request_details) and value

Can you stick in a comment here that we're checking for empty values because review requests before the author field were introduced will not have this information.
Attachment #8732468 - Flags: review?(smacleod)
https://reviewboard.mozilla.org/r/41203/#review39515

> rendering looks good :D. BTW, if you upload screenshots to a review request, rather than bugzilla, users can draw regions over the images and comment on them (and if you update the image attachment, it will allow you to diff between the versions).

Neat! I'll keep that in mind for the future.
Comment on attachment 8732468 [details]
MozReview Request: reviewboard: show the commit's author for child review requests (bug 1194913); r=smacleod

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41203/diff/5-6/
Attachment #8732468 - Flags: review?(smacleod)
Comment on attachment 8732468 [details]
MozReview Request: reviewboard: show the commit's author for child review requests (bug 1194913); r=smacleod

https://reviewboard.mozilla.org/r/41203/#review40417
Attachment #8732468 - Flags: review?(smacleod) → review+
https://hg.mozilla.org/hgcustom/version-control-tools/rev/7f1f0da46a4a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Flags: needinfo?(gps)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: