Story #9587

[Workbench] Interface to list and untrash trashed collections

Added by Brett Smith over 1 year ago. Updated 6 months ago.

Status:ResolvedStart date:07/12/2016
Priority:NormalDue date:
Assignee:Radhika Chippada% Done:

100%

Category:-
Target version:2017-06-07 sprint
Story points2.0Remaining (hours)0.00 hour
Velocity based estimate0 days

Description

Use case: Provide a simple interface for users to review what collections are trashed, and undelete them if desired.

Implementation:

- Add a /trash index page that lists collections with is_trashed=true.

- Display columns: checkbox, Name (with a link to object name / uuid), Contents (similar to /collections page), Trashed at (when it was trashed at), Created at, Un-delete button

- Selection drop down has one item to "un-delete" the selected items in the checkbox

- Search box

- Infinite scrolling (rather than paging - desirable)

trash.jpg (40.3 KB) Radhika Chippada, 06/01/2017 06:08 pm

duplicated trashed items.png (83.3 KB) Lucas Di Pentima, 06/02/2017 06:45 pm

trash_title_on_trash_page.png (67.9 KB) Lucas Di Pentima, 06/05/2017 06:23 pm


Subtasks

Task #11564: Review branch 9587-include-trash-in-group-contentsResolvedLucas Di Pentima

Task #11750: Rollback API server updates from 9587-include-trash-in-gr...ClosedRadhika Chippada

Task #11749: Review branch 9587-trash-pageResolvedRadhika Chippada


Related issues

Related to Arvados - Story #9278: [Crunch2] Document/fix handling of collections with non-n... In Progress 06/01/2016
Related to Arvados - Bug #11822: [API] Add recursive and include_trash params missing from... Duplicate
Related to Arvados - Bug #11821: [API Server] Update discovery document to include latest ... Resolved 06/07/2017

Associated revisions

Revision c182087b
Added by Radhika Chippada 6 months ago

refs #9587
Merge branch '9587-include-trash-in-group-contents'

Revision 55315b66
Added by Radhika Chippada 6 months ago

closes #9587
Merge branch '9587-trash-page'

Revision 87ea4388
Added by Radhika Chippada 6 months ago

closes #9587
Merge branch '9587-trash-page'

Revision 1595481c
Added by Radhika Chippada 5 months ago

refs #9587
Merge branch '9587-trash-page'

History

#1 Updated by Tom Morris about 1 year ago

  • Assignee changed from Brett Smith to Tom Morris

#2 Updated by Tom Morris 7 months ago

  • Assignee deleted (Tom Morris)
  • Target version set to 2017-05-10 sprint

#3 Updated by Tom Morris 7 months ago

  • Subject changed from [Workbench] Interface to list and undelete expiring collections to [Workbench] Interface to list and untrash trashed collections
  • Description updated (diff)

#4 Updated by Tom Morris 7 months ago

  • Story points set to 2.0

#5 Updated by Lucas Di Pentima 7 months ago

  • Assignee set to Lucas Di Pentima

#6 Updated by Lucas Di Pentima 7 months ago

  • Status changed from New to In Progress

#7 Updated by Lucas Di Pentima 6 months ago

Branch 9587-wb-trash-tab pushed so Radhika can continue from there.
The parameter include_trash is on the collection's controller, so I was going to see if it should be added to the projects controller too, when I had to switch focus to other stories.

#8 Updated by Lucas Di Pentima 6 months ago

  • Assignee changed from Lucas Di Pentima to Radhika Chippada

#9 Updated by Radhika Chippada 6 months ago

  • Target version changed from 2017-05-10 sprint to 2017-05-24 sprint

#10 Updated by Radhika Chippada 6 months ago

Branch 9587-include-trash-in-group-contents @ 461b3f5a2edb53adda4f3b703d77e9efc0c262e9

Added support for include_trash param in groups_controller -> contents method. This is API server update only.

Test run @ https://ci.curoverse.com/job/developer-run-tests/291/

#11 Updated by Lucas Di Pentima 6 months ago

Branch 9587-include-trash-in-group-contents LGTM, thanks.

#12 Updated by Peter Amstutz 6 months ago

is_trashed is set once trash_at is in the past. In the case of temporary files, trash_at is in the future.

I think we want workbench treat files with trash_at in the future similarly to the trashed files (trashed_at in the past).

If we show temporary files (trash_at in the future) as "normal" files, then (a) it defeats the goal of decluttering the workbench listing and (b) means they can't actually be undeleted until trash_at passes.

