Fixed bug with VM detail leading after refresh #497

bond95 · 2018-02-21T14:21:32Z

Bug: If open detail of VM that is not on first page, and refresh it, it'll leads to main page.
Fixes: #491

This change is

mareklibra · 2018-02-26T11:20:29Z

src/actions/vm.js

@@ -1,6 +1,7 @@
 import {
  CHANGE_VM_ICON,
  CHANGE_VM_ICON_BY_ID,
+  CHECK_VM_AVIABILITY,


mareklibra · 2018-02-26T11:26:59Z

src/components/Pages/index.js

@@ -110,6 +110,7 @@ const VmDetailPageConnected = connect(
  }),
  (dispatch) => ({
    getVms: ({ vmId }) => dispatch(selectVmDetail({ vmId })),
+    checkVMAviability: ({ vmId }) => dispatch(checkVMAviability({ vmId })),


I do not like this collision of names, it might be source of dummy bugs in the future or decreases readability at least.

jniederm · 2018-02-26T12:17:41Z

src/actions/vm.js

@@ -345,3 +346,12 @@ export function changeVmIconById ({ vmId, iconId }) {
    },
  }
 }
+
+export function checkVMAviability ({ vmId }) {


Can we follow the convention that only first letter in an abbreviation is capitalized? Like: checkVmAvailability.

We already use that in e.g. VmDialog, VmDetail, and others. Similarly we use CHECK_VM_AVIABILITY, not CHECK_V_M_AVIABILITY

jniederm · 2018-02-26T12:46:18Z

src/sagas.js

@@ -581,6 +582,16 @@ function* fetchUSBFilter (action) {
  }
 }

