Refactor how application selection is accessed from DriveSelectorModal (#397)

* Refactor SelectionStateModel.isCurrentDrive() to only check `.device`

This set of changes modify `SelectionStateModel.isCurrentDrive()` to
only return true if the `.device` property is equal, and ignore the
rest, since there can be tons of false positives if the drive doesn't
exactly matches otherwise.

Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>

* Implement SelectionStateModel.toggleSetDrive()

This function is useful to toggle the current drive selected based on a
drive object.

Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>

* Make use of SelectionStateModel directly in DriveSelectorModal

Currently, we were basically proxying (with some additions)
`SelectionStateModel` through a service called
`DriveSelectorStateService`, which was completely unnecessary.

Now, we make use of `SelectionStateModel`, after moving any custom logic
from `DriveSelectorStateService` to `SelectionStateModel`.

Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>

lib/gui/components/drive-selector/controllers/drive-selector.js

@@ -18,18 +18,14 @@
 
 const _ = require('lodash');
 
-module.exports = function($uibModalInstance, DriveSelectorStateService, DriveScannerService) {
+module.exports = function($uibModalInstance, DriveScannerService, SelectionStateModel) {
 
   /**
    * @summary The drive selector state
    * @property
    * @type Object
-   *
-   * @description
-   * The state has been splitted from the controller for
-   * testability purposes.
    */
-  this.state = DriveSelectorStateService;
+  this.state = SelectionStateModel;
 
   /**
    * @summary The drive scanner service
@@ -53,7 +49,7 @@ module.exports = function($uibModalInstance, DriveSelectorStateService, DriveSca
    * DriveSelectorController.closeModal();
    */
   this.closeModal = function() {
-    const selectedDrive = DriveSelectorStateService.getSelectedDrive();
+    const selectedDrive = SelectionStateModel.getDrive();
 
     // Sanity check to cover the case where a drive is selected,
     // the drive is then unplugged from the computer and the modal

lib/gui/components/drive-selector/drive-selector.js

@@ -30,7 +30,6 @@ const DriveSelector = angular.module(MODULE_NAME, [
 ]);
 
 DriveSelector.controller('DriveSelectorController', require('./controllers/drive-selector'));
-DriveSelector.service('DriveSelectorStateService', require('./services/drive-selector-state'));
 DriveSelector.service('DriveSelectorService', require('./services/drive-selector'));
 
 module.exports = MODULE_NAME;

lib/gui/components/drive-selector/services/drive-selector-state.js

@@ -1,89 +0,0 @@
-/*
- * Copyright 2016 Resin.io
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *    http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-'use strict';
-
-const _ = require('lodash');
-
-module.exports = function(SelectionStateModel) {
-  let self = this;
-
-  /**
-   * @summary Toggle select drive
-   * @function
-   * @public
-   *
-   * @param {Object} drive - drive
-   *
-   * @example
-   * DriveSelectorController.toggleSelectDrive({ drive });
-   */
-  this.toggleSelectDrive = function(drive) {
-    if (this.isSelectedDrive(drive)) {
-      SelectionStateModel.removeDrive();
-    } else {
-      SelectionStateModel.setDrive(drive);
-    }
-  };
-
-  /**
-   * @summary Check if a drive is the selected one
-   * @function
-   * @public
-   *
-   * @param {Object} drive - drive
-   * @returns {Boolean} whether the drive is selected
-   *
-   * @example
-   * if (DriveSelectorController.isSelectedDrive({ drive })) {
-   *   console.log('The drive is selected!');
-   * }
-   */
-  this.isSelectedDrive = function(drive) {
-    if (!_.has(drive, 'device')) {
-      return false;
-    }
-
-    return drive.device === _.get(self.getSelectedDrive(), 'device');
-  };
-
-  /**
-   * @summary Get selected drive
-   * @function
-   * @public
-   *
-   * @returns {Object} selected drive
-   *
-   * @example
-   * const drive = DriveSelectorStateService.getSelectedDrive();
-   */
-  this.getSelectedDrive = SelectionStateModel.getDrive;
-
-  /**
-   * @summary Has a selected drive
-   * @function
-   * @public
-   *
-   * @returns {Boolean} whether there is a selected drive
-   *
-   * @example
-   * if (DriveSelectorStateService.hasSelectedDrive()) {
-   *   console.log('There is a selected drive');
-   * }
-   */
-  this.hasSelectedDrive = SelectionStateModel.hasDrive;
-
-};

lib/gui/components/drive-selector/templates/drive-selector-modal.tpl.html

@@ -6,12 +6,12 @@
 <div class="modal-body">
   <ul class="list-group">
     <li class="list-group-item" ng-repeat="drive in modal.scanner.drives"
-      ng-click="modal.state.toggleSelectDrive(drive)">
+      ng-click="modal.state.toggleSetDrive(drive)">
         <div>
           <h4 class="list-group-item-heading">{{ drive.description }} - {{ drive.size | gigabyte | number:1 }} GB</h4>
           <p class="list-group-item-text">{{ drive.name }}</p>
         </div>
-        <span class="tick tick--success" ng-disabled="!modal.state.isSelectedDrive(drive)"></span>
+        <span class="tick tick--success" ng-disabled="!modal.state.isCurrentDrive(drive)"></span>
     </li>
   </ul>
 </div>
@@ -19,5 +19,5 @@
 <div class="modal-footer">
   <button class="btn btn-primary btn-block"
     ng-click="modal.closeModal()"
-    ng-disabled="!modal.state.hasSelectedDrive()">CONTINUE</button>
+    ng-disabled="!modal.state.hasDrive()">CONTINUE</button>
 </div>

lib/gui/models/selection-state.js

@@ -51,6 +51,26 @@ SelectionStateModel.service('SelectionStateModel', function() {
     selection.drive = drive;
   };
 
+  /**
+   * @summary Toggle set drive
+   * @function
+   * @public
+   *
+   * @param {Object} drive - drive
+   *
+   * @example
+   * SelectionStateModel.toggleSetDrive({
+   *   device: '/dev/disk2'
+   * });
+   */
+  this.toggleSetDrive = function(drive) {
+    if (self.isCurrentDrive(drive)) {
+      self.removeDrive();
+    } else {
+      self.setDrive(drive);
+    }
+  };
+
   /**
    * @summary Set a image
    * @function
@@ -192,7 +212,11 @@ SelectionStateModel.service('SelectionStateModel', function() {
    * }
    */
   this.isCurrentDrive = function(drive) {
-    return angular.equals(self.getDrive(), drive);
+    if (!drive || !drive.device) {
+      return false;
+    }
+
+    return drive.device === _.get(self.getDrive(), 'device');
   };
 
 });

tests/gui/components/drive-selector.spec.js

@@ -1,180 +0,0 @@
-'use strict';
-
-const m = require('mochainon');
-const angular = require('angular');
-require('angular-mocks');
-
-describe('Browser: DriveSelector', function() {
-
-  beforeEach(angular.mock.module(
-    require('../../../lib/gui/components/drive-selector/drive-selector')
-  ));
-
-  describe('DriveSelectorStateService', function() {
-
-    let DriveSelectorStateService;
-    let SelectionStateModel;
-
-    beforeEach(angular.mock.inject(function(_DriveSelectorStateService_, _SelectionStateModel_) {
-      DriveSelectorStateService = _DriveSelectorStateService_;
-      SelectionStateModel = _SelectionStateModel_;
-    }));
-
-    beforeEach(function() {
-      SelectionStateModel.clear();
-    });
-
-    describe('.toggleSelectDrive()', function() {
-
-      it('should be able to toggle a drive', function() {
-        const drive = {
-          device: '/dev/disk2',
-          name: 'USB Drive',
-          size: '16GB'
-        };
-
-        m.chai.expect(DriveSelectorStateService.getSelectedDrive()).to.not.exist;
-        DriveSelectorStateService.toggleSelectDrive(drive);
-        m.chai.expect(DriveSelectorStateService.getSelectedDrive()).to.deep.equal(drive);
-        DriveSelectorStateService.toggleSelectDrive(drive);
-        m.chai.expect(DriveSelectorStateService.getSelectedDrive()).to.not.exist;
-      });
-
-      it('should be able to change the current selected drive', function() {
-        const drive1 = {
-          device: '/dev/disk2',
-          name: 'USB Drive',
-          size: '16GB'
-        };
-
-        const drive2 = {
-          device: '/dev/disk3',
-          name: 'SDCARD Reader',
-          size: '4GB'
-        };
-
-        DriveSelectorStateService.toggleSelectDrive(drive1);
-        m.chai.expect(DriveSelectorStateService.getSelectedDrive()).to.deep.equal(drive1);
-        DriveSelectorStateService.toggleSelectDrive(drive2);
-        m.chai.expect(DriveSelectorStateService.getSelectedDrive()).to.deep.equal(drive2);
-      });
-
-    });
-
-    describe('.isSelectedDrive()', function() {
-
-      it('should always return false if no drive', function() {
-        const drive = {
-          device: '/dev/disk2',
-          name: 'USB Drive',
-          size: '16GB'
-        };
-
-        SelectionStateModel.removeDrive();
-        m.chai.expect(DriveSelectorStateService.isSelectedDrive(drive)).to.be.false;
-      });
-
-      it('should return true if the selected drive matches', function() {
-        const drive = {
-          device: '/dev/disk2',
-          name: 'USB Drive',
-          size: '16GB'
-        };
-
-        DriveSelectorStateService.toggleSelectDrive(drive);
-        m.chai.expect(DriveSelectorStateService.isSelectedDrive(drive)).to.be.true;
-      });
-
-      it('should return false if the selected drive does not match', function() {
-        const drive1 = {
-          device: '/dev/disk2',
-          name: 'USB Drive',
-          size: '16GB'
-        };
-
-        const drive2 = {
-          device: '/dev/disk3',
-          name: 'SDCARD Reader',
-          size: '4GB'
-        };
-
-        DriveSelectorStateService.toggleSelectDrive(drive1);
-        m.chai.expect(DriveSelectorStateService.isSelectedDrive(drive2)).to.be.false;
-      });
-
-      it('should return false if there is no selected drive and an empty object is passed', function() {
-        SelectionStateModel.removeDrive();
-        m.chai.expect(DriveSelectorStateService.isSelectedDrive({})).to.be.false;
-      });
-
-      it('should return true if the drive is selected in SelectionStateModel', function() {
-        const drive = {
-          device: '/dev/disk2',
-          name: 'USB Drive',
-          size: '16GB'
-        };
-
-        SelectionStateModel.setDrive(drive);
-        m.chai.expect(DriveSelectorStateService.isSelectedDrive(drive)).to.be.true;
-      });
-
-    });
-
-    describe('.getSelectedDrive()', function() {
-
-      it('should return undefined if no selected drive', function() {
-        SelectionStateModel.removeDrive();
-        const drive = DriveSelectorStateService.getSelectedDrive();
-        m.chai.expect(drive).to.not.exist;
-      });
-
-      it('should return undefined if the selected drive is an empty object', function() {
-        SelectionStateModel.setDrive({});
-        const drive = DriveSelectorStateService.getSelectedDrive();
-        m.chai.expect(drive).to.not.exist;
-      });
-
-      it('should return the selected drive if there is one', function() {
-        const selectedDrive = {
-          device: '/dev/disk2',
-          name: 'USB Drive',
-          size: '16GB'
-        };
-
-        DriveSelectorStateService.toggleSelectDrive(selectedDrive);
-        const drive = DriveSelectorStateService.getSelectedDrive();
-        m.chai.expect(drive).to.deep.equal(selectedDrive);
-      });
-
-    });
-
-    describe('.hasSelectedDrive()', function() {
-
-      it('should return false if no selected drive', function() {
-        SelectionStateModel.removeDrive();
-        const hasDrive = DriveSelectorStateService.hasSelectedDrive();
-        m.chai.expect(hasDrive).to.be.false;
-      });
-
-      it('should return false if the selected drive is an empty object', function() {
-        SelectionStateModel.setDrive({});
-        const hasDrive = DriveSelectorStateService.hasSelectedDrive();
-        m.chai.expect(hasDrive).to.be.false;
-      });
-
-      it('should return true if there is a selected drive', function() {
-        DriveSelectorStateService.toggleSelectDrive({
-          device: '/dev/disk2',
-          name: 'USB Drive',
-          size: '16GB'
-        });
-
-        const hasDrive = DriveSelectorStateService.hasSelectedDrive();
-        m.chai.expect(hasDrive).to.be.true;
-      });
-
-    });
-
-  });
-
-});

tests/gui/models/selection-state.spec.js

@@ -251,6 +251,14 @@ describe('Browser: SelectionState', function() {
           });
         });
 
