296887 - Do not display the quip entry field when quip_list_entry_control is set to 'closed'

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[A. Karl Kornel]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en) AppleWebKit/412 (KHTML, like Gecko) Safari/412
Build Identifier: 

When quips are turned off, an error message is presented when a user loads up quips.cgi, which is not 
surprising.  When quips are turned on and set to 'approved', the text field used for entering quips is 
displayed.

When quips are turned on and set to 'frozen', the text field and bonking button are still displayed.  The 
unsuspecting user is presented with an error only after a new quip is submitted.  Would it be OK to 
update either quips.cgi or (more likely) the default/list/quips.html template to say something 
appropriate if quip entry is frozen?

Reproducible: Always

Steps to Reproduce:
1. In an installation of Bugzilla 2.18.1, access parameters and change enablequips to 'frozen'.
2. Go to quips.cgi in said installation
Actual Results:  
I was shown a prompt to enter a quip.  I did, and I was told that the site does not permit addition of new 
quips.

Expected Results:  
Instead of being given an opportunity to enter a quip, I should have been told that no new quips are being 
accepted for this site.

The URL points to my installation of Bugzilla 2.18.1, which I'm in the process of setting up.  To be specific, 
the URL points to the quips page.  

Use '[e-mail]' as the login name and 'user' as the password to access the page as a non-priviledged 
user for purposes of checking out the bug.

Comment 1

[A. Karl Kornel]

Attachment: Patch on top of 2.18.1

At this point I'm assuming this bug is valid.

The patch modifies template/en/default/list/quips.html.tmpl on an installation
of Bugzilla 2.18.1.  Expect a patch for tip to appear soon.

NOTE: The URL points to an installation of Bugzilla 2.18.1 with this patch
applied (and quips set to frozen as of the time this patch was submitted).

Comment 2

[A. Karl Kornel]

Attachment: Patch on top of tip

Does the same as attachment 186035 [details] [diff] [review], except this time it patches
template/en/default/list/quips.html.tmpl on tip.

NOTE: I am not currently available to completely test this, but can if needed.

Comment 3

[Frédéric Buclin]

Comment on attachment 186038 [details] [diff] [review]
Patch on top of tip

