Bug #16118

Offers editing actions on read-only collections

Added by Peter Amstutz 4 months ago. Updated 2 days ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Target version:
Start date:
02/28/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

Workbench2 does not check if a user can edit a collection when rendering the collection view (
workbench2/src/views/collection-panel). As a result, if the user only has read-only access to the collection, they will experience API errors if they try to edit it. The collection view should check whether the collection is writable and have both editable and read-only views. You can check if the collection can be edited by examining the writable_by field in collection record, this is a list of uuids, if the collection is writable, the current user uuid will appear in that list, if not, the collection is read-only.


Subtasks

Task #16183: Review 16118-readonly-collections-lucasIn ProgressPeter Amstutz


Related issues

Related to Arvados - Feature #16437: Indicate when projects are not editable by userNew

Related to Arvados - Bug #16472: Shows error snackbar even when the code will handle the error.New

Blocks Arvados - Story #13494: View/copy/expunge previous versions of a collectionNew02/19/2020

Associated revisions

Revision ee499fb6
Added by Lucas Di Pentima 17 days ago

Merge branch '16118-arvboot-fixes'
Refs #16118

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

Revision 78fc0875
Added by Lucas Di Pentima 2 days ago

Merge branch '16118-readonly-collections-lucas'
Closes #16118

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <>

History

#1 Updated by Peter Amstutz 4 months ago

  • Target version changed from 2020-02-26 Sprint to 2020-03-11 Sprint

#2 Updated by Peter Amstutz 3 months ago

  • Target version changed from 2020-03-11 Sprint to 2020-02-26 Sprint
  • Assigned To set to Piotr Mrzygłowski
  • Description updated (diff)

#3 Updated by Peter Amstutz 3 months ago

  • Target version changed from 2020-02-26 Sprint to 2020-03-11 Sprint

#4 Updated by Lucas Di Pentima 3 months ago

  • Status changed from New to In Progress

#5 Updated by Lucas Di Pentima 3 months ago

Reviewing commit 08439235be955812555b80a31f1e6461c7d90513 - branch 16118-readonly-collections

  • Sorry, the ticket has a mistake, the collection objects don’t have a writable_by field, but users and project/group do. You can double check all of this on our API documentation: https://doc.arvados.org/v2.0/api/index.html
  • The workbench1’s code to check an object’s edit-ability is here: https://github.com/arvados/arvados/blob/master/apps/workbench/app/models/arvados_base.rb#L493-L501 -- let me know if it isn’t clear enough but basically a collection’s owner_uuid can point to a user or a project/group, so if a collection is inside a project, which in turn has a writable_by list including the current user's uuid, the collection is editable.
  • AFAICT, the update was about changing the context menu, but notice that the collection view also has a properties editor that shouldn’t be active, and the files inside the collection also shouldn’t be deletable or rename-able, as the collection’s content is the manifest_text field of it. Also, take into consideration that trashing/untrashing is an operation that requires an editable collection.

#6 Updated by Lucas Di Pentima 3 months ago

  • Blocks Story #13494: View/copy/expunge previous versions of a collection added

#7 Updated by Peter Amstutz 3 months ago

  • Target version changed from 2020-03-11 Sprint to 2020-03-25 Sprint

#8 Updated by Peter Amstutz 3 months ago

  • Assigned To deleted (Piotr Mrzygłowski)

#9 Updated by Peter Amstutz 2 months ago

  • Release set to 31
  • Target version deleted (2020-03-25 Sprint)

#10 Updated by Peter Amstutz about 2 months ago

  • Target version set to 2020-04-22

#11 Updated by Lucas Di Pentima about 2 months ago

  • Assigned To set to Lucas Di Pentima

#12 Updated by Lucas Di Pentima about 1 month ago

  • Target version changed from 2020-04-22 to 2020-05-06 Sprint

#13 Updated by Lucas Di Pentima 28 days ago

Updates at commit d8a3b5fd - branch 16118-readonly-collections-lucas
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/26/

Changes

  • Shows a lock icon indicating the read-only access.
  • The three-dotted 'More options' menu only shows appropriate actions.
  • The properties panel only shows properties without the 'delete tag' button.
  • The files panel general 'More options' menu shows appropriate actions.
  • The files panel individual context menu also filters editing action when read-only.
  • The files panel's upload button isn't rendered on read-only collections.

Pending

  • Integration tests - I figured that they'll depend on the UI so I would rather prefer for the changes to be reviewed first to avoid test re-writing.

#14 Updated by Lucas Di Pentima 23 days ago

  • Target version changed from 2020-05-06 Sprint to 2020-05-20 Sprint

#15 Updated by Lucas Di Pentima 18 days ago

Fixed a couple of thing at the backend side at 7e16d43dd - branch 16118-arvboot-fixes
Test run: https://ci.arvados.org/job/developer-run-tests/1843/
WB integration test retry: https://ci.arvados.org/job/developer-run-tests-apps-workbench-integration/1947/

  • Websocket & Keep-web URL configurations fixes on arvados boot.
  • Keep-web honors config when creating its arvados client pool, so now the TLS.Insecure flag set to true works when using it on arvados boot.

#16 Updated by Tom Clegg 18 days ago

@ 7e16d43dd

In source:lib/boot/supervisor.go, do these two really need to be different -- does something break if we add Path:"/" to the rest of the services' ExternalURLs, too? As it happens I've got this same change (in lots of other places too) in #16392.

Rest LGTM, thanks.

#17 Updated by Lucas Di Pentima 17 days ago

