Create a proper field for p2rb.commits

RESOLVED FIXED

Status

MozReview
General
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dminor, Unassigned)

Tracking

(Blocks: 1 bug)

Development/Staging
Dependency tree / graph

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
Created 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 98a165f61a685e98e6b0e37bba67d50751b140a0
Attachment #8577207 - Flags: review?(smacleod)

Updated

3 years ago
Depends on: 1056849

Updated

3 years ago
Blocks: 1125473
No longer depends on: 1125473
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.
Attachment #8577207 - Flags: review?(smacleod)
(Reporter)

Comment 3

3 years ago
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
Attachment #8577207 - Flags: review?(smacleod)

Updated

3 years ago
Duplicate of this bug: 1135215

Updated

3 years ago
Priority: -- → P1
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 on attachment 8577207 [details]
MozReview Request: bz://1142615/dminor

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

Fix it, then ship-it!
Attachment #8577207 - Flags: review?(smacleod) → review+
(Reporter)

Comment 7

3 years ago
Thanks, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools/rev/111fc3434a1f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Reporter)

Comment 8

3 years ago
Missed this on the original commit: https://hg.mozilla.org/hgcustom/version-control-tools/rev/637b9299e2c5
(Reporter)

Comment 9

3 years ago
Comment on attachment 8577207 [details]
MozReview Request: bz://1142615/dminor
Attachment #8577207 - Attachment is obsolete: true
Attachment #8619740 - Flags: review+
(Reporter)

Comment 10

3 years ago
Created attachment 8619740 [details]
MozReview Request: rbmozui: make p2rb.commits a proper field (bug 1142615); r=smacleod
(Assignee)

Updated

2 years ago
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.