Addressing issues from code review

This commit is contained in:
Henry 2016-02-29 17:22:13 -08:00
parent 2cc2c6a9d3
commit f192544be3
13 changed files with 147 additions and 98 deletions

View File

@ -111,9 +111,11 @@ define([
"$scope",
"$route",
"$location",
"$window",
"objectService",
"navigationService",
"urlService",
"policyService",
"DEFAULT_PATH"
]
},

View File

@ -33,6 +33,7 @@ define(
"use strict";
var ROOT_ID = "ROOT",
DEFAULT_PATH = "mine",
CONFIRM_MSG = "Unsaved changes will be lost if you leave this page.";
/**
@ -46,12 +47,14 @@ define(
* @constructor
*/
function BrowseController(
$scope,
$route,
$location,
objectService,
navigationService,
urlService,
$scope,
$route,
$location,
$window,
objectService,
navigationService,
urlService,
policyService,
defaultPath
) {
var path = [ROOT_ID].concat(
@ -81,14 +84,22 @@ 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;
}
if (navigationService.setNavigation(domainObject)) {
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

View File

@ -37,7 +37,7 @@ define(
*/
function NavigationService() {
this.navigated = undefined;
this.callbacks = {};
this.callbacks = [];
}
/**
@ -50,48 +50,27 @@ define(
/**
* Set the current navigation state. This will invoke listeners.
* Changing the navigation state will be blocked if any of the
* 'before' navigation state change listeners return 'false'.
* @param {DomainObject} domainObject the domain object to navigate to
*/
NavigationService.prototype.setNavigation = function (value) {
var canNavigate = true;
if (this.navigated !== value) {
canNavigate = (this.callbacks.before || [])
.reduce(function (previous, callback) {
//Check whether the callback returned a value of
// 'false' indicating that navigation should not
// continue. All other return values will allow
// navigation to continue
return (callback(value)!==false) && previous;
}, true);
if (canNavigate) {
this.navigated = value;
(this.callbacks.after || []).forEach(function (callback) {
callback(value);
});
}
this.navigated = value;
this.callbacks.forEach(function (callback) {
callback(value);
});
}
return canNavigate;
return true;
};
/**
* Listen for changes in navigation. The passed callback will
* be invoked with the new domain object of navigation when
* this changes. Callbacks can be registered to listen to pre or
* post-navigation events. The event to listen to is specified using
* the event parameter. In the case of pre-navigation events
* returning a false value will prevent the navigation event from
* going ahead.
* this changes.
* @param {function} callback the callback to invoke when
* navigation state changes
* @param {string} [event=after] the navigation event to listen to.
* One of 'before' or 'after'.
*/
NavigationService.prototype.addListener = function (callback, event) {
event = event || 'after';
this.callbacks[event] = this.callbacks[event] || [];
this.callbacks[event].push(callback);
NavigationService.prototype.addListener = function (callback) {
this.callbacks.push(callback);
};
/**
@ -99,12 +78,9 @@ define(
* @param {function} callback the callback which should
* no longer be invoked when navigation state
* changes
* @param {string} [event=after] the navigation event that the
* callback is registered to. One of 'before' or 'after'.
*/
NavigationService.prototype.removeListener = function (callback, event) {
event = event || 'after';
this.callbacks[event] = this.callbacks[event].filter(function (cb) {
NavigationService.prototype.removeListener = function (callback) {
this.callbacks = this.callbacks.filter(function (cb) {
return cb !== callback;
});
};

View File

@ -39,6 +39,8 @@ define(
mockUrlService,
mockDomainObject,
mockNextObject,
mockWindow,
mockPolicyService,
testDefaultRoot,
controller;
@ -55,14 +57,25 @@ 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(

View File

@ -84,24 +84,6 @@ define(
expect(callback).not.toHaveBeenCalled();
});
it("adds listeners to the 'after' state by default", function(){
expect(navigationService.callbacks.after).toBeUndefined();
navigationService.addListener(function(){});
expect(navigationService.callbacks.after).toBeDefined();
expect(navigationService.callbacks.after.length).toBe(1);
});
it("allows navigationService events to be prevented", function(){
var callback = jasmine.createSpy("callback"),
navigationResult;
callback.andReturn(false);
navigationService.addListener(callback, "before");
navigationResult = navigationService.setNavigation({});
expect(callback).toHaveBeenCalled();
expect(navigationResult).toBe(false);
});
});
}
);

View File

@ -34,6 +34,7 @@ define([
"./src/actions/SaveAction",
"./src/actions/CancelAction",
"./src/policies/EditActionPolicy",
"./src/policies/EditNavigationPolicy",
"./src/representers/EditRepresenter",
"./src/representers/EditToolbarRepresenter",
"text!./res/templates/library.html",
@ -55,6 +56,7 @@ define([
SaveAction,
CancelAction,
EditActionPolicy,
EditNavigationPolicy,
EditRepresenter,
EditToolbarRepresenter,
libraryTemplate,
@ -95,7 +97,8 @@ define([
"implementation": EditObjectController,
"depends": [
"$scope",
"$location"
"$location",
"policyService"
]
}
],
@ -183,7 +186,16 @@ define([
{
"category": "action",
"implementation": EditActionPolicy
},
{
"category": "navigation",
"message": "There are unsaved changes.",
"implementation": EditNavigationPolicy,
"depends": [
"$window"
]
}
],
"templates": [
{

View File

@ -124,7 +124,6 @@ define(
*/
EditorCapability.prototype.cancel = function () {
this.editableObject.getCapability("status").set("editing", false);
//TODO: Reset the cache as well here.
this.cache.markClean();
return resolvePromise(undefined);
};

View File

@ -36,8 +36,9 @@ define(
* @memberof platform/commonUI/edit
* @constructor
*/
function EditObjectController($scope, $location) {
function EditObjectController($scope, $location, policyService) {
this.scope = $scope;
this.policyService = policyService;
var navigatedObject;
function setViewForDomainObject(domainObject) {
@ -73,16 +74,14 @@ define(
*/
EditObjectController.prototype.getUnloadWarning = function () {
var navigatedObject = this.scope.domainObject,
editorCapability = navigatedObject &&
navigatedObject.getCapability("editor"),
statusCapability = navigatedObject &&
navigatedObject.getCapability("status"),
hasChanges = statusCapability && statusCapability.get('editing')
&& editorCapability && editorCapability.dirty();
policyMessage;
this.policyService.allow("navigation", navigatedObject, undefined, function(message) {
policyMessage = message;
});
return policyMessage;
return hasChanges ?
"Unsaved changes will be lost if you leave this page." :
undefined;
};
return EditObjectController;

View File

@ -35,7 +35,7 @@ define(
* @constructor
* @param $window the window
*/
function MCTBeforeUnload($window, navigationService) {
function MCTBeforeUnload($window) {
var unloads = [],
oldBeforeUnload = $window.onbeforeunload;
@ -57,7 +57,6 @@ define(
// Stop using this unload expression
function removeUnload() {
navigationService.removeListener(checkNavigationEvent, "before");
unloads = unloads.filter(function (callback) {
return callback !== unload;
});
@ -66,28 +65,17 @@ define(
}
}
function shouldAllowNavigation(){
// Show a dialog before allowing a location change
function checkLocationChange(event) {
// Get an unload message (if any)
var warning = unload();
// Prompt the user if there's an unload message
return !warning || $window.confirm(warning);
}
// Show a dialog before allowing a location change
function checkLocationChange(event) {
if (!shouldAllowNavigation()) {
// Prevent the route change if it was confirmed
if (warning && !$window.confirm(warning)) {
// ...and prevent the route change if it was confirmed
event.preventDefault();
}
}
// Show a dialog before allowing a location change
function checkNavigationEvent(event) {
// Return a false value to the navigationService to
// indicate that the navigation event should be prevented
return shouldAllowNavigation();
}
// If this is the first active instance of this directive,
// register as the window's beforeunload handler
if (unloads.length === 0) {
@ -102,8 +90,6 @@ define(
// Also handle route changes
scope.$on("$locationChangeStart", checkLocationChange);
navigationService.addListener(checkNavigationEvent, "before");
}
return {

View File

@ -0,0 +1,68 @@
/*****************************************************************************
* Open MCT Web, Copyright (c) 2014-2015, United States Government
* as represented by the Administrator of the National Aeronautics and Space
* Administration. All rights reserved.
*
* Open MCT Web is licensed under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* http://www.apache.org/licenses/LICENSE-2.0.
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*
* Open MCT Web includes source code licensed under additional open source
* licenses. See the Open Source Licenses file (LICENSES.md) included with
* this source code distribution or the Licensing information page available
* at runtime from the About dialog for additional information.
*****************************************************************************/
/*global define*/
define(
[],
function () {
"use strict";
/**
* Policy controlling whether navigation events should proceed
* when object is being edited.
* @memberof platform/commonUI/edit
* @constructor
* @implements {Policy.<Action, ActionContext>}
*/
function EditNavigationPolicy($window, policyService) {
this.window = $window;
this.policyService = policyService;
}
/**
* @private
*/
EditNavigationPolicy.prototype.isDirty = function(domainObject) {
var navigatedObject = domainObject,
editorCapability = navigatedObject &&
navigatedObject.getCapability("editor"),
statusCapability = navigatedObject &&
navigatedObject.getCapability("status");
return statusCapability && statusCapability.get('editing')
&& editorCapability && editorCapability.dirty();
};
/**
* Allow navigation if an object is not dirty, or if the user elects
* to proceed anyway.
* @param currentNavigation
* @returns {boolean|*} true if the object model is clean; or if
* it's dirty and the user wishes to proceed anyway.
*/
EditNavigationPolicy.prototype.allow = function (currentNavigation) {
return !this.isDirty(currentNavigation);
};
return EditNavigationPolicy;
}
);

View File

@ -147,7 +147,6 @@ define(
// Respond to the destruction of the current representation.
EditRepresenter.prototype.destroy = function destroy() {
// Nothing to clean up
return this.listenHandle && this.listenHandle();
};

View File

@ -33,6 +33,7 @@ define(
mockLocation,
mockStatusCapability,
mockCapabilities,
mockPolicyService,
controller;
// Utility function; look for a $watch on scope and fire it
@ -45,6 +46,12 @@ define(
}
beforeEach(function () {
mockPolicyService = jasmine.createSpyObj(
"policyService",
[
"allow"
]
);
mockScope = jasmine.createSpyObj(
"$scope",
[ "$on", "$watch" ]

View File

@ -128,8 +128,7 @@ define(
var typeKey = 'telemetry.panel',
type = typeService.getType(typeKey),
model = type.getInitialModel(),
newPanel,
composeAction;
newPanel;
model.type = typeKey;
newPanel = new EditableDomainObject(instantiate(model), $q);
@ -166,16 +165,12 @@ define(
editableDomainObject = createVirtualPanel(domainObject, selectedObject);
if (editableDomainObject) {
editableDomainObject.getCapability('action').perform('edit');
//navigationService.setNavigation(editableDomainObject);
broadcastDrop(id, event);
//editableDomainObject.getCapability('status').set('editing', true);
}
} else {
$q.when(action && action.perform()).then(function (result) {
//Don't go into edit mode for folders
if (domainObjectType!=='folder') {
// navigationService.setNavigation(editableDomainObject);
//editableDomainObject.getCapability('status').set('editing', true);
editableDomainObject.getCapability('action').perform('edit');
}
broadcastDrop(id, event);