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

Rename directories when changing process title #5620

Merged
merged 9 commits into from
May 2, 2023

Conversation

solth
Copy link
Member

@solth solth commented Apr 6, 2023

Fixes #5597

Copy link
Collaborator

@henning-gerhardt henning-gerhardt left a comment

Choose a reason for hiding this comment

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

Changes look good and did work for entries with and without a processtitle inside.

Maybe adding a check to compare old and new directory name and do only the renaming if they differ.

@solth
Copy link
Member Author

solth commented Apr 11, 2023

Maybe adding a check to compare old and new directory name and do only the renaming if they differ.

Thanks for the review. There already is a similar check in the ProcessForm when saving a process that compares old and new process title and only triggers renaming actions if the titles differ:

if (!process.getTitle().equals(newProcessTitle)

Ideally this would be checked inside the renaming function itself to cover all use cases but thanks to the check above the renaming function is already skipped when manually saving a single process without changing the title. Should I change this PR to adjust the code accordingly or leave that change to a separate PR?

@henning-gerhardt
Copy link
Collaborator

As the renaming functionality of the FileManagement class from the Kitodo-Filemanagement module for files or directories could be called from everywhere I would add even there a check and doing the renaming of files or directories only if they differ. I think that this additional check should be done in the pull request as the comparing should be simple and easy. If this is more complicated than move it to a separate pull request.

@solth
Copy link
Member Author

solth commented Apr 11, 2023

@henning-gerhardt I added a check to skip renaming a directory if the new name equals the old name and a corresponding test. Since the behaviour of renaming files is a little different and does not relate to the changes in this pull request I have not added a similar check, there.

@solth solth requested a review from henning-gerhardt April 11, 2023 09:57
@solth solth requested a review from henning-gerhardt April 25, 2023 08:17
@solth solth requested a review from matthias-ronge April 25, 2023 11:14
@solth solth requested a review from matthias-ronge April 28, 2023 11:13
@solth solth requested a review from henning-gerhardt April 28, 2023 16:39
Copy link
Collaborator

@henning-gerhardt henning-gerhardt left a comment

Choose a reason for hiding this comment

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

Hopefully last needed change.

@solth solth force-pushed the rename-directory branch from fe6ac4b to 9dd076d Compare May 2, 2023 09:31
@solth solth merged commit bbae957 into kitodo:master May 2, 2023
@solth solth deleted the rename-directory branch May 2, 2023 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants