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)
MozReview Graveyard
Review Board: Extension
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
Comment 1•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
(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.
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
(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.
Updated•9 years ago
|
Priority: -- → P3
Assignee | ||
Comment 5•9 years ago
|
||
[WIP] Bug 1194913 - Show the commit's author for child review requests
Assignee | ||
Comment 6•9 years ago
|
||
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.
Updated•8 years ago
|
Product: Developer Services → MozReview
Updated•8 years ago
|
Component: General → Review Board: Extension
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/41203/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/41203/
Assignee | ||
Updated•8 years ago
|
Attachment #8664447 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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/
Assignee | ||
Comment 11•8 years ago
|
||
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
Assignee | ||
Comment 12•8 years ago
|
||
The updated patch now seems to be working, but it needs a test (and/or existing tests updated).
Assignee | ||
Updated•8 years ago
|
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)
Assignee | ||
Comment 13•8 years ago
|
||
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/
Assignee | ||
Comment 14•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8732468 -
Flags: review?(smacleod)
Comment 15•8 years ago
|
||
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.
Comment 16•8 years ago
|
||
(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)
Comment 17•8 years ago
|
||
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.
Comment 18•8 years ago
|
||
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.
Assignee | ||
Comment 19•8 years ago
|
||
This is a screenshot showing how the author field is rendered in the information box.
Assignee | ||
Comment 20•8 years ago
|
||
Assignee | ||
Comment 21•8 years ago
|
||
This screenshot shows the rendering when a commit has two authors (this is rare in mozilla-central history, but it does happen).
Assignee | ||
Comment 22•8 years ago
|
||
This screenshots shows the rendering when the author field doesn't contain an email. Nothing special to see here, just posting it for completeness.
Assignee | ||
Comment 23•8 years ago
|
||
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?
Assignee | ||
Comment 24•8 years ago
|
||
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)
Comment 25•8 years ago
|
||
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 26•8 years ago
|
||
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)
Assignee | ||
Comment 27•8 years ago
|
||
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.
Assignee | ||
Comment 28•8 years ago
|
||
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 29•8 years ago
|
||
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+
Comment 30•8 years ago
|
||
https://hg.mozilla.org/hgcustom/version-control-tools/rev/7f1f0da46a4a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Flags: needinfo?(gps)
You need to log in
before you can comment on or make changes to this bug.
Description
•