Closed Bug 1232703 Opened 9 years ago Closed 8 years ago

Automation menu not displayed on diff links

Categories

(MozReview Graveyard :: General, defect)

Product:
MozReview Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dminor, Assigned: davidwalsh)

References

Details

Attachments

(3 files, 2 obsolete files)

MozReview: Add view_diff_mozreview.html template in anticipation for automation links in diff pages (Bug 1232703).
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview: Add DiffViewerDropdownActionHook to add dropdown menus to diff view (Bug 1232703).
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview: Implement DiffViewerDropdownActionHook to add Automation (Bug 1232703).
58 bytes, text/x-review-board-request
glob
: review+
Details 
The automation menu is not displayed if you follow a diff link, e.g.
https://reviewboard.mozilla.org/r/27505/diff/1/
Product: Developer Services → MozReview
This has tripped up a few people lately and *should* theoretically be pretty easy.  Mind taking a look?
Assignee: nobody → dwalsh
On it!
I've traced what's happening here and it will require an upstream patch; working on that now!
Submitted upstream patch here:  https://reviews.reviewboard.org/r/8157/

Will monitor feedback and update this bug when I have more information.
(In reply to David Walsh :davidwalsh from comment #4)
> Submitted upstream patch here:  https://reviews.reviewboard.org/r/8157/
> 
> Will monitor feedback and update this bug when I have more information.

Any updates?
Flags: needinfo?(dwalsh)
Resubmitted update based on older branch: https://reviews.reviewboard.org/r/8301/

Hopefully we get a timely response.
Flags: needinfo?(dwalsh)
Comment on attachment 8779025 [details]
MozReview: Add DiffViewerDropdownActionHook to add dropdown menus to diff view (Bug 1232703).

https://reviewboard.mozilla.org/r/70082/#review67298

At present this commit is 500'ing but a peak into `var/log/httpd/error_log` or `reviewboard/logs/reviewboard.log` shows me nothing.
Comment on attachment 8779024 [details]
MozReview:  Add view_diff_mozreview.html template in anticipation for automation links in diff pages (Bug 1232703).

https://reviewboard.mozilla.org/r/70080/#review67482

looks like you forgot to rebase these changes on top of the public tip; pulling this down resulted in changes for a lot of other unfixed bugs.
Attachment #8779024 - Flags: review?(glob)
Comment on attachment 8779025 [details]
MozReview: Add DiffViewerDropdownActionHook to add dropdown menus to diff view (Bug 1232703).

https://reviewboard.mozilla.org/r/70082/#review67944

will review fully after the rebase
Attachment #8779025 - Flags: review?(glob)
Comment on attachment 8779025 [details]
MozReview: Add DiffViewerDropdownActionHook to add dropdown menus to diff view (Bug 1232703).

https://reviewboard.mozilla.org/r/70082/#review67944

Working to rebase now.
Comment on attachment 8779025 [details]
MozReview: Add DiffViewerDropdownActionHook to add dropdown menus to diff view (Bug 1232703).

https://reviewboard.mozilla.org/r/70082/#review67944

OK, I think I rebased this properly!
Comment on attachment 8779804 [details]
MozReview: Add DiffViewerDropdownActionHook to add dropdown menus to diff view (Bug 1232703).

https://reviewboard.mozilla.org/r/70730/#review68096

FML, this went to the wrong repo.  Please disregard.
Attachment #8779804 - Flags: review-
Attachment #8779803 - Attachment is obsolete: true
Attachment #8779803 - Flags: review?(glob)
Attachment #8779804 - Attachment is obsolete: true
Attachment #8779804 - Flags: review?(glob)
Comment on attachment 8779025 [details]
MozReview: Add DiffViewerDropdownActionHook to add dropdown menus to diff view (Bug 1232703).

https://reviewboard.mozilla.org/r/70082/#review68360

::: reviewboard/reviewboard/extensions/templatetags/rb_extensions.py:62
(Diff revision 3)
>      return action_hooks(context, DiffViewerActionHook)
>  
>  
>  @register.tag
>  @basictag(takes_context=True)
> +def diffviewerdropdown_action_hooks(context):

this should be named diffviewer_dropdown_action_hooks to
(a) mirror diffviewer_action_hooks, and
(b) make it not throw a ISE/500 error
Attachment #8779025 - Flags: review?(glob)
Comment on attachment 8779024 [details]
MozReview:  Add view_diff_mozreview.html template in anticipation for automation links in diff pages (Bug 1232703).

https://reviewboard.mozilla.org/r/70080/#review68362
Attachment #8779024 - Flags: review?(glob) → review+
Comment on attachment 8779025 [details]
MozReview: Add DiffViewerDropdownActionHook to add dropdown menus to diff view (Bug 1232703).

https://reviewboard.mozilla.org/r/70082/#review68360

> this should be named diffviewer_dropdown_action_hooks to
> (a) mirror diffviewer_action_hooks, and
> (b) make it not throw a ISE/500 error

Right; like I said, I can't find a specific log that would help me find out why it's 500'ing.  Any ideas?
(In reply to David Walsh :davidwalsh from comment #23)
> Right; like I said, I can't find a specific log that would help me find out
> why it's 500'ing.  Any ideas?

sorry; i missed this question.  renaming the function from diffviewerdropdown_action_hooks to diffviewer_dropdown_action_hooks should be enough to fix the issue.

i had quite a lot of difficulty figuring out how to show the actual error too; and this is something that i plan to make easier once i've cleared my goal.

you need to switch reviewboard into debug mode in order for it to display template errors in browser:

shell into the rbweb container:
> ./mozreview shell rbweb
enable debug mode in settings_local:
> vi /reviewboard/conf/settings_local.py
>   change "DEBUG = False" to "DEBUG = True" and save
and restart apache:
> apachectl graceful

unfortunately it'll be set back to False the next time you refresh the container.
Comment on attachment 8782514 [details]
MozReview: Implement DiffViewerDropdownActionHook to add Automation (Bug 1232703).

https://reviewboard.mozilla.org/r/72658/#review70548

while the automation menu is now visible on child requests, this change results in js errors breaking large parts of the page (eg. commits table is completely missing).

Could not find a valid id for the parent review request.reviews.min.da5089870160.js:1:128
TypeError: FileDiffReviewerData is undefined
Attachment #8782514 - Flags: review?(glob) → review-
Comment on attachment 8779025 [details]
MozReview: Add DiffViewerDropdownActionHook to add dropdown menus to diff view (Bug 1232703).

https://reviewboard.mozilla.org/r/70082/#review70550
Attachment #8779025 - Flags: review?(glob) → review+
Comment on attachment 8782514 [details]
MozReview: Implement DiffViewerDropdownActionHook to add Automation (Bug 1232703).

https://reviewboard.mozilla.org/r/72658/#review70548

I logged into Admin, went to "Edit Repo", but don't know how to turn on autoland to test this.  I think the issue is that a few JS files need to be added to our `extension.py`, but need to get autoland working for testing first.
Comment on attachment 8782514 [details]
MozReview: Implement DiffViewerDropdownActionHook to add Automation (Bug 1232703).

https://reviewboard.mozilla.org/r/72658/#review70548

I...don't see these issues.  I clicked the "Trigger a Try Build" link on the "Reviews" and "Diff" tabs of single commits and the Review Summary and I saw 0 layout issues and the correct dialog/modal popped up, no issues.  Can I have more detail?
Comment on attachment 8782514 [details]
MozReview: Implement DiffViewerDropdownActionHook to add Automation (Bug 1232703).

https://reviewboard.mozilla.org/r/72658/#review70548

Looking at those errors, and inspecting the code, I really get the feeling this was an issue with :glob's environment, unrelated to this change. :glob, did you also apply the reviewboardfork changes in https://reviewboard.mozilla.org/r/70078/ before running this? Were things built from a clean environment?
Comment on attachment 8782514 [details]
MozReview: Implement DiffViewerDropdownActionHook to add Automation (Bug 1232703).

https://reviewboard.mozilla.org/r/72658/#review70548

The other thing to note, from your comment glob "the automation menu is now visible on child requests" - it has always appeared on child requests, this is about it appearing on the diff page.
Comment on attachment 8782514 [details]
MozReview: Implement DiffViewerDropdownActionHook to add Automation (Bug 1232703).

https://reviewboard.mozilla.org/r/72658/#review70548

Another look glob please?
putting on my radar
Flags: needinfo?(glob)
Comment on attachment 8782514 [details]
MozReview: Implement DiffViewerDropdownActionHook to add Automation (Bug 1232703).

https://reviewboard.mozilla.org/r/72658/#review70548

> it has always appeared on child requests, this is about it appearing on the diff page
sorry, i was testing correctly but my comment was incorrect.

after a full rebuild of my review env, and double-checking that i'm using the correct revisions in both repo, i still see the js error and missing features from before.

smac: can you test locally instead of just inspecting the code please.


after setting debug to true, i see it's a template error:

TemplateDoesNotExist at /r/1/diff/1/
reviews/review_request_box_mozreview.html

that file is referenced in the rb-fork changes, but it's not part of the repo.  i'll update my review of that change.
Flags: needinfo?(glob)
Comment on attachment 8779024 [details]
MozReview:  Add view_diff_mozreview.html template in anticipation for automation links in diff pages (Bug 1232703).

https://reviewboard.mozilla.org/r/70080/#review74992

::: reviewboard/reviewboard/templates/diffviewer/view_diff_mozreview.html:56
(Diff revision 4)
> +   </ul>
> +  </div>
> + </div>
> +
> +<div class="main">
> +{%    include "reviews/review_request_box_mozreview.html" %}

review_request_box_mozreview.html doesn't exist, and breaks the page.

please ensure the file is duplicated without any other edits when adding the _mozreview version.
Attachment #8779024 - Flags: review+ → review-
Comment on attachment 8779024 [details]
MozReview:  Add view_diff_mozreview.html template in anticipation for automation links in diff pages (Bug 1232703).

https://reviewboard.mozilla.org/r/70080/#review74992

> review_request_box_mozreview.html doesn't exist, and breaks the page.
> 
> please ensure the file is duplicated without any other edits when adding the _mozreview version.

*sigh* I confused myself last week and reverted that file update for some reason.  Will update today.
Comment on attachment 8779024 [details]
MozReview:  Add view_diff_mozreview.html template in anticipation for automation links in diff pages (Bug 1232703).

https://reviewboard.mozilla.org/r/70080/#review74992

> *sigh* I confused myself last week and reverted that file update for some reason.  Will update today.

Fixed!
Comment on attachment 8779024 [details]
MozReview:  Add view_diff_mozreview.html template in anticipation for automation links in diff pages (Bug 1232703).

https://reviewboard.mozilla.org/r/70080/#review78126
Attachment #8779024 - Flags: review?(glob) → review+
Comment on attachment 8782514 [details]
MozReview: Implement DiffViewerDropdownActionHook to add Automation (Bug 1232703).

https://reviewboard.mozilla.org/r/72658/#review78128
Attachment #8782514 - Flags: review- → review+
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/webtools/reviewboard/rev/85fe384bf911
MozReview:  Add view_diff_mozreview.html template in anticipation for automation links in diff pages . r=glob
https://hg.mozilla.org/webtools/reviewboard/rev/f55c75a21275
MozReview: Add DiffViewerDropdownActionHook to add dropdown menus to diff view . r=glob
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/a9c757ddf57a
MozReview: Implement DiffViewerDropdownActionHook to add Automation . r=glob
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: