Extract DrivesModel from DriveScannerService (#466)

Currently, `DriveScannerService` is in charge of both scanning the
available drives and maintaining the state of the currently detected
drives.

To honour the single responsibility principle, we split this service
into a `DrivesModel`, which is incharge of maintaining the state without
caring how they are detected, and `DriveScannerService`, which only
scans the available drives and modified `DrivesModel` accordingly.

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

lib/gui/app.js

@@ -38,6 +38,7 @@ const app = angular.module('Etcher', [
   require('./models/selection-state'),
   require('./models/settings'),
   require('./models/supported-formats'),
+  require('./models/drives'),
 
   // Components
   require('./components/progress-button/progress-button'),
@@ -87,6 +88,7 @@ app.controller('AppController', function(
   SelectionStateModel,
   SettingsModel,
   SupportedFormatsModel,
+  DrivesModel,
   ImageWriterService,
   AnalyticsService,
   DriveSelectorService,
@@ -98,8 +100,8 @@ app.controller('AppController', function(
   let self = this;
   this.formats = SupportedFormatsModel;
   this.selection = SelectionStateModel;
+  this.drives = DrivesModel;
   this.writer = ImageWriterService;
-  this.scanner = DriveScannerService;
   this.settings = SettingsModel.data;
   this.success = true;
 
@@ -142,7 +144,7 @@ app.controller('AppController', function(
     OSWindowProgressService.set(state.progress);
   });
 
-  this.scanner.start(2000).on('error', OSDialogService.showError).on('scan', function(drives) {
+  DriveScannerService.start(2000).on('error', OSDialogService.showError).on('scan', function(drives) {
 
     // Cover the case where you select a drive, but then eject it.
     if (self.selection.hasDrive() && !_.find(drives, self.selection.isCurrentDrive)) {
@@ -267,7 +269,7 @@ app.controller('AppController', function(
 
     // Stop scanning drives when flashing
     // otherwise Windows throws EPERM
-    self.scanner.stop();
+    DriveScannerService.stop();
 
     AnalyticsService.logEvent('Flash', {
       image: image,

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

@@ -18,7 +18,7 @@
 
 const _ = require('lodash');
 
-module.exports = function($uibModalInstance, DriveScannerService, SelectionStateModel) {
+module.exports = function($uibModalInstance, DrivesModel, SelectionStateModel) {
 
   /**
    * @summary The drive selector state
@@ -28,7 +28,7 @@ module.exports = function($uibModalInstance, DriveScannerService, SelectionState
   this.state = SelectionStateModel;
 
   /**
-   * @summary The drive scanner service
+   * @summary The drives model
    * @property
    * @type Object
    *
@@ -36,9 +36,9 @@ module.exports = function($uibModalInstance, DriveScannerService, SelectionState
    * We expose the whole service instead of the `.drives`
    * property, which is the one we're interested in since
    * this allows the property to be automatically updated
-   * when `DriveScannerService` detects a change in the drives.
+   * when `DrivesModel` detects a change in the drives.
    */
-  this.scanner = DriveScannerService;
+  this.drives = DrivesModel;
 
   /**
    * @summary Close the modal and resolve the selected drive
@@ -54,7 +54,7 @@ module.exports = function($uibModalInstance, DriveScannerService, SelectionState
     // Sanity check to cover the case where a drive is selected,
     // the drive is then unplugged from the computer and the modal
     // is resolved with a non-existent drive.
-    if (!selectedDrive || !_.includes(this.scanner.drives, selectedDrive)) {
+    if (!selectedDrive || !_.includes(this.drives.getDrives(), selectedDrive)) {
       return $uibModalInstance.dismiss();
     }
 

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

@@ -24,7 +24,7 @@ const angular = require('angular');
 const MODULE_NAME = 'Etcher.Components.DriveSelector';
 const DriveSelector = angular.module(MODULE_NAME, [
   require('angular-ui-bootstrap'),
-  require('../../modules/drive-scanner'),
+  require('../../models/drives'),
   require('../../models/selection-state'),
   require('../../utils/byte-size/byte-size')
 ]);

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

@@ -5,7 +5,7 @@
 
 <div class="modal-body">
   <ul class="list-group">
-    <li class="list-group-item" ng-repeat="drive in modal.scanner.drives"
+    <li class="list-group-item" ng-repeat="drive in modal.drives.getDrives()"
       ng-disabled="!modal.state.isDriveLargeEnough(drive)"
       ng-click="modal.state.isDriveLargeEnough(drive) && modal.state.toggleSetDrive(drive)">
         <div>

lib/gui/models/drives.js

@@ -0,0 +1,101 @@
+/*
+ * 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';
+
+/**
+ * @module Etcher.Models.Drives
+ */
+
+const angular = require('angular');
+const _ = require('lodash');
+const MODULE_NAME = 'Etcher.Models.Drives';
+const Drives = angular.module(MODULE_NAME, []);
+
+Drives.service('DrivesModel', function() {
+
+  /**
+   * @summary List of available drives
+   * @type {Object[]}
+   * @private
+   */
+  let availableDrives = [];
+
+  /**
+   * @summary Check if there are available drives
+   * @function
+   * @public
+   *
+   * @returns {Boolean} whether there are available drives
+   *
+   * @example
+   * if (DrivesModel.hasAvailableDrives()) {
+   *   console.log('There are available drives!');
+   * }
+   */
+  this.hasAvailableDrives = function() {
+    return !_.isEmpty(availableDrives);
+  };
+
+  /**
+   * @summary Set a list of drives
+   * @function
+   * @private
+   *
+   * @param {Object[]} drives - drives
+   *
+   * @throws Will throw if no drives
+   * @throws Will throw if drives is not an array of objects
+   *
+   * @example
+   * DrivesModel.setDrives([ ... ]);
+   */
+  this.setDrives = function(drives) {
+
+    if (!drives) {
+      throw new Error('Missing drives');
+    }
+
+    if (!_.isArray(drives) || !_.every(drives, _.isPlainObject)) {
+      throw new Error(`Invalid drives: ${drives}`);
+    }
+
+    // Only update if something has changed
+    // to avoid unnecessary DOM manipulations
+    // angular.equals ignores $$hashKey by default
+    if (!angular.equals(availableDrives, drives)) {
+      availableDrives = drives;
+    }
+
+  };
+
+  /**
+   * @summary Get detected drives
+   * @function
+   * @private
+   *
+   * @returns {Object[]} drives
+   *
+   * @example
+   * const drives = DrivesModel.getDrives();
+   */
+  this.getDrives = function() {
+    return availableDrives;
+  };
+
+});
+
+module.exports = MODULE_NAME;

lib/gui/modules/drive-scanner.js

@@ -27,58 +27,14 @@ const drivelist = require('drivelist');
 
 const MODULE_NAME = 'Etcher.drive-scanner';
 const driveScanner = angular.module(MODULE_NAME, [
-  require('angular-q-promisify')
+  require('angular-q-promisify'),
+  require('../models/drives')
 ]);
 
-driveScanner.service('DriveScannerService', function($q, $interval, $timeout) {
+driveScanner.service('DriveScannerService', function($q, $interval, $timeout, DrivesModel) {
   let self = this;
   let interval = null;
 
-  /**
-   * @summary List of available drives
-   * @type {Object[]}
-   * @public
-   */
-  this.drives = [];
-
-  /**
-   * @summary Check if there are available drives
-   * @function
-   * @public
-   *
-   * @returns {Boolean} whether there are available drives
-   *
-   * @example
-   * if (DriveScannerService.hasAvailableDrives()) {
-   *   console.log('There are available drives!');
-   * }
-   */
-  this.hasAvailableDrives = function() {
-    return !_.isEmpty(self.drives);
-  };
-
-  /**
-   * @summary Set the list of drives
-   * @function
-   * @private
-   *
-   * @param {Object[]} drives - drives
-   *
-   * @example
-   * DriveScannerService.scan().then(function(drives) {
-   *   DriveScannerService.setDrives(drives);
-   * });
-   */
-  this.setDrives = function(drives) {
-
-    // Only update if something has changed
-    // to avoid unnecessary DOM manipulations
-    // angular.equals ignores $$hashKey by default
-    if (!angular.equals(self.drives, drives)) {
-      self.drives = drives;
-    }
-  };
-
   /**
    * @summary Get available drives
    * @function
@@ -101,7 +57,7 @@ driveScanner.service('DriveScannerService', function($q, $interval, $timeout) {
   };
 
   /**
-   * @summary Scan drives and populate `.drives`
+   * @summary Scan drives and populate DrivesModel
    * @function
    * @public
    *
@@ -126,7 +82,7 @@ driveScanner.service('DriveScannerService', function($q, $interval, $timeout) {
     const fn = function() {
       return self.scan().then(function(drives) {
         emitter.emit('scan', drives);
-        self.setDrives(drives);
+        DrivesModel.setDrives(drives);
       }).catch(function(error) {
         emitter.emit('error', error);
       });

lib/gui/partials/main.html

@@ -42,13 +42,13 @@
       <div class="space-vertical-large">
         <div ng-hide="app.selection.hasDrive()">
 
-          <div ng-show="app.scanner.hasAvailableDrives() || !app.selection.hasImage()">
+          <div ng-show="app.drives.hasAvailableDrives() || !app.selection.hasImage()">
             <button class="btn btn-primary btn-brick"
               ng-disabled="!app.selection.hasImage()"
               ng-click="app.openDriveSelector()">Select drive</button>
           </div>
 
-          <div ng-hide="app.scanner.hasAvailableDrives() || !app.selection.hasImage()">
+          <div ng-hide="app.drives.hasAvailableDrives() || !app.selection.hasImage()">
             <button class="btn btn-danger btn-brick">Connect a drive</button>
           </div>
 

tests/gui/models/drives.spec.js

@@ -0,0 +1,132 @@
+'use strict';
+
+const m = require('mochainon');
+const angular = require('angular');
+require('angular-mocks');
+
+describe('Browser: DrivesModel', function() {
+
+  beforeEach(angular.mock.module(
+    require('../../../lib/gui/models/drives')
+  ));
+
+  describe('DrivesModel', function() {
+
+    let DrivesModel;
+
+    beforeEach(angular.mock.inject(function(_DrivesModel_) {
+      DrivesModel = _DrivesModel_;
+    }));
+
+    it('should have no drives by default', function() {
+      m.chai.expect(DrivesModel.getDrives()).to.deep.equal([]);
+    });
+
+    describe('.setDrives()', function() {
+
+      it('should throw if no drives', function() {
+        m.chai.expect(function() {
+          DrivesModel.setDrives();
+        }).to.throw('Missing drives');
+      });
+
+      it('should throw if drives is not an array', function() {
+        m.chai.expect(function() {
+          DrivesModel.setDrives(123);
+        }).to.throw('Invalid drives: 123');
+      });
+
+      it('should throw if drives is not an array of objects', function() {
+        m.chai.expect(function() {
+          DrivesModel.setDrives([
+            123,
+            123,
+            123
+          ]);
+        }).to.throw('Invalid drives: 123,123,123');
+      });
+
+    });
+
+    describe('given no drives', function() {
+
+      describe('.hasAvailableDrives()', function() {
+
+        it('should return false', function() {
+          m.chai.expect(DrivesModel.hasAvailableDrives()).to.be.false;
+        });
+
+      });
+
+      describe('.setDrives()', function() {
+
+        it('should be able to set drives', function() {
+          const drives = [
+            {
+              device: '/dev/sdb',
+              description: 'Foo',
+              size: '14G',
+              mountpoint: '/mnt/foo',
+              system: false
+            }
+          ];
+
+          DrivesModel.setDrives(drives);
+          m.chai.expect(DrivesModel.getDrives()).to.deep.equal(drives);
+        });
+
+      });
+
+    });
+
+    describe('given drives', function() {
+
+      beforeEach(function() {
+        this.drives = [
+          {
+            device: '/dev/sdb',
+            description: 'Foo',
+            size: '14G',
+            mountpoint: '/mnt/foo',
+            system: false
+          },
+          {
+            device: '/dev/sdc',
+            description: 'Bar',
+            size: '14G',
+            mountpoint: '/mnt/bar',
+            system: false
+          }
+        ];
+
+        DrivesModel.setDrives(this.drives);
+      });
+
+      describe('.hasAvailableDrives()', function() {
+
+        it('should return true', function() {
+          const hasDrives = DrivesModel.hasAvailableDrives();
+          m.chai.expect(hasDrives).to.be.true;
+        });
+
+      });
+
+      describe('.setDrives()', function() {
+
+        it('should keep the same drives if equal', function() {
+          DrivesModel.setDrives(this.drives);
+          m.chai.expect(DrivesModel.getDrives()).to.deep.equal(this.drives);
+        });
+
+        it('should consider drives with different $$hashKey the same', function() {
+          this.drives[0].$$haskey = 1234;
+          DrivesModel.setDrives(this.drives);
+          m.chai.expect(DrivesModel.getDrives()).to.deep.equal(this.drives);
+        });
+
+      });
+
+    });
+
+  });
+});

tests/gui/modules/drive-scanner.spec.js

@@ -11,26 +11,28 @@ describe('Browser: DriveScanner', function() {
     require('../../../lib/gui/modules/drive-scanner')
   ));
 
+  beforeEach(angular.mock.module(
+    require('../../../lib/gui/models/drives')
+  ));
+
   describe('DriveScannerService', function() {
 
     let $interval;
     let $rootScope;
     let $timeout;
     let $q;
+    let DrivesModel;
     let DriveScannerService;
 
-    beforeEach(angular.mock.inject(function(_$interval_, _$rootScope_, _$timeout_, _$q_, _DriveScannerService_) {
+    beforeEach(angular.mock.inject(function(_$interval_, _$rootScope_, _$timeout_, _$q_, _DriveScannerService_, _DrivesModel_) {
       $interval = _$interval_;
       $rootScope = _$rootScope_;
       $timeout = _$timeout_;
       $q = _$q_;
       DriveScannerService = _DriveScannerService_;
+      DrivesModel = _DrivesModel_;
     }));
 
-    it('should have no drives by default', function() {
-      m.chai.expect(DriveScannerService.drives).to.deep.equal([]);
-    });
-
     describe('.scan()', function() {
 
       describe('given no available drives', function() {
@@ -173,87 +175,6 @@ describe('Browser: DriveScanner', function() {
 
     });
 
-    describe('given no drives', function() {
-
-      describe('.hasAvailableDrives()', function() {
-
-        it('should return false', function() {
-          const hasDrives = DriveScannerService.hasAvailableDrives();
-          m.chai.expect(hasDrives).to.be.false;
-        });
-
-      });
-
-      describe('.setDrives()', function() {
-
-        it('should be able to set drives', function() {
-          const drives = [
-            {
-              device: '/dev/sdb',
-              description: 'Foo',
-              size: '14G',
-              mountpoint: '/mnt/foo',
-              system: false
-            }
-          ];
-
-          DriveScannerService.setDrives(drives);
-          m.chai.expect(DriveScannerService.drives).to.deep.equal(drives);
-        });
-
-      });
-
-    });
-
-    describe('given drives', function() {
-
-      beforeEach(function() {
-        this.drives = [
-          {
-            device: '/dev/sdb',
-            description: 'Foo',
-            size: '14G',
-            mountpoint: '/mnt/foo',
-            system: false
-          },
-          {
-            device: '/dev/sdc',
-            description: 'Bar',
-            size: '14G',
-            mountpoint: '/mnt/bar',
-            system: false
-          }
-        ];
-
-        DriveScannerService.drives = this.drives;
-      });
-
-      describe('.hasAvailableDrives()', function() {
-
-        it('should return true', function() {
-          const hasDrives = DriveScannerService.hasAvailableDrives();
-          m.chai.expect(hasDrives).to.be.true;
-        });
-
-      });
-
-      describe('.setDrives()', function() {
-
-        it('should keep the same drives if equal', function() {
-          DriveScannerService.setDrives(this.drives);
-          m.chai.expect(DriveScannerService.drives).to.deep.equal(this.drives);
-        });
-
-        it('should consider drives with different $$hashKey the same', function() {
-          this.drives[0].$$haskey = 1234;
-          DriveScannerService.setDrives(this.drives);
-          m.chai.expect(DriveScannerService.drives).to.deep.equal(this.drives);
-        });
-
-      });
-
-    });
-
     describe('given available drives', function() {
 
       beforeEach(function() {
@@ -286,7 +207,7 @@ describe('Browser: DriveScanner', function() {
         DriveScannerService.start(200);
         $timeout.flush();
         $interval.flush(400);
-        m.chai.expect(DriveScannerService.drives).to.deep.equal(this.drives);
+        m.chai.expect(DrivesModel.getDrives()).to.deep.equal(this.drives);
         DriveScannerService.stop();
       });