Feature #16437

Indicate when projects are not editable by user

Added by Peter Amstutz over 1 year ago. Updated about 1 year ago.

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

100%

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

Description

The project panel must visually indicate when the currently displayed project is read only, and must not offer editing actions on project contents. The "New" button should also be disabled or otherwise indicate that a new item cannot be created in the current project.


Subtasks

Task #16483: Review 16437-indicate-when-projects-are-not-editable-by-userResolvedDaniel Kutyła


Related issues

Related to Arvados - Bug #16118: Offers editing actions on read-only collectionsResolved02/28/2020

Associated revisions

Revision 70a5f1c7
Added by Daniel Kutyła over 1 year ago

Merge branch '16437-indicate-when-projects-are-not-editable-by-user'
Closes #16437

Arvados-DCO-1.1-Signed-off-by: Daniel Kutyła <>

History

#1 Updated by Peter Amstutz over 1 year ago

  • Related to Bug #16118: Offers editing actions on read-only collections added

#2 Updated by Peter Amstutz over 1 year ago

  • Description updated (diff)

#3 Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2020-06-03 Sprint to 2020-06-17 Sprint

#4 Updated by Peter Amstutz over 1 year ago

  • Description updated (diff)
  • Subject changed from Indicate when projects are read-only by user to Indicate when projects are not editable by user

#5 Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2020-06-17 Sprint to 2020-06-03 Sprint

#6 Updated by Peter Amstutz over 1 year ago

  • Assigned To set to Daniel Kutyła

#7 Updated by Peter Amstutz over 1 year ago

  • Status changed from New to In Progress

#8 Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2020-06-03 Sprint to 2020-06-17 Sprint

#9 Updated by Lucas Di Pentima over 1 year ago

Reviewing arvados-workbench2|6828128
Failed test runs:

Some comments:

  • Commit’s DCO should be set to the real author name & email.
  • I tried to run the entire test suite on Jenkins twice and it failed with a timeout error when trying to run the end-to-end tests. I think this has to do with the HTTPS: false change on the .env file, as I also had issues with it when trying to run the branch against my own arvbox instance.
  • At src/views-components/context-menu/action-sets/project-action-set.ts lines 29-35 there’s commented code, feel free to remove it as we don’t keep those on our code base.
  • At src/views-components/context-menu/action-sets/project-action-set.test.ts
    • ’should not be empty’ tests: wouldn’t be best to check for length > 0 so that they don’t fail whenever we add a new action?
    • ’should not contain projectActionSet items’ test seems to be checking the opposite, and it doesn’t fail, why is that?
  • At src/store/resources/resources.ts - I realized we should also include the case of a resource id being from a user, as those objects also have the writable_by field — You can read about it at: https://doc.arvados.org/v2.0/api/methods/users.html (you may have to use uuids that are more similar to reality on the tests, like *-tpzed-* for users, and *-j7d0g-* for groups/projects.
  • On the projects view, when right-clicking on read-only collections, the actions should be restricted too. We forgot to add this requirement to the ticket but was mentioned on https://dev.arvados.org/issues/16118#note-22
  • If I right-click on a writable project immediately below the ’shared with me’ or ‘favorites’ sections on the left side panel, I only get the read-only actions.
  • If I access a once writable and now trashed project and right-click a subproject inside it, I get all the read-write actions. It probably should only offer a ‘restore’ action, as wb2 offers when right-clicking items at the Trash’s first level. Please confirm with Peter.
  • When viewing a read-only project, if I right-click on the project’s breadcrumb, I get a read-wite context menu. This may expose some unnecessary code duplication that can be avoided.

#10 Updated by Lucas Di Pentima over 1 year ago

Reviewing arvados-workbench2|ea0b48a
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/47/

  • You should be able to launch new runs by pasting your commit hash on https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/build?delay=0sec (if you don’t have access to that page, please ask Ward or Peter for proper access)
  • For reviewing purposes I think it’s better not to address review comments by overwriting the previous (already reviewed) commit. It forces the reviewer to read the entire commit again and again, making the process slower for both parties. It’s better to address review comments on new commits, IMO.
  • At file src/store/resources/resources.test.ts: To enhance test readability, do you think it would be a good idea to make uuids references via some properly named constants? It’s difficult to follow the test cases by just having to remember which uuid is readable, writable, etc. On our API server/controller tests we have fixtures with this kind of names and its really useful when the amount of tests using them gets big. Example: https://dev.arvados.org/projects/arvados/repository/revisions/master/entry/services/api/test/fixtures/groups.yml
  • At file src/store/resources/resources.ts, line 29 - I think you could rewrite return something ? something : otherThing as return someThing || otherThing for simplicity?
  • If I favorite an editable project and then go to the ‘My Favorites’ section at the left side panel, right-clicking on the editable project shows a limited action set.

#11 Updated by Daniel Kutyła over 1 year ago

New version: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/7cd56c2b3cb65089a6268d5a3de24466d1bb8a24
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/49/

This please check commits before this one as this one is just simple merge but it is the last one so I point it.
I have addressed all the comments (created fixtures, simplified return statement and added proper context menu handling for the favourites view). I guess there may be more similiar cases but we must agree on scope for this ticket as it should be only for the projects.

#12 Updated by Daniel Kutyła over 1 year ago

New version: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/0571951f6c77ec7b73eba240c117a05a50fe6ca0
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/49/

I have addressed all the comments (created fixtures, simplified return statement and added proper context menu handling for the favourites view). I guess there may be more similiar cases but we must agree on scope for this ticket as it should be only for the projects.

#13 Updated by Lucas Di Pentima over 1 year ago

Reviewing arvados-workbench2|7cd56c2

  • As we talked on chat, when you add a merge conflict on a commit, you could just drop the commit (or correct it with git reset HEAD^ if it has some other valid changes) instead of making a new one with the correction (arvados-workbench2|e4d1296 vs arvados-workbench2|7cd56c2), that way the history is cleaner and easier to follow/review.
  • Named fixtures make tests a lot easier to read, thank you! Now a small detail: I would rather use those identifiers as local constants instead of a shared file, unless we set to build an entire fixture suite with shared readable identifiers and resource data. For now I think just moving the identifiers to resources.test.ts would be enough.
  • Tests check for the editability propery of the returned resource, but not the type. Can we add that check for completeness sake?
  • Nit: we tend to avoid trailing whitespaces, I think you could configure your editor to auto-remove them (at least on emacs & vscode it’s doable). The following files have trailing whitespaces on this branch:
    • src/store/context-menu/context-menu-actions.test.ts
    • src/store/resources/resources.test.ts
    • src/store/resources/resources.ts
  • By accident I realized that we haven’t counted the edge case where a project is shared to all users. The “all users” group has the predefined uuid clusterId-j7d0g-fffffffffffffff, and will be listed on writable_by when given write access, so I think the easy fix would be to add the check on getResourceWithEditableStatus(). Please, also add some test case for it.

#14 Updated by Daniel Kutyła over 1 year ago

  • Assigned To changed from Daniel Kutyła to Lucas Di Pentima

New version: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/612b040a79d1338ea3935dd2fc57dc0908d6ee20
Test run: https://ci.arvados.org/view/Developer/job/developer-tests-workbench2/50/

  • Removed external fixture and placed it within the test file src/store/resources/resources.test.ts
  • Added new edge case when resource is shared with all users for a given cluster
  • Cleaned trailing spaces

#15 Updated by Daniel Kutyła over 1 year ago

  • Assigned To changed from Lucas Di Pentima to Daniel Kutyła

#16 Updated by Peter Amstutz over 1 year ago

  • Target version changed from 2020-06-17 Sprint to 2020-07-01 Sprint

#17 Updated by Lucas Di Pentima over 1 year ago

As talked on sprint kickoff, the check about "all users" group should be reverted (with its tests, sorry!). With that, this branch would look to merge.

#19 Updated by Daniel Kutyła over 1 year ago

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

#20 Updated by Peter Amstutz about 1 year ago

  • Release set to 25

Also available in: Atom PDF