Group together changes made at the same time in activity log

RESOLVED FIXED in Bugzilla 2.16

Status

()

P1
enhancement
RESOLVED FIXED
17 years ago
6 years ago

People

(Reporter: bugzilla, Assigned: bugzilla)

Tracking

2.10
Bugzilla 2.16

Details

(Whiteboard: [blocker will fix])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Updated

17 years ago
Severity: normal → enhancement
There's precedent.  Bonsai does this with the cvs change logs.  I would like to 
see this implemented :-)
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.16
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.
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.
(Assignee)

Comment 4

17 years ago
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?
I would say just the changes made at that time.  It would be more distict where 
the different times were that way.
(Assignee)

Comment 6

17 years ago
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.
Assignee: justdave → gavins

Comment 7

17 years ago
please upload your patch based on the current cvs tip.
Component: Bugzilla → Creating/Changing Bugs
OS: SunOS → All
Product: Webtools → Bugzilla
Hardware: Sun → All
Version: Bugzilla 2.12 → 2.10
(Assignee)

Comment 8

17 years ago
Created attachment 47647 [details] [diff] [review]
Patch to CGI.pl to group together changes in the activity log
(Assignee)

Updated

17 years ago
Keywords: patch, review
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?
Keywords: patch, review
Created attachment 52228 [details] [diff] [review]
Previous patch did not apply cleanly against current CVS.
Attachment #47647 - Attachment is obsolete: true
Attachment #52228 - Flags: review-
(Assignee)

Comment 11

17 years ago
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.
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
(Assignee)

Comment 13

17 years ago
Created attachment 67420 [details] [diff] [review]
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???
Attachment #52228 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Keywords: patch, review
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
Attachment #67420 - Flags: review-
Created attachment 67864 [details] [diff] [review]
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.
(Assignee)

Comment 17

17 years ago
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?
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
Whiteboard: [blocker will fix]
The latest patch on bug 124937 now contains a fix for this bug.

Gerv
This is fixed by the checkin on bug 124937.

Gerv
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.