Fixes #23709 - StatisticsCharts fails handling errors #5616

boaz0 · 2018-05-27T12:22:15Z

It seems like when an error occures during fetching the data the error object is not passed to ChartBox as a string.
In addition, the action payload is different when error occures, causing the error message to be in a new ChartBox.

Before

After

theforeman-bot · 2018-05-27T12:22:23Z

Issues: #23709

coveralls · 2018-05-27T12:29:10Z

Coverage remained the same at 79.425% when pulling cc62291 on ripcurld:fixes_23709 into 59cdda3 on theforeman:develop.

coveralls · 2018-05-27T12:29:10Z

Coverage remained the same at 79.425% when pulling cc62291 on ripcurld:fixes_23709 into 59cdda3 on theforeman:develop.

coveralls · 2018-05-27T12:29:10Z

Coverage remained the same at 79.425% when pulling cc62291 on ripcurld:fixes_23709 into 59cdda3 on theforeman:develop.

ohadlevy

looks good to me, but 1. I have now idea how it stopped working (could be axios pr related? @amirfefer ?) and wondering if we can add a test for it?

boaz0 · 2018-05-27T13:27:54Z

@ohadlevy Yes sure, I am working on this right now.
If I have time, I can git-blame and investigate this more.
Although, it seems to me like an honest mistake in the unit-tests.

boaz0 · 2018-05-31T16:38:13Z

@ohadlevy you're right: a5a6b5d#diff-0be26feab8a96771c4d11efe9e3758c1L13.
jquery.ajax returns error as string while axios returns it as Error.
Also, here - it used to have id and error but now it has error and item which causes a different problem.

In regards to why tests pass - we don't have tests to ajaxRequestAction. So there was no way to make sure moving to axios would break something. Also, looking at the fixtures in reducers (such as in statistics) I find it odd that they were set to an empty object from the beginning especially that they were strings.

Anyway, this patch fixes all these regressions - adding more tests and fixing some (due to moving to axios) .

Thanks for the information Ohad.

amirfefer · 2018-06-05T11:18:34Z

webpack/assets/javascripts/react_app/redux/actions/common/index.js

