Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow param_value filter to refer to parameters with multiple values #13039

Merged

Conversation

bernt-matthias
Copy link
Contributor

@bernt-matthias bernt-matthias commented Dec 8, 2021

Consider this:

<param name="A"/>
<param name="B" type="select">
    <options ...>
        <filter type="param_value" column="0" ref="A" keep="false"/>
    </options>
</param>

then this already works if A is a simple parameter (text, int, ...) but it failed if A is a select is a select parameter with multiple="true".

The filter also did not work if ref_attribute is used for data parameters with multiple="true" and collection inputs.

Also there were no test so far for the param_value filter.

I would like to call this a bug and backport it .. ?

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these contributions under Galaxy's current license.
  • I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to [email protected].

@github-actions github-actions bot added this to the 22.01 milestone Dec 8, 2021
@bernt-matthias bernt-matthias force-pushed the topic/filter-param-value-multiple branch 3 times, most recently from 6e9b6d1 to d30d49a Compare December 10, 2021 09:39
@bernt-matthias bernt-matthias force-pushed the topic/filter-param-value-multiple branch from d30d49a to 385dbaa Compare December 16, 2021 08:55
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, can you also run the IUC tests against this branch ?

@bernt-matthias
Copy link
Contributor Author

Looks great, can you also run the IUC tests against this branch ?

Good idea. I tried galaxyproject/tools-iuc#4318 but it seems that I have insufficient permissions: Command 'run-all-tool-tests' is not configured for the user's permission level 'none'.

@bernt-matthias
Copy link
Contributor Author

Seems that tests of two tools reproducibly (also local) time out. For macs2 it seems that no param_value filter is used. I will investigate.

@bernt-matthias
Copy link
Contributor Author

Seems that the two tools timing out also timeout on dev because of #13136

So at the moment I can not answer if IUC tool tests are affected.

@bernt-matthias bernt-matthias force-pushed the topic/filter-param-value-multiple branch from 147bea5 to 2e33be7 Compare January 11, 2022 14:53
For instance if we want to have a parameter B to select values
that have not been selected in a parameter A .. and A has
`multiple="true"`

```
<param name="A" type="select" multiple="true"/>
<param name="B" type="select">
    <options ...>
        <filter type="param_value" column="0" ref="A" keep="false"/>
    </options>
</param>
```
interferes with another test
seems that it is not possible to refer other input parameters
that are defined below in the xml
ie data with multiple=true and collections
@bernt-matthias
Copy link
Contributor Author

The IUC tests are finished now: 11 failure 3678 success (compared to 12 in the recent test for the dont_template_version_command branch). the 11 are a subset of the 12.

@mvdbeek mvdbeek merged commit 6fdf4da into galaxyproject:dev Jan 14, 2022
@github-actions
Copy link

This PR was merged without a "kind/" label, please correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants