487106 - Add explanation when a whining is empty

Status: RESOLVED FIXED

(Bugzilla :: Whining, enhancement, P1)

Description

[Teemu Mannermaa (:wicked)]

Ever since bug 302420 whining message can be empty if there are no queries defined or the queries result with no bugs. There's no text to explain this on the received whine so it might be hard to know why such message was sent. We should probably add explanation texts to the whine to ease confusion. See bug 302420 comment 27 for the review comment that's the basis for this followup bug.

Comment 1

[Edmund Wong (:ewong)]

Attachment: Added explanation text for empty queries for whine e-mails

Comment 2

[Edmund Wong (:ewong)]

Attachment: Added explanation text for empty queries for whine e-mails (v2)

Overwriting the wrong patch.

Comment 3

[Edmund Wong (:ewong)]

Attachment: Added explanation text for empty queries for whine e-mails (v3)

With the diff option of ignoring whitespace changes.

Comment 4

[Justin Wood (:Callek)]

Comment on attachment 444816 [details] [diff] [review]
Added explanation text for empty queries for whine e-mails (v3)

the patch is more wording review than code-review; the template IF-stuff looks good, the choice of wording/using a table in the html.tmpl I'd rather leave to mkanat or LpSolit

In my opinion, the word choice is good (I think it *could* be better; but I have no idea of a better suggestion) and in the html.tmpl I think it might make more sense to do <h2> or <h3> instead of a table for the "no results" case

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 444816 [details] [diff] [review]
Added explanation text for empty queries for whine e-mails (v3)

Sudden thought:

"No Bugs were found that match the search criteria."

Or more specifically:

"No [% terms.bugs %] were found that match the search criteria."

I'd feel safe with this, with the html.tmpl thing I mentioned fixed, and if mkanat is ok with it, I can do the final review.

Comment 6

[Edmund Wong (:ewong)]

Attachment: Added explanation text for empty queries for whine e-mails (v4)

Changed the table format to <h3> for the mail.html.tmpl, and then changed the
message as given by comment #5.

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 444821 [details] [diff] [review]
Added explanation text for empty queries for whine e-mails (v4)

r+ _assuming_ that this is a -w diff.

Please attach the real diff here [porting r+], to ease c-n when whomever gets around to it.

Comment 8

[Edmund Wong (:ewong)]

Attachment: Added explanation text for empty queries for whine e-mails (v5)

Comment 9

[Frédéric Buclin]

The indentation in templates is 2 whitespaces, not one. Please re-attach a patch.

Comment 10

[Edmund Wong (:ewong)]

Attachment: Added explanation text for empty queries for whine e-mails (v6)

Changed indentation.

Comment 11

[Frédéric Buclin]

Comment on attachment 444870 [details] [diff] [review]
Added explanation text for empty queries for whine e-mails (v6)

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

>     [% FOREACH bug=query.bugs %]
>       <tr>
>@@ -84,8 +84,11 @@
>       </tr>
>     [% END %]
>   </table>
>+  [% END %]

The indentation is not complete.


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

>+  [%+ query.title +%]
>+  [%+ "-" FILTER repeat(query.title.length) %]
>+
>+  [% FOREACH bug=query.bugs %]
>+    [% terms.Bug +%] [%+ bug.bug_id %]:

Be careful about the indentation used in .txt.tmpl templates. Here, the indentation matters as they are not HTML templates. So we should only indent lines when that's what we really want in the output (my previous comment was about .html.tmpl templates, which are more frequent).


>+              [%- IF bug.resolution -%] Resolution: [% display_value("resolution", bug.resolution) -%]
>+                                 [%- END %]

Could someone tell me why the [% END %] has this indentation??


>+     Summary: [% bug.short_desc %]
>+
>+   [% END %]
>+
>+  [% END %]

There seems to be an extra [% END %].


Please re-request review for the updated patch as the indentation matters in .txt.tmpl templates, and is not easily fixable on checkin.

Comment 12

[Edmund Wong (:ewong)]

(In reply to comment #11)
> (From update of attachment 444870 [details] [diff] [review])
> >=== modified file 'template/en/default/whine/mail.html.tmpl'
> 
> >     [% FOREACH bug=query.bugs %]
> >       <tr>
> >@@ -84,8 +84,11 @@
> >       </tr>
> >     [% END %]
> >   </table>
> >+  [% END %]
> 
> The indentation is not complete.

I'll get this fixed.
> >+  [%+ query.title +%]
> >+  [%+ "-" FILTER repeat(query.title.length) %]
> >+
> >+  [% FOREACH bug=query.bugs %]
> >+    [% terms.Bug +%] [%+ bug.bug_id %]:
> 
> Be careful about the indentation used in .txt.tmpl templates. Here, the
> indentation matters as they are not HTML templates. So we should only indent
> lines when that's what we really want in the output (my previous comment was
> about .html.tmpl templates, which are more frequent).
>
Understood. 
 
> 
> >+              [%- IF bug.resolution -%] Resolution: [% display_value("resolution", bug.resolution) -%]
> >+                                 [%- END %]
> 
> Could someone tell me why the [% END %] has this indentation??
>
While I found it like that, I should have had the forethought
to normalize it. 
 
> 
> >+     Summary: [% bug.short_desc %]
> >+
> >+   [% END %]
> >+
> >+  [% END %]
> 
> There seems to be an extra [% END %].
>
Actually, there isn't unless I'm mistaken.  The first [% END %] is 
for the inside FOREACH loop and the second [% END %] is for the outer 
FOREACH loop(the one below the [% IF queries.size > 0 %].  
 
> 
> Please re-request review for the updated patch as the indentation matters in
> .txt.tmpl templates, and is not easily fixable on checkin.

Will do.

Thanks

Comment 13

[Edmund Wong (:ewong)]

Attachment: Added explanation text for empty queries for whine e-mails (v7)

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 444881 [details] [diff] [review]
Added explanation text for empty queries for whine e-mails (v7)

>-[%+ query.title +%]
>-[%+ "-" FILTER repeat(query.title.length) %]
>+  [%+ query.title +%]
>+  [%+ "-" FILTER repeat(query.title.length) %]

Lets not indent these. Followup review to LpSolit and this should be good to go.

(Sorry to you and him for not remembering that .txt needs whitespace review love here.)

Comment 15

[Edmund Wong (:ewong)]

Attachment: Added explanation text for empty queries for whine e-mails (v8)

Comment 16

[Frédéric Buclin]

Comment on attachment 445039 [details] [diff] [review]
Added explanation text for empty queries for whine e-mails (v8)

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

>+[% IF queries.size > 0 %]

Nit: "> 0" is useless as queries.size will be true when not empty.


>     [% FOREACH bug=query.bugs %]

The indentation of this FOREACH loop must still be fixed, as requested in comment 11. I will fix that on checkin.


>+  <h3>No [% terms.bugs %] were found that matched the search criteria.</h3>

Nit: I'm not sure putting the comment in <h3></h3> is appropriate. <b></b> would probably better.



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

>+[% IF queries.size > 0 %]

Nit: same comment about "> 0".


Otherwise looks good and works fine. r=LpSolit

Comment 17

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified template/en/default/whine/mail.html.tmpl
modified template/en/default/whine/mail.txt.tmpl
Committed revision 7187.

Comment 18

[Max Kanat-Alexander]

Added to the release notes in bug 604256.