>+  [% ELSE %]. [%# The . must be left where it is, else space appears %]
>+              [%# 'twixt it and "list" %]
>+              [%# same goes for the comma 6 lines 'bove %]
>+  [% END %]

Such a hack for a single dot. :( There must be a simpler way to do this.
Moreover, the indentation is incorrect.

>+[% IF Param("quip_list_entry_control") == "closed" %]
>+New quips may not be entered.
>+[% ELSE %]

As you test 'quip_list_entry_control' here again, I would write a single block,
i.e. mixing this part with the previous one.

Comment 4

[A. Karl Kornel]

Comment on attachment 186035 [details] [diff] [review]
Patch on top of 2.18.1

Marking obsolete.  Review would be denied for reasons similar to those in
comment 3.  Once a modified form of attachment 186038 [details] [diff] [review] is submitted, I'll submit
a modified form of this attachment.

Comment 5

[A. Karl Kornel]

(In reply to comment #3)
> (From update of attachment 186038 [details] [diff] [review] [edit])
> >+  [% ELSE %]. [%# The . must be left where it is, else space appears %]
> >+              [%# 'twixt it and "list" %]
> >+              [%# same goes for the comma 6 lines 'bove %]
> >+  [% END %]
> 
> Such a hack for a single dot. :( There must be a simpler way to do this.
> Moreover, the indentation is incorrect.

There is a simpler way:  Changing the wording.  Modified wording a little bit in this patch, and 
eliminated an ELSE block.  The indentation of the comment blocks was for alignment of the related 
comments, but since they're gone it's no problem now.  The block you refer to above now looks like 
this:


<p>
  [% terms.Bugzilla %] will pick a random quip for the headline on each
  [% terms.bug %] list.
  [% IF Param("quip_list_entry_control") != "closed" %]
    You can also add to the quip list. Type in something clever or funny or
    boring (but not obscene or offensive, please) and bonk on the button.
    [% IF Param("quip_list_entry_control") == "moderated" AND !user.groups.admin %]
      Note that your quip has to be approved before it is used.
    [% END %]
  [% END %]
</p>


> >+[% IF Param("quip_list_entry_control") == "closed" %]
> >+New quips may not be entered.
> >+[% ELSE %]
> 
> As you test 'quip_list_entry_control' here again, I would write a single block,
> i.e. mixing this part with the previous one.
> 

I could be done, but it would result in the duplication of a lot of code, which could make future editing 
harder.  For example, here's one solution that incorporates the modification above and eliminates the 
extra IF block:


<p>
  [% terms.Bugzilla %] will pick a random quip for the headline on each
  [% terms.bug %] list.
  [% IF Param("quip_list_entry_control") != "closed" %]
    You can also add to the quip list. Type in something clever or funny or
    boring (but not obscene or offensive, please) and bonk on the button.
    [% IF Param("quip_list_entry_control") == "moderated" AND !user.groups.admin %]
      Note that your quip has to be approved before it is used.
    [% END %]
</p>
<form method="post" action="quips.cgi">
  <input type="hidden" name="action" value="add">
  <input size="80" name="quip">
  <p>
    <input type="submit" value="Add This Quip">
  </p>
</form>
  [% ELSE # Param("quip_list_entry_control") == "closed" %]
</p>
New quips may not be entered.
  [% END # Param("quip_list_entry_control") != "closed" %]


For all of the lines I moved around, I did not change the indenting.  In the alternative above, there is 
code & text appearing at levels above the IF block.  I could move most of it into a TemplateToolkit 
BLOCK, which takes the value of Param("quip_list_entry_control") as a parameter, but I would still need 
2 IF/ELSE blocks to maintain the readability of the original file (although these IF blocks would only be 
testing a parameter, not a function result).  I can do either one.

Comment 6

[A. Karl Kornel]

Attachment: Patch v2 for tip

modified version of attachment 186038 [details] [diff] [review] as per comment 3.  Again, patch is to be
made against Bugzilla tip.

Comment 7

[Frédéric Buclin]

Comment on attachment 186128 [details] [diff] [review]
Patch v2 for tip

>+[% ELSE %]
>   <p>
>-    <input type="submit" value="Add This Quip">
>+    [% terms.Bugzilla %] will pick a random quip for the headline on each 
>+    [% terms.bug %] list.
>   </p>
>-</form>
>+[% END %]

As this sentence will appear in both cases, move it before the IF block, so
that you don't need to duplicate it. What I suggested on IRC was:

<p>
  [% terms.Bugzilla %] will pick a random quip for the headline on each [%
terms.bug %] list.
</p>

[% IF Param() != "closed" %]
  <p>
    ... remaining text here.
  </p>

  <form>
    ...
  </form>
[% ELSE %]
  No new quips can be entered.
[% END %]

You want to prevent the user from adding new quips. So you have to mention it.
I cannot find it in your patch. ;)

Comment 8

[Frédéric Buclin]

I meant:

> [% ELSE %]
>   <p>No new quips can be entered.</p>
> [% END %]

I forgot <p></p> tags.

Comment 9

[A. Karl Kornel]

Attachment: Patch v3 for tip

Version 3.

> You want to prevent the user from adding new quips. So you have to mention
it.

Small nit:  The admin is preventing the user from adding new quips.  I'm just
trying to move up the mention so they don't get a disappointment when they come
up with the perfect quip only to find that submissions are disabled.

Comment 10

[Frédéric Buclin]

Comment on attachment 186179 [details] [diff] [review]
Patch v3 for tip

>+[% IF Param("quip_list_entry_control") != "closed" %]
> <p>
>-  [% terms.Bugzilla %] will pick a random quip for the headline on each [% terms.bug %] list, and
>-  you can extend the quip list. Type in something clever or funny or boring
>+  You can extend the quip list. Type in something clever or funny or boring
>   (but not obscene or offensive, please) and bonk on the button.
>   [% IF Param("quip_list_entry_control") == "moderated" AND !user.groups.admin %]
>     Note that your quip has to be approved before it is used.
>@@ -73,6 +79,10 @@
>     <input type="submit" value="Add This Quip">
>   </p>
> </form>
>+[% ELSE %]


Except that the indentation is incorrect (<p></p><form></form> and subsequent
lines in the IF block should be moved 2 positions to the right), the patch
works great. So r=LpSolit

Could you please update the patch anyway? I will wait for your updated patch
before requesting approval.

Comment 11

[A. Karl Kornel]

Attachment: Patch v3.5 for tip

Copy of attachment 186179 [details] [diff] [review] modified for indentation (as per comment 10).

Comment 12

[Frédéric Buclin]

Comment on attachment 186278 [details] [diff] [review]
Patch v3.5 for tip

r=LpSolit

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Low risk polish fix, a=myk for check in during 2.20 freeze.

Comment 14

[Frédéric Buclin]

Updating the bug summary to be consistent with 2.20.

Checking in template/en/default/list/quips.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/quips.html.tmpl,v 
<--  quips.html.tmpl
new revision: 1.16; previous revision: 1.15
done