From f68de6d60acd53c80ab7b4c3072f3b1f668f1cba Mon Sep 17 00:00:00 2001 From: Ben Bolte Date: Fri, 26 Jul 2024 19:57:38 -0700 Subject: [PATCH] image fixes (#193) * image fixes * update frontend * format * 2048 -> 1536 --- frontend/src/components/files/UploadImage.tsx | 9 +- frontend/src/hooks/api.tsx | 25 +- frontend/src/pages/ListingDetails.tsx | 234 +++++++++--------- frontend/src/pages/NewListing.tsx | 18 +- store/app/crud/artifacts.py | 4 +- store/app/crud/base.py | 12 + store/app/crud/listings.py | 12 +- store/app/routers/image.py | 7 +- store/app/routers/listings.py | 20 +- store/settings/configs/local.yaml | 2 +- store/settings/environment.py | 4 +- tests/test_listings.py | 102 ++++++++ tests/test_robots.py | 39 --- 13 files changed, 290 insertions(+), 198 deletions(-) create mode 100644 tests/test_listings.py delete mode 100644 tests/test_robots.py diff --git a/frontend/src/components/files/UploadImage.tsx b/frontend/src/components/files/UploadImage.tsx index 6fc382df..eb8741ea 100644 --- a/frontend/src/components/files/UploadImage.tsx +++ b/frontend/src/components/files/UploadImage.tsx @@ -9,6 +9,9 @@ import { FileWithPath, useDropzone } from "react-dropzone"; import ReactCrop, { type Crop } from "react-image-crop"; import "react-image-crop/dist/ReactCrop.css"; +const MAX_FILE_SIZE = 25 * 1536 * 1536; +const MAX_FILE_MB = MAX_FILE_SIZE / 1024 / 1024; + interface ImageUploadProps { onUploadSuccess: (url: string) => void; } @@ -23,7 +26,6 @@ const ImageUploadComponent: React.FC = ({ const auth = useAuthentication(); const auth_api = new api(auth.api); const { theme } = useTheme(); - const MAX_FILE_SIZE = 25 * 1024 * 1024; const validFileTypes = ["image/png", "image/jpeg", "image/jpg"]; const [showModal, setShowModal] = useState(false); const fileInputRef = useRef(null); @@ -44,9 +46,7 @@ const ImageUploadComponent: React.FC = ({ // Validate file size if (file.size > MAX_FILE_SIZE) { - setFileError( - `File size should not exceed ${MAX_FILE_SIZE / 1024 / 1024} MB`, - ); + setFileError(`File size should not exceed ${MAX_FILE_MB} MB`); setSelectedFile(null); return; } @@ -201,6 +201,7 @@ const ImageUploadComponent: React.FC = ({ <> { console.log(c); setCrop(c); diff --git a/frontend/src/hooks/api.tsx b/frontend/src/hooks/api.tsx index af87c380..8c6a3510 100644 --- a/frontend/src/hooks/api.tsx +++ b/frontend/src/hooks/api.tsx @@ -14,6 +14,13 @@ export interface Listing { description?: string; } +export interface NewListing { + name: string; + description?: string; + artifact_ids: string[]; + child_ids: string[]; +} + interface GithubAuthResponse { api_key: string; } @@ -73,13 +80,13 @@ export class api { public async logout(): Promise { return this.callWrapper(async () => { - await this.api.delete("/users/logout/"); + await this.api.delete("/users/logout"); }); } public async me(): Promise { return this.callWrapper(async () => { - const res = await this.api.get("/users/me/"); + const res = await this.api.get("/users/me"); return res.data; }); } @@ -96,7 +103,7 @@ export class api { searchQuery?: string, ): Promise<[Listing[], boolean]> { return this.callWrapper(async () => { - const response = await this.api.get("/listings/", { + const response = await this.api.get("/listings/search", { params: { page, ...(searchQuery ? { search_query: searchQuery } : {}) }, }); return response.data; @@ -107,7 +114,7 @@ export class api { return this.callWrapper(async () => { const params = new URLSearchParams(); userIds.forEach((id) => params.append("ids", id)); - const response = await this.api.get("/users/batch/", { + const response = await this.api.get("/users/batch", { params, }); const map = new Map(); @@ -120,7 +127,7 @@ export class api { public async getMyListings(page: number): Promise<[Listing[], boolean]> { return this.callWrapper(async () => { - const response = await this.api.get("/listings/me/", { + const response = await this.api.get("/listings/me", { params: { page }, }); return response.data; @@ -141,21 +148,21 @@ export class api { }); } - public async addListing(listing: Listing): Promise { + public async addListing(listing: NewListing): Promise { return this.callWrapper(async () => { - await this.api.post("/listings/add/", listing); + await this.api.post("/listings/add", listing); }); } public async deleteListing(id: string | undefined): Promise { return this.callWrapper(async () => { - await this.api.delete(`/listings/delete/${id}/`); + await this.api.delete(`/listings/delete/${id}`); }); } public async editListing(listing: Listing): Promise { return this.callWrapper(async () => { - await this.api.post(`/listings/edit/${listing.id}/`, listing); + await this.api.post(`/listings/edit/${listing.id}`, listing); }); } diff --git a/frontend/src/pages/ListingDetails.tsx b/frontend/src/pages/ListingDetails.tsx index 3a734187..491227ee 100644 --- a/frontend/src/pages/ListingDetails.tsx +++ b/frontend/src/pages/ListingDetails.tsx @@ -1,4 +1,6 @@ import TCButton from "components/files/TCButton"; +import ImageComponent from "components/files/ViewImage"; +import { humanReadableError } from "constants/backend"; import { useAlertQueue } from "hooks/alerts"; import { api } from "hooks/api"; import { useAuthentication } from "hooks/auth"; @@ -24,46 +26,31 @@ interface ListingDetailsResponse { child_ids: string[]; } -import ImageComponent from "components/files/ViewImage"; - -const ListingDetails = () => { +const RenderListing = ({ + listing, + id, +}: { + listing: ListingDetailsResponse; + id: string; +}) => { const { addAlert } = useAlertQueue(); + const navigate = useNavigate(); + const auth = useAuthentication(); const auth_api = new api(auth.api); + const [userId, setUserId] = useState(null); - const { id } = useParams(); const [show, setShow] = useState(false); const [ownerEmail, setOwnerEmail] = useState(null); - const [part, setListing] = useState(null); const [imageIndex, setArtifactIndex] = useState(0); - const [error, setError] = useState(null); const [showDelete, setShowDelete] = useState(false); const handleClose = () => setShow(false); - const handleShow = () => setShow(true); const handleShowDelete = () => setShowDelete(true); const handleCloseDelete = () => setShowDelete(false); - useEffect(() => { - const fetchListing = async () => { - try { - const partData = await auth_api.getListingById(id); - setListing(partData); - const ownerEmail = await auth_api.getUserById(partData.user_id); - setOwnerEmail(ownerEmail); - } catch (err) { - if (err instanceof Error) { - setError(err.message); - } else { - setError("An unexpected error occurred"); - } - } - }; - fetchListing(); - }, [id]); - - const navigate = useNavigate(); + const { name, user_id, description, artifact_ids } = listing; useEffect(() => { if (auth.isAuthenticated) { @@ -74,56 +61,25 @@ const ListingDetails = () => { }; fetchUserId(); } catch (err) { - if (err instanceof Error) { - setError(err.message); - } else { - setError("An unexpected error occurred"); - } + addAlert(humanReadableError(err), "error"); } } }, [auth.isAuthenticated]); useEffect(() => { - if (error) { - addAlert(error, "error"); - } - }, [error]); - - if (!part) { - return ( - - - - - - - - ); - } - - const response: ListingDetailsResponse = { - name: part.name, - user_id: part.user_id, - description: part.description, - artifact_ids: part.artifact_ids, - child_ids: part.child_ids, - }; - const { name, description, artifact_ids } = response; + const fetchListing = async () => { + try { + const ownerEmail = await auth_api.getUserById(user_id); + setOwnerEmail(ownerEmail); + } catch (err) { + addAlert(humanReadableError(err), "error"); + } + }; + fetchListing(); + }, [user_id]); return ( <> - - navigate("/")}>Home - navigate("/listings/1")}> - Listings - - {name} - - @@ -156,7 +112,7 @@ const ListingDetails = () => { - {part.user_id === userId && ( + {user_id === userId && ( <> @@ -230,54 +186,50 @@ const ListingDetails = () => { )} - - - {artifact_ids && ( - - 1} - > - {artifact_ids.map((id, key) => ( - -
{ - handleShow(); - }} - > - -
- - {"caption"} - - - ))} - - - )} + + 1} + > + {artifact_ids.map((id, key) => ( + +
{ + handleShow(); + }} + > + +
+ + {"caption"} + + + ))} + + { ); }; +const ListingDetails = () => { + const { addAlert } = useAlertQueue(); + const auth = useAuthentication(); + const auth_api = new api(auth.api); + const { id } = useParams(); + const [listing, setListing] = useState(null); + + const navigate = useNavigate(); + + useEffect(() => { + const fetchListing = async () => { + try { + const partData = await auth_api.getListingById(id); + setListing(partData); + } catch (err) { + addAlert(humanReadableError(err), "error"); + } + }; + fetchListing(); + }, [id]); + + return ( + <> + + navigate("/")}>Home + navigate("/listings/1")}> + Listings + + {listing && {listing.name}} + + + {listing && id ? ( + + ) : ( + + + + + + + + )} + + ); +}; + export default ListingDetails; diff --git a/frontend/src/pages/NewListing.tsx b/frontend/src/pages/NewListing.tsx index 18ab84f7..52ca67d4 100644 --- a/frontend/src/pages/NewListing.tsx +++ b/frontend/src/pages/NewListing.tsx @@ -1,5 +1,5 @@ import ListingForm from "components/ListingForm"; -import { api, Artifact, Listing } from "hooks/api"; +import { api, Artifact } from "hooks/api"; import { useAuthentication } from "hooks/auth"; import { useTheme } from "hooks/theme"; import React, { FormEvent, useState } from "react"; @@ -22,16 +22,14 @@ const NewListing: React.FC = () => { setMessage("Please upload at least one image."); return; } - const newFormData: Listing = { - id: "", - name, - description: description, - user_id: "", - artifact_ids: artifacts.map((artifact) => artifact.id), - child_ids, - }; + try { - await auth_api.addListing(newFormData); + await auth_api.addListing({ + name, + description: description, + artifact_ids: artifacts.map((artifact) => artifact.id), + child_ids, + }); setMessage(`Listing added successfully.`); navigate("/listings/me/1"); } catch (error) { diff --git a/store/app/crud/artifacts.py b/store/app/crud/artifacts.py index 32130f03..25485ade 100644 --- a/store/app/crud/artifacts.py +++ b/store/app/crud/artifacts.py @@ -14,8 +14,8 @@ logger = logging.getLogger(__name__) SizeMapping: dict[ArtifactSize, tuple[int, int]] = { - "large": (1024, 1024), - "small": (256, 256), + "large": settings.image.large_size, + "small": settings.image.small_size, } diff --git a/store/app/crud/base.py b/store/app/crud/base.py index d5d13643..f2451294 100644 --- a/store/app/crud/base.py +++ b/store/app/crud/base.py @@ -134,6 +134,18 @@ async def _list( sort_key: Callable[[T], int], search_query: str | None = None, ) -> tuple[list[T], bool]: + """Lists items of a given class. + + Args: + item_class: The class of the items to list. + page: The page number to list. + sort_key: A function that returns the sort key for an item. + search_query: A query string to filter items by. + + Returns: + A tuple of the items on the page and a boolean indicating whether + there are more pages. + """ if search_query: response = await self._list_items( item_class, diff --git a/store/app/crud/listings.py b/store/app/crud/listings.py index a40a30a0..7c1fadd4 100644 --- a/store/app/crud/listings.py +++ b/store/app/crud/listings.py @@ -1,9 +1,10 @@ """Defines CRUD interface for managing listings.""" +import asyncio import logging -from store.app.crud.base import BaseCrud -from store.app.model import Listing, ListingTag +from store.app.crud.base import BaseCrud, ItemNotFoundError +from store.app.model import Artifact, Listing, ListingTag logger = logging.getLogger(__name__) @@ -27,6 +28,13 @@ async def get_user_listings(self, user_id: str, page: int, search_query: str) -> return await self._list_me(Listing, user_id, page, lambda x: 0, search_query) async def add_listing(self, listing: Listing) -> None: + try: + await asyncio.gather( + *[self._get_item(artifact_id, Artifact, throw_if_missing=True) for artifact_id in listing.artifact_ids], + *[self._get_item(child_id, Listing, throw_if_missing=True) for child_id in listing.child_ids], + ) + except ItemNotFoundError: + raise ValueError("One or more artifact or child IDs is invalid") await self._add_item(listing) async def delete_listing(self, listing_id: str) -> None: diff --git a/store/app/routers/image.py b/store/app/routers/image.py index 190c72cf..08578696 100644 --- a/store/app/routers/image.py +++ b/store/app/routers/image.py @@ -13,7 +13,6 @@ from store.app.model import ArtifactSize, User from store.app.routers.users import get_session_user_with_write_permission from store.settings import settings -from store.utils import new_uuid image_router = APIRouter() @@ -47,15 +46,15 @@ async def upload_image( status_code=status.HTTP_400_BAD_REQUEST, detail="Image is too large", ) - image_id = str(new_uuid()) + image = Image.open(file.file) - await crud.upload_image( + artifact = await crud.upload_image( image=image, user_id=user.id, description=description, ) - return UserInfoResponse(image_id=image_id) + return UserInfoResponse(image_id=artifact.id) except Exception as e: raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e)) diff --git a/store/app/routers/listings.py b/store/app/routers/listings.py index f6fedddc..2f60d3fa 100644 --- a/store/app/routers/listings.py +++ b/store/app/routers/listings.py @@ -19,11 +19,6 @@ logger = logging.getLogger(__name__) -@listings_router.get("/{listing_id}") -async def get_listing(listing_id: str, crud: Annotated[Crud, Depends(Crud.get)]) -> Listing | None: - return await crud.get_listing(listing_id) - - class NewListing(BaseModel): name: str child_ids: list[str] @@ -31,7 +26,7 @@ class NewListing(BaseModel): description: str | None -@listings_router.get("/") +@listings_router.get("/search") async def list_listings( crud: Annotated[Crud, Depends(Crud.get)], page: int = Query(description="Page number for pagination"), @@ -40,7 +35,7 @@ async def list_listings( return await crud.get_listings(page, search_query=search_query) -@listings_router.get("/me/") +@listings_router.get("/me") async def list_my_listings( crud: Annotated[Crud, Depends(Crud.get)], user: Annotated[User, Depends(get_session_user_with_read_permission)], @@ -50,7 +45,7 @@ async def list_my_listings( return await crud.get_user_listings(user.id, page, search_query=search_query) -@listings_router.post("/add/") +@listings_router.post("/add") async def add_listing( new_listing: NewListing, user: Annotated[User, Depends(get_session_user_with_write_permission)], @@ -69,7 +64,7 @@ async def add_listing( return True -@listings_router.delete("/delete/{listing_id}/") +@listings_router.delete("/delete/{listing_id}") async def delete_listing( listing_id: str, user: Annotated[User, Depends(get_session_user_with_write_permission)], @@ -84,7 +79,7 @@ async def delete_listing( return True -@listings_router.post("/edit/{id}/") +@listings_router.post("/edit/{id}") async def edit_listing( id: str, listing: dict[str, Any], @@ -99,3 +94,8 @@ async def edit_listing( listing["user_id"] = user.id await crud._update_item(id, Listing, listing) return True + + +@listings_router.get("/{id}") +async def get_listing(id: str, crud: Annotated[Crud, Depends(Crud.get)]) -> Listing | None: + return await crud.get_listing(id) diff --git a/store/settings/configs/local.yaml b/store/settings/configs/local.yaml index 202d6999..1bb6337c 100644 --- a/store/settings/configs/local.yaml +++ b/store/settings/configs/local.yaml @@ -7,4 +7,4 @@ dynamo: table_name: robolist-local site: homepage: http://127.0.0.1:3000 - image_base_url: http://127.0.0.1:8080/images + image_base_url: http://127.0.0.1:4566/images diff --git a/store/settings/environment.py b/store/settings/environment.py index 588d0469..4b54f97a 100644 --- a/store/settings/environment.py +++ b/store/settings/environment.py @@ -39,7 +39,9 @@ class EmailSettings: @dataclass class ImageSettings: - max_bytes: int = field(default=1024 * 1024 * 25) + large_size: tuple[int, int] = field(default=(1536, 1536)) + small_size: tuple[int, int] = field(default=(256, 256)) + max_bytes: int = field(default=1536 * 1536 * 25) quality: int = field(default=80) diff --git a/tests/test_listings.py b/tests/test_listings.py new file mode 100644 index 00000000..dedd2a50 --- /dev/null +++ b/tests/test_listings.py @@ -0,0 +1,102 @@ +"""Runs tests on the robot APIs.""" + +from pathlib import Path + +from fastapi import status +from httpx import AsyncClient +from PIL import Image + +from store.app.db import create_tables + + +async def test_listings(app_client: AsyncClient, tmpdir: Path) -> None: + await create_tables() + + # Register. + response = await app_client.get("/users/github/code/doesnt-matter") + assert response.status_code == status.HTTP_200_OK, response.json() + token = response.json()["api_key"] + auth_headers = {"Authorization": f"Bearer {token}"} + + # Upload an image. + image = Image.new("RGB", (100, 100)) + image_path = Path(tmpdir) / "test.png" + image.save(image_path) + response = await app_client.post( + "/images/upload", + headers=auth_headers, + files={"file": ("test.png", open(image_path, "rb"), "image/png")}, + ) + assert response.status_code == status.HTTP_200_OK, response.json() + assert response.json()["image_id"] is not None + image_id = response.json()["image_id"] + + # Create a listing. + response = await app_client.post( + "/listings/add", + json={ + "name": "test listing", + "description": "test description", + "artifact_ids": [image_id], + "child_ids": [], + }, + headers=auth_headers, + ) + assert response.status_code == status.HTTP_200_OK, response.json() + + # Searches for listings. + response = await app_client.get( + "/listings/search", + params={"search_query": "test", "page": 1}, + headers=auth_headers, + ) + assert response.status_code == status.HTTP_200_OK, response.json() + items, has_more = response.json() + assert len(items) == 1 + assert not has_more + + # Checks the item. + item = items[0] + assert item["name"] == "test listing" + assert item["description"] == "test description" + assert item["artifact_ids"] == [image_id] + assert item["child_ids"] == [] + + # Checks my own listings. + response = await app_client.get( + "/listings/me", + params={"page": 1}, + headers=auth_headers, + ) + assert response.status_code == status.HTTP_200_OK, response.json() + items, has_more = response.json() + assert len(items) == 1 + assert not has_more + + # Gets the listing by ID. + listing_id = item["id"] + response = await app_client.get(f"/listings/{listing_id}", headers=auth_headers) + assert response.status_code == status.HTTP_200_OK, response.json() + assert response.json() == item + + # Edits the listing. + response = await app_client.post( + f"/listings/edit/{listing_id}", + json={"name": "edited name"}, + headers=auth_headers, + ) + assert response.status_code == status.HTTP_200_OK, response.json() + + # Deletes the listing. + response = await app_client.delete(f"/listings/delete/{listing_id}", headers=auth_headers) + assert response.status_code == status.HTTP_200_OK, response.json() + + # Checks that no more listings are available. + response = await app_client.get( + "/listings/me", + params={"page": 1}, + headers=auth_headers, + ) + assert response.status_code == status.HTTP_200_OK, response.json() + items, has_more = response.json() + assert len(items) == 0 diff --git a/tests/test_robots.py b/tests/test_robots.py deleted file mode 100644 index 6077b529..00000000 --- a/tests/test_robots.py +++ /dev/null @@ -1,39 +0,0 @@ -"""Runs tests on the robot APIs.""" - -from httpx import AsyncClient - -from store.app.db import create_tables - - -async def test_robots(app_client: AsyncClient) -> None: - await create_tables() - - # Register. - response = await app_client.get("/users/github/code/doesnt-matter") - assert response.status_code == 200, response.json() - - # Create a part. - response = await app_client.post( - "/listings/add", - json={ - "name": "test part", - "description": "test description", - "images": [ - { - "url": "", - "caption": "", - } - ], - }, - ) - - # Create a robot. - response = await app_client.post( - "/robots/add", - json={ - "name": "test robot", - "description": "test description", - "bom": [], - "images": [{"url": "", "caption": ""}], - }, - )