New WebService functions discussion

Fri Apr 4 00:50:32 UTC 2008

On Fri, 04 Apr 2008 10:40:21 +1000 Noura Elhawary <nelhawar at redhat.com>
wrote:
> Note: We might need to change the param name 'full_name' in
> User.create to 'real_name' to match the other API functions User.get
> Bug#412725 and User.update Bug#416137

	Yes, I agree, but we should keep full_name as an alias for
backwards-compatibility.

>    - Security Issue: only users in 'editusers' group gets the
> following information about other users: disabled_text,
> email_enabled, and and can_login.

	Yes, that's fine.

> 3- User.update(), Bug#416137:
>   - User needs to be in 'editusers' group to be able to update other 
>     user's information ,, this is the only privilege needed.

	Correct.

>   - The changes hash returned from calling $user.update will be 
>     returned to the user, but the names will be overridden to match
>     with the agreed user field names for API consistency.

	Yes.

>   - If User.update() will eventually be allowed to update permissions
> as well, then we will need to allow persons with bless privileges for 
>     particular groups to be allowed to add/remove users from those 
>     groups.

	Yes, but don't worry about that right now.

>   - shall we call the hash key update, updates just to consistent with
>     ids and names in the passed parameters hash.

	What?

> Bugzilla/WebService/Bug.pm
>   - It is more user friendly and more useful to return names instead
> of ids , so all returned user ids for bug reporter,assigned_to , 
>     qa_contact, product and component should be converted to names, 
>     or we can return both names and ids. Or reporter, assigned_to,
> etc could be a hashes that contains the user info such as id, email, 
>     real_name, etc. component would be hash containing id, name, etc.

	Yes, it would be better to return login names as opposed to
IDs. No hashes necessary. 

	The "internals" hash item does not need to be adjusted.

>       - passing bug ids and all flag , 
>         $rpc->call('Bug.get', {ids => [1,2], all => 1})
>         returns all bug information that we can get from bug object
>         and by calling the accessor functions to get: cc, dependson, 
>         blocked, longdescs, keywords, groups, attachments, 
>         flag_types, dup_id, actual_time and milestoneurl

	I don't want to implement that right now.

>       - specifying the fields we want in return:
>        $rpc->call('Bug.get',{ids =>[1,2],fields =>[priority,
> keywords]}) and this will return only the fields requested.

	Yes, but let's call it include_fields. (As we might want to add
exclude_fields later at some point.)

> 2- Bug.get_activity(), Bug#424079:
>    - The function gets list of bug ids/aliases and returns hash
>      with keys: id, alias, incomplete_data and operation , maybe 
>      can use better names as data_iscomplete and history

	Don't return incomplete_data, that was a hack to fix an old bug
in Bugzilla and shouldn't be perpetuated in the WebService.

	You should return a hash that looks something like this:

	1234 => {
	    [
	        { time => '2008-04-02 04:00:00', field => 'priority',
                  from => 'P1', to => 'P2' },
	    ],
	},

	However, I'm not entirely certain about that format yet. We
might make some adjustments to the way the bugs_activity table is
structured in 4.0, so I'd say to put this off for now.

>    - should this function be separate or should we include it
>      in Bug.get() .. so would be something like this:

	It should be separate.

> 3- Bug.update(), Bug#418342:
>    - depends on the function set_all,, is it ready now to write it
> and change process_bug.cgi to use it?

	Yes, I suppose so. Please write it in pieces, though. That is,
move individual bits of process_bug.cgi into set_all in individual
bugs. Don't move it all at once--that will be too big to review.

>      2 issues were blocking set_all: 1- ability to make mass bug 
>      updates for everything except bug alias , and moving sending
> email code from process_bug.cgi ? so is it right time to start writing
>      set_all now?

	Yeah, you don't have to wait for those other things to at least
start moving *some* things into set_all.

> 1- API consistency between Component.pm functions, the following names
>    should be used for component fields in passed parameters or
> returned values: 
>    Component fields: id, name, product_name, product_id, default_cc
>                      default_assigned_to, default_qa_contact

	Use "product" instead of "product_name", since we'll just be
using "product" everywhere for that, I think.

>    using default_* will cause writing some extra code for 
>    Component.update to change the names returned in the changes
>    hash from $component.update to match with the above names
>    for API consistency

	Yes, that's fine.

>  is there any plans that default_*
>    names will be the ones used all over the code , replacing
>    initailowner, initialqacontact, etc ?

	It's possible, but I have no plans to implement that right now.
So just fix it in the WebService. We're much more concerned about API
consistency in the WebService than internally in Bugzilla.

> 2- Security Issue: Non privileged users shouldn't be able to know 
>    about the existence of confidential products when they try to
>    update or get components they can not access. Solution: is to 
>    get $user->get_accessible_products() for the user and check that 
>    the product names they passed is in their accessible list of 
>    products. Please take a look at the patch in Bug#385282 for 
>    that solution.

	Yes, that sounds right, though I haven't looked at the patch.

> 3- Component.update(), Bug#419456:
>    - code is getting a bit complicated in this function for 2 reasons:
>      a- change the names returned in the changes hash to match with
>         the selected component field names for API consistency.

	Perhaps you should write a general function to do that, since
we're going to have to do it in a few places.

>      b- convert user ids returned in the changes hash to login names.

	Yes, that just has to be done. I do the same thing inside of
Bug.update for fixing the bugs_activity table.

> 4- Component.get_by_product, Bug#424921:
>    - shall this function be separate or should it be in Component.get?

	It should be in Component.get.

>      I vote for making it separate because the returned value has 
>      different format to Component.get, this function should return 
>      component information grouped by passed product names, where as 
>      Component.get will return single component information for each
>      passed component?

	You should unify those return types into a single type.

> One last general note: shall we allow the values passed into the
> WebService functions to be single as well as lists for example for 
> User.get we can have
> 
> $rpc->call('User.get', { ids => [1,2]}) 
> or
> $rpc->call('User.get, {ids => 1})
> 
> do in the function code we will expect both by :
> my @ids = ref($params->{ids}) ? @{$params->{ids}} : ($params->{ids});

	I think that would be nice for users of the WebService, but I
think it's simpler to be completely consistent and always require an
array.

	-Max
-- 
http://www.everythingsolved.com/
Competent, Friendly Bugzilla and Perl Services. Everything Else, too.