Introduce Storage Connections management dialog

[mkemel]

Storage Connections management dialog is opened by clicking the
Connections button in the Storage Domains table when iSCSI Storage
Domain is selected.

The dialog displays storage connections, attached to the domain, and
also has a switch to show all the iSCSI connections. It allows 5
actions to be performed on the connections:

1. Add new connection
2. Edit connection
3. Remove connection
4. Attach connection to the domain
5. Detach connection from the domain
   Which provides UI for the existing storage connections API
   Live changes are not supported, thus the domain should be in
   maintenance mode to perform actions (4),(5), and also (2),(3) for
   attached connections

Bug: https://bugzilla.redhat.com/show_bug.cgi?id=977379





.

[ljelinkova]

Please attach some screenshots of the dialog.

[mkemel]

Please attach some screenshots of the dialog.

Done 👍

[sjd78]

At least the state mismatch between the modal and modal body needs to be resolved.

I would prefer the table to follow the newer model, but it isn't a show stopper at this point.

The fetchData cross filtering is a bit awkward. Does the selectedDoamin in addManageStorageConnectionsButton() have reference to the relevant data center id already? If yes, pass that in and skip some of the lookups.

Finally, the callback structure / location needs a final concensus (@rszwajko, WDYT?).

[mkemel]

@sjd78 @rszwajko

1. Regarding moving to Table Composable - I have started to rewrite it and I understood that it would take me more time than I thought, especially for proofing the whole thing and making sure all the small logics are still working. So maybe leave it with the regular Table
2. Got rid of all the lets in fetchData. Also, added dataCenterId to the EntityObject, sparing one fetch out of 4
3. @sjd78 setShowAll - I need it in StorageConnectionsModal to trigger it after successful Create action - new connections are naturally created unattached, and if only attached connections are shown we will not see the newly created connection. But I added useEffect to StorageConnectionsModalBody to sync the two states.
4. Callbacks, so we agree to leave them in StorageConnectionsModal? Then error and isShowAll are staying there?

[sjd78]

The changes a look good. A few more comment to address, but nothing I'd consider major.

To reply to your comment:

1. Regarding moving to Table Composable - I have started to rewrite it and I understood that it would take me more time than I thought, especially for proofing the whole thing and making sure all the small logics are still working. So maybe leave it with the regular Table

Given the time constraints, it's ok to keep the table component the way it is.

2. Got rid of all the lets in fetchData. Also, added dataCenterId to the EntityObject, sparing one fetch out of 4

The fetch code is looking much better. Good work!

3. @sjd78 setShowAll - I need it in StorageConnectionsModal to trigger it after successful Create action - new connections are naturally created unattached, and if only attached connections are shown we will not see the newly created connection. But I added useEffect to StorageConnectionsModalBody to sync the two states.

Something with how those interplay is bugging me as being overly complex. I'm not sure how to unwind it so it only need to be in one place. But...since it seems to work, it is not a blocker.

4. Callbacks, so we agree to leave them in StorageConnectionsModal? Then error and isShowAll are staying there?

Callbacks are ok where they are. In ovirt-web-ui, they would be redux calls, do stuff async and then push the results to the redux store and flow back down to the components to render. There are ways to do this without redux. A complex context object, or managing more state in the DataProvider making that the controlling component are two possibilities. Trade-offs as they are, the current way doesn't seem and better, or worse.

Same as with isShowAll and error.

In general, keeping state in one component and having the rest be controlled component makes things "simpler". The dependencies across components on state and effects I think gets the behavior you need at the cost of complexity. It seems to work, so it isn't a blocker for me.

[mkemel]

@sjd78 @rszwajko @ahadas
I have posted changes that Scott proposed, except for the Spinner one, where it broke the behavior - table is not refreshed correctly. To change it I guess I would need to do some more work, and I have already stretched the time I could do stuff here on Friday :)

Also, now I see Radek also has some proposals that might be a good thing to implement.
I'm not sure when do we need it to be merged. I can continue working on this on Sunday.

[rszwajko]

@mkemel
TLDR I don't have a rock solid bug scenario that would block this PR so I'm OK with adding improvements later on.

Note I couldn't fully click through the dialog because it doesn't support FAKE_DATA and I don't have iscsi hardware. However, I managed to run it with my NFS domain after few hacks :) The user experience was mixed i.e. consider the screenshot below

1. I could edit multiple connections (what will happen on referesh?)
2. it's possible to edit and non-editable row and then save it (failed on undefined in validators)
3. "show all' looks like a kind of filter that gets auto-enabled after "create" - more on that in this comment
   

[sgratch]

@mkemel the ui-extensions build won't start before Tuesday, so you can continue working on that on Sunday.

[mkemel]

@rszwajko

1. On refresh non-saved commit gets discarded. I didn't find how to disable the edit button back then. I guess if I would use TableComposable it would not be a problem :)
2. Saving a non-editable row (just like saving without editing) just cancels the edit. I don't get any errors there
3. Yes, that was the intent. I wanted a dialog that shows mainly existing SD connections, as far as I understand, 'Edit' is the main scenario there. But for some actions we need to see also other connections. I'm not a UX person, but I thought that double pane would make the dialog over complicated, when most of the time the user would like just to edit the connection.

Please review my latest changes, there were some in DataProvider and other places. I think it looks better now

[rszwajko]

showAll features needs some changes. Beside that OK for me.

[mkemel]

@sjd78 @sgratch Pls don't forget to squash on merge :)

[sjd78]

PR looks reasonable at this point. Can be merged once OST passes.

[sjd78]

/ost

[sjd78]

/ost

Original OST passed https://github.com/oVirt/ovirt-engine-ui-extensions/actions/runs/2053199027

The only changes since then are a merge conflict on src/constants.js and a minor fix on src/utils/fetch.js.

[sjd78]

/ost

[sjd78]

/ost