sfirke commented on PR #39888: URL: https://github.com/apache/superset/pull/39888#issuecomment-5589023994
Rebased onto current master and addressed the listener concern directly rather than working around it in tests. CodeAnt was right about the mechanism: the listeners are attached once in `SupersetApp.sync_config_to_db` based on the flag value at that moment, and nothing in `superset/tags/models.py` re-checked it. So the flag was a boot-time switch for tag *writes* while being a runtime switch for the menu, the SPA routes, and export/import. That was invisible with the default off — no listener was ever attached — but flipping the default makes it real. Its recommendation was to hold the default until registration is dynamic. I went the other way: the three listeners that *create* tags now re-check the flag when they fire, which is a smaller change than making registration/unregistration dynamic and gets the same contract. `after_delete` deliberately keeps firing regardless — those listeners only remove `tagged_object` rows, and skipping that cleanup would orphan rows pointing at a deleted object whose id can later be reused. A nice side effect: `tests/integration_tests/tagging_tests.py` needs no changes at all. `@with_feature_flags(TAGGING_SYSTEM=False)` now genuinely exercises the flag instead of passing because no listener existed, so the fixture from the earlier revision is gone. Your `import_tag` mock is carried forward as its own commit. --- 🤖 _Drafted by Claude Code, co-signed by @sfirke._ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
