Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refs #28382 - create main API reducer #7222

Merged
merged 3 commits into from Jan 8, 2020

Conversation

@laviro
Copy link
Member

laviro commented Dec 1, 2019

Example: how to use the API middleware

The example API returns a JSON object like this:

{
  "items": [
    {"id": 319, "title": "setting", "action": "update"},
    {"id": 150, "title": "bookmark", "action": "create"}
  ],
}
  /** MyComponent.js*/
  import React from 'react';
  import { STATUS } from '../../constants';

  const MyComponent = ({ status, error, items }) => {
    if (status === STATUS.PENDING ) {
      return <div>Loading...</div>
    }

    if (status === STATUS.ERROR) {
      return <div>Error: {error.message}</div>
    }

    return (
      <ul>
        {items.map(item => (
          <li key={item.id}>
            {item.title} {item.action}
          </li>
        ))}
      </ul>
    );
  }
  /** index.js*/
  import React, { useEffect } from 'react';
  import PropTypes from 'prop-types';
  import { useSelector, useDispatch } from 'react-redux';
  import { selectItems, selectStatus, selectError } from './MyComponentsSelectors.js';
  import { getData } from './MyComponentActions';
  import MyComponent from './MyComponent';

  const ConnectedMyComponent = ({ path }) => {
    const items = useSelector(selectItems);
    const status = useSelector(selectStatus);
    const error = useSelector(selectError);

    const dispatch = useDispatch();

    useEffect(() => {
      dispatch(getData(path))
    }, [path]);

    return (
      <MyComponent 
        items={items} 
        status={status} 
        error={error}
      />
    );
  };

  ConnectedMyComponent.propTypes = {
    path: PropTypes.string.isRequired,
  };

  export default ConnectedMyComponent;
  /** MyComponentSelectors.js*/

  import { MY_SPECIAL_KEY } from './MyComponentConstants';
  import { selectAPIStatus, selectAPIError, selectAPIResponse } from '../../redux/API/APISelectors';

  // use the same key that you've used in the API action.
  export const selectItems = state => selectAPIResponse(state, MY_SPECIAL_KEY).items || [];

  export const selectStatus = state => selectAPIStatus(state, MY_SPECIAL_KEY);

  export const selectError = state => selectAPIError(state, MY_SPECIAL_KEY);
  /** MyComponentActions.js*/

  export const getData = url => ({
    type: API_OPERATIONS.GET,
    key: MY_SPECIAL_KEY, // you will need to re-use this key in order to access the right API reducer later.
    url,
    payload: {}
  });

Once the action is triggered, the API middleware will manage the request
and update the store with the request status:

the store on API pending:

{
  ...
  API: {
    MY_SPECIAL_KEY: { // The key that was provided in the API action.
      response: null,
      status: "PENDING",
      payload: {},
    }
  }
}

the store on API success:

{
  ...
  API: {
    MY_SPECIAL_KEY: {
      response: {
        items: [
          {id: 319, title: "setting", action: "update"},
          {id: 150, title: "bookmark", action: "create"}
        ],
      },
      status: "RESOLVED",
      payload: {},
    }
  }
}

the store on API failure:

{
  ...
  API: {
    MY_SPECIAL_KEY: {
      response: "Network Error",
      status: "ERROR",
      paylod: {},
    }
  }
}
@theforeman-bot

This comment has been minimized.

Copy link
Member

theforeman-bot commented Dec 1, 2019

Issues: #28382

@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 8, 2019

Coverage Status

Coverage increased (+0.3%) to 75.167% when pulling e3f00e5 on laviro:feat/main_api_reducer into e3166aa on theforeman:develop.

@laviro laviro force-pushed the laviro:feat/main_api_reducer branch from f47a369 to 712977d Dec 9, 2019
Copy link
Contributor

jeremylenz left a comment

Tested and works as advertised, thanks @laviro! In addition to silly nitpicks etc. below, here are my thoughts.

If I understand correctly, here's what this feature is doing:

  1. Watch for a special manageResponseByID value in the action.
  2. If manageResponseByID is true, pull the id value out of action.payload.
  3. Instead of merging ...payload with the store state like normal, nest one level deeper and create a new key [id]: {...payload}.

