Closed allyoucanmap closed 3 years ago
@allyoucanmap let me add the this is something that should be available for any plugin, not only the TOC plugins.
@allyoucanmap I would like to start the discussion with a couple of observations:
Some random ideas:
@allyoucanmap let me know if you are interested in testing this approach.
@mbarto @giohappy I agree with you I would like to find a more generic solution that works with the current plugin architecture. Probably we could not add new system to MapStore if we solve this issue with existing expressions but I would like to understand if we can improve a bit the current behaviours or not.
all the plugins configuration properties are (or should be) dinamically configurable (you can express properties values using code), so I would like to understand if we can leverage that for this purpose (this would avoid adding a new pluggability extension, if not needed); I am not sure if the dynamic properties can be resolved at "runtime" (every time you choose a different layer, in this use case), but that would be an interesting extension to the actual dynamic configurability, and would be applicable to every plugin (so @giohappy should be really happy; I will stop with these jokes one day, but not today)
I tried again with the property disablePluginIf
in the localConfig.json, here an example using expressions:
{
"monitorState": [
{
"name": "selectedNodes",
"path": "layers.selected"
},
{
"name": "layers",
"path": "layers.flat"
}
],
"plugins": [
{
"name": "LayerDownload",
"cfg": {
"disablePluginIf": "{state('selectedNodes') && state('layers') && state('selectedNodes').length > 0 && !state('layers').find(function(layer){ return layer.id === state('selectedNodes')[0] && (layer.canDownload || layer.canEdit); })}"
}
},
{
"name": "FeatureEditor",
"cfg": {
"disablePluginIf": "{state('selectedNodes') && state('layers') && state('selectedNodes').length > 0 && !state('layers').find(function(layer){ return layer.id === state('selectedNodes')[0] && (layer.canDownload || layer.canEdit); })}"
}
}
]
}
The above example seems to do what we need and it does not need new improvement of MapStore.
My concerns about this approach are:
dynamic properties could be resolved by a dedicated HOC that you can inject for one or more plugins, so that you can add your own stuff programmatically, but still leveraging the framework capabilities
Another proposal could be to give the possibility to register functions to use inside expressions
Something like:
{
"monitorState": [
{
"name": "selectedNodes",
"path": "layers.selected"
}
],
"plugins": [
{
"name": "LayerDownload",
"cfg": {
"disablePluginIf": "{func('checkLayerPermissions')()}"
}
},
{
"name": "FeatureEditor",
"cfg": {
"disablePluginIf": "{func('checkLayerPermissions')()}"
}
}
]
}
PluginUtils.registerExpressionFunction('checkLayerPermissions', (/* premissionTypes = ['edit', 'view', 'download'] */) => {
const state = StateUtils.getState(); // or find another way to make the whole state accessible
const selectedLayer = getSelectedLayer(state ); // layer selector
return !(selectedLayer.canEdit || selectedLayer.canDownload);
});
The above example seems to do what we need and it does not need new improvement of MapStore.
That's my favourite answer.
My concerns about this approach are:
- the expression seems to become too complex to be included as a string in the localConfig.json
The ability to register functions would be an awesome addition and should solve this issue, so +1 from me.
- I'm not sure if the use of layers state in the monitored state could cause rerender issues
This is also my concern. A possibility would be to reason about a more local version of the monitored state and expression resolver (at the plugin level, or as a wrapper of it), so that we can rerender a single plugin when the state changes, instead of the whole PluginsContainer.
My 2 cents about this. disablePluginIf
works great to disable the whole plugin, and solves the problem, and adding function registering could be a good improvements.
Anyway this has the defect to override the existing logic, requiring to write in localConfig.json
a lot of logic, that is sometimes unclear, too much complicated, and risks to be not so robust, when the condition becomes difficult.
In addition, disable the whole FeatureGrid plugin, may not be a sufficient option. For instance you may need to open the feature grid, but only in edit mode.
Adding this configurations is ok for some small customization, but looks to become hard to maintain, so we should look first if we can re-design a little to make the API more general.
The visibility of tools is actually divided in many parts that need needs to be simplified. Here the list of the parts, their purpose and their replacment.
checkPluginsEnhancer
check the plugin presence and should be replaced by injection (compare tool already do this).securityEnhancer
check the user role (ADMIN can do everything, for other users visibility depends on addLayersPermissions
, removeLayersPermissions
and other properties like this in the TOC. This should be replaced with 1 general rule for the plugin, with the new injection system. Here makes sense to use "disablePluginIf" to override default behaviors. undefined
means true
because these configuration are all optional.So refactoring this system the visibility will be determined by:
I think that this hierarchy is more solid that
`disablePluginIf`: {state('selectedNodes') && state('layers') && state('selectedNodes').length > 0 && !state('layers').find(function(layer){ return layer.id === state('selectedNodes')[0] && (layer.canDownload || layer.canEdit); })}```
@mbarto
This is also my concern. A possibility would be to reason about a more local version of the monitored state and expression resolver (at the plugin level, or as a wrapper of it), so that we can rerender a single plugin when the state changes, instead of the whole PluginsContainer.
Are you meaning a monitor state that target only some declared plugins? Something like:
"monitorState": [
{
"name": "selectedNodes",
"path": "layers.selected",
"plugins": [ "FeatureEditor" ]
}
],
@offtherailz
Anyway this has the defect to override the existing logic
Checked the code to understand the current use of disablePluginIf. It seems most of them is used to prevent their visibility in cesium or mobile and other for some specific configuration of the product app layout (eg FeatureEditor). Only the GeoStoryEditor disablePluginIf is in the localConfig the others are harcoded within the plugin export. I'm wondering if this is the right use of disablePluginIf for the core framework.
disablePluginIf: "{state('mapType') === 'cesium' || state('mapType') === 'leaflet' }"
disablePluginIf: "{state('mapType') === 'leaflet'}"
isablePluginIf: "{!!(state('browser') && state('browser').mobile)}"
disablePluginIf: "{state('featuregridmode') === 'EDIT'}"
disablePluginIf: "{state('featuregridmode') === 'EDIT'}"
disablePluginIf: "{state('browser') && state('browser').safari}"
"disablePluginIf": "{!!(state('browser') && state('browser').mobile)}"
(localConfig.json)disablePluginIf: "{state('featuregridmode') === 'EDIT'}"
disablePluginIf: "{state('mapType') === 'cesium'}"
disablePluginIf: "{state('mapType') === 'cesium'}"
disablePluginIf: "{state('mapType') === 'cesium'}"
disablePluginIf: "{state('mapType') === 'cesium'}"
disablePluginIf: "{state('featuregridmode') === 'EDIT' || (state('router') && state('router').includes('/geostory/shared') && state('geostorymode') !== 'edit')}"
disablePluginIf: "{state('featuregridmode') === 'EDIT'}"
disablePluginIf: "{state('mapType') === 'cesium' || !state('printEnabled')}"
disablePluginIf: "{state('mapType') === 'cesium'}"
disablePluginIf: "{state('mapType') === 'cesium'}"
disablePluginIf: "{state('router') && (state('router').endsWith('new') || state('router').includes('newgeostory') || state('router').endsWith('dashboard'))}"
disablePluginIf: "{state('mapType') === 'leaflet' || state('mapType') === 'cesium'}"
disablePluginIf: "{state('mapType') === 'cesium'}"
disablePluginIf: "{state('mapType') === 'cesium'}"
disablePluginIf: "{state('mapType') === 'cesium'}"
checkPluginsEnhancer check the plugin presence and should be replaced by injection (compare tool already do this).
I agree that all components related to a plugin should be injected but I'm not sure is it possible to solve within this issue.
securityEnhancer check the user role (ADMIN can do everything, for other users visibility depends on addLayersPermissions, removeLayersPermissions and other properties like this in the TOC. This should be replaced with 1 general rule for the plugin, with the new injection system. Here makes sense to use "disablePluginIf" to override default behaviors.
Are you referring to the FeatureEditor or to all plugins about addLayersPermissions and removeLayersPermissions?
We need one more fine grained rule that works layer by layer, and this option should be in the layer/node configuration. permissions: {canEdit: true, canDownload: false, ...}. In this case undefined means true because these configuration are all optional.
I don't think the core framework of MapStore should be aware of the permissons of a layer it seems something more related to the "-Store" in use so probably we should implement this in the custom application or product part and add to the core a way to extend this. For Example GeoStore has not the concept of layer resource while GeoNode has.
@allyoucanmap
This is also my concern. A possibility would be to reason about a more local version of the monitored state and expression resolver (at the plugin level, or as a wrapper of it), so that we can rerender a single plugin when the state changes, instead of the whole PluginsContainer. Are you meaning a monitor state that target only some declared plugins? Something like:
"monitorState": [
{
"name": "selectedNodes",
"path": "layers.selected",
"plugins": [ "FeatureEditor" ]
}
],
I didn't think on how this should be configured (the one you mention is fine, but we could think of other ways). The main point is that it should affect only a specific plugin instead of the PluginsContainer shouldComponentUpdate, that impacts all the subtree.
Checked the code to understand the current use of disablePluginIf. It seems most of them is used to prevent their visibility in cesium or mobile and other for some specific configuration of the product app layout (eg FeatureEditor). Only the GeoStoryEditor disablePluginIf is in the localConfig the others are harcoded within the plugin export. I'm wondering if this is the right use of disablePluginIf for the core framework.
- Annotations
disablePluginIf: "{state('mapType') === 'cesium' || state('mapType') === 'leaflet' }"
I agree that we should move most of this configuration from the core to the product configuration. Maybe we could see if there are some exception (e.g. the plugin cannot work at all in specific contexts)
I don't think the core framework of MapStore should be aware of the permissons of a layer it seems something more related to the "-Store" in use so probably we should implement this in the custom application or product part and add to the core a way to extend this. For Example GeoStore has not the concept of layer resource while GeoNode has.
+1 on this point of view on security
@tdipisa we need to converge on this one as it is blocking work on GeoNode.
We need a plan to implement this ASAP, as this one has been sitting idle for 21 days
Hi, sorry for the late response. I was thinking we was basically agree on how to proceed, my fault.
I'm agree with you all that if they solves the problem they are ok for me. disablePluginIf
is already sufficient for disabling whole plugins from localConfig.json
in certain cases. Also the general dynamic configuration system for localConfig.json
makes the work for most of times. When solving a problem using this system becomes complicated it means that we need a customization (or a core support, if interesting).
About performance concerns the monitored state should be already memorized. @allyoucanmap you may check if we have real re-renders that should not be there, and if so, we can optimize that part.
My comments was intended to be more general about the current usage of disablePluginIf
in the core. Most of the times could converge in core functionalities, leaving space for customizations outside. Doing this optimization may make easier to customize the behaviur from the outside, already having good defaults. Anyway this is not strictly a part of this issue, can only help to have a more maintainable solution.
At the current time we have these use case in the core for disablePluginIf
:
router
: for hiding tools when they should not appear (new map/dashboard/,
view only mode of geostory
). This dependency make sense only for the standard product.browser
(mobile, safari), mapType <-- Compatibility reasons or mobile customization. checks to browser or mapType limitations are core, while hiding in mobile is a separate concern that should be part of a featuregridmode
: to disable plugins in edit/view mode of the feature grid. This is intended to be core.geostorymode
: only in Omnibar {state('featuregridmode') === 'EDIT' || (state('router') && state('router').includes('/geostory/shared') && state('geostorymode') !== 'edit')}
disable the top bar (that is also the search bar) in certain cases. (maybe we should move these configurations (in localConfig) or separate the plugins of the map from the general top-bar of thegeostory page (or pages in general).
printEnabled
this is a very specific flag dedicated to print plugin itself, it should converge into the print plugin.Long story short:
disablePluginIf
is already sufficient for disabling plugins from localConfig.json
. It can be used to configure additional requirements (other than the mandatory ones, that should be provided by the framework) to show/hide plugins.disablePluginIf
instances used in the main product was often quick solutions to problems that need a more dedicated solution, by concern, sometimes in the core or as part of the product. Putting them in disablePluginIf
allow quick overrides, but makes maintenance more complicated. So they should be limited to the necessary ones (e.g. product only code in product configuration, mandatory code in the core logic). not part of this issue --> after project work, when the separation between core and standard product is more defined, we can do a clean-up.So, nothing to do, except some optimization if you find some unnecessary rerendering using this functionality.
@offtherailz ok I'll proceed with the current disablePluginIf
, thanks
We were able to use the current functionalities available in MapStore. This PR uses the disablePluginIf as discussed in this issue. https://github.com/GeoNode/geonode-mapstore-client/pull/164 . Going to close this issue.
Description
We should have a way to manage and disable plugin at runtime via localConfig.json
Use case
Most of the buttons inside the TOC toolbar represents functionality allowed to a specific layer. This buttons are rendered by this component TOC/Toolbar.jsx and use hardcoded conditions to disable/enable the visibility, eg:
The layers resources of GeoNode could have restrictions based on user permissions so in some cases it's needed to remove some tools from the layer. It would be an useful enhancement to make this availablity conditions of the plugin more extensible and aware of other properties of the layer.
@mbarto @offtherailz @MV88 I would like to discuss for a generic solution that could work with the MapStore Plugin Container architecture
Acceptance criteria
Other useful information