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

[24.2] Guide users to collection builders #18857

Open
wants to merge 24 commits into
base: release_24.2
Choose a base branch
from

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Sep 19, 2024

Major action items (UPDATE 11/19 now complete for this PR):

  • 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.
    Done: The collection creator (for all types: list, paired, list:paired) allows users to upload datasets (even binding those to the required extension(s)) directly within itself, and even adds them to the resulting list (only in the case of list, for the rest they are shown in the updated available datasets to add list).

Note: paired collection is still a WIP Done ✅

Fixes #18704

guide_users_to_collection_builder.mp4

Future improvements incoming:

Maybe we can merge this for the 24.2 release and then I can build upon these changes with the following enhancements:

  • Biggest update will be when we add a "Create your own collection type" interface, which allows users to create a collection of any shape; currently only list, list:paired and paired are supported/improved by these changes. It would be cool if users can create a shape like list:list:paired on the fly. Please let me know if this is a good idea and/or what are other possibilities that can empower users further here.
  • This took A LOT of refactoring of collection creator components and we have quite a bit of duplication in each of these; meaning we would benefit from these files being refactored into reusable components (maybe even a singular component).
  • There are several little TODOs here and there right in the code changes in this PR which would improve UX and the code itself

Outdated screencast

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

@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).

@ahmedhamidawan ahmedhamidawan marked this pull request as ready for review November 19, 2024 05:52
@ahmedhamidawan ahmedhamidawan changed the title [WIP] Guide users to collection builders Guide users to collection builders Nov 19, 2024
@github-actions github-actions bot added this to the 25.0 milestone Nov 19, 2024
@ahmedhamidawan ahmedhamidawan force-pushed the guide_users_to_collection_builder branch 2 times, most recently from 53e5995 to a69b79a Compare November 20, 2024 01:44
@ahmedhamidawan ahmedhamidawan modified the milestones: 25.0, 24.2 Nov 20, 2024
@ahmedhamidawan ahmedhamidawan changed the base branch from dev to release_24.2 November 20, 2024 16:18
@ahmedhamidawan ahmedhamidawan changed the title Guide users to collection builders [24.2] Guide users to collection builders Nov 20, 2024
@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Nov 20, 2024

Seleniums definitely related, taking a look!

@jdavcs jdavcs added the highlight Included in user-facing release notes at the top label Nov 20, 2024
@ahmedhamidawan ahmedhamidawan force-pushed the guide_users_to_collection_builder branch from a69b79a to bd9746a Compare November 22, 2024 16:13
jmchilton and others added 6 commits November 22, 2024 10:13
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.
ahmedhamidawan and others added 17 commits November 22, 2024 10:13
- 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
- `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 <[email protected]>
In the case of `list`, they are now added directly to the final list.
In the case of `list:paired` and `paired`, they are added to the `workingElements`; the user can then manually add them.
This is all still done within the `CollectionCreator`.

Next thing to look at is to try to only allow/wait for those uploaded files to be attempted as collection additions if they are of `ok` state.
One way to do that is to maybe use a list to keep track of uploaded items in the creator, and if those uploaded items disappear from `workingElements` (meaning they are invalid) we have explicit Toasts mentioning this.
@ahmedhamidawan ahmedhamidawan force-pushed the guide_users_to_collection_builder branch 3 times, most recently from 6cdb23b to cb18589 Compare November 22, 2024 19:03
fix selector for name collection in collection builder

fix hide original items selector in collection creator

fix clear filters selector in collection creator
@ahmedhamidawan ahmedhamidawan force-pushed the guide_users_to_collection_builder branch from cb18589 to 5090d7b Compare November 23, 2024 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dataset-collections area/UI-UX highlight Included in user-facing release notes at the top kind/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Guide users to collection builders
6 participants