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

Cannot delete flatpages in admin #47

Open
benkonrath opened this issue Nov 21, 2014 · 20 comments
Open

Cannot delete flatpages in admin #47

benkonrath opened this issue Nov 21, 2014 · 20 comments

Comments

@benkonrath
Copy link
Contributor

Here's the error:

IntegrityError at /admin/fluent_pages/page/1/delete/

update or delete on table "fluent_pages_urlnode" violates foreign key constraint "master_id_refs_id_10e5f581" on table "fluent_pages_htmlpage_translation"
DETAIL:  Key (id)=(1) is still referenced from table "fluent_pages_htmlpage_translation".

Here are the versions that I'm using:

git+https://github.com/edoburu/django-fluent-pages.git@eb7f28db695706adc2695d0b24a30a051ecade52#egg=django-fluent-pages
django-mptt==0.6.1
django-parler==1.2.1
django-polymorphic-tree==1.0.1
django-wysiwyg==0.7.0

Can you see something wrong with the versions of the dependencies I'm using?

@benkonrath
Copy link
Contributor Author

I'm using this with Django 1.6.8.

@RealBigB
Copy link

Same problem here with a custom page type derived from fluent_pages.models.Page

IntegrityError at /admin/fluent_pages/page/2/delete/
update or delete on table "fluent_pages_urlnode" violates foreign key constraint
"master_id_refs_id_03dcaa72" on table "cms_basehtmlpage_translation"
DETAIL: Key (id)=(2) is still referenced from table "cms_basehtmlpage_translation".

using :
Django==1.6.5
django-fluent-pages==0.9b4
django-mptt==0.6.1
django-parler==1.2.1
django-polymorphic==0.6
django-polymorphic-tree==1.0.1

Note that I don't have the problem on another projet using fluent_pages 0.9b2 (django-parler==1.0).

@vdboor
Copy link
Contributor

vdboor commented Dec 30, 2014

I got the same issue as well.
This is caused by the translation model on the HtmlPage model (created in f2f8319)

It's possible that the admin can't see the HtmlPageTranslation model, depending on which proxy class it looks at.

I'm considering to add an on_delete=models.CASCADE to the parler generated model.

@vinnyrose
Copy link
Contributor

Proxy models are not inspected for fields, as fields are not expected to be defined there. The field has to be defined onto a concrete model.

@vdboor
Copy link
Contributor

vdboor commented Jan 14, 2015

Ahh, thanks for that insight! This explains a lot.

vdboor added a commit that referenced this issue Jan 14, 2015
@vdboor
Copy link
Contributor

vdboor commented Jan 14, 2015

Looking at https://code.djangoproject.com/ticket/16128 and https://code.djangoproject.com/ticket/18491 there is a fix in Django 1.7. As quick workaround for Django 1.6 I've improved the delete() method in 959211c

This doesn't entirely fix it off course, but makes the site usable again!

@vdboor
Copy link
Contributor

vdboor commented Jan 19, 2015

This workaround/fix is now released as v0.9c1, making the latest beta versions usable again!

@benkonrath
Copy link
Contributor Author

Thanks. Should I close the issue?

@benkonrath
Copy link
Contributor Author

@vdboor Can we close this issue?

@vdboor
Copy link
Contributor

vdboor commented Aug 4, 2015

Sorry! Yes, lets close this and see when it comes up again.

@vdboor vdboor closed this as completed Aug 4, 2015
@vinnyrose
Copy link
Contributor

I'm in Django 1.8. I still have to use the following code to cascade delete a page and it's children. Even though the collector seems to be finding the translations on the delete confirmation page.

@receiver(pre_delete, sender=UrlNode)
def fix_delete(sender, instance, *args, **kwargs):
    model_class = instance.polymorphic_ctype.model_class()
    if issubclass(model_class, HtmlPage):
        HtmlPageTranslation = model_class.seo_translations.related.related_model
        HtmlPageTranslation.objects.filter(master=instance.pk).delete()

@jmurty
Copy link
Contributor

jmurty commented Oct 14, 2015

We have experienced similar issues.

It is likely related to Django core issues with finding and processing reverse FK relationships on proxy models, see https://code.djangoproject.com/ticket/23076 and https://code.djangoproject.com/ticket/18012

