Minghan market two routes #318

minghansun1 · 2024-11-06T00:56:26Z

Made two routes for Item/Sublet, also made sure other routes work

… in views to use a more abstract filtering method

vcai122

Okay, code is looking a lot cleaner, really great work!

I left a decent amount of comments all just pushing you towards cleaner code and to understand what Django is actually doing, it's tough thinking about this stuff but you are doing a great job.

Also, I am mentioning a decent amount of stuff that was just code you moved over. For now since they work and are tested lets just leave a TODO to fix in a later PR (perhaps after you get everything in and working)

backend/market/models.py

vcai122 · 2024-11-11T01:36:06Z

backend/market/permissions.py

@@ -51,7 +62,7 @@ def has_permission(self, request, view):

    def has_object_permission(self, request, view, obj):
        if request.method in permissions.SAFE_METHODS:


I think this method might be wrong? If the type of obj is an Offer, then .subletter does not exist right? Let's ask @dr-Jess

backend/market/serializers.py

vcai122 · 2024-11-11T01:39:39Z

backend/market/serializers.py



 # Browse images
-class SubletImageURLSerializer(serializers.ModelSerializer):
+class ItemImageURLSerializer(serializers.ModelSerializer):
    image_url = serializers.SerializerMethodField("get_image_url")


I think you don't need the "get_image_url", that is the default method it tries to look for I believe

vcai122 · 2024-11-11T02:09:32Z

backend/market/views.py

-        sublet_id = int(self.kwargs["sublet_id"])
-        self.get_queryset()  # check if sublet exists
+        item_id = int(self.kwargs["item_id"])
+        self.get_queryset()  # check if item exists
        img_serializers = []


change this to list comprehension

vcai122 · 2024-11-11T02:10:05Z

backend/market/views.py


        # Serialize and return the queryset
-        serializer = SubletSerializerSimple(queryset, many=True)
+        serializer = self.get_serializer(queryset, many=True)
        return Response(serializer.data)


 class CreateImages(generics.CreateAPIView):


Looking at this code we are not really leveraging the fact that it is a CreateAPIView at all... let's pin this and put a TODO about this

vcai122 · 2024-11-11T02:11:43Z

backend/market/views.py

@@ -230,77 +237,75 @@ def destroy(self, request, *args, **kwargs):


 class Favorites(mixins.DestroyModelMixin, mixins.CreateModelMixin, viewsets.GenericViewSet):


This is sus, we are not leveraging the viewset/serializer at all, put another pin here (or fix if you want this fix looks simpler)

vcai122 · 2024-11-11T02:14:20Z

backend/market/views.py

+
+    queryset = queryset.filter(**filters)
+
+    # Apply tag filtering iteratively if "tags" parameter is provided


why r tags special

Tags are filtered by tags__name, which is different from the param name that the frontend passes in to filter with tags. This is because frontend thinks they're filtering based on tags, when filtering is actually being done on the name field of the Tag model.

vcai122 · 2024-11-11T02:17:07Z

backend/market/views.py

@@ -57,167 +68,163 @@ def get_queryset(self):
        return Offer.objects.filter(user=user)


-class Properties(viewsets.ModelViewSet):
+def apply_filters(


I think passing this should be called create_filters and we shouldn't be passing in a queryset.
Also filter_mappings, is_sublet, and tag_field is a bit contrived.

You should

Make a (static) function that does this for Items and does it cleanly and well without extra accessories (in the Item view)

Make a function that does this cleanly and well for sublet (in the sublet view) that uses the Item function for help to make it more concise

vcai122

One last round I think. Mainly has to due with verifying if we need the manual permissions check. Your code is look really good, I can see the effort you put in to make it clean. Awesome job!

vcai122 · 2024-11-18T20:47:13Z

backend/market/serializers.py

+    class Meta:
+        model = Tag
+        fields = "__all__"
+        read_only_fields = [field.name for field in model._meta.fields]


nice! I assume you tried "all" first and it didn't work.

Let's leave a TODO here about extracting out this pattern. For context I'm thinking about building out a small "core" library soon, so we can include a "ReadOnlySerializer" in there that we can just inherit from to make this easier!

vcai122 · 2024-11-18T20:51:56Z

backend/market/serializers.py

        ]
+        read_only_fields = ["id", "created_at", "seller", "buyers", "images"]


If I'm interpreting correct, this leaves

title, description, external_link, price, negotiable, expires_at as changeable?

id, created_at are auto generated on create, seller is manually added below, and buyers, and images can be empty, so create still works

I added favorites to read-only fields since it should only be changed through the favorites route.

vcai122 · 2024-11-18T20:52:31Z

backend/market/serializers.py

        instance = super().create(validated_data)
-        instance.save()
        return instance


nit: don't need to set instance before returning

vcai122 · 2024-11-18T20:55:59Z

backend/market/serializers.py

+        validated_data["seller"] = self.context["request"].user
+        category_name = validated_data.pop("category")
+        category_instance = Category.objects.get(name=category_name)
+        validated_data["category"] = category_instance
        instance = super().create(validated_data)
        instance.save()
        return instance

    # delete_images is a list of image ids to delete
    def update(self, instance, validated_data):


I don't see anything being altered? We are talking about the update function right?

vcai122 · 2024-11-18T20:57:01Z

backend/market/serializers.py

            or self.context["request"].user.is_superuser
        ):
            instance = super().update(instance, validated_data)
            instance.save()
            return instance
        else:
