Add feature for changing console button tooltip to "Console in use" #106

bond95 · 2017-03-17T10:19:27Z

Fixes: #66

lago-bot · 2017-03-17T10:20:15Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

lago-bot · 2017-03-17T10:20:16Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

lago-bot · 2017-03-17T10:20:18Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

lago-bot · 2017-03-17T10:20:21Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

lago-bot · 2017-03-17T10:20:23Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

lago-bot · 2017-03-17T10:20:25Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

lago-bot · 2017-03-17T10:20:27Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

lago-bot · 2017-03-17T10:20:29Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

ovirt-infra · 2017-03-20T15:03:33Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

ovirt-infra · 2017-03-20T15:03:37Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

ovirt-infra · 2017-03-20T15:03:40Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

ovirt-infra · 2017-03-20T15:03:43Z

The commiter is not a member of organization nor whitelist. Please contact maintainer for patch verification.

jelkosz · 2017-03-21T09:28:21Z

src/sagas.js

@@ -138,6 +139,7 @@ function* fetchAllVms (action) {
    yield put(removeMissingVms({ vmIdsToPreserve }))

    yield put(updateVms({ vms: internalVms, copySubResources: true }))
+    yield fetchVmsSessions({ vms: internalVms })


This is an additional query per each VM on every refresh cycle which is too high price for too small gain I'd say.
I think we should do it a bit differently. Maybe something like this:

do this if "shallowFetch == false" and than showing a msg in the VM details

also, do this as a part of the attempt to connect to the console. So, the flow would be like this:

click connect to console button

first check the active sessions, if there is some active session, warn the user that there is an active session and ask if he wants to continue anyway

continue with the taking the console normally in case the user confirms

@mareklibra what you think?

Right, it's to costly since it's a per-VM call.

I agree with @jelkosz , let's try usability when triggered within console retrieval.

landgraf · 2017-03-23T10:19:01Z

ci test please

mareklibra · 2017-03-29T10:49:48Z

Rebase is needed because of #109

mareklibra · 2017-04-06T13:28:31Z

src/components/VmActions/index.js

+    this.setState({
+      openModal: true,
+    })
+    this.props.checkConsole()


change to onConsoleSessionConfirmaClose()

mareklibra · 2017-04-06T13:29:46Z

src/components/VmActions/index.js

  }),
  (dispatch, { vm }) => ({
+    checkConsole: () => dispatch(checkConsoleInUse({ vmId: vm.get('id') })),


Please use more self-descriptive name, instead of checkConsole use i.e. onCheckConsoleSessionInUse

mareklibra · 2017-04-06T13:30:36Z

src/components/VmActions/index.js

    onGetConsole: () => dispatch(getConsole({ vmId: vm.get('id') })),
+    modalClose: () => dispatch(setConsoleInUse({ vmId: vm.get('id'), consoleInUse: false })),


Similar: modalClose --> onConsoleSessionConfirmationClosed

mareklibra · 2017-04-06T13:32:44Z

src/components/VmActions/index.js

  }),
  (dispatch, { vm }) => ({
+    checkConsole: () => dispatch(checkConsoleInUse({ vmId: vm.get('id') })),
    onGetConsole: () => dispatch(getConsole({ vmId: vm.get('id') })),


With this change the overall "Get Console" flow is becoming hard to understand.
To help, please refactor throughout the application getConsole() ---> downloadConsole() and change the action type string accordingly.

mareklibra · 2017-04-06T13:33:48Z

src/components/VmActions/sagas.js

+    sessionsInternal.find((x) => x.consoleUser) !== undefined) {
+    yield put(setConsoleInUse({ vmId, consoleInUse: true }))
+  } else {
+    yield put(getConsole({ vmId }))


after recatoring: downloadConsole() (see different comment)

mareklibra · 2017-04-06T13:39:33Z

src/sagas.js

@@ -295,6 +321,12 @@ function* schedulerPerMinute (action) {
  }
 }

+let sagasFunctions = {


Please add comment describing purpose of this wrapping object.

Something like:
Saga functions to be called from subsagas .

Or better even more descriptive.

mareklibra · 2017-04-06T13:39:52Z

src/sagas.js

@@ -308,10 +340,12 @@ export function *rootSaga () {
    takeEvery('START_VM', startVm),
    takeEvery('GET_CONSOLE_VM', getConsoleVm),


refactor: GET_CONSOLE_VM --> DOWNLOAD_CONSOLE_VM

mareklibra · 2017-04-06T13:40:49Z

src/sagas.js

@@ -308,10 +340,12 @@ export function *rootSaga () {
    takeEvery('START_VM', startVm),
    takeEvery('GET_CONSOLE_VM', getConsoleVm),
    takeEvery('SUSPEND_VM', suspendVm),
+    takeEvery('GET_SESSIONS_VM', getSessionsVm),


Is this dispatched from somewhere? Please review whole flow.

mareklibra · 2017-04-06T13:43:02Z

src/sagasBuilder.js

@@ -0,0 +1,5 @@
+import { buildSagas as VmActionsSagas } from './components/VmActions/sagas'
+
+export default (sagas) => [


This is becoming one of the key parts of the overall architecture.
Please add comment describing it's purpose, try to be as much descriptive as possible.
Try to describe the general idea behind this composition and what is expected to be done for future additions.

It will help to keep it's use consistently and make it more understandable.

mareklibra · 2017-04-06T13:49:08Z

src/components/VmActions/index.js

+    }
+
+    if (vm.get('sessions') &&
+      !(vm.get('sessions').filter(c => c.get('consoleUser')).isEmpty())) {


With this patch, the redux store now contains two related properties:

consoleUser

consoleInUse

Both filled in within different flows but used for more or less similar use case. Can they be merged into one and related flows changed accordingly? This logical duplicity might cause difficulties for maintenance.

It's correct, the existence of potentially existing parallel console session is checked right before the console is downloaded (the new VmActions/sagas.js).

Just an idea:
Within this saga, the consoleUser might be set to some constant pessimistically enforcing the state the console is in use and once the fetchVmSessions() finishes, correct value is set

No, they can not be merged in to one, only if I add actions from reducer VmAction to vms reducer, but of whole that modularity is to make every module more independent and not to make mess in global files (like sagas.js or reducers/vms.js).
Yes, it's a good idea to add consoleInUse parameter to state

mareklibra · 2017-04-06T14:22:56Z

src/actions/vm.js

+
+export function setVmSessions ({ vmId, sessions }) {
+  return {
+    type: 'SET_VM_SESSIONS',


please use constants.js

mareklibra · 2017-04-06T14:24:30Z

src/components/VmActions/actions.js

@@ -0,0 +1,20 @@
+import { SET_CONSOLE_IN_USE, CONSOLE_IN_USE } from './constants'


I would prefer a single file to be used across whole application.
It will help to avoid name conflicts without explicit namespacing.
Merge conflicts are easy to resolve.

The idea of dividing is to make every module more independent, and this action use only this module, so there can not be any conflicts.

mareklibra · 2017-04-07T12:01:45Z

For user confirmation I do prefer the
https://ethaizone.github.io/Bootstrap-Confirmation/
as written in my previous comment.

Just for a reference, if modal is supposed to be used, I would follow:
http://www.patternfly.org/pattern-library/widgets/#modal

bond95 · 2017-04-19T15:35:37Z

@mareklibra change modal window to user confirmation popup, please review

mareklibra

@bond95 , I pulled the pull request to try and I can still see the modal dialog, no confirmation component. Did you push proper version, please?

mareklibra · 2017-04-20T07:24:12Z

src/components/DetailContainer.js

@@ -25,7 +25,7 @@ class DetailContainer extends Component {
  render () {
    const { children } = this.props
    return (
-      <div className={'container-fluid ' + sharedStyle['move-left-detail']}>
+      <div className={'container-fluid ' + sharedStyle['move-left-detail'] + ' ' + sharedStyle['detail-z-index']}>


Can be left for a follow-up, but string composition with back-ticks is better in this case

mareklibra · 2017-04-20T07:27:10Z

yarn.lock

@@ -46,8 +46,8 @@ ajv-keywords@^1.0.0:
  resolved "https://registry.yarnpkg.com/ajv-keywords/-/ajv-keywords-1.5.1.tgz#314dd0a4b3368fad3dfcdc54ede6171b886daf3c"

 ajv@^4.7.0:
-  version "4.11.3"
-  resolved "https://registry.yarnpkg.com/ajv/-/ajv-4.11.3.tgz#ce30bdb90d1254f762c75af915fb3a63e7183d22"
+  version "4.11.5"


Since there are no changes to the package.json, please remove this file from commit.

bond95 · 2017-04-20T07:48:31Z

@mareklibra fixed. I think better if popup will look something like this,

mareklibra · 2017-04-20T08:11:18Z

yarn.lock

@@ -1,5707 +0,0 @@
-# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


Please do not git rm yarn.lock :-)
My previous comment was just about committing changes if the package.json is untouched.

mareklibra · 2017-04-20T08:27:31Z

src/components/VmActions/index.js

-  })
+  onConsoleClick (e) {
+    this.setState({
+      openModal: true,


please rename to consoleConfirmationAboutToOpen

mareklibra · 2017-04-20T08:31:14Z

src/components/VmActions/index.js

-    okLabel: 'Yes',
-    onOk: onSuspend,
-  })
+  onModalClose () {


please rename to onConsoleConfirmationClose()

mareklibra · 2017-04-20T08:33:08Z

src/components/VmActions/index.js

+        okLabel: 'Yes',
+        onOk: this.onConsoleDownload,
+        onCancel: this.onModalClose,
+      }))()


Can be the OnClickTopConfirmation() function executed directly without the wrapping?

I tried do it without wrapping and it throw error

mareklibra · 2017-04-20T08:37:43Z

src/components/VmActions/index.js

+  }
+
+  componentDidUpdate (prevProps, prevState) {
+    if (


I think this code can be moved to the render() method since it's called once props/state is updated.

No, it can't cause function render must be pure and can only return component, but not change other components, or added new one inside

The flow is strange, but ok, let's keep it.
Please simplify the call on line 148, I think you can call the function directly, can't you?

bond95 · 2017-04-28T13:05:36Z

@mareklibra fixed conflicts, please review

mareklibra · 2017-05-04T07:19:23Z

Merge is pending due to conflicts in oVirt/ovirt-ui-components#34

bond95 · 2017-05-25T12:50:46Z

@mareklibra please review

mareklibra · 2017-05-25T13:40:02Z

oVirt/ovirt-ui-components#34 seems to be still needed for this PR, please fix issues there. Once merged in ovirt-ui-components, we can merge here.

bond95 · 2017-05-25T14:13:32Z

@mareklibra I closed this oVirt/ovirt-ui-components#34 PR, because all changes was already rebased to web-ui

mareklibra · 2017-05-26T07:32:38Z

Replaced by #204

bond95 mentioned this pull request Mar 17, 2017

Add detection that someone is connected to the VM #66

Closed

jelkosz reviewed Mar 21, 2017

View reviewed changes

mareklibra suggested changes Apr 6, 2017

View reviewed changes

mareklibra reviewed Apr 6, 2017

View reviewed changes

mareklibra mentioned this pull request Apr 7, 2017

Shutdown confirmation widget UX issues #121

Closed

bond95 force-pushed the console-in-use branch from f38789a to 3060c27 Compare April 19, 2017 14:53

mareklibra suggested changes Apr 20, 2017

View reviewed changes

bond95 force-pushed the console-in-use branch from 3060c27 to 5e6cec6 Compare April 20, 2017 07:47

bond95 force-pushed the console-in-use branch from 5e6cec6 to 1ddcb15 Compare April 20, 2017 07:55

mareklibra suggested changes Apr 20, 2017

View reviewed changes

bond95 force-pushed the console-in-use branch from 1ddcb15 to 2384fcb Compare April 20, 2017 10:50

bond95 force-pushed the console-in-use branch 2 times, most recently from ec6e16a to 328d1a2 Compare April 28, 2017 13:04

mareklibra approved these changes May 4, 2017

View reviewed changes

Add "Console in use" feature to VmActions

cbe420c

bond95 force-pushed the console-in-use branch from 328d1a2 to cbe420c Compare May 25, 2017 12:49

mareklibra added a commit that referenced this pull request May 26, 2017

Fix yarn.lock from #106

0052f24

mareklibra mentioned this pull request May 26, 2017

Add feature for changing console button tooltip to "Console in use" #204

Merged

mareklibra closed this May 26, 2017

		onGetConsole: () => dispatch(getConsole({ vmId: vm.get('id') })),
		modalClose: () => dispatch(setConsoleInUse({ vmId: vm.get('id'), consoleInUse: false })),

		@@ -308,10 +340,12 @@ export function *rootSaga () {
		takeEvery('START_VM', startVm),
		takeEvery('GET_CONSOLE_VM', getConsoleVm),

		@@ -0,0 +1,5 @@
		import { buildSagas as VmActionsSagas } from './components/VmActions/sagas'

		export default (sagas) => [

		@@ -0,0 +1,20 @@
		import { SET_CONSOLE_IN_USE, CONSOLE_IN_USE } from './constants'

		@@ -1,5707 +0,0 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

Add feature for changing console button tooltip to "Console in use" #106

Add feature for changing console button tooltip to "Console in use" #106

Conversation

bond95 commented Mar 17, 2017 • edited by mareklibra

lago-bot commented Mar 17, 2017

lago-bot commented Mar 17, 2017

lago-bot commented Mar 17, 2017

lago-bot commented Mar 17, 2017

lago-bot commented Mar 17, 2017

lago-bot commented Mar 17, 2017

lago-bot commented Mar 17, 2017

lago-bot commented Mar 17, 2017

ovirt-infra commented Mar 20, 2017

ovirt-infra commented Mar 20, 2017

ovirt-infra commented Mar 20, 2017

ovirt-infra commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landgraf commented Mar 23, 2017

mareklibra commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mareklibra commented Apr 7, 2017

bond95 commented Apr 19, 2017

mareklibra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bond95 commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bond95 commented Apr 28, 2017

mareklibra commented May 4, 2017

bond95 commented May 25, 2017

mareklibra commented May 25, 2017

bond95 commented May 25, 2017

mareklibra commented May 26, 2017

bond95 commented Mar 17, 2017 •

edited by mareklibra