villebro commented on PR #38831: URL: https://github.com/apache/superset/pull/38831#issuecomment-4909458854
> A few non-blocking follow-ups that might be worth considering: > > 1. **`superset/subjects/utils.py:101`** — The embedded guest branch in `get_current_user_subject_ids` doesn't seem to be covered by tests. It might be worth adding one to avoid regressions. > 2. **Migration UUID backfill** — The backfill performs one `UPDATE` per subject. Would batching these updates improve performance on larger installations? > 3. **`UPDATING.md`** — Since viewers are now always seeded from `dashboard_roles`, it may be worth documenting the upgrade impact for installations with stale `dashboard_roles` data. > 4. **`superset/dashboards/schemas.py:590`** — Old dashboard bundles silently drop `roles` on import. Would it make sense to map them to viewers or at least emit a warning? > > **Nits** > > * Remove `_apply_legacy` if it's no longer used. > * The `ENABLE_VIEWERS` branches in `reports/filters.py` appear identical—could they be simplified? > * `_get_user_subject_id` swallows all exceptions. Would logging a warning help with debugging? > * `ReportModal.userId` appears unused and could potentially be removed. Thanks for the new review pass! I addressed the other bits, but I'd like to push back on 4: imports/exports are not expected to be compatible across major versions, and when this feature ships in 7.0, exports will not contain dashboard roles, making the role-to-viewer compatibility mapping a no-op. -- 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]
