Fix: deduplicate subjects on works and list items on editions #8663
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR de-duplicates, using `casefold()`, the `subjects` field on `Work` items, and field values of list items that are added to `Edition` items on import via `load()`. This does not affect edits made via the UI as they go through `process_work()` from `openlibrary/plugins/upstream/addbook.py`, which de-duplicates via a slightly differest strategy. Rather than merging two lists (existing subjects on a matched item, new ones from an import item), it takes form data with every subject and dedupes that list. Though I had hoped to unify the de-duping logic, I think that is beyond the scope of this particular issue. For more, see: internetarchive#8661
4c81801
to
fa1845e
Compare
tfmorris
reviewed
Dec 22, 2023
| def get_casefold_sort(item_list: list[str]): | ||
| return sorted([item.casefold() for item in item_list]) | ||
|
|
||
| expected = ['granite', 'Straße', 'ΠΑΡΆΔΕΙΣΟΣ', 'sandstone'] |
There was a problem hiding this comment.
Isn't "Granite" the preferred capitalization here?
There was a problem hiding this comment.
Trying to determine which casing is better would be non-trivial ; currently it's just choosing to use the first item in the list. That's good enough for now. Things like making subjects consistent (eg preferring the title case variant) might also be best handled at another layer.
cdrini
reviewed
Dec 22, 2023
cdrini
reviewed
Dec 27, 2023
cdrini
reviewed
Dec 27, 2023
cdrini
approved these changes
Dec 27, 2023
- fix comparison - remove pointless sorting - ensure tests catch case where 1 existing duplicate is removed and 1 new item is added, resulting in a final list the same length as the original.
2815144
to
8b9bffb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #8661.
This PR de-duplicates, using
casefold(), thesubjectsfield onWorkitems, and field values of list items that are added toEditionitems on re-import viaload().This does not affect edits made via the UI as they go through
DocSaveHelper()andprocess_work()fromopenlibrary/plugins/upstream/addbook.py, which de-duplicates via a slightly differest strategy. Rather than merging two lists (existing subjects on a matched item, and new ones from an import item), it takes form data with every subject as a CSV and dedupes that string.Though I had hoped to unify the de-duping logic, I think that is beyond the scope of this particular issue.
Technical
I added a slight bit of logic to the fields that looked as if they might possibly result in duplicates because of a lack of case-insensitive matching.
Testing
The unit tests should cover this, but the test is whether it's possible, via
load(), to add a duplicated subject or value in a list field on reimport after accounting for Python'sstring.casefold()method. That is to say, for the purposes of this PR, if"Straße"exists as a subject on aWork,"strasse"should not be added viaload().NOTE: Because the web UI (e.g. the
WorkandEditionedit pages) usesprocess_work(), which currently de-duplicates viastring.lower(), this PR changes the behavior there to usestring.casefold()so it matches theload()de-duping.Screenshot
Stakeholders