refactor: move auto-select logic to redux store (#516)

Now that we have a central source of truth for state mutations, the
auto-select feature fits really well in the redux store, particularly
inside the `SET_AVAILABLE_DRIVES` action.

This also has the great benefit that we can unit test the auto-selection
logic, which was not particularly trivial before, when such code lived
in the controller instead.

The only downside of this approach is that we lose the nice "Auto-select
drive" analytics event, which will be re-added very soon in a future PR.

Signed-off-by: Juan Cruz Viotti <jviottidc@gmail.com>
This commit is contained in:
Juan Cruz Viotti 2016-06-23 12:19:27 -04:00 committed by GitHub
parent 77f2b1d1cc
commit 321c653d74
3 changed files with 144 additions and 28 deletions

View File

@ -165,34 +165,6 @@ app.controller('AppController', function(
if (_.isEmpty(drives)) {
DriveSelectorService.close();
}
// Notice we only autoselect the drive if there is an image,
// which means that the first step was completed successfully,
// otherwise the drive is selected while the drive step is disabled
// which looks very weird.
if (drives.length === 1 && self.selection.hasImage()) {
const drive = _.first(drives);
if (!self.selection.isDriveValid(drive)) {
return;
}
// Do not autoselect the same drive over and over again
// and fill the logs unnecessary.
// `angular.equals` is used instead of `_.isEqual` to
// cope with `$$hashKey`.
if (!angular.equals(self.selection.getDrive(), drive)) {
if (self.selection.isDriveLargeEnough(drive)) {
self.selectDrive(drive);
AnalyticsService.logEvent('Auto-select drive', {
device: drive.device
});
}
}
}
});
this.selectImage = function(image) {

View File

@ -53,6 +53,25 @@ const store = function(state, action) {
}
const newState = state.set('availableDrives', Immutable.fromJS(action.data));
// Notice we only autoselect the drive if there is an image,
// which means that the first step was completed successfully,
// otherwise the drive is selected while the drive step is disabled
// which looks very weird.
if (action.data.length === 1 && state.hasIn([ 'selection', 'image' ])) {
const drive = _.first(action.data);
// TODO: Reuse from SelectionStateModel.isDriveValid()
if (state.getIn([ 'selection', 'image', 'size' ], 0) <= drive.size && !drive.protected) {
return store(newState, {
type: 'SELECT_DRIVE',
data: drive
});
}
}
const selectedDevice = state.getIn([ 'selection', 'drive', 'device' ]);
if (selectedDevice && !_.find(action.data, {

View File

@ -78,6 +78,131 @@ describe('Browser: DrivesModel', function() {
m.chai.expect(DrivesModel.getDrives()).to.deep.equal(drives);
});
describe('given no selected image and no selected drive', function() {
beforeEach(function() {
SelectionStateModel.removeDrive();
SelectionStateModel.removeImage();
});
it('should not auto-select a single valid available drive', function() {
m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
DrivesModel.setDrives([
{
device: '/dev/sdb',
name: 'Foo',
size: 999999999,
mountpoint: '/mnt/foo',
system: false,
protected: false
}
]);
m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
});
});
describe('given a selected image and no selected drive', function() {
beforeEach(function() {
SelectionStateModel.removeDrive();
SelectionStateModel.setImage({
path: 'foo.img',
size: 999999999
});
});
afterEach(function() {
SelectionStateModel.removeImage();
});
it('should not auto-select when there are multiple valid available drives', function() {
m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
DrivesModel.setDrives([
{
device: '/dev/sdb',
name: 'Foo',
size: 999999999,
mountpoint: '/mnt/foo',
system: false,
protected: false
},
{
device: '/dev/sdc',
name: 'Bar',
size: 999999999,
mountpoint: '/mnt/bar',
system: false,
protected: false
}
]);
m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
});
it('should auto-select a single valid available drive', function() {
m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
DrivesModel.setDrives([
{
device: '/dev/sdb',
name: 'Foo',
size: 999999999,
mountpoint: '/mnt/foo',
system: false,
protected: false
}
]);
m.chai.expect(SelectionStateModel.getDrive()).to.deep.equal({
device: '/dev/sdb',
name: 'Foo',
size: 999999999,
mountpoint: '/mnt/foo',
system: false,
protected: false
});
});
it('should not auto-select a single too small drive', function() {
m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
DrivesModel.setDrives([
{
device: '/dev/sdb',
name: 'Foo',
size: 99999999,
mountpoint: '/mnt/foo',
system: false,
protected: false
}
]);
m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
});
it('should not auto-select a single protected drive', function() {
m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
DrivesModel.setDrives([
{
device: '/dev/sdb',
name: 'Foo',
size: 999999999,
mountpoint: '/mnt/foo',
system: false,
protected: true
}
]);
m.chai.expect(SelectionStateModel.hasDrive()).to.be.false;
});
});
});
});