[#515]Adds migration scheduling to complete mapping list #514

AllenBW · 2018-07-25T19:14:17Z

Fixes #515
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1564255

What we look like

OK OK so there's an issue with the using the kebab (described here) so we're using buttons for now. We now hide "stale migrations" from the completed list, those migrations that happened in the past, and allow the user to schedule anew but there's an issue with the api serializing the keys (?) or something like that, so rescheduling an already failed migration doesn't so much work @bdunne is looking into da issue...

its a long video, but worth a watch! Its now an old video, but still worth watching

Edit: if this seems clunky, having to unschedule to re schedule, s' something that can be improved upon, but I don't want to make any promises before that it could be done by friday, so here seems like a good stopping spot... 🤞

AllenBW · 2018-07-26T13:31:47Z

@Yadnyawalkya should this be gaprindashvili/yes? piggy back off that same bz?

vconzola · 2018-07-26T15:54:04Z

@AllenBW I'm a little confused as to why Unschedule is required before the plan can be scheduled again. Does running the plan not get rid of its schedule? I think this will be very confusing to the user. If an Unschedule button needs to appear for every completed plan, then I don't want to merge this before GA. Let's take the time to do it right.

AllenBW · 2018-07-26T16:20:33Z

@vconzola no need for confusion, When we have a schedule on the record, something that was used to kick off a migration, it stays on the record until removed. But that makes sense, we might want to know when the last migration was scheduled to run, or maybe not?

UX guidance on how to "do this right" would be most appreciated. Do we clear the past schedule upon migration completion, losing that record? Provide an edit button to modify the existing schedule? Do we ignore all past schedules and treat it as a new one?

vconzola · 2018-07-26T17:42:11Z

@AllenBW I think the best UX is to clear the past schedule upon migration completion so that the options on a Completed plan become Schedule, Retry, and Archive with Archive being hidden under a kebab. I'll create a mockup.

vconzola · 2018-07-26T17:46:21Z

@AllenBW Thinking about this does make me think of another edge case, however. Any idea what happens today if a scheduled migration fails to run for some reason, e.g., network problem? Do we indicate that in the UI at all? Leads me to think that we need an error in the plan list view and also to prioritize rescheduling migrations (vs. unschedule and schedule again like we have today).

AllenBW · 2018-07-26T17:50:20Z

@vconzola 👍 ok can clear upon completion, will have that change up in a few shakes. To your other question I believe this would fail just as a normal migration, to my knowledge we aren't provided any indication as to why a schedule failed to run.

vconzola · 2018-07-26T18:05:02Z

@AllenBW I'm talking about the case where the request is never sent so the migration never starts at the scheduled time. Is this possible and a case we need to consider?

AllenBW · 2018-07-26T18:19:42Z

@vconzola Not sure I follow, the request is never sent? All api errors are handled by miq, so if theres an issue with setting the schedule we'll know then. If your saying what happens in the case the schedule fails to run? Not sure we get any feedback to that effect.

vconzola · 2018-07-26T18:37:29Z

@AllenBW That's the case I was thinking of - the schedule fails to run. Do we need to address this possibility?

AllenBW · 2018-07-26T18:41:21Z

@vconzola how abouts we chat this over in the standup 👍

vconzola · 2018-07-26T19:42:25Z

@AllenBW Here's a mockup with a schedule button added for failed Completed plans and the Archive feature moved under a kebab.

mturley · 2018-07-26T21:16:13Z

Late to the party, but this was a red flag for me:

Do we clear the past schedule upon migration completion, losing that record?

I agree with @vconzola that the remaining options should be Schedule, Retry, and Archive, and we shouldn't have to unschedule. But I don't know how I feel about losing the history of what happened. Maybe a new feature for down the line should be Migration History? I can just imagine a user realizing something was wrong with a completed migration and wanting to go look at how it was configured.

AllenBW · 2018-07-26T23:17:46Z

@vconzola

Archive as a kebab is a problem because we have a whole list item wrapped in a click event. So when you click on the kebab, rather than seeing the dropdown you are redirected to the completed migration. Working on getting the kebab to behave like the rest of the buttons, independent of the row click event, but just in case thats not immediately possible... we can either....

change the completed migration name into the redirect to the page, removing it from the whole row
swap the archive kebab for the tried and true button till this gets resolved

mturley · 2018-07-27T19:23:02Z

@AllenBW have you tried stopping propagation on the kebab's event handler? event.stopPropagation() is your friend when you have event handlers on nested elements. https://developer.mozilla.org/en-US/docs/Web/API/Event/stopPropagation

AllenBW · 2018-07-27T19:34:26Z

@mturley yes. and about a billon other things.

AllenBW · 2018-07-27T19:36:05Z

ManageIQ/manageiq-api#436 is the pr we need for this to work...

Is ready for review from other parties whenever they can spare the cycles

michaelkro · 2018-07-27T19:45:54Z

app/javascript/react/screens/App/Overview/OverviewReducer.js

@@ -100,7 +100,7 @@ export const initialState = Immutable({
  isFetchingNetworks: false,
  isRejectedNetworks: false,
  scheduleMigrationModal: false,
-  scheduleMigrationPlanId: null,
+  scheduleMigrationPlan: {},


I think we want to set the default as an empty string here due to this line

eeeeek nope, good catch, that needs to be an object, cuz the plan is an object

List components share buttons rather than duplicating in each

mturley

Little thing that might cause an error, not sure if this case is possible though.

mturley · 2018-07-27T20:09:32Z

app/javascript/react/screens/App/Overview/OverviewActions.js

+        scheduleTime,
+        plan: { id: planId }
+      } = payload;
+      const scheduleId = (payload.plan.schedules && payload.plan.schedules[0].id) || null;


This will be a TypeError if payload.plan.schedules is an empty array (truthy but [0] is undefined). Not sure if we'll ever hit that case, but we should maybe do:

const firstSchedule = payload.plan.schedules && payload.plan.schedules[0]; const scheduleId = firstSchedule ? firstSchedule.id : null;

if it exists, as in if payload.plan.schedules passes, there will be something inside 👍

mturley · 2018-07-27T20:17:52Z

app/javascript/react/screens/App/Overview/OverviewActions.js

+        body = scheduleTime
+          ? {
+              action: 'edit',
+              resource: { run_at: { ...payload.plan.schedules[0].run_at, start_time: scheduleTime } }


A nice DRY firstSchedule could be used here as well

mturley · 2018-07-27T20:19:11Z

app/javascript/react/screens/App/Overview/components/Migrations/MigrationsNotStartedList.js

-                                {__('Unschedule')}
-                              </Button>
-                            )}
+                            <ScheduleMigrationButton


mturley

Aside from the one little potential issue (maybe not even an issue?) this looks great to me.

michaelkro

Tested against the pending manageiq-api branch, and looks good to me 🎉

michaelkro · 2018-07-30T23:31:26Z

Looks like ManageIQ/manageiq-api#436 has been merged. I've looked at the new kebab code, and it looks good to me.

In the interest of time, I've gone ahead and taken a look at the CI failure. We can replace this line with something like

const archiveButton = item.find('a').filterWhere(link => link.text() === 'Archive');

to fix the failing test case

ESlint rightfully doesn't like this onclick, but its necessary to allow for the kebab button to work as intended rather than being overridden by the parent onclick

AllenBW · 2018-07-31T00:32:13Z

Updated! @michaelkro

michaelkro · 2018-07-31T00:43:52Z

🎉

…uling-to-completed-list [#515]Adds migration scheduling to complete mapping list (cherry picked from commit 5e21f6d) https://bugzilla.redhat.com/show_bug.cgi?id=1608351

simaishi · 2018-07-31T23:23:11Z

Gaprindashvili backport details:

$ git log -1
commit ca3881091f96746d3a8eac9087b58a382879b307
Author: Michael Ro <mikerodev@gmail.com>
Date:   Mon Jul 30 20:43:56 2018 -0400

    Merge pull request #514 from AllenBW/enhancement/adds-migration-scheduling-to-completed-list
    
    [#515]Adds migration scheduling to complete mapping list
    (cherry picked from commit 5e21f6dd3f4cd95cdb2becd4852cf844a79efb89)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1608351

AllenBW assigned michaelkro Jul 25, 2018

AllenBW requested review from AparnaKarve, michaelkro and mturley July 25, 2018 19:14

AllenBW added the Sprint 11 label Jul 25, 2018

AllenBW added the enhancement label Jul 25, 2018

AllenBW changed the title ~~Adds migration scheduling to complete mapping list~~ [WIP][#515]Adds migration scheduling to complete mapping list Jul 26, 2018

AllenBW added the wip label Jul 26, 2018

AllenBW force-pushed the enhancement/adds-migration-scheduling-to-completed-list branch 2 times, most recently from 5154132 to 7ac47fb Compare July 27, 2018 14:35

AllenBW removed the wip label Jul 27, 2018

AllenBW changed the title ~~[WIP][#515]Adds migration scheduling to complete mapping list~~ [#515]Adds migration scheduling to complete mapping list Jul 27, 2018

AllenBW force-pushed the enhancement/adds-migration-scheduling-to-completed-list branch 2 times, most recently from cedc7ff to 588e152 Compare July 27, 2018 17:56

michaelkro reviewed Jul 27, 2018

View reviewed changes

Abstracts migration buttons into component, adds scheduling to completed

40734eb

List components share buttons rather than duplicating in each

AllenBW force-pushed the enhancement/adds-migration-scheduling-to-completed-list branch from 588e152 to 40734eb Compare July 27, 2018 19:53

mturley reviewed Jul 27, 2018

View reviewed changes

michaelkro approved these changes Jul 27, 2018

View reviewed changes

michaelkro mentioned this pull request Jul 30, 2018

[#387] View Migrations when there are no infra mappings #439

Merged

Replaces archive button with kebab

0c65c6f

ESlint rightfully doesn't like this onclick, but its necessary to allow for the kebab button to work as intended rather than being overridden by the parent onclick

AllenBW force-pushed the enhancement/adds-migration-scheduling-to-completed-list branch from 7b0ee94 to 0c65c6f Compare July 31, 2018 00:31

michaelkro merged commit 5e21f6d into ManageIQ:master Jul 31, 2018

AllenBW deleted the enhancement/adds-migration-scheduling-to-completed-list branch July 31, 2018 12:43

michaelkro added gaprindashvili/yes blocker labels Jul 31, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#515]Adds migration scheduling to complete mapping list #514

[#515]Adds migration scheduling to complete mapping list #514

AllenBW commented Jul 25, 2018 •

edited by michaelkro

Loading

AllenBW commented Jul 26, 2018

vconzola commented Jul 26, 2018

AllenBW commented Jul 26, 2018

vconzola commented Jul 26, 2018

vconzola commented Jul 26, 2018

AllenBW commented Jul 26, 2018

vconzola commented Jul 26, 2018

AllenBW commented Jul 26, 2018

vconzola commented Jul 26, 2018

AllenBW commented Jul 26, 2018

vconzola commented Jul 26, 2018 •

edited

Loading

mturley commented Jul 26, 2018

AllenBW commented Jul 26, 2018 •

edited

Loading

mturley commented Jul 27, 2018

AllenBW commented Jul 27, 2018

AllenBW commented Jul 27, 2018

michaelkro Jul 27, 2018 •

edited

Loading

AllenBW Jul 27, 2018

mturley left a comment

mturley Jul 27, 2018

AllenBW Jul 27, 2018

mturley Jul 27, 2018

mturley Jul 27, 2018

mturley left a comment

michaelkro left a comment

michaelkro commented Jul 30, 2018

AllenBW commented Jul 31, 2018

michaelkro commented Jul 31, 2018

simaishi commented Jul 31, 2018

[#515]Adds migration scheduling to complete mapping list #514

[#515]Adds migration scheduling to complete mapping list #514

Conversation

AllenBW commented Jul 25, 2018 • edited by michaelkro Loading

What we look like

its a long video, but worth a watch! Its now an old video, but still worth watching

AllenBW commented Jul 26, 2018

vconzola commented Jul 26, 2018

AllenBW commented Jul 26, 2018

vconzola commented Jul 26, 2018

vconzola commented Jul 26, 2018

AllenBW commented Jul 26, 2018

vconzola commented Jul 26, 2018

AllenBW commented Jul 26, 2018

vconzola commented Jul 26, 2018

AllenBW commented Jul 26, 2018

vconzola commented Jul 26, 2018 • edited Loading

mturley commented Jul 26, 2018

AllenBW commented Jul 26, 2018 • edited Loading

mturley commented Jul 27, 2018

AllenBW commented Jul 27, 2018

AllenBW commented Jul 27, 2018

michaelkro Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

AllenBW Jul 27, 2018

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

mturley Jul 27, 2018

Choose a reason for hiding this comment

AllenBW Jul 27, 2018

Choose a reason for hiding this comment

mturley Jul 27, 2018

Choose a reason for hiding this comment

mturley Jul 27, 2018

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

michaelkro left a comment

Choose a reason for hiding this comment

michaelkro commented Jul 30, 2018

AllenBW commented Jul 31, 2018

michaelkro commented Jul 31, 2018

simaishi commented Jul 31, 2018

AllenBW commented Jul 25, 2018 •

edited by michaelkro

Loading

vconzola commented Jul 26, 2018 •

edited

Loading

AllenBW commented Jul 26, 2018 •

edited

Loading

michaelkro Jul 27, 2018 •

edited

Loading