Process parent link

[solth]

Fixes #5626

Builds upon #5663 by @BartChris

As discussed in that pull request, processes in projects that are not assigned to the current user cannot be chosen as parent process in the TitleRecordLinkTab by default. Instead the corresponding option will be deactivated and a note will be displayed to inform the user why he cannot link to that specific parent process:

This PR adds a new permission, though, that allows a user to link processes to parent processes in other projects which are not assigned to the current user. The option in the TitleRecordLinkTab will still contain the hint about the parent process belonging to a project to which the current user does not have regular access, but the option to select said parent process will now be activated:

@andre-hohmann & @BartChris does this pull request meet your requirements for the discussed functionality?

[BartChris]

@solth Thanks a lot for working on that again. I am again trying to work myself through the code. The general functionality seems to work good. But the way the code is written right now leads to a behavior where always only one possible parent is shown to the user. So the user does not even see all possible parents (independent of the fact wether he is able to actually select a specific process)

Imagine a situation where you have one possible parent process in the same project (as the to be created child record), and another possible parent process in a different project. Right now, only one of those is available for selection by the user.

Scenario a) Using the search button in the title record link tab:

We search for multiple possible parent processes, but after we set one of them as the parent process using setParentAsTitleRecord we break out of the for-loop so no other possible parent gets placed into the select menu.

https://github.com/kitodo/kitodo-production/blob/a9ea2187d96e57401ceed106bf09af399394d726/Kitodo/src/main/java/org/kitodo/production/forms/createprocess/TitleRecordLinkTab.java#L319-L332

This could mean, that a parent process in the same project gets skipped because a possible parent process in the other project is chosen. (Given the case that the user has the ability to link to processes in other projects). This could happen also because the found processes are not sorted in that case of a manual search as done in the case of the automatic search for a parent record after the catalog import (https://github.com/kitodo/kitodo-production/blob/a9ea2187d96e57401ceed106bf09af399394d726/Kitodo/src/main/java/org/kitodo/production/services/data/ImportService.java#L986), see b)

We should always show all possible parents when a user uses the search functionality.

Scenario b) Automatic search for parent after catalog import.

During a search for a title using catalog import Kitodo triggers an automatic search for parent records in the Kitodo database using the method loadParentProcess. This method only returns a single parent:

https://github.com/kitodo/kitodo-production/blob/a9ea2187d96e57401ceed106bf09af399394d726/Kitodo/src/main/java/org/kitodo/production/services/data/ImportService.java#L967-L970

So when calling setParentAsTitleRecord we again end up with only one selectable possible parent in our select box even if there is more than one possible parent process.

So the question would be, wether we want to show the user all possible parents in all projects or wether we want to automatically select one, as it is implicitly done with the current implementation.

[BartChris]

This is the main use in the SLUB Dresden and it works very fine - thanks a lot!

I have tried to apply to periodical volumes, but this does not work as expected. If i enter the identifier of a superordinate process, only one process of a volume is shown.

@andre-hohmann The problem is that the search implemented in the Title Record Link tab is not actually a search for a parent, but a search for every process whose ID or title matches the given input. Since every volume of the peridocial has the same base value as part of the identifier Kitodo just finds randomly one of those entries that match. (So in the example a single volume of that periodical)

https://github.com/kitodo/kitodo-production/blob/a9ea2187d96e57401ceed106bf09af399394d726/Kitodo/src/main/java/org/kitodo/production/services/data/ProcessService.java#L819

You would have to specify the precise ID of the parent in order to find only the parent process.

[andre-hohmann]

@BartChris : Thanks a lot for the investigation and explanation!

This could become more obvious, if all processes are shown, which contain the search criteria. With regard to our current use cases, it is not important but good to know.

[BartChris]

@andre-hohmann The problem is that the search implemented in the Title Record Link tab is not actually a search for a parent, but a search for every process whose ID or title matches the given input. Since every volume of the peridocial has the same base value as part of the identifier Kitodo just finds randomly one of those entries that match. (So in the example a single volume of that periodical)

The comment of the findLinkableParentProcesses method already indicates that returning only possible parents would require to look up the rulesets of the parent candidate. But it seems like this is not happening.

