Feature/46 producttype nl api

[Floris272]

Changes

• Adds the producttypen api

Notes

• location, organisation & contact will be added in a later pr.
• api will get refactored after all viewsets/serializers are done.

[Coperh]

Generally looks good I think. Nothing is broken. The use of the exclude field is generally bad

From the default project:

Check that ModelForms use Meta.fields instead of Meta.exclude.

ModelForm.Meta.exclude is dangerous because it doesn't protect against
fields that are added later. Explicit white-listing is safer and prevents
bugs such as IMA #645.

Also a few questions and random remarks

Should be Changed (or can be changed quickly):

• Formatted translation might cause issues
• Useself.get_queryset()instead of the model
• use fields = (.....) instead of exclude = (.....) ==> easier to understand and removes unexpected fields
• Reverses instead of hard coded URLS

Could Be Changed:

• Should use self.client get/post/patch/put instead of BaseApiTestCase methods
• Test that fields are actually set on basic creation tests
• Perform database stuff after validation i.e. do not perform if not validated
• Related to the last: only test for errors if the field is not none

Minor Suggestions:

• Simple serializers not in the files matching their name
• Might want to move API to separate direction but that can probably wait or just not be done
• router.py should be urls.py
• using a nested router for actuele-prijzen I think?
• (Some) validation can be moved to validators
• Could use DRF status enum
• Some better ordering in prijs update

I will not be here next week, so I cannot review changes for a while

[Coperh]

Is this still the case?

[Coperh]

Yes. Is there a reason why?

[Floris272]

Bestand needs a special serializer to be able to upload files which I not gotten working.

[Coperh]

This seems wrong to me or at least the URL.

What if in the future you want prices but for one product type?
Should it not be all producttypen/alles-actuele-prijzen/
or is producttypen/actuele-prijzen/ for all and producttypen/<id>/actuele-prijzen/ for one fine?

Not too familiar with API design TBH. Might be something you want to ask in slack

[Floris272]

Added producttypen/<id>/actuele-prijs. This also seems to be the correct way in drf using @action(detail=True)

[Coperh]

prijs_optie = PrijsOptieFactory.create(prijs=prijs)

if you delete prijs_optie, its id will be set to -1 which might be useful. Not sure if it works with custom ids though

[Floris272]

Should be Changed (or can be changed quickly):

Formatted translation might cause issues
Useself.get_queryset()instead of the model
use fields = (.....) instead of exclude = (.....) ==> easier to understand and removes unexpected fields
Reverses instead of hard coded URLS

Could Be Changed:

Should use self.client get/post/patch/put instead of BaseApiTestCase methods Kept BaseApiTestCase for client login
Test that fields are actually set on basic creation tests
Perform database stuff after validation i.e. do not perform if not validated
Related to the last: only test for errors if the field is not none

Minor Suggestions:

Simple serializers not in the files matching their name -> what to do about circular dependency?
Might want to move API to separate direction but that can probably wait or just not be done Will be done in future pr
router.py should be urls.py
using a nested router for actuele-prijzen I think? -> added detail variant producttypen/<id>/actuele-prijs
(Some) validation can be moved to validators
Could use DRF status enum
Some better ordering in prijs update

• Moved clean validation to seperate method that is called inside the model.clean and inside a Validator on the serializer.
• replaced Treebeard with an FK to self for onderwerp