From c2517c167068ae1d8ef9253f8133c77067d7a85a Mon Sep 17 00:00:00 2001 From: Henry Date: Wed, 15 Jun 2016 17:10:29 -0700 Subject: [PATCH] [Edit Mode] Canceling edit mode with unsaved changes now shows confirmation dialog to user. Fixes #664 --- platform/commonUI/browse/bundle.js | 6 +- .../commonUI/browse/src/BrowseController.js | 33 +++++------ .../browse/src/navigation/NavigateAction.js | 17 +++++- .../browse/test/BrowseControllerSpec.js | 36 ++++++------ .../test/navigation/NavigateActionSpec.js | 55 +++++++++++++++++-- .../commonUI/edit/src/actions/CancelAction.js | 14 +++-- .../edit/test/actions/CancelActionSpec.js | 9 ++- .../src/controllers/TableOptionsController.js | 6 +- 8 files changed, 121 insertions(+), 55 deletions(-) diff --git a/platform/commonUI/browse/bundle.js b/platform/commonUI/browse/bundle.js index eccf3e96d7..9fb7015456 100644 --- a/platform/commonUI/browse/bundle.js +++ b/platform/commonUI/browse/bundle.js @@ -93,11 +93,9 @@ define([ "$scope", "$route", "$location", - "$window", "objectService", "navigationService", "urlService", - "policyService", "DEFAULT_PATH" ] }, @@ -201,7 +199,9 @@ define([ "implementation": NavigateAction, "depends": [ "navigationService", - "$q" + "$q", + "policyService", + "$window" ] }, { diff --git a/platform/commonUI/browse/src/BrowseController.js b/platform/commonUI/browse/src/BrowseController.js index 53ddc14e79..1c00502a6f 100644 --- a/platform/commonUI/browse/src/BrowseController.js +++ b/platform/commonUI/browse/src/BrowseController.js @@ -44,11 +44,9 @@ define( $scope, $route, $location, - $window, objectService, navigationService, urlService, - policyService, defaultPath ) { var path = [ROOT_ID].concat( @@ -75,25 +73,10 @@ define( } - // Callback for updating the in-scope reference to the object - // that is currently navigated-to. - function setNavigation(domainObject) { - var navigationAllowed = true; - - if (domainObject === $scope.navigatedObject) { - //do nothing; - return; - } - - policyService.allow("navigation", $scope.navigatedObject, domainObject, function (message) { - navigationAllowed = $window.confirm(message + "\r\n\r\n" + - " Are you sure you want to continue?"); - }); - + function setScopeObjects(domainObject, navigationAllowed) { if (navigationAllowed) { $scope.navigatedObject = domainObject; $scope.treeModel.selectedObject = domainObject; - navigationService.setNavigation(domainObject); updateRoute(domainObject); } else { //If navigation was unsuccessful (ie. blocked), reset @@ -103,6 +86,20 @@ define( } } + // Callback for updating the in-scope reference to the object + // that is currently navigated-to. + function setNavigation(domainObject) { + if (domainObject === $scope.navigatedObject) { + //do nothing; + return; + } + if (domainObject) { + domainObject.getCapability("action").perform("navigate").then(setScopeObjects.bind(undefined, domainObject)); + } else { + setScopeObjects(domainObject, true); + } + } + function navigateTo(domainObject) { // Check if an object has been navigated-to already... diff --git a/platform/commonUI/browse/src/navigation/NavigateAction.js b/platform/commonUI/browse/src/navigation/NavigateAction.js index e9cc700f93..d1028f754d 100644 --- a/platform/commonUI/browse/src/navigation/NavigateAction.js +++ b/platform/commonUI/browse/src/navigation/NavigateAction.js @@ -33,10 +33,12 @@ define( * @constructor * @implements {Action} */ - function NavigateAction(navigationService, $q, context) { + function NavigateAction(navigationService, $q, policyService, $window, context) { this.domainObject = context.domainObject; this.$q = $q; this.navigationService = navigationService; + this.policyService = policyService; + this.$window = $window; } /** @@ -45,9 +47,20 @@ define( * navigation has been updated */ NavigateAction.prototype.perform = function () { + var self = this, + navigationAllowed = true; + + function allow() { + self.policyService.allow("navigation", self.navigationService.getNavigation(), self.domainObject, function (message) { + navigationAllowed = self.$window.confirm(message + "\r\n\r\n" + + " Are you sure you want to continue?"); + }); + return navigationAllowed; + } + // Set navigation, and wrap like a promise return this.$q.when( - this.navigationService.setNavigation(this.domainObject) + allow() && this.navigationService.setNavigation(this.domainObject) ); }; diff --git a/platform/commonUI/browse/test/BrowseControllerSpec.js b/platform/commonUI/browse/test/BrowseControllerSpec.js index 2c20467185..29d198231b 100644 --- a/platform/commonUI/browse/test/BrowseControllerSpec.js +++ b/platform/commonUI/browse/test/BrowseControllerSpec.js @@ -37,9 +37,8 @@ define( mockUrlService, mockDomainObject, mockNextObject, - mockWindow, - mockPolicyService, testDefaultRoot, + mockActionCapability, controller; function mockPromise(value) { @@ -55,25 +54,14 @@ define( mockScope, mockRoute, mockLocation, - mockWindow, mockObjectService, mockNavigationService, mockUrlService, - mockPolicyService, testDefaultRoot ); } beforeEach(function () { - mockWindow = jasmine.createSpyObj('$window', [ - "confirm" - ]); - mockWindow.confirm.andReturn(true); - - mockPolicyService = jasmine.createSpyObj('policyService', [ - 'allow' - ]); - testDefaultRoot = "some-root-level-domain-object"; mockScope = jasmine.createSpyObj( @@ -128,6 +116,8 @@ define( mockNextObject.getId.andReturn("next"); mockDomainObject.getId.andReturn(testDefaultRoot); + mockActionCapability = jasmine.createSpyObj('actionCapability', ['perform']); + instantiateController(); }); @@ -211,8 +201,13 @@ define( mockContext.getPath.andReturn( [mockRootObject, mockDomainObject, mockNextObject] ); + + //Return true from navigate action + mockActionCapability.perform.andReturn(mockPromise(true)); + mockNextObject.getCapability.andCallFake(function (c) { - return c === 'context' && mockContext; + return (c === 'context' && mockContext) || + (c === 'action' && mockActionCapability); }); mockScope.$on.andReturn(mockUnlisten); // Provide a navigation change @@ -225,6 +220,7 @@ define( mockLocation.path.andReturn("/browse/"); mockNavigationService.setNavigation.andReturn(true); + mockActionCapability.perform.andReturn(mockPromise(true)); // Exercise the Angular workaround mockNavigationService.addListener.mostRecentCall.args[0](); @@ -243,6 +239,9 @@ define( mockScope.navigatedObject = mockDomainObject; mockNavigationService.setNavigation.andReturn(true); + mockActionCapability.perform.andReturn(mockPromise(true)); + mockNextObject.getCapability.andReturn(mockActionCapability); + //Simulate a change in selected tree object mockScope.treeModel = {selectedObject: mockDomainObject}; mockScope.$watch.mostRecentCall.args[1](mockNextObject); @@ -254,11 +253,10 @@ define( it("after failed navigation event resets the selected tree" + " object", function () { mockScope.navigatedObject = mockDomainObject; - mockWindow.confirm.andReturn(false); - mockPolicyService.allow.andCallFake(function (category, object, context, callback) { - callback("unsaved changes"); - return false; - }); + + //Return false from navigation action + mockActionCapability.perform.andReturn(mockPromise(false)); + mockNextObject.getCapability.andReturn(mockActionCapability); //Simulate a change in selected tree object mockScope.treeModel = {selectedObject: mockDomainObject}; diff --git a/platform/commonUI/browse/test/navigation/NavigateActionSpec.js b/platform/commonUI/browse/test/navigation/NavigateActionSpec.js index d872022049..c43d76373c 100644 --- a/platform/commonUI/browse/test/navigation/NavigateActionSpec.js +++ b/platform/commonUI/browse/test/navigation/NavigateActionSpec.js @@ -31,6 +31,8 @@ define( var mockNavigationService, mockQ, mockDomainObject, + mockPolicyService, + mockWindow, action; function mockPromise(value) { @@ -44,25 +46,70 @@ define( beforeEach(function () { mockNavigationService = jasmine.createSpyObj( "navigationService", - ["setNavigation"] + [ + "setNavigation", + "getNavigation" + ] ); + mockNavigationService.getNavigation.andReturn({}); mockQ = { when: mockPromise }; mockDomainObject = jasmine.createSpyObj( "domainObject", ["getId", "getModel", "getCapability"] ); + mockPolicyService = jasmine.createSpyObj("policyService", + [ + "allow" + ]); + mockWindow = jasmine.createSpyObj("$window", + [ + "confirm" + ]); + action = new NavigateAction( mockNavigationService, mockQ, + mockPolicyService, + mockWindow, { domainObject: mockDomainObject } ); }); - it("invokes the navigate service when performed", function () { + it("invokes the policy service to determine if navigation" + + " allowed", function () { action.perform(); - expect(mockNavigationService.setNavigation) - .toHaveBeenCalledWith(mockDomainObject); + expect(mockPolicyService.allow) + .toHaveBeenCalledWith("navigation", jasmine.any(Object), jasmine.any(Object), jasmine.any(Function)); + }); + + it("prompts user if policy rejection", function () { + action.perform(); + expect(mockPolicyService.allow).toHaveBeenCalled(); + mockPolicyService.allow.mostRecentCall.args[3](); + expect(mockWindow.confirm).toHaveBeenCalled(); + }); + + describe("shows a prompt", function () { + beforeEach(function () { + // Ensure the allow callback is called synchronously + mockPolicyService.allow.andCallFake(function () { + return arguments[3](); + }); + }); + it("does not navigate on prompt rejection", function () { + mockWindow.confirm.andReturn(false); + action.perform(); + expect(mockNavigationService.setNavigation) + .not.toHaveBeenCalled(); + }); + + it("does navigate on prompt acceptance", function () { + mockWindow.confirm.andReturn(true); + action.perform(); + expect(mockNavigationService.setNavigation) + .toHaveBeenCalled(); + }); }); it("is only applicable when a domain object is in context", function () { diff --git a/platform/commonUI/edit/src/actions/CancelAction.js b/platform/commonUI/edit/src/actions/CancelAction.js index 9fc02a6121..598daa3f9e 100644 --- a/platform/commonUI/edit/src/actions/CancelAction.js +++ b/platform/commonUI/edit/src/actions/CancelAction.js @@ -50,18 +50,24 @@ define( //If the object existed already, navigate to refresh view // with previous object state. if (domainObject.getModel().persisted) { - domainObject.getCapability("action").perform("navigate"); + return domainObject.getCapability("action").perform("navigate"); } else { //If the object was new, and user has cancelled, then //navigate back to parent because nothing to show. - domainObject.getCapability("location").getOriginal().then(function (original) { + return domainObject.getCapability("location").getOriginal().then(function (original) { parent = original.getCapability("context").getParent(); parent.getCapability("action").perform("navigate"); }); } } - return this.domainObject.getCapability("editor").cancel() - .then(returnToBrowse); + + function cancel(allowed) { + return allowed && domainObject.getCapability("editor").cancel(); + } + + //Do navigation first in order to trigger unsaved changes dialog + return returnToBrowse() + .then(cancel); }; /** diff --git a/platform/commonUI/edit/test/actions/CancelActionSpec.js b/platform/commonUI/edit/test/actions/CancelActionSpec.js index b83fbdab2b..d2cf6ea9fa 100644 --- a/platform/commonUI/edit/test/actions/CancelActionSpec.js +++ b/platform/commonUI/edit/test/actions/CancelActionSpec.js @@ -125,6 +125,9 @@ define( it("invokes the editor capability's cancel functionality when" + " performed", function () { + mockDomainObject.getModel.andReturn({persisted: 1}); + //Return true from navigate action + capabilities.action.perform.andReturn(mockPromise(true)); action.perform(); // Should have called cancel @@ -134,13 +137,15 @@ define( expect(capabilities.editor.save).not.toHaveBeenCalled(); }); - it("navigates to object if existing", function () { + it("navigates to object if existing using navigate action", function () { mockDomainObject.getModel.andReturn({persisted: 1}); + //Return true from navigate action + capabilities.action.perform.andReturn(mockPromise(true)); action.perform(); expect(capabilities.action.perform).toHaveBeenCalledWith("navigate"); }); - it("navigates to parent if new", function () { + it("navigates to parent if new using navigate action", function () { mockDomainObject.getModel.andReturn({persisted: undefined}); action.perform(); expect(parentCapabilities.action.perform).toHaveBeenCalledWith("navigate"); diff --git a/platform/features/table/src/controllers/TableOptionsController.js b/platform/features/table/src/controllers/TableOptionsController.js index 0ca83b088c..b017261615 100644 --- a/platform/features/table/src/controllers/TableOptionsController.js +++ b/platform/features/table/src/controllers/TableOptionsController.js @@ -72,10 +72,10 @@ define( * Maintain a configuration object on scope that stores column * configuration. On change, synchronize with object model. */ - $scope.$watchCollection('configuration.table.columns', function (columns) { - if (columns) { + $scope.$watchCollection('configuration.table.columns', function (newColumns, oldColumns) { + if (newColumns !== oldColumns) { self.domainObject.useCapability('mutation', function (model) { - model.configuration.table.columns = columns; + model.configuration.table.columns = newColumns; }); self.domainObject.getCapability('persistence').persist(); }