Arvados

22605-open-wf-dialog @ dac29ddce86496a44545957cb1cde74c4b8a4567
developer-run-tests-services-workbench2: #1461

• ✅ All agreed upon points are implemented / addressed.
• n/a Anything not implemented (discovered or discussed during work) has a follow-up story.
• ✅ Code is tested and passing, both automated and manual, what manual testing was done is described
• ✅ New or changed UX/UX and has gotten feedback from stakeholders.
• n/a Documentation has been updated.
• ✅ Behaves appropriately at the intended scale (describe intended scale).
• ✅ Considered backwards and forwards compatibility issues between client and server.
• ✅ Follows our coding standards and GUI style guidelines.

The original issue (iirc) was that a user could select a workflow, click "RUN WORKFLOW", see the target location field as blank (if they noticed it at all) and then start the run and have it appear in an unexpected location. The main reason for this is that there is always a default selected location, but that is not being communicated to the user. There was also the issue where under certain circumstances, the 'cancel' button would do the same thing as the 'ok' button. Now the project picker form always opens the first time the "Project where the process will run" input renders, the target project is always visible to the user, and the logic around the form submission is corrected.

Stephen may have implementation feedback but I have a couple of behavior notes:

• The dialog box just says "Choose a project" and not "Choose project where the workflow will run" as described in the requirements
• The default selected project should be selected in the tree (with the tree expanded as necessary); Stephen can provide more info but I know he had worked on this for another ticket.

I wasn't running the latest version of the branch, so hold on.

Revised notes, looking at the correct version (dac29ddce86496a44545957cb1cde74c4b8a4567) now

• The default selected project should be selected in the tree (with the tree expanded as necessary); Stephen can provide more info but I know he had worked on this for another ticket.
• This sequence is confusing:
  1. Run a workflow where the default project is not writable.
  2. When the dialog appears, it appears that the home project is selected. Click OK without interacting with anything.
  3. The "Run Workflow" button will remain disabled -- when it would have been enabled if you explicitly selected a project

Overall I didn't see any minor things that stuck out to me, so the main feedback I have is that you can accomplish pre-selecting / expanding the tree to the selected uuid by using currentUuids={[uuid]} when calling the ProjectsTreePicker, see DirectoryTreePickerField for an example - you shouldn't need to pass include* since this is just for projects. At risk of making some of the work redundant, I think the best approach would be to set the input value to the default value when the second stage of the form open - if I'm understanding the current implementation correctly, the input is handling both the default and input value at the same time, whereas if we load the default value into the input before anything happens then the input should be none the wiser and not need any special handling - also I'm pretty sure using currentUuids (which triggers the tree to automatically load and expand) should also handle loading the necessary resources along the way.

22605-open-wf-dialog @ 6f9e7bff40b0fc65e713dea3994a7bba1a49547a

developer-run-tests-services-workbench2: #1462

This sequence is confusing...

• Addressed by providing a fallback check to the 'valid' selector in RunProcessSecondStepForm. Basically, the 'owner' field of the redux form will always be populated when RunProcessInputsForm renders, so if we know the RunProcessInputsForm is rendering, we can ignore that bit of validation and not disable the Run Workflow button. When opening the project picker on a project where the user does not have write access, the selection will default to that user's root project.

Overall I didn't see any minor things that stuck out to me, so the main feedback I have is that you can accomplish pre-selecting / expanding the tree to the selected uuid by using currentUuids={[uuid]}...

• This worked just fine, but some adjustments had to be made so that the user's root project would expand.

At risk of making some of the work redundant, I think the best approach would be to set the input value to the default value when the second stage of the form open - if I'm understanding the current implementation correctly, the input is handling both the default and input value at the same time, whereas if we load the default value into the input before anything happens then the input should be none the wiser and not need any special handling

• This would mean determining what the default project is two component tree levels up from where it is actually being used. Since the default project needs to be determined within the ProjectInputComponent anyway, this would mean going through the work of determining the default project twice, all to save less than a line of code. Unless there is a compelling reason to change this, I think it's better to leave it as-is.

also I'm pretty sure using currentUuids (which triggers the tree to automatically load and expand) should also handle loading the necessary resources along the way.

• It doesn't load the user's root project if it is not already an ancestor of the selected project. If I attempt to run a wf from a project that was shared with me, for example, and I click "Home Projects" in the picker, it will not show up on the right side because it is not loaded.

This lgtm!

This is behaving better, but still not quite right.

The gist seems to be this:

(a) If you reload the app, then select a project that you have write access to and then go to launch a workflow using the NEW button, it correctly selects the project in the dialog.

(b) If you go to launch a workflow using the toolbar, where you can't write to the default directory, this also works as expected, selecting the root project

(c) However, if you again select a project that you have write access to and then go to launch a workflow using the NEW button, it will not select the project in the picker, but instead select the root project. Weirdly, the workflow's input form will show the correct project.

It feels like step (b) leaves some lingering state that messes up step (c).

I had mistakenly set the default project to reset when the parent project was unwriteable, not the current selected project. Also set the default owner to reset when the input itself unmounts, meaning no leftover state. It should be good now.

I have bad news.

The bug in note-18 does appear to be fixed.

But I am still seeing the first bug from note-16 (the behavior in https://dev.arvados.org/attachments/3806 is the same).

If you click run on the toolbar and you can write to the project, it is supposed to select that project. The picking tree isn't expanded and the selection behaves strangely.

22605-open-wf-dialog @ c32079da83fa71bb9e65005bc9599b01691e463c
developer-run-tests-services-workbench2: #1475

There were many hurdles that stood in the way of this ticket, and it took a significant rewrite of the data flow to get everything in working order, but it is now good. New tests are written and consistently pass.

• For all of the if cases in componentDidMount and componentDidUpdate of run-wf-project-input.tsx, it would be nice to have a short comment on when the case is triggered / what user interaction it catches just to make it a little easier to follow along what situations those are handling and why
• I saw that isUserResource also checks 'email' in resource, since ResourceKind.USER is only set on the User resource, it seems like it's not necessary to check for the email field unless the intention is to check that a value is there - in which case I think it would be better to check that outside of the function to avoid confusion if someone uses isUserResource somewhere else
• In projects-tree-picker I noticed you added await this.resetAllPickers() - since resetAllPickers is a void function, awaiting it probably doesn't do anything, and I believe since RESET_TREE_PICKER is not a thunk but goes directly to the reducer, I think it's synchronous and shouldn't need awaiting. (Reading about it here https://stackoverflow.com/a/75464347/733880)

22605-open-wf-dialog @ 20214f6d8b901bfed0fc0d3b88f498e86c7f88fa
developer-run-tests-services-workbench2: #1487

• For all of the if cases in componentDidMount and componentDidUpdate ...
• I saw that isUserResource also checks...

Done and done

• In projects-tree-picker I noticed you added @await...

This is necessary because of the way react batches state updates (https://react.dev/learn/queueing-a-series-of-state-updates). Basically, because the same piece of state is changing more than once in the same function, only the latest state change will go through. In this case, when the selected uuids array changes while rendering, it will cause the selected tree node to be triggered twice, and the tree will try to expand and highlight both nodes. By awaiting the reset, we ensure that only the latest selected uuids array is activated.

developer-run-tests-services-workbench2: #1491