Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion backend/market/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,14 @@ class OfferSerializer(ModelSerializer):

class Meta:
model = Offer
fields = ["id", "user", "listing", "offered_price", "message", "created_at"]
fields = [
"id",
"user",
"listing",
"offered_price",
"message",
"created_at",
]
read_only_fields = ["id", "created_at", "user"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Once you add the status field, you should also add it here. Serializers are responsible for what data goes in and out of our API, and specifying in the fields section makes sure that when the serializer converts the python obj into a json respojnse, the field is included in the response. You should also additionally define status in read_only_fields because this serializer gets used to actually create offers by buyers. We don't want them to be able to modify status field themselves. Because of this, we would likely need a new serializer like OfferStatusSerializer that onlyt exposes id and status with id as read only. So the only thing you can write thru this serializer is status, and this is how we can follow the principle of least privilge, where the update endpoint cna only do exactly what it is meant to do. You might also want to perform some field level validation in the new serializer to make sure that status is a valid status.


def create(self, validated_data):
Expand Down
5 changes: 5 additions & 0 deletions backend/market/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@
OffersReceived,
Tags,
UserFavorites,
accept_offer,
get_current_user,
get_phone_status,
reject_offer,
send_verification_code,
verify_phone_code,
)
Expand Down Expand Up @@ -49,6 +51,9 @@
"listings/<listing_id>/offers/",
Offers.as_view({"get": "list", "post": "create", "delete": "destroy"}),
),
# 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"),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

# Image Creation
path("listings/<listing_id>/images/", CreateImages.as_view()),
# Image Deletion
Expand Down
22 changes: 22 additions & 0 deletions backend/market/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,28 @@ def list(self, request, *args, **kwargs):
return super().list(request, *args, **kwargs)


@api_view(["POST"])
@permission_classes([IsAuthenticated])
def accept_offer(request, offer_id):
offer = get_object_or_404(Offer, pk=offer_id)
if offer.listing.seller != request.user:
raise exceptions.PermissionDenied("Only the listing owner can accept offers.")
offer.status = Offer.Status.ACCEPTED
offer.save(update_fields=["status"])
return Response(OfferSerializer(offer).data)


@api_view(["POST"])
@permission_classes([IsAuthenticated])
def reject_offer(request, offer_id):
offer = get_object_or_404(Offer, pk=offer_id)
if offer.listing.seller != request.user:
raise exceptions.PermissionDenied("Only the listing owner can reject offers.")
offer.status = Offer.Status.REJECTED
offer.save(update_fields=["status"])
return Response(OfferSerializer(offer).data)

Comment on lines 362 to +365
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

3 things:

  1. Once you address my feedback for the api url, these should no longer exist. Instead, define a single view. Might want to refer to UpdateAPIView (check docs online)
  2. You are modifying the status field on the offer but status doesnt even exist in the offer model. You need to edit models.py to add status field in the Offer class (it might be bneneficial to add Status choices like
class Status(models.TextChoices):
    PENDING = "pending", "Pending"                        
    ACCEPTED = "accepted", "Accepted"                   
    REJECTED = "rejected", "Rejected"     

Remember, once you make model changes you need to make migration files, then commit that to version control
3.

if offer.listing.seller != request.user:
    raise exceptions.PermissionDenied("Only the listing owner can accept offers.")

This technically worksd but it breaks the project's pattern. Every other view delegates authorization to a permission class in permissions.py. Instead, go to permissions.py. As a side note, it's a much better practice to handle authorization logic in one place than being mixed into view logic. Its an application of separation of concern/signle responsibilitry principle where each permission class has one job and views have one job (handling req/resp logic). Refer to other permission classes and how they are used.


@api_view(["POST"])
@permission_classes([IsAuthenticated])
def send_verification_code(request):
Expand Down
17 changes: 14 additions & 3 deletions frontend/app/items/[id]/page.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
import { ListingDetail } from "@/components/listings/detail/ListingDetail";
import { getListingOrNotFound } from "@/lib/actions";
import { getCurrentUser, getListingOrNotFound, getOffersReceived } from "@/lib/actions";

export default async function ItemPage({ params }: { params: Promise<{ id: string }> }) {
const { id } = await params;
const item = await getListingOrNotFound(id);
const [item, currentUser] = await Promise.all([getListingOrNotFound(id), getCurrentUser()]);
const isOwner = currentUser?.id === item.seller.id;
const offersResponse = isOwner ? await getOffersReceived() : null;
const offers = offersResponse?.results?.filter((offer) => offer.listing === item.id) ?? [];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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


return <ListingDetail listing={item} initialIsFavorited={item.is_favorited ?? false} />;
return (
<ListingDetail
listing={item}
initialIsFavorited={item.is_favorited ?? false}
offers={offers}
offersMode={isOwner ? "received" : "made"}
canEdit={isOwner}
/>
);
}
22 changes: 19 additions & 3 deletions frontend/app/sublets/[id]/page.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,25 @@
import { ListingDetail } from "@/components/listings/detail/ListingDetail";
import { getListingOrNotFound } from "@/lib/actions";
import {
getCurrentUser,
getListingOrNotFound,
getOffersMade,
getOffersReceived,
} from "@/lib/actions";

export default async function SubletPage({ params }: { params: Promise<{ id: string }> }) {
const { id } = await params;
const sublet = await getListingOrNotFound(id);
const [sublet, currentUser] = await Promise.all([getListingOrNotFound(id), getCurrentUser()]);
const isOwner = currentUser?.id === sublet.seller.id;
const offersResponse = await (isOwner ? getOffersReceived() : getOffersMade());
const offers = offersResponse?.results?.filter((offer) => offer.listing === sublet.id) ?? [];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same comment here


return <ListingDetail listing={sublet} initialIsFavorited={sublet.is_favorited ?? false} />;
return (
<ListingDetail
listing={sublet}
initialIsFavorited={sublet.is_favorited ?? false}
offers={offers}
offersMode={isOwner ? "received" : "made"}
canEdit={isOwner}
/>
);
}
6 changes: 6 additions & 0 deletions frontend/components/listings/detail/ListingActions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ interface Props {
listingPrice: number;
listingOwnerLabel: string;
priceLabel?: string;
canEdit?: boolean;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

canEdit is not a good variable name imo. Who can edit? isOwner is better

}

type ModalState = "none" | "phone-input" | "verification" | "offer";
Expand All @@ -23,6 +24,7 @@ export const ListingActions = ({
listingPrice,
priceLabel,
listingOwnerLabel,
canEdit = false,
}: Props) => {
const [modalState, setModalState] = useState<ModalState>("none");
const [pendingPhoneNumber, setPendingPhoneNumber] = useState<string>("");
Expand All @@ -35,6 +37,10 @@ export const ListingActions = ({
queryFn: getPhoneStatus,
});

if (canEdit) {
return null;
}

const handleMakeOfferClick = () => {
if (!phoneStatus) return;

Expand Down
Loading