Signbank / signbank-video

Video handling for the Signbank sign language dictionary
Other
2 stars 2 forks source link

TaggedVideo: Use generic relations? #4

Open henrinie opened 7 years ago

henrinie commented 7 years ago

Would it make sense to make use of the contenttypes framework to handle relations? It would allow us have generic relations based on the contenttypes, like explained in https://docs.djangoproject.com/en/1.10/ref/contrib/contenttypes/#generic-relations Is there a requirement to have categories that are not models?

I guess this:

class TaggedVideo(models.Model):
    """A video that can be tagged with a category (eg. gloss, definintion, feedback)
    and a tag (eg. the gloss id) """

    objects = TaggedVideoManager()

    category = models.CharField(max_length=50)
    tag = models.CharField(max_length=50)

Could be changed to this:

class TaggedVideo(models.Model):
    """A video that can be tagged with a category (eg. gloss, definintion, feedback)
    and a tag (eg. the gloss id) """

    objects = TaggedVideoManager()

    content_type = models.ForeignKey(ContentType, on_delete=models.CASCADE)
    object_id = models.PositiveIntegerField() # The pk/id of the object
    content_object = GenericForeignKey('content_type', 'object_id')
    tag = models.CharField(max_length=50)

Would this kind of change be of any use?

Also if we implement #3 , don't we have to remove the 'unique_together' part?

class Meta:
    unique_together = (("category", "tag"),)
henrinie commented 7 years ago

@stevecassidy Have you notice this issue?

stevecassidy commented 7 years ago

@henrinie I did, I had a quick look but couldn't see any operational advantage to this approach. The use of string tags makes as few assumptions as possible about how the videos will be used, it would be possible to associate them with things other than models although right now I don't have a use case for this. Generic relations just seems like another way to achieve the same thing. I can't see an advantage.

On #3, I guess it would depend on the way that this was implemented. Currently versions are handled within the taggedvideo model.

henrinie commented 6 years ago

I've though about moving to use signbank-video in FinSL-signbank, and I've (so far) come to the conclusion that for this app to be generic enough ContentTypes framework should be used.

The reason for this is to be able to identify the object that is related to a TaggedVideo. If we'd use category and tag we would need to save a category for each object we want to associate with a TaggedVideo. This would require that we store a category within every class we want to associate videos with. Or then we would have to manually type a category in every place we want to use videos.

By using the ContentTypes framework we would be able to assign a video to any object without the object needing to know anything about the category. The category would be the objects ContentType. Then we should be able to create template tags simply by referencing the object we want to associate with a video in a template. Like {% upload_videos for object %}, {% get_videos for object %}, {% record_video for object %}, which could be used anywhere and they would load the videos or the forms needed for uploading them. The category can then be retrieved from the objects ContentType.

Please correct me if there are any flaws in that, or if the text is not clear enough.

stevecassidy commented 6 years ago

@henrinie thanks for this. The way i've used this i do have a method inside the object to get the videos for that object, hard-coding the category. It does sound like using ContentTypes would be a better solution. If you get the chance to implement this I'd be happy to see a pull request!

henrinie commented 6 years ago

@stevecassidy We are about to start planning the things we want to work on until the end of the year. If there seems to be enough time left for this, I will surely attempt to make a pull request, and if succesful, move to using this app.

henrinie commented 6 years ago

@stevecassidy Is signbank-video in production use currently? This is relevant because making a proper migration file for this change (category->content_type tag->object_id) that would change existing TaggedVideos to correct ContentTypes might be impossible to do. Probably possible for some, if their category is directly the model name.

henrinie commented 6 years ago

@stevecassidy I've started work on making signbank-video work with ContentTypes. So far I have changed quite a few things, but it will still take some more time to get something usable done.

These changes are most likely breaking for the previous category/tag system. I will most likely not provide a migration file that would sort the change, because it is quite hard to predict what categories and tags have been used, and how they would connect to ContentTypes.

When I create the pull request, it will most likely come without tests. This will be a prototype to see if you agree on the changes.

stevecassidy commented 6 years ago

Thanks @henrinie - we were just about to go into production with this so I'd rather avoid changing now, but I'll take a look at what you've done and see if I can use it. Would obviously be easier to migrate from the old system directly to this rather than going via the category/tag version.