-
Notifications
You must be signed in to change notification settings - Fork 126
Improve permissions UI for users. #2198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
1734725
d73ff6b
04ccaac
0fbf21f
b8936f2
0ef7d87
e592b4e
c71665a
8e263ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| /* | ||
| * Copyright 2020, GeoSolutions Sas. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the BSD-style license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| import React from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import { connect } from 'react-redux'; | ||
| import { createSelector } from 'reselect'; | ||
| import { Glyphicon } from 'react-bootstrap'; | ||
|
|
||
| import { createPlugin } from '@mapstore/framework/utils/PluginsUtils'; | ||
| import { setControlProperty } from '@mapstore/framework/actions/controls'; | ||
| import Message from '@mapstore/framework/components/I18N/Message'; | ||
| import controls from '@mapstore/framework/reducers/controls'; | ||
| import Button from '@mapstore/framework/components/layout/Button'; | ||
| import { mapInfoSelector } from '@mapstore/framework/selectors/map'; | ||
|
|
||
| import OverlayContainer from '@js/components/OverlayContainer'; | ||
| import { | ||
| isNewResource, | ||
| getResourceId, | ||
| getResourceData, | ||
| getViewedResourceType | ||
| } from '@js/selectors/resource'; | ||
| import { | ||
| canAccessPermissions, | ||
| canManageAnonymousPermissions, | ||
| canManageRegisteredMemberPermissions, | ||
| getDownloadUrlInfo, | ||
| getResourceTypesInfo | ||
| } from '@js/utils/ResourceUtils'; | ||
| import SharePageLink from '@js/plugins/Share/SharePageLink'; | ||
| import ShareEmbedLink from '@js/plugins/Share/ShareEmbedLink'; | ||
| import Permissions from '@js/plugins/Share/components/Permissions'; | ||
| import FlexBox from '@mapstore/framework/components/layout/FlexBox'; | ||
|
|
||
| const getEmbedUrl = (resource) => { | ||
| const { formatEmbedUrl = (_resource) => _resource?.embed_url } = getResourceTypesInfo()[resource?.resource_type] || {}; | ||
| return formatEmbedUrl(resource) ? resource?.embed_url : null; | ||
| }; | ||
| function Share({ | ||
| enabled, | ||
| onClose, | ||
| resource, | ||
| resourceType | ||
| }) { | ||
| const embedUrl = getEmbedUrl(resource); | ||
| const downloadUrl = getDownloadUrlInfo(resource)?.url; | ||
| const manageAnonymousPermissions = canManageAnonymousPermissions(resource); | ||
| const manageRegisteredMemberPermissions = canManageRegisteredMemberPermissions(resource); | ||
| return ( | ||
| <OverlayContainer | ||
| enabled={enabled} | ||
| className="gn-overlay-wrapper" | ||
| > | ||
| <section className="gn-share-panel"> | ||
| <div className="gn-share-panel-head"> | ||
| <h2><Message msgId="gnviewer.shareThisResource" /></h2> | ||
| <Button className="square-button gn-share-panel-close" onClick={() => onClose()}> | ||
| <Glyphicon glyph="1-close" /> | ||
| </Button> | ||
| </div> | ||
| <FlexBox column gap="md" className="gn-share-panel-body"> | ||
| {canAccessPermissions(resource) && <Permissions resource={resource} manageAnonymousPermissions={manageAnonymousPermissions} manageRegisteredMemberPermissions={manageRegisteredMemberPermissions} />} | ||
| {(resourceType === 'document' && !!downloadUrl) && <SharePageLink value={downloadUrl} label={<Message msgId={`gnviewer.directLink`} />} collapsible={false} />} | ||
| {embedUrl && <ShareEmbedLink embedUrl={embedUrl} label={<Message msgId={`gnviewer.embed${resourceType}`} />} />} | ||
| </FlexBox> | ||
| </section> | ||
| </OverlayContainer> | ||
| ); | ||
| } | ||
|
|
||
| Share.propTypes = { | ||
| resourceId: PropTypes.oneOfType([ PropTypes.number, PropTypes.string ]), | ||
| enabled: PropTypes.bool, | ||
| onClose: PropTypes.func | ||
| }; | ||
|
|
||
| Share.defaultProps = { | ||
| resourceId: null, | ||
| enabled: false, | ||
| onClose: () => {} | ||
| }; | ||
|
|
||
| const SharePlugin = connect( | ||
| createSelector([ | ||
| state => state?.controls?.rightOverlay?.enabled === 'Share', | ||
| getResourceData, | ||
| getViewedResourceType | ||
| ], (enabled, resource, type) => ({ | ||
| enabled, | ||
| resource, | ||
| resourceType: type | ||
| })), | ||
| { | ||
| onClose: setControlProperty.bind(null, 'rightOverlay', 'enabled', false) | ||
| } | ||
| )(Share); | ||
|
|
||
| function ShareButton({ | ||
| enabled, | ||
| variant, | ||
| onClick, | ||
| size | ||
| }) { | ||
| return enabled | ||
| ? <Button | ||
| variant={variant || "primary"} | ||
| size={size} | ||
| onClick={() => onClick()} | ||
| > | ||
| <Message msgId="share.title"/> | ||
| </Button> | ||
| : null | ||
| ; | ||
| } | ||
|
|
||
| const ConnectedShareButton = connect( | ||
| createSelector( | ||
| isNewResource, | ||
| getResourceId, | ||
| mapInfoSelector, | ||
| (isNew, resourceId, mapInfo) => ({ | ||
| enabled: !isNew && (resourceId || mapInfo?.id) | ||
| }) | ||
| ), | ||
| { | ||
| onClick: setControlProperty.bind(null, 'rightOverlay', 'enabled', 'Share') | ||
| } | ||
| )((ShareButton)); | ||
|
|
||
| export default createPlugin('Share', { | ||
| component: SharePlugin, | ||
| containers: { | ||
| ActionNavbar: { | ||
| name: 'Share', | ||
| Component: ConnectedShareButton | ||
| } | ||
| }, | ||
| epics: {}, | ||
| reducers: { | ||
| controls | ||
| } | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -476,13 +476,51 @@ export const setAvailableResourceTypes = (value) => { | |||||||||||||||||||
| availableResourceTypes = value; | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| export const canManageAnonymousPermissions = (resource) => { | ||||||||||||||||||||
| return resourceHasPermission(resource, 'can_manage_anonymous_permissions'); | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| export const canManageRegisteredMemberPermissions = (resource) => { | ||||||||||||||||||||
| return resourceHasPermission(resource, 'can_manage_registered_member_permissions'); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Get the current permission for a specific group. | ||||||||||||||||||||
| * @param {*} groupName | ||||||||||||||||||||
| * @param {*} groups | ||||||||||||||||||||
|
||||||||||||||||||||
| * @param {*} groupName | |
| * @param {*} groups | |
| * @param {string} groupName | |
| * @param {object[]} groups |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's avoid to mutate the argument object and return a new object instead, see the example below (please double check the code):
We could also try to check all groups in same function
const filterGroupPermissions = (options, groups, groupNames) => {
return groupNames.length
? Object.fromEntries(Object.keys(options)
.map((key) => {
if (groupNames.some(name => name === key)) {
const permissionValue = getPermissionForGroup(key, groups);
const currentPermission = options[key].find(p => p.name === permissionValue);
return currentPermission ? [key, currentPermission] : [key, options[key]]
}
return [key, options[key]]
}))
: options;
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getResourcePermissions function has default values for manageAnonymousPermissions and manageRegisteredMemberPermissions, but not for groups. To improve robustness and make the function signature clearer, consider adding a default value for groups, such as an empty array.
| export const getResourcePermissions = (options , groups, manageAnonymousPermissions=false, manageRegisteredMemberPermissions=false) => { | |
| export const getResourcePermissions = (options , groups = [], manageAnonymousPermissions=false, manageRegisteredMemberPermissions=false) => { |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on previous comment, please double check this
const options = filterGroupPermissions(_options, groups, [
...(manageAnonymousPermissions ? [] : ['anonymous']),
...(manageRegisteredMemberPermissions? [] : ['registered-members']),
])There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filterGroupPermissions function mutates the options object. It's a good practice to avoid mutating function arguments to prevent potential side effects. Consider creating a copy of the options object before filtering to ensure the original object remains unchanged.
| filterGroupPermissions(options, groups, 'anonymous', manageAnonymousPermissions); | |
| filterGroupPermissions(options, groups, 'registered-members', manageRegisteredMemberPermissions); | |
| let permissionsOptions = {}; | |
| Object.keys(options).forEach((key) => { | |
| const optionsCopy = JSON.parse(JSON.stringify(options)); | |
| filterGroupPermissions(optionsCopy, groups, 'anonymous', manageAnonymousPermissions); | |
| filterGroupPermissions(optionsCopy, groups, 'registered-members', manageRegisteredMemberPermissions); | |
| let permissionsOptions = {}; | |
| Object.keys(optionsCopy).forEach((key) => { |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,7 +113,8 @@ describe('Test Resource Utils', () => { | |
| } | ||
| } | ||
| }]; | ||
| const permissionOptions = getResourcePermissions(data[0].allowed_perms.compact); | ||
| const groups = []; | ||
| const permissionOptions = getResourcePermissions(data[0].allowed_perms.compact, groups); | ||
|
Comment on lines
+116
to
+117
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test for Please add test cases that cover:
|
||
| expect(permissionOptions).toEqual({ | ||
| test1: [ | ||
| { value: 'none', labelId: `gnviewer.nonePermission`, label: 'None' }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note previous PR moved code about share to another component #2204
We should not include the plugin again if it was removed previously. Now share is a tab in resources details component see DetailsShare.jsx. Please review the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done