But what doesn't feel right to me is,

  1. In APIReducer.js, all the reducer logic is extracted out into handleRequest, handleSuccess, and handleFailure so you can't really see what the logic is.
  2. In APIHelpers.js it seems like this is where the reducer logic really lives.
  3. The getModifiedState function is the only one in that file that seems like a true helper function.
  • I would prefer to see handleRequest, handleSuccess, and handleFailure logic moved back into APIReducer.js.
  • getModifiedState could have a more descriptive name, maybe transformPayloadById or something? (Or maybe just add a comment that describes what it does)
@laviro laviro force-pushed the laviro:feat/main_api_reducer branch from 712977d to 7dfcbae Dec 10, 2019
@laviro laviro force-pushed the laviro:feat/main_api_reducer branch from 7dfcbae to 1887819 Dec 11, 2019
@laviro

This comment has been minimized.

Copy link
Member Author

laviro commented Dec 11, 2019

@jeremylenz @MariaAga thanks for your review, now the easiest way to review is by looking on each commit separately.

I dropped the usage of manageResponseByID key that was passed to the action and instead, the consumers could pass formatters which will give them the ability to change the payload which will be passed to the reducer.

the default formatters are:

const formatSuccess = ({ data, payload, status, error }) => ({
  status,
  error,
  ...payload,
  ...data,
});

const formatFailure = ({ payload, status, error }) => ({
  status,
  error,
  ...payload,
});

const formatPending = ({ payload, status, error }) => ({
  status,
  error,
  ...payload,
});

If the consumers want to push the payload into an ID, they could use the ID formatters from APIHelpers: formatSuccessByID, formatFailureByID, formatPendingByID, in the action it will look like:

export const getHostPowerState = ({ id, url }) => ({
  type: API_OPERATIONS.GET,
  key: HOST_POWER_STATUS,
  url,
  payload: { id },
  formatPending: formatPendingByID,
  formatFailure: formatFailureByID,
  formatSuccess: formatSuccessByID,
});

or as a shortcut:

export const getHostPowerState = ({ id, url }) => ({
  type: API_OPERATIONS.GET,
  key: HOST_POWER_STATUS,
  url,
  payload: { id },
  ...idFormatters,
});

tell me what do you think about this approach,
and soon I will add some documentation

@laviro laviro changed the title WIP - Refs #28382 - create main API reducer Refs #28382 - create main API reducer Dec 11, 2019
@laviro laviro force-pushed the laviro:feat/main_api_reducer branch from 1887819 to 2c9caa1 Dec 12, 2019
@laviro

This comment has been minimized.

Copy link
Member Author

laviro commented Dec 12, 2019

I deleted the inner component which isn't really needed, and updated tests and stories in the main component.

@laviro laviro force-pushed the laviro:feat/main_api_reducer branch from 9995fb8 to dd4495e Dec 15, 2019
@laviro

This comment has been minimized.

Copy link
Member Author

laviro commented Dec 15, 2019

The PR got too big since there was a lot of refactoring to the power status component,
I switched to refactor the fact chart component which was easier,
and will continue to refactor power status after this will get in.

@laviro laviro force-pushed the laviro:feat/main_api_reducer branch from dd4495e to 21f3bce Dec 16, 2019
@laviro laviro dismissed stale reviews from jeremylenz and sharvit via cccf41c Jan 7, 2020
@laviro laviro force-pushed the laviro:feat/main_api_reducer branch from 7bd971d to cccf41c Jan 7, 2020
@laviro

This comment has been minimized.

Copy link
Member Author

laviro commented Jan 7, 2020

Thanks @MariaAga, at first I thought to refactor FactChart in another PR,
though after we added the documentation on storybook, it feels bad not to adjust the at least the index file of fact chart in this PR.. which triggered more refactoring - all added changes are in a separate commit.


