Migrate booknotes endpoint to FastAPI#12007
Conversation
github-project-automation
bot
moved this to Waiting Review/Merge from Staff
in Ray's Project
Armansiddiqui9
force-pushed
the
migrate-to-fastapi
branch
from
709411a to
a569bcf
Compare
RayBB
left a comment
RayBB
left a comment
There was a problem hiding this comment.
Overall, this is a great start. There's a few minor things to fix, and please fix them and then request another review, and please be sure to test thoroughly, but overall I think it looks about right!
openlibrary/fastapi/internal/api.py
Outdated
|
|
||
|
|
||
| class BooknoteResponse(BaseModel): | ||
| """Response model for booknote POST endpoint.""" |
openlibrary/fastapi/internal/api.py
Outdated
| except Exception: # noqa: BLE001 | ||
| raise HTTPException( | ||
| status_code=status.HTTP_502_BAD_GATEWAY, | ||
| detail="Error communicating with the database", | ||
| ) |
There was a problem hiding this comment.
We shouldn't need this try except it should be handled automatically by FastAPI.
openlibrary/fastapi/internal/api.py
Outdated
| - If `notes` is provided: create or update the note. | ||
| - If `notes` is omitted: remove the existing note. | ||
| """ | ||
| from openlibrary.core.models import Booknotes |
There was a problem hiding this comment.
This import needs to be at the top of the file.
openlibrary/fastapi/internal/api.py
Outdated
| include_in_schema=SHOW_INTERNAL_IN_SCHEMA, | ||
| ) | ||
| async def booknotes_post( | ||
| work_id: str, |
openlibrary/fastapi/internal/api.py
Outdated
| """ | ||
| from openlibrary.core.models import Booknotes | ||
|
|
||
| numeric_work_id = int(work_id) |
There was a problem hiding this comment.
You don't need to cast it as an int if you make it required as an integer above.
Armansiddiqui9
force-pushed
the
migrate-to-fastapi
branch
from
825a384 to
106fbf4
Compare
for more information, see https://pre-commit.ci
|
@RayBB
|
for more information, see https://pre-commit.ci
RayBB
left a comment
RayBB
left a comment
There was a problem hiding this comment.
Sorry for the long delay in reviewing here. You did excellent work. It was a little tricky to test but ultimately I used http://localhost:18080/docs#/internal/booknotes_post_works_OL_work_id_W_notes_post to test several cases and then added them to the test file.
Really appreciate your work and patience on this one!
github-project-automation
bot
moved this from Waiting Review/Merge from Staff
to Done
in Ray's Project
2 participants
Closes #11999
Migrates
class booknotes(delegate.page)fromopenlibrary/plugins/openlibrary/api.pyto FastAPI.Technical
POST /works/OL{work_id}W/notesinopenlibrary/fastapi/internal/api.pyrequire_authenticated_userdependency→returns 401 instead of legacy browser redirectredirparam (not applicable for a JSON API)BooknoteResponsePydantic modelTesting
docker compose exec web pytest openlibrary/tests/fastapi/test_booknotes.py -vScreenshot
Stakeholders
@RayBB