Merge pull request #744 from cheld/expose-external

Redesign of expose service externally

Author: bryk

src/app/frontend/deploy/deployfromsettings.html

@@ -112,17 +112,6 @@
   </kd-user-help>
 </kd-help-section>
 
-<kd-help-section>
-  <div class="md-block">
-    <md-checkbox ng-model="ctrl.isExternal" class="md-primary"
-                 ng-model-options="{ debounce: { 'default': 500, 'blur': 0 } }">
-      Expose service externally
-    </md-checkbox>
-  </div>
-  <kd-user-help>
-  </kd-user-help>
-</kd-help-section>
-
 <!-- advanced options -->
 <div ng-show="ctrl.isMoreOptionsEnabled()">
   <kd-help-section>

src/app/frontend/deploy/portmappings.html

@@ -14,13 +14,21 @@
 limitations under the License.
 -->
 
+<md-input-container class="md-block">
+  <label>Service</label>
+<md-select ng-model="ctrl.serviceType" ng-change="ctrl.changeServiceType()">
+  <md-option ng-repeat="serviceType in ctrl.serviceTypes" ng-value="serviceType">
+    {{serviceType.label}}
+  </md-option>
+</md-select>
+</md-input-container>
 <div ng-repeat="portMapping in ctrl.portMappings">
   <ng-form name="portMappingForm" layout="row">
     <md-input-container flex="50" class="kd-deploy-input-row">
       <label>Port</label>
       <input ng-model="portMapping.port" ng-change="ctrl.checkPortMapping(portMappingForm, $index)"
-             type="number" min="1" max="65535" name="port">
-      <ng-messages for="portMappingForm.port.$error" role="alert" multiple>
+             type="number" min="1" max="65535" name="port" ng-required="ctrl.isFirst($index)">
+      <ng-messages for="portMappingForm.port.$error" role="alert" >
         <ng-message when="number">Port must be an integer.</ng-message>
         <ng-message when="min">Port must greater than 0.</ng-message>
         <ng-message when="max">Port must less than 65536.</ng-message>
@@ -32,8 +40,8 @@
       <label>Target port</label>
       <input ng-model="portMapping.targetPort"
              ng-change="ctrl.checkPortMapping(portMappingForm, $index)"
-             type="number" min="1" max="65535" name="targetPort">
-      <ng-messages for="portMappingForm.targetPort.$error" role="alert" multiple>
+             type="number" min="1" max="65535" name="targetPort" ng-required="ctrl.isFirst($index)">
+      <ng-messages for="portMappingForm.targetPort.$error" role="alert" >
         <ng-message when="number">Target port must be an integer.</ng-message>
         <ng-message when="min">Target port must greater than 0.</ng-message>
         <ng-message when="max">Target port must less than 65536.</ng-message>

src/app/frontend/deploy/portmappings_controller.js

@@ -12,6 +12,25 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+/** @final */
+class ServiceType {
+  constructor(label, external) {
+    /** @export {string} */
+    this.label = label;
+    /** @export {boolean} */
+    this.external = external;
+  }
+}
+
+/** @type {ServiceType} */
+export const NO_SERVICE = new ServiceType('None', false);
+
+/** @type {ServiceType} */
+export const INT_SERVICE = new ServiceType('Internal', false);
+
+/** @type {ServiceType} */
+export const EXT_SERVICE = new ServiceType('External', true);
+
 /**
  * Controller for the port mappings directive.
  *
@@ -24,7 +43,7 @@ export default class PortMappingsController {
      * Two way data binding from the scope.
      * @export {!Array<!backendApi.PortMapping>}
      */
-    this.portMappings = [this.newEmptyPortMapping_(this.protocols[0])];
+    this.portMappings = [];
 
     /**
      * Initialized from the scope.
@@ -33,10 +52,22 @@ export default class PortMappingsController {
     this.protocols;
 
     /**
-     * Initialized from the scope.
+     * Binding to outer scope.
      * @export {boolean}
      */
     this.isExternal;
+
+    /**
+     * Available service types
+     * @export {!Array<ServiceType>}
+     */
+    this.serviceTypes = [NO_SERVICE, INT_SERVICE, EXT_SERVICE];
+
+    /**
+     * Selected service type. Binding to outer scope.
+     * @export {ServiceType}
+     */
+    this.serviceType;
   }
 
   /**
@@ -126,4 +157,27 @@ export default class PortMappingsController {
    * @private
    */
   isPortMappingFilledOrEmpty_(portMapping) { return !portMapping.port === !portMapping.targetPort; }
+
+  /**
+   * Change the service type. Port mappings are adjusted and external flag.
+   * @export
+   */
+  changeServiceType() {
+    // add or remove port mappings
+    if (this.serviceType === NO_SERVICE) {
+      this.portMappings = [];
+    } else if (this.portMappings.length === 0) {
+      this.portMappings = [this.newEmptyPortMapping_(this.protocols[0])];
+    }
+
+    // set flag
+    this.isExternal = this.serviceType.external;
+  }
+
+  /**
+   * Returns true if the given port mapping is the first in the list.
+   * @param {number} index
+   * @export
+   */
+  isFirst(index) { return (index === 0); }
 }

src/test/frontend/deploy/portmappings_controller_test.js

@@ -14,6 +14,7 @@
 
 import deployModule from 'deploy/deploy_module';
 import PortMappingsController from 'deploy/portmappings_controller';
+import * as serviceTypes from 'deploy/portmappings_controller';
 
 describe('PortMappingsController controller', () => {
   /** @type {!PortMappingsController} */
@@ -36,37 +37,73 @@ describe('PortMappingsController controller', () => {
     });
   });
 
-  it('should initialize first value', () => {
-    expect(ctrl.portMappings).toEqual([{port: null, targetPort: null, protocol: 'FOO'}]);
-  });
+  it('should be initialized without port mapping line',
+     () => { expect(ctrl.portMappings.length).toBe(0); });
 
-  it('should add new mappings when needed', () => {
-    expect(ctrl.portMappings.length).toBe(1);
+  it('should add or remove port mappings, depending on service type', () => {
 
-    ctrl.checkPortMapping(undefined, 0);
+    // select internal service will add an empty port mapping
+    ctrl.serviceType = serviceTypes.INT_SERVICE;
+    ctrl.changeServiceType();
     expect(ctrl.portMappings.length).toBe(1);
 
+    // select no service will remove all port mappings
+    ctrl.serviceType = serviceTypes.NO_SERVICE;
+    ctrl.changeServiceType();
+    expect(ctrl.portMappings.length).toBe(0);
+  });
+
+  it('should add one additional port mapping when ports are filled', () => {
+
+    // given is an empty port mapping line
+    ctrl.serviceType = serviceTypes.INT_SERVICE;
+    ctrl.changeServiceType();
+
+    // when filling
     ctrl.portMappings[0].port = 80;
     ctrl.portMappings[0].targetPort = 8080;
-
     ctrl.checkPortMapping(undefined, 0);
+
+    // then another line is added
     expect(ctrl.portMappings.length).toBe(2);
   });
 
-  it('should determine removability', () => {
+  it('should not allow removal if no port mapping line would be left over', () => {
+
+    // given is one (empty) port mapping line
+    ctrl.serviceType = serviceTypes.INT_SERVICE;
+    ctrl.changeServiceType();
+
+    // then it cannot be removed
     expect(ctrl.isRemovable(0)).toBe(false);
+  });
+
+  it('should allow removal of one line if another is left over ', () => {
+
+    // given is a filled and an empty port mapping line
+    ctrl.serviceType = serviceTypes.INT_SERVICE;
+    ctrl.changeServiceType();
     ctrl.portMappings[0].port = 80;
     ctrl.portMappings[0].targetPort = 8080;
     ctrl.checkPortMapping(undefined, 0);
+
+    // then the first line is removable
     expect(ctrl.isRemovable(0)).toBe(true);
   });
 
   it('should remove port mappings', () => {
+
+    // given is a filled and an empty port mapping line
+    ctrl.serviceType = serviceTypes.INT_SERVICE;
+    ctrl.changeServiceType();
     ctrl.portMappings[0].port = 80;
     ctrl.portMappings[0].targetPort = 8080;
     ctrl.checkPortMapping(undefined, 0);
-    expect(ctrl.portMappings.length).toBe(2);
+
+    // when removing the first line
     ctrl.remove(0);
+
+    // then the empty line is left over
     expect(ctrl.portMappings.length).toBe(1);
     expect(ctrl.portMappings[0].port).toBeNull();
   });
@@ -82,6 +119,8 @@ describe('PortMappingsController controller', () => {
     testData.forEach((testData) => {
       // given
       let [port, targetPort, portValidity, targetPortValidity] = testData;
+      ctrl.serviceType = serviceTypes.INT_SERVICE;
+      ctrl.changeServiceType();
       ctrl.portMappings[0].port = port;
       ctrl.portMappings[0].targetPort = targetPort;
 
@@ -93,4 +132,9 @@ describe('PortMappingsController controller', () => {
       expect(portMappingForm.targetPort.$valid).toEqual(targetPortValidity);
     });
   });
+
+  it('should identify first index', () => {
+    expect(ctrl.isFirst(0)).toEqual(true);
+    expect(ctrl.isFirst(1)).toEqual(false);
+  });
 });