Last Comment Bug 245375 - Scheduled whining needs custom columns
: Scheduled whining needs custom columns
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Whining (show other bugs)
: unspecified
: All All
: P1 enhancement with 18 votes (vote)
: Bugzilla 4.4
Assigned To: Kent Rogers [:mrbball]
: default-qa
Mentors:
: 277708 344658 359463 376293 440727 478022 (view as bug list)
Depends on: 185090 344878
Blocks:
  Show dependency treegraph
 
Reported: 2004-06-02 11:51 PDT by Erik Stambaugh
Modified: 2013-02-07 02:42 PST (History)
28 users (show)
LpSolit: approval+
LpSolit: approval4.4+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
robzilla_v1 (40.81 KB, patch)
2005-10-18 12:21 PDT, Rob Siklos [:robzilla]
wicked: review-
Details | Diff | Review
v2 (3.46 KB, patch)
2012-08-09 15:41 PDT, Albert Ting
LpSolit: review-
Details | Diff | Review
v3 (4.05 KB, patch)
2012-09-21 01:38 PDT, Kent Rogers [:mrbball]
no flags Details | Diff | Review
v3 (5.09 KB, patch)
2012-09-21 02:10 PDT, Kent Rogers [:mrbball]
LpSolit: review+
Details | Diff | Review

Description Erik Stambaugh 2004-06-02 11:51:53 PDT
There should be a way for the new whine scheduler to specify which columns are
listed in a message.

This could get a little bit tricky, and isn't critical to landing the scheduled
whines, so I'm putting it in its own bug.
Comment 1 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-09-18 17:37:38 PDT
Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.
Comment 2 Erik Stambaugh 2005-03-23 15:09:13 PST
*** Bug 277708 has been marked as a duplicate of this bug. ***
Comment 3 Rob Siklos [:robzilla] 2005-10-18 12:21:28 PDT
Created attachment 199964 [details] [diff] [review]
robzilla_v1

patch attached.  

Moved column definition, display column determiniation, select column
determination, and sort order determination into Search.pm, and used those
functions in whine.pl to generate the search.

To see the custom columns, the "columnlist" cgi parameter needs to be in the
saved search URL (but there is no interface for this yet).
Comment 4 Frédéric Buclin 2005-11-17 03:40:10 PST
too late for 2.22
Comment 5 Petri T. Koistinen 2006-02-03 05:21:21 PST
It would be great if whine mails could send "Full Text Bug Listing" of bugs that match query.
Comment 6 Teemu Mannermaa (:wicked) 2006-03-09 01:26:19 PST
Comment on attachment 199964 [details] [diff] [review]
robzilla_v1

Cool patch, unfortunately it has bitrotten because of changes to buglist.

Few general comments for an updated patch:
1) Maybe DefineColumn sub should be a private one? This means it should start with a _, if I'm not mistaken.
2) Search.pm is already very long and adding these subs there make it even longer. Maybe we should instead add them to some sub namespace, like Bugzilla::Search::Columns  or something like that.
Comment 7 Teemu Mannermaa (:wicked) 2006-03-09 01:27:22 PST
mkanat, any ideas about the module namespace this should use?
Comment 8 Max Kanat-Alexander 2006-03-09 04:19:42 PST
(In reply to comment #7)
> mkanat, any ideas about the module namespace this should use?

  Yeah. The whole thing should be one Bugzilla::Search::ColumnList object. It shouldn't be a module with subroutines, it should be an object that you can pass into "new Bugzilla::Search".
Comment 9 Rob Siklos [:robzilla] 2006-03-09 08:24:59 PST
I don't know when I'll get the time to update the patch, so if someone else wants to take it, go right ahead.
Comment 10 Frédéric Buclin 2006-07-24 14:23:14 PDT
*** Bug 344658 has been marked as a duplicate of this bug. ***
Comment 11 Frédéric Buclin 2006-10-19 12:11:07 PDT
This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".
Comment 12 Max Kanat-Alexander 2006-10-19 12:20:06 PDT
Untargeting this bug until it sees some action.
Comment 13 Max Kanat-Alexander 2006-11-04 02:52:05 PST
*** Bug 359463 has been marked as a duplicate of this bug. ***
Comment 14 Susan 2006-11-04 07:45:01 PST
Fixing this bug would improve our work team's production as it will help management in notification of bug status. I tried setting the flag to "yes" for blocking 3.0 to have someone continue to work this bug but the flag failed to post because I do not have the permission to post a flag, to your bug.(In reply to comment #11)
> This bug is retargetted to Bugzilla 3.2 for one of the following reasons:
> - it has no assignee (except the default one)
> - we don't expect someone to fix it in the next two weeks (i.e. before we
> freeze the trunk to prepare Bugzilla 3.0 RC1)
> - it's not a blocker
> If you are working on this bug and you think you will be able to submit a patch
> in the next two weeks, retarget this bug to 3.0.
> If this bug is something you would like to see implemented in 3.0 but you are
> not a developer or you don't think you will be able to fix this bug yourself in
> the next two weeks, please *do not* retarget this bug.
> If you think this bug should absolutely be fixed before we release 3.0, either
> ask on IRC or use the "blocking3.0 flag".

