Tag tree actions by will-moore · Pull Request #4518 · ome/openmicroscopy

will-moore · 2016-03-08T16:24:10Z

This PR updates the web with the same Tag & Tag set functionality as Insight.

To test:

Browse tags tree
Creation of tags / tag sets should appear as orphans in tree, unless tag is created with tag set selected (should be created as child of tag set).
- Create Tag and Tag set from toolbar buttons
- Create Tag and Tag set from right-click menu
Cut, Copy and Paste all data types, from toolbar and Right-click menu:
- Tag / Tagset
- Dataset / Project
- Image / Dataset
- Plate / Screen
Drag and Drop all data types that can be Cut as above
Tagged objects can also be Copied from below Tag (NOT Cut, as in Insight)
Delete Tag & Tag set

NB: @pwalczysko, @bramalingam and I had a discussion about whether you should be able to Cut OR Copy orphaned objects E.g.:

Dataset in P/D/I tree
Orphaned Image in P/D/I tree
Tag in Tag tree

Generally agreed that you should be able to Copy these (not current policy in Insight or Web & not changed in this PR).
Re: this PR, we allow to Copy:

Tagged Project/Dataset/Image etc in Tag tree (where you are not Cutting the parent link)
This is conceptually similar to Copy of an Orphaned object (also not Cutting any parent link).

This replicates Insight behaviour

will-moore · 2016-03-11T09:57:27Z

@pwalczysko Could you check whether https://trac.openmicroscopy.org/ome/ticket/13020 is fixed in this PR? I think it is. Thanks.

pwalczysko · 2016-03-11T10:29:38Z

@will-moore : Just checked, yes, the https://trac.openmicroscopy.org/ome/ticket/13020 works as expected for all private, ro, ra, rw groups and for any type of users.

pwalczysko · 2016-03-11T11:00:33Z

Creation of new tags and Tagsets - works as expected for icons and right-click in Tags Tab.

pwalczysko · 2016-03-11T11:01:43Z

Suggestion: Make the Tagset icon more distinctive from Tag icon (atm they look very similar, hard to see on first look which is which).

pwalczysko · 2016-03-11T11:06:07Z

Possible RFE see https://trac.openmicroscopy.org/ome/ticket/13172#ticket (not for this PR).

will-moore · 2016-03-18T11:32:03Z

@chris-allan Actually, just started looking at the tests and the api_links method is actually covered quite well by test_links.py and CSRF tests of manage_action_containers are actually testing that various actions work, so we're not in too bad a state. I'll review more and see what's missing...

chris-allan · 2016-03-18T12:38:39Z

Yep, seeing that now that I peel back some of the layers. Just for my own sanity I might integration test the underlying methods themselves to avoid having to pipe every condition through the myriad of options and 100+ lines of code that are in api_links() but that's up to you.

I accept that manage_action_containers() is 300+ line absolute beast. Looking at the changes more closely it would probably suffice to constrain what we are testing for the moment to the tag vs. tagset differences for o_type and the addnewcontainer branch of the if action... conditional. Making a separate comment on the logic changes there.

Since create_link() is currently an inner function it might make sense to pull it out. It's probably self contained enough that it can even be unit tested instead.

There's no coverage at all for batch_annotate(), correct?

chris-allan · 2016-03-18T12:41:32Z

                                        args=["edit", o_type, o_id]))
-        if o_type is not None and hasattr(manager, o_type) and o_id > 0:
-            # E.g. Parent o_type is 'project'...
+        if o_type == "project" and hasattr(manager, o_type) and o_id > 0:


Since the only object we could be creating in the tree at the moment that has the option of also having a parent is a Dataset that is the reason for constraining this, correct?

Yes, since we don't allow creation of Plates in the tree, and creation of Tag under Tagset is handled below.

Great. Makes sense.

will-moore · 2016-03-18T15:58:53Z

@chris-allan I split relevant tests out of test_csrf.py, added a bunch of tests for tags, split create_link() out of api_links() and split api_links() into smaller functions.

The batch_annotate() is not covered by any integration tests (it just gets HTML for display in right panel) but it is tested by Robot framework.

This fixes a very strange bug when copy and pasting a tag with images and runs from one Tagset to another. The bug occurred if the Run was not the last item (after images). As described in ome#4518 (comment)

will-moore · 2016-03-23T12:01:39Z

@pwalczysko That last commit should fix the copy/paste 404 bug #4518 (comment).

gusferguson · 2016-03-25T08:56:52Z

@will-moore cc @pwalczysko

Tested using https://cowfish.openmicroscopy.org/merge/webclient/ user-4

Repeated workflow that gave Petr 404 error on Windows 8.1:

with IE 11, Firefox and Chrome - behaved as expected - no error.

Repeated on Mac 10.11.3:

Chrome and Firefox - behaved as expected - no error.
Safari and received same 404 errors as Petr did before - see screenshots.
Resolved with refresh as before.

gusferguson · 2016-03-25T09:05:45Z

@will-moore @pwalczysko

