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

New entities #134

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

New entities #134

wants to merge 35 commits into from

Conversation

jimkang
Copy link
Contributor

@jimkang jimkang commented Feb 1, 2023

Adds the entities app and the new Entities model and corresponding serializer and view. Coming next: Tests, and permissions.

Copy link
Member

@mitchelljkotler mitchelljkotler left a comment

Choose a reason for hiding this comment

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

This is a good start, we can review more together tomorrow

config/urls.py Outdated
@@ -5,6 +5,7 @@
from django.urls import include, path
from django.views import defaults as default_views
from django.views.generic.base import RedirectView
from documentcloud.entities.views import EntityViewSet
Copy link
Member

Choose a reason for hiding this comment

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

This import is in the wrong section - running inv format should run black and isort - isort should properly sort imports

@@ -0,0 +1,3 @@
from django.contrib import admin

# Register your models here.
Copy link
Member

Choose a reason for hiding this comment

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

We probably want some sort of admin view for entities. Not top priority, an d I can help you configure this if needed

from documentcloud.core.fields import AutoCreatedField, AutoLastModifiedField
from wikidata.client import Client
from django.utils.translation import gettext_lazy as _
from .choices import EntityAccess
Copy link
Member

Choose a reason for hiding this comment

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

I prefer not to use relative imports - from documentcloud.entities.choices import EntityAccess



class Entity(models.Model):
wd_entity = None
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be an instance field instead of a class field

def __init__(self):
    self.wd_entity = None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will probably be fine as an instance field, but it will save some memory if every Entity instance shares a single Wikidata client, which contains nothing specific to any particular instance. Let me know what you think.

# A dictionary with language codes as keys.
wikipedia_url = models.JSONField()
owner = models.ForeignKey(
"users.User", related_name="entities", on_delete=models.CASCADE
Copy link
Member

Choose a reason for hiding this comment

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

Users should never be deleted, but I would change this to on_delete=models.PROTECT, which would prevent a user from being deleted if they have an entity

# entities = serializers.PrimaryKeyRelatedField(
# many=True, queryset=Entity.objects.all()
# )
# Use document's serializer
Copy link
Member

Choose a reason for hiding this comment

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

The serializer needs some work - lets review it together and compare to some of the other serializers we have

permission_classes = [permissions.IsAuthenticatedOrReadOnly]

def perform_create(self, serializer):
serializer.save(owner=self.request.user)
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to think about if public entities should have an owner at all



@api_view(["GET"])
def api_root(request, format=None):
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed

_("access"),
choices=EntityAccess.choices,
help_text=_("Designates who may access this entity."),
)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a JSONField for metadata?

from rest_framework.reverse import reverse


class EntityViewSet(viewsets.ModelViewSet):
Copy link
Member

Choose a reason for hiding this comment

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

I think we want a way to get entities by wikidata_id and name as well as by their internal ID

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.

2 participants