So, instead of looking for is_trashed=true, Workbench should filter for "trash_at != nil" in the Trash tab and "trash_at == nil" in the regular Collection tab.

#13 Updated by Tom Morris 6 months ago

This is conflating two different things - 1) something which has been deleted/trashed and 2) something which is expiring and will be deleted/trashed in the future.

These aren't the same thing and they mean different things to the user. We need to stop thinking about this from the point of view of technical limitations cause by our particular token expiration strategy and look at it from the point of view of the user.

The story as written was fine. If we want to also implement a filtering strategy for things which will be expiring at some point in the future we can do that, but it isn't the same thing.

#14 Updated by Peter Amstutz 6 months ago

Should the Trash tab show collections with "trash_at" in the future? Because if not, there won't be any way in the UI to stop a collection from being trashed in the future.

#15 Updated by Peter Amstutz 6 months ago

(The description, as written, does not define precisely the criteria for a collection to appear under the "Trash" tab)

#16 Updated by Radhika Chippada 6 months ago

TomM and Peter, the more I look into it, the more this UI design is not making sense to me. Are we sure what we want is a project#Trash tab? It feels like an index page /trashed_collections (or something like that) would be lot more useful for the users.

This project#Trash tab is not making sense because if a user wants to look at trashed collections and untrash, he needs to go into each project and look for these collections. Adding another tab that is not very useful seems like a major wasted investment in a wrong place than doing the more useful /trashed_collections page. I think a /trashed_collections page and a link to it in the "Recent collections" panel on Dashboard might be a better UI design.

#17 Updated by Peter Amstutz 6 months ago

I had a similar thought. Once an object is trashed (not just future trash) it's going to be very hard to find in order to recover it, because the default behavior is to deny that it exists.

One thought I had is if you know the uuid and request the show page in workbench, it could request the record with include_trashed=true and present you with a "do you want to untrash this" page.

#18 Updated by Radhika Chippada 6 months ago

  • Description updated (diff)

#19 Updated by Radhika Chippada 6 months ago

  • Target version changed from 2017-05-24 sprint to 2017-06-07 sprint

#20 Updated by Radhika Chippada 6 months ago

  • Description updated (diff)

#21 Updated by Radhika Chippada 6 months ago

