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

Fix: Seed.from_db() now handles client.Thing types #8886

Conversation

scottbarnes
Copy link
Collaborator

@scottbarnes scottbarnes commented Mar 7, 2024

Closes #8882

Fix.

Thanks to @cdrini for very quickly figuring out the problem.

Technical

When there is a cache miss with a list containing a redirect, Seed.from_db() is called with a client.Thing type, but the logic didn't handle this.

Testing

Create a list with a redirected item on it (e.g. by adding an item to the list and merging). Note: you cannot have the non-redirected version of the item before it on the same list or the error won't occur.

Try viewing the profile page of the account with the relevant list before and after applying the PR.

Screenshot

Before:
image

After:
image

Stakeholders

@RayBB
@cdrini

When there is a cache miss, `Seed.from_db()` is called with a
`client.Thing` type, but the logic didn't handle this.

See
internetarchive#8882 (comment).
@scottbarnes scottbarnes force-pushed the fix/8882/Seed-from-db-should-handle-client-things branch from 0d9c872 to d3a5c71 Compare March 7, 2024 23:31
@scottbarnes scottbarnes added the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Mar 7, 2024
elif isinstance(seed, Thing):
# If there is a cache miss, `seed` is a client.Thing.
# See https://github.com/internetarchive/openlibrary/issues/8882#issuecomment-1983844076
elif isinstance(seed, Thing | client.Thing):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow!! Cool TIL this syntax works in python 3.10+, nice!

@cdrini cdrini self-assigned this Mar 8, 2024
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm! Confirmed erroring sporadically on staging, working perfectly on testing! Nice @scottbarnes !

@cdrini cdrini merged commit 848c8f4 into internetarchive:master Mar 8, 2024
3 checks passed
@scottbarnes scottbarnes deleted the fix/8882/Seed-from-db-should-handle-client-things branch March 8, 2024 01:16
@RayBB
Copy link
Collaborator

RayBB commented Mar 8, 2024

Nice, thanks for the fast response :)

@jimchamp jimchamp removed the On testing.openlibrary.org This PR has been deployed to testing.openlibrary.org for testing label Apr 2, 2024
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.

error on user page: error in processing template: ValueError: Invalid seed
4 participants