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

Content type #6

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

Content type #6

wants to merge 6 commits into from

Conversation

henrinie
Copy link
Member

So I accidentally pushed to wrong remote, so my changes can also be seen in this repositorys branch content_type.
I updated the tests and they seem to pass.
I also made some styling changes, which makes comparing the changes a bit harder, sorry for that.

This change removes category and tag from TaggedVideo, and replaces them with content_type and object_id, therefore utilizing the contenttypes framework: https://docs.djangoproject.com/en/1.11/ref/contrib/contenttypes/
category is replaced by content_type, which can be used to identify Models. tag is replaced by object_id, which is the id of the object in the model of its content_type. So it is now possible to connect videos to objects.
The unique_together of TaggedVideo is changed to unique_together = (("content_type", "object_id"),)
There is one problem with migrations: It might be risky to move from the tag/category way to this new one. I tried to make migration that attempts to solve the best case scenario where category is named after a model. I am not sure if it will work, it is not tried and tested. I also noticed now that there might be potential for the migration failing due to uppercase letters in model names https://github.com/Signbank/signbank-video/blob/content_type/video/migrations/0003_auto_20170822_1023.py#L18-L19.

I added one new templatetag get_taggedvideo_for_object which can be used to get the TaggedVideo object specified for the object you supply. It is used in a template like this: {% get_taggedvideo_for_object object as taggedvideo %}.

There are some other changes too, I added a function videos() to TaggedVideo, it should return all Videos tagged to TaggedVideo, ordered by version.

  • TaggedVideoManager: I changed the way version numbers were bumped, this should fix the issue if some videos are deleted in-between.
  • Same has been done to TaggedVideo.revert()
  • TaggedVideo.video now returns the lowest version numbered video instead of version 0, perhaps this works better if for some reason version 0 does not exist.
  • I have changed the parameters in some template tags. Mostly because tag and category can be simply replaced with object.
  • I updated the example app so that these changes can be tested manually.

@stevecassidy
All in all this is not perfect, but if you think that some of these changes could work, we can work on them some more. I think that after these changes we could start preparing to move using signbank-video ourselves.

- REQUIRES MIGRATION WHICH WILL FAIL TO CONVERT PREVIOUS TAGGEDVIDEOS CATEGORY TO A CONTENTTYPE IF THE MODEL's NAME IS NOT THE SAME.
- Upload files for any object.
- Load TaggedVideo for an object in template.
- Get all videos of a TaggedVideo by referencing .videos.
- The example app is updated.
- Some changes to admin.
- Some things don't work yet.
- TaggedVideo.video now gives the lowest video with lowest version value instead of version=0.
- When deleting TaggedVideo.video, instead of changing every videos version number by -1, we now upgrade the whole list to prevent gaps.
- Changed videoplayer's video-element's ID from '{{id}}' to 'signbank-video{{id}}' to avoid possible overlaps with video element id's.
- Added param 'id' back into templatetag 'videoplayer'
- Added try-blocks into TaggedVideo poster_url and get_absolute_url, in case a Video does not exist for a TaggedVideo object.
- Made the example create a sample video/taggedvideo object to make it work.
- Added a SampleVideo to MEDIA
- Showing error messages in red in example app
@codecov-io
Copy link

codecov-io commented Aug 24, 2017

Codecov Report

Merging #6 into master will decrease coverage by 0.42%.
The diff coverage is 82.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
- Coverage   75.71%   75.29%   -0.43%     
==========================================
  Files          10       12       +2     
  Lines         280      340      +60     
==========================================
+ Hits          212      256      +44     
- Misses         68       84      +16
Impacted Files Coverage Δ
video/urls.py 100% <ø> (ø) ⬆️
video/convertvideo.py 45% <ø> (ø) ⬆️
video/admin.py 0% <0%> (ø) ⬆️
video/forms.py 100% <100%> (ø) ⬆️
video/migrations/0004_auto_20170822_1025.py 100% <100%> (ø)
video/migrations/0003_auto_20170822_1023.py 66.66% <66.66%> (ø)
video/models.py 82.65% <78.12%> (-4.1%) ⬇️
video/views.py 92.85% <85%> (-7.15%) ⬇️
video/templatetags/videotags.py 91.83% <89.74%> (+7.46%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64a4132...33b7111. Read the comment docs.

@stevecassidy
Copy link
Contributor

Thanks for this. One problem that I'm having right now might cause issues with this change and that is how to deal with homophones - where we want to link one video to many signs. Currently we do this by linking the video to the first in a chain of homophones and look back in the chain to find the sign with the video attached. I would rather have the video linked to each homophone separately.

Not sure how to achieve this without storing a reference to the video in the gloss object itself.

@henrinie
Copy link
Member Author

I don't completely understand what you mean with that.
Could it be solved by having a Homophone model that would have a m2m field to glosses?
Then connect the video to this Homophone object. https://docs.djangoproject.com/en/1.11/topics/db/examples/many_to_many/#many-to-many-relationships

@stevecassidy
Copy link
Contributor

We have the homophone relationship between glosses via the Relation model. The issue is that to find the video for a given sign, I might need to go through the homophone chain to find the sign that the video is attached to. This means I can't just say {% get_video_for object %} - or at least I need a method on the Gloss model that returns the gloss object that will have the video - that's more or less what we used to do.

@stevecassidy
Copy link
Contributor

Just realised we already have this method Gloss.get_video_gloss - that might solve the problem.

@henrinie
Copy link
Member Author

Okay good, I took a look on your Relation model and understand that part now, but I am not 100% sure about what exactly are homophones and if it matters is it the source or target. The Gloss.get_video_gloss seems to solve the issue though.

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.

3 participants