Comment 15 Frédéric Buclin 2006-11-04 07:54:13 PST
(In reply to comment #14)
> I tried setting the flag to "yes" for
> blocking 3.0 to have someone continue to work this bug but the flag failed to
> post because I do not have the permission to post a flag, to your bug.

That's because we don't let you decide whether this bug should block 3.0 or not. Normal users can only do requests (blocking3.0?). Anyway, the two weeks deadline is now over, meaning this enhancement will definitely not go into Bugzilla 3.0.
Comment 16 Frédéric Buclin 2007-04-03 12:38:22 PDT
*** Bug 376293 has been marked as a duplicate of this bug. ***
Comment 17 Kevin Benton 2008-01-16 10:04:16 PST
I agree completely that having custom format capability with whining would be fantastic.  If nothing else, I think it's important to see a last changed date as part of the columns so readers can see when something changed versus their last whine.

Rob - if you're still working on this issue, would you mind updating?  I'm reassigning to default because your last patch is more than two years old.
Comment 18 David Coleman 2008-02-21 02:41:16 PST
(In reply to comment #5)
> It would be great if whine mails could send "Full Text Bug Listing" of bugs
> that match query.

i second this; would be great to be able to send a "Full Text Bug Listing" to someone who is not in the office.
Comment 19 Max Kanat-Alexander 2008-06-21 04:08:23 PDT
*** Bug 440727 has been marked as a duplicate of this bug. ***
Comment 20 Frédéric Buclin 2009-02-11 07:35:47 PST
*** Bug 478022 has been marked as a duplicate of this bug. ***
Comment 21 zb 2009-12-10 18:45:59 PST
when will solve this problem?
Comment 22 Kevin Woodin 2010-07-09 07:26:55 PDT
I'm also in need of this fix, when is it possible to have another update since the last comment please?
Comment 23 Alon Bar-Lev 2011-01-19 11:37:52 PST
Come on, can't be that difficult.
Comment 24 Max Kanat-Alexander 2011-01-19 15:25:51 PST
(In reply to comment #23)
> Come on, can't be that difficult.

  Patches welcome:

  http://wiki.mozilla.org/Bugzilla:Developers

  Alternately, you're welcome to hire somebody to work on this. None of us get paid to work on Bugzilla.

  http://www.bugzilla.org/support/consulting.html
Comment 25 Albert Ting 2012-08-09 15:41:30 PDT
Created attachment 650703 [details] [diff] [review]
v2

Instead of supporting just custom columns, I think a better solution is to take the column list as defined in the saved query.  This also addresses supporting custom fields.

Attached is a patch that I think goes in the right direction.

One issue I see is that the abbreviation table is currently hardcoded in list/table.html.tmpl.  Instead, this should be defined in perhaps fielddefs so that it can be reused.  One could then define abbreviations through the editfields admin page.
Comment 26 Frédéric Buclin 2012-08-18 03:44:24 PDT
Comment on attachment 650703 [details] [diff] [review]
v2

>+++ whine.pl	2012-08-09 13:46:36.746252000 -0700

>+            push @searchfields, "bug_id"; 

Did you make sure bug_id is not already in @searchfields? Duplicating columns is going to make Oracle unhappy.


>+            $bug->{columnlist} = \@searchfields;

You should rather store the column list as part of $thisquery as all bugs belonging to the same query will have the same columns.



>+++ template/en/default/whine/mail.txt.tmpl	2012-08-09 15:33:58.068348000 -0700

>+  [% FOREACH col=bug.columnlist %]
>+    [% IF col.length > padding; padding = col.length; END %]

You should look at the length of field_descs.$col, not $col. Also, write this as:

  [% padding = field_descs.${col}.length IF field_descs.${col}.length > padding %]


Otherwise your patch looks good.


Note that your patch doesn't apply cleanly on the current code (4.3.2+):

Hunk #1 FAILED at 47.
1 out of 1 hunk FAILED -- saving rejects to file template/en/default/whine/mail.txt.tmpl.rej
patching file template/en/default/whine/mail.html.tmpl
Hunk #1 FAILED at 61.
1 out of 1 hunk FAILED -- saving rejects to file template/en/default/whine/mail.html.tmpl.rej
Comment 27 Kent Rogers [:mrbball] 2012-09-21 01:38:14 PDT
Created attachment 663325 [details] [diff] [review]
v3

I'm very interested in seeing this bug resolved and it seemed like only a
few tweaks to the previous patch were necessary.  Here is a new version
with the changes that were suggested, although the third suggestion didn't
work for me so I stuck with the original logic (although I switched the
variable name).

I hope I'm not stepping on Albert's toes by submitting this.  It just
seemed so close ...
Comment 28 Kent Rogers [:mrbball] 2012-09-21 02:10:52 PDT
Created attachment 663327 [details] [diff] [review]
v3

Ack, the previous attachment was the wrong version.  This one is correct.
Comment 29 Albert Ting 2012-09-21 13:22:45 PDT
Thanks Kent.  Glad that you can help! Feel free to take over.

I still think the abbreviation table in list/table.html.tmpl should be moved to a more central place so that it can be used in this patch.  Although that may be a separate ticket.  There is likely an additional patch needed to support custom abbreviations via the custom field admin page.
Comment 30 Kent Rogers [:mrbball] 2012-09-21 14:09:24 PDT
I thought about it and felt that not having abbreviations in the email shouldn't stop
this bug.  Moving the abbreviation table to a more central place is probably worth
considering, but I think it is a separate bug.
Comment 31 Frédéric Buclin 2012-10-13 05:30:30 PDT
Comment on attachment 663327 [details] [diff] [review]
v3

>=== modified file 'template/en/default/whine/mail.html.tmpl'

>+        [% FOREACH col=query.columnlist %]

Nit: add whitespaces around "=".



>=== modified file 'template/en/default/whine/mail.txt.tmpl'

>+
>+  [% largest_title = 0 %]

Don't add an empty line, else it will appear in the email.


>+  [% FOREACH col = query.columnlist %]
>+      [% NEXT IF col == 'bug_id' %]
>+      [% IF field_descs.${col}.length > largest_title %]
>+          [% largest_title = field_descs.${col}.length %]

The indentation is 2 characters in templates. Also, as it's a plain text email, the indentation matters in the output.

Also, both templates miss 'columnlist' in their INTERFACE section at the top of the template.



>=== modified file 'whine.pl'

>+        if (defined $searchparams->param('columnlist')) {

'defined' is not needed. If it's defined but empty, we should ignore it.


I tested your patch and it works great. I will fix the comments above on checkin. Thanks a lot for your patch! r=LpSolit
Comment 32 Frédéric Buclin 2012-10-13 05:33:36 PDT
Reassigning the bug to Kent as his patch is the one which is going to be checked in, but I will mention both Kent and Albert as authors in the commit message.

Just on time for Bugzilla 4.4! :)
Comment 33 Frédéric Buclin 2012-10-13 05:40:47 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified whine.pl
modified template/en/default/whine/mail.html.tmpl
modified template/en/default/whine/mail.txt.tmpl
Committed revision 8426.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.4/
modified whine.pl
modified template/en/default/whine/mail.html.tmpl
modified template/en/default/whine/mail.txt.tmpl
Committed revision 8419.
Comment 34 Frédéric Buclin 2012-11-01 12:24:19 PDT
Added to relnotes for 4.4.
Comment 35 Kent Rogers [:mrbball] 2012-11-07 12:34:49 PST
The change to whine/mail.txt.tmpl has problems on saved searches where
'short_short_desc' is one of the columns.  I see these errors:

Argument "" isn't numeric in numeric gt (>) at
template/en/default/whine/mail.txt.tmpl line 58, <DATA> line 522.
Argument "" isn't numeric in subtraction (-) at
template/en/default/whine/mail.txt.tmpl line 66, <DATA> line 522.
  .
  .

I believe this is due to 'short_short_desc' (i.e. first 60 characters) not being one
of the fields in vars.field_descs in global/field-descs.none.tmpl.

What is the proper way to fix this?

1. Add 'short_short_desc' to vars.field_descs definition in global/field-descs.none.tmpl
2. Define field_descs.short_short_desc in whine/mail.txt.tmpl (similar to how it is in
   list/change-columns.html.tmpl)

Both appear to work.
Comment 36 Frédéric Buclin 2012-11-07 12:54:16 PST
short_short_desc is already in field-descs.none.tmpl.
Comment 37 Kent Rogers [:mrbball] 2012-11-07 13:02:42 PST
Yes it is!  I didn't have that change in my tree yet.  Sorry! :-o

Note You need to log in before you can comment on or make changes to this bug.