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

Convert relationships for Enclosure, Episode, Sites into M2M to allow mix and match show creation #16

Merged
merged 15 commits into from
Nov 28, 2014

Conversation

djangolackey
Copy link
Contributor

This is based on conversations in issue #15

update admin form to include enclosure on episode;update models to make slug unique in all cases because of the m2m relationships with shows and sites; update forms to include enclosure on the episode admin panel; update migration to include the model change for slug

modify model for enclosure to make episode a ManyToManyField rather than having a ForeignKey on the Episode. This will continue to allow Episodes to have Enclosures of various types and Enclosures to be used on multiple Episodes; modifiy migrations to match updates to form to validate that only one enclosure for each mimetype can be attached to a particular episode

corrections to the validation of the Enclosure model admin form and the migration to properly handle AutoSlug

update managers and views to use the plural form for sites and shows for the new ManyToMany field; update the model to use the first shows slug when there is more than one and the current show it not known

update the ordering of enclosures to include the url because the MimeType alone provides a fairly random list

fix typo in the migration script
update fork branch from rizumu repo
update fork branch from rizumu repo via master
@rizumu
Copy link
Owner

rizumu commented Sep 17, 2014

I pushed a fix to master to exclude the migrations directory from the linting. You can merge master in to your branch or cherry-pick that commit. Then you'll want to run flake8 --max-line-length=100 --exclude=docs/conf.py,migrations . to fix for the syntax errors, there aren't too many, mostly stripping whitespace.

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.31%) when pulling e146e45 on djangolackey:update_models into c35df6b on rizumu:master.

…PS setting for licenses rather the only looking for the ability to import licenses; this is because the migration will fail if the module is installed but not in the INSTALLED_APPS with an obsure error; also moved the creation of the ForeignKey to an Alter portion of the initial migration so the dependency is not created with License since it might not be installed
@coveralls
Copy link

Coverage Status

Coverage decreased (-5.58%) when pulling dd89206 on djangolackey:update_models into c35df6b on rizumu:master.

@djangolackey
Copy link
Contributor Author

It's interesting that it passes tests (or at least the build succeeds) on some newer versions of Django, but not others. I think we are down to actual test failures now. mostly where I've changed site to sites and episode to episodes and such.

Can I assume we are looking for success on all of the Travis environment tests? .

…se a model form without a fields declaration is deprecated, which makes this class purposeless; Update Forms to add duration to EnclosureForm so it can be used for AdminEnclosureForm; update tests to take into account the ManyToMany fields now present. All tests passing in Python 2.7 and Django 1.7
@coveralls
Copy link

Coverage Status

Coverage decreased (-2.06%) when pulling 21b743c on djangolackey:update_models into c35df6b on rizumu:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) when pulling a2a46fe on djangolackey:update_models into c35df6b on rizumu:master.

@djangolackey
Copy link
Contributor Author

It appears there is one failure on the Python 3.3 version of the django master branch. I'm stumped on that one really. The last commit fixed an issue with the str def that should have been an issue forever and always on Python 2.6 according to the interwebs.

So what do you think?

Thanks,

@rizumu
Copy link
Owner

rizumu commented Oct 1, 2014

@djangolackey sorry, somehow this slipped past me. I'll test it out locally and get back to you asap.

@rizumu
Copy link
Owner

rizumu commented Oct 1, 2014

@djangolackey I've tried to keep it as simple as possible, but I'm not able to get the migration working. The steps I've taken are:

  1. start with a fresh database using the master branch of rizumu/django-podcasting (I also have django-taggit, django-licenses and easy-thumbnails installed. Django 1.7 with postgres)
  2. switch to your branch and run ./mange.py migrate - see the traceback below

If it is working for you, could you please list the steps that you are taking? I'd like to get this merged, but if it is too much trouble to get the migrations working, we can look into doing a manual migration: ex. ./manage.py dumpdata podcasting --indent=4 --format=yaml > podcasting.yaml, drop the podcasting tables, create new tables from your branch, update the podcasting.yaml to reflect the db changes and finally do a ./manage.py loaddata podcasting.yaml.

