mike-jumper commented on a change in pull request #573:
URL: https://github.com/apache/guacamole-client/pull/573#discussion_r579757092
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -68,6 +69,17 @@ angular.module('auth').factory('authenticationService',
['$injector',
*/
var AUTH_STORAGE_KEY = 'GUAC_AUTH';
+ /**
+ * List of Logout handlers that are ensured to be run before this service
+ * forces the page to be reloaded.
+ *
+ * Warning: If a handler has a page reload or redirect in it this can cause
+ * handlers not to be run.
+ *
+ * @type Array of Angular promises
Review comment:
This should be `@type Promise[]` or `@type
Promise.<WhateverItResolvesWith>[]` if the type of resolution is known. See:
https://github.com/apache/guacamole-client/blob/0091bb1aea14c567c8166f0ed8eadf7c31b6bd6e/guacamole/src/main/webapp/app/rest/services/schemaService.js#L45
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService',
['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is
triggered.
+ *
+ * @param {Function} handlerFunction(resolve, reject)
+ * The function that we want to run as a callback to the logout event
+ * The function MUST call either resolve('reason') (success callback)
or
+ * reject('reason') (failure callback). This will allow you to do
async
+ * operations inside the handlerFunction and only trigger
success/failure
+ * when your operations are finished.
+ *
+ */
+ service.registerLogoutHandler = function
registerLogoutHandler(handlerFunction) {
+ logoutHandlers.push(handlerFunction)
+ }
+
+ /**
+ * This private function transforms all of the registered logout handlers
into
+ * promises which will allow the use of $q.all() or Promise.all() to
ensure that
+ * each handler is activated.
+ */
+ let getLogoutHandlerPromises = function getLogoutHandlerPromises() {
+ let promises = [];
+
+ for (x in logoutHandlers) {
+ let handlerFunction = logoutHandlers[x];
+ promises.push(
+ $q((resolve, reject) => {
+ try {
+ handlerFunction(resolve, reject);
+ } catch (e) {
+ reject(e);
+ }
Review comment:
Won't an object thrown within a promise handler implicitly reject the
promise with that object?
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService',
['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is
triggered.
+ *
+ * @param {Function} handlerFunction(resolve, reject)
+ * The function that we want to run as a callback to the logout event
+ * The function MUST call either resolve('reason') (success callback)
or
+ * reject('reason') (failure callback). This will allow you to do
async
+ * operations inside the handlerFunction and only trigger
success/failure
+ * when your operations are finished.
+ *
+ */
+ service.registerLogoutHandler = function
registerLogoutHandler(handlerFunction) {
+ logoutHandlers.push(handlerFunction)
+ }
+
+ /**
+ * This private function transforms all of the registered logout handlers
into
+ * promises which will allow the use of $q.all() or Promise.all() to
ensure that
+ * each handler is activated.
+ */
+ let getLogoutHandlerPromises = function getLogoutHandlerPromises() {
+ let promises = [];
+
+ for (x in logoutHandlers) {
Review comment:
`x` here is implicitly global.
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService',
['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is
triggered.
+ *
+ * @param {Function} handlerFunction(resolve, reject)
+ * The function that we want to run as a callback to the logout event
+ * The function MUST call either resolve('reason') (success callback)
or
+ * reject('reason') (failure callback). This will allow you to do
async
+ * operations inside the handlerFunction and only trigger
success/failure
+ * when your operations are finished.
+ *
Review comment:
What's this blank line about? ;)
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService',
['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is
triggered.
+ *
+ * @param {Function} handlerFunction(resolve, reject)
+ * The function that we want to run as a callback to the logout event
+ * The function MUST call either resolve('reason') (success callback)
or
+ * reject('reason') (failure callback). This will allow you to do
async
+ * operations inside the handlerFunction and only trigger
success/failure
+ * when your operations are finished.
+ *
+ */
+ service.registerLogoutHandler = function
registerLogoutHandler(handlerFunction) {
+ logoutHandlers.push(handlerFunction)
+ }
+
+ /**
+ * This private function transforms all of the registered logout handlers
into
+ * promises which will allow the use of $q.all() or Promise.all() to
ensure that
+ * each handler is activated.
+ */
+ let getLogoutHandlerPromises = function getLogoutHandlerPromises() {
+ let promises = [];
+
+ for (x in logoutHandlers) {
+ let handlerFunction = logoutHandlers[x];
+ promises.push(
+ $q((resolve, reject) => {
Review comment:
Unfortunately, we can't use arrow expressions or `let` within the
Guacamole source for compatibility. We'll need to rely on the more traditional
`var` and `function`.
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService',
['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is
triggered.
+ *
+ * @param {Function} handlerFunction(resolve, reject)
+ * The function that we want to run as a callback to the logout event
+ * The function MUST call either resolve('reason') (success callback)
or
+ * reject('reason') (failure callback). This will allow you to do
async
+ * operations inside the handlerFunction and only trigger
success/failure
+ * when your operations are finished.
Review comment:
There is a handy and specific JSDoc syntax for callback functions:
https://jsdoc.app/tags-param.html#callback-functions
##########
File path: guacamole/src/main/webapp/app/auth/service/authenticationService.js
##########
@@ -394,5 +407,45 @@ angular.module('auth').factory('authenticationService',
['$injector',
};
+ /**
+ * This function accepts a callback function and stores it in an array of
+ * functions that will be run before page reload when a user logout is
triggered.
Review comment:
I recommend avoiding the temptation to document all functions as "This
function ..." and instead write as if that has already been said. This is the
general convention for documenting function behavior, and is the convention we
follow elsewhere.
For example, instead of:
```js
/**
* This function accepts a duration and beeps for that amount of time.
*
* @param {Number} duration
* The number of milliseconds that the beep should last.
*/
var beep = function beep(duration) {
...
};
```
Use:
```js
/**
* Beeps for the specified amount of time.
*
* @param {Number} duration
* The number of milliseconds that the beep should last.
*/
var beep = function beep(duration) {
...
};
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
