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

Add possibility to load collection into library #3559

Merged
merged 13 commits into from Mar 21, 2017

Conversation

@mvdbeek
Copy link
Member

commented Feb 4, 2017

and to conserve element_identifier.

The idea here is to simply add all hdas of a collection of collection_type list into a folder,
while passing along the element_identifier so that the created LDDA has the element_identifier as its name.
I have limited this to flat collections, since there are quite some choices to be made regarding the organization and naming of nested and/or paired collections. The new unzip/flatten collection tools can be used to create a flattened collection and transfer that one to the library.

A bunch of tests are included that test the creation of library/folder items via HDAs and HDCAs,
and a test that checks that the security checking works correctly after refactoring the Exception handling and moving the shared code in the FolderContentsController and LibraryContentsController into the UsesLibraryMixinItems.

I'm putting it out there so @martenson can stop me early if this would block future plans of supporting full scale collections in the library.

@mvdbeek mvdbeek added this to the 17.05 milestone Feb 4, 2017

@mvdbeek mvdbeek requested a review from martenson Feb 4, 2017

@mvdbeek mvdbeek force-pushed the mvdbeek:add_hdca_to_library branch 2 times, most recently from 752468a to aea5768 Feb 4, 2017

@mvdbeek mvdbeek force-pushed the mvdbeek:add_hdca_to_library branch from aea5768 to 8906f51 Feb 4, 2017

@mvdbeek mvdbeek changed the title WIP: Add possibility to load collection into library Add possibility to load collection into library Feb 5, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2017

This is how the "add datasets from history" dialog looks like now.
The first two checkboxes are greyed out and not clickable
collection_to_library

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2017

@galaxybot test this

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2017

This is working well, but there's still some work to be done.

  • Hidden and failed datasets are shown in the selector (has been like that before this PR as well) fixed with 828422f
  • Elements imported from collections are hidden (sometimes, but not always it seems) when imported into new histories. I hope simply flipping an attribute in the LDDA is enough to change that. fixed with 75994c8

@mvdbeek mvdbeek added status/WIP and removed status/review labels Feb 5, 2017

@mvdbeek mvdbeek force-pushed the mvdbeek:add_hdca_to_library branch from bb7bd20 to 828422f Feb 6, 2017

@mvdbeek mvdbeek removed the status/WIP label Feb 6, 2017

@bgruening

This comment has been minimized.

Copy link
Member

commented Feb 12, 2017

Work in this area is so much needed! Thanks a lot @mvdbeek!

@jmchilton

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

@mvdbeek I like this better as I sit with it. There are some good directions we could take this in and that is exciting. Thanks a ton for the contribution and for the tests!

@martenson

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

