Skip to content

Import catalog metadata from any record in the metadata editor#6443

Merged
solth merged 18 commits intokitodo:mainfrom
thomaslow:import-structural-data-from-catalog-fixes-5915
May 9, 2025
Merged

Import catalog metadata from any record in the metadata editor#6443
solth merged 18 commits intokitodo:mainfrom
thomaslow:import-structural-data-from-catalog-fixes-5915

Conversation

@thomaslow
Copy link
Copy Markdown
Collaborator

@thomaslow thomaslow commented Feb 27, 2025

Fixes #5915

Todo

  • Discuss user interface design with community
  • Improve code style
  • Add selenium tests

Demonstration video:

2025-05-02.13-07-38.mp4

…or by reusing the catalog search and hitlist dialog of the create process page.
…w record identifier of selected record after catalog search in metadata comparison dialog. Change button title description depending on whether metadata is updated for current process or metadata is imported for selected structure element. Add simple selenium test checking that catalog search dialog appears.
@thomaslow
Copy link
Copy Markdown
Collaborator Author

I merged both buttons to a single update metadata button. Depending on the current selection and information known about the process, different things happen:

  • if the root structure element is selected, and a record identifier and import configuration is know for the current process, the catalog metadata is loaded without an additional search dialog (exactly like the previous behavior)

  • if no record identifier is known for the process, or any other structure element is selected, the catalog search dialog opens, such that the user can search for the appropriate catalog record

  • if a page or parent process is selected, the button is disabled

The updated implementation is shown in the updated demonstration video at the top.

@thomaslow thomaslow marked this pull request as ready for review April 3, 2025 11:37
@thomaslow thomaslow force-pushed the import-structural-data-from-catalog-fixes-5915 branch from bd24d83 to 4658436 Compare April 4, 2025 11:32
@solth
Copy link
Copy Markdown
Member

solth commented Apr 17, 2025

@thomaslow Thank you very much for this pull request. This seems to be a very valuable addition to Kitodo and something that has been asked for many times. I wonder why you haven't got more feedback about this. Perhaps the @kitodo/kitodo-community-board would like to comment on this proposal?

@Westedt
Copy link
Copy Markdown

Westedt commented Apr 24, 2025

We've tested the trial version and are very content with the implementation, thank you!

Copy link
Copy Markdown
Collaborator

@oliver-stoehr oliver-stoehr left a comment

Choose a reason for hiding this comment

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

This is a great addition for the metadata editor!

I would like to suggest one change:
Updating existing metadata and adding new metadata (potentially from a completely different source) are different features and it could be confusing when they are accessible through the same button.
In my opinion it would be helpful to split it into two separate buttons and use fa-book as icon for importing catalog metadata.
This is the icon we already use for the "catalog search" button (and the "data transfer" in general.)
The header of the metadata panel offers enough space for two buttons and it would be clearer if both buttons were available side by side and just disabled when not applicable.

@thomaslow
Copy link
Copy Markdown
Collaborator Author

@oliver-stoehr Thank you for your review.

I understand your reasoning for having two buttons. Actually, the pull request was adding a separate button in the beginning.

I changed it to one combined button, because the update-metadata-based-on-known-import-configuration-button was disabled most of the time anyway, and it seemed wasteful to add another button that does almost exactly the same thing (also in the backend). If you look at it "in reverse" and interpret the update-metadata-based-on-known-import-configuration function as a "convenience feature" of the new button (not asking the user to type in a PPN that is already known), the combined button makes sense to me.

The current solution was already tested and accepted by users of the SPK-Berlin. They were aware of the two slightly different functions, and there were no reports of any confusion or misunderstanding. However, I don't think they had a strong opinion about this one button vs. two buttons situation.

Personally, I think both solutions would be valid.

I can change the pull request to two buttons. However, I would like to avoid having to revert these changes again in two weeks in case someone else voices their opinion about liking the combined button. I don't know how something like this is decided usually in Kitodo. Maybe someone else can comment on this so we have a clear democratic majority decision?

@Westedt What do you think?

@oliver-stoehr
Copy link
Copy Markdown
Collaborator

I can understand your concerns and you're right, you should not have to revert those changes again in a few weeks.
Seeing the "update" button as a convenience feature of the new "import" button is a good point of view.

One point that came to my mind: Is it useful to import metadata to the root element, even if the process was created via catalog import and could be updated?
(E.g. I created a process from PPN 1234 and I can update the metadata in the editor with the existing "update" button. In this scenario, is it useful to be able to use the "import" button to import additional metadata from another PPN like 1235?)
I cannot judge whether this is a realistic scenario, so I think a users opinion like @Westedt would be helpful!

If the users are fine with one combined button and there is no realistic scenario for both buttons, we should keep the combined button. In that case: It could be helpful to change the icon of the combined button, depending on the underlying function (fa-refresh and fa-book). This could help to make clear, why the button can behave differently.

Copy link
Copy Markdown
Member

@solth solth left a comment

Choose a reason for hiding this comment

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

