94303 - Group together changes made at the same time in activity log

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement, P1)

Description

[GavinS]

It would be nice in the Activity Log for a bug, if some fields (i.e. who and
when) could be grouped together (rowspan=X) for activities which all resulted
from one change.

Comment 1

[Dave Miller [:justdave]]

There's precedent.  Bonsai does this with the cvs change logs.  I would like to 
see this implemented :-)

Comment 2

[Matthew Tuck [:CodeMachine]]

Would this involve a schema change or would we just interpret the same
user/date/time?  I'd like to see this grouped in the schema eventually.

Comment 3

[Dave Miller [:justdave]]

no schema change required.  You just read the data internally ahead of time, and 
don't output anything until the data in the first two columns changes, then you 
output the first two columns with the rowspan= parameter and the as many rows as 
necessary for the other two columns.  This is how Bonsai does it, and this is how 
we do it on the patch/installation listing on the main landfill page if there's 
more than one patch installed on the same test installation.

Comment 4

[GavinS]

So, I think this would look best if the last column ('when') was moved to be the
2nd column, after the 'who' column, yes?. Would this be OK with people?

And, if the same person has made many many changes, at many different times, but
without anyone else making any changes inbetween, would you want the 'who' to
span all those changes, or to just span the changes made at one time?

Comment 5

[Dave Miller [:justdave]]

I would say just the changes made at that time.  It would be more distict where 
the different times were that way.

Comment 6

[GavinS]

I've got a patch for this, which I'll attach when 2.14 is out, so it is as
up-to-date as possible.

It will be a simple one line change to swap between the 2 possible ways the
'who' cell can span stuff, so you can check it out and see which you prefer.

I think it looks simpler (less data, less cells) when the 'who' cell spans
consecutive 'when' cells, and I think it is still possible to see the grouping
of dates.

Comment 7

[Zach Lipton [:zach]]

please upload your patch based on the current cvs tip.

Comment 8

[GavinS]

Attachment: Patch to CGI.pl to group together changes in the activity log

Comment 9

[Matthew Tuck [:CodeMachine]]

The following needs to be discussed:

1.  Currently the cells are aligned vertically centred.  This would mean that if
a user made lots of changes, there could be a substantial vertical distance
between the first row and the user name.  See
http://bugzilla.mozilla.org/show_activity.cgi?id=7954 for an example of what
will be a problem.  Top alignment might be a better policy here if we stick with
two categorisations.

2.  A variety of variables are named *_href.  To me this seems to imply HTML
links, yet they seems to be normal refs.  I'll guess this means hash ref but I'm
not sure it's a convention I like.  I think field_* would be clearer than what_*
here too.

3.  This:

    foreach (sort(keys(%$what_href))) {
        my $date = $_;

should, AFAICT, be:

    foreach my $date (sort(keys(%$what_href))) {

4.  I'm not sure I think the extra code for the second categorisation is worth
it in this instance.  However I can see us getting benefit if this code were
reusable, and you could specify the first N columns were categorised.  It might
be simpler too.

Beyond that, I don't have a 100% understanding of this patch, and might have
other issues in future.  Is this the code Bonsai is using or based on it?

Comment 10

[Matthew Tuck [:CodeMachine]]

Attachment: Previous patch did not apply cleanly against current CVS.

Comment 11

[GavinS]

1) Yes, top alignment is much better. (whatever grouping we end up with).

2) Yes, href == hash ref, but I will change that (and the other name change.)

3) Yes.

4) OK, I'll have a think about that. 

Misc things:

No, this is not the bonsai code - I haven't looked at that. I'd probably agree
that I got carried away with hashes and lists, and I suspect it is probably
easier to just read the data into 5 lists, for each column, so I will look at
re-doing the patch like that, which may be easier to understand.

Comment 12

[Dave Miller [:justdave]]

Any updates to this?  Last patch is marked needs-work, and it's been a couple
months.  If we don't have something on here in the next couple days it'll get
pushed to 2.18

Comment 13

[GavinS]

Attachment: Updated patch after suggestions.

Updated patch, still only against 2.14, I'm afraid.

1) This now aligns the user and data at the top of the cell, as suggested.

2) Variable name improvements

3) Done

4) I tried to make it simpler, and I tried a few ways of making the grouping
more generic and applicable to many columns, but I failed :( 

We've been using this for 5 months, with no problems, so it would be nice to
see it at least tested on landfill for a bit, maybe???

Comment 14

[Dave Miller [:justdave]]

Comment on attachment 67420 [details] [diff] [review]
Updated patch after suggestions.

Trying to view bug activity with this patch applied gets me this:

Bizarre copy of HASH in leave at CGI.pl line 1432.

Line 1432 of CGI.pl is here:

    foreach (keys(%$field_details)) {
      my $tmp_details_list = ${%$field_details}{$_};	 # <= 1432
      $user_span += scalar(@$tmp_details_list);
    }

Mac OS X 10.1.2, Perl 5..6.0

Comment 15

[Dave Miller [:justdave]]

Attachment: Much simpler way to do it

This one just stashes the HTML for the constantly-changing rows in a temporary
string, counting the number of rows as it goes, then outputs the first two
columns with a rowspan followed by the previously stashed data any time the
name or the date changes.

Comment 16

[Dave Miller [:justdave]]

Attachment: Example output from attachment 67864

Comment 17

[GavinS]

Yup, that's about a zillion times simpler :)

But, the user cell doesn't span many date cells, which I quite like now I've got
used to it here :(

The 'bizarre copy of hash' is fixed by removing the % from '${%$' , if anyone
does want to try that patch out. Later perls are more or less picky about that
construct - I can't remember which way it is.

I also added a 'No activity found' row in my patch, instead of just having a
table with headers but no data, which never looks any good. If someone checks in
the more simple patch, could they add that?

Comment 18

[Dave Miller [:justdave]]

the patch attached here works against the existing code...  but is pretty much
voided since we're templatizing this now.  We'll have to handle this in the
template.  See bug 124937

Comment 19

[Gervase Markham [:gerv]]

The latest patch on bug 124937 now contains a fix for this bug.

Gerv

Comment 20

[Gervase Markham [:gerv]]

This is fixed by the checkin on bug 124937.

Gerv