bito-code-review[bot] commented on code in PR #37773:
URL: https://github.com/apache/superset/pull/37773#discussion_r4042095557
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -4320,6 +4320,68 @@ def
test_request_loader_rejects_invalid_guest_token_before_bearer(
verify_jwt.assert_not_called()
[email protected](
+
"enable_legacy_password_views,enable_force_password_change,expected_registered",
+ [
+ (False, False, {"NonPasswordView"}),
+ (False, True, {"NonPasswordView", "ResetMyPasswordView"}),
+ (
+ True,
+ False,
+ {"NonPasswordView", "ResetMyPasswordView", "ResetPasswordView"},
+ ),
+ ],
+)
+def test_skip_legacy_fab_password_view_registration_keeps_forced_change_target(
+ app_context: None,
+ enable_legacy_password_views: bool,
+ enable_force_password_change: bool,
+ expected_registered: set[str],
+) -> None:
+ """Forced password changes require the self-service reset view."""
+ from flask import current_app
+ from flask_appbuilder.security.views import ResetMyPasswordView,
ResetPasswordView
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inline Imports Violate Rule</b></div>
<div id="fix">
`from flask import current_app` at 4342 duplicates the module-level import
on line 27, and the `flask_appbuilder.security.views` import has no
circular-dependency justification (this module already imports flask_appbuilder
at top level, lines 28-29). Repo rule requires module-level imports unless a
documented circular dependency exists; delete the flask re-import and hoist the
FAB view import.
</div>
</div>
<small><i>Code Review Run #147aa7</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/security/manager_test.py:
##########
@@ -4320,6 +4320,68 @@ def
test_request_loader_rejects_invalid_guest_token_before_bearer(
verify_jwt.assert_not_called()
[email protected](
+
"enable_legacy_password_views,enable_force_password_change,expected_registered",
+ [
+ (False, False, {"NonPasswordView"}),
+ (False, True, {"NonPasswordView", "ResetMyPasswordView"}),
+ (
+ True,
+ False,
+ {"NonPasswordView", "ResetMyPasswordView", "ResetPasswordView"},
+ ),
+ ],
+)
+def test_skip_legacy_fab_password_view_registration_keeps_forced_change_target(
+ app_context: None,
+ enable_legacy_password_views: bool,
+ enable_force_password_change: bool,
+ expected_registered: set[str],
+) -> None:
+ """Forced password changes require the self-service reset view."""
+ from flask import current_app
+ from flask_appbuilder.security.views import ResetMyPasswordView,
ResetPasswordView
+
+ class NonPasswordView:
+ pass
+
+ registered: list[str] = []
+
+ def add_view_no_menu(baseview: type[Any], *args: Any, **kwargs: Any) ->
type[Any]:
+ registered.append(baseview.__name__)
+ return baseview
+
+ fake_appbuilder = SimpleNamespace(add_view_no_menu=add_view_no_menu)
+ sm = SupersetSecurityManager.__new__(SupersetSecurityManager)
+ sm.appbuilder = fake_appbuilder
+
+ previous_config = {
+ "ENABLE_LEGACY_FAB_PASSWORD_VIEWS": current_app.config[
+ "ENABLE_LEGACY_FAB_PASSWORD_VIEWS"
+ ],
+ "ENABLE_FORCE_PASSWORD_CHANGE": current_app.config[
+ "ENABLE_FORCE_PASSWORD_CHANGE"
+ ],
+ }
+ current_app.config["ENABLE_LEGACY_FAB_PASSWORD_VIEWS"] = (
+ enable_legacy_password_views
+ )
+ current_app.config["ENABLE_FORCE_PASSWORD_CHANGE"] =
enable_force_password_change
+
+ original_add_view_no_menu: Callable[..., Any] =
fake_appbuilder.add_view_no_menu
+ try:
+ original_add_view_no_menu =
sm._skip_legacy_fab_password_view_registration()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead Redundant Assignment</b></div>
<div id="fix">
Line 4371 initializes `original_add_view_no_menu` from
`fake_appbuilder.add_view_no_menu`, but line 4373 immediately rebinds it to
`_skip_legacy_fab_password_view_registration()` return value before any read;
on the early-raise path the finally at 4380 restores a value identical to what
the namespace already holds. Drop the 4371 initializer and the now-unused
`Callable` import added on line 23.
</div>
</div>
<small><i>Code Review Run #147aa7</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/integration_tests/core_tests.py:
##########
@@ -120,11 +121,18 @@ def test_slice_endpoint(self):
resp = self.client.get("/slice/-1/")
assert resp.status_code == 404
- def test_admin_only_menu_views(self):
- def assert_admin_view_menus_in(role_name, assert_func):
+ def test_admin_only_menu_views(self) -> None:
+ def assert_admin_view_menus_in(
+ role_name: str, assert_func: Callable[[str, list[str]], None]
+ ) -> None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing docstrings on new test</b></div>
<div id="fix">
New test method `test_admin_only_menu_views` and its nested helper
`assert_admin_view_menus_in` carry no docstrings. BITO.md adaptive rule 12147
requires docstrings on all new Python functions, helpers, and test methods, and
rule 12148 covers new test functions specifically. Adding one-line docstrings
keeps the new typing-heavy signature consistent with the org documentation
standard.
</div>
</div>
<small><i>Code Review Run #147aa7</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/integration_tests/core_tests.py:
##########
@@ -133,6 +141,27 @@ 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) -> None:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing docstring on new test</b></div>
<div id="fix">
New test `test_legacy_fab_password_views_are_not_registered` has no
docstring. BITO.md adaptive rule 12148 mandates a docstring on every newly
added test function documenting purpose, scenario, and expected outcome. It is
especially useful here because the assertions branch on
`ENABLE_LEGACY_FAB_PASSWORD_VIEWS` and `ENABLE_FORCE_PASSWORD_CHANGE` rather
than asserting a single fixed outcome.
</div>
</div>
<small><i>Code Review Run #147aa7</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
