Conversion Host Configuration Wizard Step #1 (Location), and introducing common reducers #871

mturley · 2019-01-31T22:33:29Z

Part of the Conversion Hosts UI feature BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1693339

This is part of #855.

This PR adds the contents of the Location step (Step 1) of the Configure Conversion Host wizard:

This PR also adds a check for sufficient providers before displaying the Conversion Hosts tab of the settings page, to prevent errors in this wizard step when a user has insufficient providers configured. In this case, the empty state is shown with a button for configuring providers, similar to the Plans and Mappings views:

Since this PR would have otherwise duplicated a bunch of code related to providers and target clusters/projects, I also introduced a new pattern of reducers coupled to API resources rather than to views. We have a good amount of this kind of code duplication elsewhere in the codebase, but that's outside the scope of this PR, so I only created the providersReducer and the targetResourcesReducer (which so far only handles target clusters). We can move things into common reducers like these incrementally, rather than duplicate code, as the need arises.

...ts/ConversionHostWizard/ConversionHostWizardLocationStep/ConversionHostWizardLocationStep.js

miq-bot · 2019-02-01T22:43:56Z

Checked commits mturley/manageiq-v2v@68da0c4~...2d29e21 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. 🍪

mturley · 2019-02-01T22:46:33Z

This needs unit tests, but is otherwise ready to go.

@michaelkro, I'm adding you as a reviewer here since you were kind enough to offer, but since it's past 5pm on your last day, feel free to revoke that offer at your discretion! Once again it's been a real pleasure working with you. Cheers 🍻

michaelkro · 2019-02-04T13:35:00Z

app/javascript/react/screens/App/Settings/SettingsActions.js

