Skip to content

Draft: add rdmo.config app for Plugin model (plugin management)#1436

Open
MyPyDavid wants to merge 127 commits into2.5.0/releasefrom
add-config-app-for-plugin-model
Open

Draft: add rdmo.config app for Plugin model (plugin management)#1436
MyPyDavid wants to merge 127 commits into2.5.0/releasefrom
add-config-app-for-plugin-model

Conversation

@MyPyDavid
Copy link
Copy Markdown
Member

@MyPyDavid MyPyDavid commented Sep 30, 2025

Description

  • adds a rdmo.config app
    • with a Plugin model
    • the Plugin behaves similar to any other element (Catalog, Taks, View)
  • added a new setting PLUGINS = [ .. ] which replaces the legacy settings (e.g. PROJECT_IMPORT).

Deprecations and legacy

  • removed rdmo.core.plugins
    • moved Plugin base class to rdmo.config.plugins and renamed to PluginBase

Related issue: #1413

@MyPyDavid MyPyDavid self-assigned this Sep 30, 2025
Copy link
Copy Markdown
Member

@jochenklar jochenklar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Lets discuss in the meeting.

help_text=_('Designates whether this plugin is generally available for projects.')
)

class PluginType(models.TextChoices):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do without this, the Plugin(-class) should know this.

Copy link
Copy Markdown
Member Author

@MyPyDavid MyPyDavid Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now something like this as a utils.py function, to detect the plugin type from the class inheritance.

PLUGIN_BASES = {
    'optionset_provider': OptionsetProvider,
    'export': Export,
    'import': Import,
    'issue_provider': IssueProvider,
}

def detect_plugin_type(plugin_class):
    if not issubclass(plugin_class, Plugin):
        return "not_an_rdmo_plugin"

    for type_name, base_cls in PLUGIN_BASES.items():
        if issubclass(plugin_class, base_cls):
            return type_name
    return 'rdmo_plugin_unknown_type'

I kept it as a property in the Plugin model:

    @property
    def plugin_type(self) -> str:
        try:
            plugin_class = self.get_class()
        except Exception as e:
            return e.__class__.__qualname__.lower()
        return detect_plugin_type(plugin_class)

@MyPyDavid MyPyDavid changed the title Draft: add config app for Plugin model Draft: add rdmo.config app for Plugin model Oct 9, 2025
@MyPyDavid
Copy link
Copy Markdown
Member Author

MyPyDavid commented Oct 15, 2025

I'm getting some strange DB errors for mysql in ci(and sqlite in local testing).
In my local pytest with sqlite:

django.db.utils.OperationalError: foreign key mismatch - "questions_question" referencing "domain_attribute"

From the ci logs:

