42851 - [patch] Use listbox with input for CC management on bug form

Status: VERIFIED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P3)

Description

[Dave Miller [:justdave]]

With some bugs now getting long lists of CCs on them, trying to edit the list of 
CCs starts to be a chore.  You can accidently remove someone if you remove one 
letter from their email address while trying to edit the list.  What I propose is 
to place a listbox on the form with the current CC addresses in it.  Under the 
listbox would be a checkbox for "remove selected addresses".  If you want to 
remove someone, you select their name in the list, and check the remove box.  
There would also be a text input under it for "Add CC" to allow you to add one or 
more additional addresses.

For related bugs that aren't quite the same, see:
- bug 7345 (blind CCs)
- bug 28849 (don't allow non-editors to add/remove anyone other than themselves) 
- bug 34488 (button to add me to the CC list)

I am attaching two files to this bug.  The first is a diff of bug_form.pl with 
the code changes to place this list on the form.  The second is an HTML file with 
the output of the above form.

NOTE: DO NOT install this on a production Bugzilla (yet).  Although this 
bug_form.pl is fully functional, I have not modified process_bug.cgi to handle 
the revised fields yet, and it'll break things without both files being updated.  
I'm putting this here now so people can look at it.  Feel free to put it on a 
test installation and look at bugs with it, but don't try changing them or you'll 
likely delete people off the CC lists. :)

Regarding process_bug.cgi, I would like help updating this to handle the new 
fields.  I do not quite understand what it's doing, and don't want to break it.  
I could probably figure it out if I stared at it a while, but if whoever wrote 
the RelationSet.pm thing would take a look at it, it would probably be much 
easier for them.  (copying those two on this bug for that reason)

In the new form, the listbox is named 'cc', the checkbox for remove is named 
'removecc', and the text input for adding CCs is named 'newcc'.

Comment 1

[Dave Miller [:justdave]]

Attachment: Diffs for bug_form.pl to use listbox for CC

Comment 2

[Dave Miller [:justdave]]

Attachment: HTML output of the above file

Comment 3

[Dave Miller [:justdave]]

OK, had some time to look over the situation a little more...  looks like this 

will be doable without too much hassle if a method is added to RelationSet to 

remove the items from the set that are in the list passed to it.  Everything else 

needed to handle the new fields can be done within the existing structure.

Comment 4

[Dave Miller [:justdave]]

OK, I figured it out, and it works pretty good here after some relatively 
thorough testing.

I added "deleteItemsInString" and "deleteItemsInArray" methods to the RelationSet 
class.

process_bug.cgi has now been updated to handle the new fields.  Attachments with 
diffs for RelationSet.pm and process_bug.cgi coming right up.

Comment 5

[Dave Miller [:justdave]]

Attachment: diffs for RelationSet.pm to add method to delete items from list

Comment 6

[Dave Miller [:justdave]]

Attachment: diffs for process_bug.cgi to handle the list box for CC

Comment 7

[Tara Hernandez]

Looks like a good patch to throw on landfill..

Comment 8

[Chris Yeh]

applied to landfill

Comment 9

[Chris Yeh]

After applying this patch, it appears to not show up very well on Linux versions 
of Netscape 4.*. The "Add CC" field gets run over summary field. If your window 
width is narrow, it also rolls the new fields under.

If you could make a few tweaks to the layout so it looks a little better, we'll 
then apply it to the trunk.

You can see your patches applied at http://landfill.bluemartini.com

Comment 10

[Dave Miller [:justdave]]

OK, one more patch coming up.  This might take a few people off-guard when it 
gets applied... but it's not too bad.  I ended up moving the components popup to 
a new row right underneath product, and moving the version popup right next to 
component underneath OS & platform.  Reporter got moved up one space to the top 
row, and the CC list now occupies most of the right column.  This gives it just 
enough space that it no longer needs to be to the right of the text input boxes 
(some of which are optional, which made it difficult before).

I also fixed a couple HTML errors I found while I was at it (unclosed or 
misspelled tags).

I do apologize, but I forgot to save a copy of the file as it was after the last 
patch, so the upcoming attached patch file is against the current CVS version, 
and not against what's already on landfill. :(

Comment 11

[Dave Miller [:justdave]]

Attachment: new diffs for bug_form.pl with revised layout

Comment 12

[Dave Miller [:justdave]]

