Skip to content
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

Refactor JsonDatabaseMigration to make easier to implement future JSON setting migrations #2808

Merged
merged 3 commits into from
Jun 5, 2018

Conversation

juanluisrp
Copy link
Contributor

@juanluisrp juanluisrp commented May 25, 2018

With this refactor the only things needed to implement a JsonDatabaseMigration is to define the setting name to update and the list of JSON paths to update/modify in the method setUpNewSettingValues.

This PR also moves AdvancedSearchFormMigration to version v3.4.3 since the PR #2547 was not included in v3.4.2 release.

Another fix in this PR is to use the right types for map values in database-migration.xml.

* Fix a bug about the map's value types from `String` to `List`
* Use interface `Map` instead of concrete class `LinkedHashMap` in
`DatabaseMigration`.
* Use try-with-resources statement where possible.
* Reindent and organize imports
* This refactor make possible to implement a JsonDatabaseMigration only defining the setting name
and the list of JSON fields to update/modify.
Copy link
Member

@josegar74 josegar74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice refactoring, thanks.

@juanluisrp juanluisrp changed the title Refactor JsonDatabaseMigration to make easier to implement future JSON migrations Refactor JsonDatabaseMigration to make easier to implement future JSON setting migrations May 25, 2018
@Delawen Delawen merged commit d06907c into geonetwork:3.4.x Jun 5, 2018
fxprunayre added a commit to fxprunayre/core-geonetwork that referenced this pull request Dec 18, 2018
The ui/config settings introduced
geonetwork#1848 (and inspired
from Sextant) is somehow a bit hard to maintain.

The default value is now set from CatController only.

The default db value is empty to use the one from CatController until a
db value is defined by the catalogue admin.

JsonDatabaseMigration task should be used to update the settings (see
geonetwork#2808). We should not
set the full value in SQL migration script anymore (BTW this is a line
in conflict in most of the PR since then).

JSON editor is now used instead of the textarea.

Future work required? Maybe we should find a better way to merge the db
value and update with the latest.
fxprunayre added a commit that referenced this pull request Dec 20, 2018
* UI configuration improvement.

The ui/config settings introduced
#1848 (and inspired
from Sextant) is somehow a bit hard to maintain.

The default value is now set from CatController only.

The default db value is empty to use the one from CatController until a
db value is defined by the catalogue admin.

JsonDatabaseMigration task should be used to update the settings (see
#2808). We should not
set the full value in SQL migration script anymore (BTW this is a line
in conflict in most of the PR since then).

JSON editor is now used instead of the textarea.
pvgenuchten pushed a commit to pvgenuchten/core-geonetwork that referenced this pull request Mar 17, 2021
* UI configuration improvement.

The ui/config settings introduced
geonetwork#1848 (and inspired
from Sextant) is somehow a bit hard to maintain.

The default value is now set from CatController only.

The default db value is empty to use the one from CatController until a
db value is defined by the catalogue admin.

JsonDatabaseMigration task should be used to update the settings (see
geonetwork#2808). We should not
set the full value in SQL migration script anymore (BTW this is a line
in conflict in most of the PR since then).

JSON editor is now used instead of the textarea.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants