Arvados

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.

Updates at commit d8a3b5fd - branch 16118-readonly-collections-lucas
Test run: 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.

Fixed a couple of thing at the backend side at 7e16d43dd - branch 16118-arvboot-fixes
Test run: developer-run-tests: #1843
WB integration test retry: 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.

@ 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.

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: developer-run-tests: #1845

a20c1480b LGTM, thanks

Updates at arvados-workbench2|4bed7c3d
Test run: 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?

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

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)

• 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 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.

Updates at arvados-workbench2|eac8deef
Test runs: developer-tests-workbench2: #36

• 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.

Lucas Di Pentima wrote:

Updates at arvados-workbench2|eac8deef
Test runs: 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.

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.

Updates at d4efd52e
Test run: 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.