Skip to content
This repository has been archived by the owner on Jun 10, 2024. It is now read-only.

models mandatory video_data field #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

models mandatory video_data field #1

wants to merge 1 commit into from

Conversation

stereoit
Copy link

I had an issue with user just pasting url, e.g. https://www.youtube.com/watch?v=gDLKwTdurxg and then clicking "save" without anything else. It basically does not process the video and after save the plugin fails on:

Exception Value:    
type object argument after ** must be a mapping, not NoneType
Exception Location: /usr/lib/python2.7/site-packages/djangocms_youtube/models.py in video, line 76

Which really users vide_data as kwargs:

    @property
    def video(self):
        cls = Video(**self.video_data)
        return cls

It is enough for me to mark this field as mandatory. Probably updated migration is needed, but I have not idea how to do it here on GH.

I had an issue with user just pasting url, e.g. https://www.youtube.com/watch?v=gDLKwTdurxg and then clicking "save" without anything else. It basically does not process the video and after save the plugin fails on:

```
Exception Value:	
type object argument after ** must be a mapping, not NoneType
Exception Location:	/usr/lib/python2.7/site-packages/djangocms_youtube/models.py in video, line 76
```

Which really users `vide_data` as kwargs:
```
    @Property
    def video(self):
        cls = Video(**self.video_data)
        return cls
```

It is enough for me to mark this field as mandatory. Probably updated migration is needed, but I have not idea how to do it here on GH.
@mishbahr
Copy link
Owner

Thanks for your pull request.

Ans as you have mentioned, model changes requires migrations for for both djnago and south.

I can't merge the the changes as it is, sorry! Please add migrations or I'll try to do it in the master branch as soon as I can.

Regards

Mishbah

@stereoit
Copy link
Author

Can you give me hint how to actually do it? Do I need to copy the project
to my django-app and run migrations from there and then copy back? And how
to keep south migration and django migrations in sync?

Robert

On Wed, Jan 20, 2016 at 4:25 PM, Mishbah Razzaque [email protected]
wrote:

Thanks for your pull request.

Ans as you have mentioned, model changes requires migrations for for both
djnago and south.

I can't merge the the changes as it is, sorry! Please add migrations or
I'll try to do it in the master branch as soon as I can.

Regards

Mishbah


Reply to this email directly or view it on GitHub
#1 (comment)
.

stereoIT s.r.o.
Heřmanova 23
107 00 Praha 7
mob: +420 776 76 23 78

@mishbahr
Copy link
Owner

You'll have to setup 2 projects (django 1.6.x with South and django >=1.7), fork this repo and pip install locally (see https://pip.pypa.io/en/stable/reference/pip_install/#editable-installs)

@mishbahr
Copy link
Owner

Alternatively, please wait few days and I'll try to fix is as soon as I can.

@stereoit
Copy link
Author

Hi,

I've tried to setup local development, but installing the editable version,
brings django.1.9.2 and when I downgrade, the sample app starts to complain
quite a lot about missing dependencies. Seems I won't be able to push those
changes, can you kidnly do it?

Regards,

Robert

On Wed, Jan 20, 2016 at 4:59 PM, Mishbah Razzaque [email protected]
wrote:

Alternatively, please wait few days and I'll try to fix is as soon as I
can.


Reply to this email directly or view it on GitHub
#1 (comment)
.

stereoIT s.r.o.
Heřmanova 23
107 00 Praha 7
mob: +420 776 76 23 78

@leesolway
Copy link

Caught me out, breaking projects.

@mishbahr
Copy link
Owner

mishbahr commented Jun 14, 2016

Ideally I would like to keep video_data field optional .. change internal logic to return empty strings or sensible data e.g duration = 0 sec when video data is not available.

Unfortunately I'm super busy ... to make the suggested amends myself.. so pull requests are welcome!

@aidan-doherty
Copy link

Just got the same error can the pull request be merged to master yet ?

@mishbahr
Copy link
Owner

This merge request is not complete .. missing migrations for both django and south.

@jkaikko
Copy link

jkaikko commented Oct 11, 2016

Just a heads up to anyone experiencing this. I think the core problem might be that the google api responds really slowly and by the time people click on the save button, the video data has not been fetched (and video_data field is empty). In my case, my server was also timing out before the api request finished. Increasing nginx timeout and just waiting for the request helped.

But, who has time to wait 2 minutes for the api request to finish everytime you add a youtube video...

@aidan-doherty
Copy link

@jkaikko Its not getting as slow as that for me but making the video data field unique prevents the save and stops it from breaking the page you are adding it to. I have just created my own local version of the app without south migrations and made the required fix until it is updated on this git repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants