[24.2] Fix drag and drop for dataset collection elements

[ahmedhamidawan]

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.

[ahmedhamidawan]

I thought it should show an alert if its the wrong history_content_type for FormData (e.g.: Dragging collection to a dataset only input shows no alert). However, I see:

galaxy/client/src/components/Form/Elements/FormData/FormData.vue

Lines 468 to 472 in bf47f53

	if (props.type === "data") {
	// collection can always be mapped over a data input ... in theory.
	// One day we should also validate the map over model
	return true;
	}

but unfortunately dragging a collection to a dataset type input doesn't do anything then:

I'm curious then as to what we should do here 🤔 ?

Update:

I guess one way would be to change it to:

if (props.type === "data") {
    // if this input doesn't accept collections at all, return false
    if (!validSrcs.value.includes(SOURCE.COLLECTION)) {
        $emit("alert", "dataset collection is not a valid input for this dataset parameter.");
        return false;
    }
    // otherwise, collection can always be mapped over a data input ... in theory.
    // One day we should also validate the map over model
    return true;
}

where

const validSrcs = computed(() => {
    return variant.value ? variant.value.map((v) => v.src) : [];
});

[mvdbeek]

Dragging collection to a dataset only input shows no alert)

Because, as the comment says, it should work, and it does work on 24.2 right now:

[mvdbeek]

I would suggest to go back to 139f2d3, restore the canAcceptSrc signature to what it was on the release_24.2 branch, and determine what historyContentType is based on the data coming out of the event store. This has turned into a large refactor that i don't think is correct or necessary in order to fix dragging of collection elements. If there is refactoring to be done I would target dev, if there are other bugs to fix I would open another PR.

[ahmedhamidawan]

Dragging collection to a dataset only input shows no alert)

Because, as the comment says, it should work, and it does work on 24.2 right now:

Compare this to the GIF in my comment. The FormData in my comment has no collection variant, and the collection is definitely not being set as currentValue.

[mvdbeek]

I don't know what collection variant means. This is a simple form data input, and it works fine. If you're fixing something unrelated please describe the issue you're having, and ideally open a different PR.

[ahmedhamidawan]

I don't know what collection variant means. This is a simple form data input, and it works fine. If you're fixing something unrelated please describe the issue you're having, and ideally open a different PR.

Has Collection Button	Does not

Only reason I called it variant is because thats the variable name in FormData. My bad.
It is very clear how when i try to drop a collection in my GIF it doesn't work, given it is an input with no collection button.

Anyways. Ok. I will revert this PR to just:

I would suggest to go back to 139f2d3, restore the ...

and fix the rest targeting dev in another PR.

[ahmedhamidawan]

The first commit 09526e4 fixes this bug (drag and drop not working for DCEs) entirely.

The other one e7fe2f0 adds additional typing, as well as fixing another bug of multiple dataset drag and drop not working.
If this other commit is making this a large refactor still, or will cause trouble merging forward etc, we can totally drop it and just get the other first one in. The rest of the bugs can be fixed in dev.

[mvdbeek]

Thank you, this makes sense to me now, though I worry a little that this might get broken again in the future. A set of regression tests asserting the expected behavior would definitely be nice to have.

[ahmedhamidawan]

Thank you! Yes, I am planning to add good jest coverage (ofc I don't think I can simulate actual drag drop there but at least testing the functions themselves should be possible) in a PR targeting dev.

[mvdbeek]

Yeah, if you can make sure that what goes in and out of the event drag store is handled correctly I think you can cover a lot of ground.