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

[#690] Add warning to mapping wizard when no OSP conversion hosts are present #812

Merged
merged 7 commits into from Dec 3, 2018

Conversation

@mturley
Copy link
Contributor

mturley commented Nov 28, 2018

.set('isFetchingConversionHosts', true)
.set('isRejectedConversionHosts', false)
.set('errorFetchingConversionHosts', null);
case `${FETCH_V2V_CONVERSION_HOSTS}_FULFILLED`:

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Nov 28, 2018

Similar blocks of code found in 7 locations. Consider refactoring.

@@ -40,6 +64,10 @@ class MappingWizardClustersStep extends React.Component {
queryHostsAction(queryHostsUrl, hostIDsByClusterID);
}
});

if (targetProvider === 'openstack') {
fetchConversionHostsAction(fetchConversionHostsUrl);

This comment has been minimized.

Copy link
@michaelkro

michaelkro Dec 3, 2018

Contributor

A bit of a nit, but can we make this API call in componentDidMount instead of as a side effect of fetching clusters?

This comment has been minimized.

Copy link
@michaelkro

michaelkro Dec 3, 2018

Contributor

Oh, looking at the entire method, I see why this is here now. Nevermind, sorry about that

@mturley mturley force-pushed the mturley:690-osp-conversion-host-warning branch from 8a1be73 to ea5dfed Dec 3, 2018
Copy link
Contributor

michaelkro left a comment

We can remove the need to fire off an action to clear the conversionHosts from the redux store by

  1. Create a redux selector that filters conversion hosts down to ospConversionHosts
  2. Pass ospConversionHosts down to MappingWizardClustersStep via mapStateToProps
  3. Change default value of conversionHosts to an empty array in reducer
  4. Trigger alert if ospConversionHosts is an empty array

Reasons for my suggestion

  1. It doesn't feel right to clear the store and refetch conversion hosts every time the clusters step is visited. For example, in the case where OSP conversion hosts do exist, it does not make sense to refetch them every time. Only fetch conversion hosts if ospConversionHosts is empty
  2. This approach will allow us to achieve the desired effect with less lines of code as we can remove clearConversionHostsAction.
Copy link
Contributor

michaelkro left a comment

Since this is part of adding OSP as a provider, can you add some tests? At least for the conversion host actions/reducers

(and the selector if you choose to go that route)

.set('isFetchingConversionHosts', false)
.set('isRejectedConversionHosts', false)
.set('errorFetchingConversionHosts', null);
case `${FETCH_V2V_CONVERSION_HOSTS}_REJECTED`:

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Dec 3, 2018

Similar blocks of code found in 11 locations. Consider refactoring.

.set('isFetchingConversionHosts', true)
.set('isRejectedConversionHosts', false)
.set('errorFetchingConversionHosts', null);
case `${FETCH_V2V_CONVERSION_HOSTS}_FULFILLED`:

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Dec 3, 2018

Similar blocks of code found in 7 locations. Consider refactoring.

@@ -13,7 +13,7 @@ import { determineTargetProvider } from '../../helpers';

class MappingWizardGeneralStep extends React.Component {
componentDidMount = () => {
const { editingMapping, initialize, initialized } = this.props;
const { editingMapping, initialize, initialized, fetchConversionHostsAction, fetchConversionHostsUrl } = this.props;

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Dec 3, 2018

Similar blocks of code found in 6 locations. Consider refactoring.

expect(store.getActions()).toMatchSnapshot();
});

it('should fetch conversion hosts and return PENDING and FULFILLED action', () => {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Dec 3, 2018

Similar blocks of code found in 5 locations. Consider refactoring.

import { SET_V2V_EDITING_MAPPING, FETCH_V2V_CONVERSION_HOSTS } from '../MappingWizardGeneralStepConstants';
import { conversionHosts } from '../mappingWizardGeneralStep.fixtures';

it('sets default state', () => {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Dec 3, 2018

Similar blocks of code found in 4 locations. Consider refactoring.

}
`;

exports[`fetching conversion hosts is successful and there is data 1`] = `

This comment has been minimized.

Copy link
@michaelkro

michaelkro Dec 3, 2018

Contributor

This snapshot is a bit unwieldy, can we change the test to keep this more readable?

@@ -22,6 +22,8 @@ class MappingWizardGeneralStep extends React.Component {
targetProvider: determineTargetProvider(editingMapping)
});
}

fetchConversionHostsAction(fetchConversionHostsUrl);

This comment has been minimized.

Copy link
@michaelkro

michaelkro Dec 3, 2018

Contributor

I'm wondering if, now that we're not clearing the conversion hosts from the store, we should only fetch conversion hosts if the array is empty.

The only downside that I can think of is stale data. The user would have to refresh their browser in case a conversion host is somehow added w/o them navigating away from the mappings page

This comment has been minimized.

Copy link
@mturley

mturley Dec 3, 2018

Author Contributor

Ok, good call.

@@ -13,7 +13,14 @@ import { determineTargetProvider } from '../../helpers';

class MappingWizardGeneralStep extends React.Component {
componentDidMount = () => {
const { editingMapping, initialize, initialized } = this.props;
const {

This comment has been minimized.

Copy link
@codeclimate

codeclimate bot Dec 3, 2018

Similar blocks of code found in 4 locations. Consider refactoring.

@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Dec 3, 2018

Checked commits mturley/manageiq-v2v@fd281ee~...291ab76 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

Copy link
Contributor

michaelkro left a comment

Works great!

@michaelkro michaelkro merged commit cbcb4e5 into ManageIQ:master Dec 3, 2018
2 checks passed
2 checks passed
Hakiri No security warnings were found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
simaishi added a commit that referenced this pull request Dec 4, 2018
[#690] Add warning to mapping wizard when no OSP conversion hosts are present

(cherry picked from commit cbcb4e5)

https://bugzilla.redhat.com/show_bug.cgi?id=1654385
@simaishi

This comment has been minimized.

Copy link

simaishi commented Dec 4, 2018

Hammer backport details:

$ git log -1
commit fd367dfcc716819b853215cd0d78077a6fbea2ff
Author: Michael Ro <mikerodev@gmail.com>
Date:   Mon Dec 3 17:50:02 2018 -0500

    Merge pull request #812 from mturley/690-osp-conversion-host-warning
    
    [#690] Add warning to mapping wizard when no OSP conversion hosts are present
    
    (cherry picked from commit cbcb4e5e669822c002a34f28e53f3a442f050cca)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1654385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.