697224 - User.get should return a list of all your saved searches

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[James Long (:jlongster)]

It'd be really nice if we had access to a user's saved searches. This way we could allow users to create searches in bugzilla but have a client which can use them.

Two additional methods would be needed. One to get a list of saved searches, and another one to execute a saved search.

This would be exposed via the WebService using XMLRPC or JSONRPC.

Comment 1

[Max Kanat-Alexander]

Since bugs have to focus on only adding one thing at a time, I'm changing the summary to focus on getting information about saved searches. If you'd like to also execute them, filing a separate bug for that would be good.

Comment 2

[Koosha KM]

Attachment: patch - v1

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 639488 [details] [diff] [review]
patch - v1

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

Instead of returning just the list of names, let's instead return a list of hashes with the data for each saved search. So it would look like this:

saved_searches => [
  { 
     id   => 1,
     name => 'foo',
     url  => 'product=TestProduct1&component=component1...',
  },
  {
     id   => 2,
     name => 'bar',
     url  => 'product=TestProduct2&component=component2...',
  },
]

Create a utility function such as _saved_search_to_hash() to do the hash conversion properly. See Bugzilla::WebService::Bug::_flag_to_hash() and similar for examples.
Make sure to wrap each value with the proper $self->type() calls as well.

dkl

Comment 4

[Frédéric Buclin]

@dkl: what's this id key? What does it represent?

Comment 5

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #4)
> @dkl: what's this id key? What does it represent?

It is the 'id' of the saved search as it is in the database. It should be returned as a type 'int'. Although it is not necessary now it may be useful in the future and it is consistent to return the 'id' in the data structure. See Bugzilla::WebService::Bug::_flag_to_hash, Bugzilla::WebService::Product::_component_to_hash and others like them.

dkl

Comment 6

[Koosha KM]

Attachment: patch - v1.1

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 639719 [details] [diff] [review]
patch - v1.1

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

*** ERROR: =item without previous =over at line 838 in file Bugzilla/WebService/User.pm
not ok 119 - Bugzilla/WebService/User.pm has incorrect POD syntax --ERROR
#   Failed test 'Bugzilla/WebService/User.pm has incorrect POD syntax --ERROR'
#   at t/011pod.t line 47.

Otherwise works fine functionally so we are almost done.

dkl

::: Bugzilla/WebService/User.pm
@@ +202,4 @@
>      }
>     
>      my $in_group = $self->_filter_users_by_group(
> +        \@user_objects, $params);

whitespace change?

@@ +214,4 @@
>                  groups    => $self->_filter_bless_groups($_->groups), 
>                  email_enabled     => $self->type('boolean', $_->email_enabled),
>                  login_denied_text => $self->type('string', $_->disabledtext),
> +                saved_searches    => [map {$self->_query_to_hash($_)} @{ $_->queries }],

Nit: add space after map { and before }

map { $self->_query_to_hash($_) }

@@ +802,5 @@
>  =back
>  
> +=item saved_searches
> +
> +C<array> An array of hashes each of which represents a user's saved search and has

An array of hashes, each of which...

@@ +809,5 @@
> +=over
> +
> +=item id
> +
> +C<int> The id of the saved search as stored in database.

(Derived from the Product.components POD and may be better here)

C<int> An integer id uniquely identifying the saved search in this installation only.

@@ +821,5 @@
> +C<string> The CGI parameters for the saved search.
> +
> +=back
> +
> +B<Note>: The elements of the returning array (i.e. hashes) are ordered by the 

s/returning/returned/

@@ +822,5 @@
> +
> +=back
> +
> +B<Note>: The elements of the returning array (i.e. hashes) are ordered by the 
> +name of saved searches.

name of each saved search.

Comment 8

[Koosha KM]

