Feature/api products catalogue #65

knop-k · 2023-12-18T15:22:45Z

Hi all!
I am just starting my adventure with DRF so please give me constructive comments on what I could improve/add/change.
I have prepared an API for the product:

list view
detail view
update
delete

In Additionally:
added filtering and pagination

Feel free to ask questions and comment

jacoor · 2023-12-19T11:38:20Z

Please fix linting before we review it.

KryssPy · 2023-12-19T20:32:20Z

Dshop/apps/products_catalogue/api_urls.py

+from .api_views import ProductViewSet
+from rest_framework.routers import DefaultRouter
+
+router = DefaultRouter()


I think it would be a good idea to remove the trailing slashes. My front-end friend recently pointed out that it was easier for them that way.
router = DefaultRouter(trailing_slash=False)

I dont have preference @knop-k. Your call.

KryssPy · 2023-12-19T20:33:58Z

Dshop/apps/products_catalogue/api_views.py

+from .models import Product
+from .serializers import ProductSerializer
+from .filters import ProductFilter
+from rest_framework import viewsets
+from django_filters.rest_framework import DjangoFilterBackend


PEP8 import order rules not followed

It's not followed in the whole project. Good pointing out, we need to add something to guard id. Right now, let's ignore.

KryssPy · 2023-12-19T20:34:39Z

Dshop/apps/products_catalogue/filters.py

+from django_filters import rest_framework as filters
+from .models import Product


PEP8 import order rules not followed

KryssPy · 2023-12-19T20:36:18Z

Dshop/apps/products_catalogue/serializers.py

+    class Meta:
+        model = Product
+        fields = ['id', 'category', 'name', 'price', 'short_description', 'full_description', 'parent_product']


Just a suggestion: you can add read_only_fields to Meta (with e.g. id field)

KryssPy · 2023-12-19T20:36:46Z

Dshop/apps/products_catalogue/tests/test_api_products.py

+import pytest
+from django.urls import reverse
+from apps.products_catalogue.models import Category, Product
+# pylama:ignore=W0404, W0611
+from apps.users.conftest import api_client, login_url, login_data, user_instance, user_instance_token


PEP8 import order rules not followed

It's not followed in the whole project. Good pointing out, we need to add something to guard id. Right now, let's ignore.

jacoor

Looks good, needs small tweaks in tests.

jacoor · 2023-12-20T19:30:08Z

Dshop/apps/products_catalogue/api_urls.py

+from .api_views import ProductViewSet
+from rest_framework.routers import DefaultRouter
+
+router = DefaultRouter()


I dont have preference @knop-k. Your call.

jacoor · 2023-12-20T19:31:06Z

Dshop/apps/products_catalogue/api_views.py

+from .models import Product
+from .serializers import ProductSerializer
+from .filters import ProductFilter
+from rest_framework import viewsets
+from django_filters.rest_framework import DjangoFilterBackend


It's not followed in the whole project. Good pointing out, we need to add something to guard id. Right now, let's ignore.

jacoor · 2023-12-20T19:31:38Z

Dshop/apps/products_catalogue/tests/test_api_products.py

+import pytest
+from django.urls import reverse
+from apps.products_catalogue.models import Category, Product
+# pylama:ignore=W0404, W0611
+from apps.users.conftest import api_client, login_url, login_data, user_instance, user_instance_token


It's not followed in the whole project. Good pointing out, we need to add something to guard id. Right now, let's ignore.

Dshop/apps/products_catalogue/tests/test_api_products.py

Dshop/apps/products_catalogue/filters.py

…ture in tests

…ve and inactive and check the output structure

jacoor

Small changes. Looks good, almost there.

jacoor · 2023-12-29T11:53:42Z

Dshop/apps/products_catalogue/tests/test_api_products.py

+    results = response.data.get('results', [])
+    assert len(results) == 1
+
+    for product_data in results:


Why using loop if there is only one product?

Suggested change

for product_data in results:

product_data = results[0]

fix indentation

also, it checks that fields are there, but not fields values. Would be good to add values check too.
What I would to is prob create

def assert_active_object(data): # copy all fields values and compare to data[x] ie: fields_values = { "name": "main product" # ... more fields } for key, value in data.items(): assert fields_values[key] == value

And use that function whenever you need to compare active_object with response.

@jacoor What do you think of this approach:

def assert_active_object(data): fields_values = { "name": "main product", "price": "11.00", "short_description": "short desc", "full_description": "full_description", } for key, value in data.items(): if key in fields_values: assert fields_values[key] == value

because I don't want to check for example id or category

Sorry for late response.
For id it would be good to check if field is there, and for category too. Would be enough.

Thanks @jacoor , I added this to test

jacoor · 2024-01-07T14:18:47Z

Dshop/apps/products_catalogue/tests/test_api_products.py

+        "full_description": "full_description",
+    }
+    for key, value in data.items():
+        if key in fields_values:


please remove this if.
The goal of the test is to FAIL if something is missing, this is so called "silent failure" - test passes, you expect all good but no, field is not there.
Example: data = {}
Test will pass.

jacoor · 2024-01-07T14:20:14Z

Dshop/apps/products_catalogue/tests/test_api_products.py

-        assert 'parent_product' in product_data
+    product_data = results[0]
+
+    assert 'id' in product_data, "Field 'id' is missing in product data"


I think I would add this assertion to assert_active_object function. This way we will have one assertion in test, which is good practice.

Also enough will be to do

product_data['category'] is not None

As this will cover both cases: missing category or category being None. I would remove error message as in this case it might be confusing.
Tests should be short and concise, as this makes them easier to maintain.

…tive_object function

knop-k added 9 commits December 18, 2023 10:44

env: add django-filter

6375bbb

feat: add django-filters to instaled_apps

1dd9614

test: add tests for products_catalogue api

54ff4f3

feat: settings for pagination and filters

4f06486

feat: add urls for products_catalagoue

6a979f4

feat: add api products to main urls

9b557fa

feat: create serializers for product

f5da487

feat: add filter for product

6fe3ae1

feat: add ProductViewSet

4d35211

refactor: fix pylama error

d838339

KryssPy reviewed Dec 19, 2023

View reviewed changes

Merge branch 'dev' into feature/api_products_catalogue

84b0232

jacoor requested changes Dec 20, 2023

View reviewed changes

knop-k and others added 9 commits December 29, 2023 11:57

Merge branch 'dev' into feature/api_products_catalogue

b4a8614

fix: poetry lock

ad8496f

feat: add read only fields for 'id' in serializers

e90c067

test: add active and inactive fixture of products

1f1586f

refactor: replace create_product fixture to create_active_product fix…

eb78938

…ture in tests

test: extend test_access_protected_resource to add two products: acti…

2010e6d

…ve and inactive and check the output structure

refactor: create authenticated_api_client fixture

c123e72

refactor: remove comment

c5bd5a4

feat: add is_active in filters

b0ecc7b

jacoor requested changes Dec 29, 2023

View reviewed changes

refactor: update in test_access_protected_resource

b0efde6

knop-k requested a review from a team January 6, 2024 10:23

jacoor requested changes Jan 7, 2024

View reviewed changes

jacoor reviewed Jan 7, 2024

View reviewed changes

refactor: update in test_access_protected_resource - update assert_ac…

6cb032f

…tive_object function

knop-k requested a review from a team January 8, 2024 14:47

jacoor approved these changes Jan 13, 2024

View reviewed changes

knop-k merged commit 6b067fd into dev Jan 13, 2024
2 checks passed

knop-k deleted the feature/api_products_catalogue branch January 13, 2024 12:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/api products catalogue #65

Feature/api products catalogue #65

knop-k commented Dec 18, 2023

jacoor commented Dec 19, 2023

KryssPy Dec 19, 2023

jacoor Dec 20, 2023

KryssPy Dec 19, 2023

jacoor Dec 20, 2023

KryssPy Dec 19, 2023

KryssPy Dec 19, 2023

KryssPy Dec 19, 2023

jacoor Dec 20, 2023

jacoor left a comment

jacoor Dec 20, 2023

jacoor Dec 20, 2023

jacoor Dec 20, 2023

jacoor left a comment

jacoor Dec 29, 2023

jacoor Dec 29, 2023

knop-k Dec 29, 2023 •

edited

Loading

jacoor Jan 4, 2024

knop-k Jan 6, 2024

jacoor Jan 7, 2024

jacoor Jan 7, 2024

jacoor Jan 7, 2024

		from django_filters import rest_framework as filters
		from .models import Product

Feature/api products catalogue #65

Feature/api products catalogue #65

Conversation

knop-k commented Dec 18, 2023

jacoor commented Dec 19, 2023

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

jacoor 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

jacoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knop-k Dec 29, 2023 • 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

knop-k Dec 29, 2023 •

edited

Loading