#5139 standard user session plugin #5154

mbarto · 2020-04-17T14:33:48Z

Description

This work extends user sessions base support, to allow enabling them in the standard product with reasonable defaults, like storing the session on the browser localStorage.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/geosolutions-it/MapStore2/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x", remove the others)

Issue

#5139

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

Yes, and I documented them in migration notes
No

Other useful information

coveralls · 2020-04-18T13:41:31Z

Coverage decreased (-0.08%) to 83.772% when pulling ad8e9f9 on mbarto:user_sessions_plugin into 22dfd39 on geosolutions-it:master.

offtherailz

Looks great 👍
I only asked for some documentation (I know you provided a lot, only a few functions that are not too much clear in their role).

UserSession Plugin should stay in the list of plugins selectable for context.
moreover, should we enable user session in mapstore by default? What do you think? I think this is valid for georchestra (and it should be set enabled by default also in pluginsConfig.json for that project), but for MapStore, maybe, it should be disabled by default. @tdipisa, what do you think?

web/client/epics/config.js

offtherailz · 2020-04-21T10:50:33Z

web/client/epics/context.js

@@ -77,20 +84,50 @@ const errorToMessageId = (name, e, getState = () => {}) => {
    }
    return message;
 };
+
+const flowWithSession = (mapId, contextName, action$, getState) => {


I suggest to use createSessionFlow. It looks more clear then flowWithSession and similar to createContextFlow and createMapFlow.
Can you document arguments and returned stream, as for createMapFlow

offtherailz · 2020-04-21T10:50:48Z

web/client/epics/context.js

@@ -32,12 +34,12 @@ function ContextError(error) {
    this.originalError = error;
    this.name = "context";
 }
-const createContextFlow = (id, action$, getState) =>
+const createContextFlow = (id, session = {}, action$, getState) =>


Can you document this too, a little

web/client/localConfig.json

tdipisa · 2020-04-21T12:13:36Z

@offtherailz

moreover, should we enable user session in mapstore by default? What do you think? I think this is valid for georchestra (and it should be set enabled by default also in pluginsConfig.json for that project), but for MapStore, maybe, it should be disabled by default. @tdipisa, what do you think?

Generally speaking if we have it, we can test it. If we have it, we can use it and better maintain it over time: if this functionlity stays disabled we are never able to identify regressions at this stage.
The important thing is that in MS we use a client side session storage (and not DB).

mbarto · 2020-04-22T06:47:03Z

Generally speaking if we have it, we can test it. If we have it, we can use it and better maintain it over time: if this functionlity stays disabled we are never able to identify regressions at this stage.
The important thing is that in MS we use a client side session storage (and not DB).

I try to translate it in developer terms:
Generally speaking if we have it, we HAVE to test it. If we have it, we HAVE to use it and we have to MANTAIN it over time: if this functionlity stays disabled we are never able to identify regressions at this stage.

That said, no objections to have it enabled by default. Did you consider it also from the final user point of view? Does a user expect to see the last point of view when it reloads the map, instead of resetting to the standard view?

…nfiguration for the UserSession plugin

mbarto · 2020-04-22T07:28:09Z

@offtherailz added the requested docs and pluginsConfig.json configuration for the UserSession plugin

tdipisa · 2020-04-22T08:24:01Z

@mbarto

That said, no objections to have it enabled by default. Did you consider it also from the final user point of view? Does a user expect to see the last point of view when it reloads the map, instead of resetting to the standard view?

I need this enabled by default in MS with a client side session storage to properly test it there too. It should be a matter of configuration, isn't it?

mbarto · 2020-04-22T11:53:43Z

@tdipisa I checked, and configuration enables user sessions on localStorage by default

offtherailz · 2020-04-22T12:58:57Z

I talked with @tdipisa :
Is it possible to test this only in a context (and related maps) without setting it up globally, so we can avoid to configure session on master ? What do you think about this proposal @mbarto ?

mbarto · 2020-04-22T13:09:58Z

@offtherailz no, this configuration is not allowed at the moment: you mean to enable it only on one specific context or to enable it only for all the contexts?

web/client/localConfig.json

offtherailz · 2020-04-24T07:54:20Z

At the end we decided with @tdipisa and @mbarto to disable in MapStore, at least until the session cannot be disabled/enabled by the user.
It will be enabled in GeOrchestra

User sessions plugin improvements

geosolutions-it#5139 standard user session plugin

ee59af6

mbarto added enhancement geOrchestra labels Apr 17, 2020

mbarto added this to the 2020.02.00 milestone Apr 17, 2020

mbarto requested a review from offtherailz April 17, 2020 14:33

tdipisa assigned mbarto Apr 17, 2020

mbarto added 2 commits April 18, 2020 13:05

fix for eslint errors

4d34637

fixed translations

00d07a9

offtherailz requested changes Apr 21, 2020

View reviewed changes

geosolutions-it#5139: added some documentation and context plugins co…

691e073

…nfiguration for the UserSession plugin

mbarto commented Apr 24, 2020

View reviewed changes

web/client/localConfig.json Show resolved Hide resolved

Update web/client/localConfig.json

9f752c3

offtherailz approved these changes Apr 24, 2020

View reviewed changes

offtherailz and others added 4 commits April 24, 2020 17:58

WIP fixes

b0c3b92

Add fixes and test to allow auto-save disable

77f450c

Update context.js

0e1210b

Merge pull request #7 from offtherailz/mbarto-user_sessions_plugin

ad8e9f9

User sessions plugin improvements

offtherailz merged commit 423044f into geosolutions-it:master Apr 29, 2020

allyoucanmap mentioned this pull request Aug 9, 2024

#4675 Remove unused code #10442

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5139 standard user session plugin #5154

#5139 standard user session plugin #5154

mbarto commented Apr 17, 2020 •

edited by tdipisa

Loading

coveralls commented Apr 18, 2020 •

edited

Loading

offtherailz left a comment •

edited

Loading

offtherailz Apr 21, 2020

offtherailz Apr 21, 2020

tdipisa commented Apr 21, 2020 •

edited

Loading

mbarto commented Apr 22, 2020

mbarto commented Apr 22, 2020

tdipisa commented Apr 22, 2020

mbarto commented Apr 22, 2020

offtherailz commented Apr 22, 2020

mbarto commented Apr 22, 2020

offtherailz commented Apr 24, 2020

#5139 standard user session plugin #5154

#5139 standard user session plugin #5154

Conversation

mbarto commented Apr 17, 2020 • edited by tdipisa Loading

Description

Issue

Breaking change

Other useful information

coveralls commented Apr 18, 2020 • edited Loading

offtherailz left a comment • edited Loading

Choose a reason for hiding this comment

offtherailz Apr 21, 2020

Choose a reason for hiding this comment

offtherailz Apr 21, 2020

Choose a reason for hiding this comment

tdipisa commented Apr 21, 2020 • edited Loading

mbarto commented Apr 22, 2020

mbarto commented Apr 22, 2020

tdipisa commented Apr 22, 2020

mbarto commented Apr 22, 2020

offtherailz commented Apr 22, 2020

mbarto commented Apr 22, 2020

offtherailz commented Apr 24, 2020

mbarto commented Apr 17, 2020 •

edited by tdipisa

Loading

coveralls commented Apr 18, 2020 •

edited

Loading

offtherailz left a comment •

edited

Loading

tdipisa commented Apr 21, 2020 •

edited

Loading