From 8fa9965e5476717e574f2674c6df8c4487874634 Mon Sep 17 00:00:00 2001 From: Frédéric Buclin Date: Sun, 30 Jan 2011 13:07:59 +0100 Subject: Bug 616185: Move tags (aka lists of bugs) to their own DB tables r/a=mkanat --- Bugzilla/Bug.pm | 83 ++++++++++++++ Bugzilla/Constants.pm | 7 -- Bugzilla/DB/Schema.pm | 31 ++++- Bugzilla/Install/DB.pm | 48 ++++++-- Bugzilla/Search/Saved.pm | 12 +- Bugzilla/User.pm | 23 ++++ buglist.cgi | 125 ++++++--------------- template/en/default/global/messages.html.tmpl | 11 ++ .../en/default/global/per-bug-queries.html.tmpl | 16 +-- template/en/default/global/user-error.html.tmpl | 33 ++---- 10 files changed, 235 insertions(+), 154 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index 398843009..e516e4bf8 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -1895,6 +1895,17 @@ sub _check_strict_isolation_for_user { } } +sub _check_tag_name { + my ($invocant, $tag) = @_; + + $tag = clean_text($tag); + $tag || ThrowUserError('no_tag_to_edit'); + ThrowUserError('tag_name_too_long') if length($tag) > MAX_LEN_QUERY_NAME; + trick_taint($tag); + # Tags are all lowercase. + return lc($tag); +} + sub _check_target_milestone { my ($invocant, $target, undef, $params) = @_; my $product = blessed($invocant) ? $invocant->product_obj @@ -2866,6 +2877,78 @@ sub remove_see_also { $self->{see_also} = \@new_see_also; } +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); +} + +sub remove_tag { + my ($self, $tag) = @_; + my $dbh = Bugzilla->dbh; + my $user = Bugzilla->user; + $tag = $self->_check_tag_name($tag); + + my $tag_id = exists $user->tags->{$tag} ? $user->tags->{$tag}->{id} : undef; + # Do nothing if the user doesn't use this tag, or didn't set it for this bug. + return unless ($tag_id && grep { $_ eq $tag } @{$self->tags}); + + $dbh->do('DELETE FROM bug_tag WHERE bug_id = ? AND tag_id = ?', + undef, ($self->id, $tag_id)); + + $self->{tags} = [grep { $_ ne $tag } @{$self->tags}]; + + # Decrement the counter, and delete the tag if no bugs are using it anymore. + if (!--$user->tags->{$tag}->{bug_count}) { + $dbh->do('DELETE FROM tags WHERE name = ? AND user_id = ?', + undef, ($tag, $user->id)); + + # The list has changed. + delete $user->{tags}; + } +} + +sub tags { + my $self = shift; + my $dbh = Bugzilla->dbh; + my $user = Bugzilla->user; + + # This method doesn't support several users using the same bug object. + if (!exists $self->{tags}) { + $self->{tags} = $dbh->selectcol_arrayref( + 'SELECT name FROM bug_tag + INNER JOIN tags ON tags.id = bug_tag.tag_id + WHERE bug_id = ? AND user_id = ?', + undef, ($self->id, $user->id)); + } + return $self->{tags}; +} + ##################################################################### # Simple Accessors ##################################################################### diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index f25c4f156..5b58fd494 100644 --- a/Bugzilla/Constants.pm +++ b/Bugzilla/Constants.pm @@ -82,9 +82,6 @@ use Memoize; DEFAULT_QUERY_NAME DEFAULT_MILESTONE - QUERY_LIST - LIST_OF_BUGS - SAVE_NUM_SEARCHES COMMENT_COLS @@ -288,10 +285,6 @@ use constant DEFAULT_QUERY_NAME => '(Default query)'; # The default "defaultmilestone" created for products. use constant DEFAULT_MILESTONE => '---'; -# The possible types for saved searches. -use constant QUERY_LIST => 0; -use constant LIST_OF_BUGS => 1; - # How many of the user's most recent searches to save. use constant SAVE_NUM_SEARCHES => 10; diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm index e2c1d22e4..2e1b3f78a 100644 --- a/Bugzilla/DB/Schema.pm +++ b/Bugzilla/DB/Schema.pm @@ -953,7 +953,6 @@ use constant ABSTRACT_SCHEMA => { DELETE => 'CASCADE'}}, name => {TYPE => 'varchar(64)', NOTNULL => 1}, query => {TYPE => 'LONGTEXT', NOTNULL => 1}, - query_type => {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 0}, ], INDEXES => [ namedqueries_userid_idx => {FIELDS => [qw(userid name)], @@ -979,6 +978,36 @@ use constant ABSTRACT_SCHEMA => { ], }, + tags => { + FIELDS => [ + id => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1}, + name => {TYPE => 'varchar(64)', NOTNULL => 1}, + user_id => {TYPE => 'INT3', NOTNULL => 1, + REFERENCES => {TABLE => 'profiles', + COLUMN => 'userid', + DELETE => 'CASCADE'}}, + ], + INDEXES => [ + tags_user_id_idx => {FIELDS => [qw(user_id name)], TYPE => 'UNIQUE'}, + ], + }, + + bug_tag => { + FIELDS => [ + bug_id => {TYPE => 'INT3', NOTNULL => 1, + REFERENCES => {TABLE => 'bugs', + COLUMN => 'bug_id', + DELETE => 'CASCADE'}}, + tag_id => {TYPE => 'INT3', NOTNULL => 1, + REFERENCES => {TABLE => 'tags', + COLUMN => 'id', + DELETE => 'CASCADE'}}, + ], + INDEXES => [ + bug_tag_bug_id_idx => {FIELDS => [qw(bug_id tag_id)], TYPE => 'UNIQUE'}, + ], + }, + component_cc => { FIELDS => [ diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm index fee87fa92..7233c9dc0 100644 --- a/Bugzilla/Install/DB.pm +++ b/Bugzilla/Install/DB.pm @@ -438,10 +438,6 @@ sub update_table_definitions { # PUBLIC is a reserved word in Oracle. $dbh->bz_rename_column('series', 'public', 'is_public'); - # 2005-10-21 LpSolit@gmail.com - Bug 313020 - $dbh->bz_add_column('namedqueries', 'query_type', - {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 0}); - # 2005-11-04 LpSolit@gmail.com - Bug 305927 $dbh->bz_alter_column('groups', 'userregexp', {TYPE => 'TINYTEXT', NOTNULL => 1, DEFAULT => "''"}); @@ -549,10 +545,6 @@ sub update_table_definitions { # 2007-09-09 LpSolit@gmail.com - Bug 99215 _fix_attachment_modification_date(); - # This had the wrong definition in DB::Schema. - $dbh->bz_alter_column('namedqueries', 'query_type', - {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 0}); - $dbh->bz_drop_index('longdescs', 'longdescs_thetext_idx'); _populate_bugs_fulltext(); @@ -650,6 +642,9 @@ sub update_table_definitions { $dbh->bz_add_column('bug_see_also', 'id', {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1}); + # 2011-01-29 LpSolit@gmail.com - Bug 616185 + _migrate_user_tags(); + ################################################################ # New --TABLE-- changes should go *** A B O V E *** this point # ################################################################ @@ -3472,6 +3467,43 @@ sub _fix_series_indexes { {FIELDS => [qw(category subcategory name)], TYPE => 'UNIQUE'}); } +sub _migrate_user_tags { + my $dbh = Bugzilla->dbh; + return unless $dbh->bz_column_info('namedqueries', 'query_type'); + + my $tags = $dbh->selectall_arrayref('SELECT userid, name, query + FROM namedqueries + WHERE query_type != 0'); + + my $sth_tags = $dbh->prepare('INSERT INTO tags (user_id, name) VALUES (?, ?)'); + my $sth_bug_tag = $dbh->prepare('INSERT INTO bug_tag (bug_id, tag_id) + VALUES (?, ?)'); + my $sth_nq = $dbh->prepare('UPDATE namedqueries SET query = ? + WHERE userid = ? AND name = ?'); + + foreach my $tag (@$tags) { + my ($user_id, $name, $query) = @$tag; + # Tags are all lowercase. + my $tag_name = lc($name); + + $sth_tags->execute($user_id, $tag_name); + my $tag_id = $dbh->selectrow_array( + 'SELECT id FROM tags WHERE user_id = ? AND name = ?', + undef, ($user_id, $tag_name)); + + $query =~ s/^bug_id=//; + my @bug_ids = split(/[\s,]+/, $query); + $sth_bug_tag->execute($_, $tag_id) foreach @bug_ids; + + # Existing tags may be used in whines, or shared with + # other users. So we convert them rather than delete them. + my $encoded_name = url_quote($tag_name); + $sth_nq->execute("tag=$encoded_name", $user_id, $name); + } + + $dbh->bz_drop_column('namedqueries', 'query_type'); +} + 1; __END__ diff --git a/Bugzilla/Search/Saved.pm b/Bugzilla/Search/Saved.pm index 4b46fc75c..9828d6e02 100644 --- a/Bugzilla/Search/Saved.pm +++ b/Bugzilla/Search/Saved.pm @@ -45,17 +45,15 @@ use constant DB_COLUMNS => qw( userid name query - query_type ); use constant VALIDATORS => { name => \&_check_name, query => \&_check_query, - query_type => \&_check_query_type, link_in_footer => \&_check_link_in_footer, }; -use constant UPDATE_COLUMNS => qw(name query query_type); +use constant UPDATE_COLUMNS => qw(name query); ############### # Constructor # @@ -141,12 +139,6 @@ sub _check_query { return $cgi->query_string; } -sub _check_query_type { - my ($invocant, $type) = @_; - # Right now the only query type is LIST_OF_BUGS. - return $type ? LIST_OF_BUGS : QUERY_LIST; -} - ######################### # Database Manipulation # ######################### @@ -301,7 +293,6 @@ sub shared_with_users { # Simple Accessors # #################### -sub type { return $_[0]->{'query_type'}; } sub url { return $_[0]->{'query'}; } sub user { @@ -317,7 +308,6 @@ sub user { sub set_name { $_[0]->set('name', $_[1]); } sub set_url { $_[0]->set('query', $_[1]); } -sub set_query_type { $_[0]->set('query_type', $_[1]); } 1; diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index eafda6563..0b639ee0d 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -363,6 +363,24 @@ sub queries_available { return $self->{queries_available}; } +sub tags { + my $self = shift; + my $dbh = Bugzilla->dbh; + + if (!defined $self->{tags}) { + # We must use LEFT JOIN instead of INNER JOIN as we may be + # in the process of inserting a new tag to some bugs, + # in which case there are no bugs with this tag yet. + $self->{tags} = $dbh->selectall_hashref( + 'SELECT name, id, COUNT(bug_id) AS bug_count + FROM tags + LEFT JOIN bug_tag ON bug_tag.tag_id = tags.id + WHERE user_id = ? ' . $dbh->sql_group_by('id', 'name'), + 'name', undef, $self->id); + } + return $self->{tags}; +} + ########################## # Saved Recent Bug Lists # ########################## @@ -2074,6 +2092,11 @@ internally, such code must call this method to flush the cached result. An arrayref of group ids. The user can share their own queries with these groups. +=item C + +Returns a hashref with tag IDs as key, and a hashref with tag 'id', +'name' and 'bug_count' as value. + =back =head2 Account Lockout diff --git a/buglist.cgi b/buglist.cgi index ebce66532..3fe2ce39e 100755 --- a/buglist.cgi +++ b/buglist.cgi @@ -231,24 +231,15 @@ sub DiffDate { } sub LookupNamedQuery { - my ($name, $sharer_id, $query_type, $throw_error) = @_; - $throw_error = 1 unless defined $throw_error; + my ($name, $sharer_id) = @_; Bugzilla->login(LOGIN_REQUIRED); - my $constructor = $throw_error ? 'check' : 'new'; - my $query = Bugzilla::Search::Saved->$constructor( + my $query = Bugzilla::Search::Saved->check( { user => $sharer_id, name => $name }); - return $query if (!$query and !$throw_error); - - if (defined $query_type and $query->type != $query_type) { - ThrowUserError("missing_query", { queryname => $name, - sharer_id => $sharer_id }); - } - $query->url - || ThrowUserError("buglist_parameters_required", { queryname => $name }); + || ThrowUserError("buglist_parameters_required"); return wantarray ? ($query->url, $query->id) : $query->url; } @@ -266,15 +257,13 @@ sub LookupNamedQuery { # empty, or we will throw a UserError. # link_in_footer (optional) - 1 if the Named Query should be # displayed in the user's footer, 0 otherwise. -# query_type (optional) - 1 if the Named Query contains a list of -# bug IDs only, 0 otherwise (default). # # All parameters are validated before passing them into the database. # # Returns: A boolean true value if the query existed in the database # before, and we updated it. A boolean false value otherwise. sub InsertNamedQuery { - my ($query_name, $query, $link_in_footer, $query_type) = @_; + my ($query_name, $query, $link_in_footer) = @_; my $dbh = Bugzilla->dbh; $query_name = trim($query_name); @@ -283,13 +272,11 @@ sub InsertNamedQuery { if ($query_obj) { $query_obj->set_name($query_name); $query_obj->set_url($query); - $query_obj->set_query_type($query_type); $query_obj->update(); } else { Bugzilla::Search::Saved->create({ name => $query_name, query => $query, - query_type => $query_type, link_in_footer => $link_in_footer }); } @@ -461,10 +448,7 @@ if ($cmdtype eq "dorem") { my $query_id; ($buffer, $query_id) = LookupNamedQuery(scalar $cgi->param("namedcmd"), $user->id); - if (!$query_id) { - # The user has no query of this name. Play along. - } - else { + if ($query_id) { # Make sure the user really wants to delete his saved search. my $token = $cgi->param('token'); check_hash_token($token, [$query_id, $qname]); @@ -503,93 +487,54 @@ elsif (($cmdtype eq "doit") && defined $cgi->param('remtype')) { $user = Bugzilla->login(LOGIN_REQUIRED); my $query_name = $cgi->param('newqueryname'); my $new_query = $cgi->param('newquery'); - my $query_type = QUERY_LIST; my $token = $cgi->param('token'); check_hash_token($token, ['savedsearch']); - # If list_of_bugs is true, we are adding/removing individual bugs - # to a saved search. We get the existing list of bug IDs (if any) - # and add/remove the passed ones. + # If list_of_bugs is true, we are adding/removing tags to/from + # individual bugs. if ($cgi->param('list_of_bugs')) { - # We add or remove bugs based on the action choosen. + # We add/remove tags based on the action choosen. my $action = trim($cgi->param('action') || ''); $action =~ /^(add|remove)$/ || ThrowUserError('unknown_action', {action => $action}); - # If we are removing bugs, then we must have an existing - # saved search selected. - if ($action eq 'remove') { - $query_name && ThrowUserError('no_bugs_to_remove'); - } + my $method = "${action}_tag"; - my %bug_ids; - my $is_new_name = 0; - if ($query_name) { - my ($query, $query_id) = - LookupNamedQuery($query_name, undef, QUERY_LIST, !THROW_ERROR); - # Make sure this name is not already in use by a normal saved search. - if ($query) { - ThrowUserError('query_name_exists', {name => $query_name, - query_id => $query_id}); - } - $is_new_name = 1; - } # If no new tag name has been given, use the selected one. - $query_name ||= $cgi->param('oldqueryname'); - - # Don't throw an error if it's a new tag name: if the tag already - # exists, add/remove bugs to it, else create it. But if we are - # considering an existing tag, then it has to exist and we throw - # an error if it doesn't (hence the usage of !$is_new_name). - my ($old_query, $query_id) = - LookupNamedQuery($query_name, undef, LIST_OF_BUGS, !$is_new_name); - - if ($old_query) { - # We get the encoded query. We need to decode it. - my $old_cgi = new Bugzilla::CGI($old_query); - foreach my $bug_id (split /[\s,]+/, scalar $old_cgi->param('bug_id')) { - $bug_ids{$bug_id} = 1 if detaint_natural($bug_id); - } - } + $query_name ||= $cgi->param('oldqueryname') + or ThrowUserError('no_tag_to_edit', {action => $action}); - my $keep_bug = ($action eq 'add') ? 1 : 0; - my $changes = 0; + my @buglist; + # Validate all bug IDs before editing tags in any of them. foreach my $bug_id (split(/[\s,]+/, $cgi->param('bug_ids'))) { next unless $bug_id; - my $bug = Bugzilla::Bug->check($bug_id); - $bug_ids{$bug->id} = $keep_bug; - $changes = 1; + push(@buglist, Bugzilla::Bug->check($bug_id)); } - ThrowUserError('no_bug_ids', - {'action' => $action, - 'tag' => $query_name}) - unless $changes; - - # Only keep bug IDs we want to add/keep. Disregard deleted ones. - my @bug_ids = grep { $bug_ids{$_} == 1 } keys %bug_ids; - # If the list is now empty, we could as well delete it completely. - if (!scalar @bug_ids) { - ThrowUserError('no_bugs_in_list', {name => $query_name, - query_id => $query_id}); + + foreach my $bug (@buglist) { + $bug->$method($query_name); } - $new_query = "bug_id=" . join(',', sort {$a <=> $b} @bug_ids); - $query_type = LIST_OF_BUGS; - } - my $tofooter = 1; - my $existed_before = InsertNamedQuery($query_name, $new_query, - $tofooter, $query_type); - if ($existed_before) { - $vars->{'message'} = "buglist_updated_named_query"; + + $vars->{'message'} = 'tag_updated'; + $vars->{'action'} = $action; + $vars->{'tag'} = $query_name; + $vars->{'buglist'} = [map { $_->id } @buglist]; } else { - $vars->{'message'} = "buglist_new_named_query"; - } + my $existed_before = InsertNamedQuery($query_name, $new_query, 1); + if ($existed_before) { + $vars->{'message'} = "buglist_updated_named_query"; + } + else { + $vars->{'message'} = "buglist_new_named_query"; + } - # Make sure to invalidate any cached query data, so that the footer is - # correctly displayed - $user->flush_queries_cache(); + # Make sure to invalidate any cached query data, so that the footer is + # correctly displayed + $user->flush_queries_cache(); + + $vars->{'queryname'} = $query_name; + } - $vars->{'queryname'} = $query_name; - print $cgi->header(); $template->process("global/message.html.tmpl", $vars) || ThrowTemplateError($template->error()); diff --git a/template/en/default/global/messages.html.tmpl b/template/en/default/global/messages.html.tmpl index 89e47c1e5..5583a961e 100644 --- a/template/en/default/global/messages.html.tmpl +++ b/template/en/default/global/messages.html.tmpl @@ -865,6 +865,17 @@ The cookie that was remembering your login is now gone. [% END %] + [% ELSIF message_tag == "tag_updated" %] + [% title = "Tag Updated" %] + The '[% tag FILTER html %]' tag has been + [% IF action == "add" %] + added to + [% ELSE %] + removed from + [% END %] + [%+ buglist.size > 1 ? terms.bugs : terms.bug %] + [%+ buglist.join(", ") FILTER html %]. + [% ELSIF message_tag == "term" %] [% terms.$term FILTER html %] diff --git a/template/en/default/global/per-bug-queries.html.tmpl b/template/en/default/global/per-bug-queries.html.tmpl index a7c073ba1..90418981f 100644 --- a/template/en/default/global/per-bug-queries.html.tmpl +++ b/template/en/default/global/per-bug-queries.html.tmpl @@ -51,12 +51,6 @@ //--> - [%# Get existing lists of bugs for this user %] - [% lists_of_bugs = [] %] - [% FOREACH q = user.queries %] - [% NEXT UNLESS q.type == constants.LIST_OF_BUGS %] - [% lists_of_bugs.push(q.name) %] - [% END %]