# at
query = b'ALTER TABLE `domain_attribute` DROP COLUMN `attributeentity_ptr_id`'
 django.db.utils.OperationalError:
  (1829, "Cannot drop column 'attributeentity_ptr_id': 
 needed in a foreign key constraint 'questions_questionse_attribute_id_30ee3ea9_fk_domain_at' of table 'questions_questionset'")

The problem can be resolved by removing the app 'rdmo.config' from the installed apps again..
Trying to see it is a bug somewhere in the migrations (0037_remove_attribute.py) but can't really find it so far..

@MyPyDavid
Copy link
Copy Markdown
Member Author

I'm getting some strange DB errors for mysql in ci(and sqlite in local testing). In my local pytest with sqlite:

django.db.utils.OperationalError: foreign key mismatch - "questions_question" referencing "domain_attribute"

Think that I found the bug with help of GPT and could fix it by adding a single dependency to the ('domain', '0038_rename_attributeentity_to_attribute') in the questions.migrations.0032_meta.py file.

GPT:

You’ve got a classic multi-table-inheritance gotcha: the questions app points a FK at the child table’s primary key just before the domain app drops that child PK. SQLite complains with a vague “foreign key mismatch,” while MySQL is blunt: you can’t drop attributeentity_ptr_id on domain_attribute because it’s referenced by questions_questionset / questions_question.

Why this happens (in your tree)

  • In domain 0037, the child model Attribute (multi-table inheritance) is dismantled: it first adds a dummy column, then drops the attributeentity_ptr field (which is the PK of the child table!), and finally deletes the model.
  • Immediately after, domain 0038 renames the base class AttributeEntity → Attribute, so from here on “Attribute” refers to the base table (with a normal id PK), not the child.
  • In the questions app, migration 0031 renames the field attribute_entity → attribute, and 0032 makes that FK point to to='domain.Attribute'. At the time 0032 runs (without extra dependencies), “domain.Attribute” still means the child table (pre-rename), so the FK targets the child table’s PK (attributeentity_ptr_id).
  • Then domain 0037 tries to drop attributeentity_ptr on domain_attribute, but the FK from questions still points to that column → MySQL error 1829; SQLite “FK mismatch”.
    You also noticed removing rdmo.config “fixes” it. That’s consistent: the extra app changes the topological order so domain 0037 sneaks in before questions flips its FKs to the renamed/base model, exposing the ordering bug.

@MyPyDavid MyPyDavid changed the title Draft: add rdmo.config app for Plugin model Draft: add rdmo.config app for Plugin model Oct 21, 2025
@jochenklar
Copy link
Copy Markdown
Member

Ok got it! Good explanation by ChatGPT. This is why you don't do multi-table inheritance and why past-Jochen dismantled it.

@MyPyDavid MyPyDavid linked an issue Oct 27, 2025 that may be closed by this pull request
@jochenklar jochenklar modified the milestones: RDMO 2.4.0, RDMO 2.5.0 Oct 30, 2025
@MyPyDavid MyPyDavid force-pushed the add-config-app-for-plugin-model branch from 107f379 to f5eae22 Compare November 7, 2025 13:08
@MyPyDavid
Copy link
Copy Markdown
Member Author

MyPyDavid commented Nov 14, 2025

  • need to rebase to 2.4.0 now
    PS it ran without any conflicts

@MyPyDavid MyPyDavid force-pushed the add-config-app-for-plugin-model branch from 7031a4b to 6875f9a Compare November 14, 2025 14:52
@MyPyDavid MyPyDavid changed the base branch from 2.3.3 to 2.4.0 November 14, 2025 14:52
@MyPyDavid MyPyDavid force-pushed the add-config-app-for-plugin-model branch from 6875f9a to 9791229 Compare November 19, 2025 08:16
@MyPyDavid MyPyDavid changed the base branch from 2.4.0 to 2.5.0 November 19, 2025 08:17
@MyPyDavid MyPyDavid force-pushed the add-config-app-for-plugin-model branch from dceca19 to ae887db Compare November 24, 2025 14:45
return self.filter(Q(groups=None) | Q(groups__in=groups))


class ForSiteQuerySetMixin:
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could implement this already in #1488, see #1488 (comment)

@MyPyDavid MyPyDavid force-pushed the add-config-app-for-plugin-model branch from 27516be to 4026f9a Compare December 5, 2025 16:12
@MyPyDavid MyPyDavid changed the title Draft: add rdmo.config app for Plugin model Draft: add rdmo.config app for Plugin model (plugin managament) Dec 9, 2025
@MyPyDavid MyPyDavid marked this pull request as ready for review December 9, 2025 12:23
@MyPyDavid MyPyDavid requested a review from jochenklar December 9, 2025 12:23
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
def __str__(self):
return self.uri

def save(self, *args, **kwargs):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe create instance of plugin here an read plugin_type from the class. Handle raw.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure that the raw case needs to be handled? I could not find any other model that does it on the save method, only the signal handlers are using it so far..
Otherwise it could be simply

    if raw:
        return super().save(*args, **kwargs)

right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right, no raw needed. Fixtures use a bulk insert.

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
@MyPyDavid MyPyDavid changed the title Draft: add rdmo.config app for Plugin model (plugin managament) Draft: add rdmo.config app for Plugin model (plugin management) Jan 12, 2026
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
…rsion

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>
Copy link
Copy Markdown
Member

@jochenklar jochenklar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work. I added a lot of comments, and fixed some things already in #1574.

for plugin in Plugin.objects.filter(plugin_type='optionset_provider').exclude(url_name='')
}

if not providers:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is always empty, why should Plugin have any rows when running the migration?

@@ -85,16 +86,27 @@ def get_context_data(self, **kwargs):
context['ancestors_import'] = ancestors_import
context['memberships'] = memberships.order_by('user__last_name', '-project__level')
context['integrations'] = integrations.order_by('provider_key', '-project__level')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we need to do something about existing integrations in the database, which do not have a configured plugin (anymore).

verbose_name=_('Plugin settings'),
help_text=_('Contains the settings for this plugin in JSON format.'),
)
plugin_type = models.SlugField(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be restricted to PLUGIN_TYPES, right? I see that's in the serializer and in admin, but this could be a regular choices arg.

return self.filter(queryset)


def for_context(self, project=None, plugin_type=None, plugin_types=None,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should refactor this after the rebase and use filter_tasks_or_views_for_project since it is basically the same.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin specific part can be a filter afterwards.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order needs also be applied, since it matters when two plugins accept the same files for upload.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter_element_for_project?

return (
self
.filter_for_site(project.site)
.filter(catalogs=project.catalog)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be .filter(models.Q(catalogs=None) | models.Q(catalogs=project.catalog)).

python_path = declared.get("python_path")
restore_plugins = None
if python_path and python_path not in settings.PLUGINS:
restore_plugins = list(settings.PLUGINS)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PLUGINS is a list, if not it should crash.

# If no import source and only --clear, we are done after clear phase.
# If no import source and no clear, error.
if not plugins_from_settings and not clear:
raise CommandError("Nothing to do.")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not an error. I don't know if there is a CommandInfo. Maybe just a self.stdout.write and return?

raise CommandError("\n".join(e.messages)) from e

plugins_from_settings.extend(
merge_legacy_and_current_plugins(legacy_plugins, plugins)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the script should not merge, but first insert the regular plugins and then read and insert the legacy plugins and skipping the ones that are already in the database (by python path).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is still to complex, please remove all unnecessary code and comments. Remove all typing. The management class should be at the top of file for readability. All methods (if they are needed) should be in Command.

Also I don't get the part about the database transactions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the script should be split in two, one for the legacy settings, and one for the future regular RDMO setup. I guess that would make things easier.

@MyPyDavid
Copy link
Copy Markdown
Member Author

thanks! The #1574 also looks good, I'll try to go through the comments ASAP :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

config app: Allow managers to configure Plugin settings via GUI

3 participants