698418 - Add hooks for UI for additional login methods

Status: RESOLVED FIXED

(Bugzilla :: Extensions, enhancement)

Description

[Gervase Markham [:gerv]]

I would like to add a new login method to Bugzilla (BrowserID: bug 672841) and this requires adding a "Login with BrowserID" button to Bugzilla on the main login page and in the header. This needs some hooks.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here's what I've been using.

Gerv

Comment 2

[Max Kanat-Alexander]

Comment on attachment 570691 [details] [diff] [review]
Patch v.1

Review of attachment 570691 [details] [diff] [review]:
-----------------------------------------------------------------

::: template/en/default/account/auth/login-small.html.tmpl
@@ +44,5 @@
>          id="mini_login[% qs_suffix FILTER html %]"
>          onsubmit="return check_mini_login_fields( '[% qs_suffix FILTER html %]' );"
>    >
> +    
> +    [% Hook.process('additional_methods') %]

I don't think this is the right place. If we're going to add new links to the top of the page, they should be added when the login form is collapsed, not when it's expanded. When the login form is expanded, there isn't usually enough space to actually add more stuff and still have a reasonable UI.

::: template/en/default/account/auth/login.html.tmpl
@@ +95,4 @@
>    # their password, assuming that our auth method allows that.
>    #%]
>  
> +  [% Hook.process('additional_methods') %]

Hmm. Do you think that perhaps this should be inside of the form? Or would that not suit your needs?

Comment 3

[Gervase Markham [:gerv]]

Hi Max,

Thanks for the review :-)

(In reply to Max Kanat-Alexander from comment #2)
> I don't think this is the right place. If we're going to add new links to
> the top of the page, they should be added when the login form is collapsed,
> not when it's expanded. When the login form is expanded, there isn't usually
> enough space to actually add more stuff and still have a reasonable UI.

I'm not quite understanding you. This hook is for adding additional UI for logging in - in my use case, a "Log In With BrowserID" button (although the text is shorter than that!). Surely it makes sense for the UI to be behind the "login" expandable? Otherwise you have some login UI behind the expandable and some not.

Or have I misunderstood you?


> Hmm. Do you think that perhaps this should be inside of the form? Or would
> that not suit your needs?

Whether it's inside or outside the <form>, in and of itself, doesn't matter to me because it's just a button you click which runs some JS. But I do want to be able to stick in an <hr> and have it in a separate "section" because it's a different login method. So logically, I think it should be outside the form.

Gerv

Comment 4

[Gervase Markham [:gerv]]

mkanat: ping? :-)

Gerv

Comment 5

[Frédéric Buclin]

Comment on attachment 570691 [details] [diff] [review]
Patch v.1

>=== modified file 'template/en/default/account/auth/login-small.html.tmpl'

>+    [% Hook.process('additional_methods') %]

Here, the hook is inside <form></form>, meaning that you will send data to the script defined in <form>, unless you use JS (as for BrowserID). But this unfortunately doesn't let you easily send data to another script if you have to. For consistency with the other hook below, you should move this one outside <form></form>.


>=== modified file 'template/en/default/account/auth/login.html.tmpl'

>   # their password, assuming that our auth method allows that.
>   #%]
> 
>+  [% Hook.process('additional_methods') %]

Please move this hook before the [%# comment %] right above it. This hook has nothing to do with what the comment above it is about.


Otherwise this looks good (also when looking at stage).

Comment 6

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Review comments addressed; looking for a quick rubber stamp :-)

Gerv

Comment 7

[Frédéric Buclin]

Comment on attachment 608338 [details] [diff] [review]
Patch v.2

>=== modified file 'template/en/default/account/auth/login-small.html.tmpl'

>+    

Remove the useless whitespaces here. Otherwise looks good. r=LpSolit

Comment 8

[Gervase Markham [:gerv]]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified template/en/default/account/auth/login.html.tmpl
modified template/en/default/account/auth/login-small.html.tmpl
Committed revision 8162.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.2/                         
modified template/en/default/account/auth/login.html.tmpl
modified template/en/default/account/auth/login-small.html.tmpl
Committed revision 8056.  

Gerv