Closed Bug 152632 Opened 22 years ago Closed 22 years ago

'My Bugs' in footer doesn't reflect mybugstemplate param

Categories

(Bugzilla :: Bugzilla-General, defect)

Product:
Bugzilla
Component:
Bugzilla-General
Version:
2.17
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.16

People

(Reporter: m, Assigned: m)

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

Patch v5
2.23 KB, patch
jouni
: review+
gerv
: review+
Details | Diff | Splinter Review 
If you change the 'mybugstemplate' parameter, this change is reflected on the
front page, but not in the footer. It turns out that the 'My Bugs' query is
hard-coded into the footer. The footer should instead read the query from the
'mybugstemplate' parameter.

This can be fixed by changing line 101 of file
'template/en/default/global/useful-links.html.tmpl' from:

          <a
href="buglist.cgi?bug_status=NEW&amp;bug_status=ASSIGNED&amp;bug_status=REOPENED&amp;email1=[%
user.login FILTER url_quote
%]&amp;emailtype1=exact&amp;emailassigned_to1=1&amp;emailreporter1=1">My&nbsp;Bugs</a>

to:

          <a href="[% mybugstemplate %]">My&nbsp;Bugs</a>

i.e. remove the hard-coding of the 'My Bugs' query.
Changing component - I probably didn't mean 'User Interface'. Sorry.
Assignee: myk → justdave
Component: User Interface → Bugzilla-General
Hardware: PC → All
Umm, Gerv?
Keywords: regression
Yeah, this was on purpose. mybugstemplate is another param, like headerhtml,
that was there because we didn't have templates. There's another bug open
somewhere to make it just a stored query you get by default, which is the real
way to go, but in the mean time, we should remove the param and fix
index.html.tmpl to hard-code it as well, or remove it. The front page how has a
footer, after all.

However, the simple 2.16 fix is probably to go back to the param. That's the
least destructive option.

Thoughts?

Gerv
I'd

a) remove my bugs link from the index page (because there is the footer)
  and
b) go back to using the param until we've got a proper system (bug 69000)

There's no sense in killing existing params if we don't have at least equal
configuration options available. At the moment, I can't see such.
I agree. Currently the 'My Bugs' query must be changed in two places, simply 
because there are two links to it on one page. The param should be used 
until 'My Bugs' is an editable stored query. It's not a huge change to use the 
param, after all.
OK, let's go with Jouni's idea.

Gerv
Gerv / Malcolm: are you planning to do a patch for this? 

Marking 2.16.
Target Milestone: --- → Bugzilla 2.16
Never done a patch before, but willing to learn if someone points me in the
direction of a how-to...
Attached patch First patch (obsolete) — — Splinter Review 
Patch to make two changes as suggested by Jouni.

This being my first patch, I've almost certainly done something wrong ;-) If I
have, tell me, and I'll do something about it.

Note that my first comment was incorrect - the change to useful-links.html.tmpl
had to be slightly more than I suggested. See patch.
On the subject of index.cgi, there seem to be other links in there which are
duplicated on the footer, e.g. Log [in|out], Change prefs, Query, etc.. If we're
removing My Bugs from the page because it's duplicated, what should happen to
the rest? Or is that an issue for a separate bug?
reassign -> patch author, relevant keywords added. 
I'll try to review & test soon.
Assignee: justdave → m
Keywords: patch, review
>On the subject of index.cgi, there seem to be other links in there which are
>duplicated on the footer, e.g. Log [in|out], Change prefs, Query, etc.. If we're
>removing My Bugs from the page because it's duplicated, what should happen to
>the rest? Or is that an issue for a separate bug?

I think the index page should be an administrator configurable view to the
contents of Bugzilla (channels design et al.) - there is a bug on this, its
13xxxx something. :-) Currently it's quite hard to do things like this, so the
links are probably the only feasible content (but of course anyone can customize
it to their own liking).