+    payload: API.get(url)
+  });
+
+export const fetchProvidersAction = url => {


Food for thought...

There is already a system in place for fetching and maintaining a list of providers in the Overview screen. Now that we need similar functionality here in Settings, we are duplicating that code, and maintaining the same data in two separate places.

When resources are shared across screens like this, it might make sense to have "application level" actions/reducers. These could be made available to the screens/components that need them in the same way we're already doing it (map state to props in the container components)

A few benefits for this approach

Reduce code duplication

It would reduce the size of our reducers and make them easier to read.

Screen reducers would only need to concern themselves with the UI state and resource reducers would manage data

That being said, the work here is in line with the state management system that is already in place. I just wanted to point this out

@michaelkro thanks for reviewing! I hope you're enjoying your time off.

That's a really good point, and something I considered while implementing this. I didn't go through with it because I thought it would be a huge refactor out of scope of this change (since we already have a bit of this duplication elsewhere), but actually I think it is a really good idea to do that incrementally. I think I'll create a SharedReducer and start with providers and target compute resources. Thanks for convincing me :)

@michaelkro I did a major refactor to create the new providersReducer and targetResourcesReducer based on this advice. I'm curious what you think of this implementation.

michaelkro · 2019-02-04T14:36:52Z

...ts/ConversionHostWizard/ConversionHostWizardLocationStep/ConversionHostWizardLocationStep.js

+class ConversionHostWizardLocationStep extends React.Component {
+  componentDidMount() {
+    const { fetchProvidersAction, fetchProvidersUrl } = this.props;
+    fetchProvidersAction(fetchProvidersUrl);


Since we're fetching providers on mount, we could use this data to dynamically populate the provider type dropdown with only what is available.

Oh, that makes me think, what happens on the conversion hosts tab if there are no providers? Do we show the provider empty state like in Mappings and Overview?

That's a really good point, I hadn't considered that. I think as implemented, it would just show the conversion hosts empty state and prompt the user to open the conversion host wizard, where the providers dropdown would be empty. That's a case I need to handle.

Also handled this.

...ts/ConversionHostWizard/ConversionHostWizardLocationStep/ConversionHostWizardLocationStep.js

app/javascript/react/screens/App/Settings/SettingsReducer.js

...ts/ConversionHostWizard/ConversionHostWizardLocationStep/ConversionHostWizardLocationStep.js

...reens/App/Settings/screens/ConversionHostsSettings/__tests__/ConversionHostsSettings.test.js

app/javascript/redux/common/providers/providersReducer.js

michaelkro

I think that introducing the common redux reducers is going to be really valuable moving forward. Really nice job taking this on

Things I tested

Creating mappings (OSP and RHV)
Creating plans (OSP and RHV)
Running RHV migration. Due to our new check that waits for an available conversion host, this test was inconclusive. However, everything up till that point seems ok
Checked to see that provider type dropdown is only populated with what is available
Checked to see that provider empty state is shown if available providers are insufficient
Checked functionality of Wizard Step 1

Looks good to me!

app/javascript/redux/common/targetResources/targetResourcesReducer.js

mturley · 2019-02-05T20:38:50Z

@michaelkro thanks so much! I really appreciate your feedback, and your help moving towards a nicer pattern for the common reducers.

Now go relax!

app/javascript/redux/common/targetResources/targetResourcesReducer.js

Conversion Host Configuration Wizard Step #1 (Location), and introducing common reducers (cherry picked from commit a0ee080) https://bugzilla.redhat.com/show_bug.cgi?id=1696423

simaishi · 2019-04-05T00:13:51Z

Hammer backport details:

$ git log -1
commit 5d8c17ae03a55c9c6316cddbd35f7cbc3cd8883c
Author: Mike Turley <mike.turley@alum.cs.umass.edu>
Date:   Tue Feb 5 15:54:52 2019 -0500

    Merge pull request #871 from mturley/conv-host-wizard-step1
    
    Conversion Host Configuration Wizard Step #1 (Location), and introducing common reducers
    
    (cherry picked from commit a0ee080c47502453d76ab6d7f0629471438c184f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1696423

simaishi · 2019-04-05T00:25:36Z

@mturley Can you please review the bakcport of this PR and make necessary changes in a new PR? I skipped renaming of providers.fixtures file as the file doesn't exist in hammer branch. But it looks some other changes are needed because of that...

/home/travis/build/ManageIQ/manageiq-v2v/app/javascript/redux/common/providers/__tests__/providersActions.test.js
  5:27  error  Unable to resolve path to module '../providers.fixtures'  import/no-unresolved

The conflits backporting this PR were:

	both modified:   app/javascript/react/screens/App/Overview/OverviewReducer.js
	both modified:   app/javascript/react/screens/App/Overview/__test__/OverviewReducer.test.js
	deleted by us:   app/javascript/react/screens/App/Overview/__test__/__snapshots__/OverviewReducer.test.js.snap
	added by them:   app/javascript/redux/common/providers/providers.fixtures.js

mturley · 2019-04-05T03:30:09Z

@simaishi sure, I'll take a look first thing in the morning.

mturley · 2019-04-05T14:34:14Z

@simaishi here's a PR to fix that problem: #929

I reviewed the other changes in your backport commit, everything else looks good. Thanks again for dealing with this mess :)

mturley added enhancement wip bugzilla needed v1.1 hammer/yes z-stream Sprint 19 labels Jan 31, 2019

codeclimate bot reviewed Jan 31, 2019

View reviewed changes

...ts/ConversionHostWizard/ConversionHostWizardLocationStep/ConversionHostWizardLocationStep.js Outdated Show resolved Hide resolved

...ts/ConversionHostWizard/ConversionHostWizardLocationStep/ConversionHostWizardLocationStep.js Outdated Show resolved Hide resolved

codeclimate bot reviewed Feb 1, 2019

View reviewed changes

mturley force-pushed the conv-host-wizard-step1 branch 2 times, most recently from b085fb6 to 50093c9 Compare February 1, 2019 20:35

codeclimate bot reviewed Feb 1, 2019

View reviewed changes

mturley force-pushed the conv-host-wizard-step1 branch from 932d24e to 74ecb8a Compare February 1, 2019 22:38

codeclimate bot reviewed Feb 1, 2019

View reviewed changes

...ts/ConversionHostWizard/ConversionHostWizardLocationStep/ConversionHostWizardLocationStep.js Show resolved Hide resolved

mturley changed the title ~~[WIP] Conversion Host Configuration Wizard Step #1 (Location)~~ Conversion Host Configuration Wizard Step #1 (Location) Feb 1, 2019

mturley removed the wip label Feb 1, 2019

mturley force-pushed the conv-host-wizard-step1 branch from ee90f7b to 2d29e21 Compare February 1, 2019 22:43

mturley requested a review from michaelkro February 1, 2019 22:46

michaelkro reviewed Feb 4, 2019

View reviewed changes

mturley changed the title ~~Conversion Host Configuration Wizard Step #1 (Location)~~ [WIP] Conversion Host Configuration Wizard Step #1 (Location) Feb 4, 2019

mturley added the wip label Feb 4, 2019

michaelkro reviewed Feb 4, 2019

View reviewed changes

codeclimate bot reviewed Feb 4, 2019

View reviewed changes

...ts/ConversionHostWizard/ConversionHostWizardLocationStep/ConversionHostWizardLocationStep.js Show resolved Hide resolved

mturley force-pushed the conv-host-wizard-step1 branch from c608caa to b6b46a0 Compare February 4, 2019 17:59

codeclimate bot reviewed Feb 4, 2019

View reviewed changes

...reens/App/Settings/screens/ConversionHostsSettings/__tests__/ConversionHostsSettings.test.js Show resolved Hide resolved

codeclimate bot reviewed Feb 4, 2019

View reviewed changes

app/javascript/redux/common/providers/providersReducer.js Show resolved Hide resolved

mturley added 5 commits February 5, 2019 15:36

Filter provider options by what's available

54cfb06

Update existing tests

d04f562

Refactor fetching providers to its own common reducer

608a5fb

Refactor fetching target clusters to its own common reducer

fd6e31a

Only import actions and fields we need from new reducer

ab27150

michaelkro approved these changes Feb 5, 2019

View reviewed changes

mturley force-pushed the conv-host-wizard-step1 branch from 3590834 to ab27150 Compare February 5, 2019 20:36

codeclimate bot reviewed Feb 5, 2019

View reviewed changes

app/javascript/redux/common/targetResources/targetResourcesReducer.js Show resolved Hide resolved

Fix silent failure in Overview index test

1666994

codeclimate bot reviewed Feb 5, 2019

View reviewed changes

app/javascript/redux/common/targetResources/targetResourcesReducer.js Show resolved Hide resolved

mturley merged commit a0ee080 into ManageIQ:master Feb 5, 2019

mturley deleted the conv-host-wizard-step1 branch February 5, 2019 20:54

mturley added bz Issues filed by QE or having a BZ and removed bugzilla needed wip labels Feb 27, 2019

mturley added v1.2 and removed v1.1 labels Mar 25, 2019

This was referenced Mar 27, 2019

[HAMMER] [BZ#1680521] Filter cloud_tenants to Openstack providers explicitly (prevents errors with RHV-4.3 providers) #921

Closed

[BZ#1680521] Filter cloud_tenants to Openstack providers explicitly (prevents errors with RHV-4.3 providers) #920

Merged

simaishi added hammer/backported and removed hammer/yes labels Apr 5, 2019

mturley mentioned this pull request Apr 5, 2019

[HAMMER] Fix backport of PR#871 by adding missing fixtures for providers tests #929

Merged

mturley mentioned this pull request Apr 25, 2019

Enable (un-hide) the Conversion Hosts tab of the Settings page #922

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conversion Host Configuration Wizard Step #1 (Location), and introducing common reducers #871

Conversion Host Configuration Wizard Step #1 (Location), and introducing common reducers #871

mturley commented Jan 31, 2019 •

edited

Loading

miq-bot commented Feb 1, 2019

mturley commented Feb 1, 2019

michaelkro Feb 4, 2019 •

edited

Loading

mturley Feb 4, 2019

mturley Feb 4, 2019

michaelkro Feb 4, 2019

mturley Feb 4, 2019

mturley Feb 4, 2019

michaelkro left a comment

mturley commented Feb 5, 2019

simaishi commented Apr 5, 2019

simaishi commented Apr 5, 2019

mturley commented Apr 5, 2019

mturley commented Apr 5, 2019

Conversion Host Configuration Wizard Step #1 (Location), and introducing common reducers #871

Conversion Host Configuration Wizard Step #1 (Location), and introducing common reducers #871

Conversation

mturley commented Jan 31, 2019 • edited Loading

miq-bot commented Feb 1, 2019

mturley commented Feb 1, 2019

michaelkro Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

mturley Feb 4, 2019

Choose a reason for hiding this comment

mturley Feb 4, 2019

Choose a reason for hiding this comment

michaelkro Feb 4, 2019

Choose a reason for hiding this comment

mturley Feb 4, 2019

Choose a reason for hiding this comment

mturley Feb 4, 2019

Choose a reason for hiding this comment

michaelkro left a comment

Choose a reason for hiding this comment

mturley commented Feb 5, 2019

simaishi commented Apr 5, 2019

simaishi commented Apr 5, 2019

mturley commented Apr 5, 2019

mturley commented Apr 5, 2019

mturley commented Jan 31, 2019 •

edited

Loading

michaelkro Feb 4, 2019 •

edited

Loading