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

More v2 endpoints [WIP] #1509

Closed
wants to merge 12 commits into from
Closed

More v2 endpoints [WIP] #1509

wants to merge 12 commits into from

Conversation

lastminutediorama
Copy link
Contributor

@lastminutediorama lastminutediorama commented May 13, 2024

WIP for all CRUD actions on PlanningArea and Scenario endpoints

Comment on lines +104 to +106
area_acres = serializers.SerializerMethodField()
permissions = serializers.SerializerMethodField()
role = serializers.SerializerMethodField()
Copy link
Contributor

Choose a reason for hiding this comment

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

if this serializer is only used for create (as the name shows), we don't need any readonly fields, like scenario count, latest_updates, created_at (auto populated by django) or any of the method fields.

configuration = ConfigurationSerializer()

def create(self, validated_data):
validated_data["user"] = self.context["user"] or None
if not hasattr(validated_data, "user") and hasattr(self.context, "user"):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in here you can take the direct route of accessing self.request.user.

@@ -21,9 +27,32 @@ class PlanningAreaViewSet(viewsets.ModelViewSet):
ordering_fields = ["name", "created_at", "scenario_count"]
filterset_class = PlanningAreaFilter

def create(self, request):
try:
body = json.loads(request.body)
Copy link
Contributor

Choose a reason for hiding this comment

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

Feed the data into the serializer. In the create serializer implement the validate_geometry method (check here https://www.django-rest-framework.org/api-guide/validators/#field-level-validation).

That method can call coerce_geojson.

No need to do this wrangling of data prior to feeding it to the serializer.

class CreatePASerializer(...):

    def validate_geometry(self, geojson_value):
        try:
            geometry = coerce_geojson(geojson_value)
            # very important to return the CONVERTED value here.
            return geometry
        except ...:
            raise serializers.ValidationError(...)

request_data["planning_area"] = kwargs.pop("planningarea_pk")
is_dirty = False

# TODO: can we rely on Serializers to determine what can/cant be here?
Copy link
Contributor

Choose a reason for hiding this comment

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

Of course.

Just create a serializer with the updatable fields and that's it. No custom code needed. Status can't be updated from the API, so I think this whole method is not necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I dont' think we do updates on scenario after creation. Only notes maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, in the current version, the code is only permitting a few field updates. I'm not sure we're using any of these, but I wanted to keep parity with our unit tests, so I attempted to reimplement what we had there.

@@ -91,6 +93,56 @@ class Meta:
geo_field = "geometry"


class CreatePlanningAreaSerializer(
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also implement a object level validation hook, to allow DRF to validate the whole instance. The individual validate_<field> methods you only have access to the value provided for <field>. But there is a way to validate multiple fields together and you will need that do implement the geometry size validation.

https://www.django-rest-framework.org/api-guide/serializers/#object-level-validation
https://www.django-rest-framework.org/api-guide/serializers/#field-level-validation

class CreatePASerializer(...):
    # ....
    def validate(self, data):
        # run the geometry validation we have on services. if it fails, raise serializers.ValidationError
        # run any other validations that depends on multiple fields

        # very important to return data
        return data

def get_serializer_class(self):
if self.action == "list":
return ListPlanningAreaSerializer
if self.action == "create":
Copy link
Contributor

Choose a reason for hiding this comment

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

Since all of our viewsets have multiple serializers, we can create a base class like so:

class MultipleSerializerMixin:
    serializer_classes = {}
    def get_serializer_class(self):
        try:
            return self.serializer_classes[self.action]
        except KeyError:
            return super().get_serializer_class()

# how to use

class MySerializer(serializers.ModelSerializer, MultipleSerializerMixin):

     serializer_classes = {
         "list": MyListSerializer,
         "create": MyCreateSerializer,
         "update": MyUpdateSerializer
     }
     serializer_class = MyDefaultSerializer

@george-silva
Copy link
Contributor

Please don't forget to squash these commits 🙏

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.

None yet

2 participants