This has an interesting side effect that the datasets you make the collection from (in history) are no longer importable by themselves to the library (they don't show in the modal).

They are still part of the API response when loading the modal.

'<%= _.escape(history_item.get("hid")) %>: <%= _.escape(history_item.get("name")) %> (Dataset Collection of type <%= _.escape(collection_type) %> not supported.)',
'</a></li>',
'<% } %>',
'<% } else if (history_item.get("visible") === true && history_item.get("state") === "ok") { %>',

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Mar 21, 2017

Author Member

@martenson filtering collection elements is happening here. I think it is more logical that you can only add datasets that are visible and whose state is ok. Otherwise in a history with large collections you'd have a huge amount of datasets to select. But I can certainly revert that if you prefer.

This comment has been minimized.

Copy link
@martenson

martenson Mar 21, 2017

Member

The ones I am talking about are both visible and ok.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Mar 21, 2017

Author Member

How are you testing this? In a history where I uploaded 4 files, and I create a dataset list out of the 4 files I can select the four files separately, and I can also select the collection. If I select all 5 entries (4 separate HDAs plus one HDCA), I get the expected 8 items added to the data library.

This comment has been minimized.

Copy link
@martenson

martenson Mar 21, 2017

Member

first not shown, second (a collection with the first in it) shown, third shown

    {
        "create_time": "2017-03-21T14:37:14.404894",
        "dataset_id": "7cba3ea516d680a6",
        "deleted": false,
        "extension": "txt",
        "hid": 5,
        "history_content_type": "dataset",
        "history_id": "082d89a459541054",
        "id": "7cba3ea516d680a6",
        "name": "Pasted Entry",
        "purged": false,
        "state": "ok",
        "type": "file",
        "type_id": "dataset-7cba3ea516d680a6",
        "update_time": "2017-03-21T14:37:20.122821",
        "url": "/api/histories/082d89a459541054/contents/7cba3ea516d680a6",
        "visible": true
    },
    {
        "collection_type": "list",
        "deleted": false,
        "hid": 6,
        "history_content_type": "dataset_collection",
        "history_id": "082d89a459541054",
        "id": "7cba3ea516d680a6",
        "name": "zzz list",
        "populated": true,
        "populated_state": "ok",
        "populated_state_message": null,
        "type": "collection",
        "url": "/api/histories/082d89a459541054/contents/dataset_collections/7cba3ea516d680a6",
        "visible": true
    },
    {
        "create_time": "2017-03-21T14:51:35.040707",
        "dataset_id": "082d89a459541054",
        "deleted": false,
        "extension": "txt",
        "hid": 7,
        "history_content_type": "dataset",
        "history_id": "082d89a459541054",
        "id": "082d89a459541054",
        "name": "Pasted Entry",
        "purged": false,
        "state": "ok",
        "type": "file",
        "type_id": "dataset-082d89a459541054",
        "update_time": "2017-03-21T14:51:40.480589",
        "url": "/api/histories/082d89a459541054/contents/082d89a459541054",
        "visible": true
    }

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Mar 21, 2017

Author Member

The first and second have the same id ... should that be possible ?? Not that it would explain what's going on.

This comment has been minimized.

Copy link
@martenson

martenson Mar 21, 2017

Member

@mvdbeek that is probably it. BB collection will filter models with same ID. and this Galaxy instance is new so these are 1,2,3,4 for both datasets and collections

ping @jmchilton

martenson added some commits Mar 21, 2017

@martenson
Copy link
Member

left a comment

The unearthed bug is somewhat unrelated to this PR and afaik has no easy solution. It is also very rare and will probably never happen on e.g. Main or any production instance.
I am fine with the approach here. Thanks @mvdbeek

@martenson martenson merged commit 2795485 into galaxyproject:dev Mar 21, 2017

5 checks passed

api test Build finished. 267 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 140 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 25 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details
@@ -160,7 +163,7 @@ def create( self, trans, library_id, payload, **kwd ):
* from_hda_id: (optional, only if create_type is 'file') the
encoded id of an accessible HDA to copy into the library

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 22, 2017

Member

from_hdca_id should be documented here.

if create_type == 'file':
rval = []
if from_hda_id:
rval.append(self._copy_hda_to_library_folder( trans, self.hda_manager, self.decode_id(from_hda_id), real_folder_id, ldda_message ))

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 22, 2017

Member

This will break BioBlend.objects (and maybe other user scripts), which expects the return value to be a dict when copying a single file using from_hda_id, see https://travis-ci.org/nsoranzo/bioblend/jobs/213674739#L1484 .

Not sure if it is better to restore the previous behaviour or workaround it in BioBlend.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Mar 22, 2017

Author Member

I was thinking it would be better to consistently return a list, but I can change this to unpack the list if the length of the list is 1. I don't have a preference either way, just let me know!

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 22, 2017

Member

I surely understand why you did it this way, but I tend to be conservative when it comes to the API. Instead of unpacking the list afterwards you can just replace lines 217-222 with:

        if create_type == 'file':
            if from_hda_id:
                rval = self._copy_hda_to_library_folder( trans, self.hda_manager, self.decode_id(from_hda_id), real_folder_id, ldda_message )
            if from_hdca_id:
                rval = self._copy_hdca_to_library_folder(trans, self.hda_manager, self.decode_id(from_hdca_id), real_folder_id, ldda_message)

This comment has been minimized.

Copy link
@jmchilton

jmchilton Mar 24, 2017

Member

My vote would be that if from_hda_id is supplied we retain the original functionality and return a dict describing the result to retain backward compatibility and because I think this makes the most intuitive sense for such a call.

If this newer from_hdca_id argument is supplied - it can return a list - that is fine - or perhaps it could return the folder information as an object instead of a list? I'd be happy to take a stab at this - I'm sorry I didn't notice this initially.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Mar 24, 2017

Author Member

thanks @jmchilton, that sounds like a good plan to me.

if create_type == 'file' and from_hda_id:
return self._copy_hda_to_library_folder( trans, from_hda_id, library_id, real_folder_id, ldda_message )
from_hda_id, from_hdca_id, ldda_message = ( payload.pop( 'from_hda_id', None ), payload.pop( 'from_hdca_id', None ), payload.pop( 'ldda_message', '' ) )
log.debug(payload)

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Mar 22, 2017

Member

This can probably be removed.

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Mar 31, 2017

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Apr 3, 2017

jmchilton added a commit to jmchilton/galaxy that referenced this pull request Apr 3, 2017

mvdbeek added a commit that referenced this pull request Apr 4, 2017

Merge pull request #3857 from jmchilton/3559_touchups
Touch up #3559 based on post-merge discussions.

@bgruening bgruening referenced this pull request Jun 2, 2017

Open

New data libraries - missing features #1177

10 of 18 tasks complete

@mvdbeek mvdbeek deleted the mvdbeek:add_hdca_to_library branch Jun 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.