Story #2760

Reconcile name link behavior with intrinsic object names so that the name hierarchy in arv-mount is the same as the name hierarchy in Workbench

Added by Tom Clegg over 7 years ago. Updated over 7 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
05/11/2014
Due date:
% Done:

100%

Estimated time:
(Total: 19.50 h)
Story points:
3.0

Description

This may involve

  • having Workbench visit "name link" UUIDs instead of referenced object UUIDs. This way breadcrumbs can say "arvados > containing-folder > name-of-object-in-that-folder".

Subtasks

Task #2799: Sort out two types of names vs. ownershipResolvedTom Clegg

Task #2840: Create "home" view with folder hierarchyResolvedTom Clegg

Task #2843: Add "move to folder" widget to folders#show pageResolvedTom Clegg

Task #2847: Fix editable? so folders-in-folders are editableResolvedTom Clegg

Task #2846: Review 2760-folder-hierarchyResolvedTom Clegg

Task #2856: Offer folder behavior only for pipeline instances, templates, jobs, collections, humans, specimensResolvedTom Clegg

Task #2867: Review 2760-not-all-objects-in-foldersResolvedTom Clegg

Associated revisions

Revision 19d10711
Added by Tom Clegg over 7 years ago

2760: Merge branch 'master' into 2760-linked-names-only refs #2760

Revision a1e0f759
Added by Tom Clegg over 7 years ago

2760: Merge branch '2760-folder-hierarchy' refs #2760

Revision b80867f0
Added by Tom Clegg over 7 years ago

2760: Merge branch '2760-not-all-objects-in-folders' refs #2760

History

#1 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg over 7 years ago

  • Subject changed from Reconcile name link behavior with intrinsic object names to Reconcile name link behavior with intrinsic object names so that the name hierarchy in arv-mount is the same as the name hierarchy in Workbench

#3 Updated by Tom Clegg over 7 years ago

  • Assigned To set to Tom Clegg

#4 Updated by Tom Clegg over 7 years ago

  • Status changed from New to In Progress

#6 Updated by Brett Smith over 7 years ago

Reviewing 84decc8. The code here is very clean and easy to follow; thank you.

Please have FoldersTest#setup call super. This will be functional when you merge, because master implements IntegrationTest#setup to reset the Capybara session for improved isolation.

Is there a way to unparent a folder? In other words, make it a direct child of the "My Folders" pseudofolder? If this is possible, it wasn't clear from the interface.

The rest is more stylistic and needn't block a merge: FoldersController#index defines inner methods with def. I learned recently that this is apparently discouraged Ruby style, because the way Ruby scopes work, you're actually defining an instance method, which can lead to surprises later. To illustrate:

> class Foo
>   def bar
>     def baz
>       42
>     end
>     baz + 2
>   end
> end
 => :bar
> f = Foo.new
 => #<Foo:0x000000013403e0>
> f.baz
NoMethodError: undefined method `baz' for #<Foo:0x000000013403e0>
        from (irb):17
        from /home/brett/.rvm/rubies/ruby-2.1.1/bin/irb:11:in `<main>'
> f.bar
 => 44
> f.baz
 => 42
> g = Foo.new
 => #<Foo:0x0000000129e540>
> g.baz
 => 42

Recommended style is to define the method directly on the class, or if you need a closure, use lambda.

For the Collections file tree, one reason I was kind of adamant about using the list tags and styling them as desired is because I thought it would be better for accessibility. Someone using a screen reader would be empowered to navigate the list more quickly, skip entire lists, etc. I presume we have lots of accessibility obstacles right now, but I wanted to avoid introducing new ones at least. I realize it comes with its own problems like funky highlighting, but I'll throw this out there for consideration. Again, I don't consider this an obstacle to merging.

Thanks.

#7 Updated by Tom Clegg over 7 years ago

Brett Smith wrote:

Reviewing 84decc8. The code here is very clean and easy to follow; thank you.

Please have FoldersTest#setup call super. This will be functional when you merge, because master implements IntegrationTest#setup to reset the Capybara session for improved isolation.

Aha, I didn't realize I was being sneaky but apparently I was. It doesn't override setup, it uses "setup do ..." which is shorthand for "super; ...", aka "add this to the list of setup tasks". (Looking at docs it seems "before do ..." is the canonical form and setup is an alias.)

