Ensure `action` is always set when records are saved by jimchamp · Pull Request #12058 · internetarchive/openlibrary

jimchamp · 2026-03-09T19:00:50Z

Addresses #11955

Updates all save, save_many, and Thing._save calls to include a valid and accurate action.

We may also want to update the default save and save_many values in Infogami. This PR does that. When merged, we can check the /recentchanges views for the default types to determine if any transaction needs an action.

Technical

Testing

Screenshot

Stakeholders

for more information, see https://pre-commit.ci

Copilot

Pull request overview

This PR standardizes write operations across the codebase by ensuring action is supplied to save, save_many, and Thing._save calls, improving changeset classification/auditing.

Changes:

Added explicit action= values to many save/save_many/_save call sites (scripts + plugins).
Updated User.set_data to require an action and forward it to _save.
Refined list and import-related actions to be more specific (though some mappings/consumers may need updating).

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
scripts/update_stale_ocaid_references.py	Adds an explicit bulk redaction action to `save_many`.
scripts/copydocs.py	Adds an action to batch transfers (but currently breaks non-OL `dest` implementations).
openlibrary/plugins/upstream/covers.py	Adds actions for updating covers/photos via `_save`.
openlibrary/plugins/upstream/code.py	Adds an explicit action for revert saves.
openlibrary/plugins/upstream/addtag.py	Adds explicit tag delete/update actions.
openlibrary/plugins/upstream/addbook.py	Adds actions to several author-related saves; introduces a placeholder delete action needing finalization.
openlibrary/plugins/openlibrary/lists.py	Replaces generic `lists` action with more specific list/series actions.
openlibrary/plugins/openlibrary/code.py	Adds action for sampleload, add-author, and YAML updates (one formatting issue present).
openlibrary/plugins/openlibrary/bulk_tag.py	Adds an explicit bulk tagging action.
openlibrary/plugins/openlibrary/api.py	Adds actions for bulk delete and edition updates.
openlibrary/plugins/admin/code.py	Adds actions for spam reverts / permission updates; leaves one `_save()` without action in stats POST.
openlibrary/olbase/events.py	Adds a clarifying comment around subevent triggering.
openlibrary/core/models.py	Requires `action` for `User.set_data` and adds actions for usergroup member changes + tag creation.
openlibrary/catalog/add_book/init.py	Changes import save_many action selection (currently acknowledged as inaccurate and may affect changeset classification).
openlibrary/accounts/model.py	Updates anonymization to pass an action into `set_data`.

Comments suppressed due to low confidence (1)

openlibrary/plugins/openlibrary/lists.py:742

action="update-list-items" is a new changeset kind for list edits; currently ListChangeset is only registered for kinds lists and series. If you keep this more granular action naming, you’ll likely want to register update-list-items (and delete-list, etc.) to ListChangeset so list change rendering/logic continues to work as before.

        return lst._save(comment="Updated list.", action="update-list-items", data=changeset_data)


class list_seed_yaml(list_seeds):
    encoding = "yml"

Copilot · 2026-03-09T19:05:45Z

scripts/copydocs.py

    for group in web.group(docs, 50):
        try:
-            print(dest.save_many(group, comment=comment))
+            print(dest.save_many(group, comment=comment, action="copydocs-transfer"))


copy() supports writing to a Disk destination (and tests use FakeServer), but those save_many implementations only accept (docs, comment=...). Passing action= here will raise TypeError. Consider either updating Disk.save_many/test doubles to accept an optional action (or **kwargs) and ignore it, or only passing action when dest is an OpenLibrary instance.

openlibrary/plugins/openlibrary/code.py

Copilot · 2026-03-09T19:05:46Z

openlibrary/plugins/upstream/addbook.py

        doc = web.ctx.site.new(key, {"key": key, "type": {"key": "/type/delete"}})
-        doc._save(comment=comment)
+        # TODO : pull type from key, then compose `delete-{type}` action string
+        doc._save(comment=comment, action="delete-?")



action="delete-?" looks like a placeholder and will leak into production changesets/recentchanges. Please replace with a real, stable action value (e.g. based on the doc type/key) before merging.

Copilot · 2026-03-09T19:05:46Z

openlibrary/catalog/add_book/__init__.py

+        # This is still wrong -- the `edits` list can contain works, authors,
+        # and editions.  Some records may be new, and some may be existing.
+        # XXX : bulk-*
+        action = 'edit-edition' if existing_edition else 'add-edition'
+        web.ctx.site.save_many(edits, comment=comment, action=action)


This batch save can include works/authors/editions and both creates and updates, but the new action is derived only from existing_edition and is acknowledged as “still wrong” in the comment. Also, this changes the changeset kind away from the existing add-book value which is registered to AddBookChangeset (used for rendering/behavior). Consider keeping action="add-book" (or introducing a new bulk/import action and registering it) so changesets remain correctly classified.

Copilot · 2026-03-09T19:05:46Z

openlibrary/plugins/openlibrary/lists.py

+        action = "add-series" if list_type_plural == "series" else "add-list"
        web.ctx.site.save_many(
            records_to_save,
-            action="lists",
+            action=action,
            comment=web.input(_comment="")._comment or None,
        )


These new list/series action values (add-list/add-series) won’t match the currently registered changeset kinds for lists (lists and series in openlibrary/core/lists/model.py). That likely means list-related changesets will no longer be parsed/rendered as ListChangeset. Either keep the existing action values, or update the changeset registration to include the new action strings.

It's unclear to me if this will affect anything, aside from the /recentchanges/lists page(which will not load for me).

When this change is applied, one will need to visit /recentchanges/add-list to see newly added lists.

Copilot · 2026-03-09T19:05:47Z

openlibrary/plugins/admin/code.py

    def POST(self, today):
        """Update stats for today."""
        doc = self.get_stats(today)
+        # This seems like it would save a Thing
+        # ...but, I don't know if this endpoint is in use today.
+        # I suspect not....
        doc._save()
        raise web.seeother(web.ctx.path)


This endpoint still calls doc._save() without an action, which conflicts with the PR goal of ensuring saves always provide an action for audit/recentchanges classification. Please pass an explicit action here (even if the endpoint is rarely used) or remove the save if it’s truly obsolete.

scripts/copydocs.py

jimchamp · 2026-03-09T20:37:46Z

openlibrary/plugins/upstream/covers.py

+        # Must check `book.key` for record type, if needed
+        book._save('Update covers', action="update-cover")


Suggested change

# Must check `book.key` for record type, if needed

book._save('Update covers', action="update-cover")

book._save('Update covers', action="add-cover")

This one is tricky, as (I believe) covers can be added and removed by this handler.

How would these render in /recentchanges/add-cover if a cover were removed? Would it break the page?

jimchamp · 2026-03-09T20:39:37Z

openlibrary/plugins/upstream/addbook.py

            raise web.redirect(web.ctx.path)
        edition.set_identifiers(data)
        saveutil.save(edition)
+        # `action` : edit-book-isbn?


Suggested change

# `action` : edit-book-isbn?

jimchamp · 2026-03-09T20:41:50Z

openlibrary/plugins/upstream/addbook.py

        saveutil.save(edition)

        comment = utils.get_message("comment_add_book")
+        # XXX : creates both work and edition


Suggested change

# XXX : creates both work and edition

Using book as the record type is probably fine for cases where either a work or edition record is being updated.

jimchamp · 2026-03-09T20:43:12Z

openlibrary/plugins/upstream/addbook.py

+    # `action` is always being passed to `commit` today, but maybe not tomorrow?
+    # Should `action` be a positional argument for this method?


Suggested change

# `action` is always being passed to `commit` today, but maybe not tomorrow?

# Should `action` be a positional argument for this method?

Reviewers: Should action be a required positional parameter for this commit method?

jimchamp · 2026-03-09T20:43:46Z

openlibrary/plugins/upstream/addbook.py

        self.docs = []

-    def save(self, doc) -> None:
+    def save(self, doc) -> None:  # total misnomer -- nothing is saved in the method


Suggested change

def save(self, doc) -> None: # total misnomer -- nothing is saved in the method

def save(self, doc) -> None:

Out-of-scope for this PR.

jimchamp · 2026-03-09T21:17:49Z

openlibrary/plugins/openlibrary/bulk_tag.py


