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

[WIP] Guide users to collection builders #18857

Draft
wants to merge 16 commits into
base: dev
Choose a base branch
from

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Sep 19, 2024

Major action items:

  • By default, instead of the modals treating the items as selections from the current history, automatically filter items valid for the list (e.g.: for a list with csv elements, filter out csvs from the history in this list)
    Done: If there are required extension(s), we filter out the items with the required extension (but we also include any items that have extension that isSubTypeOfAny; meaning it can be implicitly converted to become a valid input.
  • In case nothing can be auto paried for list:paired, do not attempt to auto pair by default and simply show all items.
  • In case the current history is empty and to make it clearer in general, allow history to be switched from within the modal? Seems like a bad idea, history can just be switched from the Switch history modal; and in the case of a history-less (singular history) UI, this won't be a concern either
  • Allow files to be uploaded (and dropped) directly to either the form field or within the list builder once it is opened.

Note: paired collection is still a WIP

Fixes #18704

guide_users_to_collection_builders_wip_2.gif.mp4

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 and all my past contributions to the core galaxy codebase under the MIT license.

@mvdbeek
Copy link
Member

mvdbeek commented Sep 20, 2024

But when would we use the rule builder instead?

For anything else. I would make that a follow up, it isn't that common of a starting point

@@ -39,4 +39,14 @@ export class DatatypesMapperModel {
isSubTypeOfAny(child: string, parents: DatatypesCombinedMap["datatypes"]): boolean {
return parents.some((parent) => this.isSubType(child, parent));
}

/** For classes like `galaxy.datatypes.{parent}.{extension}`, get the extension's parent */
getParentDatatype(extension: string) {
Copy link
Member

Choose a reason for hiding this comment

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

That logic is redundant and also doesn't reflect how datatypes match. Is there a problem with using isSubTypeOfAny ?

@mvdbeek
Copy link
Member

mvdbeek commented Sep 27, 2024

  • the parent datatype (e.g.: For csv, the overall class is: galaxy.datatypes.tabular.CSV, and so the parent would be tabular: this allows for implicit datatype conversion a

Valid implicit conversions are defined by converter targets in the datatypes_conf.xml file (see https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/config/sample/datatypes_conf.xml.sample#L106 for the csv -> tabular conversion), not the class hierarchy. Direct matches without conversion are defined by the class hierarchy. Both should be handled correctly in current code, so if you need to write new logic there is something odd going on.

@mvdbeek
Copy link
Member

mvdbeek commented Sep 27, 2024

  • items for the given extensions
  • the parent datatype (e.g.: For csv, the overall class is: galaxy.datatypes.tabular.CSV, and so the parent would be tabular: this allows for implicit datatype conversion and so the list could be made a valid input for the field

I don't think it's wise to let users choose between these. We don't do that elsewhere in the interface, and users shouldn't care.

  • all items regardless of extension; meaning a list can still be made regardless of the required extension

That should only be possible if all extensions are valid for an input. This is a recipe for extremely hard to debug problems. I've spent months of my life narrowing this down and greatly reducing tool templating errors.

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Sep 27, 2024

  • the parent datatype (e.g.: For csv, the overall class is: galaxy.datatypes.tabular.CSV, and so the parent would be tabular: this allows for implicit datatype conversion a

Valid implicit conversions are defined by converter targets in the datatypes_conf.xml file (see https://github.com/galaxyproject/galaxy/blob/dev/lib/galaxy/config/sample/datatypes_conf.xml.sample#L106 for the csv -> tabular conversion), not the class hierarchy. Direct matches without conversion are defined by the class hierarchy. Both should be handled correctly in current code, so if you need to write new logic there is something odd going on.

I guess this is exactly what I was looking for; something that can indicate which items could be valid implicitly converted inputs for the list given the required explicit extensions. The DatatypesMapper is just what I ended up using and that might have been incorrect/unrelated.

Based on my reply to your review earlier, would it still make sense to add this logic to isSubClassOfAny (or use any existing implementation in the client... or bakend...), or should we forget about this extra step of allowing for potential implicit conversion?

Update:

I have removed this optional choice between the level of filtering and enforced only allowed extensions (and others that are isSubTypeOfAny).

This is an initial/draft implementation. Some of the next steps are:
- By default, instead of the modals treating the items as selections from the current history, automatically filter items valid for the list (e.g.: for a list with csv elements, filter out csvs from the history in this list).
- In case nothing can be auto paried for `list:paired`, do not attempt to auto pair by default and simply show all items.
- In case the current history is empty and to make it clearer in general, allow history to be switched from within the modal?
- Allow files to be uploaded (and dropped) directly to either the form field or within the list builder once it is opened.

One thing I have not planned for yet is the rule builder. I can see that for `list` and `list:paired`, we get that from the `props.collectionTypes` in `FormData`. But when would we use the rule builder instead?

Fixes galaxyproject#18704
This allows a collection type `list` to be created via the collection creater from the workflow/tool form directly.
It tracks the current history changes via the new `useHistoryItemsForType` composable.
It utilises the `FormSelectMany` component to easily move items between selected and unselected for list columns.
The items in the list creator can be filtered for extension, parent datatype or all items in the history, based on whether the form field required a certain extension(s) as input for the list.
This keeps the order in which the user adds items to the selection evident in the selected column.
- Converted the file(s) to composition API and typescript
- Improved styling of the modal and its components

Still need to add `extensions` handling for cases where a certain extension is required for the collection.
The `isSubClassOfAny` was incorrect logic for implicit conversions. `isSubTypeOfAny` gives us what we want as far as filtering items that would be valid for implicit conversions goes.
Also, we concluded that the `All` option was also not acceptable as only valid extensions must be enforced in the collection creator.
Place it right next to the buttons for choosing between batch or singular collection
@ahmedhamidawan ahmedhamidawan force-pushed the guide_users_to_collection_builder branch from 7d474c2 to 12f6957 Compare October 4, 2024 22:45
ahmedhamidawan and others added 3 commits October 17, 2024 19:53
- `buildCollectionModal.ts` still exists but just to create the rules collection modal, all other modals are replaced with a parent `CollectionCreatorModal`
- also added a `collectionBuilderItemsStore` that is used to store the datasets fetched for the builder; only when the builder is opened: which is when we start reacting to any changes in the `historyId`, `history.update_time` and any filter on a history selection.
`stringifyObject` does not need to be re-run for every selected value.

Co-authored-by: Laila Los <44241786+ElectronicBlueberry@users.noreply.github.com>
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.

Guide users to collection builders
3 participants