Prevent duplicate and invalid titles #6326

BartChris · 2024-11-22T12:45:48Z

henning-gerhardt · 2024-11-22T12:53:37Z

Kitodo/src/main/java/org/kitodo/config/enums/ParameterCore.java

@@ -367,7 +367,7 @@ public enum ParameterCore implements ParameterInterface {
    /**
     * Validation of process title via regular expression.
     */
-    VALIDATE_PROCESS_TITLE_REGEX(new Parameter<>("validateProzessTitelRegex", "[\\w-]*")),
+    VALIDATE_PROCESS_TITLE_REGEX(new Parameter<>("validateProzessTitelRegex", "^[a-zA-Z0-9_-]+$")),


Should the new default value not even updated in the kitodo_config.properties files?

I am wondering: This is a hard limitation of the Kitodo software, some titles lead to problems later. Should we allow to change that in the config.properties at all? Or should we hardcode that in the application? Additionally one can think of allowing additional regexes which might be checked on top of the minimal requirements. See #6055

cc @solth

At SLUB we allow even the . inside the process title as for historical reasons this was needed to create the processes. Maybe this can be changed now but this should be discussed.

One more note: the w meta character in regular expressions stands for a-z, A-Z, 0-9, including _ (underscore) . So the expression can be simplified to ^[\\w-]+$?

What about chosing a new parameter name "validateProcessTitle" which avoids the current "denglisch" (English-German mix)?

One more note: the w meta character in regular expressions stands for a-z, A-Z, 0-9, including _ (underscore) . So the expression can be simplified to ^[\\w-]+$?

Thanks i changed that. It turned out that [\\w-]+ in combination with Java's matches als works (the problem was that the necessary validation is just not called during process creation), but i think the changed regex is more expressive.

What about chosing a new parameter name "validateProcessTitle" which avoids the current "denglisch" (English-German mix)?

I also have sympathy for that, but that would require institutions having to adapt their config. I think the Pull Request, if accepted, should probably be backported which would probably be easier, when we do not change the configuration.

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

BartChris · 2024-11-22T14:23:04Z

Kitodo/src/test/java/org/kitodo/production/forms/copyprocess/CreateProcessFormIT.java

    }
