507639 - Whining doesn't allow a user to select a saved search shared to them.

Status: NEW

(Bugzilla :: Whining, enhancement)

Description

[Eric Black]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.12) Gecko/2009070818 Ubuntu/8.10 (intrepid) Firefox/3.0.12
Build Identifier: v3.2.3

If a user is using someone else's saved search that was shared to them, they can not add it as a query in the whining event screen. 

There are two work arounds that I know of, either use the 'bz_canusewhineatothers' group and create the whine or have the users that are using the shared saved search create their own search from the original and then use that for the whining. Neither is acceptable for our organization; the former because we don't want to enable that group, the former because changes to the search are not propagated to the derived searches.



Reproducible: Always

Steps to Reproduce:
1. Have someone share one of their searches with you
2. Attempt to create a whine schedule using the new shared search
Actual Results:  
The search is not a selection in the drop down list.

Expected Results:  
The shared searches should show as an option.

Comment 1

[Eric Black]

I mentioned in the developer's mailing list that it seems the db schema would need to be adjusted to add the namedqueries user_id or id to the whine_queries table. As I looked at this more, I was thinking that the 'query_name' from the whine_queries table could be changed to just the 'id' from the namedqueries table, so I looked in the code for references to 'query_name' from whine_queries and in buglist.cgi, it is used to throw an error when a user attempts to delete a saved search that is used in whining. I'm thinking that still makes sense, but a separate secondary check should be made to see if the query is used by another user's whine, and if it is, show a separate error recommending to change the user's preferences to just not show the search in the footer. How's that sound?

Comment 2

[Max Kanat-Alexander]

Well, we should definitely be using the id instead of the query_name.

You can accomplish everything by using Bugzilla::Search::Saved objects where necessary instead of just the id and raw SQL.

If the search stops being saved, we'll have to do something about that.

Comment 3

[Eric Black]

Attachment: Patch to allow shared saved searches to be used for whiners

Here's my first stab at a patch. 

This was the pre-existing rules regarding removing a user's saved search:
1. A user can not remove one of their own searches if they are using it for
their own whining.

2. When a user shares a search, they are allowed to remove the search if
another user has it in their footer.

I didn't change either of those rules, but add the following:
3. A user can not remove one of their own searches if another user is using it
in their whining events.


Based on those rules, I implemented the following:
1. In the user's saved search preference tab, the 'Forget' link to delete a
saved search would change to 'Remove from whining first' if a user was using it
for their own whining. A check was added first to see if the search is used by
another user for whining and displays ' Used by other users for whining. Unable
to remove.'. This check is before checking a user's own whining events since it
doesn't make sense to have the user remove their own whine events only to find
out they can't remove the saved search because a user is using it in their
whine events and they have no control to remove it.

2. In the user's saved search preference tab, if the saved search was shared,
the 'Share With a Group' column would display 'Shared with XX users' where XX
was the count of users using the search in their own page footers. This has
been changed to a link for 'Show shared user list' which is a link to show (in
the same space):
   a) If there are any users using the search in their footer, it will display
'Users using search in their footer:' followed by the user's real and login
names.
   b) If there are any users using the search in their whine events, it will
display 'Users using search for whining:' followed by the user's real and login
names.

3. When a user runs one of their own saved searches and it is used in someone
else's whine event, if that user tries to delete it by clicking on the "Forget
Search 'XXX'" link at the bottom of the bug list, the following error will
appear:
   "The saved search XXXX is being used by other users for Whining events and
can't be removed. Users:

    user's real name   <user's login>

The easiest thing to do is remove it from showing in your footers by unsetting
it in your Saved Searches preferences tab. "

Comment 4

[Eric Black]

I sent the following to [e-mail]:

I tried to assign the bug to myself, but it wouldn't let me or let me chose a reviewer. Let me know what I need to do. The patch I submitted was diffed from the head.

Comment 5

[Frédéric Buclin]

You must request review for your patch. Reviewers will ask approval themselves when the patch gets review+.

Comment 6

[Eric Black]

Comment on attachment 392843 [details] [diff] [review]
Patch to allow shared saved searches to be used for whiners

Sorry, I thought the review request was on the front page of the bug.

Comment 7

[Eric Black]

Attachment: This is an additional patch needed for Bugzilla/Install/DB.pm

I just realized that I forgot to add the patch for Bugzilla/Install/DB.pm. This patch was made from diff -Nru against the head version 1.67 downloaded on August 5th, 2009.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 393211 [details] [diff] [review]
This is an additional patch needed for Bugzilla/Install/DB.pm

You should not be retaining the query_name column at all.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 392843 [details] [diff] [review]
Patch to allow shared saved searches to be used for whiners

>+        ($buffer, $qid) = LookupNamedQuery(scalar $cgi->param("namedcmd"));
>+        my $saved_search = new Bugzilla::Search::Saved($qid);
>+

  No, you should be using Bugzilla::Search::Saved *only*, and not using LookupNamedQuery at all. You should do that refactoring in another bug first (we have one for it already) and then fix this bug.

>+        my $whine_users = $saved_search->whine_user_list;

  We could just call that whine_users instead of whine_user_list.

>+        my $userlist = '';

  Looks like that variable isn't being used.

>Index: editwhines.cgi

  Man. We really need an object for whines. If you wanted to create some (in another bug), you'd be my hero. :-)

