aminghadersohi commented on PR #40343: URL: https://github.com/apache/superset/pull/40343#issuecomment-4565823923
A few schema issues worth addressing before merge: **`ThemeFilter` missing `is_system*` columns** The `is_system`, `is_system_default`, and `is_system_dark` flags are the primary way to distinguish system themes from user-created ones. Without them in `ThemeFilter.col`, there's no way to ask "show me only user-created themes" or "which theme is the system default". Recommend adding all three to the `Literal`. **`ThemeInfo.json_data` should be `Dict[str, Any]` or documented as a JSON string** Currently typed as `str | None`, but the value is a serialized JSON blob containing token configuration. The LLM has to `json.loads()` it manually. Either parse it in `serialize_theme_object` (with a safe `try/except json.JSONDecodeError`) and type it as `Dict[str, Any] | None`, or at minimum add a field description making it explicit that the caller must parse the string. **`CssTemplateInfo` missing audit fields** The `CssTemplate` model has `AuditMixinNullable` (`created_by`, `changed_by`). The schema has timestamps (`created_on`, `changed_on`) but omits the user FKs. For consistency with `ChartInfo` and `DashboardInfo`, consider adding at least `created_by_name` and `changed_by_name`. **`CssTemplateFilter` and `ThemeFilter` don't allow filtering by `created_by_fk`** Low priority, but a common admin query pattern across the rest of the suite. -- 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]