+        it('should return false if an undefined value is passed', function() {
+          m.chai.expect(SelectionStateModel.isCurrentDrive()).to.be.false;
+        });
+
+        it('should return false if an empty object is passed', function() {
+          m.chai.expect(SelectionStateModel.isCurrentDrive({})).to.be.false;
+        });
+
         it('should return true given the exact same drive', function() {
           m.chai.expect(SelectionStateModel.isCurrentDrive({
             device: '/dev/sdb',
@@ -285,7 +293,7 @@ describe('Browser: SelectionState', function() {
           })).to.be.false;
         });
 
-        it('should return false if the description changes', function() {
+        it('should return true if the description changes', function() {
           m.chai.expect(SelectionStateModel.isCurrentDrive({
             device: '/dev/sdb',
             description: 'DataTraveler 3.0',
@@ -293,7 +301,7 @@ describe('Browser: SelectionState', function() {
             mountpoint: '/media/UNTITLED',
             name: '/dev/sdb',
             system: false
-          })).to.be.false;
+          })).to.be.true;
         });
 
       });
@@ -304,6 +312,14 @@ describe('Browser: SelectionState', function() {
           SelectionStateModel.removeDrive();
         });
 
+        it('should return false if an undefined value is passed', function() {
+          m.chai.expect(SelectionStateModel.isCurrentDrive()).to.be.false;
+        });
+
+        it('should return false if an empty object is passed', function() {
+          m.chai.expect(SelectionStateModel.isCurrentDrive({})).to.be.false;
+        });
+
         it('should return false for anything', function() {
 
           m.chai.expect(SelectionStateModel.isCurrentDrive({
@@ -321,6 +337,66 @@ describe('Browser: SelectionState', function() {
 
     });
 
+    describe('.toggleSetDrive()', function() {
+
+      describe('given a selected drive', function() {
+
+        beforeEach(function() {
+          this.drive = {
+            device: '/dev/sdb',
+            description: 'DataTraveler 2.0',
+            size: '7.3G',
+            mountpoint: '/media/UNTITLED',
+            name: '/dev/sdb',
+            system: false
+          };
+
+          SelectionStateModel.setDrive(this.drive);
+        });
+
+        it('should be able to remove the drive', function() {
+          m.chai.expect(SelectionStateModel.hasDrive()).to.be.true;
+          SelectionStateModel.toggleSetDrive(this.drive);
+          m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
+        });
+
+        it('should be able to replace the drive', function() {
+          const drive = {
+            device: '/dev/disk2',
+            name: 'USB Drive',
+            size: '16GB'
+          };
+
+          m.chai.expect(SelectionStateModel.getDrive()).to.deep.equal(this.drive);
+          SelectionStateModel.toggleSetDrive(drive);
+          m.chai.expect(SelectionStateModel.getDrive()).to.deep.equal(drive);
+          m.chai.expect(SelectionStateModel.getDrive()).to.not.deep.equal(this.drive);
+        });
+
+      });
+
+      describe('given no selected drive', function() {
+
+        beforeEach(function() {
+          SelectionStateModel.removeDrive();
+        });
+
+        it('should set the drive', function() {
+          const drive = {
+            device: '/dev/disk2',
+            name: 'USB Drive',
+            size: '16GB'
+          };
+
+          m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
+          SelectionStateModel.toggleSetDrive(drive);
+          m.chai.expect(SelectionStateModel.getDrive()).to.deep.equal(drive);
+        });
+
+      });
+
+    });
+
   });
 
 });