Running migrations:
  Applying podcasting.0003_auto_20141001_1616...Traceback (most recent call last):
  File "./manage.py", line 12, in <module>
    execute_from_command_line(sys.argv)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/core/management/__init__.py", line 385, in execute_from_command_line
    utility.execute()
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/core/management/__init__.py", line 377, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/core/management/base.py", line 288, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/core/management/base.py", line 338, in execute
    output = self.handle(*args, **options)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/core/management/commands/migrate.py", line 160, in handle
    executor.migrate(targets, plan, fake=options.get("fake", False))
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/migrations/executor.py", line 63, in migrate
    self.apply_migration(migration, fake=fake)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/migrations/executor.py", line 97, in apply_migration
    migration.apply(project_state, schema_editor)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/migrations/migration.py", line 107, in apply
    operation.database_forwards(self.app_label, schema_editor, project_state, new_state)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/migrations/operations/fields.py", line 131, in database_forwards
    schema_editor.alter_field(from_model, from_field, to_field)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/backends/schema.py", line 509, in alter_field
    self._alter_field(model, old_field, new_field, old_type, new_type, old_db_params, new_db_params, strict)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/backends/schema.py", line 671, in _alter_field
    params,
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/backends/schema.py", line 98, in execute
    cursor.execute(sql, params)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/backends/utils.py", line 81, in execute
    return super(CursorDebugWrapper, self).execute(sql, params)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/utils.py", line 94, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/Users/rizumu/.virtualenvs/scenemachine/lib/python2.7/site-packages/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: column "license_id" cannot be cast automatically to type integer
HINT:  Specify a USING expression to perform the conversion.

@djangolackey
Copy link
Contributor Author

I'm sorry I've been out of pocket for a couple of weeks. There was an issue with how django-licenses is loaded. Depending on whether licenses is present or not you have to alter the model field from a varchar to a foreignkey. I'm testing another option where the field isn't created except through the check. I'll let you know when I complete my tests.

I've chosen not to depend on whether the app available on the path, but instead to check the settings.INSTALLED_APPS for it. This way even if it's available, it won't be used unless it's actually in the installed apps.

thanks,

…nstalled apps rather than creating it as a varchar and converting it to a foreignkey if the app is installed. This is because if the initial isn't run at database creation the license has to be converted, which forces migration to try and treat it as a varchar to foreignkey conversion that it can't accomplish
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.97%) when pulling 6fc84db on djangolackey:update_models into c35df6b on rizumu:master.

@djangolackey
Copy link
Contributor Author

I've pushed an update that changes the initial migration. Rather than setting the field as a varchar and then converting it to a foreignkey if the Licenses app is installed, I'm delaying the creation of that field until after other pieces are done and basing the creation on the presence of licenses in settings.INSTALLED_APPS. The benefit here is that the field doesn't have to be converted if the app exists, which appears to be where the error is coming from.

One thing to note, if a person installs django-podcasting and runs manage.py migrate without licensing in their INSTALLED_APPS and then later adds licensing to their INSTALLED_APPS, they will have a bad time. This is because the field will get created as a varchar field, which will not work with a ForeignKey. They will have to migrate the Field themselves to get it to work.

The only other option is to create both a varchar and a foreignkey (only if licenses is installed) and modify the forms.py to only show one of the fields in the admin depending on if licenses is present.

I'm not sure if you want to speak to this in the docs, or what.

Thanks,

@djangolackey
Copy link
Contributor Author

Latest push handles the initial migration differently for the license app. I've set up a postgres database and tested it there. Issue was the the migrations.RunPython doesn't actually handle the migrations.AddField functionality correctly. I needed to add the conditional for licenses in an different way.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.97%) when pulling 030428d on djangolackey:update_models into c35df6b on rizumu:master.

@rizumu
Copy link
Owner

rizumu commented Nov 18, 2014

@djangolackey I'll test this out in the next couple days and get it merged asap. Sorry for the delay.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.74%) when pulling 5713503 on djangolackey:update_models into c35df6b on rizumu:master.

@rizumu
Copy link
Owner

rizumu commented Nov 28, 2014

@djangolackey everything looks good. I'll need some time to update my custom forms on my project before I can start using it, but I'm going to merge this in and tag a new version since everything is now working nicely.

rizumu added a commit that referenced this pull request Nov 28, 2014
Convert relationships for Enclosure, Episode, Sites into M2M to allow mix and match show creation
@rizumu rizumu merged commit 3d595c0 into rizumu:master Nov 28, 2014
@djangolackey djangolackey deleted the update_models branch December 2, 2014 01:28
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