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

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

Blocker: Let's please settle on a naming convention for actions before merging. Will require discussion.

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

    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.

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

        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

+        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

    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.

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

+        # 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

            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

        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

+    # `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

        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


-        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

        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


-        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

+        #
+        # 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

+        # 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

            # 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

                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

                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

        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

+        # 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?

mekarpeles · 2026-03-25T16:17:27Z

            "Edit by %s, changeset_id=%s, changes=%s", author, changeset["id"], keys
        )

+        # XXX : is this triggering a save or save_many call?


Either document answer or remove comment as necessary (otherwise others will get confused)

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

Comment thread scripts/copydocs.py Outdated

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

Merged

mekarpeles added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Mar 23, 2026

mekarpeles self-assigned this Mar 23, 2026

jimchamp added the State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] label Mar 23, 2026

mekarpeles reviewed Mar 25, 2026

View reviewed changes

jimchamp marked this pull request as draft March 25, 2026 22:33

jimchamp closed this Mar 31, 2026

jimchamp mentioned this pull request Mar 31, 2026

Update actions where needed #12226

Merged

jimchamp deleted the actions branch April 23, 2026 17:04

		# 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jimchamp commented Mar 9, 2026 •

edited

Loading

jimchamp Mar 9, 2026 •

edited

Loading