Closed Bug 936165 Opened 9 years ago Closed 9 years ago

Allow for privileged users to edit past comments and preserve comment history (BMO extension)

Categories

(bugzilla.mozilla.org :: Extensions, defect)

Production
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dkl, Assigned: dkl)

References

Details

Attachments

(1 file, 4 obsolete files)

Discussion ongoing in bug 540 but stalled due to concerns on who would be doing the editing and whether it was a good idea. Due to recent abuse events on BMO it would be useful to be able to edit comments in special cases by BMO admins. I will be posting the current work over from bug 540 to the BMO/4.2 code base in order to provide a solution. 

dkl
Attached patch 936165_1.patch (obsolete) — Splinter Review
This is the working 'extension' version of this functionality. I will work on the native version now and we can compare the merits of both.

dkl
(In reply to David Lawrence [:dkl] from comment #1)
> This is the working 'extension' version of this functionality. I will work
> on the native version now and we can compare the merits of both.

a 'native' version needs to be build around bug 540's use-case, which is to allow any users to edit comments.  while the implementation will probably require a group to acquire edit-comments rights, any native version needs to accommodate sites which set this to editbugs.

of particular concern is the email notification of changes.  this is a non-issue for admin-only edits (don't email).  both versions need to deal with private comments correctly.
focusing on this version, we also need the ability to 'delete' comments.  this *cannot* be implemented as removing the row from the longdescs table as that changes the comment number of subsequent comments.  you could set the comment text to an empty string and mark it as private instead, and change bug.comments to skip those comments.
If bug.comments skips the comments, doesn't that run the risk of external clients or API users experiencing a comment renumbering?

Perhaps we could instead have a "comment removed" metadata flag?

Gerv
(In reply to Gervase Markham [:gerv] from comment #4)
> If bug.comments skips the comments, doesn't that run the risk of external
> clients or API users experiencing a comment renumbering?

no, we return the bug's number as part of the webservice result (comment.count).
OS: Linux → All
Hardware: x86_64 → All
Summary: Allow for privileged users to edit past comments and preserve comment history (BMO customization) → Allow for privileged users to edit past comments and preserve comment history (BMO extension)
Attached patch 936165_2.patch (obsolete) — Splinter Review
Attachment #828944 - Attachment is obsolete: true
Attachment #831097 - Flags: review?(glob)
Comment on attachment 831097 [details] [diff] [review]
936165_2.patch

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

please add a param for this instead of hard coding the group name to 'admin'.  i'm sure there'll be a requirement to extend this ability to more than just admins, especially in times of high volumes of comment spam.

::: extensions/EditComments/Extension.pm
@@ +55,5 @@
> +    my ($self, $args) = @_;
> +    my $page   = $args->{'page_id'};
> +    my $vars   = $args->{'vars'};
> +    my $user   = Bugzilla->user;
> +    my $params = Bugzilla->input_params;

nit: there's no need to do any of this unless $args->{page_id} matches

@@ +116,5 @@
> +    my $last_old_comment;
> +    my $count = 0;
> +    while (my $change_ref = $sth->fetchrow_hashref()) {
> +        my %change = %$change_ref;
> +        $change{'author'} = new Bugzilla::User($change{'userid'});

use the cache on bugzilla::user.  also better written as Bugzilla::User->new(..) rather than the old perl style.

@@ +165,5 @@
> +    my $user      = Bugzilla->user;
> +    my $dbh       = Bugzilla->dbh;
> +
> +    # Silently return if not in the proper group
> +    return if !$user->in_group('admin');

same issue here about performing initialisation work prior to checking if that work is required

@@ +197,5 @@
> +
> +        $updated = 1;
> +    }
> +
> +    $bug->_sync_fulltext() if $updated;

_sync_fulltext( update_comments => 1 )

::: extensions/EditComments/template/en/default/hook/bug/comments-a_comment-end.html.tmpl
@@ +9,5 @@
> +[% IF user.in_group("admin") %]
> +  <span id="edit_comment_links_[% comment.count FILTER html %]">
> +    [<a href="javascript:void(0);" id="edit_comment_edit_link_[% comment.count FILTER html %]"
> +        onclick="editComment('[% comment.count FILTER js %]','[% comment.id FILTER js %]');">edit</a>
> +    [% IF comment.activity.size - 1 > 0 %]

i have concerns about the performance overhead of making a separate db query for each comment on a bug, especially when all you're after is a count of the edits.

it would be better to have add an edit_count (or similar) column to the longdescs table which is incremented when edited.

::: extensions/EditComments/template/en/default/pages/editcomments.html.tmpl
@@ +14,5 @@
> + %]
> +
> +<script type="text/javascript">
> +<!--
> +/* The functions below expand and collapse comments  */

nit: remove <!-- and //--> .. we don't support browsers which don't recognise the <script> tag

@@ +18,5 @@
> +/* The functions below expand and collapse comments  */
> +function toggle_comment_display(link, comment_id) {
> +    var comment = document.getElementById('comment_text_' + comment_id);
> +    var re = new RegExp(/\bcollapsed\b/);
> +    if (comment.className.match(re))

this would be better written as:
  if (YAHOO.util.Dom.hasClass('comment_text_' + comment_id, 'collapsed')) { ..

@@ +21,5 @@
> +    var re = new RegExp(/\bcollapsed\b/);
> +    if (comment.className.match(re))
> +        expand_comment(link, comment);
> +    else
> +        collapse_comment(link, comment);

always use { braces } for if/else statements

@@ +40,5 @@
> +        var link = document.getElementById('comment_link_' + id);
> +        if (action == 'collapse')
> +            collapse_comment(link, comment);
> +        else
> +            expand_comment(link, comment);

{ braces } here too please

@@ +45,5 @@
> +    }
> +}
> +
> +function collapse_comment(link, comment) {
> +    link.innerHTML = "(+)";

use "[+]" and "[-]" to match the ui on show_bug

@@ +47,5 @@
> +
> +function collapse_comment(link, comment) {
> +    link.innerHTML = "(+)";
> +    link.title = "Expand the comment.";
> +    comment.className = "collapsed";

you need to use YAHOO.util.Dom.addClass() here.  if you collapse then expand a comment it loses its bz_comment_text class, dropping the formatting.

@@ +53,5 @@
> +
> +function expand_comment(link, comment) {
> +    link.innerHTML = "(-)";
> +    link.title = "Collapse the comment";
> +    comment.className = "";

use YAHOO.util.Dom.removeClass() here

@@ +62,5 @@
> +
> +function addCollapseLink(count) {
> +    document.write(' <a href="#" id="comment_link_' + count +
> +                   '" onclick="toggle_comment_display(this, ' +  count +
> +                   '); return false;" title="Collapse the comment.">(-)</a> ');

there's no requirement to support non-javascript browsers for admin functions, and document.write causes a layout reflow.
make this html part of the template.

@@ +83,5 @@
> +
> +[% count = 0 %]
> +[% FOREACH a = comment.activity %]
> +  <div class="bz_comment">
> +    <span class="bz_comment_head">

nit: the bz_comment_head span should probably be a div.

@@ +98,5 @@
> +          <span class="vcard">
> +            (<a class="fn email" href="mailto:[% a.author.email FILTER html %]">
> +            [%- a.author.email FILTER html -%]</a>)
> +          </span>
> +          on [%+ a.time FILTER time %]

probably easier to use global/user.html.tmpl in both locations here.

@@ +106,5 @@
> +        addCollapseLink([% count %]);
> +      </script>
> +    </span>
> +    [%# Don't indent the <pre> block, since then the spaces are displayed in the
> +      # generated HTML %]

you say that, yet the <pre> block is indented :)

::: extensions/EditComments/web/js/editcomments.js
@@ +5,5 @@
> + * This Source Code Form is "Incompatible With Secondary Licenses", as
> + * defined by the Mozilla Public License, v. 2.0.
> + */
> +
> +function editComment (comment_count, comment_id) {

nit: remove space before (
* applies to all functions in this file

@@ +17,5 @@
> +
> +    YAHOO.util.Connect.initHeader('Accept', 'application/json');
> +    YAHOO.util.Connect.asyncRequest(
> +        'GET',
> +        'rest/bug/comment/' + encodeURIComponent(comment_id),

you need to authenticate to edit private comments, or comments on a private bug.

@@ +35,5 @@
> +                    alert("Get comment failed: " + res.responseText);
> +                }
> +            }
> +        }
> +    );

as there's a delay between clicking on 'edit' and the ui changing, please show the editing ui with a div containing 'Loading...' which is replaced with the textarea once loaded.
Attachment #831097 - Flags: review?(glob) → review-
> also better written as Bugzilla::User->new(..) rather than the old perl style.

i was asked to clarify this on irc, figured it's worth a mention here:

http://modernperlbooks.com/mt/2009/08/the-problems-with-indirect-object-notation.html
http://shadow.cat/blog/matt-s-trout/indirect-but-still-fatal/
> Due to recent abuse events on BMO 


Since SPAM postings are not specific to BMO, is there a bug that discusses adding this functionality to Bugzilla?  I thought this was bug 540, but from the last comments, it appears not the case.

Right now every Bugzilla admin out there has to interact with SQL directly to remove Bugzilla garbage.
(In reply to Denis Roy from comment #9)
> Since SPAM postings are not specific to BMO, is there a bug that discusses
> adding this functionality to Bugzilla?  I thought this was bug 540, but from
> the last comments, it appears not the case.

i suspect a large reason for the debate/confusion/delays on bug 540 is the disparity between "allow users to edit their comments" and "allow admins to edit comments".  these are very different use-cases :)

> Right now every Bugzilla admin out there has to interact with SQL directly
> to remove Bugzilla garbage.

indeed -- which is part of the appeal of making this an extension.  it will hopefully be easily deployable to existing sites as soon as it's completed, without them having to wait for the next major bugzilla release.  as per my comment on that bug we should do this with the goal of upstreaming in the future, with more attention to ensuring it's clearly not designed to give end-users the ability to edit comments.
Attached patch 936165_3.patch (obsolete) — Splinter Review
Addressed all of the suggested changes. Also converted back to POST jsonrpc.cgi to handle private bugs/comments for now until we find a better way to do it via REST.

dkl
Attachment #831097 - Attachment is obsolete: true
Attachment #832521 - Flags: review?(glob)
Comment on attachment 832521 [details] [diff] [review]
936165_3.patch

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

editing an attachment's comment results in the attachment header being duplicated.  if you attach a file with an empty comment, when you edit that comment you get bugzilla's 'attachment created' generated text.

::: checksetup.pl
@@ +171,4 @@
>  # Set proper rights (--CHMOD--)
>  ###########################################################################
>  
> +#fix_all_file_permissions(!$silent);

oops

::: extensions/EditComments/Extension.pm
@@ +53,5 @@
> +    my $dbh = Bugzilla->dbh;
> +    if (!$dbh->bz_column_info('longdescs', 'edit_count')) {
> +        $dbh->bz_add_column('longdescs', 'edit_count',
> +                            { TYPE => 'INT3', DEFAULT => 0 });
> +    }

nit: you don't need the if(), bz_add_column does that for you.

@@ +64,5 @@
> +sub page_before_template {
> +    my ($self, $args) = @_;
> +    my $page   = $args->{'page_id'};
> +
> +    return if $page ne 'editcomments.html';

nit: no need for the $page

@@ +231,5 @@
> +
> +sub config_modify_panels {
> +    my ($self, $args) = @_;
> +    push @{ $args->{panels}->{groupsecurity}->{params} }, {
> +        name    => 'editcommentsgroup',

please name the param edit_comments_group to match all new params.
Attachment #832521 - Flags: review?(glob) → review-
Attached patch 936165_4.patch (obsolete) — Splinter Review
Attachment #832521 - Attachment is obsolete: true
Attachment #8334610 - Flags: review?(glob)
Comment on attachment 8334610 [details] [diff] [review]
936165_4.patch

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

::: extensions/EditComments/Extension.pm
@@ +101,5 @@
> +    no warnings 'redefine';
> +    *Bugzilla::Comment::activity = \&_get_activity;
> +    *Bugzilla::Comment::edit_count = \&_edit_count;
> +    *Bugzilla::WebService::Bug::_super_translate_comment = \&Bugzilla::WebService::Bug::_translate_comment;
> +    *Bugzilla::WebService::Bug::_translate_comment = \&_new_translate_comment;

while it's neat, i can't help but feel that monkey patching over an existing private method is setting ourselves up for a fall in the future.

i would rather this implemented via a new editcomments webservice which only returns the comment's body when provided a comment_id.
Attachment #8334610 - Flags: review?(glob) → review-
(In reply to Byron Jones ‹:glob› from comment #14)
> while it's neat, i can't help but feel that monkey patching over an existing
> private method is setting ourselves up for a fall in the future.
> 
> i would rather this implemented via a new editcomments webservice which only
> returns the comment's body when provided a comment_id.

Sigh. I was looking for a reason to use this trick on a current project too :) Ok will roll new patch post haste.

dkl
Attached patch 936165_5.patchSplinter Review
Attachment #8334610 - Attachment is obsolete: true
Attachment #8335545 - Flags: review?(glob)
Comment on attachment 8335545 [details] [diff] [review]
936165_5.patch

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

r=glob \o/

::: extensions/EditComments/lib/WebService.pm
@@ +35,5 @@
> +    foreach my $comment_id (@ids) {
> +        if (!$got_ids{$comment_id}) {
> +            ThrowUserError('comment_id_invalid', { id => $comment_id });
> +        }
> +    }

nit: you only need to check that length(@ids) == length(@$comment_data)
Attachment #8335545 - Flags: review?(glob) → review+
Committed to bugzilla-dev.allizom.org initially to get an idea on how long the longdescs table alter will take to ready for push to prod.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bmo/4.2-dev
added extensions/EditComments
added extensions/EditComments/Config.pm
added extensions/EditComments/Extension.pm
added extensions/EditComments/lib
added extensions/EditComments/template
added extensions/EditComments/web
added extensions/EditComments/lib/WebService.pm
added extensions/EditComments/template/en
added extensions/EditComments/template/en/default
added extensions/EditComments/template/en/default/hook
added extensions/EditComments/template/en/default/pages
added extensions/EditComments/template/en/default/hook/admin
added extensions/EditComments/template/en/default/hook/bug
added extensions/EditComments/template/en/default/hook/global
added extensions/EditComments/template/en/default/hook/admin/params
added extensions/EditComments/template/en/default/hook/admin/params/editparams-current_panel.html.tmpl
added extensions/EditComments/template/en/default/hook/bug/comments-a_comment-end.html.tmpl
added extensions/EditComments/template/en/default/hook/bug/show-header-end.html.tmpl
added extensions/EditComments/template/en/default/hook/global/user-error-auth_failure_object.html.tmpl
added extensions/EditComments/template/en/default/hook/global/user-error-errors.html.tmpl
added extensions/EditComments/template/en/default/pages/editcomments.html.tmpl
added extensions/EditComments/web/js
added extensions/EditComments/web/styles
added extensions/EditComments/web/js/editcomments.js
added extensions/EditComments/web/styles/editcomments.css
Committed revision 8599.

dkl
the _new_translate_comment monkeypatching still exists in the latest revision.

i've removed it from bugzilla-dev:

Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bmo/4.2-dev/
modified extensions/EditComments/Extension.pm
Committed revision 8602.
Depends on: 948693
Committed.

Committing to: bzr+ssh://dlawrence%40mozilla.com@bzr.mozilla.org/bmo/4.2
added extensions/EditComments
added extensions/EditComments/Config.pm
added extensions/EditComments/Extension.pm
added extensions/EditComments/lib
added extensions/EditComments/template
added extensions/EditComments/web
added extensions/EditComments/lib/WebService.pm
added extensions/EditComments/template/en
added extensions/EditComments/template/en/default
added extensions/EditComments/template/en/default/hook
added extensions/EditComments/template/en/default/pages
added extensions/EditComments/template/en/default/hook/admin
added extensions/EditComments/template/en/default/hook/bug
added extensions/EditComments/template/en/default/hook/global
added extensions/EditComments/template/en/default/hook/admin/params
added extensions/EditComments/template/en/default/hook/admin/params/editparams-current_panel.html.tmpl
added extensions/EditComments/template/en/default/hook/bug/comments-a_comment-end.html.tmpl
added extensions/EditComments/template/en/default/hook/bug/show-header-end.html.tmpl
added extensions/EditComments/template/en/default/hook/global/user-error-auth_failure_object.html.tmpl
added extensions/EditComments/template/en/default/hook/global/user-error-errors.html.tmpl
added extensions/EditComments/template/en/default/pages/editcomments.html.tmpl
added extensions/EditComments/web/js
added extensions/EditComments/web/styles
added extensions/EditComments/web/js/editcomments.js
added extensions/EditComments/web/styles/editcomments.css
Committed revision 9205.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This feature is live now.
Component: General → Extensions: EditComments
Component: Extensions: EditComments → Extensions
You need to log in before you can comment on or make changes to this bug.