Interestingly - I went back to Safari to test the next PR and did a browser reload of the web client.
I could not then replicate the error in Safari - so I suspect this may have been some sort of caching issue.

So I would say this behaves as expected and is good to merge all else being ready.

will-moore · 2016-03-25T09:41:28Z

Thanks @gusferguson.
If @chris-allan is happy with the test coverage, then would be good to merge so I can fix conflicts with #4534

will-moore · 2016-04-14T10:34:19Z

I'd really like to get this merged so I can work on the Copy / Cut ticket in a new PR. cc @jburel

chris-allan · 2016-04-20T08:23:51Z

Happy for now until we fully refactor api_links() to aid testability and and add coverage to as much of the result as possible.

jburel · 2016-04-20T08:25:22Z

Thanks all. Merging

will-moore added 13 commits March 7, 2016 11:13

Don't disable P/D/I right-click menu and toolbar in tags tree

b064fb6

Right-click menu items to create Tag and Tagset

91b4c25

Handle creation of tags and tagsets from context menu

76e3a43

Add Tag and Tagset buttons to toolbar

1756926

Handle Tags linking/deleting in api_link

ac34196

Fix editname/tagset in manage_action_containers

46624c2

Fix Robot tests for right-click on P/D. Create P/D/S enabled

7dcf0f4

Remove print statement

6b89ab7

Don't try to cut link between tag on image 'Cut'

f63d419

flake8 fix

73714d5

Enable cut & copy in Tag Tree correctly

887a4cd

Delete of Tagset leaves Tags as orphans in tree

88666b8

flake8 fix

65703f7

will-moore closed this Mar 9, 2016

will-moore reopened this Mar 9, 2016

will-moore added 3 commits March 10, 2016 09:57

Don't show cut Dataset etc as orphans in Tag Tree

f945e05

On tagset delete, don't orphan tags if still under other tagsets

a92dd7c

Enable multi-selection of tags and tagsets in jsTree

6f34f6f

will-moore force-pushed the tag_tree_actions branch from 76059ff to 6f34f6f Compare March 10, 2016 11:17

will-moore added 2 commits March 10, 2016 12:03

Don't allow right-click 'New' if not canLink()

60e8f4d

This replicates Insight behaviour

Don't allow toolbar 'New' if not canLink()

9d8b0ed

will-moore closed this Mar 10, 2016

will-moore reopened this Mar 10, 2016

will-moore closed this Mar 10, 2016

will-moore reopened this Mar 10, 2016

chris-allan reviewed Mar 18, 2016
View reviewed changes

will-moore added 9 commits March 18, 2016 13:00

Remove un-needed handling of unloaded links in api_links()

ac92389

Move 4 manage_action_container tests to test_containers

a67e6be

Remove unused imports from test_containers.py

b260289

Moved 2 manage_action_container tests to test_tags.py

e8b8fd7

Add test_create_tag_and_tagset() to test_tags.py

20228b4

Add test_edit_tag_and_tagset() to test_tags.py

afa2cd2

Add test_link_unlink_tagset_tags() to test_links.py

0cb5f5b

Move create_link() out of api_links() in views.py

7464dd2

Split api_links() to handle POST and DELETE separately

4da68f8

flake8

8c16c4f

will-moore force-pushed the tag_tree_actions branch from 702d4f0 to 8c16c4f Compare March 21, 2016 23:45

jsTree sort plugin handles images and acquisitions (under tags)

cd9c6c4

This fixes a very strange bug when copy and pasting a tag with images and runs from one Tagset to another. The bug occurred if the Run was not the last item (after images). As described in ome#4518 (comment)

will-moore closed this Mar 24, 2016

will-moore reopened this Mar 24, 2016

will-moore mentioned this pull request Mar 25, 2016

Show tag tagset #4534

Merged

will-moore mentioned this pull request Apr 4, 2016

Show image.name fix #4540

Merged

jburel merged commit 20aa573 into ome:develop Apr 20, 2016

jburel added this to the 5.2.3 milestone May 4, 2016

will-moore deleted the tag_tree_actions branch February 18, 2019 04:08

Conversation

will-moore commented Mar 8, 2016

Uh oh!

will-moore commented Mar 11, 2016

Uh oh!

pwalczysko commented Mar 11, 2016

Uh oh!

pwalczysko commented Mar 11, 2016

Uh oh!

pwalczysko commented Mar 11, 2016

Uh oh!

pwalczysko commented Mar 11, 2016

Uh oh!

will-moore commented Mar 18, 2016

Uh oh!

chris-allan commented Mar 18, 2016

Uh oh!

chris-allan Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

will-moore Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

chris-allan Mar 18, 2016

Choose a reason for hiding this comment

Uh oh!

will-moore commented Mar 18, 2016

Uh oh!

will-moore commented Mar 23, 2016

Uh oh!

gusferguson commented Mar 25, 2016

Uh oh!

gusferguson commented Mar 25, 2016

Uh oh!

will-moore commented Mar 25, 2016

Uh oh!

will-moore commented Apr 14, 2016

Uh oh!

chris-allan commented Apr 20, 2016

Uh oh!

jburel commented Apr 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants