Bug #17306

Favorites in copy dialog is different to favorite list

Added by Daniel Kutyła 9 months ago. Updated 8 months ago.

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

100%

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

Description

Adding a project to the favorite list will show it afterwards in the workbench2 list as expected. When opening a "copy into existing collection" window from the data collection view however the project is not shown under favorite list. This is also the case for any subproject or collection within this project.
When comparing the number of projects/collectdions in the favorite list one sees a difference to the number of projects/collections shown in the copy dialog


Subtasks

Task #17317: Review 17306-Favorites-in-copy-dialog-is-different-to-favorite-listClosedDaniel Kutyła


Related issues

Related to Arvados - Bug #17396: Favorites copy dialog further issuesNew

Related to Arvados Workbench 2 - Bug #17400: Directly shared collections are displayed as "read-only"New

Related to Arvados - Bug #17436: Favorites in workflow picker dialog is different to favorite listResolved03/03/2021

Associated revisions

Revision 40d96a9d
Added by Daniel Kutyła 8 months ago

Merge branch '17306-Favorites-in-copy-dialog-is-different-to-favorite-list'
closes #17306

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

History

#3 Updated by Peter Amstutz 9 months ago

  • Status changed from New to In Progress

#4 Updated by Peter Amstutz 9 months ago

  • Release set to 37

#5 Updated by Lucas Di Pentima 8 months ago

  • Cannot reproduce the reported issue as it's described on the ticket.
  • Please write some integration tests exposing the problem so that we're sure it's fixed.

#6 Updated by Peter Amstutz 8 months ago

  • Target version changed from 2021-02-03 Sprint to 2021-02-17 sprint

#8 Updated by Lucas Di Pentima 8 months ago

Some comments/suggestions:

  • At file cypress/integration/favorites.spec.js:
    • Line 54: There's a only() call that avoids other test cases to be run.
    • Line 66 & 85: There's a wait() call that is usually a sign of brittle testing. Instead, could you instead add an assertion/guard for the test framework to wait on before continuing?
    • Lines 97-141: Could we write that part of the test without so many indentation? I remember hearing you said the new Cypress has features to do that. I think it would be good for readability/maintainability.
    • Line 125: There're trailing whitespaces that need removing (you could probably configure your editor to automatically do these kind of things).
  • At file src/store/tree-picker/tree-picker-actions.ts
    • Lines 257-263: I believe the loadFavoritesProject() action is used by several different parts of the app, sometimes needing to show all favorites. I think the fact that only writable favorites be listed should be a parameter of the action, with the default being the previous behavior so we don't add bugs to pre-existing code.
    • Related to the above, please check there're tests making sure the favorite picker works correctly on both modes: "writable only" and "all favorites".

#9 Updated by Lucas Di Pentima 8 months ago

Regarding my comment about loadFavoritesProject(), I think the discovered underlying bug also avoids actions like allowing the user to select a workflow input collection that's inside a read-only favorited project from another user. That's why I think we should be adding the possibility to display "all projects" in addition to "writable projects" (and adjusting the different client code of that action to show the proper listing in each case).

I've skimmed the code a bit and couldn't find how the left side panel's "My Favorites" tree picker is populated. It's obviously not using the loadFavoritesProject() and I think it would be really great that we re-use that code for everything that needs a favorites tree picker, so that we get an unified behavior across de entire app.

Regarding my 'individual favorited collections' comment on chat, please dismiss it, as I've re-read the code (and tested manually) and those collections are being included correctly.

#11 Updated by Lucas Di Pentima 8 months ago

