rusackas commented on code in PR #37773:
URL: https://github.com/apache/superset/pull/37773#discussion_r3975793418


##########
superset/security/manager.py:
##########
@@ -4329,6 +4329,35 @@ def is_admin(self) -> bool:
 
     # temporal change to remove the roles view from the security menu,
     # after migrating all views to frontend, we will set 
FAB_ADD_SECURITY_VIEWS = False
+    def _skip_legacy_fab_password_view_registration(self) -> Callable[..., 
Any]:
+        original_add_view_no_menu = self.appbuilder.add_view_no_menu

Review Comment:
   Agreed, `_skip_legacy_fab_password_view_registration` could use a short 
docstring given how much it's doing under the hood.



##########
tests/integration_tests/core_tests.py:
##########
@@ -154,6 +158,14 @@ def assert_admin_view_menus_in(role_name, assert_func):
         assert_admin_view_menus_in("Alpha", self.assertNotIn)
         assert_admin_view_menus_in("Gamma", self.assertNotIn)
 
+    def test_legacy_fab_password_views_are_not_registered(self):

Review Comment:
   This one's already covered, 
`test_legacy_fab_password_views_are_not_registered` has `-> None` on line 144 
in the current diff. Resolving.



-- 
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]

Reply via email to