Arvados

Review at 76dea877

I don't understand generally what's going on with the ajax:complete vs ajaxComplete and ajax:send ajaxSend changes. Can you explain a little more?

My JavaScript is not very strong so I don't have anything substantive, but I'm going to ask for some stupid little changes that make the code easier for me to follow because jQuery makes my head hurt:

• apps/workbench/app/assets/javascripts/select_modal.js
  • $(this) -> $this in the $.ajax call
  • Does the jqxhr .done() callback imply that the request succeeded? The docs sort of suggest that but I'm not sure. (If the call didn't succeed, then I think we don't want to populate modal-dialog-preview-pane with whatever's in the "data" argument.)
• apps/workbench/app/controllers/application_controller.rb
  • @objects = Group.find(params[:project_uuid]).contents({:filters => [['uuid', 'is_a', "arvados\##{ArvadosApiClient.class_kind(model_class)}"]]})
    • I can't make head or tails of this. Is this code basically filtering the uuids listed in the project_uuid param for uuids which are members of the current controller class? I'd expect every uuid in that list to be arvados#group anyway, so I'm pretty lost here.
    • Not your code, but -- can we move ArvadosApiClient.class_kind into a helper library function of some kind? It's confusing to call a class method in a totally unrelated class just because that's where it happens to be defined. Might as well move generic code to an all-purpose location.
• apps/workbench/app/controllers/collections_controller.rb
  • Leftover debugging puts statement?
• apps/workbench/app/views/collections/_choose_rows.html.erb
  • Leftover debugging puts statements?

Tim Pierce wrote:

Review at 76dea877

I don't understand generally what's going on with the ajax:complete vs ajaxComplete and ajax:send ajaxSend changes. Can you explain a little more?

Added comment in the source to explain what is going on.

My JavaScript is not very strong so I don't have anything substantive, but I'm going to ask for some stupid little changes that make the code easier for me to follow because jQuery makes my head hurt:

• apps/workbench/app/assets/javascripts/select_modal.js
  • $(this) -> $this in the $.ajax call

Fixed

• Does the jqxhr .done() callback imply that the request succeeded? The docs sort of suggest that but I'm not sure. (If the call didn't succeed, then I think we don't want to populate modal-dialog-preview-pane with whatever's in the "data" argument.)

done() is only called on success. Added fail() callback which states the preview load failed.

• apps/workbench/app/controllers/application_controller.rb
  • @objects = Group.find(params[:project_uuid]).contents({:filters => [['uuid', 'is_a', "arvados\##{ArvadosApiClient.class_kind(model_class)}"]]})
    • I can't make head or tails of this. Is this code basically filtering the uuids listed in the project_uuid param for uuids which are members of the current controller class? I'd expect every uuid in that list to be arvados#group anyway, so I'm pretty lost here.

Added comment to source.

• Not your code, but -- can we move ArvadosApiClient.class_kind into a helper library function of some kind? It's confusing to call a class method in a totally unrelated class just because that's where it happens to be defined. Might as well move generic code to an all-purpose location.

I agree this is stupid and annoying, but I don't have a better idea of where to put it at the moment.

• apps/workbench/app/controllers/collections_controller.rb
  • Leftover debugging puts statement?
• apps/workbench/app/views/collections/_choose_rows.html.erb
  • Leftover debugging puts statements?

Fixed.

LGTM, thanks.