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

Update catalog metadata #6233

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

solth
Copy link
Member

@solth solth commented Sep 24, 2024

This pull request adds the feature to re-import metadata of individual processes from a configured catalog search interface using a new button in the metadata editors metadata panel:

Bildschirmfoto 2024-09-24 um 15 57 31

If any differences are detected when comparing the newly imported metadata to that in the existing Kitodo process, the user is presented with a dialog listing all detected changes:

Bildschirmfoto 2024-09-24 um 15 59 02

Using the arrow button between the old and new values, the user can decide to either keep the value currently in Kitodo, replace it with the updated value from the source or use both values. The selection can be changed by clicking on the arrow symbol between old and new values:

Bildschirmaufnahme.2024-09-24.um.16.01.49.mp4

If the process does not contain the functional metadata of type recordIdentifier - whose value is used to load the same data record from the source - or isn't assigned an import configuration of type OPAC_SEARCH, the button to re-import metadata is deactivated:

Bildschirmfoto 2024-09-24 um 16 05 26

Import configurations are now automatically assigned to newly created processes, but in ordner to adjust old processes for metadata re-import a new list action has been added that can be used to assign a specific import configuration to all processes selected in the process list:

Bildschirmfoto 2024-09-24 um 16 10 57

In the following dialog the user can select the import configuration to assign to all selected processes:

Bildschirmfoto 2024-09-24 um 16 11 25

Note: Two new permissions have been added for a) re-importing metadata and b) setting import configurations for selected processes. Since these permissions are not set to any role by default, system admins have to assign these permissions after updating their Kitodo installtion before being able to use the function added in this pull request.

Note 2: One aspect of the original description here that had to be implemented differently was the way metadata groups are displayed in the comparison popup dialog. Since it is not possible to determine whether one metadata group is a new group or an old group with some/most/all metadata entries changed, it is not possible to show just changed values. Instead, the implemention now shows all fields of all metadata groups when differences have been detected in one type of metadata group, so the user can make an educated decision whether he wants to keep the old values or replace them with the new values.

Note 3: during implementation of this function a bug was discovered that corrupts data of collapsed metadata groups in the metadata editor which is documented in #6231. A potential fix has been provided with #6232 but that has not been reviewed or merged into the code, yet, so reviewers testing the metadata re-import of this pull request should keep that in mind and not collapse any metadata groups while testing this new feature.

Fixes #6225

@solth
Copy link
Member Author

solth commented Sep 25, 2024

@BartChris would you mind doing a review for this pull request?

@BartChris
Copy link
Collaborator

@solth Yes, sure.

@solth
Copy link
Member Author

solth commented Sep 25, 2024

@solth Yes, sure.

Thank you! (Already requested review from you 😁)

@solth solth removed the request for review from thomaslow September 25, 2024 15:13
@solth solth marked this pull request as draft September 27, 2024 13:44
@solth
Copy link
Member Author

solth commented Sep 27, 2024

Converted to "Draft" to incorporate change requests made by @subhhwendt

@solth solth force-pushed the update-catalog-metadata branch 2 times, most recently from 0b56f87 to 28738eb Compare October 2, 2024 10:01
@solth solth marked this pull request as ready for review October 7, 2024 15:02
@thomaslow
Copy link
Collaborator

thomaslow commented Oct 14, 2024

I experimented with your changes a bit. I added the new permissions, imported a process from a catalog, changed a few metadata fields, and clicked the new "sync" button.

My first interpretation of the arrow icons was that they indicate the data flow. At first, I expected the left arrow to overwrite Kitodo metadata with the imported data, assuming the left arrow would “move” the data from right to left. I didn't really notice the text color. It took a minute to understand that the arrow is pointing towards the data that is kept. Currently, I don't have an idea how to improve this symbology. Maybe you could add a title to all the icons:

  • “left arrow” - Keep Kitodo.Production metadata, ignore imported metadata
  • “right arrow” - Overwrite Kitodo.Production metadata with imported metadata
  • “left-right-arrow” - Add imported metadata field to Kitodo.Production, but keep existing metadata

If I change the “Haupttitel” (metadata key="LABEL" with maxOccurs="1" in the ruleset.xml) and use the “left-right"-arrow, I can add a second "Haupttitel" even though this is not permitted by the ruleset. There is no validation error. I can save the process. After a reload, both "Haupttitel" are listed. Maybe this is a problem? Ideally, the "left-right"-arrow should be disabled in case the ruleset does not allow to add another occurrence of this metadata field.

@solth
Copy link
Member Author

solth commented Oct 15, 2024

@thomaslow thank you for the remarks. I added some titles to the buttons as you suggested to hopefully make it easier to understand what each of them stands for. I agree that the buttons are not as inherently intuitive as we initially assumed when creating this mockup. Nonetheless I believe once people start using the new "Metadata update" dialog frequently it shouldn't take long to get accustomed to it.

As for the metadata validation that is a very good and valid point. Incorporating minOccurs and maxOccurs rules is a good idea. At the same time, metadata validation is not handled very consistently in the editor right now. On the one side, buttons for adding, duplicating or deleting metadata are deactivated based on those minOccurs and maxOccurs rules from the ruleset. On the other hand, metadata is not validated automatically when opening or saving a process in the metadata editor, but instead we have a button to trigger validation manually.

Since these new buttons in the "Update" dialog are part of the same UI as the "delete" and "add" buttons mentioned above, though, I guess applying the same rules from the ruleset would be appropriate. I do think this will require some refactoring of the existing pull request, though, so I would like to ask @subhhwendt and @apiller to confirm this is indeed what is required and how they expect this functionality to work!

@subhhwendt
Copy link
Collaborator

For all my actions as user in the metadata editor I expect that the ruleset will be considered. So I hope that it is not to complicated to integrate a check of ruleset condition for the specific metadata in order to apply the function in a next step for a complete project .

@BartChris
Copy link
Collaborator

BartChris commented Oct 15, 2024

As for the metadata validation that is a very good and valid point. Incorporating minOccurs and maxOccurs rules is a good idea. At the same time, metadata validation is not handled very consistently in the editor right now. On the one side, buttons for adding, duplicating or deleting metadata are deactivated based on those minOccurs and maxOccurs rules from the ruleset. On the other hand, metadata is not validated automatically when opening or saving a process in the metadata editor, but instead we have a button to trigger validation manually.

Regarding this point: I think the current behaviour is probably fine. We should not allow things which are illegal (adding a second metadatum when maxOccurs=1), but if a value is required and is not present, we should still allow to save. Otherwise a lot of structuring work might be lost, because the editor cannot save. (He or she might not know what to fill in at a particular field, but should still be able to save the progress).

@BartChris
Copy link
Collaborator

BartChris commented Oct 16, 2024

Things to check:
A user without the permission to re-import metadata does not see the button. A user with the necessary permissions sees the button.
works

The button is only enabled if one of the metadata elements in the current division has its key marked with use=recordIdentifier. If there is no key with this functionality attached the button stays disabled.
The button is also disabled if the field markes as recordIdentifier is empty.
works

Further Remarks

  • What happens if keys are marked as non-editable?

It is currently possible to overwrite values for keys which are marked as non-editable in the ruleset. I think this is the correct behaviour as this allows people to bring in new data from the leading system. So the non-editable would only apply to manual editing. What do you think @kitodo/kitodo-community-board?

  • When hovering over the arrow icons in the dialog header, we should not change the pointer symbol, since this indicates an action. The arrow- symbol here has no functionality. We should maybe show a tooltip here instead.

image

  • When clicking and not hovering over the ?-symbol the display of the large tooltip is broken.

image

@BartChris
Copy link
Collaborator

BartChris commented Oct 21, 2024

@solth We had a discussion on the implementation in the @kitodo/kitodo-community-board. Based on that we would like you to implement the following behaviors

  1. It should be possible to change the value of keys which are defined only in Kitodo, but not in the source catalog. In other words, it should be possible to delete values in Kitodo, if the user demands it.

Right now, this is not possible

