refactor(GUI): store drive as a reference to available drives (#599)

Instead of storing the whole selected drive object, we barely store a
reference to the corresponding drive in the available drives array (the
reference being the drive device).

This greatly simplifies the application state in the following ways:

- The drive metadata (size, description, etc) is not duplicated in the
  state, enforcing a single source of truth.
- If the selected drive stops being available (e.g: is unplugged), the
  reference doesn't hold anymore, making this functionality very natural
  to implement.
- Makes `SelectionStateModel.isCurrentDrive()` much more inuitive, since
  we don't have to document that changes in the metadata of the drive
  object, or extra keys such as `$$hashKey` don't change the result of
  this function.
- Ensures the state never goes into a problematic state where we try to
  to write to an unavailable drive.

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

lib/gui/app.js

@@ -235,7 +235,7 @@ app.controller('AppController', function(
       return;
     }
 
-    this.selection.setDrive(drive);
+    this.selection.setDrive(drive.device);
 
     AnalyticsService.logEvent('Select drive', {
       device: drive.device

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

@@ -7,7 +7,7 @@
   <ul class="list-group">
     <li class="list-group-item" ng-repeat="drive in modal.drives.getDrives() track by drive.device"
       ng-disabled="!modal.state.isDriveValid(drive)"
-      ng-click="modal.state.isDriveValid(drive) && modal.state.toggleSetDrive(drive)">
+      ng-click="modal.state.isDriveValid(drive) && modal.state.toggleSetDrive(drive.device)">
         <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>
@@ -33,7 +33,7 @@
         </div>
         <span class="tick tick--success"
           ng-show="modal.state.isDriveValid(drive)"
-          ng-disabled="!modal.state.isCurrentDrive(drive)"></span>
+          ng-disabled="!modal.state.isCurrentDrive(drive.device)"></span>
     </li>
   </ul>
 </div>

lib/gui/models/selection-state.js

@@ -24,23 +24,21 @@ const _ = require('lodash');
 const angular = require('angular');
 const Store = require('./store');
 const MODULE_NAME = 'Etcher.Models.SelectionState';
-const SelectionStateModel = angular.module(MODULE_NAME, []);
+const SelectionStateModel = angular.module(MODULE_NAME, [
+  require('./drives')
+]);
 
-SelectionStateModel.service('SelectionStateModel', function() {
+SelectionStateModel.service('SelectionStateModel', function(DrivesModel) {
 
   /**
    * @summary Set a drive
    * @function
    * @public
    *
-   * @param {Object} drive - drive
+   * @param {String} drive - drive device
    *
    * @example
-   * SelectionStateModel.setDrive({
-   *   device: '/dev/disk2',
-   *   name: 'USB drive',
-   *   size: 999999999
-   * });
+   * SelectionStateModel.setDrive('/dev/disk2');
    */
   this.setDrive = (drive) => {
     Store.dispatch({
@@ -144,12 +142,10 @@ SelectionStateModel.service('SelectionStateModel', function() {
    * @function
    * @public
    *
-   * @param {Object} drive - drive
+   * @param {String} drive - drive device
    *
    * @example
-   * SelectionStateModel.toggleSetDrive({
-   *   device: '/dev/disk2'
-   * });
+   * SelectionStateModel.toggleSetDrive('/dev/disk2');
    */
   this.toggleSetDrive = (drive) => {
     if (this.isCurrentDrive(drive)) {
@@ -189,7 +185,9 @@ SelectionStateModel.service('SelectionStateModel', function() {
    * const drive = SelectionStateModel.getDrive();
    */
   this.getDrive = () => {
-    return _.get(Store.getState().toJS(), 'selection.drive');
+    return _.find(DrivesModel.getDrives(), {
+      device: Store.getState().getIn([ 'selection', 'drive' ])
+    });
   };
 
   /**
@@ -355,27 +353,20 @@ SelectionStateModel.service('SelectionStateModel', function() {
    * @function
    * @public
    *
-   * @param {Object} drive - drive
+   * @param {String} drive - drive device
    * @returns {Boolean} whether the drive is the current drive
    *
    * @example
-   * if (SelectionStateModel.isCurrentDrive({
-   *   device: '/dev/sdb',
-   *   description: 'DataTraveler 2.0',
-   *   size: '7.3G',
-   *   mountpoint: '/media/UNTITLED',
-   *   name: '/dev/sdb',
-   *   system: false
-   * })) {
+   * if (SelectionStateModel.isCurrentDrive('/dev/sdb')) {
    *   console.log('This is the current drive!');
    * }
    */
   this.isCurrentDrive = (drive) => {
-    if (!drive || !drive.device) {
+    if (!drive) {
       return false;
     }
 
-    return drive.device === _.get(this.getDrive(), 'device');
+    return drive === _.get(this.getDrive(), 'device');
   };
 
 });

lib/gui/models/store.js

@@ -102,7 +102,7 @@ const storeReducer = (state, action) => {
         if (state.getIn([ 'selection', 'image', 'size' ], 0) <= drive.size && !drive.protected) {
           return storeReducer(newState, {
             type: ACTIONS.SELECT_DRIVE,
-            data: drive
+            data: drive.device
           });
         }
 
@@ -217,40 +217,28 @@ const storeReducer = (state, action) => {
     }
 
     case ACTIONS.SELECT_DRIVE: {
-      if (!action.data.device) {
-        throw new Error('Missing drive device');
+      if (!action.data) {
+        throw new Error('Missing drive');
       }
 
-      if (!_.isString(action.data.device)) {
-        throw new Error(`Invalid drive device: ${action.data.device}`);
+      if (!_.isString(action.data)) {
+        throw new Error(`Invalid drive: ${action.data}`);
       }
 
-      if (!action.data.name) {
-        throw new Error('Missing drive name');
+      const selectedDrive = state.get('availableDrives').find((drive) => {
+        return drive.get('device') === action.data;
+      });
+
+      if (!selectedDrive) {
+        throw new Error(`The drive is not available: ${action.data}`);
       }
 
-      if (!_.isString(action.data.name)) {
-        throw new Error(`Invalid drive name: ${action.data.name}`);
-      }
-
-      if (!action.data.size) {
-        throw new Error('Missing drive size');
-      }
-
-      if (!_.isNumber(action.data.size)) {
-        throw new Error(`Invalid drive size: ${action.data.size}`);
-      }
-
-      if (!_.isBoolean(action.data.protected)) {
-        throw new Error(`Invalid drive protected state: ${action.data.protected}`);
-      }
-
-      if (action.data.protected) {
+      if (selectedDrive.get('protected')) {
         throw new Error('The drive is write-protected');
       }
 
       // TODO: Reuse from SelectionStateModel.isDriveLargeEnough()
-      if (state.getIn([ 'selection', 'image', 'size' ], 0) > action.data.size) {
+      if (state.getIn([ 'selection', 'image', 'size' ], 0) > selectedDrive.get('size')) {
         throw new Error('The drive is not large enough');
       }
 

tests/gui/models/drives.spec.js

@@ -235,21 +235,25 @@ describe('Browser: DrivesModel', function() {
       describe('given one of the drives was selected', function() {
 
         beforeEach(function() {
-          SelectionStateModel.setDrive({
-            device: '/dev/sdc',
-            name: 'USB Drive',
-            size: 9999999,
-            mountpoint: '/mnt/bar',
-            system: false,
-            protected: false
-          });
+          DrivesModel.setDrives([
+            {
+              device: '/dev/sdc',
+              name: 'USB Drive',
+              size: 9999999,
+              mountpoint: '/mnt/bar',
+              system: false,
+              protected: false
+            }
+          ]);
+
+          SelectionStateModel.setDrive('/dev/sdc');
         });
 
         afterEach(function() {
           SelectionStateModel.removeDrive();
         });
 
-        it('should be delected if its not contain in the available drives anymore', function() {
+        it('should be deleted if its not contain in the available drives anymore', function() {
           m.chai.expect(SelectionStateModel.hasDrive()).to.be.true;
           DrivesModel.setDrives([
             {

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

@@ -10,12 +10,18 @@ describe('Browser: SelectionState', function() {
     require('../../../lib/gui/models/selection-state')
   ));
 
+  beforeEach(angular.mock.module(
+    require('../../../lib/gui/models/drives')
+  ));
+
   describe('SelectionStateModel', function() {
 
     let SelectionStateModel;
+    let DrivesModel;
 
-    beforeEach(angular.mock.inject(function(_SelectionStateModel_) {
+    beforeEach(angular.mock.inject(function(_SelectionStateModel_, _DrivesModel_) {
       SelectionStateModel = _SelectionStateModel_;
+      DrivesModel = _DrivesModel_;
     }));
 
     describe('given a clean state', function() {
@@ -126,12 +132,22 @@ describe('Browser: SelectionState', function() {
     describe('given a drive', function() {
 
       beforeEach(function() {
-        SelectionStateModel.setDrive({
-          device: '/dev/disk2',
-          name: 'USB Drive',
-          size: 999999999,
-          protected: false
-        });
+        DrivesModel.setDrives([
+          {
+            device: '/dev/disk2',
+            name: 'USB Drive',
+            size: 999999999,
+            protected: false
+          },
+          {
+            device: '/dev/disk5',
+            name: 'USB Drive',
+            size: 999999999,
+            protected: false
+          }
+        ]);
+
+        SelectionStateModel.setDrive('/dev/disk2');
       });
 
       describe('.getDrive()', function() {
@@ -160,13 +176,7 @@ describe('Browser: SelectionState', function() {
       describe('.setDrive()', function() {
 
         it('should override the drive', function() {
-          SelectionStateModel.setDrive({
-            device: '/dev/disk5',
-            name: 'USB Drive',
-            size: 999999999,
-            protected: false
-          });
-
+          SelectionStateModel.setDrive('/dev/disk5');
           const drive = SelectionStateModel.getDrive();
           m.chai.expect(drive).to.deep.equal({
             device: '/dev/disk5',
@@ -195,13 +205,16 @@ describe('Browser: SelectionState', function() {
       describe('.setDrive()', function() {
 
         it('should be able to set a drive', function() {
-          SelectionStateModel.setDrive({
-            device: '/dev/disk5',
-            name: 'USB Drive',
-            size: 999999999,
-            protected: false
-          });
+          DrivesModel.setDrives([
+            {
+              device: '/dev/disk5',
+              name: 'USB Drive',
+              size: 999999999,
+              protected: false
+            }
+          ]);
 
+          SelectionStateModel.setDrive('/dev/disk5');
           const drive = SelectionStateModel.getDrive();
           m.chai.expect(drive).to.deep.equal({
             device: '/dev/disk5',
@@ -212,98 +225,39 @@ describe('Browser: SelectionState', function() {
         });
 
         it('should throw if drive is write protected', function() {
-          m.chai.expect(function() {
-            SelectionStateModel.setDrive({
+          DrivesModel.setDrives([
+            {
               device: '/dev/disk1',
               name: 'USB Drive',
               size: 999999999,
               protected: true
-            });
+            }
+          ]);
+
+          m.chai.expect(function() {
+            SelectionStateModel.setDrive('/dev/disk1');
           }).to.throw('The drive is write-protected');
         });
 
-        it('should throw if no device', function() {
-          m.chai.expect(function() {
-            SelectionStateModel.setDrive({
+        it('should throw if the drive is not available', function() {
+          DrivesModel.setDrives([
+            {
+              device: '/dev/disk1',
               name: 'USB Drive',
               size: 999999999,
-              protected: false
-            });
-          }).to.throw('Missing drive device');
+              protected: true
+            }
+          ]);
+
+          m.chai.expect(function() {
+            SelectionStateModel.setDrive('/dev/disk5');
+          }).to.throw('The drive is not available: /dev/disk5');
         });
 
         it('should throw if device is not a string', function() {
           m.chai.expect(function() {
-            SelectionStateModel.setDrive({
-              device: 123,
-              name: 'USB Drive',
-              size: 999999999,
-              protected: false
-            });
-          }).to.throw('Invalid drive device: 123');
-        });
-
-        it('should throw if no name', function() {
-          m.chai.expect(function() {
-            SelectionStateModel.setDrive({
-              device: '/dev/disk2',
-              size: 999999999,
-              protected: false
-            });
-          }).to.throw('Missing drive name');
-        });
-
-        it('should throw if name is not a string', function() {
-          m.chai.expect(function() {
-            SelectionStateModel.setDrive({
-              device: '/dev/disk2',
-              name: 123,
-              size: 999999999,
-              protected: false
-            });
-          }).to.throw('Invalid drive name: 123');
-        });
-
-        it('should throw if no size', function() {
-          m.chai.expect(function() {
-            SelectionStateModel.setDrive({
-              device: '/dev/disk2',
-              name: 'USB Drive',
-              protected: false
-            });
-          }).to.throw('Missing drive size');
-        });
-
-        it('should throw if size is not a number', function() {
-          m.chai.expect(function() {
-            SelectionStateModel.setDrive({
-              device: '/dev/disk2',
-              name: 'USB Drive',
-              size: '999999999',
-              protected: false
-            });
-          }).to.throw('Invalid drive size: 999999999');
-        });
-
-        it('should throw if no protected property', function() {
-          m.chai.expect(function() {
-            SelectionStateModel.setDrive({
-              device: '/dev/disk2',
-              name: 'USB Drive',
-              size: 999999999
-            });
-          }).to.throw('Invalid drive protected state: undefined');
-        });
-
-        it('should throw if the protected is not boolean', function() {
-          m.chai.expect(function() {
-            SelectionStateModel.setDrive({
-              device: '/dev/disk2',
-              name: 'USB Drive',
-              size: 999999999,
-              protected: 'foo'
-            });
-          }).to.throw('Invalid drive protected state: foo');
+            SelectionStateModel.setDrive(123);
+          }).to.throw('Invalid drive: 123');
         });
 
       });
@@ -410,13 +364,17 @@ describe('Browser: SelectionState', function() {
       describe('.setDrive()', function() {
 
         it('should throw if drive is not large enough', function() {
-          m.chai.expect(function() {
-            SelectionStateModel.setDrive({
-              device: '/dev/disk1',
+          DrivesModel.setDrives([
+            {
+              device: '/dev/disk2',
               name: 'USB Drive',
               size: 999999998,
               protected: false
-            });
+            }
+          ]);
+
+          m.chai.expect(function() {
+            SelectionStateModel.setDrive('/dev/disk2');
           }).to.throw('The drive is not large enough');
         });
 
@@ -608,12 +566,16 @@ describe('Browser: SelectionState', function() {
     describe('given a drive', function() {
 
       beforeEach(function() {
-        SelectionStateModel.setDrive({
-          device: '/dev/disk1',
-          name: 'USB Drive',
-          size: 999999999,
-          protected: false
-        });
+        DrivesModel.setDrives([
+          {
+            device: '/dev/disk1',
+            name: 'USB Drive',
+            size: 999999999,
+            protected: false
+          }
+        ]);
+
+        SelectionStateModel.setDrive('/dev/disk1');
 
         SelectionStateModel.setImage({
           path: 'foo.img',
@@ -677,72 +639,31 @@ describe('Browser: SelectionState', function() {
       describe('given a selected drive', function() {
 
         beforeEach(function() {
-          SelectionStateModel.setDrive({
-            device: '/dev/sdb',
-            description: 'DataTraveler 2.0',
-            size: 999999999,
-            mountpoint: '/media/UNTITLED',
-            name: '/dev/sdb',
-            system: false,
-            protected: false
-          });
+          DrivesModel.setDrives([
+            {
+              device: '/dev/sdb',
+              description: 'DataTraveler 2.0',
+              size: 999999999,
+              mountpoint: '/media/UNTITLED',
+              name: '/dev/sdb',
+              system: false,
+              protected: false
+            }
+          ]);
+
+          SelectionStateModel.setDrive('/dev/sdb');
         });
 
         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',
-            description: 'DataTraveler 2.0',
-            size: 999999999,
-            mountpoint: '/media/UNTITLED',
-            name: '/dev/sdb',
-            system: false,
-            protected: false
-          })).to.be.true;
+          m.chai.expect(SelectionStateModel.isCurrentDrive('/dev/sdb')).to.be.true;
         });
 
-        it('should return true given the exact same drive with a $$hashKey', function() {
-          m.chai.expect(SelectionStateModel.isCurrentDrive({
-            device: '/dev/sdb',
-            description: 'DataTraveler 2.0',
-            size: 999999999,
-            mountpoint: '/media/UNTITLED',
-            name: '/dev/sdb',
-            system: false,
-            $$hashKey: 1234,
-            protected: false
-          })).to.be.true;
-        });
-
-        it('should return false if the device changes', function() {
-          m.chai.expect(SelectionStateModel.isCurrentDrive({
-            device: '/dev/sdc',
-            description: 'DataTraveler 2.0',
-            size: 999999999,
-            mountpoint: '/media/UNTITLED',
-            name: '/dev/sdb',
-            system: false,
-            protected: false
-          })).to.be.false;
-        });
-
-        it('should return true if the description changes', function() {
-          m.chai.expect(SelectionStateModel.isCurrentDrive({
-            device: '/dev/sdb',
-            description: 'DataTraveler 3.0',
-            size: 999999999,
-            mountpoint: '/media/UNTITLED',
-            name: '/dev/sdb',
-            system: false,
-            protected: false
-          })).to.be.true;
+        it('should return false if it is not the current drive', function() {
+          m.chai.expect(SelectionStateModel.isCurrentDrive('/dev/sdc')).to.be.false;
         });
 
       });
@@ -757,22 +678,8 @@ describe('Browser: SelectionState', 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({
-            device: '/dev/sdb',
-            description: 'DataTraveler 2.0',
-            size: 999999999,
-            mountpoint: '/media/UNTITLED',
-            name: '/dev/sdb',
-            system: false,
-            protected: false
-          })).to.be.false;
-
+          m.chai.expect(SelectionStateModel.isCurrentDrive('/dev/sdb')).to.be.false;
         });
 
       });
@@ -794,12 +701,22 @@ describe('Browser: SelectionState', function() {
             protected: false
           };
 
-          SelectionStateModel.setDrive(this.drive);
+          DrivesModel.setDrives([
+            this.drive,
+            {
+              device: '/dev/disk2',
+              name: 'USB Drive',
+              size: 999999999,
+              protected: false
+            }
+          ]);
+
+          SelectionStateModel.setDrive(this.drive.device);
         });
 
         it('should be able to remove the drive', function() {
           m.chai.expect(SelectionStateModel.hasDrive()).to.be.true;
-          SelectionStateModel.toggleSetDrive(this.drive);
+          SelectionStateModel.toggleSetDrive(this.drive.device);
           m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
         });
 
@@ -812,7 +729,7 @@ describe('Browser: SelectionState', function() {
           };
 
           m.chai.expect(SelectionStateModel.getDrive()).to.deep.equal(this.drive);
-          SelectionStateModel.toggleSetDrive(drive);
+          SelectionStateModel.toggleSetDrive(drive.device);
           m.chai.expect(SelectionStateModel.getDrive()).to.deep.equal(drive);
           m.chai.expect(SelectionStateModel.getDrive()).to.not.deep.equal(this.drive);
         });
@@ -834,7 +751,7 @@ describe('Browser: SelectionState', function() {
           };
 
           m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
-          SelectionStateModel.toggleSetDrive(drive);
+          SelectionStateModel.toggleSetDrive(drive.device);
           m.chai.expect(SelectionStateModel.getDrive()).to.deep.equal(drive);
         });