zephyring commented on PR #25043: URL: https://github.com/apache/superset/pull/25043#issuecomment-1687168178
> LGTM! I still think it would be best to eventually move this logic into the DAO layer, but this is a great way to get things working properly consistently with sqlalchemy events. > > To @craig-rueda's point, there are several existing tests prefixed `test_after_update_dataset` that test this functionality. I'm not sure any new tests are needed since this PR is pretty much just a refactor/improvement of this existing functionality, but it would probably be good to rename them to `test_before_update_dataset` to be consistent with naming agreed. Consolidating to domain's command layer is likely the better way of handling side effect like permission update, as there's existing patterns [here](https://github.com/apache/superset/blob/master/superset/databases/commands/update.py#L111C1-L133). But I would defer the problem to a larger refactoring until we all agree upon ideal state/target state of how permissions system should work in Superset. This PR is purely continuing with existing patterns but more on a fix side of thing. -- 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]