>@@ -326,14 +327,23 @@
>                         trick_taint($queryname);
>                         trick_taint($title);
> 
>+                        # The query names come in as the query name followed by the 
>+                        # query id, both from the 'namedqueries' table.
>+                        my $named_qid = 0;
>+                        if( $queryname =~ /^(.+)_(\d+)/ ) {
>+                            $queryname = $1;
>+                            $named_qid = $2;

  Wait, what is that? That looks like a hack, to me. You shouldn't be doing random concatenation, you should just have two entirely separate variables.

>@@ -420,12 +431,20 @@
>+my $saved_searches = Bugzilla::Search::Saved->match( { userid => $userid } );

  Wooo hooo! :-) That's definitely better than what was there before.

>+my $bz_user = new Bugzilla::User($userid);

  Maybe that should be $search_user or $whine_user?

>+foreach my $available_queries ( @{$bz_user->queries_available} ) {
>+    my $search_creator = new Bugzilla::User( $available_queries->{'userid'} ); 

  Hmmm. Maybe you should just be passing the object in $available_queries, instead of a hashref, because you can actually get a User object back out of the Search::Saved object. (This could be done as part of the separate refactoring bug.)

>Index: whine.pl
> # Contributor(s): Erik Stambaugh <[e-mail]>
>+#                 Eric Black <[e-mail]>

  It's an Eri(c|k) party! :-)

>Index: Bugzilla/DB/Schema.pm
>@@ -1309,6 +1310,8 @@
>                               REFERENCES => {TABLE => 'whine_events',
>                                              COLUMN => 'id',
>                                              DELETE => 'CASCADE'}},
>+            query_id      => {TYPE => 'INT3', NOTNULL => 1,
>+                              DEFAULT => '0'},

  FWIW, you don't need to enclose 0 in quotes.

  Also, this should have an FK into the namedqueries table.

>Index: Bugzilla/Search/Saved.pm
>+sub shared_user_list {

  There's already shared_with_users. I think we should just re-work that to return a user list, and change the few places that currently uses this to just scalar() the returned array. (Or call .size on it, in the templates.)

>+sub whine_user_list {
>+    my $ids = Bugzilla->dbh->selectcol_arrayref('SELECT owner_userid 
>+                                                   FROM whine_events 
>+                                             INNER JOIN whine_queries 
>+                                                     ON whine_events.id = whine_queries.eventid 
>+                                                  WHERE query_id = ? and owner_userid != ?',
>+                                                undef, $self->id, $self->{userid} );

  Hmm, should that perhaps be SELECT DISTINCT? That is, couldn't there be results for the same user, there?

>Index: template/en/default/account/prefs/prefs.html.tmpl
>+   javascript_urls = ['js/util.js', 'js/field.js', 'js/yui/yahoo-dom-event.js']

  You don't need to add yui-dom-event, it's on every page, in the global header.

>Index: template/en/default/account/prefs/saved-searches.html.tmpl
>+          [% IF q.whine_user_list.0 %]
>+            Used by other users for whining.<br />Unable to remove.

  Since we're not XHTML, we should just be using <br>, not <br />.

>@@ -139,9 +142,35 @@
>+            [% IF q.shared_user_list.0 || q.whine_user_list.0  %] 

  You should be calling .size on those instead of .0.

>+              <div id="bz_shared_user_container_[% q.id FILTER html %]" class="bz_default_hidden">

  Nit: That is a really long id. Also, I don't think we currently start any of our ids with bz_, so I don't think you need to start with these. (We do for some of our classes, but not our ids, as far as I know.)

>+                <br /><a href="#" id="bz_show_shared_users_[% q.id FILTER html %]">Show shared user list</a>

  Why is there just an href and no onclick?

>+              <div id="bz_shared_user_list_[% q.id FILTER html %]">
>+              [% IF q.shared_user_list.0 %]
>+                 <br />Users using search in their footer:       
>+                 [% FOREACH c = q.shared_user_list %]
>+                   <br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;[% c.name FILTER html %]&nbsp;&nbsp;&lt;[% c.login FILTER html %]&gt;

  Multiple &nbsp; is not the right way to do indentation--we should be using CSS.


>Index: template/en/default/global/user-error.html.tmpl
>+    [% FOREACH user = subjects %]
>+    <br />&nbsp;&nbsp;&nbsp;&nbsp;[%- user.name FILTER html -%]
>+    &nbsp;&nbsp;&lt;[%- user.login FILTER html -%]&gt;

  Same note here about &nbsp;.

  Also, if you want to make a list of something, you should be using <ul>. That's what it's for. :-)

  There's also generally a lot of <br> in this patch, which I think should generally be used sparingly--probably better to use <p></p> where it makes sense, and other block elements (<div>, <ul>, <ol>, etc.) otherwise.

>Index: template/en/default/whine/schedule.html.tmpl
>-                <input type="hidden" value="[% query.name FILTER html %]"
>+                <input type="hidden" value="[% query.name _ '_' _ query.query_id FILTER html %]"
>                        name="orig_query_name_[% query.id %]">

  Yeah, as I mentioned above, you should not be passing in the name at all, just the id here.

  Also, editwhines should be re-worked to use objects and not need this orig_ stuff, but of course that would be another bug.

>+        <option [% "selected" IF q.name == thisquery %] value="[% q.name _ '_' _ q.id FILTER html %]">

  Nit: <option [% ' selected="selected"' IF q.name == thisquery %]