john-bodley commented on code in PR #26023: URL: https://github.com/apache/superset/pull/26023#discussion_r1399903967
########## superset/views/base.py: ########## @@ -380,7 +380,9 @@ def menu_data(user: User) -> dict[str, Any]: @cache_manager.cache.memoize(timeout=60) -def cached_common_bootstrap_data(user: User, locale: str) -> dict[str, Any]: +def cached_common_bootstrap_data( # pylint: disable=unused-argument + user: User, user_id: int | None, locale: str Review Comment: @jfrag1 sorry I missed that detail when initially glancing over the PR description. Is a third alternative to just pass in `user_id` instead of both `user` and `user_id`? ########## superset/views/base.py: ########## @@ -424,8 +426,9 @@ def cached_common_bootstrap_data(user: User, locale: str) -> dict[str, Any]: def common_bootstrap_payload(user: User) -> dict[str, Any]: + user_id = user.id if hasattr(user, "id") else None Review Comment: It seems like the FAB [`User`](https://github.com/dpgaspar/Flask-AppBuilder/blob/master/flask_appbuilder/security/sqla/models.py#L94-L167) class will always have the `id` attribute defined and thus simply passing in `user.id` on line 431 _should_ be safe. ########## superset/views/base.py: ########## @@ -424,8 +426,9 @@ def cached_common_bootstrap_data(user: User, locale: str) -> dict[str, Any]: def common_bootstrap_payload(user: User) -> dict[str, Any]: + user_id = user.id if hasattr(user, "id") else None Review Comment: Grokking through the [code](https://github.com/search?q=repo%3Aapache%2Fsuperset%20common_bootstrap_payload&type=code) it seems like this is always `g.user`, thus rather than passing around the global, it seems clearer to remove the `user` variable from said method and thus said logic simply becomes, ```python from superset.utils.core import get_user_id def common_bootstrap_payload() -> dict[str, Any]: return { **cached_common_bootstrap_data(get_user_id(), get_locale()), "flash_messages": get_flashed_messages(with_categories=True), } ``` -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