-        web.ctx.site.save_many(docs_to_update, comment="Bulk tagging works")
+        web.ctx.site.save_many(
+            docs_to_update, comment="Bulk tagging works", action="bulk-update-work-tags"


Maybe bulk-update-tags is better here?

jimchamp · 2026-03-09T21:19:14Z

openlibrary/plugins/openlibrary/api.py

        if not data["source_records"]:
            del data["source_records"]
-        web.ctx.site.save(data, "Remove OCAID: Item no longer available to borrow.")
+        web.ctx.site.save(data, "Remove OCAID: Item no longer available to borrow.", action="update-edition")


Suggested change

web.ctx.site.save(data, "Remove OCAID: Item no longer available to borrow.", action="update-edition")

web.ctx.site.save(data, "Remove OCAID: Item no longer available to borrow.", action="update-book")

Generalize Edition and Work records as book.

jimchamp · 2026-03-09T21:19:41Z

openlibrary/plugins/openlibrary/api.py


-        web.ctx.site.save_many(delete_payload, comment)
+        # this deletes editions and works
+        web.ctx.site.save_many(delete_payload, comment, action="bulk-delete-book-records")


Suggested change

web.ctx.site.save_many(delete_payload, comment, action="bulk-delete-book-records")

web.ctx.site.save_many(delete_payload, comment, action="bulk-delete-books")

jimchamp · 2026-03-09T21:22:00Z

openlibrary/plugins/admin/code.py

+        #
+        # I suspect that we'd want to filter these out from the
+        # `/recentchanges` page.
+        page._save("updated blocked IPs", action="block-ips")


Maybe update-blocked-ips is better here? Either way, this probably shouldn't be publicly accessible via /recentchanges, which will require other changes.

jimchamp · 2026-03-09T21:24:57Z

openlibrary/catalog/add_book/__init__.py

+        # This is still wrong -- the `edits` list can contain works, authors,
+        # and editions.  Some records may be new, and some may be existing.
+        # XXX : bulk-*


Suggested change

# This is still wrong -- the `edits` list can contain works, authors,

# and editions. Some records may be new, and some may be existing.

# XXX : bulk-*

Reviewers: what should be done for this case?

jimchamp · 2026-03-09T21:25:42Z

openlibrary/accounts/model.py

            # Clear patron's profile page:
            data = {'key': patron.key, 'type': '/type/delete'}
-            patron.set_data(data)
+            patron.set_data(data, "delete-profile")


We'll likely want to filter these from /recentchanges.

jimchamp · 2026-03-09T21:28:14Z

scripts/update_stale_ocaid_references.py

                web.ctx.ip = web.ctx.ip or '127.0.0.1'
-                web.ctx.site.save_many(updated_eds, comment="Redacting ocaids")
+                web.ctx.site.save_many(
+                    updated_eds, comment="Redacting ocaids", action="bulk-redact-ocaid"


Suggested change

updated_eds, comment="Redacting ocaids", action="bulk-redact-ocaid"

updated_eds, comment="Redacting ocaids", action="bulk-edit-books"

I don't know if a bulk-redact-ocaid action is helpful at all. If it is, we'll probably want to exclude these from /recentchanges views.

jimchamp · 2026-03-09T21:36:19Z

openlibrary/plugins/upstream/code.py

                else:
-                    prev._save("revert to revision %d" % prev.revision)
+                    prev._save(
+                        "revert to revision %d" % prev.revision, action="revert-version"


Do we want to exclude these changes from /recentchanges views?

jimchamp · 2026-03-09T21:39:20Z

openlibrary/plugins/upstream/addbook.py

        edition.set_identifiers(data)
        saveutil.save(edition)
+        # `action` : edit-book-isbn?
        saveutil.commit(comment="Added an %s identifier." % typ, action="edit-book")


Maybe update-book is more memorable?

...we shouldn't change this, though...

Should we conform to an edit-{record} pattern when naming actions (e.g. edit-tag or edit-author)? I've been prefixing update- to other such actions, but edit- has precedence.

jimchamp · 2026-03-09T21:44:42Z

openlibrary/plugins/upstream/addbook.py

+        # TODO : pull type from key, then compose `delete-{type}` action string
+        doc._save(comment=comment, action="delete-?")


Reviewers: What should be done for this case? Do we want the actual type that's being deleted, or would delete-record suffice?

jimchamp added 2 commits March 5, 2026 11:41

Add actions where needed

8f2b04b

Merge branch 'master' into actions

683ebab

Copilot AI review requested due to automatic review settings March 9, 2026 19:00

Copilot started reviewing on behalf of jimchamp March 9, 2026 19:01 View session

[pre-commit.ci] auto fixes from pre-commit.com hooks

f4fd712

for more information, see https://pre-commit.ci

Copilot AI reviewed Mar 9, 2026

View reviewed changes

jimchamp commented Mar 9, 2026

View reviewed changes

scripts/copydocs.py Outdated Show resolved Hide resolved

jimchamp commented Mar 9, 2026

View reviewed changes

Remove action from copydocs save_many

42d3424

jimchamp mentioned this pull request Mar 17, 2026

Update default transaction types internetarchive/infogami#275

Open

		# Must check `book.key` for record type, if needed
		book._save('Update covers', action="update-cover")

	# Must check `book.key` for record type, if needed
	book._save('Update covers', action="update-cover")
	book._save('Update covers', action="add-cover")

		# `action` is always being passed to `commit` today, but maybe not tomorrow?
		# Should `action` be a positional argument for this method?

	def save(self, doc) -> None: # total misnomer -- nothing is saved in the method
	def save(self, doc) -> None:

	web.ctx.site.save(data, "Remove OCAID: Item no longer available to borrow.", action="update-edition")
	web.ctx.site.save(data, "Remove OCAID: Item no longer available to borrow.", action="update-book")

	web.ctx.site.save_many(delete_payload, comment, action="bulk-delete-book-records")
	web.ctx.site.save_many(delete_payload, comment, action="bulk-delete-books")

	# This is still wrong -- the `edits` list can contain works, authors,
	# and editions. Some records may be new, and some may be existing.
	# XXX : bulk-*

	updated_eds, comment="Redacting ocaids", action="bulk-redact-ocaid"
	updated_eds, comment="Redacting ocaids", action="bulk-edit-books"

		# TODO : pull type from key, then compose `delete-{type}` action string
		doc._save(comment=comment, action="delete-?")

Uh oh!

Conversation

jimchamp commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Technical

Testing

Screenshot

Stakeholders

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jimchamp Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jimchamp commented Mar 9, 2026 •

edited

Loading

jimchamp Mar 9, 2026 •

edited

Loading