Branch 9587-trash-page @ 6c40c1c5b000b2eb0967df0f7ebaf613393501bc

  • Added "Trash" to topnav (right justified to separate from the other Dashboard options)
  • Trash page shows collections with is_trashed = true with Un-trash button with recycle icon and checkbox with Selection dropdown option to Un-trash
  • Required to expose untrash api on server
  • Since we do not want the user to be able to "view" the collection itself, the collection name is displayed as text than a link (with TomM's approval)
  • The trash page lists files in the collection as "contents"
  • Added search box that searches in trashed collections only
  • Added infinite scrolling
  • For the time being leaving in the updates from branch "9587-include-trash-in-group-contents", in case we decide to use this in some manner in future

Test run @ https://ci.curoverse.com/job/developer-run-tests/308/

#22 Updated by Radhika Chippada 6 months ago

  • File trash.jpg added

Tom Morris:

  • Do we want to include a link to "owner_uuid" (project) in the table columns in the trash page?

#23 Updated by Radhika Chippada 6 months ago

  • File deleted (trash.jpg)

#24 Updated by Radhika Chippada 6 months ago

#26 Updated by Lucas Di Pentima 6 months ago

Some comments & questions:

  • When the user is on /trash, how about having a title with the word “Trash” just like when the user is inside a project?
  • Maybe it would be a good idea to also have API Server tests checking for the new endpoints.
  • The order on which the collections are listed on the trash page is by its create_at attribute, do you think it would be better if we order it by trash_at or modified_at so that the latest trashed collections appear first?
  • I’ve been playing with it on arvbox and seen some weird behavior when trashing, then untrashing and then trashing again some collections, it repeats some collections on the trash page, see attached screenshot.
  • The recycle button could have a tooltip indicating its function
  • We’re showing only 3 files from every trashed collection manifest, isn’t it too expensive to ask for the manifests just to show so little? I’m thinking about production clusters with lots of intermediary trashed collections, the amount of data the api server will have to send when scrolling, I don’t know if it’s worth it.
  • If the previous point is OK the way it is, how about taking advantage of having the manifest to show more data about the collection, for example file count and total size so that the user has more information?

#27 Updated by Lucas Di Pentima 6 months ago

Adding missing screenshot.

#28 Updated by Radhika Chippada 6 months ago

When the user is on /trash, how about having a title with the word “Trash” just like when the user is inside a project?

Updated the display elements to display in "Recent" tab pane

Maybe it would be a good idea to also have API Server tests checking for the new endpoints.

Added controller tests

The order on which the collections are listed on the trash page is by its create_at attribute, do you think it would be better if we order it by trash_at or modified_at so that the latest trashed collections appear first?

I think sorting on created_at would be more meaningful and less confusing, but that is just my opinion. In any case, this is more straightforward implementation without needing extra changes to application_controller->next_page_filters method. Leaving this as is.

I’ve been playing with it on arvbox and seen some weird behavior when trashing, then untrashing and then trashing again some collections, it repeats some collections on the trash page, see attached screenshot.

I made an update around the "last_uuids" and hopefully this is better now

The recycle button could have a tooltip indicating its function

Added

We’re showing only 3 files from every trashed collection manifest, isn’t it too expensive to ask for the manifests just to show so little? I’m thinking about production clusters with lots of intermediary trashed collections, the amount of data the api server will have to send when scrolling, I don’t know if it’s worth it.

Discussed this with TomM as well and we agreed to leave it for now. We can remove this column in future or reduce @limit if need be in the future

If the previous point is OK the way it is, how about taking advantage of having the manifest to show more data about the collection, for example file count and total size so that the user has more information?

Replaced the ellipse display to more useful info saying there are so many more files

#29 Updated by Lucas Di Pentima 6 months ago

About the trash page title: I think using the word "Recent" is confusing because in the context of a trash page, it seems to me that "recent" would be referring to "recently trashed items", and this is not the case, because the listing is ordered by creation time.

I was thinking about something along the lines a simple title like in the attached screenshot, to make it clear to someone returning to this browser tab, that is not looking at a normal collection listing, just a personal opinion.

The rest LGTM. Thanks!

#30 Updated by Radhika Chippada 6 months ago

About the trash page title: I think using the word "Recent" is confusing ...

All our pages are sorted on CreatedAt time and use the title (mostly) "Recent". So, I think it probably will not be confusing to the users. I am going to leave it as is for now and we can consider renaming it if there is such a feedback by others. Thanks.

#31 Updated by Radhika Chippada 6 months ago

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

Applied in changeset arvados|commit:55315b668b8fa04572a44fd7db6598478f54130a.

#32 Updated by Radhika Chippada 6 months ago

  • Status changed from Resolved to In Progress

Branch 9587-trash-page @ f9ff1c5444f7030fa0b372446bd40d4feed203c8

Lucas: your earlier comment "The order on which the collections are listed on the trash page is by its create_at attribute, do you think it would be better if we order it by trash_at or modified_at so that the latest trashed collections appear first" and the comment about the title of the page, made me revisit this issue. In fact, I went back and saw the original description (where we were contemplating a project tab for trash) and that also said that we want to sort the rows by trash_at time.

Hence, I made one more update to this effect. The rows are now sorted by the "trash_at" time. In addition, I rearranged the columns to display the "Trashed at" column before the "Created at" column, because that makes more sense.

Please take another look of this particular update. Thanks.

#33 Updated by Lucas Di Pentima 6 months ago

Yes, it seems to me this way is more useful, just like a trash can! we remember the last things we throw there, the oldest one are less probable to be needed back :)

About the "Recent" tab, now it's not confusing in the sense of its meaning, but I keep my opinion that the user is not having any context as to where in the workbench this page is placed. For example, when the trash was thought as a "project tab", the project title was the context and to me at least, is enough information for the user to know where she/he is standing.

The way it is now, the user has to read carefully the listing to know that it's placed on the trash page, because the only clue there is to let the user know it's on the trash page, is the URL (I think the page title has lost its purpose now that we use to have tens of open tabs on the same browser window), so if you're revisiting that topic too, I have two suggestions:

  • Add a "Trash" title
  • Make the "Trash" button have a different background color to let the user know it is "pressed" to assume the page that is being shown is Trash

Sorry for being insistent about this, I just wanted to be clear about my concern about this topic.

The current updates look good, thanks!!

#34 Updated by Radhika Chippada 6 months ago

Renamed the pane name from "Recent" to "Recent trash" (even though I do not think it is necessary because the user got here by clicking on the Trash icon in the topnav) ):

Made the Trash icon slightly bigger (since the Dashboard icon is bigger and hence there is extra room). Didn't make any changes to pressed versus not, since we do not do that anywhere else.

Thanks.

#35 Updated by Radhika Chippada 6 months ago

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

Applied in changeset arvados|commit:87ea4388edb977d246b09b78d9d4bfa5c2ba5170.

Also available in: Atom PDF