(core) add day selector to execution window #2701

icfantv · 2016-09-16T00:28:40Z

add days-of-week selector widget to pipeline execution restriction window

icfantv · 2016-09-16T00:31:17Z

@spinnaker/netflix-reviewers @spinnaker/google-reviewers, PTAL, this is a first attempt at switching to ES6 module loading and class syntax for items which will, amongst other things, allow us to move away from using require everywhere and switch to a more modern code design.

anotherchrisberry

A couple of things:

We will want to include something in the executionWindowDetails.html to indicate to the user which days are restricted, if any.
This has to work with existing pipelines, where days will not be defined. Did you test it against an existing stage with an execution window? You can do that by creating a pipeline with a stage in prestaging, then adding an execution window, then checking it locally to verify the days field is correctly populated on the stage when toggling things.

Also, can you include a screenshot to show what the component looks like on the screen?

anotherchrisberry · 2016-09-16T15:59:33Z

...ts/modules/core/pipeline/config/stages/executionWindows/ExecutionWindowDayPickerComponent.js

+  }
+
+  $onInit() {
+    console.log('executing onInit');


remove console statement

anotherchrisberry · 2016-09-16T16:00:36Z

...ts/modules/core/pipeline/config/stages/executionWindows/ExecutionWindowDayPickerComponent.js

+class ExecutionWindowDayPickerController {
+
+  constructor() {
+    this.DAYS = [{


It's a little confusing the have both DAYS and days on the controller. Maybe change this to dayOptions?

anotherchrisberry · 2016-09-16T16:01:58Z

...ts/modules/core/pipeline/config/stages/executionWindows/ExecutionWindowDayPickerComponent.js

+    days: '='
+  },
+  controller: ExecutionWindowDayPickerController,
+  controllerAs: 'dayPicker',


We are not using controllerAs syntax in components - better to stick with the default ($ctrl).

anotherchrisberry · 2016-09-16T16:04:16Z

...ts/modules/core/pipeline/config/stages/executionWindows/ExecutionWindowDayPickerComponent.js

+const MODULE_NAME = 'spinnaker.core.pipeline.stage.executionWindows.dayPicker';
+angular.module(MODULE_NAME, [])
+  .component('executionWindowDayPicker', EXECUTION_WINDOW_DAY_PICKER_COMPONENT);
+export default MODULE_NAME;


To be consistent with the rest of the code base, it's probably better to rewrite this block (L76-88) as:

module.exports = angular.module('spinnaker.core.pipeline.stage.executionWindows.dayPicker', []) .component('executionWindowDayPicker', { bindings: { days: '=' }, controller: ExecutionWindowDayController, templateUrl: require('./executionWindowDayPicker.component.html') });

anotherchrisberry · 2016-09-16T16:05:06Z

.../modules/core/pipeline/config/stages/executionWindows/ExecutionWindowDayPickerComponent.html

@@ -0,0 +1,13 @@
+<div class="btn-group">


File should be named executionWindowDayPicker.component.html to be consistent with other components.

anotherchrisberry · 2016-09-16T16:05:32Z

...ts/modules/core/pipeline/config/stages/executionWindows/ExecutionWindowDayPickerComponent.js

@@ -0,0 +1,89 @@
+import angular from 'angular';


Should be renamed executionWindowDayPicker.component.js to be consistent with other components.

anotherchrisberry · 2016-09-16T16:05:59Z

...dules/core/pipeline/config/stages/executionWindows/ExecutionWindowDayPickerComponent.spec.js

@@ -0,0 +1,87 @@
+import dayPickerComponent from './ExecutionWindowDayPickerComponent';
+
+fdescribe('Component:  Execution Window Day Picker', () => {


fdescribe should just be describe

anotherchrisberry · 2016-09-16T16:09:09Z

...ts/modules/core/pipeline/config/stages/executionWindows/ExecutionWindowDayPickerComponent.js

+  constructor() {
+    this.DAYS = [{
+      dow: 'sunday',
+      abbr: 'Sun'


To be consistent with the rest of the codebase, these should probably be key and label

anotherchrisberry · 2016-09-16T16:10:13Z

...dules/core/pipeline/config/stages/executionWindows/ExecutionWindowDayPickerComponent.spec.js

+    const days = new Set(ctrl.days);
+    for (let i = 2; i < 7; i++) {
+      expect(days.has(i)).toBe(true);
+    }


These four lines read better as:

expect(ctrl.days).toEqual([2,3,4,5,6]);

@anotherchrisberry Aside from being cleaner, this would imply that order matters for the days property, which, on the client-side, it does not. Can you confirm that days is order-dependent on the server side? In fact, I'm wondering if this should be a Set... thoughts?

anotherchrisberry · 2016-09-16T16:15:44Z

...ts/modules/core/pipeline/config/stages/executionWindows/ExecutionWindowDayPickerComponent.js

+        updated.push(index + 1);
+      }
+    });
+    this.days = updated;


Does this get applied to the underlying stage? Seems like, by reassigning the value on the controller here, it won't end up on the stage, and the stage's days field will remain unchanged.

@anotherchrisberry yes, this works and i've confirmed across page reloads that saved values are re-retrieved and set correctly.

icfantv · 2016-09-16T22:38:20Z

@anotherchrisberry @zanthrash updated per comments/discussions. PTAL. Thanks.

anotherchrisberry · 2016-09-16T23:40:52Z

Looking better - can you add something to the execution details view around https://github.com/spinnaker/deck/blob/master/app/scripts/modules/core/pipeline/config/stages/executionWindows/executionWindowsDetails.html#L7 to show which days (if any) the window is restricted to?

anotherchrisberry · 2016-09-16T23:37:25Z

app/scripts/modules/core/pipeline/config/stages/executionWindows/executionWindows.html

    </div>
  </div>
-
-</div>
+git st</div>


remove git st

yea, i caught this afterwards...not sure what happend.

ok, well....i KNOW what happened....

icfantv · 2016-09-17T00:09:50Z

@anotherchrisberry this isn't quite ready yet - i found an issue with the revert button that required completely changing how we initially render the buttons, set the states, and manage the internal model due to $onChanges only being fired when a reference to a collection changes, rather than the contents due to the performance ramifications of doing a deep watch on the collections as reported here.

icfantv · 2016-09-17T04:12:13Z

The huge advantage that this exposed (the $onChanges not being fired, was that we now effectively bind the days model directly to the UI and we no longer are using the UIBS checkbox buttons.

icfantv · 2016-09-19T18:53:03Z

New Execution Window View:

Details View:

icfantv · 2016-09-19T18:54:40Z

@anotherchrisberry @zanthrash i think we're done here, PTAL.

anotherchrisberry

LGTM

anotherchrisberry · 2016-09-19T19:05:30Z

...s/modules/core/pipeline/config/stages/executionWindows/executionWindowsDetails.controller.js

+      const days = $scope.stage.context.restrictedExecutionWindow.days;
+      if (days && (days.length > 0)) {
+        const daysAsText = replaceDays(days);
+        dayText = daysAsText.toString().replace(/,/g, ', ');


If you weren't using a Set above, you could just do days.join(', ') here. But this works, too, and I know how much you really like your Sets.

I'll fix. I'd forgotten about join.

* add days-of-week selector widget to pipeline execution restriction window

icfantv force-pushed the execution_window branch from 69cd361 to f72f75d Compare September 16, 2016 16:12

anotherchrisberry reviewed Sep 16, 2016

View reviewed changes

icfantv force-pushed the execution_window branch from f72f75d to 5bf333a Compare September 16, 2016 22:36

anotherchrisberry suggested changes Sep 16, 2016

View reviewed changes

icfantv force-pushed the execution_window branch from 5bf333a to f6e8505 Compare September 17, 2016 00:11

icfantv force-pushed the execution_window branch 3 times, most recently from fd3a89c to 1b28a1a Compare September 19, 2016 18:51

icfantv force-pushed the execution_window branch 2 times, most recently from 4844b07 to 67dfa59 Compare September 19, 2016 18:57

anotherchrisberry reviewed Sep 19, 2016

View reviewed changes

icfantv force-pushed the execution_window branch from 67dfa59 to 6818334 Compare September 19, 2016 19:05

anotherchrisberry approved these changes Sep 19, 2016

View reviewed changes

(core) add day selector to execution window

f2c828b

* add days-of-week selector widget to pipeline execution restriction window

icfantv force-pushed the execution_window branch from 6818334 to f2c828b Compare September 19, 2016 19:08

icfantv merged commit 74fc68c into spinnaker:master Sep 19, 2016

icfantv deleted the execution_window branch September 19, 2016 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(core) add day selector to execution window #2701

(core) add day selector to execution window #2701

icfantv commented Sep 16, 2016

icfantv commented Sep 16, 2016 •

edited

Loading

anotherchrisberry left a comment

anotherchrisberry Sep 16, 2016

icfantv Sep 16, 2016

anotherchrisberry Sep 16, 2016

anotherchrisberry Sep 16, 2016

anotherchrisberry Sep 16, 2016

anotherchrisberry Sep 16, 2016

anotherchrisberry Sep 16, 2016

anotherchrisberry Sep 16, 2016

icfantv Sep 16, 2016

anotherchrisberry Sep 16, 2016

anotherchrisberry Sep 16, 2016

icfantv Sep 16, 2016 •

edited

Loading

anotherchrisberry Sep 16, 2016

icfantv Sep 16, 2016

icfantv commented Sep 16, 2016

anotherchrisberry commented Sep 16, 2016

anotherchrisberry Sep 16, 2016

icfantv Sep 17, 2016

icfantv Sep 17, 2016

icfantv commented Sep 17, 2016

icfantv commented Sep 17, 2016

icfantv commented Sep 19, 2016 •

edited

Loading

icfantv commented Sep 19, 2016

anotherchrisberry left a comment

anotherchrisberry Sep 19, 2016

icfantv Sep 19, 2016

icfantv Sep 19, 2016

		@@ -0,0 +1,87 @@
		import dayPickerComponent from './ExecutionWindowDayPickerComponent';

		fdescribe('Component: Execution Window Day Picker', () => {

(core) add day selector to execution window #2701

(core) add day selector to execution window #2701

Conversation

icfantv commented Sep 16, 2016

icfantv commented Sep 16, 2016 • edited Loading

anotherchrisberry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icfantv Sep 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icfantv commented Sep 16, 2016

anotherchrisberry commented Sep 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icfantv commented Sep 17, 2016

icfantv commented Sep 17, 2016

icfantv commented Sep 19, 2016 • edited Loading

icfantv commented Sep 19, 2016

anotherchrisberry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icfantv commented Sep 16, 2016 •

edited

Loading

icfantv Sep 16, 2016 •

edited

Loading

icfantv commented Sep 19, 2016 •

edited

Loading