Arvados

• developer-tests-workbench2: #43
• developer-tests-workbench2: #44

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.

Reviewing arvados-workbench2|ea0b48a
Test run: 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.

New version: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/7cd56c2b3cb65089a6268d5a3de24466d1bb8a24
Test run: 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.

New version: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/0571951f6c77ec7b73eba240c117a05a50fe6ca0
Test run: 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.

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.

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.

New version: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/95e3ef0e518ae228bbb9179b816309ce6dc402cc
Test run: developer-tests-workbench2: #51

• Reverted handling share for all users