283695 - Ability to "mark" or "tag" comments

Status: RESOLVED WONTFIX

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041223 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041223 Firefox/1.0

The new comment-highlight feature in bug #259452 is a great feature.  But it
would be nice if there was a UI for this.  Two options that I can think of:

  a. Have a "mark" field where a user could supply a list of comment #'s.  
  b. Have a "mark" toggle button next to each comment where a person with 
     "editbugs" privs can mark/unmark the comment

In both cases, I think the information should be stored in the field,
longdescs.mark.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Joel Peshkin]

Erik - does this overlap with your desire to have comment clasifications?  You
and Albert should talk.

Comment 2

[Albert Ting]

Attachment: initial 2.19.2+ patch

Here's my first cut at this feature.

   - new field, bugs.comment_marks
   - a new "usecommentmarks" parameter
   - each comment has a "mark" checkbox
   - there is also a "clear mark" javascript link at the top to clear all the 
     marks
   - you can still use the "&mark=N,N,N" URL feature.  It will override the 
     saved marks
   - bug_activies table updated accordingly

I wasn't sure how to define arrays in a bugzilla form, so currently using a
comma separated string.

Comment 3

[Albert Ting]

Attachment: patch, v2

Updated patch.	Per Dave, it now uses $cgi and properly fetches the comment
mark values as an array rather than a concatenated string.

Comment 4

[Joel Peshkin]

Wouldn't it be a a lot cleaner to add a mark column to the logdescs table
instead of adding a comma-sepated list to the bug table?

Comment 5

[Albert Ting]

> Wouldn't it be a a lot cleaner to add a mark column to the logdescs table
> instead of adding a comma-sepated list to the bug table?

Well, I *thought* I had a good reason to keep it in the bugs table, but I can't
remember!  Maybe disk space concerns.  But if the team thinks this should be in
longdescs, then sure, I'll move it.  Anything else? 

Comment 6

[Frédéric Buclin]

Comment on attachment 179319 [details] [diff] [review]
patch, v2

Reading joel' comments, erik seems to be a better reviewer for this patch.

Comment 7

[Albert Ting]

Comment on attachment 179319 [details] [diff] [review]
patch, v2

(In reply to comment #4)
> Wouldn't it be a a lot cleaner to add a mark column to the logdescs table
> instead of adding a comma-sepated list to the bug table?

> Wouldn't it be a a lot cleaner to add a mark column to the logdescs table
> instead of adding a comma-sepated list to the bug table?

I took a stab at moving the mark column in longdescs and I don't like my
first cut.  Main issues:

  - there is no unique id for each comment, so I'm tagging them based on
    the bug_when value.  It's a minor nit pick, but if there are two
    comments posted within the same second, both will be highlighted.

  - I'm not sure how best to highlight comment mark changes in the
    bug_activity.  I could rescan the longdescs table, convert the marks to
    a list of comment numbers, and put that in bug_activity.  But that
    seems cumbersome to me.  I didn't have to change any bug_activity
    related code with the original approach.

Comments?

Comment 8

[Albert Ting]

Attachment: v2 (longdescs style)

For what it's worth, here's the longdescs style

Comment 9

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 10

[Albert Ting]

Attachment: v3

Updated patch based on the bugzilla tip.  I think the original approach makes the most sense.

Comment 11

[Olav Vitters]

Reassining to the person making the patches.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 203472 [details] [diff] [review]
v3

OK, a few comments:

* I do think that it should be in the longdescs table. It's OK that you have to use bug_when -- that's the same thing we do for "private."

The param should have underscores. use_comment_marks. It makes it easier to read.

I haven't looked too much further into the patch, but those are my initial thoughts.

Comment 13

[Albert Ting]

(In reply to comment #12)
> I do think that it should be in the longdescs table. It's OK that you have to
> use bug_when -- that's the same thing we do for "private."

Max, if I went with the longdescs approach and used bug_when, how would I report any changes in the bug_activity page?  I liked the fact that I didn't have to change any bug_activity code with the original approach.

Comment 14

[Max Kanat-Alexander]

(In reply to comment #13)
> Max, if I went with the longdescs approach and used bug_when, how would I
> report any changes in the bug_activity page?  I liked the fact that I didn't
> have to change any bug_activity code with the original approach.

  Just do things exactly the same way as the "private" checkbox does them, unless it does something silly that I'm not aware of.

Comment 15

[Albert Ting]

(In reply to comment #14)
> Just do things exactly the same way as the "private" checkbox does them,
> unless it does something silly that I'm not aware of.

I just tried using the "private" checkbox.  It doesn't record checkbox changes in the view-bug-activity page.  But I would think both "private" and "comment_marks" changes ought to be recorded in the bug-activity page? 

Comment 16

[Max Kanat-Alexander]

> I just tried using the "private" checkbox.  It doesn't record checkbox changes
> in the view-bug-activity page.  But I would think both "private" and
> "comment_marks" changes ought to be recorded in the bug-activity page? 

  Yeah, everything should be recorded in bug_activity. But if "private" doesn't current record itself in bug_activity, that can be fixed in another bug.

  You should be able to get the bug_id from the form, in addition to the bug_when. That's all you need for bug_activity...

Comment 17

[Albert Ting]

(In reply to comment #16)
> > I just tried using the "private" checkbox.  It doesn't record checkbox changes
> > in the view-bug-activity page.  But I would think both "private" and
> > "comment_marks" changes ought to be recorded in the bug-activity page? 
> 
>   Yeah, everything should be recorded in bug_activity. But if "private" doesn't
> current record itself in bug_activity, that can be fixed in another bug.
> 
>   You should be able to get the bug_id from the form, in addition to the
> bug_when. That's all you need for bug_activity...
> 

If I select 5 comments to be highlighted, do you want to record 5 "bug_when's" or 5 comment id's.  Either way, I now have to traverse both old-highlight and highlight to determine what changed and go from there.  

Comment 18

[Max Kanat-Alexander]

(In reply to comment #17)
> If I select 5 comments to be highlighted, do you want to record 5 "bug_when's"
> or 5 comment id's.  Either way, I now have to traverse both old-highlight and
> highlight to determine what changed and go from there.  

  Generally -- I'd say that you should record the comment_id in the bug_activity table, if that's the question. Yes, I know that this is difficult. If you'd like, we could make this bug depend on bug 225221.

Comment 19

[Albert Ting]

(In reply to comment #18)
> Generally -- I'd say that you should record the comment_id in the
> bug_activity table, if that's the question. Yes, I know that this is difficult.
> If you'd like, we could make this bug depend on bug 225221.

Then may I suggest we treat this as two bugs?  The first is the highlight feature itself, similar to the private comment feature.  The second would be to update the bug_activity table after bug 225221 has been fixed (for both private and highlight).

Comment 20

[Max Kanat-Alexander]

(In reply to comment #19)
> Then may I suggest we treat this as two bugs?

  Sure, sounds great to me. Just make sure that you file the second bug and give it the right dependency.

Comment 21

[Max Kanat-Alexander]

*** Bug 324647 has been marked as a duplicate of this bug. ***

Comment 22

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 23

[Albert Ting]

Attachment: v4

First pass on an updated patch based on the 12/26/06 tip.  It now uses a longdescs.mark field, and takes advantage of the longdescs.comment_id to determine which comment is being marked.  Bug-Activity table gets updated accordingly.

Comment 24

[Max Kanat-Alexander]

Comment on attachment 250243 [details] [diff] [review]
v4

I'd love to get to this, but unfortunately I won't be able to.

Comment 25

[Frédéric Buclin]

Comment on attachment 250243 [details] [diff] [review]
v4

>     $dbh->bz_add_column('milestones', 'id',
>         {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1});

Why do we want to store marked comments permanently (also meaning: why do we want a UI for it)? I mean I can understand that we add &mark=1,2,3 at the end of a URL to highlight some specific comments in a discussion, but I'm not sure we want to store this information, especially because I have no idea what a highlighted comment means (they depend on the context). And without a clear definition of highlighted comments, I don't see how this is useful as anyone could mark comments based on different criteria.

Also, what happens if you highlight a private comment (which colors win?)?


>+  use_comment_marks => "Do you wish to use comment marks?",

Do we *really* need a parameter for that? I would say no as you can bypass it by hacking the URL.


>+                        my $markid = $cgi->param("$field");
>+                        detaint_natural($markid);
>+                        $sth->execute($mark, $markid);

What happens if $markid is not an integer? You should ignore it instead of inserting 'undef' in the DB.
Also, do you make sure the user is allowed to highlight the comment, e.g. if he tries to highlight a private comment he cannot see (by hacking the URL)?


>+          <input type="hidden" name="omark-[% count %]" 
>+                 value="[% comment.mark %]">

No, do not use old-foo hidden fields. We are removing them due to race conditions, I don't want them back. This information is already stored in the DB; use it.


>+          <input type="checkbox" name="mark-[% count %]" value="1"
>+                 onClick="updateCommentMark(this, [% count %])"
>+                 id="mark-[% count %]"
>+                 [% " checked=\"checked\"" IF comment.mark %]>

If the user is not allowed to edit this checkbox, it should not be displayed. Where do you make sure the user has editbugs privs?

Comment 26

[Albert Ting]

> Why do we want to store marked comments permanently (also meaning: why do we 
> want a UI for it)?

Main advantage is to make it easier to scan a long-winded ticket and highlight the key topics/agreements.  Plus we want to permanently highlight it.  This is what my company is using this patch for and it works quite well.  

I'll leave it up to the bugzilla team to decide if this is worth including as part of the bugzilla codebase

Comment 27

[Max Kanat-Alexander]

Yes, the idea of marking comments permanently as important is valuable and I would include it in Bugzilla.

Comment 28

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 30

[Ben Bucksch (:BenB)]

LpSolit, there are a number of important bugs in bugzilla.mozilla.com, which are overrun by users and comments, because they are so important, and ignored by developers, because they have so many comments that they don't feel like reading through them, but feel they'd need to to understand the issues and previous important comments, thus devs leave the bug alone, which (partially) results in many important bugs never been fixed. This is a real issue.
Also, in theses bugs, comments tend to repeat, adding to the above problem and effects. See e.g. bug 147777 comment 86.
So, it would be useful for somebody trusted (e.g. editbugs accounts or assignee) to mark certain comments as important and standing out, so that users (and devs) who see the bug would read it.

Comment 31

[Max Kanat-Alexander]

(In reply to comment #30)
> This is a real issue.

  Yes, I agree. :-) See comment 27. We just need somebody to implement, or revise the current patch.

Comment 32

[Albert Ting]

I've been tied up but perhaps later this year.  Still, there are some open questions:  

> What happens if you highlight a private comment (which colors win?)?
I don't have a good answer.  Any suggestions?

>>+  use_comment_marks => "Do you wish to use comment marks?",
> Do we *really* need a parameter for that? I would say no as you can bypass it
> by hacking the URL.

Should this always be enabled?

Some other questions:

- Who should have privs to set a mark?  Should there be another bz group or anybody who can enter a comment?
- I got a request to also support a "strikeout" feature, to "archive" any comments that are obsolete.  It would still be visible, but greyed out.  This could also address the "edit long comment" request some folks have had.

Comment 33

[Max Kanat-Alexander]

(In reply to comment #32)
> > What happens if you highlight a private comment (which colors win?)?
> I don't have a good answer.  Any suggestions?

  We could give .bz_private a border that's always there even if the comment is marked.

> Should this always be enabled?

  Yes, it should always be enabled. We don't ever want to have parameters that turn features on or off, ever again, if we can help it.

> - Who should have privs to set a mark?  Should there be another bz group or
> anybody who can enter a comment?

  A new group that edibugs inherits by default. Some installations might want to limit it to assignees and other "super users" (who should always have the ability).

> - I got a request to also support a "strikeout" feature, to "archive" any
> comments that are obsolete.  It would still be visible, but greyed out.  This
> could also address the "edit long comment" request some folks have had.

  That would be a separate bug where we could discuss whether or not we want such a feature. Probably better would just be to collapse the comment by default.

Comment 34

[Ben Bucksch (:BenB)]

> What happens if you highlight a private comment (which colors win?)?

Given that important comments should - by nature - be rare, and private also has a checkbox which shows the private state independent of colors, important should win.

> Should there be another bz group or anybody who can enter a comment?

Anybody who can enter a comment being able to set important is obviously useless - many people wrongly think that their output is important. Agreed with Max' reply.

Comment 35

[Albert Ting]

> > Should there be another bz group or anybody who can enter a comment?
> Anybody who can enter a comment being able to set important is obviously
> useless - many people wrongly think that their output is important. Agreed with
> Max' reply.

My company is allowing anybody to mark and it's working out quite well. Self-monitoring is in place as we have a record in the bug activity page whenever somebody adds/removes a mark.  

Still, it doesn't hurt to have another group for this, which could be set to everybody.

Comment 37

[u81239]

(In reply to comment #34)
> Anybody who can enter a comment being able to set important is obviously
> useless - many people wrongly think that their output is important. Agreed with
> Max' reply.

Why not implement a comment voting up/down feature like e.g. Slashdot.org or Tweakers.net or Thottbot.com do? So that if someone incorrectly votes up his own post, others can vote it down again. From what I can see, on those sites that works really well. Of course, in a more closed environment such as my company’s Bugzilla, it wouldn’t be an issue anyway (and they could just give anyone marking privileges).

But I would definitely find the functionality very useful, I’ve made plenty of comments on my company’s bug tracking system that I would like to highlight (because they contain the core description of the issue/fix) or, equally important, hide (because they contain incorrect information).

Comment 38

[Max Kanat-Alexander]

(In reply to comment #37)
> Why not implement a comment voting up/down feature like e.g. Slashdot.org or
> Tweakers.net or Thottbot.com do?

  I think that'd make a good extension.

Comment 39

[Frédéric Buclin]

(In reply to comment #37)
> Why not implement a comment voting up/down feature like e.g. Slashdot.org or
> Tweakers.net or Thottbot.com do?

I don't see this as useful, even as an extension (sorry Max). Bugzilla is not a forum; its purpose is not to vote on the usefulness of comments.

Comment 41

[Frédéric Buclin]

(In reply to comment #30)
> LpSolit, there are a number of important bugs in bugzilla.mozilla.com, which
> are overrun by users and comments, because they are so important, and ignored
> by developers, because they have so many comments that they don't feel like
> reading through them

OK, OK, you win. I changed my mind since 2007. Go for it! :)

Comment 43

[Gyozo PAPP]

Marked and "unmarked" comments can be searched separately in the Advanced Search Page. Using boolean charts suffice my needs but probably someone else may like to distinct entries for marked/unmarked comments in the "Bug Changes" fieldset.

It may be discussed earlier (probably in a dupped bug) or it may even be trivial for Bugzilla developers. In this case sorry for the noise.

Comment 45

[Dale Forester]

RE: #44: Interesting discussion here. I recommend looking at bug 562920 for ideas that might help here:

https://bugzilla.mozilla.org/show_bug.cgi?id=562920

Comment 47

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is too invasive or too risky to be accepted for 4.4 at this point. The target milestone is set to 5.0 which is our next major release.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else on time for 5.0.

Comment 50

[:glob ✱]

a superset of the requested functionality here (boolean marking of comments) has been implemented in bug 793963 (arbitrary tagging of bugs).

marking this bug as WONTFIX as we don't need both systems.