Closed Bug 1178814 Opened 9 years ago Closed 9 years ago

No error after hitting "Review" or "Ship It!" with expired Bugzilla cookie

Categories

(MozReview Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcote, Unassigned)

References

Details

Steps to reproduce:

1. Log into Review Board.
2. Go to any commit.
3. Expire your login cookie (removing all entries from the logincookies table in Bugzilla corresponding to your user will do it).
4. Hit the "Ship It!" button.  Alternatively, hit the "Review" button, type in a comment, and hit "Publish Review".

Expected results:

A cookie-expired error will pop up (admittedly not a great one; see bug 1178811).

Actual results:

No error is presented.  The page does not refresh.  Particularly in the case of a "Ship It!", it may appear that the r+ has been applied to the Bugzilla attachment, whereas it actually silently failed.

Note that this problem will change slightly after we switch to API keys, but it will still be relevant, since the user can revoke a key.
Also worth mentioning that this bug will be fixed before API-key support is finished, since it's less involved, immediately helpful, and will still be relevant after the switch.
Priority: -- → P1
Looks like PublishErrors weren't being caught in the UI for both these actions.  I submitted an upstream review request (https://reviews.reviewboard.org/r/7479/) that fixes it.
smacleod, can you think of any not-horrible way we could deploy this, or a similar fix, locally, until the core fix is released?  Bonus if you can prod that along too. :)
Flags: needinfo?(smacleod)
(In reply to Mark Côté [:mcote] from comment #3)
> smacleod, can you think of any not-horrible way we could deploy this, or a
> similar fix, locally, until the core fix is released?  Bonus if you can prod
> that along too. :)

As discussed in irc, I believe we could probably inject some js to monkeypatch
RB.ReviewablePageView._onShipItClicked. We'll probably need to call
RB.ReviewablePageView.undelegateEvents() before monkey patching, and then call
RB.ReviewablePageView.delegateEvents() after patching.
Flags: needinfo?(smacleod)
My patch to Review Board core[1] ended up being a little different from what I discussed with smacleod.  It may still be possible to monkey-patch it into our extension, but ideally we'll just wait until the next 2.0.x release.  I just pinged the Review Board maintainers for an ETA on that.

[1] https://github.com/reviewboard/reviewboard/commit/f90aa6ca6e146077839b1fb95818ecf0e9fb3c88
Looks like they'll have a release this weekend, so this should be fixed next week.  Also bug 993233, an even better fix that won't rely on sessions, should be deployed next week as well.
The fix for bug 993233 has been deployed, so this should not be an issue anymore except in two instances: a user without editbugs tries to do something that results in a disallowed action in Bugzilla (e.g. a "ship it" without having been requested for review), or if someone revokes their MozReview API key in Bugzilla and tries to perform an action in MozReview without logging out and back in again.  Both those should be relatively rare, although we will upgrade Review Board soon to catch these as well.
This is fixed now that Review Board 2.0.20 has been deployed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.