616185 - Move tags (aka lists of bugs) to their own DB tables

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement, P1)

Description

[Frédéric Buclin]

To improve the tagging system, we want to be able to edit them without having to commit changes to the page, i.e. we would use something like AJAX (pyrzak said YUI can do it, yay!) to only edit tags, but leave the bug report alone. As mkanat said in bug 372023 comment 0, something similar to the keyword autocompletion system would be great (start typing a tag name, and Bugzilla completes it for you based on your existing tag names).

But to do this, we need a webservice method which lets us set tags (I suggest Bug.set_tags as name). A method to get tags is not required as we can load them when loading the bug report.

Once this WS method is available, pyrzak will be able to write the front-end code (in a separate bug I'm going to file in a few minutes). Being myself not very familiar with writing WS methods (as I never had to), I wondered if timello would agree to help us here.

If the webservice method needs backend code, such as refactoring how we save tags, I'm willing to help, as I wrote the original code (my idea is to create a new DB table which would have three columns: user ID, bug ID, tag name). I hope mkanat is fine with this proposal. :)

Comment 1

[Tiago Mello [:timello]]

(In reply to comment #0)
> 
> Once this WS method is available, pyrzak will be able to write the front-end
> code (in a separate bug I'm going to file in a few minutes). Being myself not
> very familiar with writing WS methods (as I never had to), I wondered if
> timello would agree to help us here.
> 

This is great! I can help here for sure. Just assign the bug to me.

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> This is great! I can help here for sure. Just assign the bug to me.

Great, thanks! :)

I will file the refactor bug once mkanat gives me his approval for my proposal in comment 0.

Comment 3

[Max Kanat-Alexander]

I think it's a great idea to have this as part of the WebService, but how about we just add it to Bug.update instead?

Comment 4

[Max Kanat-Alexander]

For the DB tables, too, I think it might be best to have:

tag: tag_id, user_id, tag_name
bug_tag: bug_id, tag_id

So that it's a bit more like a multi-select that's user-based.

Comment 5

[Frédéric Buclin]

(In reply to comment #3)
> I think it's a great idea to have this as part of the WebService, but how about
> we just add it to Bug.update instead?

I don't think that's a good idea, because personal tags really aren't part of a bug, and the job of Bug.update is to update the bug itself. Also, Bug.update affects fields which are global, while personal tags are user-specific.


(In reply to comment #4)
> For the DB tables, too, I think it might be best to have:
> 
> tag: tag_id, user_id, tag_name
> bug_tag: bug_id, tag_id

This brings no value, and adds complexity. You would need to bind tag.tag_id and bug_tag.tag_id together every time you want to look at tags. The tag name will already be an identifier as when you alter the tag name in a bug, you are basically creating a new tag and deleting the old one (so you will never have the same tag ID with a different tag name). My proposal is easier, and unlike multi-select fields, tags are free text forms.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> I don't think that's a good idea, because personal tags really aren't part of a
> bug, and the job of Bug.update is to update the bug itself. Also, Bug.update
> affects fields which are global, while personal tags are user-specific.

  That's true. I suppose a separate function would be good for now, because trying to work user tags into Bugzilla::Bug->update() would be tricky (since they wouldn't update delta_ts).

> > tag: tag_id, user_id, tag_name
> > bug_tag: bug_id, tag_id
> 
> This brings no value, and adds complexity.

  Actually, no, it does bring some significant value. First off, it brings us database normalization, which is usually good from a theoretical perspective. But in the practical sense, it makes bug_tag (which could be an enormous table with millions of rows) into a fixed-width table containing only integers, while "tag" is a much smaller table containing the variable-length strings.

> The tag name
> will already be an identifier as when you alter the tag name in a bug, you are
> basically creating a new tag and deleting the old one (so you will never have
> the same tag ID with a different tag name). My proposal is easier, and unlike
> multi-select fields, tags are free text forms.

  Ah, I was still imagining tags would be created automatically--there doesn't need to be a separate UI to populate the "tag" table.


  The only problem here is that I'm certain people will want to share tags (and even make them publicly accessible), and since you're about to decouple them from the saved search system, that would mean that we'd have to implement a whole *separate* sharing system, which I don't want to do. Do you have a solution for that?

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
>   That's true. I suppose a separate function would be good for now

OK, cool. So I will stick with Bug.set_tags. :)


>   Actually, no, it does bring some significant value. First off, it brings us
> database normalization, which is usually good from a theoretical perspective.
> But in the practical sense, it makes bug_tag (which could be an enormous table
> with millions of rows) into a fixed-width table containing only integers, while
> "tag" is a much smaller table containing the variable-length strings.

Database normalization remains optimal as tag names are identifiers themselves. The unique key would be (bug_id, user_id, tag_name) instead of (bug_id, tag_id), but this isn't worse, from a normalization point of view. But I see your point. Your 2nd argument about having a fixed-width table convinces me much more. ;)


>   Ah, I was still imagining tags would be created automatically--there doesn't
> need to be a separate UI to populate the "tag" table.

Sure, I don't want to have a separate UI to populate the tag table either. If you type a tag name which you already typed in the past, it will reuse its ID, else it will create a new ID for this new name. No separate UI needed. And if a tag is no longer used in any bug, then we would delete its ID as well, automatically.


>   The only problem here is that I'm certain people will want to share tags (and
> even make them publicly accessible), and since you're about to decouple them
> from the saved search system, that would mean that we'd have to implement a
> whole *separate* sharing system, which I don't want to do. Do you have a
> solution for that?

Yes. The current URL for saved search is of the form:

 buglist.cgi?cmdtype=runnamed&namedcmd=foo

which triggers a call to LookupNamedQuery() which finds the query of the form buglist.cgi?bug_id=1,2,3,4,....

With the separate tag table(s), we would have a URL of the form:

 buglist.cgi?cmdtype=runnamed&namedcmd=foo&tag=1

This will trigger the exact same call to LookupNamedQuery(), and this subroutine already takes the query type as its 3rd argument, so it would now take tag=1 instead of QUERY_LIST. If LookupNamedQuery() gets this 3rd argument, it will look at the tag table(s) instead of the namedqueries table. It will create the buglist URL on the fly, to reflect the current behavior. We would create Bugzilla::Search::Tag->new() to do all the work in a centralized way (which will also be required by the webservice method), which would have a similar role to the existing Bugzilla::Search::Saved->new() method. You would then call the ->url method on this object, as we do currently.

If you want to share the tag-based buglist with other users, you simply append &sharer_id=123456 to the URL as we do now.

Comment 8

[Max Kanat-Alexander]

Hmm. Okay. Do we even really need a new table, though? I mean, couldn't we implement the new tags system as you describe it, but just using the old saved search table? Then we wouldn't even need &tag=1. I think we should just start out using the existing database model, and figure out a different one when we find a case where we actually require it.

Comment 9

[Frédéric Buclin]

(In reply to comment #8)
> Hmm. Okay. Do we even really need a new table, though?

Yes, we do. The plan is to display tags a bug belongs to in the bug itself (bug 616191), and also as a column in buglists (bug 616192). But we don't want to parse all saved searches to extract those containing the desired bug ID.

Comment 10

[Max Kanat-Alexander]

(In reply to comment #9)
> Yes, we do. The plan is to display tags a bug belongs to in the bug itself (bug
> 616191), and also as a column in buglists (bug 616192). But we don't want to
> parse all saved searches to extract those containing the desired bug ID.

  Ah, okay, I see why you need a new table then.

  In that case, I think you're going to hit an architectural issue, once you want to search within a tag. It will be much simpler if your search URL looks like:

  buglist.cgi?tag=my_tag

  And that's it, except for shared searches which would have something like:

  buglist.cgi?tag=my_tag&tag_owner=[e-mail]

  Or something like that.

Comment 11

[Frédéric Buclin]

(In reply to comment #10)
>   In that case, I think you're going to hit an architectural issue, once you
> want to search within a tag. It will be much simpler if your search URL looks
> like:
> 
>   buglist.cgi?tag=my_tag
> 
>   And that's it, except for shared searches which would have something like:
> 
>   buglist.cgi?tag=my_tag&tag_owner=[e-mail]

Oh, the only reason I added cmdtype=runnamed&namedcmd=foo was because this way, we immediately enter the correct IF block in buglist.cgi (which is how the current URLs for saved searches look like, being regular queries or a list of bugs). If we simply pass tag=my_tag, we would have to hack buglist.cgi to automatically append this omitted part of the URL.

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> Oh, the only reason I added cmdtype=runnamed&namedcmd=foo was because this way,
> we immediately enter the correct IF block in buglist.cgi (which is how the
> current URLs for saved searches look like, being regular queries or a list of
> bugs). If we simply pass tag=my_tag, we would have to hack buglist.cgi to
> automatically append this omitted part of the URL.

  Yeah, I was thinking that too. But how about we implement tag searching *entirely* in Search.pm instead? That is, just make it a part of the normal search criteria, and don't worry about creating a Bugzilla::Search::Tag class (and instead just add the relevant code to Bugzilla::Bug for updating tags).

Comment 13

[Frédéric Buclin]

This would work for sure, if you are willing to implement this in Search.pm. :) In this case, I suggest to do it in two steps: first, I file a bug to create the new DB table(s) and move existing lists of bugs to this new table(s) and fix buglist.cgi to start looking at it when looking for and creating new tags. I would probably fix it myself as I wrote the original code. Then, in a separate bug, you fix Search.pm to directly get tag=my_tag itself, instead of letting buglist.cgi do it. This means you will be able to also remove the temporary hack I will add to buglist.cgi during the transition. Or do you want to do it all at once in a single bug?

Comment 14

[Max Kanat-Alexander]

Ah, I do think that we should do it in two bugs. However, I think that you should simply *remove* the existing tag code from buglist.cgi, and then I can do the Search.pm stuff separately. I don't think we need to have temporary hack code in buglist.cgi--there just won't be any way to search for tags on trunk, for a brief period.

Comment 15

[Max Kanat-Alexander]

Also, since we talked so much about refactoring in this bug, maybe this bug should be the parent refactoring bug and we should file a separate bug for the WebServices part.

Comment 16

[Frédéric Buclin]

(In reply to comment #15)
> Also, since we talked so much about refactoring in this bug, maybe this bug
> should be the parent refactoring bug and we should file a separate bug for the
> WebServices part.

OK, makes sense. So to make sure, do we want to go with 2 new tables, as you suggested in comment 4, or do we finally stay with only 1 new table as I suggested in comment 0?

Comment 17

[Max Kanat-Alexander]

I think we want two tables, just for the value of it being fixed-width if nothing else.

Comment 18

[Frédéric Buclin]

I cloned this bug as bug 616336 for the WebService method. This one will be about the DB code.

Comment 19

[Frédéric Buclin]

Attachment: WIP, v0.9

The current UI remains fully functional with this patch, but 1) we no longer automatically create a saved search with the given tag name (the user will have to create a query with this tag name as search criteria if he plans to share it or have it displayed in the page footer), and 2) it's no longer possible to query for bugs tagged with some given name (this will be fixed in bug 616341).

Said otherwise, we are removing some hacks from the code, which is good, and we no longer consider tags as "special", i.e. we don't create a saved search for them (for comparison, we don't create a saved search for priority P1, P2, etc..., or any other bug attribute, so we are consistent).

This WIP patch leaves the code in buglist.cgi for now. That's the patch I initially planned to submit for review. But I will do some more code refactoring, which is moving as much code as possible into Bugzilla::Bug::add_tags(), Bugzilla::Bug::remove_tags() and Bugzilla::Bug::tags(). I already implemented Bugzilla::User::tags(), as I needed it in a template. I wonder if a new Bugzilla::Tag class would make sense. But I suppose this is not needed, at least for now.

Comment 20

[Frédéric Buclin]

Attachment: patch, v1

OK, here is a fully tested patch, with the relevant code moved into Bugzilla::Bug. Once the new UI is in place, the remaining code in buglist.cgi will go away.

Comment 21

[David E. Ross]

What impact will this have on existing tags and searches based on them?

Comment 22

[Frédéric Buclin]

(In reply to comment #21)
> What impact will this have on existing tags and searches based on them?

Tags and existing searches will stay. But you won't be able to use them for a little while till bug 616341 is fixed. This will be done on time for Bugzilla 4.2rc1, though.

Comment 23

[Max Kanat-Alexander]

Comment on attachment 500137 [details] [diff] [review]
patch, v1

  This is a really sweet cleanup. :-) Some thoughts, though:

>Index: Bugzilla/User.pm
>+             LEFT JOIN bug_tag ON bug_tag.tag_id = tags.id
>+             WHERE user_id = ? ' . $dbh->sql_group_by('id', 'name'),
>+            'name', undef, $self->id);

  Hmm. Would it be simpler here to just return an arrayref? I think it probably would be. That's what we ended up doing with all the other stuff that's like this (like groups).

