Arvados

Radhika Chippada wrote:

1. line #7 and 8: It would be better if you can add the fourth parameter, the error message, to aid in case of failures

Added.

-- doc/api/methods/groups.html.textile.liquid

1. "although they are owned by different users/groups" -- Instead of "they are", should we say "they may be"? I am assuming this group itself might own some folders.

Indeed. Updated wording here ("even if they are owned") and added an "also" earlier in the sentence to make it more clear this is a superset.

2. Same comment about users also

Updated that too.

-- doc/api/schema/Link.html.textile.liquid

1. "This works for all object types, not just jobs" -- Do you mean "this applies for all object . . . "?

Corrected.

-- services/api/app/controllers/ApplicationController

1. Do we want to call "_owned_items_REQUIRES_parameters" as "_owned_items_REQUIRED_parameters"?

I think of it as "the method requires some parameters". "params_expected_by_#{method}" might be better ("required" is a misnomer: optional params are listed there too). I would prefer to do this with something conventional that comes with Rails, but I haven't found such a thing yet.

2. "include_indirect" is a bit confusing. Do we want to call it "include_linked"?

Changed to include_linked.

3. Question about following comment? What do you mean by this? I see that find_object_by_uuid has "@where = { uuid: params[:uuid] }".

" # We stuffed params[:uuid] into @where in find_object_by_uuid,

1. but we don't want it there any more. "

Wrote a longer comment, then realized it wasn't even needed because @where has already been overwritten using params[:where] between find_object_by_uuid and owned_items. Removed.

-- load_limit_offset_order_params method

1. If invalid limit value is provided: instead of raising error, do we want to use DEFAULT_LIMIT? Same question about offset.

IMO better to tell the client "you can't do that" than to silently ignore: easier to debug clients. I've taken a step in the opposite direction: get more picky ("123foo" is no longer an acceptable limit) and added some tests to ensure 422 for bogus limit/offset.

services/api/test/functional/arvados/v1/groups_controller_test.rb

1. Does it make sense to add a test to get objects for a group_class that has no such data? For example, "get list of garbage_group_class"?

Indeed. Added. Also for group_class=nil.

services/api/test/test_helper.rb

1. Is "jresponse" a standard name? Why not "json_response"?

Changed to json_response.

-- I may have missed it, but if not: can you please add an explanation in documentation about group_class? What is it and purpose. Also, do we support only a certain set of values for group_class?

One line about it at /doc/api/schema/Group.html. (Added a bit more.)

So far, like link_class, we let clients put whatever we want here but certain behavior is triggered by well known values like link_class=permission, group_class=folder.