Final review status not updated after last open issue gets marked as fixed

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: whimboo, Assigned: glob)

Tracking

Details

MozReview Requests

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

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
I have seen this today while fixing the one and remaining open issue for https://reviewboard.mozilla.org/r/37941. Ted gave r+ for both commits, but opened an issue. I fixed that issue and marked it as such. So the icon right next to Ted avatar shows ship it. But in the commits table above I'm still seeing `!1` and not `r+` as status. This got solved after shift-reloading the page.
Yeah, the issues display in the commits view isn't being updated dynamically - only on full page reload. The front-end should be rewired to update it dynamically.
See Also: → bug 1198570
Duplicate of this bug: 1266610
As I noted in the dupe, this also means you can't autoland until refreshing the page.

Byron, how can we get this prioritized?
Flags: needinfo?(glob)
(Assignee)

Comment 4

2 years ago
this hasn't been addressed yet because that part of the code has undergone a large rewrite (bug 1197879).

i'm currently working on sorting out some issues that fell out of those changes; i can at least check if the story is better after that work.  if not, i'm happy to poke at this once i have some other high priority items resolved.
Flags: needinfo?(glob)
(Assignee)

Comment 5

2 years ago
this issue still happens following the changes.

off the top of my head there's a few things we could do:

1. split the table rendering into its own template, and add an api call to get the rendered output.  update the table via xhr when issues are fixed or reopened.
2. move the rendering of the commits table from server-side django to client-side backbone.  triggering a refresh when state changes should then be simple.
3. mirror some of the logic from the django template client-side, update the table "just enough" to make it look correct.

i'm going to see how much work (1) is, and if i encounter too many roadblocks eyeball (3).  (2) should probably happen when we get around to redoing that part of the UI completely.
Assignee: nobody → glob
(Assignee)

Comment 6

2 years ago
Created attachment 8770130 [details]
MozReview: trigger a jquery JS event when an issue status changes (bug 1253552)

In order to update the commits table when an issue's status is changed, we need
to trigger an event that we can listen for.

Review commit: https://reviewboard.mozilla.org/r/63692/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63692/
Attachment #8770130 - Flags: review?(dwalsh)
(Assignee)

Comment 7

2 years ago
Created attachment 8770131 [details]
mozreview: move commits table into its file (bug 1253552)

In order to support updating the commits table in-page as issues are
resolved/reopened, we need to move the table into its own file so it can be
requested separately.

Review commit: https://reviewboard.mozilla.org/r/63696/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63696/
Attachment #8770131 - Flags: review?(dwalsh)
Attachment #8770132 - Flags: review?(smacleod)
Attachment #8770133 - Flags: review?(dwalsh)
(Assignee)

Comment 8

2 years ago
Created attachment 8770132 [details]
MozReview: add commits-summary-table view (bug 1253552);

Add a view that returns just the commits table.

Review commit: https://reviewboard.mozilla.org/r/63698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63698/
(Assignee)

Comment 9

2 years ago
Created attachment 8770133 [details]
mozreview: update commits table when issue status changes (bug 1253552)

Listen for the mr:issue_status_changed event and refresh the commits table.

Review commit: https://reviewboard.mozilla.org/r/63700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63700/
Comment on attachment 8770130 [details]
MozReview: trigger a jquery JS event when an issue status changes (bug 1253552)

https://reviewboard.mozilla.org/r/63692/#review60702

Works for me!
Attachment #8770130 - Flags: review?(dwalsh) → review+
https://reviewboard.mozilla.org/r/63700/#review60676

Functionally worked for me!

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:18
(Diff revision 1)
>    <pre id="error-stack"></pre>
>    <a href="#" id="error-stack-toggle">Stack</a>
>    <a href="#" id="error-close">Close</a>
>  </div>
> +
> +<div id="mozreview-data"

I hate to nit this type of thing but can we have the attributes end with `id`?  Will make the coad more descriptive and easier to know what's expected from those attributes
Comment on attachment 8770131 [details]
mozreview: move commits table into its file (bug 1253552)

https://reviewboard.mozilla.org/r/63696/#review60704
Attachment #8770131 - Flags: review?(dwalsh) → review+
https://reviewboard.mozilla.org/r/63700/#review60676

> I hate to nit this type of thing but can we have the attributes end with `id`?  Will make the coad more descriptive and easier to know what's expected from those attributes

I spelt "code" wrong.  HOW?!
Comment on attachment 8770130 [details]
MozReview: trigger a jquery JS event when an issue status changes (bug 1253552)

https://reviewboard.mozilla.org/r/63692/#review60750

::: reviewboard/reviewboard/static/rb/js/views/reviewBoxView.js:229
(Diff revision 1)
>  
>          this._updateLabels();
> +
> +        // MozReview: trigger a jquery js event so we can update
> +        // the commits table when an issue status changes.
> +        $(document).trigger('mr:issue_status_changed', this);

This feels like perfect fodder for a js extension hook - they're quite simple to write[1].

