Skip to content
This repository has been archived by the owner on Sep 5, 2024. It is now read-only.

[WIP][#370] Cancel a migration's tasks #511

Closed
wants to merge 4 commits into from

Conversation

priley86
Copy link
Member

@priley86 priley86 commented Jul 20, 2018

fixes #370
fixes https://bugzilla.redhat.com/show_bug.cgi?id=1564257

refactors #509

Work completed:

  1. Disable Cancel Migration if no tasks are selected:

screen shot 2018-07-20 at 2 18 00 pm

  1. Enable Cancel Migration after a task/tasks have been selected (ignore cancel option on failed task, that should be prevented in the future):

screen shot 2018-07-20 at 2 18 09 pm

  1. Add Cancel Migrations confirm Modal showing the tasks selected:

screen shot 2018-07-20 at 2 18 23 pm

Work remaining:

  • Select All - what behavior should this have @vconzola ? Do we select all in the currently visible paginated page of tasks? Or do we select all tasks (across all pages)? Wasn't positive based on UX if that should be a dropdown with both options for the user (both are possible).
  • Add of the {x} of {y} selected messaging beneath the Toolbar.
  • Add cancellation icons to indicate task has been cancelled. This would be based on the task['options']['cancellation_request_id'] already in our existing tasks list.
  • Add reducers to handle the Cancel confirm actions and the error/success notifications. @vconzola can you spell these out a bit further? I can take a stab at this based on what I'm understanding about monitoring the cancellation requests.

Edge case discovered:

  • if the Plan Details now displays completed VMs from previous requests, these should be disabled from cancellation (along w/ other completed VMs). @vconzola would just like to confirm that we are only allowing "Cancel" on VMs currently in pending/in progress?

@mturley mturley changed the title [WIP] Cancel a migration's tasks [WIP][#370] Cancel a migration's tasks Jul 20, 2018
@bthurber
Copy link

@mturley
Copy link
Contributor

mturley commented Jul 23, 2018

I'm now unblocked and back to working on this. I added the "X of Y selected" piece under the cancel button. Working on Select All next.

kmydexlgzx

michaelkro and others added 3 commits July 24, 2018 20:40
[WIP] Import DropdownButton

[WIP] Conditionally render log links

[WIP] Update mock data

[WIP] Update log link rendering

[WIP] Fetch running playbook

[WIP] Display playbook name in popover

[WIP] i18n

[WIP] Fix mocks

[WIP] Update fixtures

[WIP] Update playbook status

[WIP] Update popover content

[WIP] Update fetching ansible playbook template

[WIP] Fix Popover formatting

[WIP] Use status constants

[WIP] Test errors

[WIP] Real action for fetching playbook template

[WIP] Mock fetching OrchestrationStack

[WIP] Remove mock responses and data
Copy link
Contributor

@michaelkro michaelkro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple things that resulted from the rebase

notificationType: 'error',
persistent: true,
actionEnabled: false
});
reject(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will result with duplicate error messages as we started getting error notification for free when we moved away from using axios to make requests to the MIQ API. (review comment)

if (task.options.playbooks) {
const hasPlaybookService = task.options.playbooks;

if (hasPlaybookService) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We removed the unnecessary const declaration and moved task.options.playbooks directly into the conditional here (review comment)

@@ -247,7 +247,7 @@ class PlanRequestDetailList extends React.Component {
overlayTriggerClick = task => {
if (task.options.playbooks) {
const playbookStatuses = task.options.playbooks;
let runningPlaybook = null;
let runningPlaybook;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set the default value to null here (review comment)

@mturley
Copy link
Contributor

mturley commented Jul 26, 2018

This work is now being handled in another PR.

PR shuffle 👉 #517 👉

@mturley mturley closed this Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User can cancel a migration in progress
5 participants