|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
39 bytes, text/x-review-board-request
|Details | Review|
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.
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
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.  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 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
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.  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+
Thanks, pushed to: https://hg.mozilla.org/hgcustom/version-control-tools/rev/111fc3434a1f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Missed this on the original commit: https://hg.mozilla.org/hgcustom/version-control-tools/rev/637b9299e2c5
Comment on attachment 8577207 [details] MozReview Request: bz://1142615/dminor
Created attachment 8619740 [details] MozReview Request: rbmozui: make p2rb.commits a proper field (bug 1142615); r=smacleod
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.