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

Status: RESOLVED FIXED

(MozReview Graveyard :: Review Board: User Interface, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 1

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

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.

Comment 3

[:Gijs (he/him)]

As I noted in the dupe, this also means you can't autoland until refreshing the page.

Byron, how can we get this prioritized?

Comment 4

[:glob ✱]

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.

Comment 5

[:glob ✱]

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.

Comment 6

[:glob ✱]

Attachment: 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/

Comment 7

[:glob ✱]

Attachment: 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/

Comment 8

[:glob ✱]

Attachment: 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/

Comment 9

[:glob ✱]

Attachment: 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 10

[David Walsh :davidwalsh]

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!

Comment 11

[David Walsh :davidwalsh]

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 12

[David Walsh :davidwalsh]

Comment on attachment 8770131 [details]
mozreview: move commits table into its file (bug 1253552)

https://reviewboard.mozilla.org/r/63696/#review60704

Comment 13

[David Walsh :davidwalsh]

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 14

[Steven MacLeod [:smacleod]]

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.

Comment 15

[Steven MacLeod [:smacleod]]

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

Comment 16

[David Walsh :davidwalsh]

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

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

Comment 17

[:glob ✱]

Attachment: mozreview: update login error handler to use shortcuts.render (bug 1253552)

Simplify login error renderer, and rename for clarity.

Review commit: https://reviewboard.mozilla.org/r/70846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70846/

Comment 18

[:glob ✱]

Comment on attachment 8770131 [details]
mozreview: move commits table into its file (bug 1253552)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63696/diff/1-2/

Comment 19

[:glob ✱]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63698/diff/1-2/

Comment 20

[:glob ✱]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63700/diff/1-2/

Comment 21

[Steven MacLeod [:smacleod]]

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

https://reviewboard.mozilla.org/r/70846/#review69468

Comment 22

[Steven MacLeod [:smacleod]]

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`)

Comment 23

[Steven MacLeod [:smacleod]]

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

[David Walsh :davidwalsh]

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! :)

Comment 25

[:glob ✱]

Comment on attachment 8770131 [details]
mozreview: move commits table into its file (bug 1253552)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63696/diff/2-3/

Comment 26

[:glob ✱]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70846/diff/1-2/

Comment 27

[:glob ✱]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63698/diff/2-3/

Comment 28

[:glob ✱]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63700/diff/2-3/

Comment 29

[David Walsh :davidwalsh]

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

[David Walsh :davidwalsh]

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

Comment 31

[Steven MacLeod [:smacleod]]

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...

Comment 32

[:glob ✱]

Comment on attachment 8770131 [details]
mozreview: move commits table into its file (bug 1253552)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63696/diff/3-4/

Comment 33

[:glob ✱]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70846/diff/2-3/

Comment 34

[:glob ✱]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63698/diff/3-4/

Comment 35

[:glob ✱]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63700/diff/3-4/

Comment 36

[Pulsebot]

Pushed by [e-mail]:
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