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

fix #12011: move/delete datasets that share images #2180

Closed
wants to merge 5 commits into from

Conversation

mtbc
Copy link
Member

@mtbc mtbc commented Mar 19, 2014

not ready for review
Fixes http://trac.openmicroscopy.org.uk/ome/ticket/12011 and assists http://trac.openmicroscopy.org.uk/ome/ticket/11610.

Try moving and deleting datasets that contain images that are also in other datasets. Something not-obviously-wrong ought to happen, which probably suffices until the major graph traversal reworking of https://trac.openmicroscopy.org.uk/ome/ticket/11779 is done. Datasets should be movable and deletable and their images ought not to be left orphaned.

Also check that http://ci.openmicroscopy.org/job/OMERO-5.0-merge-integration-python/lastSuccessfulBuild/testReport/test.gatewaytest/test_chgrp/ and http://ci.openmicroscopy.org/job/OMERO-5.0-merge-integration-python/lastSuccessfulBuild/testReport/test.integration.test_chgrp/TestChgrp/ have no skips or fails.

@bpindelski
Copy link

There is a possibility that this is breaking http://ci.openmicroscopy.org/view/Failing/job/OMERO-5.0-merge-integration-python/160/. Holding off with review for now.

@mtbc mtbc added the dev_5_0 label Mar 20, 2014
@mtbc
Copy link
Member Author

mtbc commented Mar 20, 2014

Okay to review. @ximenesuk is kindly sorting out the integration tests to match the new behavior.

@joshmoore
Copy link
Member

I'm not sure if just changing the tests will work here. Probably needs a review of spreadsheets and such. /cc @pwalczysko @bramalingam etc.

@mtbc
Copy link
Member Author

mtbc commented Mar 20, 2014

The change is just that now when you try to move a dataset, it moves what it can but leaves behind images that can't be moved. (Or, same for delete.) The tests were moving a dataset containing an image that couldn't be moved, and expecting the move to fail: the fix is to instead have them expect the image not to move.

The do-what-you-can feels like a feature to me, but if it should just return an error instead and not do anything and have the user sort out which images are the problem ones, we could do that instead.

@mtbc
Copy link
Member Author

mtbc commented Mar 20, 2014

The curious can try the do-what-you-can out on gretzky now. (-:

@mtbc
Copy link
Member Author

mtbc commented Mar 21, 2014

@pwalczysko can decide if this is okay before @ximenesuk fixes the tests and this PR goes in.

@pwalczysko
Copy link
Member

will deal with this after return from US.

@will-moore
Copy link
Member

Move to Group works fine in both clients - Images stays behind if it is in a Dataset that is not moving. Delete also works OK (tested in web).

@mtbc
Copy link
Member Author

mtbc commented Mar 21, 2014

Great, thank you very much for testing.

@pwalczysko
Copy link
Member

screen shot 2014-03-31 at 12 09 30
The above problem is catered for by @ximenesuk I understand ? (see screenshot)
The only thing I have here is following workflow (in Insight):

-create a dataset ("Duplicate-1")
-copy an image to it (from another dataset named "Duplicate-2")
-Move "Duplicate-1" dataset to another group
-Move proceeds, the image is moved with okay -> expected
-Go back to original group and check the "Duplicate-2" dataset
-the copied image there is not able to display, first I see black thumb, then the black thumb vanishes -> not expected, I got no warning

If the 1) problem is really catered for and the 2) problem is possibly envisaged as "necessary evil at this stage", then ready to merge.

@mtbc
Copy link
Member Author

mtbc commented Mar 31, 2014

@ximenesuk can cater for integration test issues but the other problem is unacceptable: I will investigate.

@mtbc
Copy link
Member Author

mtbc commented Mar 31, 2014

Oh, I see what you mean: this is an issue where the client's view of the dataset got out of sync, so that also if you refresh "Duplicate-2" after the move, the copied-then-moved image also goes away and the black thumb never returns? If so I am much happier, especially as no nasty exception appears; I could file a ticket about that.

@mtbc
Copy link
Member Author

mtbc commented Mar 31, 2014

@pwalczysko: apart from the temporary black thumbnail issue, do you see any other problems? Is this moving/deleting behavior in general an okay idea?

@mtbc
Copy link
Member Author

mtbc commented Mar 31, 2014

@ximenesuk: okay to receive test fixes now then I guess, PR or cherry-pick as you like.

@pwalczysko
Copy link
Member

@mtbc yes, otherwise it works fine. It is as you describe, the black thumb is temporarily there, afterwards vanishes and does not come back.

(I meant I should have been warned about the vanishing of the copied image, but this is a matter of larger discussion, this PR improves matters definitely).

@mtbc
Copy link
Member Author

mtbc commented Mar 31, 2014

Great, thank you. If we merge this PR I can file a ticket about that then.

@mtbc
Copy link
Member Author

mtbc commented Apr 1, 2014

@ximenesuk discovered that this PR allows splitting MIFs via the API and is therefore presently unacceptable.
Update: now appears to work.

@mtbc mtbc changed the title fix #12011: move/delete datasets that share images EXCLUDED: fix #12011: move/delete datasets that share images Apr 11, 2014
@mtbc mtbc changed the title EXCLUDED: fix #12011: move/delete datasets that share images fix #12011: move/delete datasets that share images Apr 14, 2014
@mtbc
Copy link
Member Author

mtbc commented Apr 15, 2014

--exclude as this PR now breaks plate delete.

@mtbc
Copy link
Member Author

mtbc commented Apr 15, 2014

Some dataset delete test tweaks probably coming from @ximenesuk.

@ximenesuk
Copy link
Contributor

I think this addresses the issue correctly ximenesuk@861a69f Please cherry-pick.

@joshmoore
Copy link
Member

@mtbc: is this ready for review again?

@mtbc
Copy link
Member Author

mtbc commented Apr 24, 2014

No, this is the PR depending on our "spec.xml advice" e-mail thread. I can experiment some more with it myself, though, or if your judgment is that it's probably going to just be too hairy then maybe we just close and postpone until after the alternative graph traversal implementation work settles. (At least for the original ticket, perhaps client people can at least catch the error and provide a helpful dialog message.)

@joshmoore
Copy link
Member

Added http://trac.openmicroscopy.org.uk/ome/ticket/11610 to https://trac.openmicroscopy.org.uk/ome/ticket/11752 since this will likely profit from the new graph implementation. http://trac.openmicroscopy.org.uk/ome/ticket/12011 needs to be re-opened for a client review. /cc @will-moore @jburel

Closing for the moment until we have a better way to solve these issues.

@joshmoore joshmoore closed this Apr 24, 2014
@mtbc mtbc deleted the trac-12011-dataset-image-graph branch April 29, 2014 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants