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

Do not reset the ORDERLABEL if division is a page #6238

Closed
wants to merge 1 commit into from

Conversation

BartChris
Copy link
Collaborator

@BartChris BartChris commented Sep 27, 2024

fixes #5220
alternative: #6244
We should not reset the ORDERLABEL of a page during metadata preservation since this breaks the pagination when the ORDERLABEL is set to excluded.
While this fixes the pagination issue there are other potential problems. I did an alternative pull request which tries to adress them all:
#6244

@BartChris BartChris force-pushed the preserve_page_orderlabel branch from 3c011d4 to 7ece32b Compare September 27, 2024 16:37
@BartChris BartChris marked this pull request as draft September 29, 2024 15:47
@BartChris
Copy link
Collaborator Author

BartChris commented Sep 30, 2024

Although i did not yet notice any problems when not resetting the ORDERLABEL for pages (as done in this PR), we can also extend the original logic. The root cause of the linked issue is, that the necessary logic for setting the LABEL and ORDERLABEL (which are set to NULL in the beginning) is not called for hidden metatdata.

Pair<BiConsumer<Division<?>, String>, String> metsFieldValue = row.getStructureFieldValue();
if (Objects.nonNull(metsFieldValue)) {
metsFieldValue.getKey().accept(division, metsFieldValue.getValue());

To solve the issue at hand we could for hiddenMetadata call the setters directly to not lose the ORDERLABELs and therefor our pagination. We would by this also preserve hidden division LABEL's, which might be lost.

private void processHiddenMetadata() {
        for (Metadata metadatum : hiddenMetadata) {
            if (metadatum instanceof MetadataEntry) {
                MetadataEntry metadataEntry = (MetadataEntry) metadatum;
                String key = metadataEntry.getKey().toUpperCase();
                String value = metadataEntry.getValue();
                switch (key) {
                    case "LABEL":
                        division.setLabel(value);
                        break;
                    case "ORDERLABEL":
                        division.setOrderlabel(value);
                        break;
                    case "CONTENTIDS":
                        division.getContentIds().add(URI.create(value));
                        break;
                }
            }
        }
        metadata.addAll(hiddenMetadata);
    }

A more general remark: The necessary setters for the divisions of (non-hidden) metadata elements in the UI are called as a side effect of the (IMHO very complicated) BiConsumer logic, which does some transformations to the values from the tree, but mainly defines the setters for different division values, which are then applied. There may be a specific reason for using functional interfaces here, but I wonder if there might be opportunities to simplify the logic for better readability and maintainability.

@solth
Copy link
Member

solth commented Oct 1, 2024

@BartChris Please rebase against current master to avoid failing builds caused by broken Selenium tests!

@BartChris BartChris force-pushed the preserve_page_orderlabel branch from 7ece32b to d155ee6 Compare October 1, 2024 07:58
@BartChris BartChris marked this pull request as ready for review October 1, 2024 16:29
@solth solth requested a review from henning-gerhardt October 8, 2024 12:58
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.

I can not review this small change as I do not know how to archive the suggested change. As test code is not changed is showing me that there is not test code or tests are not affected by this change.

@solth solth self-requested a review October 8, 2024 13:05
@BartChris BartChris closed this Jan 22, 2025
@BartChris
Copy link
Collaborator Author

Since #6244 is merged, this is not needed any more.

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.

Setting the ORDERLABEL to excluded breaks the pagination
3 participants