Arvados

It seems to come down to a very simple line of code that decides whether to show an input parameter or not. In pipeline_instances/_show_inputs.html.erb:

          <% if (pvalue_spec[:description] or
                 ((pvalue_spec[:required] or pvalue_spec[:optional] == false) and
                  not pvalue_spec[:value])) %>

So there is a procedural way to deal with this, which is to add a description. If the parameter has a description it will always be displayed in the inputs tab.

Or, in the case of required parameters, the and not pvalue_spec[:value] clause could be removed entirely... But I'm not sure what the original intent was behind it.

This latter approach would seem to make the most sense to me. Any reason why not?

Phil Hodgson wrote:

Or, in the case of required parameters, the and not pvalue_spec[:value] clause could be removed entirely... But I'm not sure what the original intent was behind it.

This latter approach would seem to make the most sense to me. Any reason why not?

Yes, I think just removing and not pvalue_spec[:value] should do it.

This was implemented with the (apparently incorrect) assumption that a pipeline author would not specify a parameter that is mandatory but has no default and no description.

(This leaves us without a way to distinguish "basic" from "advanced" inputs -- the "inputs" tab will tend to hide a single user-provided input in a sea of default configuration settings -- but this should be an explicit flag anyway: it should be possible to provide a description for an advanced config field.)

Review feedback:

• Worked in my manual testing

• Code update looks good (per Tom's agreed upon solution)

• I think it would be desirable that we test this update. You probably will be able to update test/integration/pipeline_instances_test.rb -> def create_and_run_pipeline_in_aproject. You can add an extra assertion before "click_link 'Advanced'" to assert that the Inputs tab shows the selected file.

• It would also be better to update the test "Create and run a pipeline" to do the above assertion. Then click on the Choose button again and verify that it can be edited. (Please merge master into your branch to ensure you have the latest)

• Please do let me know if you are still not able to run the tests and would like me to add the test assertions.

I've added the tests you suggested, though I'm not sure the second one where we check a second time that it can still be edited is worth the overhead that it adds! Anyway, there it is. Thanks.

• The first check to see Inputs tab in "def create_and_run_pipeline_in_aproject" is ok. But we do not want to click on it and repeat selection here. This would then impact all tests (as you said) and is unnecessary overhead. We basically want to revert it back to what it was and just add one extra assertion to see that inputs are visible in Inputs tab after selecting input. After selecting input and clicking ok, we are in the Inputs tab anyways and we just want to assert that the selected input is present.

• I think it would help to verify that input can be selected more than once at least in one test and hence I think updating the one test " test 'Create and run a pipeline' " would be better. So, your new method "check_input_inedible" should only be used by this test and can be simplified to meet this test's needs. BTW, "inedible" a typo?

Sorry for complicating things. Thanks.

I understand now. I've made the changes you recommended. Thanks!

• This version looks awesome.

• One question. Why did you remove the description for the pipeline template fixture? I did not see any test failures if I were to add it back. If there is no compelling reason, would you please put it back.

LGTM after you put the description back in the template fixture. Thanks.

I had to remove the description from the pipeline_template fixture used for testing so that I could reproduce the original problem, which was the disappearing inputs. Because of the way the conditions were written, an input parameter with a description would always be displayed, but one without a description would disappear after being specified. Tom explained why this was probably the case further above in the comments.

I'll merge then. Thanks a lot!