Tom Clegg wrote:

@ 7e16d43dd

In source:lib/boot/supervisor.go, do these two really need to be different -- does something break if we add Path:"/" to the rest of the services' ExternalURLs, too? As it happens I've got this same change (in lots of other places too) in #16392.

I guess not, I was just following what I saw on already working clusters. Updated at a20c1480b .
Test run: https://ci.arvados.org/job/developer-run-tests/1845/

#18 Updated by Tom Clegg 16 days ago

a20c1480b LGTM, thanks

#19 Updated by Lucas Di Pentima 11 days ago

Updates at arvados-workbench2|4bed7c3d
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/30/

  • Changes the padlock with a 'Read-only' label besides the collection name.
  • Integrates fixes related to changes on trailing slashes on URL handling and login methods.
  • Adds test for both readonly and writable collection UI.

Note: At arvados-workbench2|fc898afb there's deactivated code at src/index.tsx:L107 that was preventing to succeed the readonly test case because a snackbar is being displayed on every service request that get an error. This happens whether the error is catched or not up on the stack (see src/services/ancestors-service/ancestors-service.ts:L29 and src/services/common-service/common-service.ts:L83) and I think it isn't a good approach. What do you think? It's useful for the developer to have a visual cue of errors but not for the end user, can we just print a console message and no snackbar?

#20 Updated by Lucas Di Pentima 11 days ago

Although the integration tests failed on Jenkins, they don't on my side: even running them in docker. Confusing!

#21 Updated by Peter Amstutz 10 days ago

  • Related to Feature #16437: Indicate when projects are not editable by user added

#22 Updated by Peter Amstutz 10 days ago

Reviewing arvados-workbench2|4bed7c3d982b8d9198186a37a1edbdf746b1e0e2

I noticed that you can still access editing actions by right-clicking on the collection in the project view. However that is a bug for #16437 (read only projects)

In the test 'shows collection by URL'
  • When isWritable is true, it changes the ownership of the collection. Is there a reason to do that instead of have the admin always own it and choose between 'can_read' and 'can_write'?
  • I see when it is writable it submits to collection-properties-form, however when it is not writable there is no corresponding test that the form should be missing.

I don't really like the exclamation point in a circle next to "Read only" -- that icon is often used to mean "something is wrong that needs your attention" and in this case nothing is wrong. I am not sure what would be a better icon to use but I did a little research and found https://github.com/FortAwesome/Font-Awesome/issues/4906 which links to https://github.com/nyon/fontawesome-actions so there's a couple options, combining two icons, or adopting an additional icon set.

Dispatching snackbar from errorFn -- I had enabled it because otherwise many operations in wb2 will fail silently with no indication something went wrong. On the other hand, I think you mentioned that because there is no context it can also report errors for things that are not really errors (where wb2 is probing for something and expects it may fail.) Having this happen at the "services" layer was a quick fix until we could do a more systematic audit of error handling (#15581). I'm not really sure what to do for this branch, especially if you are getting spurious errors (that are not really errors) that interfere with testing.

Looking at https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/31/console

The stack trace doesn't seem to indicate the line of the test that is failing? Or is it there and I am just missing it?

From the error message it looks like it is probably failing on

cy.get('[data-cy=collection-info-panel]')

which happens right after trying to visit the page, so I guess it is failing to load the page or it is taking too long.

Need to look into this some more, not passing on jenkins is obviously a blocker.

#23 Updated by Lucas Di Pentima 9 days ago

  • Target version changed from 2020-05-20 Sprint to 2020-06-03 Sprint

#24 Updated by Lucas Di Pentima 3 days ago

Updates at arvados-workbench2|eac8deef
Test runs: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/36/

Updates:
  • Enhances tests: creates collections owned by a shared project to check for readonly UI changes. Also checks for properties form not being displayed on read-only collections.
  • Improves "read-only" icon by composing 2 different from font-awesome: a pencil & a slash.
  • Restores error snackbar display on service layer errors.
  • Changes the testing viewport to 1920x1080 so that snackbar spurious errors don't get in the way of buttons being tested.

I have been trying to make the optional parameter work but for some reason I not being able yet, committed WIP on another branch 16118-service-layer-snackbar-fix, as I don't want this to block the brach.

#25 Updated by Peter Amstutz 2 days ago

Lucas Di Pentima wrote:

Updates at arvados-workbench2|eac8deef
Test runs: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/36/

  • Improves "read-only" icon by composing 2 different from font-awesome: a pencil & a slash.

This looks much better.

I have one suggestion: instead of a Chip component with both the icon and words "Read-only" could we just have just the icon with "Read-only" as a tooltip?

Rest LGTM.

#26 Updated by Peter Amstutz 2 days ago

Lucas Di Pentima wrote:

I have been trying to make the optional parameter work but for some reason I not being able yet, committed WIP on another branch 16118-service-layer-snackbar-fix, as I don't want this to block the brach.

I did notice some spurious error snackbars on initial login/load and then when clicking on "shared with me" so it would be good to suppress those while still being able to have visibility on unexpected errors.

#27 Updated by Lucas Di Pentima 2 days ago

Updates at commit:d4efd52e
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/37/

  • Replaces the Chip with icon with just the icon + tooltip, as requested.
  • Updates test to look for the icon instead of the "Read-only" string.

#28 Updated by Lucas Di Pentima 2 days ago

  • Related to Bug #16472: Shows error snackbar even when the code will handle the error. added

#29 Updated by Anonymous 2 days ago

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

Also available in: Atom PDF