Is there a way to unparent a folder? In other words, make it a direct child of the "My Folders" pseudofolder? If this is possible, it wasn't clear from the interface.

Yes, you should be able to do this, but currently your own name is your top-level folder. Perhaps just need to show "My Folders" as the top level folder (owner_uuid=user_uuid under the hood) rather than a "My Folders" non-item + "Your Name Here" top-level folder. Sound right?

The rest is more stylistic and needn't block a merge: FoldersController#index defines inner methods with def. I learned recently that this is apparently discouraged Ruby style, because the way Ruby scopes work, you're actually defining an instance method, which can lead to surprises later. To illustrate:

Recommended style is to define the method directly on the class, or if you need a closure, use lambda.

Hm, yes, in fact that's not the scope I was expecting. Fixed in c95e276

For the Collections file tree, one reason I was kind of adamant about using the list tags and styling them as desired is because I thought it would be better for accessibility. Someone using a screen reader would be empowered to navigate the list more quickly, skip entire lists, etc. I presume we have lots of accessibility obstacles right now, but I wanted to avoid introducing new ones at least. I realize it comes with its own problems like funky highlighting, but I'll throw this out there for consideration. Again, I don't consider this an obstacle to merging.

Good point. I'm OK with leaving it as a "known issue" right now. (Would "div within div" help a screen reader? That would give us more options for highlighting styles, while still expressing the nested structure. Surely we'll want something nested in any case when we add expand/collapse.)

#8 Updated by Brett Smith over 7 years ago

Reviewing c95e2762

Tom Clegg wrote:

Brett Smith wrote:

Please have FoldersTest#setup call super. This will be functional when you merge, because master implements IntegrationTest#setup to reset the Capybara session for improved isolation.

Aha, I didn't realize I was being sneaky but apparently I was. It doesn't override setup, it uses "setup do ..." which is shorthand for "super; ...", aka "add this to the list of setup tasks". (Looking at docs it seems "before do ..." is the canonical form and setup is an alias.)

Not sneaky, I should've caught that. Thanks for explaining. This does seem like a better approach overall.

Is there a way to unparent a folder? In other words, make it a direct child of the "My Folders" pseudofolder? If this is possible, it wasn't clear from the interface.

Yes, you should be able to do this, but currently your own name is your top-level folder. Perhaps just need to show "My Folders" as the top level folder (owner_uuid=user_uuid under the hood) rather than a "My Folders" non-item + "Your Name Here" top-level folder. Sound right?

Yeah, that was what tripped me up. I understood why "My Folders" and "Shared With Me" were distinct, but I didn't understand it wasn't possible to go between "My Folders" and "[account name]." I think your proposal would be a great improvement if it's not too difficult.

For the Collections file tree, one reason I was kind of adamant about using the list tags and styling them as desired is because I thought it would be better for accessibility. Someone using a screen reader would be empowered to navigate the list more quickly, skip entire lists, etc. I presume we have lots of accessibility obstacles right now, but I wanted to avoid introducing new ones at least. I realize it comes with its own problems like funky highlighting, but I'll throw this out there for consideration. Again, I don't consider this an obstacle to merging.

Good point. I'm OK with leaving it as a "known issue" right now.

I think that's good. Later on maybe we can figure out one solution for styling trees generally, and then apply that to all the ones we have. (One idea that came to me was a sort of hybrid of my approach and yours: use semantic list markup, but style away all the list margins and padding, and instead put an empty div at the beginning of each li to visually present the hierarchy.)

(Would "div within div" help a screen reader? That would give us more options for highlighting styles, while still expressing the nested structure. Surely we'll want something nested in any case when we add expand/collapse.)

Off the top of my head, I don't really know. I'm doubtful, because the whole point of divs is that they don't provide any semantics. But I know some people I could ask.

#11 Updated by Tom Clegg over 7 years ago

#13 Updated by Brett Smith over 7 years ago

I think 7c8e4a8d is good to merge. Thanks.

#14 Updated by Brett Smith over 7 years ago

Reviewing 52bf3c092. It looks good to merge to me. I'm a little nervous that the API server and Workbench have to duplicate information about what data can go in folders—I'm worried that we'll see weird bugs later if they diverge—but I don't have a better idea either. I'm not even sure where you might document this and be relatively confident that people could see it when they worked on this, starting from either end.

#15 Updated by Tom Clegg over 7 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF