Make behaviour of repeated import configurable #5613

matthias-ronge · 2023-03-30T12:37:22Z

Adds a new control in the <settings> in the ruleset, which determines how metadata behaves in the "additional import" function. There are three settings, reimport="replace" will overwrite the existing metadata with the new ones (this is already the default case), reimport="add" will add the new metadata to the older ones, and reimport="keep" will keep the old existing metadata without replacing them.

Resolves #5410

henning-gerhardt

It is only a code review. I did not check any added nor changed functionality.

Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/Settings.java

Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/RulesetManagement.java

Cf. kitodo#4087

If someone get this message as he use a non valid entry it would be very difficult to find the right place and the causing issue. Maybe something like that "Used not supported reimport case {}" where{} is replaced by the wrong value can make it easier for him.

markusweigelt · 2023-04-13T15:19:16Z

Kitodo/rulesets/ruleset.xsd

@@ -347,6 +363,7 @@
        <xs:attribute type="xs:boolean" name="editable" default="true"/>
        <xs:attribute type="xs:boolean" name="excluded" default="false"/>
        <xs:attribute type="xs:boolean" name="multiline" default="false"/>
+        <xs:attribute type="ruleset:Reimport" name="reimport" default="replace"/>


Shouldn't we take keep as default value here. Cause overwriting the metadata always leads to the loss of customized content.

So far it has been common that we always use the default which reflects the previous functionality. This is, so that after an update, the administrators have to make as few adjustments as possible to keep the system working as before. And the previous behavior was, that the metadata was replaced. And, I'd vote for add as the default behavior, by the way.

The default value can not be somehow configurable and not hard-coded?
Some suggestions :

via kitodo-config-properties : new config line reimport.default=replace

or a better way via the GUI Mask : a new selectOneMenu input field with all the Reimport Enum entries in Edit ruleset and create new ruleset pages.

one question would be, wether this setting should be configurable

ruleset-wide?

project-wide?

client-wide?

system-wide?

I would probably also prefer to define it in the ruleset (or project) because it is a user decision and not an administrator decision in my opinion.

Your suggestions are worth considering, but that is beyond the scope of this issue, and also deviates from what the bar camp group agreed on at the practice meeting in last November. If you want this function, I would like to ask you to open a separate issue for it.

markusweigelt · 2023-04-13T15:21:54Z

Kitodo-DataEditor/src/test/resources/testReimportOfMetadataModes.xml

+        <setting key="metadataToAdd" reimport="add" />
+        <setting key="metadataToReplaceExplicitly" reimport="replace" />
+        <setting key="metadataToKeep" reimport="keep" />
+


Setting with default without reimport attribute is missing.

This is intentional. <key id="metadataToReplaceByDefault"> has no <setting>, so the default is used.

markusweigelt · 2023-04-13T15:43:19Z

Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/RulesetManagement.java

+                    currentMetadata.addAll(currentEntries);
+                    break;
+                case REPLACE:
+                    currentMetadata.addAll(updateEntries.isEmpty() ? currentEntries : updateEntries);


This looks like a conditional replacement. If updateEntry value is empty it will not replaced. I think this should be at least in the description of the ruleset.xsd.

Maybe it makes sense to make two replace variants out of this. e.g. like it is the case with git-reset https://git-scm.com/docs/git-reset. So REPLACE_HARD will be to replace independent of updateEntries value and REPLACE_SOFT checks the updateEntries value before.

Yes. currentEntries is the metadata (for that one key) that is in the user interface. It may be manually entered, or it may have come from the previous import, if that import returned different fields, like first importing a manuscript record from the library catalog, and then importing the record for that manuscript from a secondary database, that holds different information. Without the check, any other metadata, that doesn’t come with the second import, would be deleted during the repeated import, which should not be what the user expects.

markusweigelt

Great work! Thx that you have implemented a reimport option that works even better and is simply more thought out.

Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/RulesetManagement.java

IkramMaalej

