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

Reusable plugins cannot have no article #1137

Open
SlyPerdrix opened this issue Aug 10, 2021 · 1 comment
Open

Reusable plugins cannot have no article #1137

SlyPerdrix opened this issue Aug 10, 2021 · 1 comment

Comments

@SlyPerdrix
Copy link
Contributor

I started to make test for models.baseplugins.py but I came across a problem

def test_reusable_plugins_related(self):
    a1 = Article.objects.create()
    a2 = Article.objects.create()

    p = ReusablePlugin.objects.create(article=a1)
    pk = p.pk
    p.articles.add(a2)
    p.save()

    self.assertEqual(a1, p.article)
    self.assertNotIn(a1, p.articles.all())

    p.article = None
    p.save() # <---- Raise a RelatedObjectDoesNotExist exception

    self.assertIsEqual(p.article, a2)

    a2.delete()
    with self.assertRaises(ReusablePlugin.DoesNotExist):
        ReusablePlugin.objects.get(pk=pk)

I feel like the intent of the presave signal isn't to raise an exception when instance.article is None.

@disable_signal_for_loaddata
def on_reusable_plugin_pre_save(**kwargs):
    # Automatically make the original article the first one in the added
    # set
    instance = kwargs["instance"]
    if not instance.article: # <---- Raise a RelatedObjectDoesNotExist exception
        articles = instance.articles.all()
        if articles.exists():
            instance.article = articles[0]

My guess is the following part in ReusablePlugin class didn't do anything or not anymore.

# The article on which the plugin was originally created.
# Used to apply permissions.
ArticlePlugin.article.on_delete = models.SET_NULL
ArticlePlugin.article.verbose_name = _("original article")
ArticlePlugin.article.help_text = _("Permissions are inherited from this article")
ArticlePlugin.article.null = True
ArticlePlugin.article.blank = True

According to the django documentation you cannot modified inherited fields without a proper abstract class, so the ReusablePlugin.article cannot be different than ArticlePlugin.article

I dont know how much of a hindrance it is for attachment plugin but we should either :

  • remove the part of the code that threat ReusablePlugin.article as if it has null==blank==True
  • make ArticlePlugin class abstract (with database change)
  • remove the inheritance of ReusablePlugin from ArticlePlugin (with database change)

keep in mind that it also mean that on_delete==models.CASCADE and if we keep the current code the feature "the attachment isnt deleted when the main Article is deleted" didnt work as intended i assume.

@benjaoming
Copy link
Member

I can imagine that forcing an article relation on a plugin is intended to ensure that a ArticlePlugin/ReusablePlugin instance is always associated with some Article.

What is the case that ReusablePlugin should satisfy when it has no article relations? Is it so that it keeps existing despite all usage (relations) being removed?

Maybe the easiest way is to create a new base abstract model, ReusableNullablePlugin and then refactor ReusablePlugin and ArticlePlugin so that a new abstract base class with everything except the non-nullable article fk field is created and inherited from and the nullable/not-nullable article fk is then explicit on the base models themselves? This way, no migrations and changes would be necessary for existing usage.

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