Scheduled whining needs custom columns

RESOLVED FIXED in Bugzilla 4.4

Status

()

Bugzilla
Whining
P1
enhancement
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Erik Stambaugh, Assigned: mrbball)

Tracking

unspecified
Bugzilla 4.4
Dependency tree / graph
Bug Flags:
approval +
approval4.4 +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Updated

13 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Bugzilla 2.20
Component: Email Notifications → Whining
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.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
(Reporter)

Comment 2

12 years ago
*** Bug 277708 has been marked as a duplicate of this bug. ***
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).
Assignee: erik → robzilla

Updated

12 years ago
Attachment #199964 - Flags: review?

Updated

12 years ago
Whiteboard: patch awaiting review

Comment 4

12 years ago
too late for 2.22
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24

Comment 5

12 years ago
It would be great if whine mails could send "Full Text Bug Listing" of bugs that match query.
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.
Attachment #199964 - Flags: review? → review-
mkanat, any ideas about the module namespace this should use?

Comment 8

12 years ago
(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".
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.
Whiteboard: patch awaiting review

Updated

11 years ago
QA Contact: mattyt-bugzilla → default-qa

Comment 10

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

Comment 11

11 years ago
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".
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2

Comment 12

11 years ago
Untargeting this bug until it sees some action.
Target Milestone: Bugzilla 3.2 → ---

Comment 13

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

Comment 14

11 years ago
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

11 years ago
(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.

Updated

10 years ago
Duplicate of this bug: 376293

Comment 17

10 years ago
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.
Assignee: robzilla2 → whining
Status: ASSIGNED → NEW

Updated

10 years ago
Priority: P3 → P1

Comment 18

10 years ago
(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.

Updated

9 years ago
Duplicate of this bug: 440727
Assignee: whining → wicked

Updated

9 years ago
Depends on: 344878

Updated

9 years ago
Depends on: 297382
No longer depends on: 297382

Updated

9 years ago
Duplicate of this bug: 478022

Comment 21

8 years ago
when will solve this problem?

Comment 22

7 years ago
I'm also in need of this fix, when is it possible to have another update since the last comment please?

Comment 23

7 years ago
Come on, can't be that difficult.

Comment 24

7 years ago
(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

5 years ago
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.
Attachment #199964 - Attachment is obsolete: true
Attachment #650703 - Flags: review?(wicked)

Comment 26

5 years ago
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
Attachment #650703 - Flags: review?(wicked) → review-

Updated

5 years ago
Assignee: wicked → altlist
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 4.4
(Assignee)

Comment 27

5 years ago
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 ...
Attachment #663325 - Flags: review?(LpSolit)
(Assignee)

Comment 28

5 years ago
Created attachment 663327 [details] [diff] [review]
v3

Ack, the previous attachment was the wrong version.  This one is correct.
Attachment #663325 - Attachment is obsolete: true
Attachment #663325 - Flags: review?(LpSolit)
Attachment #663327 - Flags: review?(LpSolit)

Comment 29

5 years ago
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.
(Assignee)

Comment 30

5 years ago
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.

Updated

5 years ago
Attachment #650703 - Attachment is obsolete: true

Comment 31

5 years ago
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
Attachment #663327 - Flags: review?(LpSolit) → review+

Comment 32

5 years ago
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! :)
Assignee: altlist → kar
Flags: approval4.4+
Flags: approval+
Keywords: relnote

Comment 33

5 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 34

5 years ago
Added to relnotes for 4.4.
Keywords: relnote
(Assignee)

Comment 35

5 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 36

5 years ago
short_short_desc is already in field-descs.none.tmpl.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 37

5 years ago
Yes it is!  I didn't have that change in my tree yet.  Sorry! :-o
You need to log in before you can comment on or make changes to this bug.