55161 - Show Activity: Old/New Values don't display full data

Status: RESOLVED FIXED

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

Description

[timeless]

[e-mail] bug_status NEW ASSIGNED 2000-09-08 08:16:46
[e-mail] cc 
[e-mail],[e-mail],[e-mail],[e-mail],dveditz
@netscape.com,[e-mail],[e-mail],[e-mail],morse@netsc
ape.com,[e-mail],[e-mail],[e-mail] 
[e-mail],[e-mail],[e-mail],[e-mail],dougt
@netscape.com,[e-mail],[e-mail],[e-mail],lord@net
scape.com,[e-mail],[e-mail],[e-mail],shaver@m
ozilla.org,valeski 2000-09-09 20:06:07
[e-mail] cc 
[e-mail],[e-mail],[e-mail],[e-mail],dougt
@netscape.com,[e-mail],[e-mail],[e-mail],lord@net
scape.com,[e-mail],[e-mail],[e-mail],shaver@m
ozilla.org,valeski 
[e-mail],[e-mail],[e-mail],[e-mail],ddrinan@n
etscape.com,[e-mail],[e-mail],[e-mail],gagan@netsc
ape.com,[e-mail],[e-mail],[e-mail],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

Comment 1

[Matthew Tuck [:CodeMachine]]

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.

Comment 2

[timeless]

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

Comment 3

[Dave Miller [:justdave]]

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.

Comment 4

[Jacob Steenhagen]

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.

Comment 5

[Dave Miller [:justdave]]

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

size increased in the database.

Comment 6

[Jacob Steenhagen]

'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. :)

Comment 7

[Dave Miller [:justdave]]

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.

Comment 8

[Jacob Steenhagen]

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

Comment 9

[Jacob Steenhagen]

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

Comment 10

[Jacob Steenhagen]

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

Comment 11

[Matthew Tuck [:CodeMachine]]

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.

Comment 12

[Jacob Steenhagen]

Attachment: partial patch

Comment 13

[Jacob Steenhagen]

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).

Comment 14

[Jacob Steenhagen]

Attachment: patch v1.0

Comment 15

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

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.

Comment 16

[Jacob Steenhagen]

> (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.

Comment 17

[Jacob Steenhagen]

Attachment: Patch v2

Comment 18

[Dave Miller [:justdave]]

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?)

Comment 19

[Jacob Steenhagen]

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.

Comment 20

[Dave Miller [:justdave]]

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.

Comment 21

[Jacob Steenhagen]

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.

Comment 22

[Dave Miller [:justdave]]

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....

Comment 23

[Dave Miller [:justdave]]

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

Comment 24

[Jacob Steenhagen]

Checked in.

Comment 25

[Dave Miller [:justdave]]

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 
([e-mail]), giving this error message and the time and 
date of the error. 

Comment 26

[Jacob Steenhagen]

Attachment: bugfix v1

Comment 27

[Jacob Steenhagen]

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

Comment 28

[Dave Miller [:justdave]]

r= justdave

it's in.

Comment 29

[Matthew Tuck [:CodeMachine]]

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

Comment 30

[Jacob Steenhagen]

OK, filed bug 92276.

Comment 31

[Jacob Steenhagen]

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).

Comment 32

[Jacob Steenhagen]

Attachment: bugfix 2 v1

Comment 33

[Dave Miller [:justdave]]

r= justdave

checked in.

Comment 34

[Jacob Steenhagen]

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

Comment 35

[Peter Lairo]

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.

Comment 36

[Jacob Steenhagen]

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.

Comment 37

[Dave Miller [:justdave]]

Moving to Bugzilla product