Also, the front page should be easy to approach even for the beginner; thus
replicating the basic links could be cool (note that my bugs isn't generally
needed by a bugzilla first-timer). So I'd go for leaving it as is now and
reconsidering when front page redesign has landed and we get some user experience.

However, if you have some proposals with arguments to support them, feel free to
file a separate bug.

Comment on attachment 88423 [details] [diff] [review]
First patch

Ok, this fixes up the front page but wastes all the others :-)
You can't use the subst hash in useful-links because no cgi
passes any reasonable values for that (apart from index).

This got a bit more messy than I thought, because we need to move
the %userid% subst handling from index.cgi to quietly_check_login(). 

Gerv, what's your opinion on this? Should qcl push a %userid% 
subst variable to %vars or should useful-links be hacked to use 
the existing user.* template var?
Attachment #88423 - Flags: review-
This is why this param sucks :-)

The correct way to do it is probably:

<a href="[% Param('mybugstemplate').replace("%userid%", user.login) %]">My Bugs</a>

(that's pseudocode - you may need to mess with it quite a bit to get it to work,
but it's the correct approach.)

Gerv
><a href="[% Param('mybugstemplate').replace("%userid%", user.login) %]">
>My Bugs</a>

I was thinking about preserving the subst hash for further possible
replacements, but yeah, you're right. Your proposal is much simpler and easier
to do, and we don't need the expandability from the subst hash anymore. I hope
the proper solution will land sooner than anyone wants another replacement param
there. :-)

So, um.

Why don't we just remove the param, and make people edit the template? Thats the
only place its in, isn't it?

Then we can let it be stored in teh db when its stored in the db.
>Why don't we just remove the param, and make people edit the template? Thats the
>only place its in, isn't it?

I wouldn't do this because useful-links template is simply not the correct place
for configuring something like the user's default query. 

Technically you're right, but conceptually the solution of making administrators
configure their system through templates is odd. Templates are IMO user
interface things, not for configuration of functionality. See a similar
discussion in bug 143650 comment 10 and forward.

Jouni: this is not the user's "default query" - they configure that themselves
though the query interface. It's just a random link to a query that someone once
thought everyone would find useful. And, as such, a template is a perfectly
reasonable place to configure it.

Gerv
Attached patch Patch v2 (obsolete) — — Splinter Review 
How about this approach...

<a href="[% PerformSubsts(Param('mybugstemplate'), {'userid' => user.login})
%]">My&nbsp;Bugs</a>

This seems to work OK for me. Comments?
Attachment #88423 - Attachment is obsolete: true
...or should that be "user.login FILTER url_quote" as in the original footer code?
Yes, it should. :-)

Gerv
Attached patch Patch v3 (obsolete) — — Splinter Review 
Third time lucky (hopefully) :-)

Changed so the e-mail address is filtered through url_quote.
Attachment #88663 - Attachment is obsolete: true
>Jouni: this is not the user's "default query" - they configure that themselves
>though the query interface. It's just a random link to a query that someone once
>thought everyone would find useful. And, as such, a template is a perfectly
>reasonable place to configure it.

I don't think it's "just a random link", it's a feature that has been there for
quite some time and it has a role in the Bugzilla world plus a configuration
interface people are used to. If it were a new feature, it could well be in the
template. Now that we have this param, I think we can use it until we've got the
proper solution available.

But you're right, it's certainly not the default query. My fingers were faster
than my brain. :-) 
Comment on attachment 88716 [details] [diff] [review]
Patch v3

The patch should also remove the line that sets $vars->{'subst'} in index.cgi,
since it's no longer needed after the template starts accessing userid
directly.

>+          [% filtered_username = user.login FILTER url_quote %]

Why the temporary variable here?
Attached patch Patch v4 (obsolete) — — Splinter Review 
Patch with additional change to index.cgi as suggested by Jouni.

> Why the temporary variable here?
I had used it because in this situation, "user.login FILTER url_quote" failed
with "unexpected token (FILTER)". Turns out "url_quote(user.login)" works, so
no need for the temporary variable in this patch.
Attachment #88716 - Attachment is obsolete: true
The fact that the filter is exposed as a function is probably an implemention
detail. (Or, more probably, that the filter is _callable_ as a function directly
is the detail. I think. Anyone know for sure?) If you do this, you need to use a
temp var because of the way the TT grammar is defined.

Why use performSubsts, instead of replace? Its not like extra variables cost us
anything measurable, and performSubsts is probably slower by the same
almost-unmeasurable ammount. Then you could remove that line from globals.pl, too
The url_quote function is defined in CGI.pl - it's not part of the Template
Toolkit. Anyway, I'll use a temporary variable and a FILTER if that's what you want.

The only reason for using PerformSubsts rather than replace is for consistency
with other params. PerformSubsts will let you substitute other params, not just
%userid%. E.g. you could use %urlbase% etc. in the My Bugs link.

Comments on which we should use, before I make another patch?
>Comments on which we should use, before I make another patch?

I'm fine with just using replace and a temp variable, but don't know about
bbaetz and gerv.

"The url_quote function is defined in CGI.pl - it's not part of the Template
Toolkit."

Yes, but its only exposed to TT as a filter, unless global funcs are somehow
being pulled in, which would be odd.

I'm happy with replace + a temp var, too. The only other places to use
performsubst are the untemplatised bugmail stuff, sidebar.cgi (to do exactly
what this is doing here), and showdependancygraph.cgi (to handle the urlbase)

The dependancygraph stuff can be fixed by just replacing the single variable, so
lets just use replace.

gerv?
Attached patch Patch v5 — — Splinter Review 
Here goes another patch... This time I'm using replace with a temporary
variable.

Is there a chance I've got it right this time? ;-)
Attachment #88762 - Attachment is obsolete: true
Comment on attachment 88782 [details] [diff] [review]
Patch v5

The patch works and I'm (finally? :-)) satisfied with the design -> r=jouni

I believe we should wait for Gerv to either r= this or request more changes.
Attachment #88782 - Flags: review+
Comment on attachment 88782 [details] [diff] [review]
Patch v5

I'm happy with this too, but lets wait for gerv
Comment on attachment 88782 [details] [diff] [review]
Patch v5

r=gerv.

Gerv
Attachment #88782 - Flags: review+
Umm, OK, what happens next? Since this bug is assigned to me, I suspect I've got
to do something to get this patch committed into the tree. And since this is my
first patch, I don't know what I'm supposed to do... :-)
According to the review guide on bugzilla.org, 'If the bug owner has checkin
authority, they should check the patch in, otherwise the reviewer who adds the
"second-review" attachment status should do this.  If the reviewer can't, anyone
else who can should.'

So theoretically, what you should do is to bug Gerv about this. But he's got his
hands full of stuff anyway, so I might just as well do it now. 
Checked in, trunk and branch:

Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.5; previous revision: 1.4
done
Checking in template/en/default/index.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/index.html.tmpl,v  <-- 
index.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v
 <--  useful-links.html.tmpl
new revision: 1.5; previous revision: 1.4
done

Checking in index.cgi;
/cvsroot/mozilla/webtools/bugzilla/index.cgi,v  <--  index.cgi
new revision: 1.4.2.1; previous revision: 1.4
done
Checking in template/en/default/index.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/index.html.tmpl,v  <-- 
index.html.tmpl
new revision: 1.4.2.2; previous revision: 1.4.2.1
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v
 <--  useful-links.html.tmpl
new revision: 1.2.2.3; previous revision: 1.2.2.2
done


Thanks for the patch and bearing with our conflicting reviews, Malcolm :-)
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Yes, Malcolm - thanks for the patch. I think the guide should say that if the
person doesn't have checkin privs, anyone with a spare five minutes should check
it in. :-)

Gerv
I agree with you on your proposal about the new wording in the guide. Filed bug
153814 on that.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: