Add support for browser.retain_editor_history setting #1419
Conversation
also change local defaults for community edition
| 'browser.remote_content_hostname_allowlist': 'guides.neo4j.com, localhost', | ||
| 'browser.retain_connection_credentials': false | ||
| 'browser.retain_connection_credentials': false, | ||
| 'browser.retain_editor_history': false |
There was a problem hiding this comment.
Would it not make sense to default this to true?
There was a problem hiding this comment.
As that's the current behaviour?
There was a problem hiding this comment.
Worth noting that the !isEnterprise stuff defaults to true since edition default/before connect is null
There was a problem hiding this comment.
Perhaps it's better to explicitly look for "community" rather than not enterprise?
There was a problem hiding this comment.
The server does default this setting to true, but for safety it was decided to default to false on the client side.
The settings are only ever loaded after initial discovery is done, so by that point we know the version/edition.
Also, only enterprise supports the setting, so that's why I decided to code it as "not enterprise"
There was a problem hiding this comment.
Canny and Sentry both check allow shouldAllowOutgoing connections before connecting
misc code cleanup
| * Helpers | ||
| */ | ||
|
|
||
| export const VERSION_FOR_EDITOR_HISTORY_SETTING = '4.3.0' |
There was a problem hiding this comment.
does this need to be exported
| isEnterprise(state) && versionHasEditorHistorySetting(getVersion(state)) | ||
|
|
||
| export const shouldAllowOutgoingConnections = (state: any) => | ||
| !isEnterprise(state) || getAllowOutgoingConnections(state) |
There was a problem hiding this comment.
I think this would send outgoing connections before we connect to a database? I think that's a no-go in airgapped deployments
| export const shouldRetainConnectionCredentials = (state: any) => | ||
| !isEnterprise(state) || getRetainConnectionCredentials(state) | ||
|
|
||
| export const shouldRetainEditorHistory = (state: any) => |
There was a problem hiding this comment.
it's a nit but could we move this to by be the other history getters? (so to line 146). Feel free to disregard this comment, just find it slightly more readable
OskarDamkjaer
left a comment
OskarDamkjaer
left a comment
There was a problem hiding this comment.
I the current impl would do outgoing connections before connecting
also misc code cleanup
OskarDamkjaer
left a comment
OskarDamkjaer
left a comment
There was a problem hiding this comment.
There might be more places where we want to check hasEdition before checking isEnterprise in the future, this looks good for the release
jharris4
changed the title
2 participants
also change local defaults for community edition