-            raise serializers.ValidationError("You do not have permission to update this sublet.")
+            raise serializers.ValidationError("You do not have permission to update this item.")

    def destroy(self, instance):


Can you remove this and test?

I feel like we do not actually need this given that you wrote the permission classes above (get_object_permissions should be responsible for controlling who can change the object)

vcai122 · 2024-11-18T21:02:19Z

backend/market/serializers.py

+        fields = ["id", "item", "address", "beds", "baths", "start_date", "end_date"]
+        read_only_fields = ["id"]
+
+    def create(self, validated_data):


gotcha, this looks good then!

vcai122 · 2024-11-18T21:03:36Z

backend/market/serializers.py

+        instance = super().create(validated_data)
+        return instance
+
+    def update(self, instance, validated_data):


I think we can use the item serializer for update just like you did above?

Also if you find that we don't need this explicit permission check on the serializer, lets remove it

vcai122 · 2024-11-18T21:07:40Z

backend/market/serializers.py

+            instance.save()
+            return instance
+
+    def destroy(self, instance):


a little bit. Doesn't seem terrible because our end user is in the same room as us (ios/android developers), so we have the liberty of assuming they aren't clueless, and we would be removing room for error in our code.

but i could go either way. I did see you overode delete on models which is good if we want to go the delete sublet route as that would ensure deletion on admin portal will also delete the item. So you have no need to double delete as you are here. Also clean up this method lol (and remove permission checking if you verify we don't need)

codecov · 2024-11-23T16:34:55Z

Codecov Report

Attention: Patch coverage is 54.39560% with 83 lines in your changes missing coverage. Please review.

Project coverage is 86.68%. Comparing base (316e43a) to head (15274d4).

Files with missing lines	Patch %	Lines
backend/market/views.py	32.92%	55 Missing ⚠️
backend/market/serializers.py	65.30%	17 Missing ⚠️
backend/market/models.py	80.64%	6 Missing ⚠️
backend/market/permissions.py	44.44%	5 Missing ⚠️

Additional details and impacted files

@@                      Coverage Diff                      @@
##           create-market-from-sublet     #318      +/-   ##
=============================================================
+ Coverage                      79.94%   86.68%   +6.74%     
=============================================================
  Files                             68       68              
  Lines                           3032     3035       +3     
=============================================================
+ Hits                            2424     2631     +207     
+ Misses                           608      404     -204

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

minghansun1 added 7 commits October 23, 2024 00:26

Updated models.py

502b49c

updated models and changed permissions

318db5c

renamed everything from sublet to market, added app to installed_apps

b76fc05

Updated serializers and views to support creating a sublet through item

da3fbe8

Added serializer level validation for category+sublet

9b66650

Addressed unaddressed feedback and feedback from today

24dbfad

two routes for Item/Sublet, made sure other features worked too

4771b60

minghansun1 requested a review from vcai122 November 6, 2024 00:56

minghansun1 added 2 commits November 10, 2024 19:02

Removed Read Serializer, Refactored list function for Item and Sublet…

846697f

… in views to use a more abstract filtering method

used code formatter

c4610ba

vcai122 requested changes Nov 11, 2024

View reviewed changes

minghansun1 added 3 commits November 17, 2024 13:19

fixed most of Vincent's comments

2e66213

fixed all comments except last one

306f986

Addressed all comments

3e2c8c1

vcai122 requested changes Nov 18, 2024

View reviewed changes

minghansun1 added 3 commits November 22, 2024 18:40

addressed all concerns

c52049e

deleted unused apply_filters function

8efe703

linted

15274d4

added 7 tests for create andupdate

8b32eb1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minghan market two routes #318

Minghan market two routes #318

minghansun1 commented Nov 6, 2024

vcai122 left a comment

vcai122 Nov 11, 2024

vcai122 Nov 11, 2024

vcai122 Nov 11, 2024

vcai122 Nov 11, 2024

vcai122 Nov 11, 2024

vcai122 Nov 11, 2024

minghansun1 Nov 17, 2024 •

edited

Loading

vcai122 Nov 11, 2024

vcai122 left a comment •

edited

Loading

vcai122 Nov 18, 2024

vcai122 Nov 18, 2024

minghansun1 Nov 22, 2024

vcai122 Nov 18, 2024

vcai122 Nov 18, 2024

vcai122 Nov 18, 2024

vcai122 Nov 18, 2024

vcai122 Nov 18, 2024

minghansun1 Nov 22, 2024

vcai122 Nov 18, 2024

codecov bot commented Nov 23, 2024

		@@ -51,7 +62,7 @@ def has_permission(self, request, view):

		def has_object_permission(self, request, view, obj):
		if request.method in permissions.SAFE_METHODS:

		@@ -230,77 +237,75 @@ def destroy(self, request, args, *kwargs):


		class Favorites(mixins.DestroyModelMixin, mixins.CreateModelMixin, viewsets.GenericViewSet):


		queryset = queryset.filter(**filters)

		# Apply tag filtering iteratively if "tags" parameter is provided

		]
		read_only_fields = ["id", "created_at", "seller", "buyers", "images"]

Minghan market two routes #318

Are you sure you want to change the base?

Minghan market two routes #318

Conversation

minghansun1 commented Nov 6, 2024

vcai122 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minghansun1 Nov 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcai122 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 23, 2024

Codecov Report

minghansun1 Nov 17, 2024 •

edited

Loading

vcai122 left a comment •

edited

Loading