Conversion Host Configuration - List view #3 - Polling for tasks and rendering enable/disable status

[mturley]

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

Closes #862.
Closes #856.

To see tasks in the various states, you can test against this database: https://drive.google.com/open?id=1l26rbhp36v6lCfpwKGASxEJknKnI32oe

Screens

Implementation Details

• This PR implements polling for the conversion host enable/disable tasks, similar to what we have on the Migration Plans page for plan request tasks, but with some improvements over that implementation that we might want to port back over to the Plans page in the future:

  • The startPolling method is idempotent, and starts with an immediate fetch. We don't need to explicitly fetch resources before we start polling, and we can call startPolling multiple times without creating multiple timers (instead it fetches resources immediately and restarts the timer, effectively skipping to the end of the current polling interval).
  • To prevent polling from disrupting a modal, we simply check for modal status in the polling handler and do nothing if there is an open modal. This way, we don't have to call stopPolling when the modal opens, simplifying the logic in componentDidUpdate.
  • To see the results of the action taken in a modal (the wizard or the remove modal), we don't need to have the modal explicitly fetch the same resources we poll for. Instead, we detect when a modal closes and call startPolling again (without having stopped it), which resets the timer and fetches the new resources immediately. The modal code doesn't need to handle anything outside its scope, and we don't need to duplicate the same fetches in many places like we do with plans.
  • We don't need a willUnmount class property because we check for the presence of a timer before we reset polling in componentDidUpdate.
  • By using React component state instead of a class property for hasMadeInitialFetch, we can drive the spinner from that value without caring about all the isFetching properties.

• I renamed some existing redux properties to make them a little easier to distinguish:

  • isRejectedConversionHost -> isRejectedDeletingConversionHost
  • isRejectedConversionHosts -> isRejectedFetchingConversionHosts
  • showConversionHostDeleteModal -> conversionHostDeleteModalVisible (to distinguish it from showConversionHostDeleteModalAction)

• In the Delete Modal:

  • I renamed the "Delete Conversion Host" terminology to "Remove Conversion Host" to match the mockup, because I missed this when reviewing Conversion Host: list view phase 2 #882
  • I disabled the Delete button while deletion is in progress

• In order to display the conversion hosts in progress of being configured (which do not have conversion_host objects yet in the API), and the status/log info for configured conversion hosts (which need an associated completed task to get that information), we have to:

  • load all conversion host enable and disable tasks
  • parse the task name with a regular expression to get the operation, resource name, type and id
  • index all the tasks by resource type, resource id, and operation to make association easier
  • associate existing conversion hosts with any tasks that contain their resource type/id
  • get any in-progress or failed tasks that are not associated with a conversion host
  • combine these filtered tasks and the conversion hosts with associated task metadata.

  This mess is accomplished by the helpers in Settings/helpers.js and done outside the components (parsing and indexing happens in the reducer, associating and combining happens in ConversionHostsSettings/index.js).

• It is important to note that the enable tasks in progress are filtered such that only the most recent task for each resource is included in the list. This way, if enablement fails and the user manually retries, when the new task is created the old task will no longer display in the list, and we'll never end up with one resource showing up in multiple list items.

• Because the back end support is not ready, the Remove and Retry buttons on a failed enablement task and the Download Log action on any task have been hidden (disabled by boolean flags we should remove when the implementation is ready). To track these, I opened Conversion Host Enablement - List View - Remove button on a failed enablement task - Need API request to mark failed task as acknowledged #900, Conversion Host Enablement - List View - Retry button on a failed enablement task - Need API support #901 and Conversion Host Enablement - List View - Download Log action - Need API support #902 respectively.

• To prevent requesting enablement of a conversion host that is already enabled or in the process of being enabled, this PR filters those hosts out of the Hosts step of the wizard.

• In the interest of time, I'm not letting the lack of unit tests block this PR, but these complex helper functions would benefit a lot from unit testing. We should write these tests as soon as possible. (see Conversion Host Enablement - Unit Tests #884)

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[miq-bot]

Checked commits mturley/manageiq-v2v@ee33fdc~...197e0de 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. 🍰

[himdel]

Adding yet another place with a copy of this code?

app/javascript/react/screens/App/Overview/Overview.js , app/javascript/react/screens/App/Mappings/Mappings.js and app/javascript/react/screens/App/Plan/Plan.js all do the same thing here.... sounds like something that should be abstracted away

[mturley]

@himdel I noticed that as well, but I was hesitant to refactor those as part of this PR since they are... well.. a bit fragile. I figured we should go back and revisit the polling abstraction later on, since we need to get this in for the build on March 15th. If you feel strongly about it though, I can take a look and see what it would take to abstract this away.

One reason I didn't do it right away is that this new polling implementation has some subtle differences, most notably the fact that it restarts automatically when a modal closes (instead of those modals forcing another check when they close of the same resources we poll for). So the refactor would involve changing that behavior too.

[himdel]

Agreed @mturley :)

I don't think there's any pressing need to fix this in this PR, as long as things are moving forward.

Just wanted to make sure the pattern won't be spreading unchecked, better catch this sooner than later :)

[mturley]

I agree. Opened #904 to track it, I'll put it on my list :)

[simaishi]

Hammer backport details:

$ git log -1
commit 4f06604d6511d4fe36cf6668e11b14c4e33ae3ec
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Tue Mar 12 12:36:01 2019 +0100

    Merge pull request #889 from mturley/conv-host-tasks-polling
    
    Conversion Host Configuration - List view #3 - Polling for tasks and rendering enable/disable status
    
    (cherry picked from commit d4ea6f601f4e6b8af24e6e997e626d92b4d0b616)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1696423