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

TaggedVideo: Use generic relations? #4

Open
henrinie opened this issue Mar 9, 2017 · 8 comments
Open

TaggedVideo: Use generic relations? #4

henrinie opened this issue Mar 9, 2017 · 8 comments

Comments

@henrinie
Copy link
Member

henrinie commented Mar 9, 2017

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)
  • 'category' would be replaced with 'content_type'.
  • 'object_id' would refer to the pk/id of the object of the selected 'content_type'.
  • 'content_object' would have a relation to the object that is currently referred with 'category' and 'tag'.
  • 'category' and 'tag' would be removed.

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
Copy link
Member Author

henrinie commented May 9, 2017

@stevecassidy Have you notice this issue?

@stevecassidy
Copy link
Contributor

@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
Copy link
Member Author

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
Copy link
Contributor

@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
Copy link
Member Author

@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
Copy link
Member Author

@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
Copy link
Member Author

@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
Copy link
Contributor

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.

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

No branches or pull requests

2 participants