close
Skip to content

fix(add_book): use _data.pop to strip infobase metadata from matched author - #13032

Merged
mekarpeles merged 1 commit into
internetarchive:masterfrom
TechGenius-Karan:13016/fix/author-metadata-cleanup-loop
Jul 6, 2026
Merged

fix(add_book): use _data.pop to strip infobase metadata from matched author#13032
mekarpeles merged 1 commit into
internetarchive:masterfrom
TechGenius-Karan:13016/fix/author-metadata-cleanup-loop

Conversation

@TechGenius-Karan

Copy link
Copy Markdown
Contributor

Closes #13016

**Fix**

The metadata cleanup loop in author_import_record_to_author() was broken in two ways:

  1. existing.k accessed a literal attribute named "k" (always Nothing/falsy) instead of the loop variable k,
    so the condition was never True.
  2. Even with the condition corrected, del existing[k] raises AttributeError because Infogami Thing objects
    have no __delitem__.

Fix: use existing._data.pop(k, None) to remove the metadata keys (last_modified, id, revision, created)
directly from the underlying data dict, which is the only supported mutation path on a Thing.

Technical

  • Thing.__setitem__ delegates to self._getdata()[key], but there is no __delitem__ — mutation must go through
    _data directly.
  • The mock_site.save() helper automatically appends revision, last_modified, and created to every saved
    document, so tests that return a matched author exercised this code path in CI.
  • No behaviour changes to the matching logic itself — only the post-match metadata strip is fixed.

Testing

  1. Run the affected test file:
    pytest openlibrary/catalog/add_book/tests/test_load_book.py -v
    All 35 tests should pass, including the new test_infobase_metadata_keys_stripped_from_existing_author which
    directly verifies that last_modified, revision, and created are absent from the returned author when a match
    is found.

  2. Before this fix, 7 tests in that file failed with AttributeError: __delitem__ whenever find_entity() returned
    a match. All now pass.

Screenshot

N/A — no UI changes.

Stakeholders

@openlibrary-bot

Copy link
Copy Markdown
Collaborator

Thank you @TechGenius-Karan for this PR!

🤖 Copilot has been assigned for an initial review.

The linked issue hasn't been triaged yet — triage happens on Mondays and Fridays. There are currently 99 open non-draft PRs in the review queue.

PR triage checklist (maintainers)
  • PR description — not empty; explains what the change does and how to verify it
  • References an issue — PR body contains Closes #13016
    • Linked issue is triaged — has a Priority: * label (not just Needs: Triage)
    • Linked issue is assigned — has at least one assignee
  • Commit history clean — no WIP/fixup/conflict noise; commit messages are meaningful
  • CI passing — pre-commit.ci still running at time of comment
  • Test cases present — 22 lines of new tests added in test_load_book.py
  • Proof of testing — PR body describes pytest openlibrary/catalog/add_book/tests/test_load_book.py and expected outcomes ✓

Note

This comment was automatically generated by PAM, Open Library's Project AI Manager. PAM provides status visibility, performs basic project management functions, and gives actionable feedback so contributors aren't left waiting.

@github-actions github-actions Bot added the Needs: Response Issues which require feedback from lead label Jun 25, 2026
@mekarpeles mekarpeles removed the Needs: Response Issues which require feedback from lead label Jul 4, 2026
@mekarpeles
mekarpeles merged commit 1b077ac into internetarchive:master Jul 6, 2026
4 checks passed
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.

bug(add_book): metadata cleanup loop uses literal attribute 'k' instead of variable k

3 participants