Story #1970

Create groups and use them like project folders to manage objects in Workbench.

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

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

100%

Estimated time:
(Total: 33.00 h)
Story points:
5.0
Release relationship:
Auto

Description

UI features
  • All readable folders show up in left nav section
    • reasonable max height, scroll if needed
    • type in "filter" box to filter (live in browser, not ajax)
    • "New folder" button → pop-up → refresh the folder list
  • "Folders" page lists my folders, and folders shared with me (either mine are at the top, or I can filter all/sharedwithme/justmine)
    • Rename and delete can be done here
  • "Show folder" page
    • "Contents": list all objects that are in the folder
    • "Sharing": list everyone who has access to the folder (see #2044)
    • "Activity": list recent activity (see #2056)
    • This is essentially a "Dashboard" page, restricted to "stuff in this folder/group" instead of "my stuff".
  • Adding and removing objects from a folder
    • "Add all to..." button on existing "selections" drop-down → select a writable folder from a list
    • "Add to..." drop-down at top of object pages
    • "Add to..." drop-down next to each object on index pages
    • "Remove from folder" button next to each object in folder view
API server features
  • Refuse to delete a group if it is the owner_uuid of any object
  • Add a "group_class" attribute. (Set to "folder" for folders.)
Screenshot from 2014-04-14 16_19_58.png (92.5 KB) Screenshot from 2014-04-14 16_19_58.png groups#show mockup Tom Clegg, 04/14/2014 04:20 PM
Screenshot from 2014-05-01 07_50_40.png (115 KB) Screenshot from 2014-05-01 07_50_40.png Tom Clegg, 05/01/2014 10:52 AM

Subtasks

Task #2698: Editable name link for each item in a folderResolvedTom Clegg

Task #2630: Buttons to add/remove foldersResolvedTom Clegg

Task #2721: Test ownership of new name linksResolvedTom Clegg

Task #2697: Add "Move to folder" widget on "show" pagesResolvedTom Clegg

Task #2624: Show list of folders somewhere in left nav areaResolvedTom Clegg

Task #2172: New UI to present contents of groupResolvedTom Clegg

Task #2727: Add "remove" buttons to folder contents panelResolvedTom Clegg

Task #2720: Prevent duplicate name links after add+editResolvedTom Clegg

Task #2726: Add search feature to folder contents panelResolvedTom Clegg

Task #2735: Review 1970-folder-view branchResolvedBrett Smith

Associated revisions

Revision fc442822
Added by Tom Clegg over 7 years ago

Merge branch '1970-folder-view'

closes #1970

History

#1 Updated by Tom Clegg over 7 years ago

  • Subject changed from Create, browse, display, and share groups/projects in Workbench (sets of collections, pipelines, etc) to Create, browse, display, set-default, and share groups/projects in Workbench (sets of collections, pipelines, etc)

#2 Updated by Peter Amstutz over 7 years ago

  • Story points set to 2.0

#3 Updated by Tom Clegg over 7 years ago

  • Target version set to 2014-05-07 Storing and Organizing Data

#4 Updated by Tom Clegg over 7 years ago

  • Target version changed from 2014-05-07 Storing and Organizing Data to 2014-03-05 Data management

#5 Updated by Tom Clegg over 7 years ago

  • Subject changed from Create, browse, display, set-default, and share groups/projects in Workbench (sets of collections, pipelines, etc) to Create groups and use them like folders to manage objects in Workbench.

#6 Updated by Tom Clegg over 7 years ago

  • Subject changed from Create groups and use them like folders to manage objects in Workbench. to Create groups and use them like project folders to manage objects in Workbench.

#7 Updated by Tom Clegg over 7 years ago

  • Target version changed from 2014-03-05 Data management to 2014-05-07 Storing and Organizing Data

#8 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#9 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#10 Updated by Tom Clegg over 7 years ago

  • Target version changed from 2014-05-07 Storing and Organizing Data to 2014-04-16 Dev tools and data/resource management

#11 Updated by Tom Clegg over 7 years ago

  • Story points changed from 2.0 to 5.0

#12 Updated by Tom Clegg over 7 years ago

  • Story points changed from 5.0 to 3.0

#13 Updated by Tom Clegg over 7 years ago

  • Target version changed from 2014-04-16 Dev tools and data/resource management to 2014-05-07 Storing and Organizing Data

#14 Updated by Tom Clegg over 7 years ago

  • Target version deleted (2014-05-07 Storing and Organizing Data)
  • Release changed from 4 to 8

#15 Updated by Tom Clegg over 7 years ago

  • Target version set to 2014-05-07 Storing and Organizing Data

#16 Updated by Tom Clegg over 7 years ago

  • Story points changed from 3.0 to 5.0

#17 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#18 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#20 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#21 Updated by Tom Clegg over 7 years ago

  • Assigned To set to Tom Clegg

#23 Updated by Brett Smith over 7 years ago

I'm looking at c52a78b3, and I'm calling it now: Tom wins this sprint review. No other demo could possibly compare to this branch for raw eye candy. "Oh, we can run Crunch jobs in Docker containers now? That's nice. Me? Oh, I just MADE THE WORKBENCH INTERFACE GLORIOUS. BEHOLD my expansive collection of beautiful screenshots!" I can hear the business team ohh-ing and ahh-ing already.

Seriously, this is very impressive work. Kudos. My comments feel even more nit-pickier than usual by comparison (and really, they're mostly questions for understanding rather than suggestions for improvements), but here goes anyway:

  • ActionsController calls #send(sym.to_s). Sending symbols is allowed (and I think preferred), so you can omit .to_s.
  • FoldersController#index returns @object. What is that for index methods?
  • The first folder integration test has no assertion. Could you assert find('.panel').has_text?('I just edited this.')?
  • Maybe related, I'm curious why you removed some of the intermediate assertions in the existing integration tests.
  • I'm not wild about the cloud icon for virtual machines in the navigation sidebar. Virtual machines may be in the cloud, but they are not themselves the cloud. Is there a generic computer/workstation icon? A work desk might make sense, too, since the virtual machine is where people work on Crunch scripts.

Thanks much.

#24 Updated by Tom Clegg over 7 years ago

Brett Smith wrote:

Seriously, this is very impressive work. Kudos. My comments feel even more nit-pickier than usual by comparison (and really, they're mostly questions for understanding rather than suggestions for improvements), but here goes anyway:

Thanks!

  • ActionsController calls #send(sym.to_s). Sending symbols is allowed (and I think preferred), so you can omit .to_s.

Indeed, fixed.

  • FoldersController#index returns @object. What is that for index methods?

Hm, not actually sure why that got there. Removed.

  • The first folder integration test has no assertion. Could you assert find('.panel').has_text?('I just edited this.')?

Fixed. (I had commented it out pending discovering the magic incantation to reconcile Capybara with x-editable. "visit current_path" isn't perfect but better than nothing.)

  • Maybe related, I'm curious why you removed some of the intermediate assertions in the existing integration tests.

Some of the tests changed from "click the uuid link" to "click the Show button on the row with this uuid". While I was updating them, noticing find() errors out if the links don't exist, I figured the extra assert before find().click doesn't serve any purpose (unless it has a helpful error message).

  • I'm not wild about the cloud icon for virtual machines in the navigation sidebar. Virtual machines may be in the cloud, but they are not themselves the cloud. Is there a generic computer/workstation icon? A work desk might make sense, too, since the virtual machine is where people work on Crunch scripts.

Yeah, I'm not super keen on it either. I've changed it to the "terminal" icon, see how that goes. See http://fortawesome.github.io/Font-Awesome/icons/

Latest: 16c58b7

#25 Updated by Tom Clegg over 7 years ago

  • Status changed from New to In Progress

#26 Updated by Brett Smith over 7 years ago

  • Status changed from In Progress to New

On 16c58b75

Tom Clegg wrote:

Brett Smith wrote:

  • The first folder integration test has no assertion. Could you assert find('.panel').has_text?('I just edited this.')?

Fixed. (I had commented it out pending discovering the magic incantation to reconcile Capybara with x-editable. "visit current_path" isn't perfect but better than nothing.)

You say below that find errors out if it doesn't find what it's looking for. So won't this always either pass or raise an exception? Or does that only apply when you try to click the result?

  • Maybe related, I'm curious why you removed some of the intermediate assertions in the existing integration tests.

Some of the tests changed from "click the uuid link" to "click the Show button on the row with this uuid". While I was updating them, noticing find() errors out if the links don't exist, I figured the extra assert before find().click doesn't serve any purpose (unless it has a helpful error message).

+1 to helpful messages. My personal feeling is that an assert is a much stronger statement of intent than an uncaught exception. A failed assertion says, "I expected this behavior, and didn't get it." An exception leaves open questions to answer: is this bug in the test code, or in the application? And if it's in the application, is the expected behavior important, or not? So I like redundant assertions because I feel like they help others better interpret the results—e.g., many of my recent controller tests assert_response :success even though they go on to check things that could only be true after success.

But this is a pretty open-ended style thing, and I can definitely agree that the subject's a little more slippery when you're talking about relatively fine-grained navigation of HTML. That's my two cents, but either way, once the specific test issue above is settled I'm happy (psyched, even) to see this branch merged.

#27 Updated by Brett Smith over 7 years ago

  • Status changed from New to In Progress

#28 Updated by Tom Clegg over 7 years ago

Brett Smith wrote:

On 16c58b75

You say below that find errors out if it doesn't find what it's looking for. So won't this always either pass or raise an exception? Or does that only apply when you try to click the result?

That's true, the word "assert" doesn't actually do anything because failure results in an exception.

  • Tried wrapping in "assert_nothing_raised", but that somehow causes find() to always fail.
  • Tried using the "message" option to find(), but that feature has been broken/removed from Capybara.
  • Added a comment (in a848b6e) explaining that find() raises an exception if the tested behavior fails.
  • Added a find?() method (in 32ca1dc) to use with assert.
+    assert(find?('.panel', text: 'I just edited this.'),
+           "Description update did not survive page refresh")

Totally on board with improving our testing standards/patterns. In particular, valuable to make more clear in the tests which parts are "setting up the test" (rearrange as needed) and which parts are "behavior actually being tested" (when you touch this code, be careful that you are still testing this behavior!).

#29 Updated by Anonymous over 7 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 95 to 100

Applied in changeset arvados|commit:fc4428224984cb71b961d17410205b535153c7f2.

Also available in: Atom PDF