openstack/horizon
Code Issues Proposed changes

transfer-table: Rethink CHANGED message handler

Browse Source 
Part of the fix to bug #1489618 was working only accidentally. The
implicit update logic of the allocated table (and the allocatedIds dict)
treated ctrl.available.sourceItems and ctrl.allocated.sourceItems as
parallel arrays, while they were not parallel in fact.

This change allows the sender of the CHANGED message to specify all four
tables and by that spare any implicit logic of updating some of the
tables. However if a table is not included in the CHANGED message it
will be left unchanged. The event is also renamed according to the new
meaning.

The single sender of the original message from the horizon repo (ie.
Launch Instance / Source) is updated.

The original message type and its handler logic is removed without
deprecation. That theoretically could cause problems for horizon plugins
outside of the horizon repo. But I find that unlikely because if
somebody had relied on that logic they would have likely discovered
already that it was faulty.

Change-Id: I38972558e1823f9a88702d2ebcb8de5244cfe16a
Related-Change: I647b31c7a280af4e10040fb27b4436d489fd8163
Related-Bug: #1489618
This commit is contained in:
Bence Romsics 2017-08-14 15:30:51 +02:00
parent b5896d6fbb
commit 58b14a83bc
5 changed files with 54 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

30
horizon/static/framework/widgets/transfer-table/transfer-table.controller.js
View File

@ -90,10 +90,10 @@
// if available transfer table is updated dynamically (e.g. based on a dropdown
// selection like in Launch Instance Boot Source), we need to update our data accordingly
var availableChangedWatcher = $scope.$on(events.AVAIL_CHANGED, onAvailChanged);
var tablesChangedWatcher = $scope.$on(events.TABLES_CHANGED, onTablesChanged);
$scope.$on('$destroy', function () {
availableChangedWatcher();
tablesChangedWatcher();
});
init(trModel);
@ -208,18 +208,22 @@
Array.prototype.push.apply(ctrl.allocated.sourceItems, orderedItems);
}
function onAvailChanged(e, args) {
ctrl.available = {
sourceItems: args.data.available,
displayedItems: args.data.displayedAvailable ? args.data.displayedAvailable : []
};
for (var i = 0; i < ctrl.available.sourceItems.length; i++) {
var item = ctrl.available.sourceItems[i];
if (item.id in ctrl.allocatedIds) {
ctrl.allocated.sourceItems.splice(i, 1);
delete ctrl.allocatedIds[item.id];
function onTablesChanged(e, args) {
if (args.data.available) {
ctrl.available.sourceItems = args.data.available;
}
if (args.data.displayedAvailable) {
ctrl.available.displayedItems = args.data.displayedAvailable;
}
if (args.data.allocated) {
ctrl.allocated.sourceItems = args.data.allocated;
ctrl.allocatedIds = {};
ctrl.allocated.sourceItems.forEach(function(item) {
ctrl.allocatedIds[item.id] = true;
});
}
if (args.data.displayedAllocated) {
ctrl.allocated.displayedItems = args.data.displayedAllocated;
}
}

34
horizon/static/framework/widgets/transfer-table/transfer-table.controller.spec.js
View File

@ -205,19 +205,27 @@
}
function testTransferTableChanged() {
var oldAvailableCount = 10;
trCtrl.available.sourceItems = generateItems(oldAvailableCount);
expect(trCtrl.available.sourceItems.length).toEqual(oldAvailableCount);
var availableCount = 4;
var newItems = {
"data": { available: generateItems(availableCount) }
};
spyOn(scope, '$broadcast').and.callThrough();
scope.$broadcast('horizon.framework.widgets.transfer-table.AVAIL_CHANGED', newItems);
expect(scope.$broadcast).toHaveBeenCalledWith(
'horizon.framework.widgets.transfer-table.AVAIL_CHANGED', newItems);
expect(trCtrl.available.sourceItems.length).toEqual(availableCount);
scope.$broadcast(
'horizon.framework.widgets.transfer-table.TABLES_CHANGED',
{data: {available: [{id: 1}]}}
);
scope.$broadcast(
'horizon.framework.widgets.transfer-table.TABLES_CHANGED',
{data: {displayedAvailable: [{id: 2}]}}
);
scope.$broadcast(
'horizon.framework.widgets.transfer-table.TABLES_CHANGED',
{data: {allocated: [{id: 3}]}}
);
scope.$broadcast(
'horizon.framework.widgets.transfer-table.TABLES_CHANGED',
{data: {displayedAllocated: [{id: 4}]}}
);
expect(trCtrl.available.sourceItems).toEqual([{id: 1}]);
expect(trCtrl.available.displayedItems).toEqual([{id: 2}]);
expect(trCtrl.allocated.sourceItems).toEqual([{id: 3}]);
expect(trCtrl.allocatedIds).toEqual({3: true});
expect(trCtrl.allocated.displayedItems).toEqual([{id: 4}]);
}
}); // end of core functions

2
horizon/static/framework/widgets/transfer-table/transfer-table.module.js
View File

@ -82,7 +82,7 @@
*/
function events() {
return {
AVAIL_CHANGED: 'horizon.framework.widgets.transfer-table.AVAIL_CHANGED'
TABLES_CHANGED: 'horizon.framework.widgets.transfer-table.TABLES_CHANGED'
};
}

2
openstack_dashboard/dashboards/project/static/dashboard/project/workflow/launch-instance/source/source.controller.js
View File

@ -334,7 +334,7 @@
},
function onBootSourceChange(newValue, oldValue) {
if (newValue !== oldValue) {
$scope.$broadcast(events.AVAIL_CHANGED, {
$scope.$broadcast(events.TABLES_CHANGED, {
'data': bootSources[newValue]
});
}

13
releasenotes/notes/remove-transfer-table-avail-changed-cfae61341b5fea71.yaml Normal file
View File

@ -0,0 +1,13 @@
---
other:
- |
(For Horizon plugin developers) The AVAIL_CHANGED event of transfer
table is removed. It is superseded by event TABLES_CHANGED. The
name of AVAIL_CHANGED was misleading because it implicitly and
uncontrollably updated the allocated table too. The new event allows
independent updates to all four tables. We believe it is safe to
remove AVAIL_CHANGED without deprecation because its implementation
contained a bug that must have been discovered before if anybody
had used it. Anyway possible out-of-tree plugin maintainers are
recommended to consume the new event even if your plugins relied on
the buggy behavior of AVAIL_CHANGED.