+
+    @Test
+    public void shouldNotAllowDuplicateTitles() throws Exception {


We probably have to take into account the configuration here. On some systems this is allowed...
Edit: maybe not necessary.

BartChris · 2024-12-06T11:10:01Z

I think, for now, it would be less disruptive if institutions are allowed to override the regex in the configuration. This approach might introduce the possibility of configurations that Kitodo cannot handle (e.g., titles containing whitespace). However, one could consider introducing a baseline regex in the future—one that enforces basic constraints, such as disallowing whitespace (defined in the application and not changeable).

The configuration value could then be redefined and renamed to build on top of this baseline regex, allowing institutions to apply additional rules. For example, institutions could enforce identifiers that always start with a specific prefix, such as "digi_".

henning-gerhardt

I'm tested this change and let a few code comments inside.

The changes are working if you have access to all projects or at least to the same projects where a process is already existing. It did not work if the already process is an other project where the user has no access.

As in both referenced issues there is no mention if process duplication check should work over all projects for a client independent of the user as project access or not I'm can not judge if this is solution is in current state correct or not. In my opinion the duplication check should work over all projects of a client and not only over the projects where the user has access.

henning-gerhardt · 2024-12-10T08:58:28Z

Kitodo/src/test/java/org/kitodo/production/forms/copyprocess/CreateProcessFormIT.java

+    }
+
+    // Helper to clean up database and file system
+    private void cleanUpProcess(Process process) throws Exception {


Just a note: this method is only executed if all method executions before calling this method was successful as this method is called at the end of test execution. This could lead to a dirty repository and tests which are executed after this could executed in a wrong way. Maybe an approach with @AfterEach should be considered so the cleanup is done independent of a successful or failed test.

Thank you, i tried to adapt the tests.

henning-gerhardt · 2024-12-10T09:02:36Z

Kitodo/src/main/resources/kitodo_config.properties

@@ -404,7 +404,7 @@ numberOfMetaBackups=8
 useMetadatenvalidierung=true

 # Validierung der Vorgangstitel ueber regulaeren Ausdruck
-validateProzessTitelRegex=[\\w-]*
+validateProzessTitelRegex=^[\\w-]+$


Should the other kitodo_config.properties files with this entry be adjusted too?

henning-gerhardt · 2024-12-10T09:32:14Z

For an other issue I switched to release 3.7.1 and tried by accident to create a new monographic process from a catalogue but I got the message, that this process is already existing after I clicked on the save button in the create process form. So at least some kind of title duplication check is done even without this change.

…he client

BartChris · 2024-12-11T17:07:09Z

The changes are working if you have access to all projects or at least to the same projects where a process is already existing. It did not work if the already process is an other project where the user has no access.

As in both referenced issues there is no mention if process duplication check should work over all projects for a client independent of the user as project access or not I'm can not judge if this is solution is in current state correct or not. In my opinion the duplication check should work over all projects of a client and not only over the projects where the user has access.

I agree, the check is useless if it does not check every title of every project (in the same client); i adapted the logic and the tests to ensure that duplicate titles are not allowed on the same client. (No matter if the user has access to the projects or not)

henning-gerhardt · 2024-12-12T08:54:29Z

Kitodo/src/test/java/org/kitodo/production/forms/copyprocess/CreateProcessFormIT.java

-        Integer processId = newProcess.getId();
-        processService.remove(processId);
-        fileService.delete(URI.create(processId.toString()));
+        createdProcess = underTest.getMainProcess();


The determination of the created process could be moved before the first assertion is executed so you have anytime the right process information before cleaning up. If adjusted here it should be adjusted on the other places too.

Thanks, good idea. I did that.

henning-gerhardt

In general the changes are right. My two notes (adjustment of the other existing kitodo_config.properties files and the determination of the created process inside the tests) can be done and even not they do nothing wrong.

BartChris · 2024-12-12T14:13:57Z

Kitodo/src/test/java/org/kitodo/production/forms/copyprocess/CreateProcessFormIT.java

+        // Second process creation with duplicate title
+        CreateProcessForm underTestTwo = setupCreateProcessForm("Monograph");
+        underTestTwo.getMainProcess().setTitle("title");
+        underTestTwo.getMainProcess().setProject(ServiceManager.getProjectService().getById(secondProjectId));


In my previous implementation, I used project 2 for both process creations in the shouldNotAllowProcessTitlesInProjectsTheUserDoesNotBelongTo test. I changed that because the second process creation should happen in project 1, where user 2 has access. This ensures that the failure occurs due to a duplicate title, not because the user lacks access to the project. While it worked before, this change clarifies the test's intent.

solth · 2025-01-23T09:10:13Z

However, one could consider introducing a baseline regex in the future—one that enforces basic constraints, such as disallowing whitespace (defined in the application and not changeable).

I think this is a very good idea. This would prevent technically problematic titles while still keeping some flexibility for institutions that want or need slight variations from the default in their process titles!

BartChris · 2025-01-23T09:30:37Z

However, one could consider introducing a baseline regex in the future—one that enforces basic constraints, such as disallowing whitespace (defined in the application and not changeable).

I think this is a very good idea. This would prevent technically problematic titles while still keeping some flexibility for institutions that want or need slight variations from the default in their process titles!

If we go down this road, we should maybe make this configurable in the project(?) settings and remove the value from the Kitodo config, to allow for more flexibility.

henning-gerhardt · 2025-01-23T10:11:46Z

However, one could consider introducing a baseline regex in the future—one that enforces basic constraints, such as disallowing whitespace (defined in the application and not changeable).

I think this is a very good idea. This would prevent technically problematic titles while still keeping some flexibility for institutions that want or need slight variations from the default in their process titles!

If we go down this road, we should maybe make this configurable in the project(?) settings and remove the value from the Kitodo config, to allow for more flexibility.

Read I this correct that this value is moved from a global / general place to a project specific place? How should this value be transferred if you have a lot of projects which need the same value? Did we really need different values for the projects?

BartChris · 2025-01-23T10:36:45Z

Read I this correct that this value is moved from a global / general place to a project specific place? How should this value be transferred if you have a lot of projects which need the same value? Did we really need different values for the projects?

Good point. Maybe a 'layererd' architecture

Global general setting in the application
Instance specific setting in the config.properties
Project specific setting in the project
(4. Maybe even in the tenant (Mandant), to have tenant-specific settings)

henning-gerhardt · 2025-01-23T10:40:51Z

Read I this correct that this value is moved from a global / general place to a project specific place? How should this value be transferred if you have a lot of projects which need the same value? Did we really need different values for the projects?

Good point. Maybe a 'layererd' architecture
1. Global general setting in the **application**

2. **Instance specific** setting in the config.properties

3. **Project specific** setting in the project
   (4. Maybe even in the tenant (Mandant), to have tenant-specific settings)

If we use a layered architecture, I would use

global / instance setting in kitodo_config.properties with an application coded value as fallback (like in many other places already used)
optional tenant / client setting
project specific setting

solth · 2025-01-23T10:43:47Z

Good point. Maybe a 'layererd' architecture

Yes, I just had the same idea. Not sure, if we need all these layers, though - perhaps just a global parameter in kitodo_config.properties that can be overwritten for each project in the projects settings in the GUI would be enough. Not sure if a client wide setting is really required.

Tbh, I also think it is not asked too much to update one string manually in multiple projects, even if an institution has more than just a handful of projects - this is just a simple copy & paste task, after all.

solth · 2025-01-23T10:54:57Z

I agree, the check is useless if it does not check every title of every project (in the same client); i adapted the logic and the tests to ensure that duplicate titles are not allowed on the same client. (No matter if the user has access to the projects or not)

I think we have to keep in mind that in special circumstances a user might see processes of projects that do not belong to one of the clients he is assigned to - specifically when he has the special permission to link new processes to processes of unassigned projects, see #5869 - and if and how a duplicate check would have to take this into account.

BartChris · 2025-01-23T11:04:54Z

I agree, the check is useless if it does not check every title of every project (in the same client); i adapted the logic and the tests to ensure that duplicate titles are not allowed on the same client. (No matter if the user has access to the projects or not)

I think we have to keep in mind that in special circumstances a user might see processes of projects that do not belong to one of the clients he is assigned to - specifically when he has the special permission to link new processes to processes of unassigned projects, see #5869 - and if and how a duplicate check would have to take this into account.

What i implemented right now is that the search for duplicate titles is done across all projects (of the current client/tenant), even in those the user does NOT have access to. In some setups having duplicate titles could have really bad consequences, so it should be prevented. Some user might not even see the process titles in the list, which he or she is not allowed to set, but that's the way it is.

BartChris force-pushed the fix_duplicate_titles branch 2 times, most recently from e8c63dd to f23289c Compare November 22, 2024 12:51

henning-gerhardt reviewed Nov 22, 2024

View reviewed changes

Kitodo/src/main/java/org/kitodo/production/forms/createprocess/CreateProcessForm.java Outdated Show resolved Hide resolved

BartChris force-pushed the fix_duplicate_titles branch from f23289c to 6037595 Compare November 22, 2024 12:55

BartChris commented Nov 22, 2024

View reviewed changes

BartChris added 4 commits December 6, 2024 11:42

Add necessary temp process processing logic to process creation form

4d9c5fc

Add necessary rulesetManagament during test

eeff580

Add new tests to test invalid or duplicate titles; improve test logic

75d4aa0

refactor tests

849dadc

BartChris force-pushed the fix_duplicate_titles branch from a035cde to 849dadc Compare December 6, 2024 10:43

BartChris marked this pull request as ready for review December 6, 2024 11:05

Update kitodo_config.properties

667867d

BartChris force-pushed the fix_duplicate_titles branch from 1e7c3a8 to 667867d Compare December 6, 2024 11:20

solth requested a review from henning-gerhardt December 6, 2024 13:51

henning-gerhardt reviewed Dec 10, 2024

View reviewed changes

BartChris added 2 commits December 10, 2024 18:35

Refactor tests to include review comments

5d6a7d4

Refactor search query to check process title across all projects of t…

d463f2c

…he client

BartChris marked this pull request as draft December 10, 2024 19:04

BartChris marked this pull request as ready for review December 11, 2024 17:05

Add tests for duplicate titles in projects the user does not belong to

b843df8

BartChris force-pushed the fix_duplicate_titles branch from 2c3ce88 to b843df8 Compare December 11, 2024 17:08

extend test

9bd8055

solth requested a review from henning-gerhardt December 11, 2024 18:23

henning-gerhardt reviewed Dec 12, 2024

View reviewed changes

henning-gerhardt approved these changes Dec 12, 2024

View reviewed changes

refactor tests

2e960a2

BartChris commented Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent duplicate and invalid titles #6326

Prevent duplicate and invalid titles #6326

BartChris commented Nov 22, 2024

henning-gerhardt Nov 22, 2024

BartChris Nov 22, 2024

henning-gerhardt Nov 22, 2024

henning-gerhardt Nov 22, 2024

stweil Nov 22, 2024

BartChris Dec 6, 2024 •

edited

Loading

BartChris Nov 22, 2024 •

edited

Loading

BartChris commented Dec 6, 2024 •

edited

Loading

henning-gerhardt left a comment

henning-gerhardt Dec 10, 2024

BartChris Dec 10, 2024

henning-gerhardt Dec 10, 2024

henning-gerhardt commented Dec 10, 2024

BartChris commented Dec 11, 2024 •

edited

Loading

henning-gerhardt Dec 12, 2024

BartChris Dec 12, 2024 •

edited

Loading

henning-gerhardt left a comment

BartChris Dec 12, 2024 •

edited

Loading

solth commented Jan 23, 2025

BartChris commented Jan 23, 2025

henning-gerhardt commented Jan 23, 2025

BartChris commented Jan 23, 2025 •

edited

Loading

henning-gerhardt commented Jan 23, 2025

solth commented Jan 23, 2025 •

edited

Loading

solth commented Jan 23, 2025

BartChris commented Jan 23, 2025 •

edited

Loading

Prevent duplicate and invalid titles #6326

Are you sure you want to change the base?

Prevent duplicate and invalid titles #6326

Conversation

BartChris commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BartChris Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

BartChris Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

BartChris commented Dec 6, 2024 • edited Loading

henning-gerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henning-gerhardt commented Dec 10, 2024

BartChris commented Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

BartChris Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

henning-gerhardt left a comment

Choose a reason for hiding this comment

BartChris Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

solth commented Jan 23, 2025

BartChris commented Jan 23, 2025

henning-gerhardt commented Jan 23, 2025

BartChris commented Jan 23, 2025 • edited Loading

henning-gerhardt commented Jan 23, 2025

solth commented Jan 23, 2025 • edited Loading

solth commented Jan 23, 2025

BartChris commented Jan 23, 2025 • edited Loading

BartChris Dec 6, 2024 •

edited

Loading

BartChris Nov 22, 2024 •

edited

Loading

BartChris commented Dec 6, 2024 •

edited

Loading

BartChris commented Dec 11, 2024 •

edited

Loading

BartChris Dec 12, 2024 •

edited

Loading

BartChris Dec 12, 2024 •

edited

Loading

BartChris commented Jan 23, 2025 •

edited

Loading

solth commented Jan 23, 2025 •

edited

Loading

BartChris commented Jan 23, 2025 •

edited

Loading