Closed
Bug 1248008
Opened 9 years ago
Closed 8 years ago
No obviously sane way to add review comments to the commit message
Categories
(MozReview Graveyard :: General, defect)
MozReview Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: zalun)
References
Details
Attachments
(6 files, 14 obsolete files)
58 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smacleod
:
review+
|
Details |
I'm doing a review on a commit that has some problems that need fixing in the commit message. Unlike "edit attachment as comment" in Bugzilla, which allows me to trivially add review comments on the commit message, mozreview doesn't seem to have a decent way to do this.
Comment 1•9 years ago
|
||
smacleod: I know we've talked about this. Pretty sure you had an idea. Want to brain dump so someone else can implement it? Also, is this a dupe?
Flags: needinfo?(smacleod)
Updated•9 years ago
|
Product: Developer Services → MozReview
Comment 2•9 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #1)
> smacleod: I know we've talked about this. Pretty sure you had an idea. Want
> to brain dump so someone else can implement it? Also, is this a dupe?
The easiest way we can hack around this is just create a plain text attachment on the review request containing the commit message - this will allow a reviewer to comment on lines like the diff viewer (we could also update this attachment when the message changes, giving you a diff).
A more involved way would be hacking the addition of a "commit message file" into the diffviewer (which could be done by spoofing a file addition)
Review Board 2.6 (release date unknown) adds a mechanism for general review comments with issues which could be used to do something like this as well.
Flags: needinfo?(smacleod)
Comment 3•9 years ago
|
||
Yes, this was filed as bug 1194354, but there's now more context here so I'll dupe the other to this one.
Comment 5•9 years ago
|
||
Never mind, that other bug is a separate issue.
Comment 6•9 years ago
|
||
Some notes:
* The code that creates/updates Review Requests after a push is the BatchReviewRequestResource web API, specifically _process_submission() (http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/resources/batch_review_request.py#l352).
* You're going to need to create or update a FileAttachment object (https://github.com/reviewboard/reviewboard/blob/release-2.5.x/reviewboard/attachments/models.py#L55) in that function.
* You will need tests for this, which will involve modifying or creating a new mach command (http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/testing/vcttesting/reviewboard/mach_commands.py).
Assignee: nobody → pzalewa
Assignee | ||
Comment 7•9 years ago
|
||
Adding some info here as it's gonna be easier to refer to.
Commit message is read to set `draft.description` in `update_review_request`: http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/resources/batch_review_request.py#l914
It happens on every push.
If a new commit is created `update_review_request` is called from http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/resources/batch_review_request.py#l683 This is the place where attachment should be created
If commit is amended (`hg commit --amend`) - from http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/resources/batch_review_request.py#l589 This is the place where attachment should be updated
Assignee | ||
Comment 8•9 years ago
|
||
As for above r/place/case - code should be inside `update_review_request`
Assignee | ||
Comment 9•9 years ago
|
||
I tried to add text attachment to a review and UI did not responded.
Assignee | ||
Comment 10•9 years ago
|
||
MozReview does not have a way to comment on commit message.
This patch creates a plain text attachment so reviewer can comment on it.
If a commit message has been amended attachment will be updated.
Feature is not finished - no snippet is visible (None is printed instead).
Review commit: https://reviewboard.mozilla.org/r/52441/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52441/
Attachment #8752153 -
Flags: review?(smacleod)
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/1-2/
Comment 12•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review49856
Looks good (although smacleod might be pickier :), but as discussed I think we need (a) tests and (b) support for the on-hover comment button that we implemented for diffs in https://bugzilla.mozilla.org/show_bug.cgi?id=1249647.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:930
(Diff revision 2)
> + commit_message_temp.flush()
> + try:
> + comment_attachment = draft.file_attachments.get(caption='commit_message')
> + except FileAttachment.DoesNotExist:
> + draft.file_attachments.create(
> + caption='commit_message',
This is the caption displayed to users in the UI right? If so, I think it should be a more human-readable string, e.g. "Commit message".
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/2-3/
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/3-4/
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/4-5/
Comment 16•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review50422
There are a few problems here:
* The review comments are not mirrored to Bugzilla. You'll have to update `on_review_request_publishing()` in `pylib/mozreview/mozreview/signal_handlers.py`.
* Updates to a commit message, e.g. after `hg commit --amend`, are not reflected in the attachment. I pushed up a commit, the amended it to just change the commit message and push it up again, and the Description field was updated but not the contents of the Commit message file attachment.
::: hgext/reviewboard/tests/test-commits-added.t:56
(Diff revision 5)
> +Commit message attachments should be created
> +
> + $ reviewboard dump-attachments 2
> + - id: 1
> + caption: Commit message
> + filename: tmpjknDs5
You're going to need a * (glob) here because this is a randomly generated filename that will change in each test run.
::: hgext/reviewboard/tests/test-commits-added.t:64
(Diff revision 5)
> + 1 - Foo 1</pre><pre></pre><pre>MozReview-Commit-ID: 124Bxg</pre></div></div>'
> +
> + $ reviewboard dump-attachments 3
> + - id: 2
> + caption: Commit message
> + filename: tmpn9Kbo7
* (glob) here too.
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/5-6/
Attachment #8752153 -
Flags: review?(mcote)
Updated•9 years ago
|
Attachment #8752153 -
Flags: review?(mcote)
Comment 18•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review50458
Also note there should be tests for both things.
Assignee | ||
Comment 19•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review50422
The only error I've got on --amend is a `ERROR - - Failed to update the following attachments: set([1])` (number changes depending on the review request)
Looks like it's failing when trying to update the comment attachment at bugzilla https://reviewboard-hg.mozilla.org/version-control-tools/file/tip/pylib/mozreview/mozreview/bugzilla/client.py#l200. `ids_to_update` is empty ([]) for the creation of the attachment and equals `[{'comment': u'Review request updated; see interdiff: http://172.16.59.128:55556/r/5/diff/1-2/\n', 'file_name': 'reviewboard-5-url.txt', 'attachment_id': 3, 'summary': u'MozReview Request: NEW K bug 11 r?level1'}]` when attachment is updated. I tried both methods of updating a FileField, presented in the diff and comment_attachment.file.save(att_name, File(commit_message_temp)), both with the same result.
I assumed the attachment should exist only in reviewboard, but now I get that the comments to the message's attachment should be pushed to bugzilla like other comments.
I'm looking now why the commit message attachment is not created for bugzilla in the first place.
Assignee | ||
Comment 20•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review50422
Please specify "review comments". Information about a MozReview request appears in bugzilla as a comment.
Please confirm there is a need to add commit message attachment to bugzilla as well (as explained above).
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/6-7/
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/7-8/
Comment 23•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Zalun tells me this is still a work in progress, so clearing review flags.
Attachment #8752153 -
Flags: review?(smacleod)
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/8-9/
Attachment #8752153 -
Attachment description: MozReview Request: Save commit message as a file attachment (bug 1248008) r?smacleod r?mcote → MozReview Request: Save commit message as a file attachment (bug 1248008)
Attachment #8752153 -
Flags: review?(smacleod)
Attachment #8752153 -
Flags: review?(mcote)
Comment 25•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review51862
I'm going to clear the review flag and let :mcote keep taking care of this until it's further along.
Attachment #8752153 -
Flags: review?(smacleod)
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/9-10/
Comment 27•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Clearing again since this is still in progress. Zalun, I see that you took out the r?s from the commit message, but could you also clear the reviewers from Review Board via the Review Summary link? That way I believe it will stop re-requesting me for review until you're ready. :)
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 28•9 years ago
|
||
Done
Assignee | ||
Comment 29•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review50422
--amend is now supported - working on the comments
Comment 30•9 years ago
|
||
We'll need the docs to be updated for this change too, please.
Assignee | ||
Comment 31•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/10-11/
Assignee | ||
Comment 32•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/11-12/
Assignee | ||
Comment 33•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review50422
Added attachment comments - no diff thought. I think it's not saved. It might be created live (based on FileAttachmentHistory), question is only if it should be done.
Assignee | ||
Comment 34•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/12-13/
Attachment #8752153 -
Attachment description: MozReview Request: Save commit message as a file attachment (bug 1248008) → MozReview Request: Save commit message as a file attachment (bug 1248008) r?mcote
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 35•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review49856
About the [+]:
I'm searching for a way to add a viewdiff.less to attachment review.
2. 1. I've found that reviewboard/static/rb/pages/diffviewer.less is loaded
2. In reviewboard/staticbundles.py diffviewer.less is mentioned in 'reviews'
3. Adding 'reviewes': { ... viewdiff.less} to mozreview/extensions.py didn't worked (I mean the viewdiff.less is not used to build the css)
I'm looking ...
Comment 36•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review54376
This is good stuff! A few more things to consider:
As discussed earlier today, our extension customizes the draft banner to read "Finish..." instead of "Edit Review" and "Publish". We need to do the same thing on file-attachment views as well. This stuff is done in http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/static/mozreview/js/review.js. You might be able to just change http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/extension.py#l163 to use `reviewable_url_names` instead, as with the on-hover "+" diff-viewer enhancement that we discussed.
Aside from that, mostly some smaller things:
::: hgext/reviewboard/tests/test-commits-message-attachment.t:13
(Diff revision 13)
> + $ echo 'foo1' > foo
> + $ hg commit -A -m 'Bug 1 - Foo 1'
> + adding foo
> + $ echo 'foo2' > foo
> + $ hg commit -m 'Bug 1 - Foo 2'
> + $ hg push
Trailing whitespace.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:8
(Diff revision 13)
> +from django.core.files.base import File
> +from django.core.files.base import ContentFile
Combine these two lines.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:925
(Diff revision 13)
>
> draft.summary = commit['message'].splitlines()[0]
> draft.description = commit['message']
> draft.bugs_closed = commit['bug']
>
> + # --- create/update a new commit message attachment
Nit: we try to follow PEP 8 on comments, that is, "Comments should be complete sentences. If a comment is a phrase or sentence, its first word should be capitalized, unless it is an identifier that begins with a lower case letter".
https://www.python.org/dev/peps/pep-0008/#comments
::: pylib/mozreview/mozreview/resources/batch_review_request.py:934
(Diff revision 13)
> + cm_temp.write(draft.description)
> + cm_temp.flush()
> + cm_file = File(cm_temp)
> + cm_caption = 'Commit message'
> + comment_atts = draft.file_attachments.filter(caption=cm_caption)
> + if len(comment_atts) == 0:
Nit: I think `if not comment_atts:` is slightly more Pythonic.
Also we try to follow Review Board's coding standards, which extend PEP 8. Specifically here (and later), blank lines around "if" blocks.
https://www.reviewboard.org/docs/codebase/dev/coding-standards/#spacing
::: pylib/mozreview/mozreview/resources/batch_review_request.py:953
(Diff revision 13)
> + try:
> + latest = att_history.file_attachments.latest()
> + except FileAttachment.DoesNotExist:
> + latest = None
> + if latest is None:
> + # This should theoretically never happen, but who knows.
We should log an error then.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:959
(Diff revision 13)
> + att_revision = 1
> + elif latest.review_request.exists():
> + # This is a new update in the draft.
> + att_revision = latest.attachment_revision + 1
> + else:
> + # This should actually be the case...
I don't understand this comment.
::: pylib/rbbz/rbbz/diffs.py:326
(Diff revision 13)
> +def render_plain_fileattachment_comment(comment):
> + """render a comment on a attached file"""
> + ret = ''
> + # XXX comment.diff_against_file_attachment is None - should it be like that?
> + if comment.file_attachment.caption == 'Commit message':
> + ret = 'Comment to a commit message:\n'
> + else:
> + ret = 'Comment to "%s":\n' % comment.file_attachment.caption
> +
> + return '%s\n%s' % (ret, comment.text)
We should provide some context in the comment. If it's a review against the file itself, then print out the line(s) that are commented, e.g.
> Update foo (bug 1234); r?level3
And if it's a comment on a commit-message diff, render it as a regular diff (as `render_comment_plain()` does).
Users really like context in the BMO comments. :)
::: pylib/rbbz/rbbz/diffs.py:327
(Diff revision 13)
> lines.append(u"\n%s" % comment)
>
> return u"\n".join(lines)
>
> +def render_plain_fileattachment_comment(comment):
> + """render a comment on a attached file"""
Nit: "an attached file". Also see PEP 8 for comment style.
::: pylib/rbbz/rbbz/diffs.py:329
(Diff revision 13)
> return u"\n".join(lines)
>
> +def render_plain_fileattachment_comment(comment):
> + """render a comment on a attached file"""
> + ret = ''
> + # XXX comment.diff_against_file_attachment is None - should it be like that?
That's a good question. Worth asking smacleod or mconley, although they might not even know.
::: pylib/rbbz/rbbz/diffs.py:331
(Diff revision 13)
> +def render_plain_fileattachment_comment(comment):
> + """render a comment on a attached file"""
> + ret = ''
> + # XXX comment.diff_against_file_attachment is None - should it be like that?
> + if comment.file_attachment.caption == 'Commit message':
> + ret = 'Comment to a commit message:\n'
Nit: "Comment on the commit message"
Attachment #8752153 -
Flags: review?(mcote) → review-
Assignee | ||
Comment 37•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review54376
> I don't understand this comment.
Copied from reviewboard :)
> We should provide some context in the comment. If it's a review against the file itself, then print out the line(s) that are commented, e.g.
>
> > Update foo (bug 1234); r?level3
>
> And if it's a comment on a commit-message diff, render it as a regular diff (as `render_comment_plain()` does).
>
> Users really like context in the BMO comments. :)
Good call,
I will have to dig a little further to get the commented line (I think we do store it). The diff however is different story - I would need to create the diff for bugzilla - we don't store it for attachments.
I've left the XXX comment as I haven't found any case in which it is used. I will search more in the reviewboard's code for that a little more.
Assignee | ||
Comment 38•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/13-14/
Attachment #8752153 -
Attachment description: MozReview Request: Save commit message as a file attachment (bug 1248008) r?mcote → Save commit message as a file attachment (bug 1248008)
Attachment #8752153 -
Flags: review?(smacleod)
Attachment #8752153 -
Flags: review?(mcote)
Attachment #8752153 -
Flags: review-
Assignee | ||
Comment 39•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/14-15/
Comment 40•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Clearing the flags since there's more to do on this patch still.
Attachment #8752153 -
Flags: review?(smacleod)
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 41•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review49856
[+] is there, but all comments are marked with number "1" only. I believe a JS file is not included properly
Assignee | ||
Comment 42•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review49856
OK, it's not a number of comments on file in general, but number of comments on the line
Assignee | ||
Comment 43•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review54376
> Good call,
> I will have to dig a little further to get the commented line (I think we do store it). The diff however is different story - I would need to create the diff for bugzilla - we don't store it for attachments.
> I've left the XXX comment as I haven't found any case in which it is used. I will search more in the reviewboard's code for that a little more.
I like that in bugzilla code has different background AFAIK it comes from "> " at the begin of the line. Since I've got no diff here - can I use a "> " and mark commented line with a * ? - mozre in a screenshot.
Assignee | ||
Comment 44•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/15-16/
Assignee | ||
Comment 45•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review54376
> I like that in bugzilla code has different background AFAIK it comes from "> " at the begin of the line. Since I've got no diff here - can I use a "> " and mark commented line with a * ? - mozre in a screenshot.
I'm also looking into finding a way to provide a diff...
Assignee | ||
Comment 46•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review54376
> That's a good question. Worth asking smacleod or mconley, although they might not even know.
It is set in view when updating. It's just the last attached file (not a diff)
Assignee | ||
Comment 47•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review54376
Added the `reviewable_url_names` in `extension.py`, but nothing changed. Assuming the situation is similar to CSS, I tried TemplateHook - added `mozreview/review-scripts-js.html`, but no change there. If it would work, I'd need to add more complexity as TemplateHook is using `review_request_url_names` which also expands `diffviewer_url_names`.
Assignee | ||
Comment 48•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/16-17/
Assignee | ||
Comment 49•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review54376
I think I've got it in, there is an error, as this html is not exactly as in reviews - `init.rr.js` throws "Could not find a valid id for the parent review request."
Assignee | ||
Comment 50•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review54376
Should I remove the Publish button? I haven't seen it in the `review.js` ...
Assignee | ||
Comment 51•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/17-18/
Attachment #8752153 -
Flags: review?(mcote)
Comment 52•9 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review54376
Yes, it should be removed. It is not shown in the regular diff view.
Assignee | ||
Comment 53•9 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/18-19/
Assignee | ||
Comment 54•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/19-20/
Assignee | ||
Comment 55•8 years ago
|
||
Comment 56•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review56746
I see the Finish Review button now; however, the dialog still shows the "Ship it" checkbox instead of the review-flag dropdown. As previously, switching to the Diff or Reviews view works. So looks like you just need to put that override in the File Attachment view as well.
Also, it seems that when you highlight X lines, the BMO comment only reflects X-1 lines. For example, if I comment on lines 1-3, the BMO comment only displays lines 1-2.
::: pylib/rbbz/rbbz/diffs.py:359
(Diff revisions 14 - 20)
> + att_f = comment.file_attachment.file
> + att_f.open()
> + lines = list(att_f)
> +
> + att_f.close()
> + logger.info(lines)
I don't think we want to be logging this all at info level in production. :)
::: pylib/rbbz/rbbz/diffs.py:364
(Diff revisions 14 - 20)
> + logger.info(lines)
> +
> + # adding a ">" sign to commented lines
> + first_line_index = first_line - 1
> + last_line_index = min(first_line_index + num_lines, len(lines)) - 1
> + ret = ['Comment on "%s":\n\n' % caption]
I think we should drop the quotes, so that it looks like
Comment on Commit Message
Looks a little nicer.
::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:24
(Diff revision 20)
> + // understand what clicking it does.
> + $("#review-link").text("Finish Review...");
> +
> + // Remove "Publish" button
> + $('#review-banner-publish').remove();
> +
As I remarked in my general comment, you'll need the review-flag dropdown stuff in here as well (ideally this would be in a single JS file that we could load in both places, to avoid copying it).
::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:25
(Diff revision 20)
> + $("#review-link").text("Finish Review...");
> +
> + // Remove "Publish" button
> + $('#review-banner-publish').remove();
> +
> + // No MozReview here
Yeah, I don't think any of this is required. The first part is for the metadata panel, which isn't displayed in the File Attachment view, and the second part is for the Reviews view specifically.
Attachment #8752153 -
Flags: review?(mcote) → review-
Assignee | ||
Comment 57•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review56746
Now this (r+/r-/r?) is slightly more complicated. The problem is with Backbone - the extension `mozreview/js/review_flah.js` is not initialized. I mean its initialize function is not even called, View is created though. I've also added the `mozreview_ready` trigger... I'll deal with it today.
Assignee | ||
Comment 58•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/20-21/
Attachment #8752153 -
Flags: review- → review?(mcote)
Assignee | ||
Comment 59•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/21-22/
Comment 60•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review58020
Not sure if this is ready for me to review yet, but I just tried it out again. I like where it's going, although the following jumped out:
* The rendering on the Reviews view seems broken. The commits table doesn't render properly, and I get this error: "Well, this is embarassing... Something's gone wrong in either retrieving or manipulating these review requests. Sorry about that. Please consider filing a bug and including the following information: ..."
* The "Review" link beside description should be a button.
* The "Files" header is still there even though there are no files. It should also be hidden unless there are other file attachments uploaded by the author.
Finally, I was thinking, when you're ready, you could flag davidwalsh for review too, for the front-end stuff.
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 61•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/22-23/
Attachment #8752153 -
Flags: review?(mcote)
Assignee | ||
Comment 62•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/23-24/
Assignee | ||
Comment 63•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review58020
* I've seen the "embarrassing" bit as well, I thought it was some sort of a glitch... will dive into.
* Button - please show me another one. Like [Collapse changes] ?
* Yes, I was wondering if it's there if no attachments are present. I'll fix it
Assignee | ||
Comment 64•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/24-25/
Assignee | ||
Comment 65•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review58020
So, there is only button left - how you would like to have it?
Comment 66•8 years ago
|
||
https://reviewboard.mozilla.org/r/52439/#review58166
::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:3
(Diff revision 28)
> +// A hack to allow review_flag.js work without MozReview object
> +
> +var MozReview = { isParent: false };
Would this be safer?
```
var MozReview = window.MozReview || {};
MozReview.isParent = false;
```
::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:9
(Diff revision 28)
> +
> +$(document).ready(function() {
> + // Workaround until we get a template hook point before
> + // the draft banner thing.
> + $("#new-navbar").insertBefore(".box.review-request");
> + $("#new-navbar").show();
We can probably chain the `show()` onto the `insertBefore()'
::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:29
(Diff revision 28)
> + $("#review-link").text("Finish Review...");
> +
> + // Remove "Publish" button
> + $('#review-banner-publish').remove();
> +
> + $(document).trigger("mozreview_ready");
I don't think we want this; `init_rr.js` is the only script that should be triggering `mozreview_ready`.
::: pylib/mozreview/mozreview/static/mozreview/js/review.js:25
(Diff revision 28)
> $("#review-link").text("Finish Review...");
>
> + // Move commit message file attachment's review button to the right
> + // commit message
> + // hide the file_attachment
> + var attachments = $('.file-container');
We can do `$('.file-container').each` instead of your `for` loop.
Assignee | ||
Comment 67•8 years ago
|
||
https://reviewboard.mozilla.org/r/52439/#review58166
> I don't think we want this; `init_rr.js` is the only script that should be triggering `mozreview_ready`.
init_rr.js bases on existense of a parent review DOM object. Hence the special file for file_attachments.
Assignee | ||
Comment 68•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/25-26/
Assignee | ||
Comment 69•8 years ago
|
||
https://reviewboard.mozilla.org/r/52439/#review58166
> init_rr.js bases on existense of a parent review DOM object. Hence the special file for file_attachments.
Please reopen if you've got other solution
Comment 70•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review60838
This is getting closer, but I noticed one bug: the "Review" link is duplicated for all revisions past 1. In fact, there's one Review link for each revision. :) So revision 3 of a commit will see "ReviewReviewReview".
Also please file a follow-up bug, with the "depends on" field set to this bug, for quoting the diff itself in Bugzilla comments when the reviewer leaves a review on the diff of two revisions of a commit message.
Attachment #8752153 -
Flags: review?(mcote) → review-
Comment 71•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review60878
As noted in my previous quick review, this is really really close. Just a few little things to clean up and then we're good to go.
::: pylib/mozreview/mozreview/extension.py:280
(Diff revision 26)
> + # TODO check if this one is not an overkill
> + TemplateHook(self, 'base-css', 'mozreview/review-stylings-css.html',
> + apply_to=reviewable_url_names)
As you note, I believe you only need this and not the TemplateHook() call above, since reviewable_url_names includes (I believe) all of review_request_url_names.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:939
(Diff revision 26)
> + cm_file = File(cm_temp)
> + cm_caption = 'Commit message'
> + comment_atts = draft.file_attachments.filter(caption=cm_caption)
> +
> + if not comment_atts:
> + # no comment attachment found in review request, create a new one
As previously noted, comments should be complete sentences, starting with a capital and ending with a period (though the period can be omitted for short comments) as per PEP 8.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:947
(Diff revision 26)
> + # at least one commit attachment found
> + # find attachment history and update attachment
Proper sentences here too.
::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:1
(Diff revision 26)
> +/* mozreview viewdiff.less */
> +
I don't think this is needed.
::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:6
(Diff revision 26)
> +// A hack to allow review_flag.js work without MozReview object
> +
> +var MozReview = window.MozReview || { isParent: false };
> +
> +$(document).ready(function() {
> + // Workaround until we get a template hook point before
Now that we know this is working, we should get rid of the duplication between this file and review.js. It's only a matter of time until someone creates a bug (or spends a lot of time debugging) by updating one file but not the other.
Could you make a common function that both file_attachmen_review.js and review.js call, to avoid the redundancy?
::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:27
(Diff revision 26)
> + // Change string of "Review" button to be a verb so people better
> + // understand what clicking it does.
> + $("#review-link").text("Finish Review...");
> +
> + // Remove "Publish" button
> + $('#review-banner-publish').remove();
For other views, this is actually removed in css (http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/static/mozreview/css/review.less#l49). Probably best to use that same technique.
::: pylib/mozreview/mozreview/static/mozreview/js/review.js:27
(Diff revision 26)
> + // Move commit message file attachment's review button to the right
> + // commit message
> + // hide the file_attachment
> + var attachments = $('.file-container');
> + attachments.each(function() {
> + if ($(this).find('.file-caption a.edit').text() === 'Commit message') {
Just a thought--now that we're removing the attachment UI, is the caption displayed anywhere (like on the file-attachment page)? If not, I wonder if we should try to make the caption more unique, to avoid someone uploading a file manually with the same caption and then accidentally triggering this code. It's unlikely, but by making the caption a weird, longer string, it'll be even less likely.
That's only assuming the caption is no longer visible anywhere.
::: pylib/rbbz/rbbz/diffs.py:329
(Diff revision 26)
> + # Do not provide additional data if not commenting on source
> + if comment.extra_data['viewMode'] != 'source':
> + return 'Comment on "%s"\n\n%s' % (comment.file_attachment.caption,
> + comment.text)
I'm not exactly sure what this means. What is a viewMode that's not source, and do we definitely want to comment in Bugzilla in that case?
Assignee | ||
Comment 72•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review60838
It was picking file attachments mentioned later in "Review request changed" message. We might want to not add these...
Assignee | ||
Comment 73•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/26-27/
Attachment #8752153 -
Flags: review- → review?(mcote)
Assignee | ||
Comment 74•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/27-28/
Assignee | ||
Comment 75•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/28-29/
Assignee | ||
Comment 76•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review60878
> As you note, I believe you only need this and not the TemplateHook() call above, since reviewable_url_names includes (I believe) all of review_request_url_names.
It does not include it - https://github.com/reviewboard/reviewboard/blob/master/reviewboard/urls.py#L25-L40
But I think it needs to be just the urls we want...
Assignee | ||
Comment 77•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review60878
> I'm not exactly sure what this means. What is a viewMode that's not source, and do we definitely want to comment in Bugzilla in that case?
It is another type then - (image, zip)
Assignee | ||
Comment 78•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review60878
> Just a thought--now that we're removing the attachment UI, is the caption displayed anywhere (like on the file-attachment page)? If not, I wonder if we should try to make the caption more unique, to avoid someone uploading a file manually with the same caption and then accidentally triggering this code. It's unlikely, but by making the caption a weird, longer string, it'll be even less likely.
>
> That's only assuming the caption is no longer visible anywhere.
It is visible in the "attachment added message" as in https://reviewboard.mozilla.org/r/52441/file/318/
Comment 79•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review60878
> It is visible in the "attachment added message" as in https://reviewboard.mozilla.org/r/52441/file/318/
Hm that's not terribly important. Then again, with the commit message being automatically uploaded, it's probably not much of an issue. Let's leave it for now.
> It is another type then - (image, zip)
Ah okay.
Comment 80•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review60878
> As previously noted, comments should be complete sentences, starting with a capital and ending with a period (though the period can be omitted for short comments) as per PEP 8.
Doesn't look like you addressed this in the new commit.
> Proper sentences here too.
Doesn't look like you addressed this in the new commit.
> I don't think this is needed.
Doesn't look like you addressed this in the new commit.
Comment 81•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review61320
Almost done looking at this again, but so far it looks good aside from the missed issues from the last review, and a couple minor comments below. Will give this a final look tomorrow.
::: pylib/mozreview/mozreview/extension.py:125
(Diff revision 29)
> apply_to = review_request_url_names
>
>
> class MRReviewFlagExtension(JSExtension):
> model_class = 'MRReviewFlag.Extension'
> - apply_to = review_request_url_names
> + apply_to = reviewable_url_names
Based on your comment that `reviewable_url_names` does not include everything in `review_request_url_names`, doesn't this (and possibly other places) need to be changed as you do with the `TemplateHooks()` below?
::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:4
(Diff revision 29)
> +// A hack to allow review_flag.js work without MozReview object
> +
> +var MozReview = window.MozReview || { isParent: false };
> +
Ah! So in the end we could include `review.js` on all pages? That's pretty nice. :)
Assignee | ||
Comment 82•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/29-30/
Assignee | ||
Comment 83•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review61320
> Ah! So in the end we could include `review.js` on all pages? That's pretty nice. :)
Yes, I remember there were issues, but after these were sorted with adding `review_flag.js`
Assignee | ||
Comment 84•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review60878
> Doesn't look like you addressed this in the new commit.
I marked is as fixed before puching the changes... (same for the next comment messages)
Assignee | ||
Comment 85•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review61320
> Based on your comment that `reviewable_url_names` does not include everything in `review_request_url_names`, doesn't this (and possibly other places) need to be changed as you do with the `TemplateHooks()` below?
Good catch, checking the diff
Comment 86•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review60878
> I marked is as fixed before puching the changes... (same for the next comment messages)
Okay that's confusing. :) You pushed several commits and flagged me for review, so I figured it was good to review again. If you have work in progress, please remove me from the reviewers list until the patch is ready for a proper review again.
Assignee | ||
Comment 87•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/30-31/
Updated•8 years ago
|
Attachment #8752153 -
Flags: review?(mcote) → review-
Comment 88•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review61954
Functionally, this appears to be working just fine. \o/ I have one comment about how you're hiding the file list, and a few more little nits.
::: pylib/mozreview/mozreview/extension.py:125
(Diff revision 31)
> apply_to = review_request_url_names
>
>
> class MRReviewFlagExtension(JSExtension):
> model_class = 'MRReviewFlag.Extension'
> - apply_to = review_request_url_names
> + apply_to = reviewable_url_names + ['file-attachment', 'screenshot']
You have `file_attachment_url_names` defined now, so you might as well use it here.
Also, you switched from `review_request_url_names` to `reviewable_url_names`. This means that the `review-request-detail` view is no longer included. I don't think that was intentional, was it?
::: pylib/mozreview/mozreview/extension.py:294
(Diff revision 31)
> # All of our review request styling is injected via
> # review-stylings-css, which in turn loads the review.css static
> # bundle.
> TemplateHook(self, 'base-css', 'mozreview/review-stylings-css.html',
> - apply_to=review_request_url_names)
> + apply_to=review_request_url_names + [
> + 'file-attachment',
Use `file_attachment_url_names` here. Or, even better, since I think `MRReviewFlagExtension` should be using `review_request_url_names + file_attachment_url_names`, create a new variable containing both those and use that in both places.
::: pylib/mozreview/mozreview/static/mozreview/js/review.js:36
(Diff revision 31)
> + reviewButton.removeClass('file-review')
> + .addClass('description-review')
> + .text('Review')
> + .appendTo(descriptionLabel);
> + if (attachments.length === 1) {
> + $('#file-list').parent().hide();
So this causes a momentary flicker, which is kind of annoying. I think we should do the complement of what you have here, which we do in other places: in CSS, hide `#file-list` by default, and then here unhide it if `attachments.length > 1`.
::: pylib/mozreview/mozreview/static/mozreview/js/review_flag.js
(Diff revision 31)
> this.model.get('extraData')[this.key] = val;
> this.save()
> }
> });
>
> -
Leave this blank line in (particularly since you didn't touch this file elsewhere, but also because I believe we keep two lines between JavaScript "class" definitions as well.
::: pylib/rbbz/rbbz/diffs.py:352
(Diff revision 31)
> + first_line = begin_line_num
> + num_lines = end_line_num - begin_line_num + 1
> +
> + caption = comment.file_attachment.caption
> + # This is the base comment of a review, so we
> + # should quote the code the comment is adressing.
Nit: "addressing"
::: pylib/rbbz/rbbz/diffs.py:364
(Diff revision 31)
> + for index in range(len(lines)):
> + if index >= first_line_index and index <= last_line_index:
> + prefix = '> '
> + ret.append('%s%s' % (prefix, lines[index]))
Here's a simpler way to do this:
ret += ['> %s' % line for line in lines[first_line_index:last_line_index+1]]
Comment 89•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review61980
Oh you'll need to rebase this as well, since rbbz went away.
Assignee | ||
Comment 90•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review61954
> So this causes a momentary flicker, which is kind of annoying. I think we should do the complement of what you have here, which we do in other places: in CSS, hide `#file-list` by default, and then here unhide it if `attachments.length > 1`.
That was slightly more complicated as we need to hide `#file-list`'s parent. I'm hiding `#review_request_extra div.clearfix`. Also, because it's been shown before by inline CSS (using JS I suppose) I need to remove this class to show the div.
Assignee | ||
Comment 91•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/31-32/
Attachment #8752153 -
Flags: review- → review?(mcote)
Updated•8 years ago
|
Attachment #8752153 -
Flags: review?(mcote) → review+
Comment 92•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review62248
I *think* this is okay now. :) I was just testing out what happens if you manually attach more files, and there was some strange behaviour. However, that might have nothing to do with your patch; I went back to the parent of your commit and the behaviour continued.
You will still need to rebase your commit on @. Please give it a quick test after you do, including adding another file attachment manually. Then you can push it up. Flag me for review; I'll just give it a quick look and then autoland it.
Other than that, just two very tiny nits.
::: pylib/mozreview/mozreview/static/mozreview/css/review.less:153
(Diff revisions 31 - 32)
> padding-left: 10px;
> padding-right: 10px;
> font-size: 10pt;
> }
>
> +
Just one blank line needed here.
::: pylib/mozreview/mozreview/static/mozreview/css/review.less:162
(Diff revisions 31 - 32)
> +}
> +#review_request_extra div.more_attachments {
> + display: block;
> +}
> +
> +
Just one blank line needed here.
Assignee | ||
Comment 93•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/32-33/
Assignee | ||
Comment 94•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/33-34/
Comment 95•8 years ago
|
||
https://reviewboard.mozilla.org/r/52441/#review63076
All tests pass for me. \o/
Giving it to smacleod for another look.
Comment 96•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
Bug 1291732 prevented me from flagging you via Review Board.
Attachment #8752153 -
Flags: review?(smacleod)
Comment 97•8 years ago
|
||
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review66746
::: hgext/reviewboard/tests/test-commits-message-attachment.t:1
(Diff revision 34)
> +#require mozreviewdocker
This isn't testing for any of the cross-posting to bugzilla, that should be tested as well.
::: pylib/mozreview/mozreview/diffs.py:6
(Diff revision 34)
> +import logging
> +logger = logging.getLogger(__name__)
This is unused, please make sure your code additions pass cleanly under pyflakes.
::: pylib/mozreview/mozreview/diffs.py:327
(Diff revision 34)
> lines.append(u"\n%s" % comment)
>
> return u"\n".join(lines)
>
>
> +def render_plain_fileattachment_comment(comment):
You're not dealing with replies at all, we should follow the pattern used by diff comments.
::: pylib/mozreview/mozreview/diffs.py:327
(Diff revision 34)
> +def render_plain_fileattachment_comment(comment):
> + """Render a comment on an attached file"""
So currently this has nothing to do with "diffs" like the name of the file would indicate... we might want to consider moving this, or giving things a rename to indicate the actual overall purpose.
::: pylib/mozreview/mozreview/diffs.py:339
(Diff revision 34)
> + if begin_line_num == end_line_num:
> + if begin_line_num < 6:
> + first_line = 1
> + num_lines = end_line_num
> + else:
> + first_line = begin_line_num - 5
> + num_lines = 6
> + else:
> + first_line = begin_line_num
> + num_lines = end_line_num - begin_line_num + 1
```Python
if begin_line_num == end_line_num:
first_line = max(1, begin_line_num - 5)
else:
first_line = begin_line_num
num_lines = end_line_num - first_line + 1
```
is a bit cleaner IMO. That being said, does 5 lines of context *really* make sense for the commit message? you're generally going to be dealing with a much smaller file than source code, and the vast majority will be english, not code. We should consider reducing the lines, or even not including the context.
::: pylib/mozreview/mozreview/diffs.py:351
(Diff revision 34)
> + # This is the base comment of a review, so we
> + # should quote the code the comment is addressing.
We're not quoting code
::: pylib/mozreview/mozreview/diffs.py:354
(Diff revision 34)
> + att_f = comment.file_attachment.file
> + att_f.open()
> + lines = list(att_f)
> +
> + att_f.close()
We should use RBs builtin mechanism for grabbing the file contents (it deals with caching etc).
You'll want to go through the Review UI (which generally deals with grabbing the relavant fragment of the file for a comment, but does it in a rendered way we can't use).
Try something like `comment.review_ui.get_text()` which should return the file contents from the cache, or generate it, as a string.
::: pylib/mozreview/mozreview/diffs.py:360
(Diff revision 34)
> + att_f.open()
> + lines = list(att_f)
> +
> + att_f.close()
> +
> + # adding a ">" sign to commented lines
This block below does more than just this, it's creating the whole rendering of the comment and the quoted section of the attachment.
I think something like "Blockquote the commented lines." is more descriptive of what we're doing as well.
::: pylib/mozreview/mozreview/diffs.py:363
(Diff revision 34)
> + ret = ['Comment on %s:\n\n' % caption]
> + ret += ['> %s' % line for line in lines[first_line_index:last_line_index + 1]]
> + code = u"".join(ret)
> + return u"%s\n%s" % (code, comment.text)
I don't think we want a blank space between the file the comment's on, and the quoting of the file.
::: pylib/mozreview/mozreview/extension.py:121
(Diff revision 34)
> +# These url names are used to set file_attachment_review JS
> +file_attachment_url_names = [
> + 'file-attachment',
> + 'screenshot',
> +]
> +rr_and_attachment_url_names = review_request_url_names + file_attachment_url_names
I think a more future proof solution would be to use `reviewboard.urls.reviewable_url_names`. This'll make sure we get any future areas which will allow popping up the review dialog etc.
Although, this doesn't include `'review-request-detail'`... How about we define our own constant as `rr_and_reviewable_url_names = reviewable_url_names + ['review-request-detail']`
::: pylib/mozreview/mozreview/extension.py:185
(Diff revision 34)
> 'mozreview/js/ui.mozreviewautocomplete.js',
> 'mozreview/js/review_flag.js',
> ],
> 'apply_to': review_request_url_names,
> },
> + # As HTML is different there is a need to initiate less
Differant than what? in what way?
Is there a real perf gain from splitting things up like this into multiple bundles? you're including the same js file into multiple minified bundles now, which is a separate resource the client needs to pull down and cache.
::: pylib/mozreview/mozreview/extension.py:310
(Diff revision 34)
> + TemplateHook(self, 'file_attachment_review',
> + 'mozreview/file_attachment_review-js.html',
> + apply_to=file_attachment_url_names)
This isn't actually doing anything, the first argument here is supposed to be the hookpoint where this will execute, but `file_attachment_review` isn't a real hookpoint. Your js gets included just by having the `apply_to` key in the bundle definition. The reason we haven't done that for CSS is historical, and something that should really change.
Also, `file_attachment_review-js.html` doesn't even exist.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:927
(Diff revision 34)
>
> draft.summary = commit['message'].splitlines()[0]
> draft.description = commit['message']
> draft.bugs_closed = commit['bug']
>
> + # Creating/updating a new commit message attachment started
Comments like this should be ended with a period.
I think something like "Create/update the commit message attachment." is more in line with the current style.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:929
(Diff revision 34)
> draft.description = commit['message']
> draft.bugs_closed = commit['bug']
>
> + # Creating/updating a new commit message attachment started
> +
> + # Prepare attachment file
period.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:931
(Diff revision 34)
>
> + # Creating/updating a new commit message attachment started
> +
> + # Prepare attachment file
> + # cm_* means commit_message_*
> + cm_temp = NamedTemporaryFile(delete=True)
Please don't create a temporary file for the contents - move directly from the string to the format django requires by using `ContentFile`
::: pylib/mozreview/mozreview/resources/batch_review_request.py:935
(Diff revision 34)
> + cm_caption = 'Commit message'
> + comment_atts = draft.file_attachments.filter(caption=cm_caption)
I really don't think we should rely on the caption being equal for identifying our specially handled commit message file. We should store information about which id corresponds to our special file inside of our CommitData table.
We should also use that stored data to prevent the author or anyone else from updating / deleting the special commit message attachment, other than our special batch endpoint. This will be important as we start to integrate the attachment review better, possibly adding custom UI and hiding the normal UI, etc.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:936
(Diff revision 34)
> + cm_temp = NamedTemporaryFile(delete=True)
> + cm_temp.write(draft.description)
> + cm_temp.flush()
> + cm_file = File(cm_temp)
> + cm_caption = 'Commit message'
> + comment_atts = draft.file_attachments.filter(caption=cm_caption)
`comment_atts` or `commit_message_atts`?
::: pylib/mozreview/mozreview/resources/batch_review_request.py:939
(Diff revision 34)
> + cm_file = File(cm_temp)
> + cm_caption = 'Commit message'
> + comment_atts = draft.file_attachments.filter(caption=cm_caption)
> +
> + if not comment_atts:
> + # There is no comment attachment in review request.
"comment attachment"? "commit message attachment"?
::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:1
(Diff revision 34)
> +// A hack to allow review_flag.js work without MozReview object
> +
> +var MozReview = window.MozReview || { isParent: false };
> +
> +$(document).ready(function() {
> + $(document).trigger("mozreview_ready");
> +});
Why is this hack even a thing? Is there a reason you don't want the MozReview object around? If this is needed it's going to need a good explanation listed.
::: pylib/mozreview/mozreview/static/mozreview/js/review.js:22
(Diff revision 34)
> + // Move commit message file attachment's review button to the right
> + // commit message
> + // hide the file_attachment
I don't really understand this comment.
::: pylib/mozreview/mozreview/static/mozreview/js/review.js:31
(Diff revision 34)
> + reviewButton.removeClass('file-review')
> + .addClass('description-review')
> + .text('Review')
> + .appendTo(descriptionLabel);
> + $(this).hide();
Why exactly are we doing this? It's not clear to me from the screenshots / comments / commit description what the purpose is. Can you elaborate please?
::: pylib/mozreview/mozreview/static/mozreview/js/review.js:39
(Diff revision 34)
> + if (attachments.length > 1) {
> + $('#file-list').parent()
> + .removeClass('clearfix')
> + .addClass('more_atachments');
> + }
This too, I'm unsure what's going on.
Attachment #8752153 -
Flags: review?(smacleod) → review-
Assignee | ||
Comment 98•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review66746
> So currently this has nothing to do with "diffs" like the name of the file would indicate... we might want to consider moving this, or giving things a rename to indicate the actual overall purpose.
There is a follow-up bug to add this (bug 1287736), it includes testing in bugzilla, I think I will just change it to duplicate and make it a part of this patch...
> ```Python
> if begin_line_num == end_line_num:
> first_line = max(1, begin_line_num - 5)
> else:
> first_line = begin_line_num
>
> num_lines = end_line_num - first_line + 1
> ```
>
> is a bit cleaner IMO. That being said, does 5 lines of context *really* make sense for the commit message? you're generally going to be dealing with a much smaller file than source code, and the vast majority will be english, not code. We should consider reducing the lines, or even not including the context.
About the 5 lines around - sure, I was just following the diff rule in code.
> Differant than what? in what way?
>
> Is there a real perf gain from splitting things up like this into multiple bundles? you're including the same js file into multiple minified bundles now, which is a separate resource the client needs to pull down and cache.
The environment (HTML) is different. Reviewing attachment page has no objet refering to the parent review.
> Why is this hack even a thing? Is there a reason you don't want the MozReview object around? If this is needed it's going to need a good explanation listed.
HTML doesn't have parent review object and MozReview is failing to initiate.
More explanation is needed indeed.
> Why exactly are we doing this? It's not clear to me from the screenshots / comments / commit description what the purpose is. Can you elaborate please?
I'm taking Review link of the file attachment and placing it to the right of the original commit message. Then I'm hiding entire file attachment.
> This too, I'm unsure what's going on.
Because at the beginning there is only one file attachment (commit message one), I needed to hide entire File section, not only the attachment. Since it was blinking (showing <h>Files</h> and hiding it afterwards) @mcote suggested to hide it by default and show it if there is more than 1 attachment present. Adding a class 'more_attachments' is the solution.
Assignee | ||
Comment 99•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review66746
> This is unused, please make sure your code additions pass cleanly under pyflakes.
pyflakes actually passed it - I might have some very liberal setting
Comment hidden (mozreview-request) |
Assignee | ||
Comment 101•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review66746
> I think a more future proof solution would be to use `reviewboard.urls.reviewable_url_names`. This'll make sure we get any future areas which will allow popping up the review dialog etc.
>
> Although, this doesn't include `'review-request-detail'`... How about we define our own constant as `rr_and_reviewable_url_names = reviewable_url_names + ['review-request-detail']`
I will need a better explanation of how hooks are working - a diagram or something as long as I will get an answer on this
Assignee | ||
Comment 102•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)
https://reviewboard.mozilla.org/r/52441/#review66746
> I really don't think we should rely on the caption being equal for identifying our specially handled commit message file. We should store information about which id corresponds to our special file inside of our CommitData table.
>
> We should also use that stored data to prevent the author or anyone else from updating / deleting the special commit message attachment, other than our special batch endpoint. This will be important as we start to integrate the attachment review better, possibly adding custom UI and hiding the normal UI, etc.
I've written the file attachment id to the extra_data. No idea how to get it out in front-end yet
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8752153 -
Attachment is obsolete: true
Attachment #8752153 -
Flags: review?(smacleod)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 109•8 years ago
|
||
mozreview-review |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review76290
::: pylib/mozreview/mozreview/resources/batch_review_request.py:914
(Diff revision 5)
> + from reviewboard.diffviewer.diffutils import convert_to_unicode
> + from reviewboard.scmtools.core import PRE_CREATION, UNKNOWN, FileNotFoundError
> + from reviewboard.diffviewer.differ import DiffCompatVersion
> + from django.utils.encoding import smart_unicode
Imports generally belong at the top of the file. I believe the only time we include them in functions is for mach commands, since they are typically run individually.
Assignee | ||
Comment 110•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review76290
> Imports generally belong at the top of the file. I believe the only time we include them in functions is for mach commands, since they are typically run individually.
OK, I've copied most stuff from managers, but I didn't like it as well
Comment 111•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review76290
> OK, I've copied most stuff from managers, but I didn't like it as well
"managers"? Where's that?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 113•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review76290
> "managers"? Where's that?
https://github.com/reviewboard/reviewboard/blob/master/reviewboard/diffviewer/managers.py#L443
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 121•8 years ago
|
||
mozreview-review |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review78004
::: pylib/mozreview/mozreview/resources/batch_review_request.py:1031
(Diff revision 9)
> + commit_message_name,
> + len(commit_message_lines),
> + '%s\n' % '\n'.join(['+%s' % l for l in commit_message_lines]))
> +
> + # Create file FileDiff containing the commit message
> + commit_message_filediff = FileDiff.objects.create(
Unfortunately having filediffs in the right order doesn't help. It's working well for loading filediffs from normal request (i.e. /r/2/diff/1-3/), but order becomes acting different if user moves the slider to change revisions. For now I see new file coming to top.
Sorting has to happen in JS as /api/review-requests/2/diff-context/?revision=1&interdiff-revision=3 provides files in the right order.
The repo I'm checking contains 2 files and 5 revisiona. First file (foo) is added in root commit, the second (bar) in revision 2. If left slider handle is moved to show [0/1/2] bar becomes the first, however if right slider is moved commit-message is at the top. I'm looking now to find the inconsistency of this behaviour.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 123•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review78004
> Unfortunately having filediffs in the right order doesn't help. It's working well for loading filediffs from normal request (i.e. /r/2/diff/1-3/), but order becomes acting different if user moves the slider to change revisions. For now I see new file coming to top.
>
> Sorting has to happen in JS as /api/review-requests/2/diff-context/?revision=1&interdiff-revision=3 provides files in the right order.
>
> The repo I'm checking contains 2 files and 5 revisiona. First file (foo) is added in root commit, the second (bar) in revision 2. If left slider handle is moved to show [0/1/2] bar becomes the first, however if right slider is moved commit-message is at the top. I'm looking now to find the inconsistency of this behaviour.
I've sorted out the order of the file using a little hackery solution (added `data-extra_data` to difffile's table and checking if it contains "is_commit_message_file" string)
There is `table#diff_index` element which has always the order as RB displays it. It's generated with `diffFileIndexView.js` but I don't see any sorting there.
Assignee | ||
Comment 124•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review78004
> I've sorted out the order of the file using a little hackery solution (added `data-extra_data` to difffile's table and checking if it contains "is_commit_message_file" string)
> There is `table#diff_index` element which has always the order as RB displays it. It's generated with `diffFileIndexView.js` but I don't see any sorting there.
BTW the repo structure in testing is generated using these commands:
echo root > foo
hg commit -A -m 'root commit'
hg phase --public -r .
echo foo > foo
hg commit -m 'Bug 1 - Foo 1'
hg push -y
echo foo2 > foo
hg commit --amend -m 'Bug 1 - Foo 2'
hg push -y
echo foo > foo2
hg commit -A --amend -m 'Bug 1 - Foo2 added'
hg push -y
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8792824 -
Attachment is obsolete: true
Attachment #8792824 -
Flags: review?(smacleod)
Assignee | ||
Updated•8 years ago
|
Attachment #8793266 -
Attachment is obsolete: true
Attachment #8793266 -
Flags: review?(smacleod)
Assignee | ||
Updated•8 years ago
|
Attachment #8793267 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8793703 -
Flags: review?(smacleod)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 142•8 years ago
|
||
mozreview-review |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
::: pylib/mozreview/mozreview/resources/batch_review_request.py:914
(Diff revision 18)
> unrecognized.add(reviewer)
>
> return reviewers, unrecognized
>
>
> +def create_diffset_from_data(repository, diff_file_contents, request,
I was really hoping we wouldn't need to copy this code, as it's quite large and complex.
I thought you indicated there was another code location sorting was taking place as well? Meaning having the first id wouldn't be enough. What happened to that case? Were we mistaken and having the lower ID will cover it?
Rather than bringing all this code in I think I'd prefer we hack around it slightly (while not as clean DB wise, code wise it will be much better, IMO).
I propose to get our FileDiff to have a lower id number we create it first. Since a filediff requires a DiffSet though, we can create a phony DiffSet (hell, we could even reuse the same one for each repository). The DiffSet we create won't be tied to a DiffSetHistory, so it won't show up for any review requests.
Then, we create the commit FileDiff using the bogus DiffSet and save it, giving it an id. Then once we've called `create_from_data()` we can switch the DiffSet for our commit FileDiff to the result.
It would look something like:
```Python
temp_diffset = get_temp_diffset()
commit_file_diff = FileDiff.objects.create(
diffset=temp_diffset,
...)
actual_diffset = DiffSet.objects.create_from_data(...)
commit_file_diff.diffset = actual_diffset
commit_file_diff.save()
```
What do you think about this?
::: pylib/mozreview/mozreview/resources/batch_review_request.py:1020
(Diff revision 18)
> + # Prepare commit message data
> + commit_message_name = '000-commit-message-%s' % diffset.base_commit_id[0:5]
> + commit_message_lines = description.split('\n')
> + commit_message_diff = 'diff --git a/%s b/%s\nnew file mode 100644\n--- /dev/null\n+++ b/%s\n@@ -0,0 +1,%s @@\n%s' % (
> + commit_message_name,
> + commit_message_name,
> + commit_message_name,
> + len(commit_message_lines),
> + '%s\n' % '\n'.join(['+%s' % l for l in commit_message_lines]))
This method is extremely complex making it very hard to review, especially not knowing what has changed from when it was pulled it. If the alternative solution I've proposed doesn't work I'd much prefer this be pulled in as its own commit. Having the commits structured in a way that lets me review specific pieces you've changed / added will make my life a lot easier.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:1157
(Diff revision 18)
> + commit_data = fetch_commit_data(draft)
> + commit_data.draft_extra_data.update({
> + AUTHOR_KEY: commit['author'],
> + COMMIT_ID_KEY: commit['id'],
> + FIRST_PUBLIC_ANCESTOR_KEY: commit['first_public_ancestor'],
> + COMMIT_MESSAGE_ID_KEY: commit_message_filediff.pk
How will this work for previoius diff revisions? we'll need to store a list or some sort of mapping from diff revision to commit message filediff id.
::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:137
(Diff revision 18)
> + // Place the container at the top or right after the highlighting DIV.
> + if (commitMessageFileContainer && !commitMessageFileContainer.is(':first-child')) {
> + if ($('.diff-highlight').length > 0) {
> + var highlight = $('.diff-highlight');
> + if (!commitMessageFileContainer.is(':nth-child(2)')) {
> + highlight.after(commitMessageFileContainer)
> + }
> + } else {
> + diffs.prepend(commitMessageFileContainer);
> + }
> + }
> + });
Why isn't it the first entry? What is RB doing that causes this and where is it happening?
::: pylib/mozreview/mozreview/templatetags/mozreview.py:52
(Diff revision 18)
> +@register.filter()
> +def commit_message_id(review_request_details):
> + """Return the id of a commit message file"""
> + commit_data = fetch_commit_data(review_request_details)
> + return str(commit_data.get_for(review_request_details, COMMIT_MESSAGE_ID_KEY))
> +
> +@register.filter()
> +def has_commit_message_file(review_request_details):
> + """Return 'has_commit_message_file' if commit message file has been created
> + for the review request."""
> + commit_data = fetch_commit_data(review_request_details)
> + if commit_data.get_for(review_request_details, COMMIT_MESSAGE_ID_KEY):
> + return 'has_commit_message_file'
> + return ''
These don't appear to be used?
Attachment #8786836 -
Flags: review?(smacleod) → review-
Comment 143•8 years ago
|
||
mozreview-review |
Comment on attachment 8793703 [details]
MozReview: provide filediff's extra_data as data fields (Bug 1248008)
https://reviewboard.mozilla.org/r/80392/#review83036
::: reviewboard/reviewboard/templates/diffviewer/diff_file_fragment_mozreview.html:53
(Diff revision 3)
> - data-lines-equal="{{equal_lines}}">
> + data-lines-equal="{{equal_lines}}"
> + {% for k, v in file.filediff.extra_data.items %}
> + data-extra-{{ k }}="{{ v }}"
> + {% endfor %}>
We should have access to the specific filediff id of the file we're looking for, so I don't think it's needed that we spit out the extra data for everything (especially since Review Board stores a bunch of data in here).
We should include the needed information in our own template of some sort (from the extension) that the javascript can make use of.
Attachment #8793703 -
Flags: review?(smacleod) → review-
Comment 144•8 years ago
|
||
mozreview-review |
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action (Bug 1248008)
https://reviewboard.mozilla.org/r/78616/#review83034
::: reviewboard/reviewboard/static/rb/js/pages/views/diffViewerPageView_mozreview.js:405
(Diff revision 3)
> if ($anchor.length !== 0) {
> this.selectAnchor($anchor);
> this._startAtAnchorName = null;
> }
> }
>
(Bogus line for general issue)
Please include a justification for this event in the commit message. Part of me thinks we should be attacking the sorting of the files, rather than trying to move things after render (to avoid jank, and make sure paging won't screw us up). Is there a reason why we can't do that?
::: reviewboard/reviewboard/static/rb/js/pages/views/diffViewerPageView_mozreview.js:406
(Diff revision 3)
> + // Emit a fileDiffRenderedEvent
> + this.trigger('fileDiffRendered', diffReviewable.get('fileDiffID'));
I wonder if we should namespace stuff like this so we know it's coming from review board...
Attachment #8791064 -
Flags: review?(smacleod) → review-
Comment 145•8 years ago
|
||
mozreview-review |
Comment on attachment 8793700 [details]
MozReview: replace diff_file_fragment (Bug 1248008)
https://reviewboard.mozilla.org/r/80390/#review83038
Clearing until other commits addressed.
Attachment #8793700 -
Flags: review?(smacleod)
Comment 146•8 years ago
|
||
mozreview-review |
Comment on attachment 8791063 [details]
MozReview: Add diffViewerPageView_mozreview.js to differentiate commit message file from others (Bug 1248008)
https://reviewboard.mozilla.org/r/78614/#review83040
Clearing until other commits addressed.
Attachment #8791063 -
Flags: review?(smacleod)
Assignee | ||
Comment 147•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action (Bug 1248008)
https://reviewboard.mozilla.org/r/78616/#review83034
> I wonder if we should namespace stuff like this so we know it's coming from review board...
++ adding this
Assignee | ||
Comment 148•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8793703 [details]
MozReview: provide filediff's extra_data as data fields (Bug 1248008)
https://reviewboard.mozilla.org/r/80392/#review83036
> We should have access to the specific filediff id of the file we're looking for, so I don't think it's needed that we spit out the extra data for everything (especially since Review Board stores a bunch of data in here).
>
> We should include the needed information in our own template of some sort (from the extension) that the javascript can make use of.
That would require us storing somewhere an array of commit message filediff id per revision.
Assignee | ||
Comment 149•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> I was really hoping we wouldn't need to copy this code, as it's quite large and complex.
>
> I thought you indicated there was another code location sorting was taking place as well? Meaning having the first id wouldn't be enough. What happened to that case? Were we mistaken and having the lower ID will cover it?
>
> Rather than bringing all this code in I think I'd prefer we hack around it slightly (while not as clean DB wise, code wise it will be much better, IMO).
>
> I propose to get our FileDiff to have a lower id number we create it first. Since a filediff requires a DiffSet though, we can create a phony DiffSet (hell, we could even reuse the same one for each repository). The DiffSet we create won't be tied to a DiffSetHistory, so it won't show up for any review requests.
>
> Then, we create the commit FileDiff using the bogus DiffSet and save it, giving it an id. Then once we've called `create_from_data()` we can switch the DiffSet for our commit FileDiff to the result.
>
> It would look something like:
> ```Python
> temp_diffset = get_temp_diffset()
> commit_file_diff = FileDiff.objects.create(
> diffset=temp_diffset,
> ...)
>
>
> actual_diffset = DiffSet.objects.create_from_data(...)
> commit_file_diff.diffset = actual_diffset
> commit_file_diff.save()
> ```
>
> What do you think about this?
Wouldn't it create commit message filefidd as the last file added to the set? Definitely worth a try.
Assignee | ||
Comment 150•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> Why isn't it the first entry? What is RB doing that causes this and where is it happening?
If filediff list is loaded from address, first DIV becomes a highlighting DIV. If it is loaded after moving revisions slider highlight DIV is destroyed and the first filediff becomes the first DIV.
Assignee | ||
Comment 151•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> These don't appear to be used?
Yes, thanks.
Comment 153•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action (Bug 1248008)
https://reviewboard.mozilla.org/r/78616/#review83034
> ++ adding this
i've used a "mr:" prefix in bug 1253552.
Assignee | ||
Comment 154•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> Wouldn't it create commit message filefidd as the last file added to the set? Definitely worth a try.
Seems fine - the only change in test is the id number of the diffset
Assignee | ||
Comment 155•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> How will this work for previoius diff revisions? we'll need to store a list or some sort of mapping from diff revision to commit message filediff id.
Previous commit_data will simply not have this. We're currently not using this field, but it might be useful if we will change the method we pick the commit message filediff DOM element. We need to decide if this should happen now (this is possible, there is a need to change template to store the array and listen to review slider's event to find the right pk). This however is only a way to find the element from the same data we've got now.
As of the list - we might store it per review request or generate at will. It's important to store the commit message filediff pk within commit data.
Assignee | ||
Comment 156•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> Seems fine - the only change in test is the id number of the diffset
There is one test which is not working with that method - test-repo-discovery.t
I've got that error:
```
$ mozreview exec hgrb /var/hg/venv_pash/bin/hg -R /repo/hg/mozilla/autoreview createrepomanifest ${MERCURIAL_URL} ssh://bad.host/
- 36d47cf7c6cbd312de3aeb3b2f770c650b014053 http://$DOCKER_HOSTNAME:$HGPORT/b ssh://bad.host/b
- 4016108b6e06add4c0ddde40dee8c7b9aa410f58 http://$DOCKER_HOSTNAME:$HGPORT/a ssh://bad.host/a
+ abort: repository / not found!
+ [255]
```
The code I'm using:
```python
def get_temp_repository():
try:
repository = Repository.objects.get(name='temp repository')
except Repository.DoesNotExist:
repository = Repository.objects.create(
name='temp repository',
path='/',
tool=Tool.objects.get(name='Mercurial'))
return repository
def get_temp_diffset():
"""Get or create a temporary diffset."""
try:
diffset = DiffSet.objects.get(name='temp diffset')
except DiffSet.DoesNotExist:
diffset = DiffSet.objects.create(
name='temp diffset',
revision=0,
repository=get_temp_repository()
)
return diffset
# ... and later in update_review_request
commit_message_filediff = FileDiff.objects.create(
diffset=get_temp_diffset(),
source_file=commit_message_name,
dest_file=commit_message_name,
source_revision=PRE_CREATION,
dest_detail='parent_diff',
parent_diff='',
binary=False,
status='M',
diff=commit_message_diff)
```
Comment 157•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> There is one test which is not working with that method - test-repo-discovery.t
> I've got that error:
> ```
> $ mozreview exec hgrb /var/hg/venv_pash/bin/hg -R /repo/hg/mozilla/autoreview createrepomanifest ${MERCURIAL_URL} ssh://bad.host/
> - 36d47cf7c6cbd312de3aeb3b2f770c650b014053 http://$DOCKER_HOSTNAME:$HGPORT/b ssh://bad.host/b
> - 4016108b6e06add4c0ddde40dee8c7b9aa410f58 http://$DOCKER_HOSTNAME:$HGPORT/a ssh://bad.host/a
> + abort: repository / not found!
> + [255]
> ```
>
> The code I'm using:
> ```python
> def get_temp_repository():
> try:
> repository = Repository.objects.get(name='temp repository')
> except Repository.DoesNotExist:
> repository = Repository.objects.create(
> name='temp repository',
> path='/',
> tool=Tool.objects.get(name='Mercurial'))
> return repository
>
>
> def get_temp_diffset():
> """Get or create a temporary diffset."""
> try:
> diffset = DiffSet.objects.get(name='temp diffset')
> except DiffSet.DoesNotExist:
> diffset = DiffSet.objects.create(
> name='temp diffset',
> revision=0,
> repository=get_temp_repository()
> )
> return diffset
>
> # ... and later in update_review_request
> commit_message_filediff = FileDiff.objects.create(
> diffset=get_temp_diffset(),
> source_file=commit_message_name,
> dest_file=commit_message_name,
> source_revision=PRE_CREATION,
> dest_detail='parent_diff',
> parent_diff='',
> binary=False,
> status='M',
> diff=commit_message_diff)
> ```
No need for a temp repository, just assing the temp diffset to the actual repository for the review request. we'll end up with one temp diffset per repo but that's not a big deal.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8793700 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8793703 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 165•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> If filediff list is loaded from address, first DIV becomes a highlighting DIV. If it is loaded after moving revisions slider highlight DIV is destroyed and the first filediff becomes the first DIV.
Ya, but we discussed ways to handle this with CSS, why are you needing to add a class instead of using the methods we discussed?
Comment 166•8 years ago
|
||
mozreview-review |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
::: pylib/mozreview/mozreview/resources/batch_review_request.py:918
(Diff revisions 18 - 21)
>
>
> -def create_diffset_from_data(repository, diff_file_contents, request,
> - base_commit_id, description):
> - """Create a DiffSet from raw diff data
> -
> +def get_temp_diffset(repository):
> + """Get or create a temporary diffset."""
> + try:
> + diffset = DiffSet.objects.get(name='temp diffset', repository__pk=repository.pk)
I'm not 100% sure on how RB uses the diffset name, it also doesn't have a database index, so I don't think that's what we should rely on to find the temp diffset (this could perform really poorly with the large production DB).
Lets store the ID number of our temp diffset somewhere and use that instead.
::: pylib/mozreview/mozreview/extension.py:302
(Diff revision 21)
> + TemplateHook(self, 'base-scripts-post', 'mozreview/viewdiff-commit-message-file-js.html',
> + apply_to=diffviewer_url_names)
JS should be included as part of a bundle (see how that's done above) - that being said though, this should just follow the conventions of other things and be added to the markup as a data attribute, not a script tag.
::: pylib/mozreview/mozreview/extra_data.py:36
(Diff revision 21)
> FIRST_PUBLIC_ANCESTOR_KEY = MOZREVIEW_KEY + '.first_public_ancestor'
> IDENTIFIER_KEY = MOZREVIEW_KEY + '.identifier'
> SQUASHED_KEY = MOZREVIEW_KEY + '.is_squashed'
> UNPUBLISHED_KEY = MOZREVIEW_KEY + '.unpublished_rids'
> PUBLISH_AS_KEY = MOZREVIEW_KEY + '.publish_as'
> +COMMIT_MESSAGE_ID_KEY = MOZREVIEW_KEY + '.commit_message_id'
`COMMIT_MSG_FILEDIFF_IDS_KEY`
::: pylib/mozreview/mozreview/resources/batch_review_request.py:42
(Diff revision 21)
> ReviewRequest,
> ReviewRequestDraft,
> )
> from reviewboard.scmtools.models import (
> Repository,
> + Tool,
You don't appear to use this? please make sure you're running pyflake on your code and not adding any extra errors.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:915
(Diff revision 21)
> +def get_temp_diffset(repository):
> + """Get or create a temporary diffset."""
Please expand the docstring to include a detailed explanation of why this is needed.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:970
(Diff revision 21)
> + # get temp diffset
> + temp_diffset = get_temp_diffset(rr.repository)
> + # Prepare commit message data
> + base_commit_id = commit.get('base_commit_id')
> + commit_message_name = '000-commit-message-%s' % base_commit_id[0:5]
> + commit_message_lines = draft.description.split('\n')
You should take this from the commit structure passed in, not the draft description (we may change how we use description after this case, it might not always be the commit message).
::: pylib/mozreview/mozreview/resources/batch_review_request.py:971
(Diff revision 21)
> + temp_diffset = get_temp_diffset(rr.repository)
> + # Prepare commit message data
> + base_commit_id = commit.get('base_commit_id')
> + commit_message_name = '000-commit-message-%s' % base_commit_id[0:5]
> + commit_message_lines = draft.description.split('\n')
> + commit_message_diff = 'diff --git a/%s b/%s\nnew file mode 100644\n--- /dev/null\n+++ b/%s\n@@ -0,0 +1,%s @@\n%s' % (
Can you define this as a constant multiline string (with named format identifiers) at the top of the file. ex:
```Python
COMMIT_MSG_DIFF_FORMAT='''
diff --git a/
...
...
...
'''.lsplit()
```
::: pylib/mozreview/mozreview/resources/batch_review_request.py:978
(Diff revision 21)
> + commit_message_name,
> + commit_message_name,
> + len(commit_message_lines),
> + '%s\n' % '\n'.join(['+%s' % l for l in commit_message_lines]))
> +
> + # create commit message filediff using temp diffset
Please document that this *must* come before the full diffset creation for the id ordering.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:984
(Diff revision 21)
> + commit_message_filediff = FileDiff.objects.create(
> + diffset=temp_diffset,
> + source_file=commit_message_name,
> + dest_file=commit_message_name,
> + source_revision=PRE_CREATION,
> + dest_detail='parent_diff',
Why are you setting this to parent_diff? this isn't a parent_diff at all, and this field is described as "destination file details"
::: pylib/mozreview/mozreview/resources/batch_review_request.py:1014
(Diff revision 21)
> - base_commit_id=commit.get('base_commit_id'),
> + base_commit_id=base_commit_id,
> save=True,
> )
> +
> update_diffset_history(rr, diffset)
> + # diffset.extra_data['commit-message-id'] = commit_message_filediff.pk
Leftover?
::: pylib/mozreview/mozreview/resources/batch_review_request.py:1023
(Diff revision 21)
> + commit_message_filediff.diffset = diffset
> + commit_message_filediff.save()
This could probably use a comment expalining how we're switching it over from the temporary now that the actual diffset is created.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:1027
(Diff revision 21)
> + commit_message_dict = json.loads(commit_data.draft_extra_data.get(COMMIT_MESSAGE_ID_KEY, '{}'))
> + commit_message_dict[str(diffset.revision)] = commit_message_filediff.pk
I'd say this variable name is misleading.
::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:116
(Diff revision 21)
> + var diffs = $('#diffs');
> + var currentCommitMessageIds = [-1, commitMessageIds[currentRevisionNumber]];
> + page.on('mr:revisionSelected', function(revisions) {
> + // We never know which one will be used to display the commit
> + // message filediff...
> + currentCommitMessageIds = [commitMessageIds[revisions[0]], commitMessageIds[revisions[1]]];
> + });
> + var commitMessageFileContainer;
> + var diffFileInfoAnchorTr;
> + page.on('mr:fileDiffRendered', function(fileDiffID) {
> + if (currentCommitMessageIds.indexOf(fileDiffID) >= 0) {
> + // Add class to the TABLE and select its container.
> + var commitMessageFile = $('#file' + fileDiffID).addClass('commit-message-file');
> + commitMessageFileContainer = commitMessageFile.parents('.diff-container');
> + // remove 000 and hashtag from commit message file name
> + var fileNameRow = commitMessageFile.find('.filename-row th');
> + var fileNameRowText = fileNameRow.text().trim();
> + var fileNameRowTextArr = fileNameRowText.split('-');
> + var newFileNameRowText = fileNameRowTextArr.slice(1, fileNameRowTextArr.length - 1).join('-');
> + fileNameRow.contents().last()[0].textContent = newFileNameRowText;
> + var diffFileInfoAnchor = $('.diff-file-info a:contains(' + fileNameRowText + ')')
> + diffFileInfoAnchorTr = diffFileInfoAnchor.parent().parent();
> + diffFileInfoAnchor.text(newFileNameRowText);
> + }
> + if (diffFileInfoAnchorTr && !diffFileInfoAnchorTr.is(':first-child')) {
> + diffFileInfoAnchorTr.parent().prepend(diffFileInfoAnchorTr);
> + }
> + // Place the container at the top or right after the highlighting DIV.
> + if (commitMessageFileContainer && !commitMessageFileContainer.is(':first-child')) {
> + if ($('.diff-highlight').length > 0) {
> + var highlight = $('.diff-highlight');
> + if (!commitMessageFileContainer.is(':nth-child(2)')) {
> + highlight.after(commitMessageFileContainer)
> + }
> + } else {
> + diffs.prepend(commitMessageFileContainer);
> + }
> + }
> + });
I still haven't been convinced any of this is needed? Please either fully document why our commit message would be sorted out of order (the highlight element can be dealt with by css). Or switch to the css method.
::: pylib/mozreview/mozreview/templatetags/mozreview.py:52
(Diff revision 21)
> """Return the commit id of a review request or review request draft"""
> commit_data = fetch_commit_data(review_request_details)
> return str(commit_data.get_for(review_request_details, COMMIT_ID_KEY))
>
> +@register.filter()
> +def commit_message_ids(review_request_details):
Very misleading function name.
::: pylib/mozreview/mozreview/templatetags/mozreview.py:55
(Diff revision 21)
> + commit_message_ids = commit_data.get_for(review_request_details, COMMIT_MESSAGE_ID_KEY)
> + if not commit_message_ids:
> + return '[]'
`comit_data.get_for()` takes a `default=` kwarg you can use instead of the conditional here.
Attachment #8786836 -
Flags: review?(smacleod) → review-
Comment 167•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action (Bug 1248008)
https://reviewboard.mozilla.org/r/78616/#review83034
> (Bogus line for general issue)
>
> Please include a justification for this event in the commit message. Part of me thinks we should be attacking the sorting of the files, rather than trying to move things after render (to avoid jank, and make sure paging won't screw us up). Is there a reason why we can't do that?
You explain what you're doing in the commit message now, but the justification "There is a need to create an event when user switches revisions as different
fildiff might be loaded" doesn't make sense to me. Why would there be a different filediff loaded first? what cases does this happen in?
Comment 168•8 years ago
|
||
mozreview-review |
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action (Bug 1248008)
https://reviewboard.mozilla.org/r/78616/#review88504
r- until comment on previous review is addressed again.
Attachment #8791064 -
Flags: review?(smacleod) → review-
Comment 169•8 years ago
|
||
mozreview-review |
Comment on attachment 8791063 [details]
MozReview: Add diffViewerPageView_mozreview.js to differentiate commit message file from others (Bug 1248008)
https://reviewboard.mozilla.org/r/78614/#review88506
Clearing flag until next update.
Attachment #8791063 -
Flags: review?(smacleod)
Assignee | ||
Comment 170•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> Ya, but we discussed ways to handle this with CSS, why are you needing to add a class instead of using the methods we discussed?
CSS would require to redefine all designs changed for the commit message filediff. If we would declare that a title is italic we would need to redeclare non-italic for others. Which is problematic and would need to be changed every time diff style has changed.
Assignee | ||
Comment 171•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> CSS would require to redefine all designs changed for the commit message filediff. If we would declare that a title is italic we would need to redeclare non-italic for others. Which is problematic and would need to be changed every time diff style has changed.
This is the good CSS solution - https://jsfiddle.net/wwu1uska/15/
To use it we have to always have a commit message filediff (it needs to be created for old diffsets on read or migrate database).
Assignee | ||
Comment 172•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Why are you setting this to parent_diff? this isn't a parent_diff at all, and this field is described as "destination file details"
I've printed out other filediffs from the diffset and blindly repeated what was there.
I've now found in reviewboard that it is a destination revision (and source_revision is a source revision, PRE_CREATION only for the first time) used later in diff:
# ...
source_revision='6bba278',
dest_detail='465d217',
diff=(
b'diff --git a/diffutils.py b/diffutils.py\n'
b'index 6bba278..465d217 100644\n'
# ...
To make it properly I'll need to find latest revision with commit message file and change accordingly. Is that right?
Assignee | ||
Comment 173•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Please document that this *must* come before the full diffset creation for the id ordering.
Do you remember why we had to make it as a first one?
I'm not so sure we still need to make it as a first file. We do know it's ordered alphabetically. Maybe it is not needed in the end.
I'll check it after finishing other issues.
Assignee | ||
Comment 174•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> I'd say this variable name is misleading.
Changing to commit_message_ids
Assignee | ||
Comment 175•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Very misleading function name.
I don't know why it's misleading - it actually returns a list of commit message filediff ids...
I've changed it to commit_message_filediff_ids for now.
Assignee | ||
Comment 176•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> This is the good CSS solution - https://jsfiddle.net/wwu1uska/15/
> To use it we have to always have a commit message filediff (it needs to be created for old diffsets on read or migrate database).
Instead of forcing the commit message creation, a class might be added to the body tag - hasCommitMsgFilediff
Assignee | ||
Comment 177•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> JS should be included as part of a bundle (see how that's done above) - that being said though, this should just follow the conventions of other things and be added to the markup as a data attribute, not a script tag.
Changed to a data attribute.
Assignee | ||
Comment 178•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> I still haven't been convinced any of this is needed? Please either fully document why our commit message would be sorted out of order (the highlight element can be dealt with by css). Or switch to the css method.
Reordering seems to happen somewhere in JS - https://www.dropbox.com/s/xu1avtvkn36e86s/commit-message-default-ordering.mov?dl=0
Comment 179•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> Instead of forcing the commit message creation, a class might be added to the body tag - hasCommitMsgFilediff
We don't want to create filediffs going backwards. We can either use the class method you describe, or stick the css in it's own bundle which we optionally include in a templatehook. Either should be fine. As for the name, please use dash seperated lowercase (or underscores, we mix both :/) rather than camel case.
Comment 180•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Do you remember why we had to make it as a first one?
> I'm not so sure we still need to make it as a first file. We do know it's ordered alphabetically. Maybe it is not needed in the end.
> I'll check it after finishing other issues.
Ordering is not done alphabetically, it's done by primary key order - except in the case of header vs source files of the same name. This is why we need to create it first.
> I've printed out other filediffs from the diffset and blindly repeated what was there.
>
> I've now found in reviewboard that it is a destination revision (and source_revision is a source revision, PRE_CREATION only for the first time) used later in diff:
> # ...
> source_revision='6bba278',
> dest_detail='465d217',
> diff=(
> b'diff --git a/diffutils.py b/diffutils.py\n'
> b'index 6bba278..465d217 100644\n'
> # ...
>
> To make it properly I'll need to find latest revision with commit message file and change accordingly. Is that right?
Since the commit message file is considered a new file, I believe what you want here is probably the value of PRE_CREATION. Please look into what RB does here though.
> Changing to commit_message_ids
please include `filediff` in the name.
> Reordering seems to happen somewhere in JS - https://www.dropbox.com/s/xu1avtvkn36e86s/commit-message-default-ordering.mov?dl=0
Can you investigate where Review Board is doing this, it might be a quick fix either in the fork or by poking at the js initially so that we don't have to run this and move our file everytime. I'd also like to be sure it's not possible our commit message ends up on the second page so a JS fix like this wouldn't catch it.
> I don't know why it's misleading - it actually returns a list of commit message filediff ids...
> I've changed it to commit_message_filediff_ids for now.
It's not clear what the id is for. Is it the sha of a commit that has a commit message we want? is it a filediff? etc. It'll be much easier to understand later if we explicitly call out `filediff` here.
Assignee | ||
Comment 181•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Ordering is not done alphabetically, it's done by primary key order - except in the case of header vs source files of the same name. This is why we need to create it first.
We've changed the name of the file with a 000- prefix to be sure it comes first.
Assignee | ||
Comment 182•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> We've changed the name of the file with a 000- prefix to be sure it comes first.
I'll keep it, just trying to understand where it matters.
Assignee | ||
Comment 183•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> We don't want to create filediffs going backwards. We can either use the class method you describe, or stick the css in it's own bundle which we optionally include in a templatehook. Either should be fine. As for the name, please use dash seperated lowercase (or underscores, we mix both :/) rather than camel case.
className it is then.
Comment 184•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> className it is then.
No, the opposite, I specifically *don't* want camelCase. please use lowercase with dashes seperating the words.
Assignee | ||
Comment 185•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review83032
> No, the opposite, I specifically *don't* want camelCase. please use lowercase with dashes seperating the words.
yes yes - I was referring that the solution would involve setting the class name.
className is the attribute/function which is setting it - I even had with dashes in the code already :)
Assignee | ||
Comment 186•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Since the commit message file is considered a new file, I believe what you want here is probably the value of PRE_CREATION. Please look into what RB does here though.
I've been logging this value and in local repo new files have it empty...
Comment hidden (mozreview-request) |
Assignee | ||
Comment 188•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Can you investigate where Review Board is doing this, it might be a quick fix either in the fork or by poking at the js initially so that we don't have to run this and move our file everytime. I'd also like to be sure it's not possible our commit message ends up on the second page so a JS fix like this wouldn't catch it.
It happens here - https://reviewboard-hg.mozilla.org/reviewboard/file/0b7119c8649a699c739f308cb39b429183734a8c/reviewboard/reviewboard/static/rb/js/pages/models/diffViewerPageModel.js#l49
`attrs.files.models` is ordered as received from server and after set is called `this.attributes.files` is ordered as we see in UI
Comment 189•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> I'll keep it, just trying to understand where it matters.
It all came from the fact when we found that RB was ordering by primary key on initial load (with the exception of swapping header / source files with the same name). This corresponds to the same order the files appear in the generated diff... which I guess is why it does this? We've known about this for many weeks - are you indicating you've found something that makes us incorrect?
> It happens here - https://reviewboard-hg.mozilla.org/reviewboard/file/0b7119c8649a699c739f308cb39b429183734a8c/reviewboard/reviewboard/static/rb/js/pages/models/diffViewerPageModel.js#l49
> `attrs.files.models` is ordered as received from server and after set is called `this.attributes.files` is ordered as we see in UI
So you're saying it's coming from the server sorted differently when it's an API request, rather than the initial rendering? Where in the backend is it doing the sorting?
Assignee | ||
Comment 190•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> So you're saying it's coming from the server sorted differently when it's an API request, rather than the initial rendering? Where in the backend is it doing the sorting?
Server is providing a desired order (alphabetical). In file mentioned above `set` function is called to set the model's `attributes.files` - `this.attributes.files.set(attrs.files.models)`. Unfortunately Backbone Collection's `set` function is not updating objects which already are present and adds the other objects at the end of the set. This changes the order of filediffs.
Comment 191•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Server is providing a desired order (alphabetical). In file mentioned above `set` function is called to set the model's `attributes.files` - `this.attributes.files.set(attrs.files.models)`. Unfortunately Backbone Collection's `set` function is not updating objects which already are present and adds the other objects at the end of the set. This changes the order of filediffs.
To be more precise, The server provides the `FileDiff`s sorted by primary key
in ascending order. This happens to be alphabetical as a coincidence rather
than purposefully (The `FileDiff`s are created in the order they appear in the
diff file itself, which is coming alphabetically from mercurial). The
distinction is quite important as it's why we use the temporary `DiffSet` to
create our commit `FileDiff` first.
With respect to the ordering when updating the slider, you can take a look at
the source code for backbone's set function[1] to see that by default it
performs a merge of the existing models, rather than fulling replacing them
(New items are added, missing items are removed, and existing items are merged).
It appears this merge operation does not preserve the order of the models
which are passed into set (instead the existing items end up before the others).
So, with all that being said, I'd call Review Board's ordering of the
`FileDiff`s after the slider move a bug. This is definitely something we'll need
to fix ourselves as part of this change so it can be deployed ASAP.
Please investigate a solution for fixing this in the fork. My gut tells me we
might just be able to turn off the merge feature of the `set()` by passing
that option in - I'm not sure if that tells it to fully replace the models or
to not bother touching existing ones. Another option would be to actually
perform a sort on the models, using something that will preserve the order we
want (ascending `FileDiff` primary key).
[1] http://backbonejs.org/docs/backbone.html#section-111
Comment 192•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> To be more precise, The server provides the `FileDiff`s sorted by primary key
> in ascending order. This happens to be alphabetical as a coincidence rather
> than purposefully (The `FileDiff`s are created in the order they appear in the
> diff file itself, which is coming alphabetically from mercurial). The
> distinction is quite important as it's why we use the temporary `DiffSet` to
> create our commit `FileDiff` first.
>
> With respect to the ordering when updating the slider, you can take a look at
> the source code for backbone's set function[1] to see that by default it
> performs a merge of the existing models, rather than fulling replacing them
> (New items are added, missing items are removed, and existing items are merged).
> It appears this merge operation does not preserve the order of the models
> which are passed into set (instead the existing items end up before the others).
>
> So, with all that being said, I'd call Review Board's ordering of the
> `FileDiff`s after the slider move a bug. This is definitely something we'll need
> to fix ourselves as part of this change so it can be deployed ASAP.
>
> Please investigate a solution for fixing this in the fork. My gut tells me we
> might just be able to turn off the merge feature of the `set()` by passing
> that option in - I'm not sure if that tells it to fully replace the models or
> to not bother touching existing ones. Another option would be to actually
> perform a sort on the models, using something that will preserve the order we
> want (ascending `FileDiff` primary key).
>
>
> [1] http://backbonejs.org/docs/backbone.html#section-111
Ah, right, the Web API endpoint being used for the no reload diff revision
update *is* sorting alphabetically. I wonder if it's worth "fixing" that
so it's consistent with the full page load behaviour? Either way this
shouldn't affect the fix that's needed for the backbone stuff.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8791063 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 197•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Ah, right, the Web API endpoint being used for the no reload diff revision
> update *is* sorting alphabetically. I wonder if it's worth "fixing" that
> so it's consistent with the full page load behaviour? Either way this
> shouldn't affect the fix that's needed for the backbone stuff.
Ordering is now done in the fork. This file and events are now used only to rename the commit message filename.
If there would be a way to mark the `DiffFile` commit message object, collection could be sorted by that field as well. That would give us an opportunity to remove the prefix from filename and remove this modification (delete events as well).
Comment 198•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Ordering is now done in the fork. This file and events are now used only to rename the commit message filename.
> If there would be a way to mark the `DiffFile` commit message object, collection could be sorted by that field as well. That would give us an opportunity to remove the prefix from filename and remove this modification (delete events as well).
Couldn't we just have it sort by `FileDiff` id as well (which would keep thinks consistent)? I think that solves all the issues.
Assignee | ||
Comment 199•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> It all came from the fact when we found that RB was ordering by primary key on initial load (with the exception of swapping header / source files with the same name). This corresponds to the same order the files appear in the generated diff... which I guess is why it does this? We've known about this for many weeks - are you indicating you've found something that makes us incorrect?
Filename would be irrelevant if order by id would be identical to order by destFilename with the exception of injected commit-msg-filediff as a first element. But it's as we said before and we're keeping the prefix.
Assignee | ||
Comment 200•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action (Bug 1248008)
https://reviewboard.mozilla.org/r/78616/#review83034
> You explain what you're doing in the commit message now, but the justification "There is a need to create an event when user switches revisions as different
> fildiff might be loaded" doesn't make sense to me. Why would there be a different filediff loaded first? what cases does this happen in?
I guess it should read
"different filediff might be loaded for the commit message in chosen revision."
Comment 201•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> I'm not 100% sure on how RB uses the diffset name, it also doesn't have a database index, so I don't think that's what we should rely on to find the temp diffset (this could perform really poorly with the large production DB).
>
> Lets store the ID number of our temp diffset somewhere and use that instead.
Re-opening, you have marked this as fixed but did not fix it.
> `COMMIT_MSG_FILEDIFF_IDS_KEY`
Re-opening, you've added an S, but did not include "FILEDIFF" in the name. Please go with the name I've suggested.
> Couldn't we just have it sort by `FileDiff` id as well (which would keep thinks consistent)? I think that solves all the issues.
re-opening this issue. As I said, we should just be able to make the files sort by ID in the fork, which means we can get rid of the commit message file prefix, as well as the javascript hook you're using to rename it. Please make this change.
Comment 202•8 years ago
|
||
mozreview-review |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
Please ensure you're not closing issues which have not been fixed.
::: hgext/reviewboard/tests/test-commit-message-as-file.t:17
(Diff revision 23)
> + $ echo foo1 > foo.h
> + $ hg commit -m 'Bug 1 - Foo 1'
> + $ hg push
> + pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
> + (adding commit id to 1 changesets)
> + saved backup bundle to $TESTTMP/client/.hg/strip-backup/8d901902ff4b-c4c7c89c-addcommitid.hg (glob)
you have a `(glob)` on this but you're not doing any globbing? Please ensure the identifiers of the strip backup are stable or properly glob the parts that aren't.
::: hgext/reviewboard/tests/test-commit-message-as-file.t:89
(Diff revision 23)
> + - +foo1
> + - ''
> + approved: false
> + approval_failure: A suitable reviewer has not given a "Ship It!"
> +
> +Amended commit message should result with a changed attachment
Changed attachment? You're not looking at attachments after this.
::: hgext/reviewboard/tests/test-commit-message-as-file.t:92
(Diff revision 23)
> + approval_failure: A suitable reviewer has not given a "Ship It!"
> +
> +Amended commit message should result with a changed attachment
> +
> + $ hg commit --amend -m 'Bug 1 - Foo 2 - amended'
> + saved backup bundle to $TESTTMP/client/.hg/strip-backup/4a9cf7e91820-f76b5126-amend-backup.hg (glob)
`(glob)` again.
::: hgext/reviewboard/tests/test-commit-message-as-file.t:96
(Diff revision 23)
> + $ hg commit --amend -m 'Bug 1 - Foo 2 - amended'
> + saved backup bundle to $TESTTMP/client/.hg/strip-backup/4a9cf7e91820-f76b5126-amend-backup.hg (glob)
> + $ hg push
> + pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
> + (adding commit id to 1 changesets)
> + saved backup bundle to $TESTTMP/client/.hg/strip-backup/2358ed0ad9b5-42479a45-addcommitid.hg (glob)
`(glob)` again.
::: hgext/reviewboard/tests/test-commit-message-as-file.t:117
(Diff revision 23)
> + (review requests lack reviewers; visit review url to assign reviewers)
> +
> + publish these review requests now (Yn)? y
> + (published review request 1)
> +
> +Test commit message file creation
Please be more specific, what is this next part of the test actually attempting to verify?
::: pylib/mozreview/mozreview/extension.py:302
(Diff revision 23)
> + TemplateHook(self, 'base-scripts-post', 'mozreview/viewdiff-commit-msg-file-data.html',
> + apply_to=diffviewer_url_names)
You generally use `'base-scripts-post'` for javascript rather than html. `'base-after-content'` is a better location for this.
Also, since we're adding this new template for data related to the diff, it's probably better to give it a generic name like "diff-data.html", to match the "user-data.html" that already exists. This way if we have anymore diff data to inject we have a nice place for it and don't create extra TemplateHooks or changes.
::: pylib/mozreview/mozreview/extra_data.py:36
(Diff revision 23)
> FIRST_PUBLIC_ANCESTOR_KEY = MOZREVIEW_KEY + '.first_public_ancestor'
> IDENTIFIER_KEY = MOZREVIEW_KEY + '.identifier'
> SQUASHED_KEY = MOZREVIEW_KEY + '.is_squashed'
> UNPUBLISHED_KEY = MOZREVIEW_KEY + '.unpublished_rids'
> PUBLISH_AS_KEY = MOZREVIEW_KEY + '.publish_as'
> +COMMIT_MESSAGE_IDS_KEY = MOZREVIEW_KEY + '.commit_message_ids'
I've reopened the original issue for this name, please take a look.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:918
(Diff revision 23)
> + A diffset is needed to create a commit message filediff as the first one
> + of the set. It is returned here and created if none found. Relation to
> + temp diffset is gonna be changed to the right one after its creation."""
```
Creating a FileDiff requires an already existing DiffSet
(There is a non-nullable foreign key). Since we must
create a particular FileDiff before its DiffSet is created
we first create a temporary DiffSet to point at.
Create this temporary DiffSet, or return it if it has been
created in the past."""
```
::: pylib/mozreview/mozreview/resources/batch_review_request.py:922
(Diff revision 23)
> +
> + A diffset is needed to create a commit message filediff as the first one
> + of the set. It is returned here and created if none found. Relation to
> + temp diffset is gonna be changed to the right one after its creation."""
> + try:
> + diffset = DiffSet.objects.get(name='temp diffset', repository__pk=repository.pk)
Please see re-opened issue about this line.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:969
(Diff revision 23)
> + # Commit message filediff needs to be created as a first filediff in a set
> + # Get temp diffset for commit_message_filediff creation.
> + temp_diffset = get_temp_diffset(rr.repository)
> Commit message filediff needs to be created as a first filediff in a set
In the future someone reading this code would want to know *why* not just that it has to be. Please document this in detail.
> Get temp diffset for commit_message_filediff creation.
The reasoning for this should also be rolled into the comment.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:973
(Diff revision 23)
> + base_commit_id = commit.get('base_commit_id')
> + commit_message_name = '000-commit-message-%s' % base_commit_id[0:5]
This is going to change when the commit series has been rebased, which will manifest really strangely in the interdiff (We'll show the previous commit message file as deleted, and a completely new one as added, instead of properly showing the difference). You'll need to use something stable for this commit throughout the entire series.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:982
(Diff revision 23)
> + # Create commit message filediff using temp diffset
> + commit_message_filediff = FileDiff.objects.create(
> + diffset=temp_diffset,
> + source_file=commit_message_name,
> + dest_file=commit_message_name,
> + source_revision=PRE_CREATION,
> + dest_detail='',
> + parent_diff='',
> + binary=False,
> + status='M',
> + diff=commit_message_diff)
Creating the commit message filediff should only be done if this is a completely new commit review request, or if the review request we're updating already had commit filediffs.
We don't want to all of a sudden add commit message files to review requests that didn't start with them.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:1002
(Diff revision 23)
> for user in sorted(reviewer_users):
> draft.target_people.add(user)
> logger.debug('adding reviewer %s to #%d' % (user.username, rr.id))
>
> try:
> + # Create the diffset from raw data
This comment doesn't really add any extra understanding to the code.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:1025
(Diff revision 23)
> + # Assign the commit message filediff to the right diffset.
> + commit_message_filediff.diffset = diffset
> + commit_message_filediff.save()
Something like the following would be much more informative:
```
# Now that the proper DiffSet has been created, re-assign
# the commit message FileDiff we created to the new DiffSet.
```
(Side note, please properly capatalize the python class names when you are referring to them - this should be done throughout the entire patch, not just this line)
::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:92
(Diff revision 23)
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.insert tr td,
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.insert tr td,
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.replace tr td,
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.replace tr td {
> + background-color: #ffdfd7;
> + border-color: #bdd8b7;
> +}
> +
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.insert tr.selected,
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.insert tr.selected,
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.replace tr.selected,
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.replace tr.selected {
> + background-color: #e9bdac;
> +}
> +
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.insert tr th,
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.insert tr th,
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.replace tr th,
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.replace tr th {
> + background-color: #ddb8b7;
Please use the features of less to get rid of all the redundancy here.
::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:92
(Diff revision 23)
> }
> .comment-block-container-draft {
> border-top: 2px dotted #aceb6f; // @comment-flag-draft-color;
> }
> +
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.insert tr td,
Please ensure there is no trailing whitespace. You have it on several of the lines you've added here.
::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:96
(Diff revision 23)
> + background-color: #ffdfd7;
> + border-color: #bdd8b7;
Please include some screenshots of what the new styling looks like in your next update.
::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:124
(Diff revision 23)
> + if (currentRevisionNumber) {
> + var commitMessageIds = commitMessageIdsData.data('commitMessageIds');
> + }
> + if (currentRevisionNumber && commitMessageIds[currentRevisionNumber]) {
> + // Revision has commit message file element. Mark body accordingly.
> + $('body').addClass('has-commit-msg-file');
Will this result in a flicker when the page is loading? You could use a TemplateHook to conditionally load your CSS depending on the request, or two template hooks (one before content, one after) to wrap things in an element conditionally (not sure if this will mess up RBs CSS though).
If this doesn't flicker because of the order the JS runs, I'd prefer we add the class to an element much closer to the FileDiffs, rather than the body.
::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:127
(Diff revision 23)
> + page.on('mr:revisionSelected', function(revisions) {
> + // Revision slider triggers an event giving two revision numbers.
> + // Displayed commit message file will be related to one of them.
> + currentCommitMessageIds = [commitMessageIds[revisions[0]], commitMessageIds[revisions[1]]];
> + });
> +
> + // Rename the commit message filename every time it loads.
> + page.on('mr:fileDiffRendered', function(fileDiffID) {
> + // Check if rendered filediff is a commit message file related to
> + // current revision
> + if (currentCommitMessageIds.indexOf(fileDiffID) >= 0) {
> + var commitMessageFile = $('#file' + fileDiffID);
> + // Remove 000 and the hashtag from commit message file name
> + var fileNameRow = commitMessageFile.find('.filename-row th');
> + var fileNameRowText = fileNameRow.text().trim();
> + var fileNameRowTextArr = fileNameRowText.split('-');
> + var newFileNameRowText = fileNameRowTextArr.slice(1, fileNameRowTextArr.length - 1).join('-');
> + fileNameRow.contents().last()[0].textContent = newFileNameRowText;
> + // Rename commit message filename in the file info list
> + var diffFileInfoAnchor = $('.diff-file-info a:contains(' + fileNameRowText + ')')
> + var diffFileInfoAnchorTr = diffFileInfoAnchor.parent().parent();
> + diffFileInfoAnchor.text(newFileNameRowText);
> + }
> + });
Please see reopened issue in my previous review talking about how we can get rid of these listeners.
Attachment #8786836 -
Flags: review?(smacleod) → review-
Comment 203•8 years ago
|
||
mozreview-review |
Comment on attachment 8807138 [details]
Specify ordering filediffs by destFilename (Bug 1248008)
https://reviewboard.mozilla.org/r/90402/#review91976
::: reviewboard/reviewboard/static/rb/js/diffviewer/collections/diffFileCollection_mozreview.js:6
(Diff revision 1)
> /*
> * A collection of files.
> */
> RB.DiffFileCollection = Backbone.Collection.extend({
> - model: RB.DiffFile
> + model: RB.DiffFile,
> + comparator: 'destFilename'
comparing by id should get rid of a bunch of the sorting issues - see review request in our extension for more information.
Attachment #8807138 -
Flags: review?(smacleod) → review-
Comment 204•8 years ago
|
||
mozreview-review |
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action (Bug 1248008)
https://reviewboard.mozilla.org/r/78616/#review91980
in the extension review request I talk about how we don't need this.
Attachment #8791064 -
Flags: review?(smacleod) → review-
Comment 205•8 years ago
|
||
mozreview-review |
Comment on attachment 8807137 [details]
Add diffFileCollection_mozreview.js to specify sorting by id (Bug 1248008)
https://reviewboard.mozilla.org/r/90400/#review91984
::: reviewboard/reviewboard/static/rb/js/diffviewer/collections/diffFileCollection_mozreview.js:1
(Diff revision 1)
> +/*
> + * A collection of files.
> + */
> +RB.DiffFileCollection = Backbone.Collection.extend({
> + model: RB.DiffFile
> +});
This file should be marked as a copy of the old file. Please use "hg cp" instead of just creating a new file.
Attachment #8807137 -
Flags: review?(smacleod) → review-
Assignee | ||
Comment 206•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Re-opening, you have marked this as fixed but did not fix it.
I had a broken repository and this slipped from copying, sorry.
Assignee | ||
Comment 207•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> you have a `(glob)` on this but you're not doing any globbing? Please ensure the identifiers of the strip backup are stable or properly glob the parts that aren't.
I got it from other test, I think it was `test-commit-s-added.t` - doesn't `$TESTTMP` need to be accompanied with `(glob)`?
Assignee | ||
Comment 208•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Please be more specific, what is this next part of the test actually attempting to verify?
I've removed it after fixing the "attachment" bit
Assignee | ||
Comment 209•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> ```
> Creating a FileDiff requires an already existing DiffSet
> (There is a non-nullable foreign key). Since we must
> create a particular FileDiff before its DiffSet is created
> we first create a temporary DiffSet to point at.
>
> Create this temporary DiffSet, or return it if it has been
> created in the past."""
> ```
Thanks
Assignee | ||
Comment 210•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> > Commit message filediff needs to be created as a first filediff in a set
>
> In the future someone reading this code would want to know *why* not just that it has to be. Please document this in detail.
>
> > Get temp diffset for commit_message_filediff creation.
>
> The reasoning for this should also be rolled into the comment.
I've written this instead:
# As commit message filediff has to be displayed as the first file it
# needs to be created before other filediffs in the DiffSet.
# FileDiff object has a required `DiffSet` field. Since before commit
# filediff is created we must create a temporary DiffSet.
# Later in the code it is replaced with the target one.
Assignee | ||
Comment 211•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> This is going to change when the commit series has been rebased, which will manifest really strangely in the interdiff (We'll show the previous commit message file as deleted, and a completely new one as added, instead of properly showing the difference). You'll need to use something stable for this commit throughout the entire series.
What do you think about storing the commit_message_name (with first commit_id) in extra_data?
Assignee | ||
Comment 212•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Something like the following would be much more informative:
> ```
> # Now that the proper DiffSet has been created, re-assign
> # the commit message FileDiff we created to the new DiffSet.
> ```
>
> (Side note, please properly capatalize the python class names when you are referring to them - this should be done throughout the entire patch, not just this line)
Do you think I should fix entire file? There is a lot of that outside of the patch.
Assignee | ||
Comment 213•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Will this result in a flicker when the page is loading? You could use a TemplateHook to conditionally load your CSS depending on the request, or two template hooks (one before content, one after) to wrap things in an element conditionally (not sure if this will mess up RBs CSS though).
>
> If this doesn't flicker because of the order the JS runs, I'd prefer we add the class to an element much closer to the FileDiffs, rather than the body.
It doesn't flicker, the class is set before the filediff is loaded (even on local network)
Assignee | ||
Comment 214•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Please see reopened issue in my previous review talking about how we can get rid of these listeners.
When it's gonna be ordered by ID we can remove the '000-' prefix, but we need to decide what to do with the suffix.
Assignee | ||
Comment 215•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Creating the commit message filediff should only be done if this is a completely new commit review request, or if the review request we're updating already had commit filediffs.
>
> We don't want to all of a sudden add commit message files to review requests that didn't start with them.
I've got it done, I'm creating the commit msg filediff in such case:
if (commit_data.draft_extra_data.get(COMMIT_MSG_FILEDIFF_IDS_KEY, False) or not rr.get_latest_diffset()):
I have trouble with testing.
I wanted to remove the `p2rb.commit_message_ids` from database using direct call to sqlite3, but there is a problem with parentheses.
This is the SQL I'm trying to run within test:
UPDATE mozreview_commitdata SET extra_data='{p2rb.first_public_ancestor: 'df67364c205763de5ad1d2c33fa78f87f6618289', p2rb.is_squashed: false, p2rb.author: 'test', p2rb.identifier: 'bz://1/mynick', p2rb.commit_id: 'ecfec372b48ce7b49eccf655e422acb4283283e0', p2rb: true}' WHERE review_request_id=2
Assignee | ||
Comment 216•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> re-opening this issue. As I said, we should just be able to make the files sort by ID in the fork, which means we can get rid of the commit message file prefix, as well as the javascript hook you're using to rename it. Please make this change.
`DiffViewerView` is extending `TemplateView`. It's `get` method is called every time server is preparing the diffsets list. The `get_context_data` is calling `diffviewer.diffutils.get_diff_files` which is sorting the result using `get_sorted_filediffs` from the same module.
If the dataflow is changed (`get_diff_files` returns unsorted list), server is responding with the order by id fashion, however in the front-end an alphabetical order is forced by JS. To fix this I've changed the `comparator` field in `reviewboard/reviewboard/static/rb/js/diffviewer/collections/diffFileCollection_mozreview.js`[1] from `destFilename` to `id`.
Interdiff `0-3` has desired order, but for interdiff `1-3` the order is `[commit-message, foo, bar]` which relates to their ids `[2, 3, 10]`. This is no longer alphabetical and is changing across other interdiffs. I don't think it's a desired solution.
Note: The change in JS would be enough to place the commit msg filediff on top of the list. There could be a potential issue in paging if many files would come before commit message filediff filename as ordering on the server would remain by `dest_filename`.
[1] https://reviewboard.mozilla.org/r/90402/diff/1#0
Assignee | ||
Comment 217•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> It doesn't flicker, the class is set before the filediff is loaded (even on local network)
The `.has-commit-msg-file` is now added to `#diffs`
Assignee | ||
Comment 218•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> `DiffViewerView` is extending `TemplateView`. It's `get` method is called every time server is preparing the diffsets list. The `get_context_data` is calling `diffviewer.diffutils.get_diff_files` which is sorting the result using `get_sorted_filediffs` from the same module.
>
> If the dataflow is changed (`get_diff_files` returns unsorted list), server is responding with the order by id fashion, however in the front-end an alphabetical order is forced by JS. To fix this I've changed the `comparator` field in `reviewboard/reviewboard/static/rb/js/diffviewer/collections/diffFileCollection_mozreview.js`[1] from `destFilename` to `id`.
>
> Interdiff `0-3` has desired order, but for interdiff `1-3` the order is `[commit-message, foo, bar]` which relates to their ids `[2, 3, 10]`. This is no longer alphabetical and is changing across other interdiffs. I don't think it's a desired solution.
>
> Note: The change in JS would be enough to place the commit msg filediff on top of the list. There could be a potential issue in paging if many files would come before commit message filediff filename as ordering on the server would remain by `dest_filename`.
>
> [1] https://reviewboard.mozilla.org/r/90402/diff/1#0
We agreed on following steps:
Check if pagination is indeed an issue if ordering by id is done in front-end only. That would eliminate the need to hack the `get_diff_files` in the fork.
There is no problem if order of files changes in interdiffs as long as paging is still right.
Assignee | ||
Comment 219•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> What do you think about storing the commit_message_name (with first commit_id) in extra_data?
Try to use a plain "commit-message" as a filenam. An additional check is needed on interdiffs.
> I've got it done, I'm creating the commit msg filediff in such case:
>
> if (commit_data.draft_extra_data.get(COMMIT_MSG_FILEDIFF_IDS_KEY, False) or not rr.get_latest_diffset()):
>
> I have trouble with testing.
> I wanted to remove the `p2rb.commit_message_ids` from database using direct call to sqlite3, but there is a problem with parentheses.
>
> This is the SQL I'm trying to run within test:
>
> UPDATE mozreview_commitdata SET extra_data='{p2rb.first_public_ancestor: 'df67364c205763de5ad1d2c33fa78f87f6618289', p2rb.is_squashed: false, p2rb.author: 'test', p2rb.identifier: 'bz://1/mynick', p2rb.commit_id: 'ecfec372b48ce7b49eccf655e422acb4283283e0', p2rb: true}' WHERE review_request_id=2
Create a SQL fixtures containing an entire structure using the code created before this task. Inject the file and test against old-style data.
> When it's gonna be ordered by ID we can remove the '000-' prefix, but we need to decide what to do with the suffix.
This might be solved in 2 ways without using JavaScript.
1. It might be possible to change the destFilename to plain 'commit-message' (requires additional tests mentioned above)
2. The name might be chanegd using CSS
Assignee | ||
Comment 220•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> The `.has-commit-msg-file` is now added to `#diffs`
There is an issue in the condition under this class is added. It is added regardless of the page. This has to happen only on the first page of diffs.
Comment 221•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> I got it from other test, I think it was `test-commit-s-added.t` - doesn't `$TESTTMP` need to be accompanied with `(glob)`?
Just to get it written down, we discussed this in our 1-1, and you do not need `(glob)` for just using the `$<name>` variables the test provides.
> I've written this instead:
> # As commit message filediff has to be displayed as the first file it
> # needs to be created before other filediffs in the DiffSet.
> # FileDiff object has a required `DiffSet` field. Since before commit
> # filediff is created we must create a temporary DiffSet.
> # Later in the code it is replaced with the target one.
Sounds good.
> Do you think I should fix entire file? There is a lot of that outside of the patch.
Keeping it to new code is fine.
Assignee | ||
Comment 222•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Try to use a plain "commit-message" as a filenam. An additional check is needed on interdiffs.
Interdiffs fail if the name is the same. Unique name needs to be stored in extra_data.
> This might be solved in 2 ways without using JavaScript.
> 1. It might be possible to change the destFilename to plain 'commit-message' (requires additional tests mentioned above)
> 2. The name might be chanegd using CSS
CSS is the way
Assignee | ||
Comment 223•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Just to get it written down, we discussed this in our 1-1, and you do not need `(glob)` for just using the `$<name>` variables the test provides.
using hg push in test is printing (glob) out:
$ hg push
pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
(adding commit id to 1 changesets)
- saved backup bundle to $TESTTMP/client/.hg/strip-backup/8d901902ff4b-c4c7c89c-addcommitid.hg
+ saved backup bundle to $TESTTMP/client/.hg/strip-backup/8d901902ff4b-c4c7c89c-addcommitid.hg (glob)
Assignee | ||
Comment 224•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8807137 [details]
Add diffFileCollection_mozreview.js to specify sorting by id (Bug 1248008)
https://reviewboard.mozilla.org/r/90400/#review91984
> This file should be marked as a copy of the old file. Please use "hg cp" instead of just creating a new file.
I'm wondering how to achieve that in MozReview - should I discard the review request and create a new one?
Assignee | ||
Comment 225•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Create a SQL fixtures containing an entire structure using the code created before this task. Inject the file and test against old-style data.
I've got trouble entering the sql file with commands to sqlite, tried piping subprocesses, but no luck
Assignee | ||
Comment 226•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> We agreed on following steps:
>
> Check if pagination is indeed an issue if ordering by id is done in front-end only. That would eliminate the need to hack the `get_diff_files` in the fork.
>
> There is no problem if order of files changes in interdiffs as long as paging is still right.
Server provides the `FileDiff`s in primary key order, but it sorts them if paging is involved.
Assignee | ||
Comment 227•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Server provides the `FileDiff`s in primary key order, but it sorts them if paging is involved.
I'm sorry - my last comment might be misunderstood.
If `get_diff_files` is not changed paging becomes an issue. Which means that commit message `FileDiff` might be displayed on a page other than the first one.
Assignee | ||
Comment 228•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> CSS is the way
I think it's hard/impossible to do without adding <span> around the name in `FileDiff` row and list.
So some additional change in the fork is needed.
Comment 229•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> I'm sorry - my last comment might be misunderstood.
> If `get_diff_files` is not changed paging becomes an issue. Which means that commit message `FileDiff` might be displayed on a page other than the first one.
Okay, so we discussed in IRC and you've let me know the sorting will happen server side if pagination is required. Meaning we'll need to modify the fork to prevent this sort and ensure our commit message gets put on the first page and is first.
Comment 230•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> using hg push in test is printing (glob) out:
>
> $ hg push
> pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
> (adding commit id to 1 changesets)
> - saved backup bundle to $TESTTMP/client/.hg/strip-backup/8d901902ff4b-c4c7c89c-addcommitid.hg
> + saved backup bundle to $TESTTMP/client/.hg/strip-backup/8d901902ff4b-c4c7c89c-addcommitid.hg (glob)
Huh, strange - I don't know what causes the glob to be required...
> I think it's hard/impossible to do without adding <span> around the name in `FileDiff` row and list.
> So some additional change in the fork is needed.
Hard to have the CSS do it without new markup? if you post a small fiddle or something like with the selecting first child I could take a look - I guess if we're already making fork changes for the sorting and js and stuff a little markup change wouldn't be a huge deal though.
Comment 231•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8807137 [details]
Add diffFileCollection_mozreview.js to specify sorting by id (Bug 1248008)
https://reviewboard.mozilla.org/r/90400/#review91984
> I'm wondering how to achieve that in MozReview - should I discard the review request and create a new one?
Ah, nope, no need to discard the review request. You'll just need to change it so the commit is created by using `hg cp ...` instead of manually copying the file contents. There might be a way to automatically recognise this, I think :gps has mentioned it before. But after you've done that as long as it has the same MozReview-Commit-Id when you push it will be fine.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8791064 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8807138 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 235•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Huh, strange - I don't know what causes the glob to be required...
Dropping it then. We can create another bug as it touches other tests as well
Assignee | ||
Comment 236•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Hard to have the CSS do it without new markup? if you post a small fiddle or something like with the selecting first child I could take a look - I guess if we're already making fork changes for the sorting and js and stuff a little markup change wouldn't be a huge deal though.
Minimal example: https://jsfiddle.net/zalun/o733uyvs/2/
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8807137 -
Attachment is obsolete: true
Attachment #8807137 -
Flags: review?(smacleod)
Assignee | ||
Updated•8 years ago
|
Attachment #8824353 -
Attachment is obsolete: true
Attachment #8824353 -
Flags: review?(smacleod)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 241•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> Minimal example: https://jsfiddle.net/zalun/o733uyvs/2/
And the solution - https://jsfiddle.net/o733uyvs/6/
Assignee | ||
Comment 242•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> And the solution - https://jsfiddle.net/o733uyvs/6/
I've got it working for the DiffFile, but #diff_index is more complicated (filename sits within an anchor). :davidwalsh implemented a mr:diffview-loaded event which does exactly the same as our former mr:FileDiffRendered [1]. I think it would be OK to create another event called mr:diff_context-loaded whih would be placed in the same file after context is loaded [2].
Are you OK with that?
[1] https://hg.mozilla.org/webtools/reviewboard/rev/8c09c3912c62
[2] https://reviewboard-hg.mozilla.org/reviewboard/file/tip/reviewboard/reviewboard/static/rb/js/pages/views/diffViewerPageView_mozreview.js#l735
Assignee | ||
Comment 243•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review88476
> Okay, so we discussed in IRC and you've let me know the sorting will happen server side if pagination is required. Meaning we'll need to modify the fork to prevent this sort and ensure our commit message gets put on the first page and is first.
Fixed in https://reviewboard.mozilla.org/r/102916/diff/2
Comment 244•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review91938
> I've got it working for the DiffFile, but #diff_index is more complicated (filename sits within an anchor). :davidwalsh implemented a mr:diffview-loaded event which does exactly the same as our former mr:FileDiffRendered [1]. I think it would be OK to create another event called mr:diff_context-loaded whih would be placed in the same file after context is loaded [2].
>
> Are you OK with that?
>
> [1] https://hg.mozilla.org/webtools/reviewboard/rev/8c09c3912c62
> [2] https://reviewboard-hg.mozilla.org/reviewboard/file/tip/reviewboard/reviewboard/static/rb/js/pages/views/diffViewerPageView_mozreview.js#l735
Ya I guess it'd be fine to do that. Thanks for the links :)
Comment 245•8 years ago
|
||
mozreview-review |
Comment on attachment 8824386 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) Bug 1248008
https://reviewboard.mozilla.org/r/102916/#review105770
Just a nit about the comment, should be r+ after that's updated.
::: reviewboard/reviewboard/diffviewer/diffutils.py:407
(Diff revision 2)
> - if len(files) == 1:
> + # MozReview is adding a commit message filediff which has to be the first
> + # from the set. Sorting by dest_filename would be causing issues when
> + # paging is involved.
We should mention that since the incoming diffs are already sorted in a logical way (by hg when it generates them) changing this won't really matter.
Something like:
```
# MozReview is adding a commit message filediff which has to be the first
# from the set. Sorting by dest_filename would cause issues when paging
# is involved. Since the filediff's are already in a logical order when
# sorted by id (which they should be here) removing this sort shouldn't
# make UX worse.
```
Attachment #8824386 -
Flags: review?(smacleod) → review-
Comment 246•8 years ago
|
||
mozreview-review |
Comment on attachment 8824452 [details]
Add diffFileCollection_mozreview.js to specify sorting by id (Bug 1248008)
https://reviewboard.mozilla.org/r/102954/#review105772
Attachment #8824452 -
Flags: review?(smacleod) → review+
Comment 247•8 years ago
|
||
mozreview-review |
Comment on attachment 8824453 [details]
MozReview: Specify ordering filediffs by id (Bug 1248008)
https://reviewboard.mozilla.org/r/102956/#review105774
Attachment #8824453 -
Flags: review?(smacleod) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 250•8 years ago
|
||
mozreview-review |
Comment on attachment 8824386 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) Bug 1248008
https://reviewboard.mozilla.org/r/102916/#review105946
Attachment #8824386 -
Flags: review?(smacleod) → review+
Comment 251•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8824386 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) Bug 1248008
https://reviewboard.mozilla.org/r/102916/#review105946
Actually, can you post a new revision that fixes the commit message summary line, it's missing the brackets.
Comment 252•8 years ago
|
||
mozreview-review |
Comment on attachment 8827471 [details]
MozReview: Add mr:diff-context-loaded event (Bug 1248008)
https://reviewboard.mozilla.org/r/105140/#review105960
Please post another revision with the bug bracketed in the commit message.
Attachment #8827471 -
Flags: review?(smacleod) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8824453 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8824386 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8827471 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 259•8 years ago
|
||
mozreview-review |
Comment on attachment 8828063 [details]
MozReview: Specify ordering filediffs by id (Bug 1248008)
https://reviewboard.mozilla.org/r/105576/#review106722
Attachment #8828063 -
Flags: review?(smacleod) → review+
Comment 260•8 years ago
|
||
mozreview-review |
Comment on attachment 8828064 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) (Bug 1248008)
https://reviewboard.mozilla.org/r/105578/#review106724
This commit message still doesn't have the `Bug <#>` in parenthesis`[1][2]`. Also, what happened to the previous review requests? why are they discarded and these are new ones?
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1248008#c251
[2] https://reviewboard.mozilla.org/r/102916/
Attachment #8828064 -
Flags: review?(smacleod) → review-
Comment 261•8 years ago
|
||
mozreview-review |
Comment on attachment 8828065 [details]
MozReview: Add mr:diff-context-loaded event (Bug 1248008)
https://reviewboard.mozilla.org/r/105580/#review106728
Attachment #8828065 -
Flags: review?(smacleod) → review+
Comment 262•8 years ago
|
||
mozreview-review |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review106926
Typo in your commit message, last paragraph "DileDiff"
::: hgext/reviewboard/tests/sql/remove-commit-message-file.sql:1
(Diff revision 24)
> +--- commit message filediff
> +DELETE FROM diffviewer_filediff WHERE id=2;
> +
> +--- temp diffset
> +DELETE FROM diffviewer_diffset WHERE id=2;
> +
> +--- remove extra_data fields from commitdata
> +UPDATE "mozreview_commitdata" SET
> + extra_data='{"p2rb.first_public_ancestor": "df67364c205763de5ad1d2c33fa78f87f6618289", "p2rb.temp_diffset_id": 2, "p2rb.is_squashed": false, "p2rb.author": "test", "p2rb.identifier": "bz://1/mynick", "p2rb.commit_id": "4a9cf7e9182050f2125ae6c10b21cfc78f6b25ef", "p2rb": true}',
> + draft_extra_data = '{"p2rb.temp_diffset_id": 2, "p2rb.first_public_ancestor": "df67364c205763de5ad1d2c33fa78f87f6618289", "p2rb.is_squashed": false, "p2rb.author": "test", "p2rb.commit_id": "4a9cf7e9182050f2125ae6c10b21cfc78f6b25ef", "p2rb.identifier": "bz://1/mynick", "p2rb": true}'
> + WHERE review_request_id=2;
This is very specific to the single test, but it's named in a generic way. How about you just fold this into the test, cat it into a file or something as part of the test. You should also document how this is generated, as a small change to the test will require regeneration.
::: hgext/reviewboard/tests/sql/remove-commit-message-file.sql:8
(Diff revision 24)
> +UPDATE "mozreview_commitdata" SET
> + extra_data='{"p2rb.first_public_ancestor": "df67364c205763de5ad1d2c33fa78f87f6618289", "p2rb.temp_diffset_id": 2, "p2rb.is_squashed": false, "p2rb.author": "test", "p2rb.identifier": "bz://1/mynick", "p2rb.commit_id": "4a9cf7e9182050f2125ae6c10b21cfc78f6b25ef", "p2rb": true}',
> + draft_extra_data = '{"p2rb.temp_diffset_id": 2, "p2rb.first_public_ancestor": "df67364c205763de5ad1d2c33fa78f87f6618289", "p2rb.is_squashed": false, "p2rb.author": "test", "p2rb.commit_id": "4a9cf7e9182050f2125ae6c10b21cfc78f6b25ef", "p2rb.identifier": "bz://1/mynick", "p2rb": true}'
Trailing whitespace
::: hgext/reviewboard/tests/test-auth.t:179
(Diff revision 24)
> +
> +
> +
This is really strange... does the test fail if you don't do this? if not you should remove this (and the others like it)
::: hgext/reviewboard/tests/test-commit-message-as-file.t:191
(Diff revision 24)
> + - +foo1
> + - ''
> + approved: false
> + approval_failure: A suitable reviewer has not given a "Ship It!"
> +
> +ReviewRequest created before implementing this change remains intacted.
"intacted"?
::: hgext/reviewboard/tests/test-review-request-delegation.t:87
(Diff revision 24)
> reviewer2:
> review_flag: r?
> ship_it: false
> diff:
> delete: 1
> - insert: 1
> + insert: 4
Hrmmm, I didn't even think that we'd be messing with this size measure... my inital reaction was that we should filter it out, but I guess the commit message is fine counting into size. That being said it might be better to be split out into it's own thing. Not something that we need to/should address now though, just thought I'd note it.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:927
(Diff revision 24)
> +def get_temp_diffset(id, repository):
> + """Get or create a temporary diffset.
This should really only need the repository (see my other comment about using one diffset per repository)
::: pylib/mozreview/mozreview/resources/batch_review_request.py:1014
(Diff revision 24)
> + temp_diffset = get_temp_diffset(commit_data.draft_extra_data.get(
> + TEMP_DIFFSET_ID_KEY, None), rr.repository)
You're still creating multiple diffsets per repository, since you're storing the id in the commit extra data, rather than something global - one per repostiory. Please change this so the DiffSet is created once per repository.
::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:110
(Diff revision 24)
> +.has-commit-msg-file {
> + .diff-container:first-child .sidebyside,
> + :not(.diff-container) + .diff-container .sidebyside {
This *really* ought to be documented, explaining how it works and why we need such a strange selector.
::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:112
(Diff revision 24)
> border-top: 2px dotted #aceb6f; // @comment-flag-draft-color;
> }
> +
> +.has-commit-msg-file {
> + .diff-container:first-child .sidebyside,
> + :not(.diff-container) + .diff-container .sidebyside {
Is there a tab character inserted here? RB is complaining... Please make sure you're not mixing tabs and spaces.
::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:113
(Diff revision 24)
> + tbody.insert tr td,
> + tbody.replace tr td {
> + background-color: #ffdfd7;
Ah, are you changing the commit messages addition to red-ish? I think it should either stay green for the insertion, or just be made white. The issue comes about though when dealing with an interdiff where colouring is important.
It's probably better to use something else to differentiate the commit message.
::: pylib/mozreview/mozreview/templatetags/mozreview.py:54
(Diff revision 24)
> return str(commit_data.get_for(review_request_details, COMMIT_ID_KEY))
>
>
> +@register.filter()
> +def commit_message_filediff_ids(review_request_details):
> + """Return the commit id of a review request or review request draft"""
it's not a commit id, it's a filediff id for the commit message.
Attachment #8786836 -
Flags: review?(smacleod) → review-
Assignee | ||
Comment 263•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review106926
> Trailing whitespace
Huh - switching back to VIM
Assignee | ||
Comment 264•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review106926
> Ah, are you changing the commit messages addition to red-ish? I think it should either stay green for the insertion, or just be made white. The issue comes about though when dealing with an interdiff where colouring is important.
>
> It's probably better to use something else to differentiate the commit message.
I had a request to make commit message FileDiff different from others.
Making id red-ish was more to see how (and if) these might be changed. I left it like this as screenshots were not commented. I'd go with same color, but italic font instead of current bold one. It would make it less important and slightly different.
Assignee | ||
Comment 265•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review106926
Comment 266•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review106926
> I had a request to make commit message FileDiff different from others.
> Making id red-ish was more to see how (and if) these might be changed. I left it like this as screenshots were not commented. I'd go with same color, but italic font instead of current bold one. It would make it less important and slightly different.
I actually think the text should be left the same, but doing something like changing the title bar styling, or the "filename" which is "Commit message" styling, to make that stand out. The actual diff itself is probably best left alone.
Assignee | ||
Comment 267•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review106926
> I actually think the text should be left the same, but doing something like changing the title bar styling, or the "filename" which is "Commit message" styling, to make that stand out. The actual diff itself is probably best left alone.
Sorry, I wasn't clear on this - I wanted to leave colors on text and change style on filename
Assignee | ||
Comment 268•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8828064 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) (Bug 1248008)
https://reviewboard.mozilla.org/r/105578/#review106724
I've commited all review requests, reviewboard automatically discarded the other ones
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 271•8 years ago
|
||
mozreview-review |
Comment on attachment 8828064 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) (Bug 1248008)
https://reviewboard.mozilla.org/r/105578/#review107908
Attachment #8828064 -
Flags: review?(smacleod) → review+
Comment hidden (mozreview-request) |
Comment 273•8 years ago
|
||
mozreview-review |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review108444
::: hgext/reviewboard/tests/test-commit-message-as-file.t:189
(Diff revision 25)
> +ReviewRequest created before implementing this change remains untouched.
> +No commit message filediff is created.
> +SQL="\"UPDATE mozreview_commitdata SET extra_data='{p2rb.first_public_ancestor: 'df67364c205763de5ad1d2c33fa78f87f6618289', p2rb.is_squashed: false, p2rb.author: 'test', p2rb.identifier: 'bz://1/mynick', p2rb.commit_id: 'ecfec372b48ce7b49eccf655e422acb4283283e0', p2rb: true}' WHERE review_request_id=2\""
> +mozreview exec rbweb "sqlite3 /reviewboard/data/reviewboard.db $SQL"
There's nothing actually being tested here? You don't do anything after this...
::: hgext/reviewboard/tests/test-commit-message-file-old-structure.t:179
(Diff revision 25)
> + - +foo2
> + - ''
> + approved: false
> + approval_failure: A suitable reviewer has not given a "Ship It!"
> +
> +Entirely new review request within the same parent has a commit-message FileDiff.
Review requests with the same parent *shouldn't* get commit message FileDiffs. This will be really strange since you'll have to look at two different locations for commit messages.
::: pylib/mozreview/mozreview/extra_data.py:48
(Diff revision 25)
> AUTHOR_KEY,
> FIRST_PUBLIC_ANCESTOR_KEY,
> IDENTIFIER_KEY,
> COMMIT_ID_KEY,
> + COMMIT_MSG_FILEDIFF_IDS_KEY,
> + TEMP_DIFFSET_ID_KEY,
This isn't part of commit data and shouldn't be in here.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:24
(Diff revision 25)
> from reviewboard.accounts.backends import (
> get_enabled_auth_backends,
> )
> from reviewboard.accounts.errors import (
> UserQueryError,
> )
> from reviewboard.diffviewer.models import (
> DiffSet,
> + FileDiff,
> )
> from reviewboard.reviews.models import (
> ReviewRequest,
> ReviewRequestDraft,
> )
> from reviewboard.scmtools.models import (
> Repository,
> )
> from reviewboard.webapi.decorators import (
> webapi_check_local_site,
> )
> from reviewboard.webapi.encoder import (
> status_to_string,
> )
> from reviewboard.webapi.resources import (
> WebAPIResource,
> )
> +from reviewboard.scmtools.core import (
> + PRE_CREATION,
> +)
>
> from mozreview.errors import (
> REVIEW_REQUEST_UPDATE_NOT_ALLOWED,
> )
> from mozreview.extra_data import (
> AUTHOR_KEY,
> BASE_COMMIT_KEY,
> COMMITS_KEY,
> COMMIT_ID_KEY,
> DISCARD_ON_PUBLISH_KEY,
> fetch_commit_data,
> FIRST_PUBLIC_ANCESTOR_KEY,
> IDENTIFIER_KEY,
> MOZREVIEW_KEY,
> SQUASHED_KEY,
> UNPUBLISHED_KEY,
> + COMMIT_MSG_FILEDIFF_IDS_KEY,
> + COMMIT_MSG_FILENAME_KEY,
> + TEMP_DIFFSET_ID_KEY,
Alphabetical. including inside the import statements.
::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:119
(Diff revision 25)
> + We use an ugly CSS hack to rename unique filename "commit-message-[hash]"
> + visually to "commit-message"
> + */
> +.has-commit-msg-file {
> + .diff-container:first-child .sidebyside,
> + :not(.diff-container) + .diff-container .sidebyside {
You're mixing tabs and spaces or something again. Please make sure these don't exist in your patch before asking for review.
::: testing/vcttesting/mozreview_mach_commands.py:143
(Diff revision 25)
> print('export ADMIN_PASSWORD=%s' % mr.admin_password)
> print('export HGSSH_HOST=%s' % mr.ssh_hostname)
> print('export HGSSH_PORT=%d' % mr.ssh_port)
> print('export PULSE_HOST=%s' % mr.pulse_host)
> print('export PULSE_PORT=%d' % mr.pulse_port)
> + print('export RBWEB_ID=%s' % mr.rbweb_id)
Why are you printing this?
::: testing/vcttesting/mozreview_mach_commands.py:382
(Diff revision 25)
> + """Run sequence of commands stored in file on a database on docker
> + container.
> + """
Summary line of a docstring should not wrap. Expansion on the summary can take place in lines below it, keep the summary short.
::: testing/vcttesting/mozreview_mach_commands.py:390
(Diff revision 25)
> + # TODO Make it work on other containers if needed
> + if name != 'rbweb':
> + raise NotImplementedError('Only `rbweb` container is implemented')
> +
> + mr = self._get_mozreview(None)
> + cid = getattr(mr, '%s_id' % name, None)
please use a less cryptic name.
Attachment #8786836 -
Flags: review?(smacleod) → review-
Assignee | ||
Comment 274•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review108444
> There's nothing actually being tested here? You don't do anything after this...
I've removed it - it is all moved to another test.
Assignee | ||
Comment 275•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review108444
> Why are you printing this?
left from a docker cp in the test file, removing
Comment hidden (mozreview-request) |
Comment 277•8 years ago
|
||
mozreview-review |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review108966
::: pylib/mozreview/mozreview/resources/batch_review_request.py:101
(Diff revisions 25 - 26)
> You must reopen the review request before it can be updated.
> Review requests should only be reopened if your changes have not landed or have
> been backed out - file new bugs for follow-up work.
> '''.strip()
>
> +TEMP_DIFFSET_ID_KEY = MOZREVIEW_KEY + '.temp_diffset_id'
This should still go in the extra_data file, just not be marked as drafted, and be put with a line between it in it's own new group rather than the commitdata constants.
::: pylib/mozreview/mozreview/resources/batch_review_request.py:1008
(Diff revisions 25 - 26)
> - if (commit_data.draft_extra_data.get(COMMIT_MSG_FILEDIFF_IDS_KEY, False)
> - or not rr.get_latest_diffset()):
> + sibling_generator = gen_child_rrs(get_parent_rr(rr))
> + a_sibling = _get_sibling(sibling_generator, rr)
> + should_create_commit_msg_filediff = (
> + not a_sibling and (
> + # Check if created after commit message filediff implementation.
> + commit_data.draft_extra_data.get(COMMIT_MSG_FILEDIFF_IDS_KEY, False)
> + # Check if it's a first push.
> + or not rr.get_latest_diffset()
> + ) or
> + # Check if sibling is created after commit msg FileDiff implementation.
> + a_sibling and fetch_commit_data(a_sibling).extra_data.get(
> + COMMIT_MSG_FILEDIFF_IDS_KEY, False)
> + )
It would be *way* simpler to just check an extra_data field on the parent, and set it when a commit message is created on the initial push of new stuff.
::: hgext/reviewboard/tests/test-commit-message-as-file.t:1
(Diff revision 26)
> +#require mozreviewdocker
(Bogus diff comment so I can open an issue).
Your commit message doesn't follow the conventions`[1]` for version-control-tools.
>Save commit message as a file (bug 1248008) r=smacleod
You're missing the `mozreview:` to start this. Might also be a better summary to say "mozreview: inject commit message as a FileDiff to allow reviewing it..."
>MozReview does not have a way to comment on commit message. This patch is solving the issue by adding a commit message FileDiff to the DiffSet. It is using a standard FileDiff and appears only in ReviewBoard. In every other aspect it works as a file from repository.
>
>Commit message file is given a name "commit-message" with a suffix (4 characters sliced from mercurial hashtag) to make it unique.
>
>DOM "#review_request" element has a "has-commit-msg-file" class to add style.
>
>The unique commit message with a suffix filename is changed to "commit-message" by a CSS hack.
>
>There is a listener for a mr:diff-context-loaded event which changes the filename of the commit message FileDiff in #diff_index list.
>
>The change doesn't affect existing ReviewRequests. No commit message FileDiff is added in that case.
This reads as a bunch of disjointed list items. It would be much more understandable if written in paragraphs, with the why included rather than just what is changed.
`[1]`https://mozilla-version-control-tools.readthedocs.io/en/latest/devguide/contributing.html#commit-creation-guidelines
Attachment #8786836 -
Flags: review?(smacleod) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 279•8 years ago
|
||
mozreview-review |
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
https://reviewboard.mozilla.org/r/75722/#review109248
::: pylib/mozreview/mozreview/resources/batch_review_request.py:68
(Diff revisions 25 - 27)
> + COMMIT_MSG_FILENAME_KEY,
> DISCARD_ON_PUBLISH_KEY,
> fetch_commit_data,
> FIRST_PUBLIC_ANCESTOR_KEY,
> + get_parent_rr,
> + gen_child_rrs,
I've left this one by mistake.
I'll wait for the review to avoid creating an almost empty revision.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Description
•