1142615 - Create a proper field for p2rb.commits

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Dan Minor [:dminor]]

Right now we store commit information in extra_data but it should really be a proper field to allow this information to be stored as part of a change description.

Comment 1

[Dan Minor [:dminor]]

Attachment: MozReview Request: bz://1142615/dminor

/r/5325 - rbmozui: make p2rb.commits a proper field (bug 1142615); r=smacleod

Pull down this commit:

hg pull review -r 98a165f61a685e98e6b0e37bba67d50751b140a0

Comment 2

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5325/#review4605

::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 1)
>          ReviewRequestFieldsHook(self, 'main', [CommitsListField])
>          # This forces the Commits field to be the top item.
>          main_fieldset.field_classes.insert(0,
>                                             main_fieldset.field_classes.pop())
>  
>          # The above hack forced Commits at the top, but the rest of these
>          # fields are fine below the Description.
> +        ReviewRequestFieldsHook(self, 'main', [CommitsField])

The `CommitsListField` and `CommitsField` should really be combined into one - The `epoch` used for allowing empty publishes on the parent request should become its own field `CombinedReviewersField` or something.

Doing this all properly though is going to mean merging work with the new server-side rendered table. I'm torn on whether you should start that process here, or we should just have the new table rebased ontop of this and then moved into your new field.

How do you feel about this?

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 1)
> -    label = _("Commits")
> +    label = _("CommitsList")

I believe this actually gets displayed on the review request page as the label for whetever the field renders - Another reason why we should probably get these two fields merged.

Either way though, whatever does the actual rendering of the commits should probably have the `label` "Commits". I don't think these labels actually need to be unique either.

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 1)
> +class CommitsField(BaseReviewRequestField):

I'm thinking we might need to override `has_value_changed` to compare the json commit by commit. Or maybe just comparing the serialization is fine. Just make sure you check we're properly comparing as equal in all cases where the commits were just sha identical.

[1] https://github.com/reviewboard/reviewboard/blob/1bde43d5534c49946f139c0af32e999766adaa67/reviewboard/reviews/fields.py#L136-L142

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 1)
> +    def load_value(self, review_request_details):
> +        return review_request_details.extra_data.get('p2rb.commits')

This is unchanged from the base class, we should just leave it out.

Comment 3

[Dan Minor [:dminor]]

Comment on attachment 8577207 [details]
MozReview Request: bz://1142615/dminor

/r/5325 - rbmozui: make p2rb.commits a proper field (bug 1142615); r=smacleod

Pull down this commit:

hg pull review -r 6d8412899eae1fc7f3ea98dde2d2dc35c8e77588

Comment 5

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/5325/#review5685

Looks good for the most part, just a couple small issues.

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 2)
>      def render_change_entry_html(self, info):
>          return ""

This should be changed to override the `get_change_entry_sections_html` like I mentioned above.

Eventually when the new commits table lands, we'll want to actually render something meaningful for the commits changing and remove the list of commits from the description. That's not in the scope of this bug though.

::: pylib/rbmozui/rbmozui/fields.py
(Diff revision 2)
> +    def should_render(self, value):
> +        return False

We'll also want to override `get_change_entry_sections_html`(1) to probably just return an empty list. This will stop it from appearing in the box which shows what changed on publish (the `render_change_entry_html` below which returned an empty string was taking care of that before).

Really though what we should probably be doing is making the change description rendering of this field show something like "Changes to reviewers were made". To do that we'd just need to update the two methods mentioned above to return something reasonable instead of rendering based on actual values of the epoch. We'd also probably want to make sure the title `get_change_entry_sections_html` is returning makes sense.

[1] https://github.com/reviewboard/reviewboard/blob/62ac05f3bc4103f8cea5a6af7de2dad7fd5b54b9/reviewboard/reviews/fields.py#L190

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8577207 [details]
MozReview Request: bz://1142615/dminor

https://reviewboard.mozilla.org/r/5323/#review5687

Fix it, then ship-it!

Comment 7

[Dan Minor [:dminor]]

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

Comment 8

[Dan Minor [:dminor]]

Missed this on the original commit: https://hg.mozilla.org/hgcustom/version-control-tools/rev/637b9299e2c5

Comment 10

[Dan Minor [:dminor]]

Attachment: MozReview Request: rbmozui: make p2rb.commits a proper field (bug 1142615); r=smacleod