timeless Reporter
	Description • 24 years ago

shaver@mozilla.org bug_status NEW ASSIGNED 2000-09-08 08:16:46
bryner@netscape.com cc 
David@Krause.net,brendan@mozilla.org,bryner@uiuc.edu,dougt@netscape.com,dveditz
@netscape.com,gagan@netscape.com,leaf@mozilla.org,lord@netscape.com,morse@netsc
ape.com,pollmann@netscape.com,shaver@mozilla.org,valeski@netscape.com 
David@Krause.net,brendan@mozilla.org,bryner@uiuc.edu,ddrinan@netscape.com,dougt
@netscape.com,dveditz@netscape.com,gagan@netscape.com,leaf@mozilla.org,lord@net
scape.com,morse@netscape.com,pollmann@netscape.com,relyea@netscape.com,shaver@m
ozilla.org,valeski 2000-09-09 20:06:07
shaver@mozilla.org cc 
David@Krause.net,brendan@mozilla.org,bryner@uiuc.edu,ddrinan@netscape.com,dougt
@netscape.com,dveditz@netscape.com,gagan@netscape.com,leaf@mozilla.org,lord@net
scape.com,morse@netscape.com,pollmann@netscape.com,relyea@netscape.com,shaver@m
ozilla.org,valeski 
David@Krause.net,ben@netscape.com,brendan@mozilla.org,bryner@uiuc.edu,ddrinan@n
etscape.com,dougt@netscape.com,dveditz@netscape.com,edburns@acm.org,gagan@netsc
ape.com,leaf@mozilla.org,lord@netscape.com,morse@netscape.com,pollmann@netscape
.com,relyea@netsca 2000-09-12 10:22:40

At least 7 people added to it, after adding to cc each user should appear on 
cht list.

hear we have, relyea@netsca among others. This also appears in bug diffs

	Matthew Tuck [:CodeMachine]
	Comment 1 • 24 years ago

Timeless, if I can understand what you're trying to say, I don't understand what
the problem is.  Someone just entered a default CC list.

	timeless Reporter
	Comment 2 • 24 years ago

the problem is that the logging field is too narrow, and that we treat the 
real field as a string for diffing instead of a list.

all changed output [both email and log] should be of the form:
cc -userlist         | +userlist
   ^people removed   | ^people added
or something similar

	Dave Miller [:justdave]
	Comment 3 • 24 years ago

this is covered in another bug somewhere...  I don't have the number handy at the 
moment.  if I think of it, I'll come back and look for it after I get caught up.

	Jacob Steenhagen Assignee
	Comment 4 • 24 years ago

Bug 61015 covers this for the e-mail... can't locate one for show_activity.cgi.
It is, of course, still limited by the field size in the DB.

	Dave Miller [:justdave]
	Comment 5 • 24 years ago

If it used the technique listed here, it probably wouldn't NEED to have the field 

size increased in the database.

	Jacob Steenhagen Assignee
	Comment 6 • 24 years ago

'tis true.  There are basically two fixes for this:
1) Increase the field size in the DB and make the output code deal w/it
2) Change the way the data is stored (and add code to checksetup.pl to process 
   this change on existing installations).

So then the question is, which is the right one?  Some would probably argue that
it's nice to see the entire old/new value.  I myslef like #2 better... the only
reason I bring up number two (and used that method in 61015) is it's easier. :)

	Dave Miller [:justdave]
	Comment 7 • 24 years ago

well, no matter what you change the database field to, it's still going to be 
limited in size unless you make it a blob.  Blobs are inherently evil for 
anything other than comments since they can't be indexed and make searching 
inefficient.  This makes "The Right Way" be #2.  But it's going to be a pain in 
the butt to convert the old records because many of them are incomplete because 
of being truncated by the field size being too small in the past.  Perhaps we 
could convert the old records like this:  change "," to "\n", add "+" in front of 
each line in "new value", add "-" in front of each line in "old value", and leave 
it at that.  Don't try to sort duplicates between the fields.  You would lose one 
character per email address by adding the +/- characters, which would truncate 
off the end still if the field was already overflowed on the existing entry.

	Jacob Steenhagen Assignee
	Comment 8 • 24 years ago

*** Bug 61015 has been marked as a duplicate of this bug. ***

	Jacob Steenhagen Assignee
	Comment 9 • 23 years ago

I plan on doing something based on "number 2" (changing the fields to be
removed/added instead of old/new).

	Jacob Steenhagen Assignee
	Comment 10 • 23 years ago