Searches for linkable processes based on user input. A process can be linked if it has the same rule set, belongs to the same client, and the topmost element of the logical outline below the selected parent element is an allowed child. For the latter, the data file must be read at the moment. This will be aborted after a timeout so that the user gets an answer (which may be incomplete) in finite time.

We could use ProcessService.canCreateChildProcess for that. But i am not sure about the performance implications if we apply that here (it would be good, if Elasticsearch would allow us to filter out all possible parents):

https://github.com/kitodo/kitodo-production/blob/a9ea2187d96e57401ceed106bf09af399394d726/Kitodo/src/main/java/org/kitodo/production/services/data/ImportService.java#L1431

and extend the check with that condition:

 public ArrayList<SelectItem> getPotentialParentProcesses(List<ProcessDTO> parentCandidates, int maxNumber)
            throws DAOException, IOException {
        ArrayList<SelectItem> possibleParentProcesses = new ArrayList<>();
        for (ProcessDTO process : parentCandidates.subList(0, Math.min(parentCandidates.size(), maxNumber))) {
            if (ProcessService.canCreateChildProcess(process)) {
                SelectItem selectItem = new SelectItem(process.getId().toString(), process.getTitle());
                selectItem.setDisabled(!userMayLinkToParent(process.getId()));
                if (!processInAssignedProject(process.getId())) {
                    String problem = Helper.getTranslation("projectNotAssignedToCurrentUser", process.getProject().getTitle());
                    selectItem.setDescription(problem);
                    selectItem.setLabel(selectItem.getLabel() + " (" + problem + ")");
                }
                possibleParentProcesses.add(selectItem);
            }
        }
        return possibleParentProcesses;
    }

or filter the records directly in the search service

  public List<ProcessDTO> findLinkableParentProcesses(String searchInput, int rulesetId)
            throws DataException, DAOException, IOException {

        BoolQueryBuilder processQuery = new BoolQueryBuilder()
                .should(createSimpleWildcardQuery(ProcessTypeField.TITLE.getKey(), searchInput));
        if (searchInput.matches("\\d*")) {
            processQuery.should(new MatchQueryBuilder(ProcessTypeField.ID.getKey(), searchInput));
        }
        BoolQueryBuilder query = new BoolQueryBuilder().must(processQuery)
                .must(new MatchQueryBuilder(ProcessTypeField.RULESET.getKey(), rulesetId));
        List<ProcessDTO> filteredProcesses = new ArrayList<>();
        for (ProcessDTO process : findByQueryInAllProjects(query, false)) {
                if (ProcessService.canCreateChildProcess(process)) {
                    filteredProcesses.add(process);
                }
        }
        return filteredProcesses;
    }

[andre-hohmann]

@solth and @BartChris : i am not sure, how to proceed with @BartChris last proposal.

On the one hand, i think there is no need to solve the problem for periodical-processes. Maybe, this is only a problem of the SLUB Dresden due to the the local configuration to create the process title - and we have no disadvantage with the current workflows. On the other hand, i feel a bit uncomfortable to approve the pull request despite i am not fully convinced about the functionality.

As i do not see a use case which affects the SLUB Dresden (or other institutions) and i cannot estimate the resources and effects of the last proposal to improve the functionality, i would approve the current version of the PR. Then it can be merged. We will then find out whether the behavior of periodical process leads to specific problems in other institutions and we could pick up the discussion again.

Is this procedure OK for you?

If yes, @henning-gerhardt could review the technical aspects of this PR.

[BartChris]

@andre-hohmann I also want to move forward here, so i agree that we should strive for a merge. Regarding my different comments from above:

• The association to a parent process can always be changed later on. So yes, there might be situations, where we have multiple instances of a parent catalog id and Kitodo might (if one process is chosen automatically during import) associate to the wrong "process", but the user can change that manually. So i would be on board with the behaviour of the PR, that Kitodo (when automatically searching for a parent after import) picks one. Even if it picks the "wrong" process. The user can then correct this manually (e.g. by searching again in the Title Link tab or later in the metadata editor). I think we can optimize here (not so easy since this effects mass import), but maybe we can do that at a later point.
• The fact that the search in the Title Record Link Tab is not a search for possible parents only (but for everything) might be corrected in a later Pull Request, so that we longer delay the main changes here.
• The behaviour that the search in the Title Link tab only returns one result should probably be changed since in Kitodo 3.6 the software returns more than one result.