[1] For an example, see `CommentDialogHook` in upstream.
Attachment #8770130 - Flags: review-
Comment on attachment 8770132 [details]
MozReview: add commits-summary-table view (bug 1253552);

https://reviewboard.mozilla.org/r/63698/#review60754

::: pylib/mozreview/mozreview/views.py:282
(Diff revision 1)
> +    if not parent_request.is_accessible_by(request.user):
> +        return HttpResponseNotAllowed('Permission denied')

Permissions checking should come immediately after existence checking.

::: pylib/mozreview/mozreview/views.py:298
(Diff revision 1)
> +    return HttpResponse(
> +        render_to_string('mozreview/commits-requests.html', {
> +            'review_request_details': child_request,
> +            'children_details': children_details,
> +        }),
> +        mimetype='text/html')

https://github.com/django/django/blob/1.6.11/django/shortcuts/__init__.py#L31
Attachment #8770132 - Flags: review?(smacleod)
Attachment #8770133 - Flags: review?(dwalsh) → review-
Comment on attachment 8770133 [details]
mozreview: update commits table when issue status changes (bug 1253552)

https://reviewboard.mozilla.org/r/63700/#review62234
(Assignee)

Updated

2 years ago
Attachment #8770130 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

2 years ago
mozreview-review
Comment on attachment 8779981 [details]
mozreview: update login error handler to use shortcuts.render (bug 1253552)

https://reviewboard.mozilla.org/r/70846/#review69468
Attachment #8779981 - Flags: review?(smacleod) → review+

Comment 22

2 years ago
mozreview-review
Comment on attachment 8770132 [details]
MozReview: add commits-summary-table view (bug 1253552);

https://reviewboard.mozilla.org/r/63698/#review69474

::: pylib/mozreview/mozreview/views.py:283
(Diff revision 2)
> +    try:
> +        child_request = ReviewRequest.objects.get(id=child_id)
> +    except ReviewRequest.DoesNotExist:
> +        return HttpResponseNotFound('Child Not Found')

We need to verify that this child is actually the child of the parent, and that the user has permission to access it. As it is now I *think* you could extract info from a specified review request (especially if the template grows to include more info from `review_request_details`)
Attachment #8770132 - Flags: review?(smacleod) → review-

Comment 23

2 years ago
mozreview-review
Comment on attachment 8770133 [details]
mozreview: update commits table when issue status changes (bug 1253552)

https://reviewboard.mozilla.org/r/63700/#review69470

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:393
(Diff revision 2)
> +  // Refresh the commits table when issues are fixed/dropped/reopened.
> +  RB.PageManager.getPage().commentIssueManager.on('issueStatusUpdated',

We also need to recalculate where something can be autolanded, updating the automation menu... which may be more difficult. Or at minimum change the tooltip on the autoland and try buttons to indidcate a page refresh is required (ew)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8770133 [details]
mozreview: update commits table when issue status changes (bug 1253552)

https://reviewboard.mozilla.org/r/63700/#review69902

Outside of smacleod's comments, this worked for me! :)

Updated

2 years ago
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8770133 [details]
mozreview: update commits table when issue status changes (bug 1253552)

https://reviewboard.mozilla.org/r/63700/#review79760

::: pylib/mozreview/mozreview/static/mozreview/js/commits.js:408
(Diff revision 3)
> +        });
> +
> +      // Update the parentReviewRequest object, then update the autoland menu.
> +      MozReview.parentReviewRequest.fetch({
> +        success: function() {
> +          $(document).trigger('update_autoland_menuitem');

We might want to prefix all of our other events like this in the future.

Comment 30

2 years ago
mozreview-review
Comment on attachment 8770133 [details]
mozreview: update commits table when issue status changes (bug 1253552)

https://reviewboard.mozilla.org/r/63700/#review80302

Works and functions well so I'm R+'ing.

At some point we should switch to camel case for our JavaScript -- underscores are blah in JavaScript land :D
Attachment #8770133 - Flags: review?(dwalsh) → review+

Comment 31

2 years ago
mozreview-review
Comment on attachment 8770132 [details]
MozReview: add commits-summary-table view (bug 1253552);

https://reviewboard.mozilla.org/r/63698/#review83066

::: pylib/mozreview/mozreview/templates/mozreview/commits-requests.html:2
(Diff revision 3)
>  {% comment %}
> -This is the template for the "Commits" list in a push-based review request.
> +This is the template for the "Commits" list in a review request. It is used

Oh man, takes me back to the days when this whole mess we call MozReview was "push to Review Board" or "p2rb", the idea being it would be generic and used outside Mozilla...
Attachment #8770132 - Flags: review?(smacleod) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

2 years ago
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6b465dea76b1
mozreview: move commits table into its file r=davidwalsh
https://hg.mozilla.org/hgcustom/version-control-tools/rev/1587cc30c28f
mozreview: update login error handler to use shortcuts.render r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/623614b92f4a
MozReview: add commits-summary-table view ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/d6e14db5f03c
mozreview: update commits table when issue status changes r=davidwalsh
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.