793963 - add the ability to tag comments with arbitrary tags

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[:glob ✱]

allow users with editbugs to add tags to individual comments.

this will give users the ability to thread conversations, mark comments as spam, identify important comments, etc, without a large reworking of the bugzilla ui.

Comment 1

[:glob ✱]

Attachment: patch v1

first shot at the comment-tagging implementation

Comment 2

[Simon Green]

15:59 < simon> No change to B/WebService/Bug.pm to add/show tags in comments?
16:00 < simon> That would seem like an important thing to me.
16:00 < simon> I think you should be able to set a tag when adding a new comment, and show tags when retriving comments.

Comment 3

[:glob ✱]

random notes:

- i've created a CommentTag class, which loads just the fields required to update a comment.  this is to avoid loading the comment's blob just to update a tag.  we may want to extend this to cover all fields except for thetext, because there are other places in bugzilla where we're loading an entire comment just to deal with the metadata

- i just noticed i forgot to add Regexp::Common to the module dependency list.  i'll add that in a later revision (or drop this feature if we think it isn't appropriate)

- i haven't written the perldoc for the webservice calls yet

(In reply to Simon Green from comment #2)
> 15:59 < simon> No change to B/WebService/Bug.pm to add/show tags in comments?
> 16:00 < simon> That would seem like an important thing to me.
> 16:00 < simon> I think you should be able to set a tag when adding a new
> comment, and show tags when retriving comments.

- good point, will do

Comment 4

[:glob ✱]

Attachment: screenshot

Comment 5

[Simon Green]

another thought, what about having a predefined list of tags that uses YUI autocomplete (like it does with keywords), and an params option to enforce the tags to only use the predefined list, or allow any tag.

I'm thinking that if that option isn't available then I'll have obsolete, opsolete, obsollete and obselet as tags in my database :)

Comment 6

[:glob ✱]

(In reply to Simon Green from comment #5)
> another thought, what about having a predefined list of tags that uses YUI
> autocomplete (like it does with keywords), and an params option to enforce
> the tags to only use the predefined list, or allow any tag.
> 
> I'm thinking that if that option isn't available then I'll have obsolete,
> opsolete, obsollete and obselet as tags in my database :)

the code already uses yui's autocomplete drawing from existing tags, which should go some way to minimising typos.

adding a hard coded list of tags would be possible, but if required that's something that i think should be added at a later date.

Comment 7

[Frédéric Buclin]

Note that if we implement this bug, then bug 283695 will be wontfix'ed. We don't need both.

Comment 8

[Frédéric Buclin]

Comment on attachment 664365 [details] [diff] [review]
patch v1

>=== added file 'Bugzilla/CommentTag.pm'

>+use Regexp::Common;

>+    $tag =~ /$RE{profanity}/
>+        and ThrowUserError('comment_tag_invalid', { tag => $tag });

I don't see the point to use profanity here. Users allowed to tag comments are trusted users, or are expected to be. If they aren't, they should have their privs to tag bugs removed. Also, I suspect this regexp is english-centric and doesn't take other languages into account. This check should go away, especially because Regexp::Common is not in the core Perl distribution, and I don't want to force someone to install it to tag comments.



>=== added file 'Bugzilla/CommentTags.pm'

I don't see why you need separate modules besides Bugzilla::Comment just to tag comments. Everything should go into Bugzilla/Comment.pm. For bugs, we don't have Bug::Reporter, Bug::StatusWhiteBoard or anything like that, and it wouldn't make sense to have them. Here too, it doesn't make sense to have them.


>+sub increment {
>+    my ($self) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    $self->{'weight'}++;
>+    $dbh->do('UPDATE longdescs_tags SET weight = weight + 1 WHERE id = ?', undef, $self->id);
>+}
>+
>+sub decrement {
>+    my ($self) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    $self->{'weight'}--;
>+    $dbh->do('UPDATE longdescs_tags SET weight = weight - 1 WHERE id = ?', undef, $self->id);
>+    $dbh->do('DELETE FROM longdescs_tags WHERE id = ? AND weight <= 0', undef, $self->id)
>+        if $self->{'weight'} <= 0;
>+}

So the only goal of these two methods is to have a cache to improve performance, like we had for votes? Do we have any evidence that this is useful? As comment tags are stored outside the longdescs table, it should remain pretty small, and with indexes in the table, it should remain pretty fast. Also, the DELETE statement above doesn't need the 'AND weight <= 0' part, because you already check this in Perl. If they don't match, you are already in trouble.



>=== modified file 'Bugzilla/Config/BugFields.pm'

>+   name => 'collapsed_comment_tags',
>+   type => 't',
>+   default => 'obsolete,spam',

Please add a whitespace after the comma.



>=== modified file 'Bugzilla/Config/GroupSecurity.pm'

>+sub _check_comment_taggers_group {
>+    my $group_name = shift;
>+    if ($group_name && !Bugzilla->feature('jsonrpc')) {
>+        return "Comment tagging requires installation of the JSONRPC feature";
>+    }
>+    return check_group($group_name);
>+}

All checkers must go into Bugzilla/Config/Common.pm and return data in the expected way.



>=== modified file 'Bugzilla/DB/Schema.pm'

>+            tags            => {TYPE => 'varchar(255)'}

IMO, comment tags should be stored in a separate table. I think it's bad design to concatenate them here, especially to run queries including comment tags. The table should have |bug_id, comment_id, tag| as columns, with an index on bug_id and tag. And this would let you remove the artificial restriction of 10 tags per comment.


>+    longdescs_tags => {
>+        FIELDS => [
>+            id     => { TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1 },
>+            tag    => { TYPE => 'varchar(24)',  NOTNULL => 1 },
>+            weight => { TYPE => 'INT3',         NOTNULL => 1 },
>+        ],
>+    },

'tag' must be UNIQUE.


>+    longdescs_tags_activity => {

Why a separate table for comment tags while comment privacy in stored in bugs_activity? It's a pity to clone a whole table. Also, it will need indexes for perf reasons.




>=== modified file 'Bugzilla/Field.pm'

>+    {name => 'comment_tag',           desc => 'Comment Tag', buglist => 0},

Nit: buglist => 0 is already the default value.



>=== modified file 'Bugzilla/Install/DB.pm'

>+    $dbh->bz_add_column('longdescs', 'tags', { TYPE => 'varchar(255)' });

As I said, I think it's a very bad idea to add this column which is used only as cache.



>=== modified file 'Bugzilla/User.pm'

>+sub can_tag_comments {

Please add POD for this method.



>=== added file 'Bugzilla/WebService/CommentTag.pm'

This must go into Bug.pm. No reason to have Bug.add_comment, Bug.comments and Bug.update there and stuff about comment tags here. That's totally inconsistent. All you need to do is to update Bug.add_comment and Bug.update to accept a 'tags' hash. You can already set comment privacy with these methods, so reuse them.


Also, do not forget POD for all the new methods you have in the patch.

Comment 9

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #8)

> The table should have |bug_id, comment_id, tag| as columns, with an
> index on bug_id and tag.

I take that back. It should have |comment_id, tag| only. The bug ID can already be retrieved thanks to the longdescs table, and we can join both tables with a LEFT JOIN to restrict the query to comments from a given bug only.

Comment 10

[:glob ✱]

(In reply to Frédéric Buclin from comment #8)
> I don't see the point to use profanity here.

no worries, will remove (as per comment 3).

> I don't see why you need separate modules besides Bugzilla::Comment just to
> tag comments.

see comment 3

> So the only goal of these two methods is to have a cache to improve performance, like
> we had for votes? Do we have any evidence that this is useful? As comment tags are
> stored outside the longdescs table[..]

no, they are not for caching, they update the data.  comment tags /are/ stored in the longdescs table, not outside of it.

> >=== modified file 'Bugzilla/DB/Schema.pm'
> 
> >+            tags            => {TYPE => 'varchar(255)'}
> 
> IMO, comment tags should be stored in a separate table. I think it's bad
> design to concatenate them here, especially to run queries including comment
> tags. The table should have |bug_id, comment_id, tag| as columns, with an
> index on bug_id and tag. And this would let you remove the artificial
> restriction of 10 tags per comment.

yeah, i went through a few design rounds with tags in a separate table, or in a single field on the longdescs table.

my concern with a separate table is that would require a join for each comment in the bug, and i have concerns over the performance impact this would have.  i ended up choosing to optimise for showing bugs rather than searching.

> >+    longdescs_tags_activity => {
> 
> Why a separate table for comment tags while comment privacy in stored in
> bugs_activity?

i was informed that it would be problematic to have entries in bugs_activity without updating a bug's last-modified timestamp, so they had to go into a different table.

> >=== modified file 'Bugzilla/Install/DB.pm'
> 
> >+    $dbh->bz_add_column('longdescs', 'tags', { TYPE => 'varchar(255)' });
> 
> As I said, I think it's a very bad idea to add this column which is used
> only as cache.

it isn't a cache, it contains the actual tags.

> This must go into Bug.pm. No reason to have Bug.add_comment, Bug.comments and Bug.update
> there and stuff about comment tags here. That's totally inconsistent. All you need to do
> is to update Bug.add_comment and Bug.update to accept a 'tags' hash. You can already set
> comment privacy with these methods, so reuse them.

i can move them into Bug.pm.  as per comment 3 i'll also integrate the calls into the normal webservice calls (but without triggering updates to a bug's last-modified timestamp, etc), but to avoid unnecessarily loading a full bug comment object i'll shift these lightweight methods into Bug.pm and continue to call them from our web pages.

Comment 11

[Albert Ting]

While we may not want bug 283695 if this bug gets addressed, I think there is a difference in use model that I hope still gets addressed.  Bug 283695 supports a pre-defined "mark" tag (similar to "private"), where the comment gets auto-highlighted.  In fact, I prefer to only see an admin-managed list of available tags as opposed user-defined tags.

Comment 12

[:glob ✱]

Attachment: patch v2

this revision:
  - tracks tags in the longdecs_tags table
  - changes thetext loading in comments to lazy
  - preloads text and tags for all comments when showing a bug
  - moves methods into bugzilla/comment.pm and uses standard update mechanisms
  - moves methods into bugzilla/webservice/bug.pm

note: there's no perldocs as i want to wait for the implementation to be finalised before documenting it.

Comment 13

[:glob ✱]

(In reply to Frédéric Buclin from comment #8)
> So the only goal of these two methods is to have a cache to improve
> performance, like we had for votes? Do we have any evidence that this is
> useful?

with the implementation in patch 1 it was required, and wasn't an performance tweak.
with patch 2, it is a performance tweak.

as the query will be executed with each *keypress* for autocomplete, performance is critical.  using a separate table avoids a group-by, which requires a temp table.

Comment 15

[Frédéric Buclin]

Comment on attachment 667382 [details] [diff] [review]
patch v2

I don't know which part of the JS code is responsible for this, but it doesn't let me add tags with accents, which is pretty common on french (and in german too). Also, I would ideally like to be able to type "spam, ignore" and then see it split as two tags. Also, for usability, could it be possible to display the text box where we click the [tag] link? This way there is no need to move the mouse to click the text box on the left of the comment area. Another thing which confused me is that if I type a tag which is two characters long, then when I hit the "Enter" keystroke, it submits the whole page instead of the tag only. That wasn't expected. If I type one more character, only the tag is submitted. Maybe a "error: tag too short" message should be displayed besides the tag field (in red)?

Comment 16

[Frédéric Buclin]

Note that you have two minor bitrots, which are easily fixed manually:

one in Bug.pm, where you replaced

  ORDER BY bugs_activity.bug_when";

by

  ORDER BY bugs_activity.bug_when, bugs_activity.id";


and one in bug/edit.html.tmpl, where I removed the indentation of

  <script type="text/javascript">
  <!--

which appears right below your new code.

Comment 17

[Frédéric Buclin]

Comment on attachment 667382 [details] [diff] [review]
patch v2

>=== added file 'Bugzilla/CommentTagWeights.pm'

Wouldn't it make more sense to have it as a subclass of Bugzilla::Comment, i.e. Bugzilla::Comment::TagWeights? I personally have some doubts about its usefulness. All this could be added to Bugzilla/Comment.pm directly.



>=== modified file 'Bugzilla/Config/BugFields.pm'

>+   name => 'collapsed_comment_tags',

You forgot to add its description into admin/params/bugfields.html.tmpl.

Comment 18

[Frédéric Buclin]

Comment on attachment 667382 [details] [diff] [review]
patch v2

r- due to the comments above (especially accents are a showstopper for non-english users), but I only focused on its usability for now. I really didn't focus on the code itself yet. But the UI looks very good. :)

Comment 19

[:glob ✱]

(In reply to Frédéric Buclin from comment #17)
> >=== added file 'Bugzilla/CommentTagWeights.pm'
>
> Wouldn't it make more sense to have it as a subclass of Bugzilla::Comment,
> i.e. Bugzilla::Comment::TagWeights? I personally have some doubts about its
> usefulness. All this could be added to Bugzilla/Comment.pm directly.

i wrote it this way because the weights are in in their own table, and using Bugzilla::Object saves a whole lot of code. Bugzilla::Object requires one package per table, so it can't be folded into Bugzilla/Comment.pm.

Comment 20

[:glob ✱]

(In reply to Frédéric Buclin from comment #15)

> - it doesn't let me add tags with accents
> - like to be able to type "spam, ignore" and then see it split as two tags
> - if I type a tag which is two characters long, then when I hit Enter keystroke, it
>   submits the whole page instead of the tag only
> - Bugzilla::CommentTagWeights --> Bugzilla::Comment::TagWeights

all fixed in next revision

> Also, for usability, could it be possible to display the text box where we
> click the [tag] link? 

no, there isn't enough room for this, especially for users with long names, and i think it makes more sense for the text box to be placed where the tags are displayed.

> there is no need to move the mouse to click the text box on the left
> of the comment area.

the text box is automatically focused, there's no need to click on it.

Comment 21

[:glob ✱]

Attachment: patch v3

Comment 22

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #21)
> Created attachment 688735 [details] [diff] [review]
> patch v3

Bitrot in several files. Will attempt to continue review but please attach updated patch.

dkl

Comment 23

[David Lawrence [:dkl]]

t/011pod.t ........... 105/344 
#   Failed test 'Bugzilla/Comment.pm POD coverage is 83%. Undocumented methods: collapsed, remove_tag, add_tag, tags'
#   at t/011pod.t line 103.

#   Failed test 'Bugzilla/CommentTagWeights.pm POD coverage is 0%'
#   at t/011pod.t line 117.

#   Failed test 'Bugzilla/Config/Common.pm POD coverage is 96%. Undocumented methods: check_comment_taggers_group'
#   at t/011pod.t line 103.
t/011pod.t ........... 270/344 
#   Failed test 'Bugzilla/User.pm POD coverage is 98%. Undocumented methods: can_tag_comments'
#   at t/011pod.t line 103.

#   Failed test 'Bugzilla/WebService/Bug.pm POD coverage is 85%. Undocumented methods: search_comment_tags, add_comment_tag, remove_comment_tag'
#   at t/011pod.t line 103.
# Looks like you failed 5 tests of 344.
t/011pod.t ........... Dubious, test returned 5 (wstat 1280, 0x500)
Failed 5/344 subtests

Comment 24

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #23)
> Undocumented methods

those tests were added after this patch was created.
i'll provide an updated patch.

Comment 25

[David Lawrence [:dkl]]

Comment on attachment 688735 [details] [diff] [review]
patch v3

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

1. Need REST Resources for the new webservice methods.

2. template/en/default/bug/comments.html.tmpl: s/count/comment.count/

::: Bugzilla/Bug.pm
@@ +3831,5 @@
> +        push @args, $self->id;
> +        push @args, $starttime if defined $starttime;
> +    }
> +
> +    $query .= "ORDER BY bugs_activity.bug_when, bugs_activity.id";

Unknown column 'bugs_activity.bug_when' and 'bugs_activity.id in 'order clause'

Change to $query .= "ORDER BY bug_when";

::: Bugzilla/Comment.pm
@@ +82,5 @@
>      my $self = shift;
> +    my ($changes, $old_comment) = $self->SUPER::update(@_);
> +
> +    if (exists $changes->{'thetext'} || exists $changes->{'isprivate'}) {
> +        $self->bug->_sync_fulltext();

$self->bug->_sync_fulltext( update_comments => 1 );

@@ +91,5 @@
> +    my ($removed_tags, $added_tags) = diff_arrays(\@old_tags, \@new_tags);
> +
> +    if (@$removed_tags || @$added_tags) {
> +        my $dbh = Bugzilla->dbh;
> +        my $when = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');

Should this not be an optional parameter to update() so we can reuse the same timestamp from $bug->update so as to group other changes with these?

@@ +210,5 @@
> +    $self->{'tags'} ||= Bugzilla->dbh->selectcol_arrayref(
> +        "SELECT tag
> +           FROM longdescs_tags
> +          WHERE comment_id = ?
> +          ORDER BY tag -- $$ $self",

debugging?

::: Bugzilla/Config/BugFields.pm
@@ +89,5 @@
> +  {
> +   name => 'collapsed_comment_tags',
> +   type => 't',
> +   default => 'obsolete, spam',
> +  });

Needs a description added to template/en/default/admin/params/bugfields.html.tmpl

::: Bugzilla/User.pm
@@ +1835,5 @@
> +        my $group = Bugzilla->params->{'comment_taggers_group'};
> +        $self->{'can_tag_comments'} =
> +            ($group && $self->in_group($group)) ? 1 : 0;
> +    }
> +    return $self->{'can_tag_comments'};

Just to clarify, does an empty group mean anyone can tag or tagging is disabled completely? If the former then this will need to be updated and the param description in editparams.cgi should describe it that way.

::: Bugzilla/WebService/Bug.pm
@@ +330,5 @@
> +    Bugzilla->login(LOGIN_REQUIRED);
> +    Bugzilla->user->can_tag_comments
> +        || ThrowUserError("auth_failure", { group  => Bugzilla->params->{'comment_taggers_group'},
> +                                            action => "add",
> +                                            object => "comment_tags"});

If Bugzilla->params->{'comment_taggers_group'} is empty then can_tag_comments is negative and the failure will be called with an empty value for 'group'.

@@ +361,5 @@
> +    Bugzilla->login(LOGIN_REQUIRED);
> +    Bugzilla->user->can_tag_comments
> +        || ThrowUserError("auth_failure", { group  => Bugzilla->params->{'comment_taggers_group'},
> +                                            action => "remove",
> +                                            object => "comment_tags"});

same as above

@@ +377,5 @@
> +    defined $tag || ThrowCodeError('param_required', { param => 'tag' });
> +
> +    my $dbh = Bugzilla->dbh;
> +    $dbh->bz_start_transaction();
> +    $comment->remove_tag($tag);

Should we be able to remove multiple tags (delimited by ',') similar to add_comment_tag()?

@@ +391,5 @@
> +    Bugzilla->login(LOGIN_REQUIRED);
> +    Bugzilla->user->can_tag_comments
> +        || ThrowUserError("auth_failure", { group  => Bugzilla->params->{'comment_taggers_group'},
> +                                            action => "search",
> +                                            object => "comment_tags"});

same as above

::: skins/standard/show_bug.css
@@ +132,5 @@
> +.bz_comment_tag {
> +    border: 1px solid #c8c8ba;
> +    padding: 0px 2px;
> +    margin: 2px;
> +    border-radius: 0.5em;

The border is very faint on my test system and could use very slightly more padding. Maybe use a darker outline or colored backrgound.

::: template/en/default/admin/params/groupsecurity.html.tmpl
@@ +29,5 @@
>    querysharegroup => "The name of the group of users who can share their " _
>                       "saved searches with others.",
>  
> +  comment_taggers_group => "The name of the group of users who can tag comments.",
> +

Should add to description that empty means off or means anyone.

::: template/en/default/list/list.html.tmpl
@@ -72,4 @@
>  
>    [% IF debug %]
>      <div class="bz_query_debug">
> -      <p>[% query FILTER html %]</p>

This change should not be here.

Comment 26

[:glob ✱]

thanks for the r+.

there's a lot of changes to touch on commit, i think it's safer to attach a new patch with the revisions :)

> @@ +91,5 @@
> > +    my ($removed_tags, $added_tags) = diff_arrays(\@old_tags, \@new_tags);
> > +
> > +    if (@$removed_tags || @$added_tags) {
> > +        my $dbh = Bugzilla->dbh;
> > +        my $when = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> 
> Should this not be an optional parameter to update() so we can reuse the
> same timestamp from $bug->update so as to group other changes with these?

as changes to tags are committed as soon as they are made, it isn't possible to group them with other changes.

> If Bugzilla->params->{'comment_taggers_group'} is empty then
> can_tag_comments is negative and the failure will be called with an empty
> value for 'group'.

ah, that param shouldn't be empty; i'll update the validator.

> The border is very faint on my test system and could use very slightly more
> padding. Maybe use a darker outline or colored backrgound.

no worries, i'll play with the border colour and padding.

Comment 27

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #26)
> thanks for the r+.
> 
> there's a lot of changes to touch on commit, i think it's safer to attach a
> new patch with the revisions :)

Ugh. Should have waited til I had a nights sleep. fat fingered it as that was supposed to be r- so I am glad you doing a new patch anyway ;)

> as changes to tags are committed as soon as they are made, it isn't possible
> to group them with other changes.

Correct. Realize that now as they are not being made on Submit along with other changes.

dkl

Comment 28

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #26)
> ah, that param shouldn't be empty; i'll update the validator.

actually it needs to support an empty param to disable comment tagging.  i'll update the description and address the errors thrown.

> > The border is very faint on my test system and could use very slightly more
> > padding. Maybe use a darker outline or colored backrgound.
> 
> no worries, i'll play with the border colour and padding.

that border colour is the same as the comment's border on dusk.  it looks weird to have a darker colour :(

Comment 29

[:glob ✱]

Attachment: 793963_4.patch

this addresses review points, along with the following major changes:

- changed the webservice to a single update_comment_tags method which accepts add & remove (simplifies code and is more aligned with other methods)

- removed the lazy-loading of comment.thetext
  - the idea with the lazy-loading was to avoid loading it when setting tags
  - bug->comments always calls comments->preload, which loads the full text even
    with lazy loading, so i was adding an additional sql query with no benefit

Comment 30

[David Lawrence [:dkl]]

Comment on attachment 831518 [details] [diff] [review]
793963_4.patch

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

::: Bugzilla/Config/Common.pm
@@ +526,4 @@
>  
>  =item check_smtp_ssl
>  
> +=item check_comment_taggers_group

Nit: add a function description instead of adding the in need of pod list.

::: Bugzilla/DB/Schema.pm
@@ +414,5 @@
> +                                            COLUMN => 'comment_id',
> +                                            DELETE => 'CASCADE' }},
> +            tag        => { TYPE => 'varchar(24)',  NOTNULL => 1 },
> +        ],
> +    },

unique index for (comment_id, tag)?

::: Bugzilla/WebService/Bug.pm
@@ +337,5 @@
> +
> +    # Don't load comment tags unless enabled
> +    if (Bugzilla->params->{'comment_taggers_group'}) {
> +        $comment_hash->{tags} = $comment->tags;
> +    }

nit:

if (Bugzilla->params->{'comment_taggers_group'}) {
    my @tags;
    foreach my $tag (@{ $comment->tags }) {
        push(@tags, $self->type('string', $tag);
    }
    $comment_hash->{tags} = \@tags;
}

@@ +1010,5 @@
>  
> +sub update_comment_tags {
> +    my ($self, $params) = @_;
> +
> +    Bugzilla->login(LOGIN_REQUIRED);

my $user = Bugzilla->login(LOGIN_REQUIRED);

Since you need to use Bugzilla->user in several places you can use $user instead.

::: Bugzilla/WebService/Constants.pm
@@ +102,5 @@
> +    # Comment tagging
> +    comment_tag_disabled => 125,
> +    comment_tag_invalid => 126,
> +    comment_tag_too_long => 127,
> +    comment_tag_too_short => 128,

Would be nice if these were in order by I realize you are trying to group them with the other comment related errors.

::: js/comment-tagging.js
@@ +11,5 @@
> +    ctag_div  : false,
> +    ctag_add  : false,
> +    counter   : 0,
> +    min_len   : 3,
> +    max_len   : 24,

min_len and max_len should come from the constants?

@@ +40,5 @@
> +            resultsList : "result"
> +        };
> +
> +        var ac = new YAHOO.widget.AutoComplete('bz_ctag_add', 'bz_ctag_autocomp', ds);
> +        ac.maxResultsDisplayed = 7;

Why 7 here and limit=10 in the query params?

Comment 31

[:glob ✱]

Attachment: 793963_5.patch

(In reply to David Lawrence [:dkl] from comment #30)
> Review of attachment 831518 [details] [diff] [review]:
> ::: js/comment-tagging.js
> @@ +11,5 @@
> > +    ctag_div  : false,
> > +    ctag_add  : false,
> > +    counter   : 0,
> > +    min_len   : 3,
> > +    max_len   : 24,
> 
> min_len and max_len should come from the constants?

as that's a .js file we can't get to the constants from there.
they are set from the constants in bug/edit.html.tmpl:
> YAHOO.bugzilla.commentTagging.min_len = [% constants.MIN_COMMENT_TAG_LENGTH FILTER js %];
> YAHOO.bugzilla.commentTagging.max_len = [% constants.MAX_COMMENT_TAG_LENGTH FILTER js %];

Comment 32

[David Lawrence [:dkl]]

Comment on attachment 8334390 [details] [diff] [review]
793963_5.patch

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

I figured out what was going on with the tag wackiness when adding multiple at once. If you type something like "this new tag" and hit enter, it is firing off three separate Bug.update_comment_tag requests asynchronously. Whichever one finishes last is the final list of tags display (even though all three were eventually store in the db). So you may see just one of the tags or maybe two but usually not all three. If you reload the bug page, you will see all of the tags correctly. I would suggest updating it to combine the multiple tags into a single Bug.update_tag_comments if possible. Otherwise r=dkl if you want to address this as a later bug.

dkl

Comment 33

[Frédéric Buclin]

Comment on attachment 8334390 [details] [diff] [review]
793963_5.patch

>=== added directory 'Bugzilla/Comment'

>+        foreach my $tag (@$removed_tags) {

>+            $dbh->do(
>+                "DELETE FROM longdescs_tags WHERE comment_id = ? AND tag = ?",
>+                undef, $self->id, $tag);
>+            $dbh->do(
>+                "INSERT INTO longdescs_tags_activity
>+                (bug_id, comment_id, who, bug_when, added, removed)
>+                VALUES (?, ?, ?, ?, ?, ?)",
>+                undef, $self->bug_id, $self->id, Bugzilla->user->id, $when, '', $tag);

When involved in a loop, always prepare SQL statements outside the loop, and then call $sth->execute(). This always make a huge performance difference.


>+        foreach my $tag (@$added_tags) {

Same here.



>=== added file 'Bugzilla/Comment/TagWeights.pm'

>+use base qw(Bugzilla::Object);

We now require Perl 5.10.1, so use |use parent| instead of |use base|, which is no longer recommended.

Comment 34

[:glob ✱]

Attachment: 793963_6.patch

even though the last patch got an r+, i'd like to get another set of eyes on the fixes i've incorporated which consolidate setting of multiple tags into a single call, as well as lpsolit's points.

Comment 35

[:glob ✱]

Attachment: 793963_7.patch

fixes an issue where adding the same tag twice at the same time results in no tags being added.

Comment 36

[David Lawrence [:dkl]]

Comment on attachment 8336595 [details] [diff] [review]
793963_7.patch

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

To much fanfare, r=dkl. One nit pick I just saw after your recent change is that if you are now able to add multple tags in one API call, the bugs_activity entry should have the tags together instead if separate entries. You can change this in a later bug or on commit.

\o/
dkl

Comment 37

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified show_activity.cgi
modified Bugzilla/Bug.pm
added Bugzilla/Comment
modified Bugzilla/Comment.pm
modified Bugzilla/Constants.pm
modified Bugzilla/Field.pm
modified Bugzilla/User.pm
added Bugzilla/Comment/TagWeights.pm
modified Bugzilla/Config/BugFields.pm
modified Bugzilla/Config/Common.pm
modified Bugzilla/Config/GroupSecurity.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/Server/REST/Resources/Bug.pm
added js/comment-tagging.js
modified js/comments.js
modified js/util.js
modified skins/standard/show_bug.css
modified template/en/default/admin/params/bugfields.html.tmpl
modified template/en/default/admin/params/groupsecurity.html.tmpl
modified template/en/default/bug/comments.html.tmpl
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/show-header.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 8817.

Comment 38

[:glob ✱]

fix for:
206 - --WARNING template/en/default/global/user-error.html.tmpl has 1 unused error tag(s):
# user error tag 'tag_name_invalid' is defined at line(s) (1747) but is not used anywhere

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified template/en/default/global/user-error.html.tmpl
Committed revision 8818.

Comment 39

[Frédéric Buclin]

In very old installations, some bugs may have no comments, and so Bugzilla crashes if you try to access these bugs. I fixed this bustage:

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
Committed revision 8819.

Comment 40

[Thomas D. (:thomas8)]

Byron, thanks for this feature, this comes with great potential. :)

Comment 41

[Frédéric Buclin]

Added to relnotes for 5.0rc1.