From 58b14a83bce12cceeeabcc3531f4b2c842591b5f Mon Sep 17 00:00:00 2001 From: Bence Romsics Date: Mon, 14 Aug 2017 15:30:51 +0200 Subject: [PATCH] transfer-table: Rethink CHANGED message handler 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 --- .../transfer-table.controller.js | 32 +++++++++-------- .../transfer-table.controller.spec.js | 34 ++++++++++++------- .../transfer-table/transfer-table.module.js | 2 +- .../source/source.controller.js | 2 +- ...-table-avail-changed-cfae61341b5fea71.yaml | 13 +++++++ 5 files changed, 54 insertions(+), 29 deletions(-) create mode 100644 releasenotes/notes/remove-transfer-table-avail-changed-cfae61341b5fea71.yaml diff --git a/horizon/static/framework/widgets/transfer-table/transfer-table.controller.js b/horizon/static/framework/widgets/transfer-table/transfer-table.controller.js index e140d09718..1b29f189d4 100644 --- a/horizon/static/framework/widgets/transfer-table/transfer-table.controller.js +++ b/horizon/static/framework/widgets/transfer-table/transfer-table.controller.js @@ -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; } } diff --git a/horizon/static/framework/widgets/transfer-table/transfer-table.controller.spec.js b/horizon/static/framework/widgets/transfer-table/transfer-table.controller.spec.js index 522b521e6f..941c255099 100644 --- a/horizon/static/framework/widgets/transfer-table/transfer-table.controller.spec.js +++ b/horizon/static/framework/widgets/transfer-table/transfer-table.controller.spec.js @@ -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 diff --git a/horizon/static/framework/widgets/transfer-table/transfer-table.module.js b/horizon/static/framework/widgets/transfer-table/transfer-table.module.js index 057a6d4e55..3498dd60a9 100644 --- a/horizon/static/framework/widgets/transfer-table/transfer-table.module.js +++ b/horizon/static/framework/widgets/transfer-table/transfer-table.module.js @@ -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' }; } diff --git a/openstack_dashboard/dashboards/project/static/dashboard/project/workflow/launch-instance/source/source.controller.js b/openstack_dashboard/dashboards/project/static/dashboard/project/workflow/launch-instance/source/source.controller.js index 9f11c3b2ec..e69c5c3676 100644 --- a/openstack_dashboard/dashboards/project/static/dashboard/project/workflow/launch-instance/source/source.controller.js +++ b/openstack_dashboard/dashboards/project/static/dashboard/project/workflow/launch-instance/source/source.controller.js @@ -334,7 +334,7 @@ }, function onBootSourceChange(newValue, oldValue) { if (newValue !== oldValue) { - $scope.$broadcast(events.AVAIL_CHANGED, { + $scope.$broadcast(events.TABLES_CHANGED, { 'data': bootSources[newValue] }); } diff --git a/releasenotes/notes/remove-transfer-table-avail-changed-cfae61341b5fea71.yaml b/releasenotes/notes/remove-transfer-table-avail-changed-cfae61341b5fea71.yaml new file mode 100644 index 0000000000..d37fe30357 --- /dev/null +++ b/releasenotes/notes/remove-transfer-table-avail-changed-cfae61341b5fea71.yaml @@ -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.