Arvados

From reading the subject line and the existing a-r-p-i code combined, I'm guessing what we want is that, before it creates any jobs, a-r-p-i tries to get every collection listed as an input parameter of the pipeline, and bails if any of them return 403 or 404. Is that about right?

Detecting it early in a-r-p-i sounds worthwhile too, but I think it's more urgent for Workbench to detect this condition, and disable the Run button until it's corrected.

• Someone shared a pipeline template with you, but forgot to share all of the inputs needed to run it (e.g., those inputs were in a different project which isn't shared).
• You're running a pipeline template that's public but depends on nonfree/nondistributable software.

In the latter case the "description" field for the script_parameter is expected to say something like: "This should be version 1.2.3 of the 'example' tool, which you can download from example.com/download/?arch=amd64."

Discussing this with Bryan, we thought we might address the "user might not notice download instructions" vs. "error message has to be generic" tension by putting the description itself (not just the "input is 404" message) in a visual "error" state. (The Bootstrap validation states seem to take this approach already.)

(IRC) bcosc
11:24 qr1hi-p5p6p-8584ylktoumirn0 is a good template for testing, its the one we specifically want this issue to address. We do not give collection 2e98fdc8e90f4c48a0714b711767c9ce+76 access to the public, we want them to upload their own. I usually set this collection as a default parameter so people do not have to set it in the future. This collection is needed around job 5 in the pipeline. Currently, the pipeline will run until job 5 and then an error will ...

implementation notes:

• Extended the existing pipeline_instances.js
  • Previously, this was showing a red (pink actually) background for links that were required and empty
  • Added a check to also check if the attr value is not readable for this when required
  • Added a similar block to check unreadable required "input" elements. Added red background for input elements only when they are not readable but not when empty, to ensure the previous user experience continues to be supported
  • Since I extended the existing behavior, I did not have to use additional "bootstrap validation states"
  • Do we want tooltips added as well? Especially, in the case of inputs that are not readable and not editable?

• Added a text warning at the top of pipeline instance's Inputs and Components tabs to the effect that "one or more inputs are not readable …"

• Made several updates to the work made for #5365 to also be able to preload collection inputs of "command" component parameters

• It seems like the update I made to line #38 of pipeline_instances.js is unnecessary. I do not think there will be an editable collection link that is not readable. Either it will be an input element with chooser or un-clickable read-only collection uuid/ pdh (application_helper -> render_pipeline_component_attribute method). I think I should revert it.
  

   if ($tag.hasClass("editable-empty") || $tag.hasClass("unreadable-input")) { 

Reviewing 0b1d679

In pipeline_instances.js, you more or less copied the JavaScript code to highlight missing required inputs, and applied it to unreadable inputs. I believe you can reduce code duplication here by just expanding the first jQuery selector to select all the elements we want to check with $('a.editable.required, input.required'), and looping over those results. This would needlessly check input elements for the editable-empty class, but I think that's harmless.

In pipeline_instances_helper.rb, could we use a Bootstrap alert in render_unreadable_inputs_present, rather than writing up our own CSS from scratch? I think that would provide more consistency with the rest of the Workbench UI, and it's a little more visually distinct than setting the text color alone.

• In link_to_arvados_object_if_readable, in the case where a required attribute is unreadable, is it necessary to include link_text_if_not_readable inside the raw() call? Other cases don't do this, so it seems like whether or not the string is interpreted HTML-safe will depend on the readability of the object, which could surprise a caller. Would it work instead to say something like raw("opening tags string") + link_text_if_not_readable + raw("closing tags string")?
• Why does the object_readable method take a resource_class argument if it always resets it on the second line (resource_class = resource_class_for_uuid(attrvalue))?
• The return value of object_readable can vary a lot. If a resource class can't be found, it's nil; otherwise, if the resource class is a collection, it's a boolean; otherwise, if the object can be found, it's that object; otherwise, it's nil. I think it would be easier to reuse this method in the future if the return type were more consistent. I think it would be more useful for the future if the return value was "the object if it's readable, else nil," but I don't feel strongly about that if you prefer booleans.
• It might be useful to add a comment to object_readable explaining that it works from preloaded objects when available. That's an important distinction between this method and ArvadosModel.find?.

Thanks.

Brett said:

In pipeline_instances.js, you more or less copied the JavaScript code to highlight missing required inputs, and applied it to unreadable inputs. I believe you can reduce code duplication here by just expanding the first jQuery selector to select all the elements we want to check with $('a.editable.required, input.required'), and looping over those results. This would needlessly check input elements for the editable-empty class, but I think that's harmless.

In the case of a.editable.required, $tag.parent() is being worked on; and in the case of input.required, $tag.parent().parent() is being worked on. Rather than doing an if block for this, I am leaving it as is (since we need an if statement anyways). This way, we can continue to preserve the preexisting behavior when inputs that are not yet selected or not shows with red background.

In pipeline_instances_helper.rb, could we use a Bootstrap alert in render_unreadable_inputs_present, rather than writing up our own CSS from scratch? I think that would provide more consistency with the rest of the Workbench UI, and it's a little more visually distinct than setting the text color alone.

Done. Yes, it looks much better and consistent with other areas now.

In application_helper.rb:

In link_to_arvados_object_if_readable, in the case where a required attribute is unreadable, is it necessary to include link_text_if_not_readable inside the raw() call? Other cases don't do this, so it seems like whether or not the string is interpreted HTML-safe will depend on the readability of the object, which could surprise a caller. Would it work instead to say something like raw("opening tags string") + link_text_if_not_readable + raw("closing tags string")?

Done

Why does the object_readable method take a resource_class argument if it always resets it on the second line (resource_class = resource_class_for_uuid(attrvalue))?

Corrected this.

The return value of object_readable can vary a lot. If a resource class can't be found, it's nil; otherwise, if the resource class is a collection, it's a boolean; otherwise, if the object can be found, it's that object; otherwise, it's nil. I think it would be easier to reuse this method in the future if the return type were more consistent. I think it would be more useful for the future if the return value was "the object if it's readable, else nil," but I don't feel strongly about that if you prefer booleans.

Updated this to always return the object or nil

It might be useful to add a comment to object_readable explaining that it works from preloaded objects when available. That's an important distinction between this method and ArvadosModel.find?.

Done

• While testing, I noticed that the functionality is not extended to the case where "dataclass=File". I added this and added a test

• One anonymous_access_test is failing and I will update this and make sure all tests are passing before merging to master (I wanted to make sure I turn this around to you before you end your work day today).

Thanks.

On dc96093e, just one small last thing:

Radhika Chippada wrote:

• While testing, I noticed that the functionality is not extended to the case where "dataclass=File". I added this and added a test

Would it make the new test fixture more realistic if the value actually referred to a file inside the collection? (i.e., zzzzz-4zz18-bv31uwvy3neko21/bar)

If that change works for you and the tests still pass, I'm happy to see this merged. Thanks.