@@ -11,5 +11,5 @@ export const ajaxRequestAction = ({
  dispatch({ type: requestAction, payload: item });
  return API.get(url)
    .then(({ data }) => dispatch({ type: successAction, payload: { ...item, ...data } }))
-    .catch(error => dispatch({ type: failedAction, payload: { error, item } }));
+    .catch(error => dispatch({ type: failedAction, payload: { item, error: error.message } }));


does it work with all the components that use ajaxRequestAction?

Yes, it does. I did do manual testing if that helps.
Basically, there are 4 components that are doing API calls:
BreadcrumbBar, Bookmarks, Host's PowerStatus and Statistics.
3 out of them are using ajaxRequestAction - BreadcrumbBar doesn't use ajaxRequestAction probably because of payload structure nuance.

Bookmarks doesn't use any element set in item but does expect error to be the error message - which is fixed in this patch.
Host's PowerStatus and Statistics however are expecting the payload to have id given in item (I guess matan missed that change when he pushed this) so I decided to keep item there and use it in reducers.

I guess it would be nicer if I keep the order, though (for a readable diff):

payload: { error: error.message, item }

boaz0 · 2018-06-05T12:43:56Z

webpack/assets/javascripts/react_app/components/statistics/StatisticsChartsList.js

@@ -32,7 +32,7 @@ class StatisticsChartsList extends React.Component {
        chart={chart}
        noDataMsg={__('No data available')}
        tip={__('Expand the chart')}
-        errorText={chart.error}
+        errorText={chart.error ? chart.error.message : ''}


Forgot to discard this change, though.

amirfefer · 2018-06-07T14:21:04Z

webpack/assets/javascripts/react_app/redux/actions/common/index.js

@@ -11,5 +11,5 @@ export const ajaxRequestAction = ({
  dispatch({ type: requestAction, payload: item });
  return API.get(url)
    .then(({ data }) => dispatch({ type: successAction, payload: { ...item, ...data } }))
-    .catch(error => dispatch({ type: failedAction, payload: { error, item } }));
+    .catch(error => dispatch({ type: failedAction, payload: { error: error.message, item } }));


I have one concern about storing only the error message:
the message property is optional, and could be blank, plus if we'd like to distinct between error types we couldn't do it only with the error's message.

No problem. We can keep storing the error object but will map from error object to error message on the component side.

amirfefer

Thanks @ripcurld looks good !
I left some minor inline comments

Next time just please add a comment after an update :)

amirfefer · 2018-06-12T15:42:08Z

webpack/assets/javascripts/react_app/redux/actions/hosts/powerStatus/powerStatus.fixtures.js

@@ -27,4 +27,3 @@ export const onSuccessActions =
    payload: mockSuccessRequests[0].response,
    type: 'HOST_POWER_STATUS_SUCCESS',
  };
-


unnecessary deletion

amirfefer · 2018-06-12T15:48:15Z

webpack/assets/javascripts/react_app/redux/reducers/hosts/powerStatus/powerStatus.test.js

+      prev: stateBeforeResponse,
+      action: {
+        type: types.HOST_POWER_STATUS_FAILURE,
+        payload: { error, item: { id: request.id } },


could we store the entire resource under item ?

By saying "entire resource" are you talking about the request?
Basically you want to change this line to

payload: { error, item: request }

Right?

After a second look, only the id is stored via ajaxRequestAction

sharvit · 2018-06-13T08:27:56Z

webpack/assets/javascripts/react_app/redux/reducers/hosts/powerStatus/powerStatus.test.js

+    'should return the initial state': {
+      prev: undefined,
+      action: {},
+      next: initialState,


I personally prefer snapshots rather then supply a preconfigured results object.

I will see what I can do about that.

boaz0 · 2018-06-17T09:44:15Z

@sharvit @amirfefer
done! feel free to add more feedback.

amirfefer · 2018-06-17T10:25:41Z

webpack/assets/javascripts/react_app/components/statistics/StatisticsChartsList.js

@@ -32,7 +32,7 @@ class StatisticsChartsList extends React.Component {
        chart={chart}
        noDataMsg={__('No data available')}
        tip={__('Expand the chart')}
-        errorText={chart.error}
+        errorText={(chart.error || {}).message ? chart.error.message : ''}


could errorText remain undefined instead of initialize empty string each render?

errorText={chart.error && chart.error.message}

The FAILURE action dispatched in ajaxRequestAction is not aligned with the bookmarks, powerStatus and statistics reducers: - Moving to axios returns Error object instead of a string - The payload doesn't contain "id" but "item" This patch fixes that problem and makes components work on failures from API. In addition, tests were added and fixed to avoid regressions. Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>

boaz0 · 2018-06-18T08:44:32Z

[test katello]

amirfefer

LGTM 👍

ohadlevy · 2018-06-18T10:47:20Z

[test katello]

ohadlevy · 2018-06-18T12:12:01Z

test failure unrelated. thanks @ripcurld

theforeman-bot added Needs testing Not yet reviewed labels May 27, 2018

ohadlevy reviewed May 27, 2018

View reviewed changes

pondrejk previously approved these changes Jun 4, 2018

View reviewed changes

theforeman-bot removed the Not yet reviewed label Jun 4, 2018

amirfefer reviewed Jun 5, 2018

View reviewed changes

boaz0 commented Jun 5, 2018

View reviewed changes

boaz0 dismissed pondrejk’s stale review via 8b0cc96 June 7, 2018 08:40

amirfefer reviewed Jun 7, 2018

View reviewed changes

amirfefer suggested changes Jun 12, 2018

View reviewed changes

theforeman-bot added the Waiting on contributor label Jun 12, 2018

sharvit reviewed Jun 13, 2018

View reviewed changes

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Jun 17, 2018

amirfefer reviewed Jun 17, 2018

View reviewed changes

amirfefer approved these changes Jun 18, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Jun 18, 2018

ohadlevy approved these changes Jun 18, 2018

View reviewed changes

ohadlevy merged commit 7d6d573 into theforeman:develop Jun 18, 2018

boaz0 deleted the fixes_23709 branch June 18, 2018 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #23709 - StatisticsCharts fails handling errors #5616

Fixes #23709 - StatisticsCharts fails handling errors #5616

boaz0 commented May 27, 2018

theforeman-bot commented May 27, 2018

coveralls commented May 27, 2018

coveralls commented May 27, 2018

coveralls commented May 27, 2018

ohadlevy left a comment

boaz0 commented May 27, 2018

boaz0 commented May 31, 2018

amirfefer Jun 5, 2018 •

edited

boaz0 Jun 5, 2018

boaz0 Jun 5, 2018

boaz0 Jun 5, 2018

amirfefer Jun 7, 2018

boaz0 Jun 7, 2018

amirfefer left a comment •

edited

amirfefer Jun 12, 2018

amirfefer Jun 12, 2018

boaz0 Jun 12, 2018

amirfefer Jun 13, 2018

sharvit Jun 13, 2018

boaz0 Jun 13, 2018

boaz0 commented Jun 17, 2018

amirfefer Jun 17, 2018

boaz0 commented Jun 18, 2018

amirfefer left a comment

ohadlevy commented Jun 18, 2018

ohadlevy commented Jun 18, 2018

Fixes #23709 - StatisticsCharts fails handling errors #5616

Fixes #23709 - StatisticsCharts fails handling errors #5616

Conversation

boaz0 commented May 27, 2018

Before

After

theforeman-bot commented May 27, 2018

coveralls commented May 27, 2018

coveralls commented May 27, 2018

coveralls commented May 27, 2018

ohadlevy left a comment

Choose a reason for hiding this comment

boaz0 commented May 27, 2018

boaz0 commented May 31, 2018

amirfefer Jun 5, 2018 • edited

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

amirfefer left a comment • edited

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

boaz0 commented Jun 17, 2018

Choose a reason for hiding this comment

boaz0 commented Jun 18, 2018

amirfefer left a comment

Choose a reason for hiding this comment

ohadlevy commented Jun 18, 2018

ohadlevy commented Jun 18, 2018

amirfefer Jun 5, 2018 •

edited

amirfefer left a comment •

edited