1245404 - Have all calls to global/user.html.tmpl pass a mode

Status: REOPENED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Albert Ting]

global/user.html.tmpl is used in several places (attachment, comment, email, reporter, etc).  It would help if user.html.tmpl knew where it was being called from.

In my case, I created a popup to display custom profile details, including some icons.  This works fine for the comment section, yet it doesn't work when creating an email message.  I also want a different behavior for users tied to attachments, etc.

Comment 1

[Frédéric Buclin]

All templates already know where they are called from using the internal [% component.callers %] variable, see http://www.template-toolkit.org/docs/manual/Variables.html#section_component. So passing an explicit variable should not be needed in your case.

Comment 2

[Dylan Hardison [:dylan] (he/him)]

Sorry to disagree, Frederic -- but relying on component.caller seems brittle. I'd prefer explicitly passing the context that global/user.html.tmpl is needed, as Albert suggests. 

Albert: proceed if you have a patch for this.

Comment 3

[Albert Ting]

I don't have a patch just yet. Currently using a bug/comments-user-start hook so that my popup only works for comments.  Probably next quarter as part of adding more user fields.

Comment 4

[Albert Ting]

BTW, any preference in terms of variable name?  Should it be "context", "mode", etc?

Comment 5

[Albert Ting]

Dylan, bug/edit.html.tmpl has five global/user.html.tmpl calls.  I believe it is sufficient to pass the same mode for all them (.ie edit) as opposed to unique values (reporter, qa_contact, assigned_to, or even assigned_to_with_edit).  Agreed?

Comment 6

[Frédéric Buclin]