Oh, and I forgot to mention, the space after the "Cc:" header is now blank 
(instead of containing an empty list and a checkbox) if there are no CCs on that 
bug.  The Add CC: line is always there, but the listbox and the checkbox are now 
only shown if there are already CCs on that bug.  (The listbox looks kind of 
stupid with nothing in it, so I got rid of it if it's empty).

Comment 13

[Dave Miller [:justdave]]

oops, was playing around with it on landfill (fun to have a place to play :)



Discovered a bug already.  if there is nothing selected in the CC list and you 

check the box for "remove selected CCs" it goes boom with a Software Error when 

you hit Commit.  I'll have a fix for that in here shortly.  (And yes, I'll 

remember to diff it against the result of what I just posted this time so you 

don't have to reload the original off the CVS server again :)

Comment 14

[Dave Miller [:justdave]]

Attachment: bugfix for checkmark with nothing selected in process_bug.cgi

Comment 15

[Chris Yeh]

Applied latest patches to landfill. Go ahead and check it out and see if it 
works like you expect. I like what you did with the layout.

Comment 16

[Dawn Endico]

I suggest moving the "Add CC" field above the CC field. The way it is now,
the field  hangs out there in space, especially when there are no cc's. If
you move it above the CC field, then there are never any gaps betwween the
fields in the right column and you have more space to display more CC's.

Comment 17

[Dave Miller [:justdave]]

OK. I agree.  Done.  Patch coming right up. :)

Comment 18

[Dave Miller [:justdave]]

Attachment: diff to swap CC and Add CC on bug_form.pl

Comment 19

[timeless]

fwiw, we had a col span issue the last time i checked (which caused the cc 
listbox to collide w/ two edit boxes).  If i find a current source then i'll 
write a patch.  --good night all.

Comment 20

[Dave Miller [:justdave]]

Timeless, that got fixed already in the patch I uploaded on the 19th.  The CC 
List isn't even next to the edit boxes anymore.

Comment 21

[Chris Yeh]

committed patches to trunk. marking as fixed. thanks Dave!

Comment 22

[timeless]

Attachment: patch makes attached fields as wide as they were before the cc stuff; and cleans up columns and cc

Comment 23

[Dave Miller [:justdave]]

The original (before the patch you just posted) ensures that there is a CC field 
available, whether or not there is a list box generated on the page.  The 
$cc_element is assigned a hidden input field with an empty "cc" field in it.  
Then there is an if construct in there that reassigns that $cc_element to the 
listbox element IF there are CC's on the bug.  With your patch there are now no 
"cc" fields in existance on the resulting HTML if there are no CC's on the bug, 
and there are two "cc' fields (one hidden, one the listbox) if there are CC's.

Also, all of the columns with  s in them that you're removing with this 
patch were added because there was a complaint about the fields running together, 
so that added a tiny bit of space between the fields to keep them from running 
into each other.

I think I see what you're trying to fix though (the attachment listing).  It's 
actually the column span in the code generating the attachment list that's the 
problem.  I left that one out when I edited the rest of the colspans to match the 
new table width. (my oops).

Comment 24

[Dave Miller [:justdave]]

Attachment: patch that fixes column widths, fixes attachment list spacing

Comment 25

[Dave Miller [:justdave]]

OK, I did have the column widths messed up anyway...  there's one less column 
than I thought there was.  The new patch I just posted fixes all the COLSPANs 
where they're supposed to be (including the attachment list ones that got left 
out last time).

Comment 26

[Chris Yeh]

applied to landfill

Comment 27

[Dave Miller [:justdave]]

OK, Chris, since you still haven't checked that last one into the trunk yet, 
here's one more thing to go with it:

Index: process_bug.cgi
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v
retrieving revision 1.59
diff -u -r1.59 process_bug.cgi
--- process_bug.cgi     2000/06/21 19:03:45     1.59
+++ process_bug.cgi     2000/07/02 17:04:11
@@ -36,6 +36,7 @@
 use vars %::versions,
     %::components,
     %::COOKIE,
+    %::MFORM,
     %::keywordsbyname,
     %::legal_keywords,
     %::legal_opsys,

This shuts up the "used only once" warning I keep getting in my apache error log, 
thanks to using $::MFORM{cc} to grab the contents of the listbox...

Comment 28

[Chris Yeh]

patched applied and checked into trunk.

Comment 29

[Matthew Tuck [:CodeMachine]]

Verif this feature is present.  Any bugs should now be filed separately.

Comment 30

[Dave Miller [:justdave]]

In search of accurate queries....  (sorry for the spam)

Comment 31

[Dave Miller [:justdave]]

Moving closed bugs to Bugzilla product