Improve folder editing form#6993
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 22 |
| Duplication | 12 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
| private Map<String, Folder> getFolderMap() { | ||
| return getFolderList().parallelStream().collect(Collectors.toMap(Folder::getFileGroup, Function.identity())); | ||
| return getFolderList().stream() | ||
| .filter(folder -> StringUtils.isNotBlank(folder.getFileGroup())) |
There was a problem hiding this comment.
This is a change in behavior. I suppose it is safer to only allow folders with a file group as a functional folder. Empty file groups are not forbidden by the UI in general. It might be that they lead to problems downstream.
There was a problem hiding this comment.
This does behave a little strange right now. Folders without a filegroup are not just still possible, but they are also offered as an option for specific usages like "preview" etc:
But selecting such a folder without filegroup and then saving the project does not trigger an error, thus does not inform the user about an incorrect configuration, but rather accepts the setting, but then deactivates the "preview" functionality altogether on its own:
It should either be prevented to set a folder without METS filegroup in the "Folder use" section of the project settings, if we really want to introduce this change, or keep the existing, more liberal behaviour where filegroups are not required for using the contents of a folder for some specific usages.
Since I am not a user I cannot really judge wether this change is going in the right direction, though, so it would be great if @andre-hohmann , @subhhwendt or @henning-gerhardt could comment on this.
There was a problem hiding this comment.
Thanks for looking into it; i will also check in more detail.
There was a problem hiding this comment.
I think it might be better to revert this change and return to the more liberal behaviour as my PR aims at making the existing functionality work, so it should probably not change the existing functionality. It can be discussed seperately whether we want to introduce a behaviour change here.
There was a problem hiding this comment.
I removed this change, which was incomplete as you pointed out.
There was a problem hiding this comment.
That the empty value is allowed (at least on adding new folder entries) must be an error as you can not empty a file group name later (at least in 3.8.4).
Thanks that would simplify things. I would in addition to that consider adding a DB constraint to enforce that filegroups in a project are unique and non NULL.
There was a problem hiding this comment.
I am not sure that is what @henning-gerhardt meant. IIRC leaving file groups empty in the first place when defining a new folder has always been possible. It just wasn't allowed to remove an already existing file group from an already existing folder.
There was a problem hiding this comment.
The problem is, when you want to do ANY change to the folder with the empty file group later, as then it is not even possible to make the file group empty. So changing anything in the folder automatically applies a file group.
There was a problem hiding this comment.
If the possibility for empty filegroups has to be preserved and this file groups has to be selectable as preview folder for example i will probably define some sentinel value (for empty strings) which i use in the UI to make non-selected state differentiable from the selection of an empty string. Because that right now leads to problems as it seems.
I am not sure if using the empty filegroup as preview leads to downstream problems later.
There was a problem hiding this comment.
The problem is, when you want to do ANY change to the folder with the empty file group later, as then it is not even possible to make the file group empty. So changing anything in the folder automatically applies a file group.
OK, in 3.9 this works, so i suppose then this became stricter. Selecting the empty filegroup as preview also leads to some UI-problems in 3.9 though, but appearantly works. I will try what i can to preserve the 3.9 behavior, as backward compatibility is probably critical here.
BartChris
force-pushed
the
folder_editing_stream
branch
from
efaacc8 to
43d7dfc
Compare
BartChris
changed the title
BartChris
force-pushed
the
folder_editing_stream
branch
from
b2c926e to
6f81d80
Compare
|
I further refined this to address the edge case where a folder is deleted and a new one with the same fileGroup is added in the same edit session and afterwards assigned to a usage group. This can create duplicates. |
BartChris
force-pushed
the
folder_editing_stream
branch
2 times, most recently
from
cca5657 to
7d584a5
Compare
BartChris
force-pushed
the
folder_editing_stream
branch
from
7d584a5 to
6e623b8
Compare
BartChris
force-pushed
the
folder_editing_stream
branch
from
6e623b8 to
c38f547
Compare
|
Still some buggy behavior with the file groups... will check again.. |
BartChris
force-pushed
the
folder_editing_stream
branch
3 times, most recently
from
55c2eb9 to
e0cc327
Compare
|
@solth Maybe you can also test again to see if it still possible to break the form. |
BartChris
force-pushed
the
folder_editing_stream
branch
from
c75796a to
e915170
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
BartChris
force-pushed
the
folder_editing_stream
branch
from
50cf7d2 to
6be0df9
Compare
BartChris
force-pushed
the
folder_editing_stream
branch
from
6be0df9 to
bb5f8ec
Compare
BartChris
force-pushed
the
folder_editing_stream
branch
from
c065d61 to
ecbd62c
Compare
matthias-ronge
left a comment
matthias-ronge
left a comment
There was a problem hiding this comment.
I think I understand why it works, and the code looks correct.
solth
left a comment
solth
left a comment
There was a problem hiding this comment.
Works for me as well! Code looks good, too. I do think retaining the current behaviour was the right decision. Thanks for the changes, @BartChris !
5 participants
This PR improves upon #6946.
When i edit folders and in the same UI interaction session also edit the folder usage (Which folder is used to show thumbnails etc.) sometimes the screen gets stuck on Save. It seems to me as if using "parallel stream" is not safe here and has unwanted effects. After my changes i can edit both the general folder settings as well as their usage.
I also improve the file group handling insofar as when a filegroup of an existing folder is changed to the filegroup of another folder and an error indicates a duplicate file group we do not switch to the rejected file group in the UI.