When a metadata is configured as reimport="add" in the settings section, and this metadata is also configured with maxOccurs="1" for example in the restrictions section on a structural element, after an additional import, this metadata will be displayed twice and not once, which should be the correct behavior.

Therefore, the maximum number of occurrences is unchecked for the case of reimport="add"

matthias-ronge · 2023-05-30T13:27:47Z

I've modified the code to take this into account. It was a major change and I've now extracted the function into its own class. I've added cases to the test to test this new behaviour as well.

matthias-ronge · 2023-05-31T09:29:17Z

@solth I tested locally, MetadataST finished without errors. I cannot say why it failed here.

solth · 2023-06-01T13:04:22Z

@solth I tested locally, MetadataST finished without errors. I cannot say why it failed here.

6th build was successful 😅

solth · 2023-07-10T19:36:21Z

@BartChris do you have any further change requests or would you approve this pull request?

BartChris · 2023-07-11T16:12:59Z

@solth I just had a note on the configurability of the reimport of metadata, but this is beyond the scope of this pull request. So i am fine with the pull request as it is right now.

solth

@matthias-ronge thanks a lot for this valuable contribution. I have no objections, just some tiny suggestions to optimize code style in a few places.

Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/ReimportMetadata.java

Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/RulesetManagement.java

Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/xml/Reimport.java

Kitodo/src/main/java/org/kitodo/production/forms/createprocess/ProcessFieldedMetadata.java

…ReimportMetadata.java Co-authored-by: Arved Solth <solth@effective-webwork.de>

…xml/Reimport.java Co-authored-by: Arved Solth <solth@effective-webwork.de>

…/ProcessFieldedMetadata.java Co-authored-by: Arved Solth <solth@effective-webwork.de>

andre-hohmann · 2023-08-24T05:28:09Z

@matthias-ronge : Thanks a lot for the additional functionality. We talked about that in the Community Board and want to ask, if you could describe the settings reimport="replace", reimport="add" and reimport="keep" in Regelsatz 3.x, as for example Schlüssel mit besonderer Verwendung.

We asked ourselves, if a delete-function is realized by reimport="replace":

If metadata in the source is deleted and an additional import executed, the metadata in Kitodo.Production is deleted (or replaced by "null").

Is this assumption correct?

matthias-ronge added 11 commits March 30, 2023 10:20

Implement interface and tests

81af102

Get reimport settings from ruleset

76725ee

Implement metadata update

3dd9ac4

Implement use

a04c25d

Clean-up

d4b2085

Organize imports

88c50d1

Add javadoc

96eb697

Fix checkstyle (kitodo-data-editor)

7214ff3

Fix checkstyle

e31c7c2

Explain use on ruleset subhh.xml

418dc62

Fix Codacy complains

0d7371b

solth requested review from henning-gerhardt and markusweigelt and removed request for henning-gerhardt March 31, 2023 13:42

henning-gerhardt suggested changes Apr 3, 2023

View reviewed changes

matthias-ronge added 3 commits April 11, 2023 14:29

Use wrong use of Objects.nonNull() to check for null values

92e3938

Cf. kitodo#4087

Replace 'var' typing with explicit type naming

b83c520

matthias-ronge force-pushed the issue-5410 branch from f20b293 to b83c520 Compare April 11, 2023 15:11

markusweigelt suggested changes Apr 13, 2023

View reviewed changes

Add an explaination that 'replace' does not delete not-imported metadata

eb12937

matthias-ronge requested review from solth, markusweigelt and henning-gerhardt April 27, 2023 09:35

IkramMaalej self-requested a review May 22, 2023 13:03

markusweigelt approved these changes May 23, 2023

View reviewed changes

IkramMaalej suggested changes May 24, 2023

View reviewed changes

Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/RulesetManagement.java Outdated Show resolved Hide resolved

Split long method

6af03c4

matthias-ronge requested a review from IkramMaalej May 24, 2023 13:26

IkramMaalej reviewed May 25, 2023

View reviewed changes

Implements the reimport behaviour respecting the maxOccurs setting

1979dfe