So the main suggested change to this PR would be that we should display multiple results in the search in the Title Record Link tab since there is no real reason why that worked in Kitodo 3.6 but does not work in 3.7 anymore. (And it is also required to enable the user tho choose between multiple possible parents).

[henning-gerhardt]

@BartChris

we should display multiple results in the search in the Title Record Link tab since there is no real reason why that worked in Kitodo 3.6 but does not work in 3.7 anymore

Is this issue introduced by this pull request or is this issue not related to this pull request and is existing already in 3.7 / master branch? If this issue is not related to this pull request then it should be solved by a separate pull request.

[andre-hohmann]

Thanks! I will open a new issue to describe the missing functionality, which has been summarized by @BartChris. It is then possible to create an own PR for that issue with the necessary discussion.

[BartChris]

@BartChris

we should display multiple results in the search in the Title Record Link tab since there is no real reason why that worked in Kitodo 3.6 but does not work in 3.7 anymore

Is this issue introduced by this pull request or is this issue not related to this pull request and is existing already in 3.7 / master branch? If this issue is not related to this pull request then it should be solved by a separate pull request.

This is introduced by the current pull request. In master you get multiple hits.

[henning-gerhardt]

This is introduced by the current pull request. In master you get multiple hits.

Then this must be fixed in current pull request.

[solth]

@andre-hohmann , @BartChris , @henning-gerhardt sorry for not responding earlier, I didn't find the time to work on this pull request in the last few weeks. I will try to incorporate the changes in the functionality (show multiple potential parent processes in the pulldown menu as options, maybe check/filter for "canCreateChildren") and implement the change requests from the code review.

[solth]

We could use ProcessService.canCreateChildProcess for that. But i am not sure about the performance implications if we apply that here (it would be good, if Elasticsearch would allow us to filter out all possible parents): ... or filter the records directly in the search service ...

I think we need to do the check in ImportService.getPotentialParentProcesses instead of ProcessService.findLinkableParentProcesses, because the former is also used in TitleRecordLinkTab.setParentAsTitleRecord (for automatically determining the parent process during import) and thus would not be covered if we added the filter to the later function. @BartChris do you agree?

[solth]

Also, wouldn't it be necessary to also check for createChildrenWithCalendar in addition to createChildrenFromParent, to be able to manually link newspaper issues?

[solth]

• The behaviour that the search in the Title Link tab only returns one result should probably be changed since in Kitodo 3.6 the software returns more than one result.

This still confuses me, because the code limits the list of potential process parents to one in prior versions (3.5, 3.6 etc) as well:

parentCandidates.add(new SelectItem(parentProcess.getId().toString(), parentProcess.getTitle()));
createProcessForm.getTitleRecordLinkTab().setPossibleParentProcesses(parentCandidates);

[BartChris]

We could use ProcessService.canCreateChildProcess for that. But i am not sure about the performance implications if we apply that here (it would be good, if Elasticsearch would allow us to filter out all possible parents): ... or filter the records directly in the search service ...

I think we need to do the check in ImportService.getPotentialParentProcesses instead of ProcessService.findLinkableParentProcesses, because the former is also used in TitleRecordLinkTab.setParentAsTitleRecord (for automatically determining the parent process during import) and thus would not be covered if we added the filter to the later function. @BartChris do you agree?

As outlined in my code comment, the main problem is that the filtering for possible parents has to be applied on the whole result set. It can probably be done in both functions: ProcessService.findLinkableParentProcesses or TitleRecordLinkTab.setParentAsTitleRecord but before limiting down the displayed results to only 10. We might get back a lot of hits unfortunately, so those checks maybe take too long. I will try to experiment how performance is affected if the checkl for potential parents come first.