(In reply to David Lawrence [:dkl] from comment #7)
> Comment on attachment 639719 [details] [diff] [review]
> patch - v1.1
> 
> Review of attachment 639719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> *** ERROR: =item without previous =over at line 838 in file
> Bugzilla/WebService/User.pm
> not ok 119 - Bugzilla/WebService/User.pm has incorrect POD syntax --ERROR
> #   Failed test 'Bugzilla/WebService/User.pm has incorrect POD syntax
> --ERROR'
> #   at t/011pod.t line 47.
> 
> Otherwise works fine functionally so we are almost done.
> 

Works fine on my system. I don't think this problem is coming from my patch. Look for you local changes.

Comment 9

[Koosha KM]

Attachment: patch - v1.2

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 639989 [details] [diff] [review]
patch - v1.2

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

Looks good and works as expected. r=dkl

Comment 11

[Koosha KM]

(In reply to David Lawrence [:dkl] from comment #7)
> Comment on attachment 639719 [details] [diff] [review] [diff] [review]
> patch - v1.1
> 
> Review of attachment 639719 [details] [diff] [review] [diff] [review]:
> -----------------------------------------------------------------
> 
> *** ERROR: =item without previous =over at line 838 in file
> Bugzilla/WebService/User.pm
> not ok 119 - Bugzilla/WebService/User.pm has incorrect POD syntax --ERROR
> #   Failed test 'Bugzilla/WebService/User.pm has incorrect POD syntax
> --ERROR'
> #   at t/011pod.t line 47.
> 
> Otherwise works fine functionally so we are almost done.
> 

I'm sorry. You were right. I didn't check the generated HTML. I thought errors show up while generating the doc not in the resultant doc (I knew that before but I forgot it!!!).

Anyway, a '=over' before line 804 solves the issue.

Comment 12

[David Lawrence [:dkl]]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #11)
> I'm sorry. You were right. I didn't check the generated HTML. I thought
> errors show up while generating the doc not in the resultant doc (I knew
> that before but I forgot it!!!).
> 
> Anyway, a '=over' before line 804 solves the issue.

Cool. We can fix it on commit. Remind me or whoever does the commit to fix it then.

dkl

Comment 13

[David Lawrence [:dkl]]

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

Comment 14

[Frédéric Buclin]

Oops, I'm withdrawing my approval. It's not acceptable to disclose saved searches to all users, even to admins. They should only be accessible if you are querying your own account.

I backed out the patch:

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

Comment 15

[Frédéric Buclin]

Comment on attachment 639989 [details] [diff] [review]
patch - v1.2

r- per my previous comment. From the UI, admins have no way to get this information, unless they impersonate the user account, in which case the user is informed by email that someone is looking at his account. There is no reason to let everyone silently get this information using User.get. This information should be returned only if you query your own account.

Comment 16

[Koosha KM]

Attachment: patch - v1.3

This patch also removes the existing redundant code.

Comment 17

[Frédéric Buclin]

Comment on attachment 651362 [details] [diff] [review]
patch - v1.3

>+    # Make the @users array bigger in advance to gain some performance.
>+    $#users += $#$in_group;

This is a bad idea:

>perl -MTime::HiRes -wE 'my @a; my $t1 = Time::HiRes::time(); push(@a, $_) for (1..1e7); my $t2 = Time::HiRes::time(); say $t2 - $t1; say scalar(@a)'
1.28934717178345
10000000

>perl -MTime::HiRes -wE 'my @a; $#a = 1e7; my $t1 = Time::HiRes::time(); push(@a, $_) for (1..1e7); my $t2 = Time::HiRes::time(); say $t2 - $t1; say scalar(@a)'
1.55815100669861
20000001

As you can see, the code is much slower and @users now has the wrong size. Drop this hack from your patch.


>+    foreach my $user (@$in_group) {
>+        push(@users, filter($params, $user_info));
>+}

The closing } of the foreach loop has the wrong indentation.


Otherwise your patch looks good, but I need to test it first before setting r+/-.

Comment 18

[Koosha KM]

(In reply to Frédéric Buclin from comment #17)
> Comment on attachment 651362 [details] [diff] [review]
> patch - v1.3
> 
> >+    # Make the @users array bigger in advance to gain some performance.
> >+    $#users += $#$in_group;
> 
> This is a bad idea:
> 

I don't know, but this is what the Camel book recommends:

"You can gain some measure of efficiency by pre-extending an array that is going to get big."

Please correct this on checkin: "In other word" => "In other words".

Comment 19

[Koosha KM]

I got it. If we're going to pre-extend the array, we must use the index-based assignment and not use push().

$users[$i] = ...

Comment 20

[Koosha KM]

But, push() is more readable.

Comment 21

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #18)
> "You can gain some measure of efficiency by pre-extending an array that is
> going to get big."

An array with less than 50 items is not big. We are not talking about 10000000 items here. We shouldn't start with such pseudo perf-improvements, else our code is quickly going to become messy.

If you are really interested in improving performances, use Devel::NYTProf. ;)

Comment 22

[Frédéric Buclin]

Comment on attachment 651362 [details] [diff] [review]
patch - v1.3

>+B<Note>: The elements of the returned array (i.e. hashes) are ordered by the 
>+name of each saved search.

This information is really not important and should be removed.


>+B<Note>: Only the saved searches of the logged in user will be returned. In other 
>+word, you won't be authorized to access other users' saved searches even if you are
>+the administrator.

This should be merged with the already existing Note.


I like the code refactoring. Avoiding duplicated code is nice and makes it clearer what is returned or not. :) Your patch is working fine, thanks! r=LpSolit

Comment 23

[Frédéric Buclin]

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

Comment 24

[Koosha KM]

(In reply to Frédéric Buclin from comment #17)
> Comment on attachment 651362 [details] [diff] [review] [diff] [review]
> patch - v1.3
> 
> >+    # Make the @users array bigger in advance to gain some performance.
> >+    $#users += $#$in_group;
> 
> This is a bad idea:

You forgot to remove these lines on chekin!

Comment 25

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #24)
> You forgot to remove these lines on chekin!

Ah right, thanks. This is now done.

Comment 26

[Frédéric Buclin]

About QA tests:

As we cannot create saved searches using the WebServices, I can only check that they aren't leaked to other users. But I cannot check that the correct data is returned (or I would have to mix a Selenium and a WS test script).

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.4/
modified t/webservice_user_get.t
Committed revision 233.

Comment 27

[Frédéric Buclin]

Added to the relnotes for 4.4.

Comment 28

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #1)
> Since bugs have to focus on only adding one thing at a time, I'm changing
> the summary to focus on getting information about saved searches. If you'd
> like to also execute them, filing a separate bug for that would be good.

Filed as bug 903436.