matthias-ronge requested a review from IkramMaalej May 30, 2023 13:27

matthias-ronge added 2 commits May 31, 2023 09:19

Fix import

6183027

Fix test dummy class

9b5fb2e

IkramMaalej approved these changes Jun 5, 2023

View reviewed changes

solth added the feature label Jul 8, 2023

solth reviewed Jul 12, 2023

View reviewed changes

matthias-ronge and others added 5 commits July 13, 2023 09:34

Better explain the use of English

b379c3a

Update Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/…

370c019

…ReimportMetadata.java Co-authored-by: Arved Solth <solth@effective-webwork.de>

Update Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/…

2177480

…ReimportMetadata.java Co-authored-by: Arved Solth <solth@effective-webwork.de>

Update Kitodo-DataEditor/src/main/java/org/kitodo/dataeditor/ruleset/…

599eaeb

…xml/Reimport.java Co-authored-by: Arved Solth <solth@effective-webwork.de>

Update Kitodo/src/main/java/org/kitodo/production/forms/createprocess…

32b1469

…/ProcessFieldedMetadata.java Co-authored-by: Arved Solth <solth@effective-webwork.de>

solth approved these changes Jul 13, 2023

View reviewed changes

solth merged commit 4fa4310 into kitodo:master Jul 13, 2023
2 checks passed

matthias-ronge mentioned this pull request Nov 16, 2023

Restore the functionality to create processes using ActiveMQ #4837

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make behaviour of repeated import configurable #5613

Make behaviour of repeated import configurable #5613

matthias-ronge commented Mar 30, 2023 •

edited

Loading

henning-gerhardt left a comment

markusweigelt Apr 13, 2023

matthias-ronge Apr 19, 2023 •

edited

Loading

IkramMaalej May 26, 2023 •

edited

Loading

BartChris May 26, 2023 •

edited

Loading

matthias-ronge May 30, 2023

markusweigelt Apr 13, 2023

matthias-ronge Apr 19, 2023

markusweigelt Apr 13, 2023

matthias-ronge Apr 19, 2023

markusweigelt left a comment

IkramMaalej left a comment

matthias-ronge commented May 30, 2023

matthias-ronge commented May 31, 2023

solth commented Jun 1, 2023

solth commented Jul 10, 2023

BartChris commented Jul 11, 2023

solth left a comment •

edited

Loading

andre-hohmann commented Aug 24, 2023

Make behaviour of repeated import configurable #5613

Make behaviour of repeated import configurable #5613

Conversation

matthias-ronge commented Mar 30, 2023 • edited Loading

henning-gerhardt left a comment

Choose a reason for hiding this comment

markusweigelt Apr 13, 2023

Choose a reason for hiding this comment

matthias-ronge Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

IkramMaalej May 26, 2023 • edited Loading

Choose a reason for hiding this comment

BartChris May 26, 2023 • edited Loading

Choose a reason for hiding this comment

matthias-ronge May 30, 2023

Choose a reason for hiding this comment

markusweigelt Apr 13, 2023

Choose a reason for hiding this comment

matthias-ronge Apr 19, 2023

Choose a reason for hiding this comment

markusweigelt Apr 13, 2023

Choose a reason for hiding this comment

matthias-ronge Apr 19, 2023

Choose a reason for hiding this comment

markusweigelt left a comment

Choose a reason for hiding this comment

IkramMaalej left a comment

Choose a reason for hiding this comment

matthias-ronge commented May 30, 2023

matthias-ronge commented May 31, 2023

solth commented Jun 1, 2023

solth commented Jul 10, 2023

BartChris commented Jul 11, 2023

solth left a comment • edited Loading

Choose a reason for hiding this comment

andre-hohmann commented Aug 24, 2023

matthias-ronge commented Mar 30, 2023 •

edited

Loading

matthias-ronge Apr 19, 2023 •

edited

Loading

IkramMaalej May 26, 2023 •

edited

Loading

BartChris May 26, 2023 •

edited

Loading

solth left a comment •

edited

Loading