// deleting values (e.g. replacing them with empty, new values) is not supported
if (newValues.isEmpty()) {
selectionMode = Reimport.KEEP;

The default setting in the interface should stay "KEEP" (the data in Kitodo), but it should be possible to change that by clicking on the arrow (only left and right arrow possible in that scenario). If somebody explicitely defines "REPLACE" as setting for this key in the ruleset, the default setting in the interface is also "REPLACE", but the user should be able to change that as well by clicking on the arrow in the interface.

  1. We would apply the ruleset during reimport, but would restrict it to only specific aspects. In general we would check only for the min- and max-Ocuurence rules. To be more precise:
  • If the user wants to import a values for a simple key which does not conform to the occurence settings in the ruleset, importing the value(s) for that key is not possible (arrow is greyed out)

  • So if the current setting for language is "eng" and maxOccurence for language is 1, the user cannot import the values "eng" and "ger" for language key. We grey out the arrow and keep the data currently in Kitodo

  • If minOccurs is 1, there is one value in Kitodo and the there is nothing coming from the catalog for this value, we cannot delete the value in Kitodo.

  • Those rules should also apply for groups, but only on top level, not for nested keys, which means:

  • if there should be max 1 person group in the metadata, it is impossible to import 2 persons. It is also not possible to import a record with 0 persons if we need at least one person value group.

  • We should not check for the rules of sub keys. If a "first name" is required for persons, and one person coming from the import does not have a first name, we can still import the whole group, since group key values are handeld en bloc.

What should definitely be not considered is the setting for editing values. This only applies to manual editings and not changes coming from the source system. Setting a key to non editable should not prevent overwriting the key during re-import. (If the key is not set to "protected - see 3)

  1. We would like to extend the possible settings for keys

https://github.com/kitodo/kitodo-production/blob/7896b2241baf63114abd8b524d0f1f49ef76394e/Kitodo/src/main/java/org/kitodo/production/services/dataeditor/DataEditorService.java#L551-L650

It should also be possible to "protect" a key from being overriden. As this is now possible for all the keys (see 1.), we would like to have an additional setting "protect", which makes it impossible to override a key in Kitodo with values from the catalog by disabling the arrows for that key in the user interface. This would be really important for some identifiers.
This makes it maybe necessary to adapt the logic here.
https://github.com/kitodo/kitodo-production/pull/5613/files

Would it be possible to add that?

  1. We also discussed wether it makes sense to disallow the mixing of metadata in case of groups, since this can have confusing effects. After discussing we concluded that we would keep the current behaviour of alsow allowing using the values from both sides. The user then maybe has to clean up the data afterwards.

@solth solth marked this pull request as draft October 22, 2024 07:08
@solth
Copy link
Member Author

solth commented Oct 22, 2024

@solth We had a discussion on the implementation in the @kitodo/kitodo-community-board. Based on that we would like you to implement the following behaviors

1. It should be possible to change the value of keys which are defined only in Kitodo, but not in the source catalog.  In other words, it should be possible to delete values in Kitodo, if the user demands it.

Right now, this is not possible

// deleting values (e.g. replacing them with empty, new values) is not supported
if (newValues.isEmpty()) {
selectionMode = Reimport.KEEP;

I implemented it this way in accordance with the description in #5904:

"Eine Funktion “Löschen” von Metadaten in Kitodo.Production wird ausdrücklich nicht erwünscht. Auch dann nicht, wenn die Metadaten nicht (mehr) im Quellsystem vorhanden sind."

I can remove this check and allow selecting empty values from the catalog, effectively deleting values in Kitodo, but I think it's important to note that this is a change of the original concept.

2. We would apply the ruleset during reimport, but would restrict it to only specific aspects. In general we would check only for the min- and max-Ocuurence rules. To be more precise:

* If the user wants to import a values for a simple key which does not conform to the occurence settings in the ruleset, importing the value(s) for that key is not possible (arrow is greyed out)

* So if the current setting for language is "eng" and maxOccurence for language is 1, the user cannot import the values "eng" and "ger" for language key. We grey out the arrow and keep the data currently in Kitodo

* If minOccurs is 1,  there is one value in Kitodo and the there is nothing coming from the catalog for this value, we cannot delete the value in Kitodo.

* Those rules should also apply for groups, but only on top level, not for nested keys, which means:

* if there should be max 1 person group in the metadata, it is impossible to import 2 persons. It is also not possible to import a record with 0 persons if we need at least one person value group.

* We should not check for the rules of sub keys. If a "first name" is required for persons, and one person coming from the import does not have a first name, we can still import the whole group, since group key values are handeld en bloc.

I will try to incorporate the minOccurs and maxOccurs rules as you described, that will take some time.

3. We would like to extend the possible settings for keys

https://github.com/kitodo/kitodo-production/blob/7896b2241baf63114abd8b524d0f1f49ef76394e/Kitodo/src/main/java/org/kitodo/production/services/dataeditor/DataEditorService.java#L551-L650

It should also be possible to "protect" a key from being overriden. As this is now possible for all the keys (see 1.), we would like to have an additional setting "protect", which makes it impossible to override a key in Kitodo with values from the catalog by disabling the arrows for that key in the user interface. This would be really important for some identifiers. This makes it maybe necessary to adapt the logic here. https://github.com/kitodo/kitodo-production/pull/5613/files
Would it be possible to add that?

While I see the reasoning behind the idea of an additional setting, this is out of scope of the current pull request and will have to be added separately later.

  1. We also discussed wether it makes sense to disallow the mixing of metadata in case of groups, since this can have confusing effects. After discussing we concluded that we would keep the current behaviour of alsow allowing using the values from both sides. The user then maybe has to clean up the data afterwards.

This is something that has already been ruled out in the original concept (#5904):

"Es werden entweder alle Gruppen übernommen oder gar keine"

So this is something I didn't consider to begin with 😉

@BartChris
Copy link
Collaborator

Thanks a lot @solth

"Eine Funktion “Löschen” von Metadaten in Kitodo.Production wird ausdrücklich nicht erwünscht. Auch dann nicht, wenn die Metadaten nicht (mehr) im Quellsystem vorhanden sind."

I can remove this check and allow selecting empty values from the catalog, effectively deleting values in Kitodo, but I think it's important to note that this is a change of the original concept.

I understand, and I apologize for the confusion caused by the back and forth on this issue. We’ve reconsidered, and there might indeed be situations where, for instance, a subtitle is deleted in the catalog. In such cases, it was deemed important to allow for the deletion of the incorrect subtitle in Kitodo, even if it means that all Kitodo keys could potentially be deleted (effectively changing the original concept). By implementing ruleset checks (as described in point 2), the deletion of a required key is still prevented.

It should also be possible to "protect" a key from being overriden. As this is now possible for all the keys (see 1.), we would like to have an additional setting "protect", which makes it impossible to override a key in Kitodo with values from the catalog by disabling the arrows for that key in the user interface.

While I see the reasoning behind the idea of an additional setting, this is out of scope of the current pull request and will have to be added separately later.

Alright, i agree this can be added later.

@BartChris
Copy link
Collaborator

If I change the “Haupttitel” (metadata key="LABEL" with maxOccurs="1" in the ruleset.xml) and use the “left-right"-arrow, I can add a second "Haupttitel" even though this is not permitted by the ruleset. There is no validation error. I can save the process. After a reload, both "Haupttitel" are listed. Maybe this is a problem? Ideally, the "left-right"-arrow should be disabled in case the ruleset does not allow to add another occurrence of this metadata field.

I encountered some anomalies while validating the 'LABEL' key, so it might be that the validation failure is specifically related to this key. But i have not tested that. The implementation of our validation recommendations should prevent this constellation as well.

@solth
Copy link
Member Author

solth commented Oct 22, 2024

I experimented with your changes a bit. I added the new permissions, imported a process from a catalog, changed a few metadata fields, and clicked the new "sync" button.

My first interpretation of the arrow icons was that they indicate the data flow. At first, I expected the left arrow to overwrite Kitodo metadata with the imported data, assuming the left arrow would “move” the data from right to left. I didn't really notice the text color. It took a minute to understand that the arrow is pointing towards the data that is kept. Currently, I don't have an idea how to improve this symbology. Maybe you could add a title to all the icons:

* “left arrow” - Keep Kitodo.Production metadata, ignore imported metadata

* “right arrow” - Overwrite Kitodo.Production metadata with imported metadata

* “left-right-arrow” - Add imported metadata field to Kitodo.Production, but keep existing metadata

@thomaslow I added a tooltip to the double arrow in the header as well as to the individual arrow buttons in each row to make it clearer what each arrow means.

If I change the “Haupttitel” (metadata key="LABEL" with maxOccurs="1" in the ruleset.xml) and use the “left-right"-arrow, I can add a second "Haupttitel" even though this is not permitted by the ruleset. There is no validation error. I can save the process. After a reload, both "Haupttitel" are listed. Maybe this is a problem? Ideally, the "left-right"-arrow should be disabled in case the ruleset does not allow to add another occurrence of this metadata field.

I added some checks that take ruleset minOccurs and maxOccurs rules into account and should prevent errors like the one you described.

@solth
Copy link
Member Author

solth commented Oct 22, 2024

@BartChris @thomaslow @subhhwendt I added some checks incorporating the minOccurs and maxOccurs rules from the ruleset that should prevent selecting more or less metadata entries than allowed. This is done by skipping the "next" re-import mode that would normally follow the current one.

Normally, when clicking the "Keep" button (⬅️ ) it is replaced with the "Add" button (↔️ ) and then by the "Replace" button (➡️ ).

The new checks first verify that the next re-import mode in this cycle is possible while adhering to the rules from the ruleset. Otherwise the next mode in the order described above is skipped. If no new mode is possible, the button remains the same.

@solth solth marked this pull request as ready for review October 22, 2024 17:49
@solth solth marked this pull request as ready for review October 28, 2024 12:35
@solth solth requested a review from BartChris October 28, 2024 12:35
@solth
Copy link
Member Author

solth commented Oct 28, 2024

After some testing we discovered some subtle bugs:

1. If a new metadatum (group or simple) is coming from the catalog and there is nothing on the Kitodo side, it is impossible to add that data.

image

One can press "apply" here, but Erscheinungsjahr 2011 is not added to the Kitodo metadata.

2. After re-importing metadata no input field can be edited anymore. One has to close the metadata editor and open it again in order to edit the metadata again.

3. The ruleset (min and max Occurence)  is not considered when displaying the arrows initially, only when changing the initial arrow. If e.g. maxOccurence for key "language" is 1 and the catalog has two languages the arrow should initially not point to the catalog side ("REPLACE"), which would allow to import invalid data. The initial setting then should be "KEEP".

@BartChris These should all be fixed now. Would you mind re-reviewing the pull request? I think I have incorporated all of your remarks.

Copy link
Collaborator

@BartChris BartChris left a comment

Choose a reason for hiding this comment

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

Looks good now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update metadata
5 participants