Danny: This bug report is unveiling more related issues, here're my comments:

  • At file cypress/integration/favorites.spec.js
    • Line 131: The test is relying on the fact that "Favorites" is listed last on the chooser dialog, could this be done in a more ordering-independent way? Like adding some data-cy attribute specifically named after every category?
    • Line 134: This also seems to be relying on ordering, could it be just checking that the form-dialog doesn't contain the read-only shared project name?
    • The test is about moving a collection into another project, but only gets up to the point of listing favorited (writable) shared projects on the picker. Could you extend it to do the moving and checking that it was successful?
    • Could you rename the mySharedProjectN vars with more meaningful names? One is read-only, the other writable, I think that fact should be on the names so that reading the test is easier.
  • This ticket's description talks about the operation of selecting a file inside a collection and using the "copy selected into collection" option, could you add a complete test for that? I've tried it manually and I'm seeing two problems:
    • When having a favorited read-only collection, it's being listed on the picker and obviously returns an error when trying to copy something into it.
    • When trying to copy something into a writable collection I'm getting the error: "422 Unprocessable Entity: Manifest text Invalid manifest: does not end with newline" (haven't checked what's wb2 trying to do to get this message)
  • The ProjectsTreePicker now has the same issue that loadFavoritesProject() had on my last review: that is hard-coding configuration and what I think we want is to be able to set it up differently depending on the place it's being used. A quick lookup at the code shows me that ProjectsTreePicker is being used on:
    • src/views-components/projects-tree-picker/tree-picker-field.tsx
    • src/views/run-process-panel/inputs/directory-array-input.tsx
    • src/views/run-process-panel/inputs/directory-input.tsx
    • src/views/run-process-panel/inputs/file-array-input.tsx
    • src/views/run-process-panel/inputs/file-input.tsx
  • ...could you investigate the use cases on every one of the above files and see in which mode the ProjectsTreePicker should be used? The src/views/run-process-panel/inputs/* files probably need to be able to show all objects, because I suppose they're workflow inputs selectors.
  • Also I saw that ProjectsTreePicker includes SharedTreePicker and PublicFavoritesTreePicker both of which I believe they should be configurable to list "writable" or "all" objects, do you agree?

#12 Updated by Daniel Kutyła 8 months ago

New version first commit: https://dev.arvados.org/projects/arvados-workbench-2/repository/revisions/4860e74d347552476c77e313c85502787b6d7dfe
Test run: https://ci.arvados.org/job/developer-tests-workbench2/285/

Fixed copying collection elements to another collection, this should work although it still contains some flaws
- collection with many elements that only few are copied to another collection can perform very slow (due to required removal of non selected files)
- copy of nested elements is not possible due to parent being not selected when at least one of the children is not selected

#13 Updated by Lucas Di Pentima 8 months ago

  • Minor issue: typo on data-cy attribute at src/views-components/projects-tree-picker/projects-tree-picker.tsx Line 37 (and tests)
  • Integration tests failed on Jenkins:
    • The collections panel test failure was already fixed on current master, so I think you can dismiss it
    • The favorites test (cypress/integration/favorites.spec.js line 175) failure also happens on my local environment. Also I believe you could remove the loginAs() call from line 163 as it's already logged in as active user on line 146.
  • I manually tried to copy some file to a collection inside favorited shared (with write access) project from other user and it's not listed inside the "Favorites" picker. Can you also write an integration test for that case?
  • The same happens when I try to copy a selected file to a favorited shared (writable) collection, the target collection isn't listed.
  • Related to both above bulletpoints: the "Shared with me" tree picker does show both writable and read-only objects on a chooser that should just show writable objects because what the user is trying to do is write a file on some target.
    • I think this should be fixed as it's related to the "showOnlyWritable" flag that was added on this branch.
    • Please also add tests for this.

#14 Updated by Lucas Di Pentima 8 months ago

  • Related to Bug #17396: Favorites copy dialog further issues added

#15 Updated by Lucas Di Pentima 8 months ago

  • Related to Bug #17400: Directly shared collections are displayed as "read-only" added

#16 Updated by Peter Amstutz 8 months ago

  • Target version changed from 2021-02-17 sprint to 2021-03-03 sprint

#18 Updated by Lucas Di Pentima 8 months ago

This LGTM, thanks!

#19 Updated by Daniel Kutyła 8 months ago

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

#20 Updated by Peter Amstutz 8 months ago

  • Related to Bug #17436: Favorites in workflow picker dialog is different to favorite list added

Also available in: Atom PDF