1123139 - Remove commit list / links, pull information and commit message from "Description" field

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Mike Conley (:mconley) (:⚙️)]

With bug 1064111 fixed, the commit list and links are superfluous.

The commit message and pull information can be put into their own fields.

The description could then be used as a way for the reviewee to optionally describe their solution to fixing the bug. This could also be guessed from any additional comments below the first line of the commit message.

Comment 1

[Botond Ballo [:botond]]

+1, I've actually been meaning to request a "description" field myself.

Comment 2

[Dan Minor [:dminor]]

:smacleod, mind if I pick this one up?

Comment 3

[Steven MacLeod [:smacleod]]

(In reply to Dan Minor [:dminor] from comment #2)
> :smacleod, mind if I pick this one up?

Go for it - assigned to you.

Comment 4

[Dan Minor [:dminor]]

It makes sense to me to remove the commits list and move the links to their own section.

Since we're trying to encourage people to write meaningful commit messages, I think we should always populate the description of child review requests based upon the commit message, rather than adding a separate section for it.

That would leave the description field on the parent review request for general comments, if any.

Comment 5

[Botond Ballo [:botond]]

(In reply to Dan Minor [:dminor] from comment #4)
> Since we're trying to encourage people to write meaningful commit messages,
> I think we should always populate the description of child review requests
> based upon the commit message, rather than adding a separate section for it.
> 
> That would leave the description field on the parent review request for
> general comments, if any.

I feel that there is a place for per-commit general comments, too. I understand the desire to write meaningful commit messages, but I find there is often information to be conveyed to the reviewer that's relevant to the context of the review but less so to someone looking at the commit later. Especially in a long commit series, it's useful to be able to associate such information with a specific commit.

Comment 6

[Dan Minor [:dminor]]

We discussed this in the MozReview meeting and decided to create a separate field for information to be conveyed the reviewer that isn't properly part of the commit message. I filed Bug 1194354 for this.

Comment 7

[Dan Minor [:dminor]]

Attachment: MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

mozreview: remove extraneous information from description (bug 1123139); r?smacleod

This removes commit list and link information from the "Description" field and
reserves it for the commit summary. The field is hidden for parent review
requests as they lack a commit summary.

A new field is added which contains the command to pull down the commits as
well as a link to the commits through the Mercurial web interface.

Comment 8

[Dan Minor [:dminor]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

mozreview: remove extraneous information from description (bug 1123139); r?smacleod

This removes commit list and link information from the "Description" field and
reserves it for the commit summary. The field is hidden for parent review
requests as they lack a commit summary.

A new field is added which contains the command to pull down the commits as
well as a link to the commits through the Mercurial web interface.

Comment 9

[Steven MacLeod [:smacleod]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

https://reviewboard.mozilla.org/r/16319/#review14897

Have you run the tests? I'd have thought the description changes would definitely require test updates.

::: pylib/mozreview/mozreview/fields.py:88
(Diff revision 1)
> +    field_id = "p2rb.commit_id"

If you're creating a field for this you'll need to remove it from `DRAFTED_EXTRA_DATA_KEYS` in `rbbz.extension`.

Unless an extra data key has a field associated it won't be copied from the draft to the review request - We copy things in `DRAFTED_EXTRA_DATA_KEYS` manually.

::: pylib/mozreview/mozreview/fields.py:93
(Diff revision 1)
> +    def has_value_changed(self, old_value, new_value):
> +        return old_value != new_value

This matches the default behaviour, lets not define it.

::: pylib/mozreview/mozreview/fields.py:102
(Diff revision 1)
> +    def as_html(self):

We should be overriding `render_value` instead of `as_html`. It will be passed the value of `p2rb.commit_id` which we are supposed to use for the rendering. That should also simplify this quite a bit.

Comment 10

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/16319/#review14897

heh, I only ran the selenium tests, I forgot that we end up dumping the description everywhere.

Comment 11

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/16319/#review14897

Comment 12

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/16319/#review14897

Comment 13

[Dan Minor [:dminor]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio

This removes commit list and link information from the "Description" field and
reserves it for the commit summary. The field is hidden for parent review
requests as they lack a commit summary.

A new field is added which contains the command to pull down the commits as
well as a link to the commits through the Mercurial web interface.

Comment 14

[Mauro Doglio [:mdoglio]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

https://reviewboard.mozilla.org/r/16319/#review15377

Comment 15

[Dan Minor [:dminor]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio

This removes commit list and link information from the "Description" field and
reserves it for the commit summary. The field is hidden for parent review
requests as they lack a commit summary.

A new field is added which contains the command to pull down the commits as
well as a link to the commits through the Mercurial web interface.

Comment 16

[Dan Minor [:dminor]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio

This removes commit list and link information from the "Description" field and
reserves it for the commit summary. The field is hidden for parent review
requests as they lack a commit summary.

A new field is added which contains the command to pull down the commits as
well as a link to the commits through the Mercurial web interface.

Comment 17

[Steven MacLeod [:smacleod]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

https://reviewboard.mozilla.org/r/16319/#review15511

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

https://reviewboard.mozilla.org/r/16319/#review15519

::: pylib/mozreview/mozreview/templates/mozreview/hg-info.html:25
(Diff revision 2)
> +    {% endif %}

Something else we may want to add here is the command to import this single commit.

  $ hg import https://reviewboard-hg.../rev/<sha1>
  
Ideally we'd also sneak a "--exact" in there because "import" applies the changeset against the "." revision by default and "--exact" makes it apply to the changeset defined in the commit. Using "--exact" ensures there will be no conflicts. But it may not work if the client doesn't have the parent changeset locally.
  
All that being said, we should prefer people use `hg pull` because that is a more "native" workflow and there won't be conflicts if parent changesets are missing. However, if this is someone landing or taking over the series, they'll almost always want to "hg rebase" after the pull. We could steer them in the direction of `hg pull --rebase` or we could implement a custom client-side command/alias for doing the pull+rebase easily. e.g. `hg prb mr://16319`. Oh yeah, Mercurial supports custom protocol handlers which can do magic things :)

Having said all that, let's just advertise "hg import <url>" for starters and see how that works. We can iterate later.

Comment 19

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/16319/#review15377

Comment 20

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/16319/#review15511

Comment 21

[Dan Minor [:dminor]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio

This removes commit list and link information from the "Description" field and
reserves it for the commit summary. The field is hidden for parent review
requests as they lack a commit summary.

New fields are added to the Information FieldSet with commands to import and
pull the commits.

Comment 22

[Dan Minor [:dminor]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio

This removes commit list and link information from the "Description" field and
reserves it for the commit summary. The field is hidden for parent review
requests as they lack a commit summary.

New fields are added to the Information FieldSet with commands to import and
pull the commits.

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

https://reviewboard.mozilla.org/r/16319/#review15631

I'd *really* like to see a Selenium test for this feature.

::: hgext/reviewboard/tests/test-commits-deleted-obsolescence.t:397
(Diff revision 3)
> +<<<<<<< dest
>    description:
>    - /r/3 - Bug 1 - Foo 2
>    - /r/5 - Bug 1 - Foo 4
>    - ''
>    - 'Pull down these commits:'
>    - ''
>    - hg pull -r 2fbc30f77859fa4be2e173866fa71c52d394f2c4 http://*:$HGPORT/test-repo (glob)
> +=======
> +  description: This is the parent review request
> +>>>>>>> source

merge conflict markers!

::: pylib/mozreview/mozreview/fields.py:99
(Diff revision 3)
> +        if commit is None:

commit != commit_id. I guess this never runs?

::: pylib/mozreview/mozreview/fields.py:105
(Diff revision 3)
> +        return ('<input style="font-size: smaller; position: relative; '
> +                'top:-3px" value="hg import -r %s %s" onclick="this.select();" '
> +                'readonly="">' % (commit_id[:12], repo_path))

These values need to be escaped to prevent XSS, as rare as it may seem.

Also, the command is:

  $ hg import http://<repo>/rev/<commit>

::: pylib/mozreview/mozreview/fields.py:129
(Diff revision 3)
> +        return ('<input style="font-size: smaller; position: relative; '
> +                'top:-3px" value="hg pull -r %s %s" onclick="this.select();" '
> +                'readonly="">' % (commit_id[:12], repo_path))

Escape.

Comment 24

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/16319/#review15633

Comment 25

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/16319/#review15635

Comment 26

[Steven MacLeod [:smacleod]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

https://reviewboard.mozilla.org/r/16319/#review15629

::: pylib/mozreview/mozreview/fields.py:92
(Diff revision 3)
> +    def load_value(self, review_request_details):
> +        return self.review_request_details.extra_data.get('p2rb.commit_id')

As we discussed in person I'm confused that this actually worked.

What we should do is replace `render_value` with `as_html`, and then just scrap our implementation of `load_value` (copying that code into `as_html` to fetch the value there).

::: pylib/mozreview/mozreview/fields.py:105
(Diff revision 3)
> +        return ('<input style="font-size: smaller; position: relative; '
> +                'top:-3px" value="hg import -r %s %s" onclick="this.select();" '
> +                'readonly="">' % (commit_id[:12], repo_path))

We should build this using a template which makes the escaping easy, and this styling reusable between the two fields.

::: pylib/mozreview/mozreview/fields.py:115
(Diff revision 3)
> +    def render_value(self, commit_id):

let's just fake out this review request field too (don't forget to add it back to drafted extra data keys). That way the implementations will be similar.

::: pylib/rbbz/rbbz/extension.py
(Diff revision 3)
> -    'p2rb.commit_id',

Add this back when you fake the field.

Comment 27

[Dan Minor [:dminor]]

https://reviewboard.mozilla.org/r/16319/#review15633

Comment 28

[Dan Minor [:dminor]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

This removes commit list and link information from the "Description" field and
reserves it for the commit summary. The field is hidden for parent review
requests as they lack a commit summary.

New fields are added to the Information FieldSet with commands to import and
pull the commits.

Comment 29

[Dan Minor [:dminor]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

This removes commit list and link information from the "Description" field and
reserves it for the commit summary. The field is hidden for parent review
requests as they lack a commit summary.

New fields are added to the Information FieldSet with commands to import and
pull the commits.

Comment 30

[Steven MacLeod [:smacleod]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

https://reviewboard.mozilla.org/r/16319/#review15739

looking really good, I think we can probably land this after one more pass.

::: pylib/mozreview/mozreview/fields.py:94
(Diff revision 5)
> +        commit_id = self.review_request_details.extra_data.get('p2rb.commit_id')

We should be using `mozreview.extra_data.COMMIT_ID_KEY`.

::: pylib/mozreview/mozreview/fields.py:111
(Diff revision 5)
> +        commit_id = self.review_request_details.extra_data.get('p2rb.commit_id')

We should be using `mozreview.extra_data.COMMIT_ID_KEY`.

::: pylib/mozreview/mozreview/fields.py:111
(Diff revision 5)
> +        commit_id = self.review_request_details.extra_data.get('p2rb.commit_id')
> +
> +        if not commit_id:

let's differentiate between child and parent by calling `is_parent` instead of using the lack of a commit id as a proxy.

In the parent case we'll do the same thing, use the last accessible child for the pull command, and in the child case we can just use a present commit id, or display nothing if it's empty.

::: pylib/mozreview/mozreview/fields.py:120
(Diff revision 5)
> +            commit_id = children[-1].extra_data.get('p2rb.commit_id')

We should be using `mozreview.extra_data.COMMIT_ID_KEY`.

::: pylib/mozreview/mozreview/templates/mozreview/commit-info.html:2
(Diff revision 5)
> +This is the template for the instructions on how to deal with commits. 

trailing whitespace.

Comment 31

[Dan Minor [:dminor]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

This removes commit list and link information from the "Description" field and
reserves it for the commit summary. The field is hidden for parent review
requests as they lack a commit summary.

New fields are added to the Information FieldSet with commands to import and
pull the commits.

Comment 32

[Gregory Szorc [:gps]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

https://reviewboard.mozilla.org/r/16319/#review15781

Looks great!

Comment 33

[Steven MacLeod [:smacleod]]

Comment on attachment 8648908 [details]
MozReview Request: mozreview: remove extraneous information from description (bug 1123139); r?smacleod,mdoglio,gps

https://reviewboard.mozilla.org/r/16319/#review15783

::: pylib/mozreview/mozreview/fields.py:15
(Diff revision 6)
> -from mozreview.extra_data import gen_child_rrs, get_parent_rr, is_parent, is_pushed
> +from mozreview.extra_data import (COMMIT_ID_KEY,
> +                                  gen_child_rrs,
> +                                  get_parent_rr,
> +                                  is_parent,
> +                                  is_pushed,
> +                                  )

Lets switch to mercurial style multi line import
```
from mozreview.extra_data import (
    COMMIT_ID_KEY
    ...
)

::: pylib/mozreview/mozreview/fields.py:104
(Diff revision 6)
> +            logging.error('No commit_id for review request: %d' % (
> +                review_request.id))
> +            commit_id = ''

In this case we should not show anything in the field, instead of still showing the hg command with the revision blanked.

::: pylib/mozreview/mozreview/fields.py:139
(Diff revision 6)
> +            commit_id = ''

Same in this case, we want to render the field but blank.

Comment 34

[Dan Minor [:dminor]]

Thanks, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools/rev/9918621281d1