[Edit Mode] Canceling edit mode with unsaved changes now shows confirmation dialog to user. Fixes #664

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"
                     ]
                 },
                 {

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
+            function setScopeObjects(domainObject, navigationAllowed) {
-            // 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?");
-                });
-
                 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...

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)
             );
         };
 

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) {
+                //Return false from navigation action
-                    callback("unsaved changes");
+                mockActionCapability.perform.andReturn(mockPromise(false));
-                    return false;
+                mockNextObject.getCapability.andReturn(mockActionCapability);
-                });
 
                 //Simulate a change in selected tree object
                 mockScope.treeModel = {selectedObject: mockDomainObject};

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(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)
-                    .toHaveBeenCalledWith(mockDomainObject);
+                        .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 () {

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);
         };
 
         /**

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");

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) {
+            $scope.$watchCollection('configuration.table.columns', function (newColumns, oldColumns) {
-                if (columns) {
+                if (newColumns !== oldColumns) {
                     self.domainObject.useCapability('mutation', function (model) {
-                        model.configuration.table.columns = columns;
+                        model.configuration.table.columns = newColumns;
                     });
                     self.domainObject.getCapability('persistence').persist();
                 }