(In reply to Albert Ting from comment #5)
> Dylan, bug/edit.html.tmpl has five global/user.html.tmpl calls.  I believe
> it is sufficient to pass the same mode for all them (.ie edit) as opposed to
> unique values (reporter, qa_contact, assigned_to, or even
> assigned_to_with_edit).  Agreed?

If you pass 5 times the same mode, then I don't see the advantage over component.callers. So you should be more explicit and pass 5 different modes, in case you want to trigger different actions based on different roles.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

(In reply to Frédéric Buclin from comment #6)
> (In reply to Albert Ting from comment #5)
> > Dylan, bug/edit.html.tmpl has five global/user.html.tmpl calls.  I believe
> > it is sufficient to pass the same mode for all them (.ie edit) as opposed to
> > unique values (reporter, qa_contact, assigned_to, or even
> > assigned_to_with_edit).  Agreed?
> 
> If you pass 5 times the same mode, then I don't see the advantage over
> component.callers. So you should be more explicit and pass 5 different
> modes, in case you want to trigger different actions based on different
> roles.

Using one mode (edit) is a fine first pass solution. Those call sites can be changed if we need further specialization.
component.callers is still wrong because it is action at a distance.

Comment 8

[Albert Ting]

Attachment: v1

Initial patch.  One thing I didn't expect was the user_cache.  Had to change the id to include the mode, otherwise bugzilla would reuse, say, an attachment user block in a comment user block.

Comment 9

[Frédéric Buclin]

(In reply to Albert Ting from comment #8)
> Initial patch.  One thing I didn't expect was the user_cache.  Had to change
> the id to include the mode, otherwise bugzilla would reuse, say, an
> attachment user block in a comment user block.

Yes, that's the point of using user_cache, to avoid having to call global/user.html.tmpl again and again, because the time penalty is big when you have many attachments and comments. The idea behind user_cache is to reuse already generated templates. Your patch defeats this.

Comment 10

[Frédéric Buclin]

Comment on attachment 8723323 [details] [diff] [review]
v1

>+++ template/en/default/attachment/list.html.tmpl	2016-02-24 00:29:15.295015000 -0600
>@@ -94,7 +94,7 @@
>-            [% attacher_id = attachment.attacher.id %]
>+            [% attacher_id = attachment.attacher.id _ "attacher_list" %]
>             [% UNLESS user_cache.$attacher_id %]
>               [% user_cache.$attacher_id = BLOCK %]
>-                [% INCLUDE global/user.html.tmpl who = attachment.attacher %]
>+                [% INCLUDE global/user.html.tmpl who = attachment.attacher, mode = "attacher_list" %]
>               [% END %]
>             [% END %]

There is no reason to force a time penalty to everybody only for a few extensions which could benefit from this 'mode' attribute. So please add boolean to bypass this check which you can set with a hook. I think the 'template_before_process' hook already lets you do this.

Comment 11

[Albert Ting]

(In reply to Frédéric Buclin from comment #10)
> There is no reason to force a time penalty to everybody only for a few
> extensions which could benefit from this 'mode' attribute. 

Including the mode is required, otherwise it's a real bug, you could be using the wrong cached block.  I hit this myself.  Plus I don't think it's much of a time penalty, even if there is a large number of comments/attachments by multiple users.

If a hook is still required, could you clarify?  Don't quite understand the suggestion.

Comment 12

[Frédéric Buclin]

(In reply to Albert Ting from comment #11)
> Including the mode is required, otherwise it's a real bug, you could be
> using the wrong cached block.  I hit this myself.

This is why I suggested to add a boolean, to recall global/user.html.tmpl when the mode is relevant to you.


>  Plus I don't think it's
> much of a time penalty, even if there is a large number of
> comments/attachments by multiple users.

I implemented the user cache on purpose, see bug 731562.


> If a hook is still required, could you clarify?  Don't quite understand the
> suggestion.

Before templates are processed, the 'template_before_process' hook is called, see https://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/Hook.html#template_before_process
This hook lets you define extra variables, such as e.g. 'bypass_user_cache' or the name you want. You could then update the check to be:

    [% IF bypass_user_cache || !user_cache.$attacher_id %]

This way, installations which do not care about the mode will still benefit from the user cache (because bypass_user_cache will be undefined, i.e. false), and installations which care can set bypass_user_cache = 1 based on criteria defined by the extension.

Comment 13

[Albert Ting]

Hi Frederic,

I agree the user cache is useful, hence I don't want to bypass the user cache.  Instead, I want to change the user_cache lookup from 'comment.author.id' to 'comment.author.id _ "commenter"'.   I don't see how I can do that in the template_before_process.

Comment 14

[Frédéric Buclin]

(In reply to Albert Ting from comment #13)
> Instead, I want to change the user_cache lookup from
> 'comment.author.id' to 'comment.author.id _ "commenter"'.

If you do that, then the number of calls to global/user.html.tmpl will be multiplied by 2! I know this what you want in your case, but that's not what we want for the vast majority of installations (because the role is irrelevant to them).

So if you still want to use the user cache, then things are a bit more complicated. I would suggest that your extension manage its own user cache, and substitute the template in the official user cache with the one you want, based on who calls it. I think this would work. The 'file' parameter of the hook will tell who is the caller, and from here, your extension can call global/user.html.tmpl itself, if the template is not already available with the desired role. And this way, you don't need to hack attachment/list.html.tmpl at all. Either that, or I suggest you manage it as a local customization, to not impact everybody.

Comment 15

[Albert Ting]

I don't think it's 2x.  The user_cache is only for commenters and attachers.  So you would only see 2x if a ticket had N unique commenters and the same N folks each attached a file.  In reality, I think the number of attachers is just a handful.  Besides, neither the reporter, assignee, nor qa_contact are using the user_cache; these would more frequent than the number of unique attachers. 

Managing my own user cache won't work, as I don't see how to override the current user_cache via a hook, as well as override the hardcoded id lookups used in attachment/list.html and comments.html.  

Another option could be to move all the user_cache code inside user.html.tmpl.  Then all user fields would benefit from the cache, including email, reporter, assignee, qa_contact, etc.

Comment 16

[Frédéric Buclin]

(In reply to Albert Ting from comment #15)
> Another option could be to move all the user_cache code inside
> user.html.tmpl.  Then all user fields would benefit from the cache,
> including email, reporter, assignee, qa_contact, etc.

Hum no. What is slow is to call another template in a loop. So if you move the cache in global/user.html.tmpl, then you would have to call the template anyway. The perf win would go away.

Comment 17

[Albert Ting]

(In reply to Frédéric Buclin from comment #16)
> Hum no. What is slow is to call another template in a loop. So if you move
> the cache in global/user.html.tmpl, then you would have to call the template
> anyway. The perf win would go away.

Ok.  Given we've appear to accept the performance without using user_cache for reporter/assignee/qa_contact, it doesn't seem significant to include attachments that don't happen too often.  But if we still don't want to use mode-based ids for the user cache, what other options are there?  Even if I followed comment #1, it still won't work, especially if somebody decides to use user_cache for bugmail.html.tmpl.