485418 - Code and template hooks for userprefs.cgi to be able to add additional tabs

Status: RESOLVED FIXED

(Bugzilla :: Extensions, enhancement, P2)

Description

[Matt Rogers]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/4.2; Linux) KHTML/4.2.1 (like Gecko)
Build Identifier: 

I'd like to add an extension to bugzilla that gives users the ability to create so called 'canned responses' so that one or more bugs that need to be dealt with in the same way (i.e. closed due to lack of information) can be handled quickly and with a consistent response.

Reproducible: Couldn't Reproduce

Comment 1

[Max Kanat-Alexander]

Sound like good hooks to have!

Comment 2

[Frédéric Buclin]

Taking. I already have code ready, but I still need to write documentation and examples for the hooks.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

That's the hook I use with GCC Bugzilla.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 476654 [details] [diff] [review]
patch, v1

>Index: userprefs.cgi
>+    Bugzilla::Hook::process('userprefs_tabs', { 'vars' => $vars });

  I think we should call this user_preferences instead, in case we ever move this code into Bugzilla::User or some other place. Or perhaps user_update_preferences?

>Index: Bugzilla/Hook.pm
>+This hashref contains two keys by default. C<changes_saved> is true
>+when the user clicks "Submit Changes". In that case, the data should be
>+validated and the DB updated accordingly. C<current_tab_name> contains
>+the name of the panel being displayed.

  Since hooks are generally supposed to be a stable API, I think that we should actually explicitly pass these two variables as separate items, and not as a part of $vars. Will $vars sometimes also contain more data? Could the extension author just use Bugzilla->input_params to get them?

> You should always make sure that
>+the name of the panel matches what you expect it to be. 

  Should probably start a new paragraph there.

> You can add as many new
>+key/value pairs as you want to this hashref. It will be passed to the template.

  Why not just use template_before_process and simply set a hook that allows people to add new named tabs, kind of like how the Config stuff works? I think that could be pretty nice.

>+Make sure to end your subroutine with C<last SWITCH;> to notify the
>+script that this panel exists and that you took care of it. Else an
>+error is thrown reporting that this panel is unknown.

  Hmm, instead of that you'll want to have them send something back explicitly in the args, because we might change the structure of that code.

>Index: extensions/Example/Extension.pm
>+    my $value = Bugzilla->cgi->param('example_pref');

  Use input_params instead--it's more standard Extension style.


  Everything else looks really nice! :-)

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
>   Since hooks are generally supposed to be a stable API, I think that we should
> actually explicitly pass these two variables as separate items, and not as a
> part of $vars. Will $vars sometimes also contain more data? Could the extension
> author just use Bugzilla->input_params to get them?

I can pass the content of these two variables separately, to make the API stable, but they will still be passed as part of $vars, as userprefs.cgi needs them. Also, yes, $vars can sometimes contain more data. My GCC extension does that when validating input data. So I don't think Bugzilla->input_params is usable here, unless I can update some existing keys and add new ones without overridding everything else.


>   Why not just use template_before_process and simply set a hook that allows
> people to add new named tabs, kind of like how the Config stuff works? I think
> that could be pretty nice.

No idea what you mean, but the goal here is really to keep things simple.

Comment 6

[Frédéric Buclin]

Attachment: patch, v1

I addressed all other comments.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 482283 [details] [diff] [review]
patch, v1

  This actually looks pretty great! A few tiny changes would be good, though:

>+    Bugzilla::Hook::process('user_preferences',
>+                            { 'vars'       => $vars,
>+                              save_changes => $save_changes,
>+                              current_tab  => $current_tab_name });
>+
>+    # extension_panel is set to true by extensions if the tab name is valid.
>+    last SWITCH if $vars->{extension_panel};

  Instead of putting extension_panel into $vars, you could either pass an arrayref (like we do for the check_can_change_field hook) or just a reference to a scalar, and call it something like $handled?

>Index: Bugzilla/Hook.pm
>+Make sure to end your subroutine with C<$args-E<gt>{'vars'}-E<gt>{extension_panel} = 1>

  Use C<< >> there instead. So like C<< $args-> >>.

Comment 8

[Frédéric Buclin]

Attachment: patch, v2

Fixed both comments.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 488100 [details] [diff] [review]
patch, v2

Ah, just one more tiny fix--the POD for "handled" needs to note that it's a reference to a scalar, not a scalar.

Comment 10

[Frédéric Buclin]

Attachment: patch, v3

Comment 11

[Max Kanat-Alexander]

Comment on attachment 488103 [details] [diff] [review]
patch, v3

Hmm, okay. Something more like:

=item C<handled>

This is B<reference> to a scalar, not a scalar. (So you would set it like C<$$handled = 1>>, not like C<$handled=1>.) Set this to a true value to let Bugzilla know that the passed-in panel is valid and that you have handled it. (Otherwise, Bugzilla will throw an error that the panel is invalid.) Don't set this to true if you didn't handle the panel listed in C<current_tab>.



With that, r+.

Comment 12

[Frédéric Buclin]

Attachment: patch, v3.1

Fixed POD. Carrying r+ forward. Thanks for the review! :)

Comment 13

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified userprefs.cgi
modified Bugzilla/Hook.pm
modified extensions/Example/Extension.pm
added extensions/Example/template/en/default/account
added extensions/Example/template/en/default/account/prefs
added extensions/Example/template/en/default/account/prefs/my_tab.html.tmpl
added extensions/Example/template/en/default/hook/account
added extensions/Example/template/en/default/hook/account/prefs
added extensions/Example/template/en/default/hook/account/prefs/prefs-tabs.html.tmpl
modified template/en/default/account/prefs/prefs.html.tmpl
Committed revision 7597.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified userprefs.cgi
modified Bugzilla/Hook.pm
modified extensions/Example/Extension.pm
added extensions/Example/template/en/default/account
added extensions/Example/template/en/default/account/prefs
added extensions/Example/template/en/default/account/prefs/my_tab.html.tmpl
added extensions/Example/template/en/default/hook/account
added extensions/Example/template/en/default/hook/account/prefs
added extensions/Example/template/en/default/hook/account/prefs/prefs-tabs.html.tmpl
modified template/en/default/account/prefs/prefs.html.tmpl
Committed revision 7477.