I am an favor of separated buttons as well. The reason is that at least for the logical root element, both actions - updating metadata from a linked import source and augmenting existing metadata with metadata from a differenct source/catalog - are semantically different and therefor it should be possible to trigger them individually. Updating metadata should stay possible for users without the knowledge of where the metadata was originally retrieved from and requiring them to select the correct catalog manually.

Also, in my opinion importing metadata into the root element of a process that does not have a linked import confguration, yet, should actually set the import configuration of that process to the catalog source the user actively selected, so that next time the metadata can be updated directly from the stored catalog/source via the first, dedicated "Update" button (or augmented with metadata from a different source via the separate, dedicated "Import"/"Augment" button.)

@thomaslow
Copy link
Copy Markdown
Collaborator Author

I am an favor of separated buttons as well.

Ok. I'll separate the buttons again.

Also, in my opinion importing metadata into the root element of a process that does not have a linked import configuration, yet, should actually set the import configuration of that process to the catalog source the user actively selected, so that next time the metadata can be updated directly from the stored catalog/source via the first, dedicated "Update" button (or augmented with metadata from a different source via the separate, dedicated "Import"/"Augment" button.)

Maybe, but that would mix the functions of both buttons again. For example, if there is no import configuration set for a process yet, and the users clicks the "augment" button, but accidentally or on-purpose types in a "wrong" PPN to augment the metadata from a record that is not really the source-record, the wrong default import configuration is remembered for this process. This makes everything more complicated. There would need to be a separate option such that the user can decide whether to remember this import configuration as the default configuration. Unfortunately, this is not covered by #5915.

thomaslow and others added 2 commits May 2, 2025 12:36
Co-authored-by: Arved Solth <solth@effective-webwork.de>
…etadata based on an existing import configuration and importing metadata from a catalog.
@thomaslow
Copy link
Copy Markdown
Collaborator Author

I updated the demonstration video to show that there are now 2 buttons:

  • the previous update button that only works if the process was created from an import configuration
  • the new import button that can be used for any logical structure element (including the root node)

@solth
Copy link
Copy Markdown
Member

solth commented May 5, 2025

Right now the button "OPAC durchsuchen" remains disabled after entering a search ID:

Bildschirmfoto 2025-05-05 um 08 46 16

I made sure to reload the page including its cache to prevent problems stemming from outdated javascript or style resources. Did I miss something?

Edit: now it works!

* Perform a catalog search and open either the histlist dialog (if there are multiple search results)
* or the metadata comparison dialog, if there is only a single search result.
*/
public void catalogSearch() {
Copy link
Copy Markdown
Member

@solth solth May 5, 2025

Choose a reason for hiding this comment

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

My IDE falsely marks this method and selectRecord as unused. Perhaps it would make things clearer for developers in the future if we add a comment that this method and selectRecord are indeed not unused, but that their method names are passed to the hitlistDialog template in the metadataEditor and processFromTemplate templates?

existingMetadata,
tempProcess.getWorkpiece().getLogicalStructure().getMetadata(),
dataEditorForm.getPriorityList(),
dataEditorForm.getSelectedStructure().get().getType()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since dataEditorForm.getSelectedStructure() returns an Optional<LogicalDivision>, should we not first check whether the division isPresent before calling get() on the optional object?

@Westedt
Copy link
Copy Markdown

Westedt commented May 5, 2025

We actually liked the previous version with one shared button, but we are also fine with the new solution. Thanks @thomaslow !

@solth
Copy link
Copy Markdown
Member

solth commented May 8, 2025

I made sure to reload the page including its cache to prevent problems stemming from outdated javascript or style resources. Did I miss something?

I just tried again and know it works. Perhaps my last build was not somehow corrupted or I forgot to replace some modules or reload my browser cache. Anyway: now the "OPAC durchsuchen" button is activated when "Wert" field is filled in. 👍

Copy link
Copy Markdown
Member

@solth solth left a comment

Choose a reason for hiding this comment

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

Works very well for me and I have no further change requests for the code - apart from the little nitpicking above.

@solth
Copy link
Copy Markdown
Member

solth commented May 8, 2025

@thomaslow if you want you can accept my small suggestions regarding code changes, but they are not condition for merging this pull request. @oliver-stoehr do you approve this pull request in its current state?

thomaslow and others added 2 commits May 9, 2025 00:01
@thomaslow
Copy link
Copy Markdown
Collaborator Author

@solth Thank you for your feedback. I included your code suggestions. Unfortunately, I currently cannot investigate, test or implement any changes this week and most of next week.

@solth
Copy link
Copy Markdown
Member

solth commented May 9, 2025

Unfortunately, I currently cannot investigate, test or implement any changes this week and most of next week.

No worries, the failing build was caused by an unreliable test unrelated to your changes. I re-started the build and now it went through successfully.

@solth solth merged commit 0433b97 into kitodo:main May 9, 2025
5 of 6 checks passed
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.

Import of structural data elements from the catalog

4 participants