Introduce FsFile-based representation of checked paths and adjust destination of imported files.

[mtbc]

No description provided.

[joshmoore]

Please don't change the copyright of files, and instead just update year ranges (e.g. 2012-2013).

[mtbc]

Can I just leave it alone then? I can't see how, not actually having any relationship with Glencoe, I can possibly be claiming any rights for Glencoe for code I wrote.

[joshmoore]

Labeled as needs-fixes due to

Ice.UnknownException
    unknown = "java.lang.NullPointerException
                at ome.services.blitz.repo.ManagedRepositoryI.suggestOnConflict(ManagedRepositoryI.java:515)
                at ome.services.blitz.repo.ManagedRepositoryI.importFileset(ManagedRepositoryI.java:157)
                at omero.grid._ManagedRepositoryTie.importFileset(_ManagedRepositoryTie.java:64)
                at omero.grid._ManagedRepositoryDisp.___importFileset(_ManagedRepositoryDisp.java:192)
                at omero.grid._ManagedRepositoryDisp.__dispatch(_ManagedRepositoryDisp.java:317)
                at IceInternal.Incoming.invoke(Incoming.java:159)
                at Ice.ConnectionI.invokeAll(ConnectionI.java:2037)
                at Ice.ConnectionI.message(ConnectionI.java:972)
                at IceInternal.ThreadPool.run(ThreadPool.java:577)
                at IceInternal.ThreadPool.access$100(ThreadPool.java:12)
                at IceInternal.ThreadPool$EventHandlerThread.run(ThreadPool.java:971)

in http://hudson.openmicroscopy.org.uk/job/OMERO-merge-develop/139/console

[mtbc]

Interesting, command-line import has been working for me, I'll try with the automatically imported set of files.

[mtbc]

I can import fine as root, the problem seems to come when running the auto_import.py script. In PublicRepositoryI.makeDir we try to create user-1-8/2013-01/31/92428153/fresh_import/test_images_metadata/fv1000.

First, we try to register user-1-8, and that goes through fine.

Then, we try to register user-1-8/2013-01 and that throws a SecurityViolation,

No annotate access for parent directory: 17

[joshmoore]

One possible workaround until the logic for handling the user is put in, would be to put the timestamp at the top.

[mtbc]

Hopefully this will allow the auto import to proceed well, with this latest commit it went okay for me locally.

[jburel]

Closing see #671

[mtbc]

It doesn't look like anything lately merged into develop conflicts with anything here.

[bpindelski]

The overall design looks good. Code looks nicely separated into classes with single responsibilities. Good to merge.

Two things worth looking at are failing tests in CheckedPathTest.java and ManagedRepositoryITest.java. Also the addition of AssumeNG in #642 would save @mtbc writing code to do conditional test execution (Windows/Unix).

[jburel]

@mtbc: general comment. Missing java doc for several methods, it will also be good to describe what the test is about.

[mtbc]

It should be only unit tests that miss Javadoc; I was just following the pattern in the managed repository's unit tests. I can add Javadoc in the next PR though. I tried to make the method names and error string texts quite descriptive so each test's purpose should already be fairly clear.

I was surprised that CheckedPathTest isn't among the FS test suite. The unit tests failures in those classes preexist this PR but, yes, we should make sure there's a row on the spreadsheet for looking at and fixing that.

[jburel]

I usually do not like the fact that we only use the test's name to describe the test itself.
I have seen tests evolving w/o name modification => what you test is not what is written.

[joshmoore]

@mtbc, 4cb86e1 (on joshmoore/fs-path) adds in the top-level user-directory requirement. This sits on top of this PR if you would like to test locally. The Python tests and very likely the Java tests will all need to be updated to take this into account.

[mtbc]

Let's hope that people changing the tests change the Javadoc. (-:

[jburel]

@mtbc: thanks... Let's hope so.

[joshmoore]

@mtbc, done with reviewing all the commits. These are largely stylistic comments for future PRs.

The only other issue that occurs to me is whether or not you want a commit from me to fix the failing unit tests (are there still any?)

[mtbc]

The only failing unit tests that I'm aware of, fail in the same way without this PR. Certainly we should probably look at them and decide what to do with them -- I would have fixed them myself if they were simple and obvious -- but I am happy to help out with actually fixing them.

[jburel]

@mtbc, @joshmoore: happy to discuss. I tend to disagree with not adding doc in general regardless of the methods/tests.

[joshmoore]

@mtbc: re-tests, ok. Couldn't remember if it was with or without the PR. Let's try to get that done when this is merged.

@jburel: yup, we can discuss. But a comment on tearDown unless there is something critical to discuss is like a comment on a getter "Gets the Foo". It's just something to maintain that provides no user value.

[jburel]

I am not talking about tearDown

[joshmoore]

I was. 😄 (That's what my comment was on)

[mtbc]

So as to preserve the value of the review and testing of this PR, I've pushed only some easy fixes; acting on the other comments will be in a later PR so this one can be merged and others can more easily base their work against it.

[joshmoore]

@mtbc, visually these look fine. If you want to try for a new build before Monday, just speak up in devteam. Otherwise, if this passes Monday morning, then let's merge. Thanks!

[joshmoore]

This passed today's build. Merging so that we can start basing the rest of our work off of it.