>Index: Bugzilla/Bug.pm
>+sub _check_tag_name {
>+    my ($invocant, $tag) = @_;
>+
>+    $tag = clean_text($tag);
>+    $tag || ThrowUserError('no_tag_to_edit');
>+    trick_taint($tag);

  Hmm. Should we check that its name isn't too long, also?

>@@ -2844,6 +2853,80 @@ sub remove_see_also {
>     $self->{see_also} = \@new_see_also;
> }
> 
>+sub add_tag {
>+        $tag_id = $dbh->selectrow_array('SELECT id FROM tags
>+                                         WHERE name = ? AND user_id = ?',
>+                                         undef, ($tag, $user->id));
>+        # The list has changed.
>+        delete $user->{tags};

  I think we should have a $user->clear_tags_cache method, in case we start to have other user methods around tags.

>+    # Increment the counter. Do it before the SQL call below,
>+    # to not count the tag twice.
>+    $user->tags->{$tag}->{bug_counter}++;

  That should be bug_count, no?

>+sub tags {
>+        my %tags = @{$dbh->selectcol_arrayref(
>+            'SELECT id, name FROM bug_tag
>+             INNER JOIN tags ON tags.id = bug_tag.tag_id
>+             WHERE bug_id = ? AND user_id = ?',
>+             {Columns => [1,2]}, ($self->id, $user->id))};

  Ah, I think it would be better to use an arrayref of hashrefs here, too.

  Also, SQL formatting should be more like our normal SQL formatting.

>Index: Bugzilla/DB/Schema.pm
>+    bug_tag => {
>+        INDEXES => [
>+            bug_tag_bug_id_idx => {FIELDS => [qw(bug_id tag_id)], TYPE => 'UNIQUE'},
>+            bug_tag_tag_id_idx => ['tag_id'],

  Hmm, why the index on tag_id alone?

>Index: Bugzilla/Install/DB.pm
>+    my $sth1 = $dbh->prepare('INSERT INTO tags (user_id, name) VALUES (?, ?)');
>+    my $sth2 = $dbh->prepare('INSERT INTO bug_tag (bug_id, tag_id) VALUES (?, ?)');
>+    my $sth3 = $dbh->prepare('UPDATE namedqueries SET query = ?
>+                              WHERE userid = ? AND name = ?');

  Would be nice for those to have clearer names, like sth_tags, sth_bug_tags, sth_nq

>Index: template/en/default/global/user-error.html.tmpl
>+  [% ELSIF error == "no_tag_to_edit" %]
>     [% title = "No Tag Selected" %]
>-    You didn't select a tag from which to remove [% terms.bugs %].
>+    You didn't select any tag [%~ IF action == "add" %] nor created a new one [%~ END %].

  You tried to create or remove a tag with no name.

Comment 24

[Frédéric Buclin]

(In reply to comment #23)
>   Hmm. Would it be simpler here to just return an arrayref?

The way I use tags in the code, this is much easier for me as I can directly walk through hashes, without having to loop over an array first.


>   Hmm. Should we check that its name isn't too long, also?

I can check its length, sure.


>   I think we should have a $user->clear_tags_cache method, in case we start to
> have other user methods around tags.

I think this can wait till we effectively have such another method which would also need it.


>   That should be bug_count, no?

Oh, wow, yes! Thanks for catching that. :)


>   Ah, I think it would be better to use an arrayref of hashrefs here, too.

Hum, no, I really want to have a hashref in hands, to not having to loop over an array everytime I want something.


>   Hmm, why the index on tag_id alone?

Because you asked me to do so:

(18:45:09) mkanat: LpSolit: Also, I will need an index on tag_id, I think, on bug_tag.
(18:45:23) LpSolit: mkanat: for Search.pm?
(18:45:25) mkanat: LpSolit: Oh, also, the index you have there needs to be named bug_tag_bug_id_idx.
(18:45:29) mkanat: LpSolit: Yeah, for Search.pm.
(18:47:54) LpSolit: mkanat: http://bugzilla.pastebin.mozilla.org/882376
(18:48:20) mkanat: LpSolit: Yeah, looks awesome. :-)

So I already had bug_tag_bug_id_idx, and you asked me for an index on tag_id anyway. Did I misunderstand something? :)


>   Would be nice for those to have clearer names, like sth_tags, sth_bug_tags,
> sth_nq

OK, will do it.

Comment 25

[Frédéric Buclin]

Attachment: patch, v1.1

Carrying forward mkanat's r+. I will wait for an answer about the tag_id index before committing this patch (it's just one line removal if we don't need it).

Comment 26

[Max Kanat-Alexander]

Comment on attachment 500137 [details] [diff] [review]
patch, v1

This should have been an r-, my mistake.

Comment 27

[Max Kanat-Alexander]

(In reply to comment #24)
> The way I use tags in the code, this is much easier for me as I can directly
> walk through hashes, without having to loop over an array first.

  Ahh, actually, I'm certain that it would be simpler to return an arrayref. You should then also implement a has_tag method for Bugzilla::Bug to check if there's already a tag on it.

> I think this can wait till we effectively have such another method which would
> also need it.

  That sounds fine.

> Hum, no, I really want to have a hashref in hands, to not having to loop over
> an array everytime I want something.

  I think having an arrayref of tags and then a has_tag method will be good enough. Every single other method in Bugzilla::Bug and Bugzilla::User that is like this returns arrayrefs.

> So I already had bug_tag_bug_id_idx, and you asked me for an index on tag_id
> anyway. Did I misunderstand something? :)

  Oh...I'm not sure if the index is needed now, or not, actually. Let's not have the tag_id index yet, and if we need it, I'll add it as part of the Search.pm patch.

Comment 28

[Max Kanat-Alexander]

Okay, I see how having tags as a hashref in the User object is valuable. However, since "my $user" is always Bugzilla->user, in Bugzilla::Bug, at the least there's no reason to have a hashref there.

Comment 29

[Max Kanat-Alexander]

I do suspect that the hashref in Bugzilla::User will make things complex in the future, though--whenever we start to use hashrefs like this (instead of arrays of objects), things get complex eventually.

Comment 30

[Max Kanat-Alexander]

(In reply to comment #29)
> I do suspect that the hashref in Bugzilla::User will make things complex in the
> future, though--whenever we start to use hashrefs like this (instead of arrays
> of objects), things get complex eventually.

  But maybe not. :-) So I'm okay to keep the hashref in Bugzilla::User for now.

Comment 31

[Frédéric Buclin]

Attachment: patch, v2

Remove the useless index, and use an arrayref for $bug->tags.

Comment 32

[Max Kanat-Alexander]

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

Frickin' awesome. Thank you! :-)

Comment 33

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified buglist.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/Constants.pm
modified Bugzilla/User.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified Bugzilla/Search/Saved.pm
modified template/en/default/global/messages.html.tmpl
modified template/en/default/global/per-bug-queries.html.tmpl
modified template/en/default/global/user-error.html.tmpl
Committed revision 7687.

Comment 34

[Frédéric Buclin]

Attachment: fix bustage

I committed this fix when I realized that in Bugzilla 3.x, commas are encoded as %2C while Bugzilla 2.22 was storing them unencoded. This was making PostgreSQL to fail.

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

Comment 35

[Frédéric Buclin]

OK, reopening! For some reason, some lists of bugs (aka tags) are stored in the namedqueries DB table with queries like:

 bug_id=123&columnlist=bug_severity%2Cpriority%2Cassigned_to%2Cbug_status%2Cresolution%2Cop_sys%2Cshort_desc

 bug_id=1;columnlist=bug_severity%2Cpriority%2Cassigned_to%2Cbug_status%2Cresolution%2Cshort_desc%2Cproduct%2Ccomponent%2Cqa_contact_realname

i.e. the columnlist is appended to the list of bugs. Nowhere in the code in buglist.cgi is there anything about appending the columnlist. I suspect colchange.cgi to mess with it. And of course, Bugzilla::Install::DB::_migrate_user_tags() didn't expect to found columnlist in the query, and parses it incorrectly. This makes tinderbox to fail.

Comment 36

[Frédéric Buclin]

Ah yes, colchange.cgi is the culprit:

    if ($cgi->param('save_columns_for_search')
        && defined $search && $search->user->id == Bugzilla->user->id) 
    {
        my $params = new Bugzilla::CGI($search->url);
        $params->param('columnlist', join(",", @collist));
        $search->set_url($params->query_string());
        $search->update();
    }

I will need to find a clever regexp to parse this correctly.

Comment 37

[Frédéric Buclin]

Attachment: fix bustage, part 2

Correctly parse queries with the columnlist appended to the query. I added the two INSERT SQL instructions in eval {}, in case the DB has been only partially populated. Note that I didn't test this patch as my two 4.1 test installations have already been successfully upgraded, and so I don't enter this code anymore when running checksetup.pl.

Comment 38

[Frédéric Buclin]

Attachment: fix bustage, part 2

I prefer the $columnlist = $2 if $2 syntax.

Comment 39

[Frédéric Buclin]

Comment on attachment 508236 [details] [diff] [review]
fix bustage, part 2

I tested this patch on both MySQL and PostgreSQL, and it now correctly fixes all testcases I could imagine: list of bugs with columnlist appended, list of bugs with no columnlist, a single bug ID. So I committed this patch to prevent further breakage, and I will leave the request for review just in case.

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

Comment 40

[Max Kanat-Alexander]

(In reply to comment #35)
> i.e. the columnlist is appended to the list of bugs. Nowhere in the code in
> buglist.cgi is there anything about appending the columnlist. I suspect
> colchange.cgi to mess with it. And of course,
> Bugzilla::Install::DB::_migrate_user_tags() didn't expect to found columnlist
> in the query, and parses it incorrectly. This makes tinderbox to fail.

  The best solution here would have been to have URI parse the query string and actually extract what you needed.

(In reply to comment #37)
> Correctly parse queries with the columnlist appended to the query. I added the
> two INSERT SQL instructions in eval {}, in case the DB has been only partially
> populated. 

  That will hide any other possible errors, though, too, so I'd be somewhat concerned about it. For installations that were in a bad state, they only would have been there for a few days, so I'd just advise them to do a TRUNCATE TABLE and start again. After all, you did put the update code inside of a transaction to handle just this sort of thing, right?

Comment 41

[Frédéric Buclin]

(In reply to comment #40)
> After all, you did put the update code inside of a transaction
> to handle just this sort of thing, right?

No, I didn't add transactions.

Comment 42

[Max Kanat-Alexander]

Attachment: Redesign Install::DB code

I talked with LpSolit on IRC and he agreed that it was OK for me to redesign this code. We figure that the eval was only necessary for the few hours that automated updaters would have hit the previous problem, so we don't need it anymore.

I also added indicate_progress and fixed a problem with trying to migrate a saved search that had bug_id=1,001.

Comment 43

[Max Kanat-Alexander]

Comment on attachment 508577 [details] [diff] [review]
Redesign Install::DB code

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Install/DB.pm
modified template/en/default/setup/strings.txt.pl
Committed revision 7692.

Comment 44

[David E. Ross]

What is the version in which this is fixed?

Comment 45

[Frédéric Buclin]

See the target milestone: 4.2.

Comment 46

[Tiago Mello [:timello]]

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

>Index: Bugzilla/Bug.pm
>===================================================================
>+sub add_tag {
>+    my ($self, $tag) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    my $user = Bugzilla->user;
>+    $tag = $self->_check_tag_name($tag);
>+
>+    my $tag_id = $user->tags->{$tag}->{id};
>+    # If this tag doesn't exist for this user yet, create it.
>+    if (!$tag_id) {
>+        $dbh->do('INSERT INTO tags (user_id, name) VALUES (?, ?)',
>+                  undef, ($user->id, $tag));
>+
>+        $tag_id = $dbh->selectrow_array('SELECT id FROM tags
>+                                         WHERE name = ? AND user_id = ?',
>+                                         undef, ($tag, $user->id));
>+        # The list has changed.
>+        delete $user->{tags};
>+    }
>+    # Do nothing if this tag is already set for this bug.
>+    return if grep { $_ eq $tag } @{$self->tags};
>+
>+    # Increment the counter. Do it before the SQL call below,
>+    # to not count the tag twice.
>+    $user->tags->{$tag}->{bug_count}++;
>+
>+    $dbh->do('INSERT INTO bug_tag (bug_id, tag_id) VALUES (?, ?)',
>+              undef, ($self->id, $tag_id));
>+
>+    push(@{$self->{tags}}, $tag);
>+}

I'm just wondering why we are not doing as we do for the see_also here. Instead of doing the INSERT here
why don't we just store the added tags in a $self->{added_tags} and insert them in the ->update method?

The same for the remove_tag.

Comment 47

[Tiago Mello [:timello]]

Since we are going to use the Bug.update in the Bug 616336, I think that should be treated in the Bugzilla::Bug->update, shouldn't it?

Comment 48

[Tiago Mello [:timello]]

Also, I think we should add a $self->_add_remove($params, 'tags'); in the Bugzilla::Bug->set_all method, otherwise the add and remove method won't be called.

Comment 49

[Frédéric Buclin]

(In reply to comment #47)
> Since we are going to use the Bug.update in the Bug 616336, I think that should
> be treated in the Bugzilla::Bug->update, shouldn't it?

I don't think we should use Bug.update to handle tags, which is why we don't follow what see_also does. Tags shouldn't update delta_ts, tags shouldn't send any email, tags shouldn't be updated at the same time as the bug itself.