245375 - Scheduled whining needs custom columns

Status: RESOLVED FIXED

(Bugzilla :: Whining, enhancement, P1)

Description

[Erik Stambaugh]

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]]

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]

*** Bug 277708 has been marked as a duplicate of this bug. ***

Comment 3

[Rob Siklos [:robzilla]]

Attachment: 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]

too late for 2.22

Comment 5

[Petri T. Koistinen]

It would be great if whine mails could send "Full Text Bug Listing" of bugs that match query.

Comment 6

[Teemu Mannermaa (:wicked)]

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)]

mkanat, any ideas about the module namespace this should use?

Comment 8

[Max Kanat-Alexander]

(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]]

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]

*** Bug 344658 has been marked as a duplicate of this bug. ***

Comment 11

[Frédéric Buclin]

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]

Untargeting this bug until it sees some action.

Comment 13

[Max Kanat-Alexander]

*** Bug 359463 has been marked as a duplicate of this bug. ***

Comment 14

[Susan]

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]

(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 17

[Kevin Benton]

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]

(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 21

[zb]

when will solve this problem？

Comment 22

[Kevin Woodin]

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]

Come on, can't be that difficult.

Comment 24

[Max Kanat-Alexander]

(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]

Attachment: 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]

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]]

Attachment: 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]]

Attachment: v3

Ack, the previous attachment was the wrong version.  This one is correct.

Comment 29

[Albert Ting]

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]]

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]

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]

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]

Committing to: bzr+ssh://[e-mail]/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://[e-mail]/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]

Added to relnotes for 4.4.

Comment 35

[Kent Rogers [:mrbball]]

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]

short_short_desc is already in field-descs.none.tmpl.

Comment 37

[Kent Rogers [:mrbball]]

Yes it is!  I didn't have that change in my tree yet.  Sorry! :-o