Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/5 verbruiksobjecten #31

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Floris272
Copy link
Contributor

@Floris272 Floris272 commented Jan 21, 2025

Changes

  • Adds verbruiksobject_schema to product_type
  • Adds verbruiksobject to product
  • Adds django-json-schema-model dependency repo

@Floris272 Floris272 linked an issue Jan 21, 2025 that may be closed by this pull request
@Floris272
Copy link
Contributor Author

Floris272 commented Jan 27, 2025

rebased to #36 so that it can be reviewed, will keep this pr drafted to make sure everything is added after #36 & #28

@Floris272 Floris272 force-pushed the feature/5-verbruiksobjecten branch from dd0a11e to a0c408c Compare January 29, 2025 09:45
@Floris272 Floris272 marked this pull request as ready for review January 29, 2025 09:48
@Floris272 Floris272 requested a review from annashamray January 29, 2025 09:48
@Floris272 Floris272 force-pushed the feature/5-verbruiksobjecten branch from a0c408c to 18ba95b Compare February 4, 2025 09:37
Copy link

@annashamray annashamray left a comment

Choose a reason for hiding this comment

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

My main question is about creating separate request for JSON schemas: I think it's excessive and not beneficial.

I don't see a lot of advantage of using django_json_schema_model for now, but I'm also not against it, we can use it and see how it will go

Comment on lines +221 to +222
except ValidationError:
raise ValidationError(

Choose a reason for hiding this comment

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

I don't quite understand why you catch and reraise the same error here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To generalize the error message, and link it to the field.

@@ -4,6 +4,7 @@
from django.db import models
from django.utils.translation import gettext_lazy as _

from django_json_schema_model.models import JsonSchema

Choose a reason for hiding this comment

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

I'll put here my questions about code for django_json_schema_model.models.JsonSchema:

  1. It looks like name field should be unique
  2. Why do you use Draft202012Validator in the clean() instead of using $schema keyword defined in the JSON schema?

Copy link
Contributor Author

@Floris272 Floris272 Feb 18, 2025

Choose a reason for hiding this comment

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

Instead of validate("{}", self.schema) you mean?

I felt like Draft202012Validator.check_schema(self.schema) was cleaner and it is also what is called within jsonschema.validate.


def validate_schema(self, schema):
try:
Draft202012Validator.check_schema(schema)

Choose a reason for hiding this comment

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

same question as above: Why not to use validator based on $schema?


class JsonSchemaSerializer(serializers.ModelSerializer):

schema = serializers.DictField()

Choose a reason for hiding this comment

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

Why is this line needed? Is including it in Meta not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spectacular will render the type of JsonField as any in the type of dictField as object.


class Meta:
model = JsonSchema
fields = "__all__"

Choose a reason for hiding this comment

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

Same comment as above: it's better to define serializer fields explicitly. For example here it's not obvious is PK included in the serializer

@@ -77,6 +79,17 @@ class ProductTypeSerializer(serializers.ModelSerializer):
links = NestedLinkSerializer(many=True, read_only=True)
bestanden = NestedBestandSerializer(many=True, read_only=True)

verbruiksobject_schema = JsonSchemaSerializer(read_only=True)
verbruiksobject_schema_id = serializers.PrimaryKeyRelatedField(

Choose a reason for hiding this comment

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

Is it correct that to create a producttype with json schema you need to make two requests?

  • create JSON schema
  • Use PK of the created schema and create a producttype with such PK

It doesn't look very convenient. Could you just make it in one request?
Moreover I don't think exposing PKs in the public API is a good idea

Choose a reason for hiding this comment

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

Also let's discuss JsonSchema model:
it has name field, which you use as a filter for its endpoint. But you never expose it in the Producttypen endpoint.
So how will user know what schema name to use as a filter

I think it would be better to use Json Schema name instead of PK here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was that a schema can be linked to multiple product types so because of this, it's created with a separate endpoint and added to a producttype using its id.

I'll make the name unique and use that as the identifier within the api, but i think two requests are always needed unless it becomes a 1 to 1 relation.

@Floris272 Floris272 force-pushed the feature/5-verbruiksobjecten branch from 18ba95b to d21583a Compare February 21, 2025 12:14
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 80.68182% with 17 lines in your changes missing coverage. Please review.

Project coverage is 84.75%. Comparing base (f00b267) to head (d21583a).
Report is 6 commits behind head on master.

Files with missing lines Patch % Lines
...rc/open_producten/producttypen/admin/jsonschema.py 0.00% 16 Missing ⚠️
...c/open_producten/producttypen/models/jsonschema.py 95.65% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
- Coverage   84.92%   84.75%   -0.18%     
==========================================
  Files         106      109       +3     
  Lines        2269     2355      +86     
  Branches      149      150       +1     
==========================================
+ Hits         1927     1996      +69     
- Misses        319      336      +17     
  Partials       23       23              

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

@Floris272
Copy link
Contributor Author

After discussing with Alex I decided to remove the package and move the JsonSchema model to open producten.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Product verbruiksobjecten
3 participants