*** Bug 89003 has been marked as a duplicate of this bug. ***

	Matthew Tuck [:CodeMachine]
	Comment 11 • 23 years ago

Could we do this and fix notifications up so they still display the whole value
and yet don't clip?  Or if not so they still display the whole value and clip?

I'd rather wait until bug #86201 to change the behaviour of notifications, when
the user will be able to choose which they prefer.  And I strongly suggest this
is considered a 2.14 blocker, we should fix this ASAP to prevent further data
loss.

	Jacob Steenhagen Assignee
	Comment 13 • 23 years ago

This still needs work, but the idea is now instead of "old/new" it shows
"added/removed".  The processing code is only different for keywords, deps, and
ccs.  It is currently running at http://landfill.tequilarista.org/bz55161/
(which is a clone of the bugs db, so changes made on this install won't effect
the main one).  The place where it's most evident that work is still needed is
the activity page... it shows the original full string old/new as well as the
newer compact version.

This also has some code in it that should help with bug 12819 (which seems to
have a bug in it... cc is now showing up as a change every time, even if it's
blank... I'll work on that next chance I get).

	Myk Melez [:myk] [@mykmelez]
	Comment 15 • 23 years ago

I tested the functionality on landfill (except for mass cc changes, which I
don't have permissions for), and it seems to work just fine.  I only found minor
issues with the patch itself:

>+            # If we can't determine hat changed, put a ? in both fields

hat -> what

>+# Take two comma or space seperated strings and return what

seperated -> separated

>+        my ($added, $removed) = "";
   .....
>+            if (@add) { $added = join (", ", @add) }
>+            if (@remove) { $removed = join (", ", @remove) }

It isn't necessary to test for values in the arrays before joining them, because
an empty array joins to an empty string, so this can be shortened to:

+            my $added = join (", ", @add);
+            my $removed = join (", ", @remove);

Same here:

>+    my ($removed, $added) = "";
   .....
>+    if (@remove) { $removed = join (", ", @remove) }
>+    if (@add) { $added = join (", ", @add) }

And here:


>+            my $removed = scalar(@removed) ? SqlQuote(join(", ", @removed)) :
"''";
>+            my $added = scalar(@added) ? SqlQuote(join(", ", @added)) : "''";

Also, it would be useful when fixing the activity log in checksetup.pl to print
out the bug number being processed every 500 log entries or so.  That way
administrators could get a sense of how the fixing is progressing over time.  I
write this while watching my GUI grind to a halt as my terminal window fills up
with dots and I wonder how much longer this is going to take.

	Jacob Steenhagen Assignee
	Comment 16 • 23 years ago

> (except for mass cc changes, which I don't have permissions for)

You should, it only requires editbugs and you have that (and the regexp for it
is .*).

>>+        my ($added, $removed) = "";
>   .....
>>+            if (@add) { $added = join (", ", @add) }
>>+            if (@remove) { $removed = join (", ", @remove) }
...
>+            my $added = join (", ", @add);
>+            my $removed = join (", ", @remove);

In checksetup.pl I still have to decalare it first due to scope... in the other
places it's changed to declare at the same time it joins (and in all cases I
took out the if (@add) type stuff).
> Also, it would be useful when fixing the activity log in checksetup.pl to 
> print out the bug number being processed every 500 log entries or so.

Hmm... so that .'s aren't good enough?  The bugs_activity table isn't in order
by bug number, so I'd have to add an ORDER BY clause to the query.  Also, this
would make it so the UPDATEs are non-sequention. I *think* that means the
process would take longer, but I could be wrong there.


The last remaining issue I have is the wording on the mass change page. As a
part of step 2 it says "(It's <b>always</b> a good idea to add some comment
explaining what you're doing.)" but that's not really true as it defeats the
mail filtering (esp. w/cc only changes).  I changed it to "If the change you are
making requires an explanation, include it in the comments box)." but I think
there's gotta be a better way to say that.

	Dave Miller [:justdave]
	Comment 18 • 23 years ago

ok, took a look through at patch v2.

It looks to me like you removed the code to do a forceCC on the removed CCs from 
process_bug, and made processmail do itself by pulling those values from the 
activity table.  However, I see you adding the @added CCs to the forceCC list 
instead of the @removed ones.  Shouldn't the added CCs already be on the bug's 
main CC list? (and thus already included anyway?)

	Jacob Steenhagen Assignee
	Comment 19 • 23 years ago

processmail currently looks at the activity log to determine who's added so I 
just changed it to use the new format.  I suppose processmail should be 
modified to use the activity log for all "force" e-mails instead of the command 
line params.

BTW, it has to determine who's been added because of the "add or removed" 
filter.

	Dave Miller [:justdave]
	Comment 20 • 23 years ago

what about figuring out who's been removed though?  Unless I'm missing something, 
this patch makes it so people being removed from a CC won't get emailed about 
them being removed.

	Jacob Steenhagen Assignee
	Comment 21 • 23 years ago

I know it's not the best situation, but it is the way the code is now.

Removed is passed in on the command line (as calculated by process_bug.cgi).  
Added is figured out by processmail (using the activity log).  That means 
currently a lot of added to cc is probably missed due this bug.

This is true of QA, Owner, and CC.  It wouldn't be a bad idea to pick on method 
and run with it (I'd vote for using the activity log), but I think that's 
outside the scope of this bug.

	Dave Miller [:justdave]
	Comment 22 • 23 years ago

ok, except as I said in my first comment, this patch looks like it removes the 
code from process_bug.cgi that calculates the deleted addresses...  thus there's 
nothing getting passed in --forcecc on the command line....

	Dave Miller [:justdave]
	Comment 23 • 23 years ago

ok, I found it.  It's that magic diffstrings routine :)  OK, looks good from 
here.

	Jacob Steenhagen Assignee
	Comment 24 • 23 years ago

Checked in.

	Dave Miller [:justdave]
	Comment 25 • 23 years ago

queries are busted with this patch in place.  Try querying on field [bug_status] 
changed to [RESOLVED]

Software error:

SELECT bugs.bug_id, bugs.groupset, substring(bugs.rep_platform, 1, 3), 
substring(bugs.bug_status,1,4),
substring(bugs.resolution,1,4), substring(bugs.component, 1, 8), 
substring(bugs.product, 1, 8), substring(bugs.short_desc, 1, 60)
FROM bugs, profiles map_assigned_to, profiles map_reporter LEFT JOIN profiles 
map_qa_contact ON bugs.qa_contact =
map_qa_contact.userid, bugs_activity actcheck WHERE bugs.assigned_to = 
map_assigned_to.userid AND bugs.reporter =
map_reporter.userid AND bugs.groupset & 0 = bugs.groupset AND actcheck.bug_id = 
bugs.bug_id AND ( actcheck.fieldid = 8) AND
actcheck.bug_when >= '2001/07/21 00:00:00' AND actcheck.newvalue = 'RESOLVED' AND 
(bugs.bug_status = 'NEW' OR bugs.bug_status =
'ASSIGNED' OR bugs.bug_status = 'REOPENED') GROUP BY bugs.bug_id ORDER BY 
bugs.priority, bugs.bug_severity: Unknown column
'actcheck.newvalue' in 'where clause' at globals.pl line 205. 

For help, please send mail to the webmaster 
(webmaster@landfill.tequilarista.org), giving this error message and the time and 
date of the error. 

	Jacob Steenhagen Assignee
	Comment 27 • 23 years ago

Grr... I "grep"ed for "oldvalue" and not "newvalue" so I missed that entry in
buglist :(

	Dave Miller [:justdave]
	Comment 28 • 23 years ago

r= justdave

it's in.

	Matthew Tuck [:CodeMachine]
	Comment 29 • 23 years ago

I don't understand the rearchitecting Jake suggested, but I suggest someone
remember to file it.

	Jacob Steenhagen Assignee
	Comment 30 • 23 years ago

OK, filed bug 92276.

	Jacob Steenhagen Assignee
	Comment 31 • 23 years ago

Recent conversation on IRC have determined that the conversion from old/new to
removed/added would be inaccuracte if the activity field was already full
(determined by its length being 255 characters).  These values should then be
prefaced w/a '?' to say that they may be inaccurate (rather than simply not
providing any data).  Also, the '?' should be explained on the activity page
(but only if a '?' is found).

	Dave Miller [:justdave]
	Comment 33 • 23 years ago

r= justdave

checked in.

	Jacob Steenhagen Assignee
	Comment 34 • 23 years ago

*** Bug 61015 has been marked as a duplicate of this bug. ***

	Peter Lairo
	Comment 35 • 23 years ago

Since the bug to fix that (bug 61015) was marked a dupe of this: 
I still don't see the summary of bugs when dependency changes are reported by
e-mail. Are you sure this has been fixed? It seems not.

	Jacob Steenhagen Assignee
	Comment 36 • 23 years ago

Peter, no that has not been fixed yet.  But it was neither a part of this bug
nor bug 61015.  I think you are looking for bug 28736.

	Dave Miller [:justdave]
	Comment 37 • 23 years ago

Moving to Bugzilla product