(We probably need a fast way to identify (possible) parents in other places as well https://github.com/kitodo/kitodo-production/issues/5731)

[solth]

We could use ProcessService.canCreateChildProcess for that. But i am not sure about the performance implications if we apply that here (it would be good, if Elasticsearch would allow us to filter out all possible parents): ... or filter the records directly in the search service ...

I think we need to do the check in ImportService.getPotentialParentProcesses instead of ProcessService.findLinkableParentProcesses, because the former is also used in TitleRecordLinkTab.setParentAsTitleRecord (for automatically determining the parent process during import) and thus would not be covered if we added the filter to the later function. @BartChris do you agree?

As outlined in my code comment, the main problem is that the filtering for possible parents has to be applied on the whole result set. It can probably be done in both functions: ProcessService.findLinkableParentProcesses or TitleRecordLinkTab.setParentAsTitleRecord but before limiting down the displayed results to only 10. We might get back a lot of hits unfortunately, so those checks maybe take too long.

Thanks for the analysis. While fixing the failing test I already moved the filtering of the processes from the ImportService to ProcessService.findLinkableParentProcesses because it wasn't required in setParentAsTitleRecord after all.

I just need to adjust some test resources and hope it will work as expected afterwards. If you don't mind I would request a another review from you, so can see if there is something still missing or not.

[BartChris]

Yes, of course. I will review

[BartChris]

@solth Another thing which i already stumbled over in the old PR (https://github.com/kitodo/kitodo-production/pull/5663#issuecomment-1821226911) but forgot again: We also need to apply the authority checks to the ImportService.loadParentProcess method, otherwise a process could be linked although the user does not have rights for the project of the parent process.

[solth]

Another thing which i already stumbled over in the old PR (#5663 (comment)) but forgot again: We also need to apply the authority checks to the ImportService.loadParentProcess method, otherwise a process could be linked although the user does not have rights for the project of the parent process.

It seems this still needs some more time in the oven. I will try to incorporate that aspect when I find time, which I hope will be at the end of the month.

[solth]

I have made the following adjustments:

• the method TitleRecordLinkTab.setParentAsTitleRecord does not reset the list of possibleParentProcesses anymore (I can't see any reason why this would be done here in the first place, so I think this was bugged in the past as well)
• the options representing the potential parent processes in the pulldown menu are now sorted alphabetically
• the potential parent processes are now filtered to only allow those that are either configured as canCreateChildProcess or canCreateProcessWithCalendar in the ruleset
• I removed the call to getPotentialParentProcesses from setParentAsTitleRecord, and added the new filter mentioned above directly to the ProcessService.findLinkableParentProcesses as @BartChris suggested above

Still TODO:

• incorporate checks into ImportService.loadParentProcess

@BartChris , @henning-gerhardt @andre-hohmann would you mind checking again whether this version now meets all requirements you mentioned or whether something still needs to be changed (apart from the open "TODO" mentioned above) ?

[BartChris]

@solth I tested a lot of combinations and for me everything works. One little thing thing to correct is probably the message which is shown, when you manually search for a process title and nothing is found:

The message should be changed to: "The search completed, but nothing was found. You can only find processes that have the same rule set and are possible parent processes."

Regarding the TODO: After testing again i noticed that we already check for the permission of the user in the setParentAsTitleRecord-method

https://github.com/effective-webwork/kitodo-production/blob/e6baeb9f31f2f565d1edb9473476c510c655e061/Kitodo/src/main/java/org/kitodo/production/forms/createprocess/TitleRecordLinkTab.java#L449

which in the cases i tested gets called when we use the MetadataImportDIalog

https://github.com/effective-webwork/kitodo-production/blob/e6baeb9f31f2f565d1edb9473476c510c655e061/Kitodo/src/main/java/org/kitodo/production/forms/createprocess/MetadataImportDialog.java#L75

So it seems that there is already protection against somebody attaching the process to a parent process, although he does not have the rights.

I cannot say for sure wether it gets called in all cases because i do not fully understand the cases here:

if (numberOfProcesses == 1 && Objects.nonNull(parentTempProcess)) {
            // case 1: only one process was imported => load DB parent into "TitleRecordLinkTab"
            this.createProcessForm.getTitleRecordLinkTab().setParentAsTitleRecord(parentTempProcess.getProcess());
        } else {
            // case 2: multiple processes imported and one ancestor found in DB => add ancestor to list
            if (Objects.nonNull(parentTempProcess)) {
                this.createProcessForm.getProcesses().add(parentTempProcess);
            }

However, i think that this would definitley not protect against (not allowed) linkings during Mass import so i would propose to add the check in loadParentProcess as well (as you have already suggested).

There could be some further remarks on what gets selected while using loadParentRecord. Because that effectively decides what the process gets linked to (especially during Mass import since there is no mechanism to choose another one there), but i for now support the PR in the current form.

TLDR: I support the current implementation and still think the implementation of the one outstanding TODO is necesary. But i think in generally we can move forward now. Great work!

[andre-hohmann]

From my users' point of point, it works fine now - thanks a lot!

Here are some aspects/results, which i have tested:

1. It is possible to create a child process, even if the superordinate process is located in a project to which the user has no permissions
2. Only the parent process is shown in the result list of the field "Vorgang auswählen"
3. In "Vorgang auswählen" it is indicated: "Das Projekt "Adressbücher" ist Ihnen nicht zugewiesen!" - if the user has no permissions for the project of the superordinate process.

[solth]

TLDR: I support the current implementation and still think the implementation of the one outstanding TODO is necesary. But i think in generally we can move forward now. Great work!

@BartChris sorry for the long delay. So do you think we should merge the pull request as is and add further adjustments in separate pull requests, or should the outstanding TODO be included in this PR?

[BartChris]

@solth Thanks a lot. I would merge the PR as is and add the outstanding TODOs (Most important a check for permissions during mass import) later.

[solth]

@BartChris thanks for the response. I just fixed a test so that the CI build is now succesfull. Would you mind checking whether or not you approve the current state of the PR/branch? I will then merge it after your approval.

[solth]

@BartChris can you formally approve this PR or make change requests if you see something that should be adjusted before merging?

[BartChris]

@solth I approve the Pull Request in the current form.

We probably need to document the behavior (in the current or in an adapted form) before releasing 3.7. Outstanding issues which can be adressed later:

• during Mass import there is no check wether the user triggering the Mass import has the right to attach a process to a parent process in a project he or she does not have access to. as The check wether this is allowed is only reached during "normal" import via the UI (https://github.com/kitodo/kitodo-production/blob/d40c2fd92a84cbb243e13b1ac76a1d2a34860ebe/Kitodo/src/main/java/org/kitodo/production/forms/createprocess/TitleRecordLinkTab.java#L449)
• When somebody imports a record using the UI, which has a parent record, Kitodo will automatically find the parent record. But if the user does not have the necessary right the method setParentAsTitleRecord checks the rights (https://github.com/kitodo/kitodo-production/blob/d40c2fd92a84cbb243e13b1ac76a1d2a34860ebe/Kitodo/src/main/java/org/kitodo/production/forms/createprocess/TitleRecordLinkTab.java#L449) so that the process is not shown to the user. The dropdown is just empty. I suggest that in that case, the parent record is shown, but greyed out, so that the user has an idea of what is actually going on. (Kitodo behinds the scenes gets the parent record from the UI, creates a parentTempProcess, but discards it while constructing the TitleRecordLinkTab). Calling setChosenParentProcess(null);

[henning-gerhardt]

@BartChris : maybe by opening two issues with references to this issue? Both issues can be solved independent of each other (or maybe even together) so maybe one of the issues could be solved earlier than the other. Especially the second sound for me more important then the first one but maybe only as we did not use the mass import feature.

[BartChris]

I will add those issues, when the current PR gets merged and will also inspect wether there are more potential features to consider and to be adressed in issues. (e.g. topics outlined in this comment https://github.com/kitodo/kitodo-production/pull/5869#issuecomment-1894328022, section on automatic import)

[solth]

I approve the Pull Request in the current form.

@BartChris Sorry, I thought you had the "Approve" button as in other pull requests when reviewing the changes in the "Files" tab, but apparently that was not the case.