+function* checkVmAviability (action) {


Maybe this can be simplified:

yield fetchSingleVm({ payload: { vmId: action.payload.vmId } }) const vm = yield select(state => state.getIn(['vms', 'vms', action.payload.vmId]) if (!vm) { yield put(redirectRoute({ route: '/' })) }

It could save one request.

jniederm · 2018-02-26T12:50:20Z

src/sagas.js

@@ -581,6 +582,16 @@ function* fetchUSBFilter (action) {
  }
 }

+function* checkVmAvailability (action) {


copy from previous pull request version

Maybe this can be simplified:

yield fetchSingleVm({ payload: { vmId: action.payload.vmId } }) const vm = yield select(state => state.getIn(['vms', 'vms', action.payload.vmId]) if (!vm) { yield put(redirectRoute({ route: '/' })) }

It could save one request.

No, because if we call fetchSingleVm it update state vms, before adding fetched VM to state, thus VmDetailPage will update and call check again, and again, and again... Thus my version is more simplified. Otherwise it need to add more lines for take care of this recursion.

I see. So how about (pseudo-code):

const vmFromStore = yield select(state => state.getIn(['vms', 'vms', action.payload.vmId]) if (vmFromStore) { return } yield fetchSingleVm({ payload: { vmId: action.payload.vmId } }) const loadedVm = yield select(state => state.getIn(['vms', 'vms', action.payload.vmId]) if (!loadedVm) { yield put(redirectRoute({ route: '/' })) }

I agree that originally posted version is easy to read. I find network requests expensive and requesting the same entity twice in this case not necessary.

It doing the same thing, and we already check availability of VM in current vms state, in VmDetailPage component.

bond95 · 2018-02-26T15:02:45Z

@jniederm @mareklibra Done.

jniederm

except that new line LGTM

jniederm · 2018-02-26T15:08:22Z

src/components/PageRouter.js

@@ -103,6 +103,7 @@ class PageRouter extends React.Component {

  render () {
    let { route, location, history, routeReducer } = this.props
+


Now this seems pretty unrelated.

bond95 · 2018-02-27T12:07:03Z

@jniederm Done.

mareklibra · 2018-02-28T09:49:12Z

Review status: 0 of 1 files reviewed at latest revision, 7 unresolved discussions.

src/components/Pages/index.js, line 22 at r4 (raw file):

    const vmInStore = this.props.vms.getIn(['vms', this.props.match.params.id])
    if (!vmInStore && Selectors.isFilterChecked()) {
      this.props.getVms({ vmId: this.props.match.params.id })

This can cause firing of multiple same API requests at the app init.

I would move that to componentWillMount()

Comments from Reviewable

bond95 · 2018-02-28T10:59:42Z

@mareklibra it already in componentWillMount, otherwise it wouldn't work. Because component will mount only one time, if internet speed gonna be too slow then it don't get filterChecked in time.

mareklibra · 2018-02-28T11:27:04Z

I'm sorry, I don't understand following sentence:

it already in componentWillMount, otherwise it wouldn't work.

In recent implementation, I can see the call within componentWillUpdate() , which is not called for first (initial) rendering but for every other, means whenever props or state change. This can cause multiple API requests until the very first one finishes.
This can be fixed in multiple ways, the simplest one seems to be just moving the call to the componentWillMount or similar to ensure, it is called just once.

If availability of state.config.isFilterChecked is an issue at the time of componentWillUpdate() calling, it can be fixed i.e. either by

saga (not preferred)
a wait function performing up to N sleeps of 50ms (or so) until state.config.isFilterChecked === true, then calling a callback function which is the dispatch in our case

bond95 · 2018-02-28T11:32:25Z

@mareklibra

I'm sorry, I don't understand following sentence:

Here you can see almost the same functionality https://github.com/bond95/ovirt-web-ui/blob/d972d0d2a9dfbec9909239e7cdabba678aa174ba/src/components/Pages/index.js#L13

This can cause multiple API requests until the very first one finishes.

It wouldn't, because if VM already in redux state, then it don't gonna send request again, you can see it here in vmInStore:

if (!vmInStore && Selectors.isFilterChecked()) {

mareklibra · 2018-02-28T12:58:51Z

It wouldn't, because if VM already in redux state,

It will. The componentWillUpdate() is called whenever props or state are changed. And at the app initialization, there are multiple changes occurring, so this might be called multiple times shen the VM is still not in the store (means untile the first getVm() finishes, consider scenario with overloaded server).

bond95 · 2018-02-28T14:19:49Z

@mareklibra what do you think now?

Bug: If open detail of VM that is not on first page, and refresh it, it'll leads to main page. Fixes: oVirt#491

bond95 requested review from jniederm and mareklibra February 21, 2018 14:21

mareklibra suggested changes Feb 26, 2018

View reviewed changes

jniederm reviewed Feb 26, 2018

View reviewed changes

bond95 force-pushed the vm-detail-refresh branch from baa4aa0 to b24bda3 Compare February 26, 2018 12:45

jniederm reviewed Feb 26, 2018

View reviewed changes

jniederm suggested changes Feb 26, 2018

View reviewed changes

bond95 force-pushed the vm-detail-refresh branch from b24bda3 to 92adbca Compare February 26, 2018 15:02

jniederm reviewed Feb 26, 2018

View reviewed changes

bond95 force-pushed the vm-detail-refresh branch from 92adbca to d972d0d Compare February 27, 2018 12:06

jniederm approved these changes Feb 27, 2018

View reviewed changes

bond95 force-pushed the vm-detail-refresh branch from d972d0d to f08dd8d Compare February 28, 2018 14:19

Fixed bug with VM detail leading after refresh

258d869

Bug: If open detail of VM that is not on first page, and refresh it, it'll leads to main page. Fixes: oVirt#491

bond95 force-pushed the vm-detail-refresh branch from f08dd8d to 258d869 Compare February 28, 2018 14:30

mareklibra approved these changes Feb 28, 2018

View reviewed changes

mareklibra merged commit f9bfe46 into oVirt:master Feb 28, 2018

mareklibra mentioned this pull request Feb 28, 2018

Release 1.3.6 #503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed bug with VM detail leading after refresh #497

Fixed bug with VM detail leading after refresh #497

bond95 commented Feb 21, 2018 •

edited by mareklibra

Loading

mareklibra Feb 26, 2018

mareklibra Feb 26, 2018

jniederm Feb 26, 2018

jniederm Feb 26, 2018

jniederm Feb 26, 2018

bond95 Feb 26, 2018

jniederm Feb 26, 2018

bond95 Feb 26, 2018 •

edited

Loading

bond95 commented Feb 26, 2018

jniederm left a comment

jniederm Feb 26, 2018

bond95 commented Feb 27, 2018

mareklibra commented Feb 28, 2018

bond95 commented Feb 28, 2018

mareklibra commented Feb 28, 2018

bond95 commented Feb 28, 2018 •

edited

Loading

mareklibra commented Feb 28, 2018

bond95 commented Feb 28, 2018

		@@ -103,6 +103,7 @@ class PageRouter extends React.Component {

		render () {
		let { route, location, history, routeReducer } = this.props

Fixed bug with VM detail leading after refresh #497

Fixed bug with VM detail leading after refresh #497

Conversation

bond95 commented Feb 21, 2018 • edited by mareklibra Loading

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 Feb 26, 2018 • edited Loading

Choose a reason for hiding this comment

bond95 commented Feb 26, 2018

jniederm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bond95 commented Feb 27, 2018

mareklibra commented Feb 28, 2018

bond95 commented Feb 28, 2018

mareklibra commented Feb 28, 2018

bond95 commented Feb 28, 2018 • edited Loading

mareklibra commented Feb 28, 2018

bond95 commented Feb 28, 2018

bond95 commented Feb 21, 2018 •

edited by mareklibra

Loading

bond95 Feb 26, 2018 •

edited

Loading

bond95 commented Feb 28, 2018 •

edited

Loading