The Django issue has recently been fixed on the master branch, though we haven't yet been able to fully confirm it due to deadlines and since it doesn't really help us for now (we are using Django 1.7).

Has your pre-delete work-around been reliable @vinnyrose? We might do something similar.

@vinnyrose
Copy link
Contributor

It has been reliable in Django 1.6 and we tested today in Django 1.8 (skipped 1.7) with no problems. I don't know if 'related_model' is the correct attribute on the 'seo_translations' 'related' attribute, it might be 'model' in django 1.7.

@mrmachine
Copy link
Contributor

Yep. This is an issue for any model (not only translations) that has FKs to one of fluent's proxy models (e.g. Page or HtmlPage).

@vdboor any thoughts on how we might be able to work-around this in fluent, given that the fix won't be in Django until 1.10 and 1.8 is an LTS release?

Monkey patching the collector to inspect related descriptors on proxy models?

Or a pre_delete signal handler that is configurable (via settings) to either cascade or set null specific models that might be related to a UrlNode proxy model, or a default cascade behaviour for unspecified models?

The former would hopefully also display the full list of collected objects on the admin delete page. The latter would just silently cascade or set null on delete.

As this is actually not specifically a problem with fluent, but it does affect fluent, it would be nice if the workaround was in fluent-utils and was general enough to be adapted for use with any proxy models that might have FKs pointing at them.

And should we re-open this issue, or make a new issue?

@jmurty
Copy link
Contributor

jmurty commented Oct 14, 2015

FWIW we could provide a sample monkey-patch hack of Django's Collector that allows it to find and delete the proxy models' reverse FK relationships. As @mrmachine notes, this makes the pre-delete admin screen more accurate.

However we are wary about using this hack in production, so we are looking at less intrusive options.

@vdboor vdboor reopened this Oct 20, 2015
@vdboor
Copy link
Contributor

vdboor commented Oct 20, 2015

Yikes.

@mrmachine: I'd be ok with having a hack in fluent, consider it for "pre 1.9 Django compatibility". If the collector can be patched in a clean way, we might be able to ship it, but I'd rather not monkey patch Django silently from a thirdparty app.

@mrmachine
Copy link
Contributor

@vdboor We have a working monkey patch to Django's collector that we are applying in our project's AppConfig.ready() method. I was thinking about adding the patch and an AppConfig mixing class to fluent-utils along with some notes in the documentation about how users can enable the patch in their project via the mixin. This way we would not be silently patching Django, but would have a solution ready for users who have FKs to fluent's proxy models.

@mrmachine
Copy link
Contributor

Also, I think Django won't get this fix until the next release after 1.9, due to the timings of when it went in :(

@jpotterm
Copy link
Contributor

I agree, monkey patching should definitely be opt-in. The pre_delete signal handler @vinnyrose posted has been working well for us in production so far.

@vinnyrose
Copy link
Contributor

1.9 just went into beta, and the fix for these bugs was not merged in, so it won't make it in until 1.10 since these are not "release blocking". The AppConfig route sounds like a good idea, it fixes the pre-delete confirmation screen and sounds to be general enough to deal with any FK on_delete behavior. Unlike my pre_delete signal solution which only does CASCADE.

@mrmachine I would like to see the AppConfig solution if you can share it.

If it is possible, it would be convenient to have @mrmachine's mixin added to an alternative AppConfig in fluent_pages.apps itself i.e.:

class FluentAppCollectorPatchConfig(CollectorPatchMixin, FluentAppConfig):
    pass

Then users can specify the monkey-patching AppConfig in the INSTALLED_APPS settings, i.e. instead of "fluent_pages" they would put "fluent_pages.apps.FluentAppCollectorPatchConfig".

Perhaps for those that don't want to use the monkey patch the default FluentAppConfig can provide a ready method that sets up the pre_delete solution (the collector patch mixin will override the ready method)? It is possible to find all FK fields on a model, and then look at the on_delete behavior on those field and perform that action (i.e. no need for per-model-settings or a default behavior). We could cache the on_delete behaviors per model class per field at startup for better performance.

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 a pull request may close this issue.

7 participants