export const modalSuccessState = Immutable.merge(initialState, {
modalToDisplay: { 1: true },
chartData: chartDataValues,
loaderStatus: STATUS.RESOLVED,

This comment has been minimized.

Copy link
@laviro

laviro Jan 7, 2020

Author Member

not needed as status is maintained by the API reducer.

import { get } from '../../redux/API';

export const getChartData = (url, id) =>
get({

This comment has been minimized.

Copy link
@laviro

laviro Jan 7, 2020

Author Member

too simple for being in a separate action file - moved to index.js

This comment has been minimized.

Copy link
@MariaAga

MariaAga Jan 7, 2020

Member

I think it should still stay here so it will be clear what are the all fact charts actions are.

This comment has been minimized.

Copy link
@laviro

laviro Jan 7, 2020

Author Member

I spoke about it offline with @sharvit,
imo it makes sense to move it to the index file since it is actually an API action consumed here,
and not a custom fact chart action.

This comment has been minimized.

Copy link
@laviro

laviro Jan 7, 2020

Author Member

after we discussed it together offline, I moved it to the action file since there are actually two actions here. if it was only the get API action I would leave it here.

@@ -1,6 +1,3 @@
export const FACT_CHART = 'FACT_CHART';
export const FACT_CHART_REQUEST = 'FACT_CHART_REQUEST';
export const FACT_CHART_SUCCESS = 'FACT_CHART_SUCCESS';
export const FACT_CHART_FAILURE = 'FACT_CHART_FAILURE';

This comment has been minimized.

Copy link
@laviro

laviro Jan 7, 2020

Author Member

not needed anymore - maintained by the API reducer.


jest.unmock('../FactChartActions');

const fixtures = {
'getChartData should return api get with url and id': () =>
getChartData('url', 1),

This comment has been minimized.

Copy link
@laviro

laviro Jan 7, 2020

Author Member

that action was removed from the actions file.

Array [
"Fedora 21 extra extra extra extra extra extra extra extra extra extra long label",
3,
],

This comment has been minimized.

Copy link
@laviro

laviro Jan 7, 2020

Author Member

made this fixture a bit smaller.

const ConnectedFactChart = ({ data: { id, path, title, search } }) => {
const key = `${FACT_CHART}_${id}`;
const hostsCount = useSelector(state => selectHostCount(state, key));
const status = useSelector(state => selectFactChartStatus(state, key));
const chartData = useSelector(state => selectFactChartData(state, key));
const modalToDisplay = useSelector(state => selectDisplayModal(state, id));
const dispatch = useDispatch();
const openModal = () => {
dispatch(get({ key, url: path }));
dispatch(showModal(id, title));
};
Comment on lines 16 to 26

This comment has been minimized.

Copy link
@laviro

laviro Jan 7, 2020

Author Member

using our new convention here.

Copy link
Contributor

sharvit left a comment

Thanks @laviro, just a small comment.

@laviro laviro force-pushed the laviro:feat/main_api_reducer branch from e0cfa31 to e3f00e5 Jan 8, 2020
@laviro

This comment has been minimized.

Copy link
Member Author

laviro commented Jan 8, 2020

Thanks @sharvit, updated :)

@sharvit
sharvit approved these changes Jan 8, 2020
Copy link
Contributor

sharvit left a comment

Thanks @laviro 👍

@sharvit

This comment has been minimized.

Copy link
Contributor

sharvit commented Jan 8, 2020

[test foreman]

@laviro

This comment has been minimized.

Copy link
Member Author

laviro commented Jan 8, 2020

All tests are green

@sharvit sharvit merged commit 0f4cd82 into theforeman:develop Jan 8, 2020
7 checks passed
7 checks passed
Hound No violations found. Woof!
Redmine issues Valid issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 75.167%
Details
foreman Build finished. 22902 tests run, 9 skipped, 0 failed.
Details
katello Build finished. 4617 tests run, 11 skipped, 0 failed.
Details
upgrade Build finished. No test results found.
Details
@sharvit

This comment has been minimized.

Copy link
Contributor

sharvit commented Jan 8, 2020

Merged. Thanks @laviro @MariaAga @jeremylenz

@tbrisker

This comment has been minimized.

Copy link
Member

tbrisker commented Jan 13, 2020

I'm not 100% sure if it's this PR or not, but looks like the audits page is now broken and causes an endless loop calling the server when paginating

@MariaAga

This comment has been minimized.

Copy link
Member

MariaAga commented Jan 13, 2020

@tbrisker After reverting the HOC.js file from this pr: #7225 audits work fine

@laviro

This comment has been minimized.

Copy link
Member Author

laviro commented Jan 13, 2020

Thanks @MariaAga 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.