Lau/listing view by LautaroJBeck · Pull Request #58 · pennlabs/penn-marketplace

LautaroJBeck · 2026-03-01T18:21:59Z

No description provided.

i30101 · 2026-03-22T17:29:06Z

+      ? resolveConditionValue(listingState.additional_data.condition)
+      : ""
+  );
+  const [draftExpiresAt, setDraftExpiresAt] = useState(


unused variables

i30101 · 2026-03-22T17:57:17Z

+                    <div className="space-y-2">
+                      <label className="text-sm font-medium text-gray-700">Title</label>
+                      <Input value={draftTitle} onChange={(e) => setDraftTitle(e.target.value)} />
+                    </div>


Input component's borders are partly hidden on the left, fix styling

jamesdoh0109

Sorry for the delay on the review. I left a bunch of comments and I encourage you to go through each of them carefully and address them. Generally, get more familiarized with our codebase and what frameworks/libraries we use, and also the coding convention and pattern we use. Try to leverage these when you write code. You should have good context about the codebase you are working in. Good code isn't always necessarily the cleanest, most elegant code, but rather one that follows the convention set in the codebase.

Very likely, there will prob be more comments after you address the current ones. But i encourage you to take this PR as an opportunity to set yourself a better foundation - both in understanding the patterns we've established and building the habit of reading existing code before writing new code

jamesdoh0109 · 2026-03-01T18:35:39Z

+  const offersResponse = isOwner ? await getOffersReceived() : null;
+  const offers = offersResponse?.results?.filter((offer) => offer.listing === item.id) ?? [];


addressed offline, but see if you can fetch offers just for this item, not all offers and filter by id

jamesdoh0109 · 2026-03-01T18:35:47Z

+  const offersResponse = await (isOwner ? getOffersReceived() : getOffersMade());
+  const offers = offersResponse?.results?.filter((offer) => offer.listing === sublet.id) ?? [];


Same comment here

jamesdoh0109 · 2026-03-01T18:42:47Z

+  rejected: { label: "Rejected", className: "bg-red-100 text-red-700" },
+};
+
+const OfferCard = ({


Pull this out into a separte component in its own file

jamesdoh0109 · 2026-03-01T18:45:27Z

+  onStatusChange,
+}: {
+  offer: Offer;
+  offersMode: "received" | "made";


There seems to be no reason to have this prop. You only check for offersMode == "received" in this, so this component should only be rendered when offersMode is "received". Honestly, isOwner should be able to determien this

jamesdoh0109 · 2026-03-01T19:03:59Z

+    # Offer accept / reject
+    path("offers/<int:offer_id>/accept/", accept_offer, name="offer-accept"),
+    path("offers/<int:offer_id>/reject/", reject_offer, name="offer-reject"),


addressed offline, but this is not consistent w REST API convention. Instead of having verbs like "accept" and "reject", you can just directly edit the offer object (i.e. PATCH /offers), and then just modify the offer's status

jamesdoh0109 · 2026-03-27T15:57:13Z

+  );
+};
+
+const OffersSection = ({


Can also be its own component file. Put all the offer related components under listings/offer

jamesdoh0109 · 2026-03-27T16:00:11Z

            listingOwnerLabel={listingOwnerLabel}
+            canEdit={canEdit}
          />
+          {offersMode === "received" && <OffersSection offers={offers} offersMode={offersMode} />}


What if i made an offer for some item? Do i still want to see "Make Offer" button? I think instead, its better to show the offer i made for the item and then show its status as a read only badge

jamesdoh0109 · 2026-03-27T16:09:18Z

+  offersMode,
+  canEdit,
+}: Props) => {
+  const [listingState, setListingState] = useState(listing);


Right now, the initial listing gets copied into useState(listing) and then whenever you update/edit a listing, this local state gets updated and you just show this value in the UI. This is a really bad approach because the local state can drift from the server and it duplicates what tanstack query already does. Also, if the update req to backend ever fails, setListingState never runs but the user gets no feedback and we just silently swallow them with console.log(error). And lastly, you should see how much complexity you added to your code by introducting this state.

Generally, you should get familiar with tanstack query (useQuery and useMutation) and how we use it throughout our codebase. In this situation, replace the useState(listing) wiuth useQuery call seeded w the initial data from the server componentn prop. ANd then you can convert handleEditSave and handleDeleteConfirm to useMutation calls that invalidate the query on success rather than manually resetting local state. You already did a similar thing with toggleFavoriteMutation - follow this convention

jamesdoh0109 · 2026-03-27T16:10:52Z

+
  const toggleFavoriteMutation = useMutation({
-    meta: { suppressErrorToast: true }, // since it's noisy to show error toast on top of optimistic update
+    meta: { suppressErrorToast: true },


why are you getting rid of this comment

can you revert back the comment plz

jamesdoh0109 · 2026-03-27T16:14:01Z

+            canEdit={canEdit}
          />
+          {offersMode === "received" && <OffersSection offers={offers} offersMode={offersMode} />}
+          {canEdit && (


I would encourage you to put these into ListingActions. There, you can parse isOwner and then either display these edit/delete buttons or make offer button. This is a much better way to componentize

The edit form right now has no validation. In our code base, we use react hook form and zod schemas but looks like you are again using raw useState. Please look at how we handle these in other forms. You might also be able to leverage existing form components instead of defining a brand new form

LautaroJBeck · 2026-03-29T18:00:28Z

@@ -0,0 +1,109 @@
+from decimal import Decimal


This file is a script to artificially add offers to one listing

Ok maybe make a comment in the file. Also not sure seed_offer is the best name

jamesdoh0109 · 2026-04-03T19:22:50Z

Do not commit this - all pycache should be in .gitignore

jamesdoh0109 · 2026-04-03T19:23:03Z

Do not commit this - all pycache should be in .gitignore

jamesdoh0109 · 2026-04-03T19:50:05Z

        self.check_object_permissions(self.request, obj)
        self.perform_destroy(obj)
-        return Response(status=status.HTTP_204_NO_CONTENT)
+        # Keep a JSON body so the frontend `serverFetch` can safely parse.


I think this comment is not necessary imo

jamesdoh0109 · 2026-04-03T20:03:00Z

+        name="offers-mine",
+    ),
+    # Update offer status (PATCH)
+    path("offers/<int:offer_id>/", change_offer_status, name="offer-status"),


Make it clear that this is for modifying status by adding /status at the end (like how you did for details)

jamesdoh0109 · 2026-04-03T20:23:32Z

+    offer = Offer.objects.filter(listing_id=listing_id_int, user=request.user).first()
+    if offer is None:
+        raise exceptions.NotFound("No offer for this listing")
+    return Response(OfferSerializer(offer).data)


The new FBVs (change_offer_status, get_my_offer_for_listing, change_offer_details) break from the codebase's convention of using class-based views for resource operations. The existing pattern is:

Class-based views (ModelViewSet, ListAPIView, UpdateAPIView, etc.) for resource CRUD (Listings, Offers, Favorites, Tags)

Function-based views (@api_view) for one-off actions (send_verification_code, get_current_user, etc.)

Using FBVs here also forces manual object-level permission checks (since DRF's @permission_classes decorator only runs has_permission, not has_object_permission), which is why the permission logic is inconsistent across the three views — change_offer_status uses a manual loop, change_offer_details inlines the check, and get_my_offer_for_listing doesn't need one.

Consider refactoring to class-based views:

class OfferStatusUpdate(UpdateAPIView): """Allow the listing seller to update an offer's status.""" queryset = Offer.objects.all() serializer_class = OfferStatusSerializer permission_classes = [ListingOwnerOffersPermission | IsSuperUser] lookup_url_kwarg = "offer_id" http_method_names = ["patch"] def update(self, request, *args, **kwargs): response = super().update(request, *args, partial=True, **kwargs) return Response(OfferSerializer(self.get_object()).data) class OfferDetailsUpdate(UpdateAPIView): """Allow the offer owner to edit offered_price and message.""" queryset = Offer.objects.all() serializer_class = OfferDetailsSerializer permission_classes = [OfferOwnerPermission | IsSuperUser] lookup_url_kwarg = "offer_id" http_method_names = ["patch"] def update(self, request, *args, **kwargs): response = super().update(request, *args, partial=True, **kwargs) return Response(OfferSerializer(self.get_object()).data) class MyOfferForListing(RetrieveAPIView): """Return the authenticated user's offer for a given listing.""" serializer_class = OfferSerializer permission_classes = [IsAuthenticated] def get_object(self): offer = Offer.objects.filter( listing_id=self.kwargs["listing_id"], user=self.request.user, ).first() if offer is None: raise exceptions.NotFound("No offer for this listing") return offer

Note: OfferDetailsUpdate needs an OfferOwnerPermission that checks obj.user == request.user. The original OfferOwnerPermission was renamed to ListingOwnerOffersPermission and repurposed — but ListingOwnerOffersPermission returns obj.listing.seller == request.user for PATCH, which is wrong for offer-owner edits. Consider keeping the original OfferOwnerPermission alongside the new ListingOwnerOffersPermission.

jamesdoh0109

Left some more backend comments, we can talk during gbm, but this should be my last set of backend comments

jamesdoh0109 · 2026-04-05T15:47:33Z

@@ -0,0 +1,109 @@
+from decimal import Decimal


Ok maybe make a comment in the file. Also not sure seed_offer is the best name

jamesdoh0109 · 2026-04-05T15:48:49Z

+        if request.method in ("PATCH", "PUT"):
+            return obj.user_id == request.user.id
+        return False


Suggested change

if request.method in ("PATCH", "PUT"):

return obj.user_id == request.user.id

return False

return request.method in {"PATCH", "PUT"} and obj.user_id == request.user.id

jamesdoh0109 · 2026-04-05T15:49:40Z

+        if request.method in permissions.SAFE_METHODS:
+            return obj.listing.seller == request.user
+
+        if request.method in ("PATCH", "PUT"):


you can prob combine these 2 conditions since they are doing the same thing

jamesdoh0109 · 2026-04-05T16:10:00Z

    """

-    permission_classes = [OfferOwnerPermission | IsSuperUser]
+    permission_classes = [ListingOwnerOffersPermission | IsSuperUser]


ListingOwnerOffersPermission mixes seller and buyer logic, and the Offers viewset relies on DRF internals for correct access control.

The Offers viewset serves three actions (list, create, destroy) for two different actors (seller and buyer), but applies a single permission class (ListingOwnerOffersPermission) to all of them. This causes a few issues:

create works by accident. ListingOwnerOffersPermission.has_object_permission only grants access to the listing seller (for GET/PATCH/PUT) or the offer creator (fallthrough). But create works for any authenticated user because DRF never calls has_object_permission on create actions — there's no object yet. The permission class doesn't actually permit this; it just never gets asked.

destroy has redundant access control. The destroy() method hard-filters the queryset to user=request.user, so only the buyer's own offer is ever fetched. The fallthrough in has_object_permission (return obj.user == request.user) then passes for the same user. The permission check is redundant with the view logic, and the intent is unclear.

Seller and buyer checks are mixed in one class. ListingOwnerOffersPermission checks obj.listing.seller for GET/PATCH/PUT but obj.user for DELETE. The class name and docstring suggest it's about the listing owner, but it quietly handles buyer logic too.

Fix: Replace the single permission_classes on the Offers viewset with get_permissions() that returns the appropriate permission per action — IsAuthenticated for create, OfferOwnerPermission | IsSuperUser for destroy, and ListingOwnerOffersPermission | IsSuperUser for list. Then simplify both permission classes: ListingOwnerOffersPermission should only check obj.listing.seller == request.user, and OfferOwnerPermission should only check obj.user_id == request.user.id, with no method-specific branching (the view controls which methods are allowed).

jamesdoh0109

I'll most likely have more comments after this round of feedback, but for your frontend, try to refactor so that ListingActions.tsx has only logic that actually correspond to "actions" - so button for "Make Offer" (if it is not your item" or buttons for editing and deleting (if it is your item). Then your OffersSection component, you can conditionally render either an offer that you have made for an item that is not yours or offers that you have received for items that are yours. For Offer Card, try to see the difference between my offer card vs offer card for offers you received.

jamesdoh0109 · 2026-04-05T17:39:49Z

+  const deleteMutation = useMutation({
+    mutationFn: () => deleteListing(listing.id),
+    onSuccess: () => {
+      router.push(listing.listing_type === "sublet" ? "/sublets" : "/items");
+    },
+    onError: () => {
+      onOpenChange(false);
+    },
+  });


Take a look into invalidating the tanstack cache for the list/optimistically updating it so we see that the listing is deleted right away in the list

jamesdoh0109 · 2026-04-05T17:51:13Z

Lets make this into a page, not modal, including images. That way we can also re use what we have

jamesdoh0109 · 2026-04-05T18:03:21Z

  const [modalState, setModalState] = useState<ModalState>("none");
  const [pendingPhoneNumber, setPendingPhoneNumber] = useState<string>("");
  const [isChangingPhone, setIsChangingPhone] = useState<boolean>(false);
+  const [isEditing, setIsEditing] = useState(false);


Prob wont need this anymore after we remove edit modal

jamesdoh0109 · 2026-04-05T18:22:42Z

+  return await serverFetch<PaginatedResponse<Offer>>("/market/offers/received/");
+}
+
+export async function getOffersForListing(listingId: number) {


maybe getOffersReceivedForListing?

jamesdoh0109 · 2026-04-05T18:48:23Z

+  const offers = offersResponse?.results ?? [];
+  const initialMyOffer = !isOwner ? await getMyOfferForListing(item.id) : null;


Can we rename these to offersReceived and myOfferGiven?

Or something similar

jamesdoh0109 · 2026-04-05T18:48:41Z

+  const offersResponse = isOwner ? await getOffersForListing(sublet.id) : null;
+  const offers = offersResponse?.results ?? [];
+  const initialMyOffer = !isOwner ? await getMyOfferForListing(sublet.id) : null;


Same comment here about naming

jamesdoh0109

Left comments

jamesdoh0109 · 2026-04-09T19:04:30Z

+    Permission for the owner of a listing:
+    - GET: listing seller can view offers on their listing
+    - PATCH/PUT: listing seller can update offer status
+    - DELETE: offer creator can delete/withdraw their own offer


Suggested change

Permission for the owner of a listing:

- GET: listing seller can view offers on their listing

- PATCH/PUT: listing seller can update offer status

- DELETE: offer creator can delete/withdraw their own offer

Permission for the listing seller:

- GET: can view offers on their listing

- PATCH/PUT: can update offer status

jamesdoh0109 · 2026-04-09T19:04:31Z

+        if request.method in permissions.SAFE_METHODS:
+            return obj.listing.seller == request.user
+
+        if request.method in ("PATCH", "PUT"):
            return obj.listing.seller == request.user

        return obj.user == request.user


Suggested change

if request.method in permissions.SAFE_METHODS:

return obj.listing.seller == request.user

if request.method in ("PATCH", "PUT"):

return obj.listing.seller == request.user

return obj.user == request.user

return obj.listing.seller == request.user

jamesdoh0109 · 2026-04-09T19:04:33Z

+class OfferOwnerPermission(permissions.BasePermission):
+    """
+    Permission for the buyer who created an offer:
+    - PATCH/PUT: offer owner can edit their own offer (e.g. price/message)


Suggested change

- PATCH/PUT: offer owner can edit their own offer (e.g. price/message)

- PATCH/PUT: can edit their own offer (price/message)

- DELETE: can withdraw their own offer

jamesdoh0109 · 2026-04-09T19:04:34Z

+        if request.method in ("PATCH", "PUT"):
+            return obj.user_id == request.user.id
+        return False


Suggested change

if request.method in ("PATCH", "PUT"):

return obj.user_id == request.user.id

return False

return obj.user_id == request.user.id

jamesdoh0109 · 2026-04-19T18:43:50Z

+
  const toggleFavoriteMutation = useMutation({
-    meta: { suppressErrorToast: true }, // since it's noisy to show error toast on top of optimistic update
+    meta: { suppressErrorToast: true },


can you revert back the comment plz

jamesdoh0109 · 2026-04-24T17:56:01Z

I really dont see the reason for having this whole separate component for editing. Cant we just reuse the ItemForm and SubletForm? Obviously, we'll have to refactor a bit to conditionally pre-fill the form with exisitng listing data or render either "Edit Item" or "Create Item" button

jamesdoh0109 · 2026-04-24T17:58:36Z

Instead of having separate MyOfferCard.tsx and OfferCard.tsx, can we combine them into one offer card component since they display mostly similar things?

jamesdoh0109 · 2026-04-24T17:59:32Z

OffersReceived isnt an accurate name if you are also conditionally rendering offer that I gave

jamesdoh0109 · 2026-04-24T18:01:36Z

+  try {
+    return await serverFetch<Offer>(`/market/listings/${listingId}/offers/mine/`);
+  } catch (error) {
+    if (error instanceof APIError && error.status === 404) return null;
+    throw error;
+  }
+}


No need to catch here, since the new serverFetch will handle

jamesdoh0109 · 2026-04-24T18:02:38Z

What is this for? You didnt make any changes to package.json?

Resolve conflicts: hydrate listing query + keep offers/edit UX; SubletForm with geocode + edit mode; SubletMap on detail; 404-safe getMyOfferForListing Co-authored-by: Cursor <cursoragent@cursor.com>

LautaroJBeck added 2 commits February 28, 2026 01:55

Adding seller's view

fd9c0e7

Deleting My offers view

5a10647

LautaroJBeck changed the title ~~Lao/listing view~~ Lau/listing view Mar 1, 2026

LautaroJBeck added 2 commits March 1, 2026 14:39

Adding edit form for sublets

28cb881

Solving 500 error when deleting item

34ae14f

Base automatically changed from james/better-error-handling to master March 19, 2026 20:15

i30101 reviewed Mar 22, 2026

View reviewed changes

pennlabs deleted a comment from i30101 Mar 27, 2026

jamesdoh0109 requested changes Mar 27, 2026

View reviewed changes

LautaroJBeck added 2 commits March 27, 2026 18:23

Working on PR comments

9226799

Finalizing changes

5bbc124

LautaroJBeck commented Mar 29, 2026

View reviewed changes

LautaroJBeck added 2 commits April 3, 2026 04:03

Addressing comments, adding new view to see offers made

8a00698

Fixing type check errors

b553214

jamesdoh0109 reviewed Apr 3, 2026

View reviewed changes

Refactoring FBVs and changing .gitignore

da542e0

jamesdoh0109 reviewed Apr 5, 2026

View reviewed changes

Addressing backend comments 2

35175a8

jamesdoh0109 reviewed Apr 5, 2026

View reviewed changes

LautaroJBeck added 2 commits April 19, 2026 14:10

Addressing frontend

ca8ac1c

Addressing frontend comments 2

075c60b

jamesdoh0109 reviewed Apr 24, 2026

View reviewed changes

LautaroJBeck and others added 3 commits May 13, 2026 06:51

Refactoring edit items routes 1

bd8f917

Merge branch 'master' into lao/listing-view

79cafc1

Resolve conflicts: hydrate listing query + keep offers/edit UX; SubletForm with geocode + edit mode; SubletMap on detail; 404-safe getMyOfferForListing Co-authored-by: Cursor <cursoragent@cursor.com>

Merging new serverFetch function into the branch

35ed950

		const offersResponse = isOwner ? await getOffersReceived() : null;
		const offers = offersResponse?.results?.filter((offer) => offer.listing === item.id) ?? [];

		const offersResponse = await (isOwner ? getOffersReceived() : getOffersMade());
		const offers = offersResponse?.results?.filter((offer) => offer.listing === sublet.id) ?? [];

		const offers = offersResponse?.results ?? [];
		const initialMyOffer = !isOwner ? await getMyOfferForListing(item.id) : null;

	- PATCH/PUT: offer owner can edit their own offer (e.g. price/message)
	- PATCH/PUT: can edit their own offer (price/message)
	- DELETE: can withdraw their own offer

Conversation

LautaroJBeck commented Mar 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesdoh0109 left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesdoh0109 left a comment

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!

jamesdoh0109 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

jamesdoh0109 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jamesdoh0109 left a comment •

edited

Loading