You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2018/02/19 14:59:08 UTC
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
GitHub user necouchman opened a pull request:
https://github.com/apache/guacamole-client/pull/255
GUACAMOLE-508: Take user to home page when Settings pages are available.
Took a stab at an implementation of the improvement for 508, taking user to home screen when Settings pages are available. It's worth noting that the "Preferences" page for users is pretty much always available, so this only does it if there is more than one Settings page available.
Not sure it's the most efficient route...open to suggestions.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/guacamole-client GUACAMOLE-508
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-client/pull/255.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #255
----
commit 87eac9803530d4ed9d8d554a162e5edc0b972b27
Author: Nick Couchman <vn...@...>
Date: 2018-02-19T14:57:07Z
GUACAMOLE-508: Take user to home page when Settings pages are available.
----
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r172008665
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -57,12 +57,24 @@ angular.module('navigation').factory('userPageService', ['$injector',
* A map of all root connection groups visible to the current user,
* where each key is the identifier of the corresponding data source.
*
+ * @param {Object.<String, PermissionSet>} permissions
+ * A map of all permissions granted to the current user, where each
+ * key is the identifier of the corresponding data source.
+ *
+ * @param {Object.<String,
--- End diff --
Looks like some copypasta snuck in here.
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r172016020
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -57,12 +57,24 @@ angular.module('navigation').factory('userPageService', ['$injector',
* A map of all root connection groups visible to the current user,
* where each key is the identifier of the corresponding data source.
*
+ * @param {Object.<String, PermissionSet>} permissions
+ * A map of all permissions granted to the current user, where each
+ * key is the identifier of the corresponding data source.
+ *
+ * @param {Object.<String,
--- End diff --
Ha. I started to type it myself, then copied it from the function that calls this one and forgot to remove the copy I started to type. Like any good pasta, I've cleaned it up.
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r171997603
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -140,13 +144,20 @@ angular.module('navigation').factory('userPageService', ['$injector',
var deferred = $q.defer();
// Resolve promise using home page derived from root connection groups
- dataSourceService.apply(
+ var getRootGroups = dataSourceService.apply(
connectionGroupService.getConnectionGroupTree,
authenticationService.getAvailableDataSources(),
ConnectionGroup.ROOT_IDENTIFIER
- )
- .then(function rootConnectionGroupsRetrieved(rootGroups) {
- deferred.resolve(generateHomePage(rootGroups));
+ );
+ var getPermissionSets = dataSourceService.apply(
+ permissionService.getPermissions,
+ authenticationService.getAvailableDataSources(),
+ authenticationService.getCurrentUsername()
+ );
+
+ $q.all([getRootGroups,getPermissionSets])
+ .then(function rootConnectionGroupsPermissionsRetrieved(data) {
+ deferred.resolve(generateHomePage(data[0],data[1]));
--- End diff --
According to [the AngularJS documentation for `$q.all()`](https://code.angularjs.org/1.3.16/docs/api/ng/service/$q#all), the function can accept either an array (in which case `data` will be a parallel array, as you use here) or an object (in which case `data` will be an object with identical keys pointing to the results). For the sake of readability, I suggest switching to the latter.
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r171997077
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -60,9 +60,13 @@ angular.module('navigation').factory('userPageService', ['$injector',
* @returns {PageDefinition}
* The user's home page.
*/
- var generateHomePage = function generateHomePage(rootGroups) {
+ var generateHomePage = function generateHomePage(rootGroups,permissions) {
var homePage = null;
+ var settingsPages = generateSettingsPages(permissions);
+
+ if (settingsPages.length > 1)
--- End diff --
I'm generally OK with this check (and honestly can't think of a better way), but please include a comment to document what's going on here. The need for this check is not inherently clear.
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r172008724
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -57,12 +57,24 @@ angular.module('navigation').factory('userPageService', ['$injector',
* A map of all root connection groups visible to the current user,
* where each key is the identifier of the corresponding data source.
*
+ * @param {Object.<String, PermissionSet>} permissions
+ * A map of all permissions granted to the current user, where each
+ * key is the identifier of the corresponding data source.
+ *
+ * @param {Object.<String,
+ *
* @returns {PageDefinition}
* The user's home page.
*/
- var generateHomePage = function generateHomePage(rootGroups) {
+ var generateHomePage = function generateHomePage(rootGroups, permissions) {
var homePage = null;
+ var settingsPages = generateSettingsPages(permissions);
+
+ // If we have more than one setting page, return the main home page
--- End diff --
Better, but the critical fact we want to highlight here is that _more than one settings page indicates that the user has administrative privileges_, and thus redirecting to a single connection would hurt the user experience. The fact that this comparison checks for more than one page is clear from the `> 1`, but the meaning behind that check is non-obvious.
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r172003256
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -60,9 +60,13 @@ angular.module('navigation').factory('userPageService', ['$injector',
* @returns {PageDefinition}
* The user's home page.
*/
- var generateHomePage = function generateHomePage(rootGroups) {
+ var generateHomePage = function generateHomePage(rootGroups,permissions) {
var homePage = null;
+ var settingsPages = generateSettingsPages(permissions);
+
+ if (settingsPages.length > 1)
--- End diff --
Commented.
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r172003270
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -60,9 +60,13 @@ angular.module('navigation').factory('userPageService', ['$injector',
* @returns {PageDefinition}
* The user's home page.
*/
- var generateHomePage = function generateHomePage(rootGroups) {
+ var generateHomePage = function generateHomePage(rootGroups,permissions) {
--- End diff --
Styled and documented.
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-client/pull/255
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r171996946
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -60,9 +60,13 @@ angular.module('navigation').factory('userPageService', ['$injector',
* @returns {PageDefinition}
* The user's home page.
*/
- var generateHomePage = function generateHomePage(rootGroups) {
+ var generateHomePage = function generateHomePage(rootGroups,permissions) {
--- End diff --
The newly-added `permissions` parameter needs to be documented.
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r171996915
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -60,9 +60,13 @@ angular.module('navigation').factory('userPageService', ['$injector',
* @returns {PageDefinition}
* The user's home page.
*/
- var generateHomePage = function generateHomePage(rootGroups) {
+ var generateHomePage = function generateHomePage(rootGroups,permissions) {
--- End diff --
Please include a space following the comma before the `permissions` parameter.
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r172016032
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -57,12 +57,24 @@ angular.module('navigation').factory('userPageService', ['$injector',
* A map of all root connection groups visible to the current user,
* where each key is the identifier of the corresponding data source.
*
+ * @param {Object.<String, PermissionSet>} permissions
+ * A map of all permissions granted to the current user, where each
+ * key is the identifier of the corresponding data source.
+ *
+ * @param {Object.<String,
+ *
* @returns {PageDefinition}
* The user's home page.
*/
- var generateHomePage = function generateHomePage(rootGroups) {
+ var generateHomePage = function generateHomePage(rootGroups, permissions) {
var homePage = null;
+ var settingsPages = generateSettingsPages(permissions);
+
+ // If we have more than one setting page, return the main home page
--- End diff --
Okay, added some more verbiage there.
---
[GitHub] guacamole-client pull request #255: GUACAMOLE-508: Take user to home page wh...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/255#discussion_r172003967
--- Diff: guacamole/src/main/webapp/app/navigation/services/userPageService.js ---
@@ -140,13 +144,20 @@ angular.module('navigation').factory('userPageService', ['$injector',
var deferred = $q.defer();
// Resolve promise using home page derived from root connection groups
- dataSourceService.apply(
+ var getRootGroups = dataSourceService.apply(
connectionGroupService.getConnectionGroupTree,
authenticationService.getAvailableDataSources(),
ConnectionGroup.ROOT_IDENTIFIER
- )
- .then(function rootConnectionGroupsRetrieved(rootGroups) {
- deferred.resolve(generateHomePage(rootGroups));
+ );
+ var getPermissionSets = dataSourceService.apply(
+ permissionService.getPermissions,
+ authenticationService.getAvailableDataSources(),
+ authenticationService.getCurrentUsername()
+ );
+
+ $q.all([getRootGroups,getPermissionSets])
+ .then(function rootConnectionGroupsPermissionsRetrieved(data) {
+ deferred.resolve(generateHomePage(data[0],data[1]));
--- End diff --
Objectified.
---