If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Port the inline history facility from the Bugzilla Tweaks add-on to a BMO extension

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Extensions: InlineHistory
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

Production

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
The inline history facility in the Bugzilla Tweaks add-on is excellent.

Given upstream support for inline history isn't going to make 4.2, we should port the Bugzilla Tweaks implementation to a Bugzilla extension.

Aside from making the feature available to more users, we can also greatly simplify the implementation by doing a lot of heavy lifting in a template rather than relying on scraping the history from html in a hidden iframe.
(Assignee)

Comment 1

6 years ago
Created attachment 543129 [details] [diff] [review]
patch v1

this isn't a straight port, the display of the changes inline is slightly different from the add-on, and CC changes are currently always shown.
Attachment #543129 - Flags: review?(dkl)

Comment 2

6 years ago
(In reply to comment #1)
> and CC changes are currently always shown.

That is... sub-optimal.  :(
(Assignee)

Comment 3

6 years ago
(In reply to comment #2)
> (In reply to comment #1)
> > and CC changes are currently always shown.
> 
> That is... sub-optimal.  :(

you've talked me into adding support for cc toggling :)
(Assignee)

Updated

6 years ago
Attachment #543129 - Flags: review?(dkl)
(Assignee)

Comment 4

6 years ago
Created attachment 543375 [details] [diff] [review]
patch v2

CC changes are not hidden by default, along with a swag of other changes.
Attachment #543129 - Attachment is obsolete: true
Attachment #543375 - Flags: review?(dkl)
(Assignee)

Comment 5

6 years ago
Created attachment 544182 [details] [diff] [review]
patch v3

changes:
- fixes issue on post-update bug page
- displays duplicates as history instead of comment

this revision also adds a div with id "inline-history-ext".  bugzilla tweaks (and forks) should disable their inline history processing if this div is present.
Attachment #543375 - Attachment is obsolete: true
Attachment #544182 - Flags: review?(dkl)
Attachment #543375 - Flags: review?(dkl)
Comment on attachment 544182 [details] [diff] [review]
patch v3

Review of attachment 544182 [details] [diff] [review]:
-----------------------------------------------------------------

I know that some of these comments refer to the original code/style from the BZ Tweaks Addon so are just carry overs. But we should be able to make any improvements needed at the same time as we convert over.

Also per our discussion on IRC, it would be nice if the activity changes were one per line and displayed below each respective comment separated by a line. See bugs.launchpad.net for examples.

dkl

::: extensions/InlineHistory/Extension.pm
@@ +39,5 @@
> +    my $user = Bugzilla->user;
> +
> +    return unless
> +        $file eq 'bug/show.html.tmpl'
> +        || $file eq 'bug/process/results.html.tmpl';

Need to include attachment/created.html.tmpl and attachment/updated.html.tmpl as well. There has to be an easier way to do this such as just looking for show-header.html.tmpl but upon experimenting, it didn't work for me.

@@ +44,5 @@
> +    return unless $user && $user->id && $user->settings;
> +    return unless $user->settings->{'inline_history'}->{'value'} eq 'on';
> +
> +    return unless Bugzilla->input_params->{'id'} =~ /^(\d+)$/;
> +    my $bug_id = $1;

As mentioned above that you have to include include attachment/created.html.tmpl and attachment/updated.html.tmpl but the above
does not work with looking for 'id' as coming from attachment.cgi, that is the attachment id. When I first tried this I got history from another bug instead matching that attachment id. What you may need to do is to check for those special cases, $vars->{bugs}[0]{bug_id} to load the proper history.

::: extensions/InlineHistory/web/style.css
@@ +25,5 @@
> + */
> +
> +.ih_history {
> +    background: none !important;
> +    max-width: 80%;

Looks better if 100% as it will be same size as the other comments.

@@ +33,5 @@
> +    font-weight: normal;
> +}
> +
> +.ih_history .old, .ih_inlinehistory .old {
> +    text-decoration: line-through;

Do we really need to do the strike-through of the old value if we already use an arrow to show the transition? It makes it difficult to read sometimes.
Attachment #544182 - Flags: review?(dkl) → review-
(Assignee)

Comment 7

6 years ago
(In reply to comment #6)

thanks for the review dkl :)

> Need to include attachment/created.html.tmpl and [..]

d'oh :)  nice catch.

> > +.ih_history {
> > +    background: none !important;
> > +    max-width: 80%;
> 
> Looks better if 100% as it will be same size as the other comments.

originally i had this, however on the standard skin there's nothing to differentiate comments with activity blocks.
 
> Do we really need to do the strike-through of the old value if we already
> use an arrow to show the transition? It makes it difficult to read sometimes.

that makes sense; i was aiming for tweaks-parity, but i agree we may as well tweak tweaks.
(Assignee)

Comment 8

6 years ago
Created attachment 548499 [details] [diff] [review]
patch v4
Attachment #544182 - Attachment is obsolete: true
Attachment #548499 - Flags: review?(dkl)
Comment on attachment 548499 [details] [diff] [review]
patch v4

Review of attachment 548499 [details] [diff] [review]:
-----------------------------------------------------------------

Looks really good. Just a couple comments that can be fixed on checkin if you think they are good ideas.

r=dkl

::: extensions/InlineHistory/Extension.pm
@@ +30,5 @@
> +use Bugzilla::User::Setting;
> +use Bugzilla::Constants;
> +use Bugzilla::Attachment;
> +
> +our $VERSION = '1';

Maybe use the same version as the current Bugzilla Tweaks add-on (1.10) since it is sharing the same jscript otherwise I am fine with 1.

@@ +92,5 @@
> +    my ($bug_id, $activity) = @_;
> +
> +    my $dbh = Bugzilla->dbh;
> +    my $sth = $dbh->prepare("
> +        SELECT profiles.login_name, bug_when, extra_data

$dbh->sql_date_format('bugs_activity.bug_when', '%Y.%m.%d %H:%i:%s') to be consistent with Bug::GetBugActivity.

::: extensions/InlineHistory/web/style.css
@@ +30,5 @@
> +    max-width: 80%;
> +    color: #444;
> +}
> +
> +.ih_inlinehistory {

I would still like to see some sort of subtle dividing line between the comment (none if there is no comment) and the changes. Ex:

border-top: 1px solid #C8C8BA;
Attachment #548499 - Flags: review?(dkl) → review+
(Assignee)

Comment 10

6 years ago
(In reply to comment #9)
> > +our $VERSION = '1';
> 
> Maybe use the same version as the current Bugzilla Tweaks add-on (1.10)
> since it is sharing the same jscript otherwise I am fine with 1.

while this ext stems from tweaks, it isn't true to say that they share the same javascript.
 

i also need to incorporate tweak's linking flags in the attachment table to the comment/item where it was set, so there'll be another revision.
(Assignee)

Comment 11

6 years ago
WIP patch pushed to https://bugzilla-stage-tip.mozilla.org/

you have to be logged in to see the inline-history.
it's controlled by a user preference and is enabled by default.

i'm yet to link bug-level flags to the correct point in history, and ensure it works without issue on bmo, 4.0 and trunk.
(Assignee)

Updated

6 years ago
Depends on: 675953

Comment 12

6 years ago
(In reply to comment #11)
> WIP patch pushed to https://bugzilla-stage-tip.mozilla.org/
> 
> you have to be logged in to see the inline-history.
> it's controlled by a user preference and is enabled by default.

I don't see any inline history (https://bugzilla-stage-tip.mozilla.org/show_bug.cgi?id=240933 for example) or any new preferences.  Am I missing something?
I see it some places, such as here:

https://bugzilla-stage-tip.mozilla.org/show_bug.cgi?id=399651
(In reply to comment #12)
> (In reply to comment #11)
> I don't see any inline history
> (https://bugzilla-stage-tip.mozilla.org/show_bug.cgi?id=240933 for example)
> or any new preferences.  Am I missing something?

I do see the preference in my userprefs.cgi page. Look for "When viewing a bug, show all bug activity".


(In reply to comment #13)
> I see it some places, such as here:
> 
> https://bugzilla-stage-tip.mozilla.org/show_bug.cgi?id=399651

I also see the changes for me. Make sure you select "Show CC Changes" to the right of the comments to add more activity. I also disabled the current Bugzilla Tweaks extension if that makes any difference.

dkl
Also the history is below each comment now instead above so make sure you look there instead. There is a subtle line between the comment and the changes.

dkl
(Assignee)

Comment 16

6 years ago
(In reply to comment #12)
> I don't see any inline history
> (https://bugzilla-stage-tip.mozilla.org/show_bug.cgi?id=240933 for example)
> or any new preferences.  Am I missing something?

you found a bug, triggered by an attachment with a ' in the description (i was using the wrong filter).

(time passes)

ok, try again.
Hey,

This is awesome! Thanks, glob and dkl, for doing this! Now I don't have to worry about getting Bugzilla Tweaks ported to SeaMonkey to still be able to effectively read BMO bug reports. :)
(Assignee)

Comment 18

6 years ago
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bmo/4.0/
added extensions/InlineHistory
added extensions/InlineHistory/Config.pm
added extensions/InlineHistory/Extension.pm
added extensions/InlineHistory/README
added extensions/InlineHistory/template
added extensions/InlineHistory/web
added extensions/InlineHistory/template/en
added extensions/InlineHistory/template/en/default
added extensions/InlineHistory/template/en/default/hook
added extensions/InlineHistory/template/en/default/hook/bug
added extensions/InlineHistory/template/en/default/hook/global
added extensions/InlineHistory/template/en/default/hook/bug/comments-aftercomments.html.tmpl
added extensions/InlineHistory/template/en/default/hook/bug/show-header-end.html.tmpl
added extensions/InlineHistory/template/en/default/hook/global/setting-descs-settings.none.tmpl
added extensions/InlineHistory/web/inline-history.js
added extensions/InlineHistory/web/style.css
Committed revision 7845.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Component: Extensions: Other → Extensions: Inline History
QA Contact: extensions → inline-history

Comment 19

6 years ago
Hi Bryon,

is this add-on also available for other bugzilla installations?
(waiting bug 11368)
(Assignee)

Comment 20

6 years ago
(In reply to bigstijn from comment #19)
> is this add-on also available for other bugzilla installations?
> (waiting bug 11368)

it _should_ work on other installations, however it's largely untested.  i've put the link to the source in the url field.

Comment 21

3 years ago
(In reply to Byron Jones ‹:glob› from comment #20)
> it _should_ work on other installations, however it's largely untested. 
> i've put the link to the source in the url field.

Has it moved? Link doesn't show anything there.
Yes, we've moved everything to git.  Link updated.
PSA: This extensions seems to work fine on a vanilla 5.0rc3 install, no issues encountered yet.
You need to log in before you can comment on or make changes to this bug.