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

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
User Interface
--
minor
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: A. Karl Kornel, Assigned: A. Karl Kornel)

Tracking

2.18.1
Bugzilla 2.20
Bug Flags:
approval +

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

13 years ago
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 'user@cse.cse' 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.
(Assignee)

Updated

13 years ago
Version: unspecified → 2.18.1
(Assignee)

Comment 1

13 years ago
Created attachment 186035 [details] [diff] [review]
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).
Attachment #186035 - Flags: review?(myk)
(Assignee)

Comment 2

13 years ago
Created attachment 186038 [details] [diff] [review]
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.
Attachment #186038 - Flags: review?(myk)

Comment 3

13 years ago
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.
Attachment #186038 - Flags: review?(myk) → review-
(Assignee)

Comment 4

13 years ago
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.
Attachment #186035 - Attachment is obsolete: true
(Assignee)

Comment 5

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

Updated

13 years ago
Attachment #186035 - Flags: review?(myk)
(Assignee)

Comment 6

13 years ago
Created attachment 186128 [details] [diff] [review]
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.
Attachment #186038 - Attachment is obsolete: true
Attachment #186128 - Flags: review?(myk)

Comment 7

13 years ago
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. ;)
Attachment #186128 - Flags: review?(myk) → review-

Comment 8

13 years ago
I meant:

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

I forgot <p></p> tags.
Assignee: myk → kornel.1
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 9

13 years ago
Created attachment 186179 [details] [diff] [review]
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.
Attachment #186128 - Attachment is obsolete: true
Attachment #186179 - Flags: review?(LpSolit)

Comment 10

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

Updated

13 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 11

13 years ago
Created attachment 186278 [details] [diff] [review]
Patch v3.5 for tip

Copy of attachment 186179 [details] [diff] [review] modified for indentation (as per comment 10).
Attachment #186179 - Attachment is obsolete: true
Attachment #186278 - Flags: review?(LpSolit)

Comment 12

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

r=LpSolit
Attachment #186278 - Flags: review?(LpSolit) → review+

Updated

13 years ago
Flags: approval?
Low risk polish fix, a=myk for check in during 2.20 freeze.
Flags: approval? → approval+

Comment 14

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Summary: Quip entry field is available when enablequips is frozen → Do not display the quip entry field when quip_list_entry_control is set to 'closed'
You need to log in before you can comment on or make changes to this bug.