bito-code-review[bot] commented on code in PR #41606:
URL: https://github.com/apache/superset/pull/41606#discussion_r3584612804


##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1062,6 +1062,312 @@ class UpdateDashboardResponse(BaseModel):
     )
 
 
+class ManageDashboardOwnersRequest(BaseModel):
+    """Request schema for explicit add/remove dashboard owner management.
+
+    Unlike ``update_dashboard``'s dropped ``owners`` field (a full-replacement
+    list with no safety guard, so an empty or partial list could silently
+    orphan a dashboard), this tool takes explicit add/remove operations and
+    rejects any change that would leave the dashboard with zero owners.
+
+    "Owners" here means USER-type entries in the dashboard's Subject-based
+    ``editors`` list (the ownership model apache/superset#38831 introduced,
+    replacing the legacy ``owners`` relationship). Any ROLE- or GROUP-type
+    editors already on the dashboard are left untouched by this tool.
+    """
+
+    identifier: int | str = Field(
+        ...,
+        description=(
+            "Dashboard ID (integer), UUID, or slug. Same identifier shape "
+            "accepted by ``get_dashboard_info``."
+        ),
+    )
+    add_owner_ids: List[int] = Field(
+        default_factory=list,
+        description=(
+            "User IDs to add as dashboard owners. Discover IDs with 
``find_users``."
+        ),
+    )
+    remove_owner_ids: List[int] = Field(
+        default_factory=list,
+        description=(
+            "User IDs to remove from dashboard owners. Rejected if it would "
+            "leave the dashboard with zero owners, or if an ID is not "
+            "currently an owner."
+        ),
+    )
+
+    @model_validator(mode="after")
+    def _validate_operations(self) -> "ManageDashboardOwnersRequest":
+        if not self.add_owner_ids and not self.remove_owner_ids:
+            raise ValueError(
+                "At least one of add_owner_ids or remove_owner_ids is 
required."
+            )
+        overlap: list[int] = sorted(
+            set(self.add_owner_ids) & set(self.remove_owner_ids)
+        )
+        if overlap:
+            raise ValueError(
+                "User IDs cannot appear in both add_owner_ids and "
+                f"remove_owner_ids: {overlap}."
+            )
+        return self
+
+
+class ManageDashboardOwnersResponse(BaseModel):
+    """Response schema for ``manage_dashboard_owners``."""
+
+    owners: List[SubjectInfo] = Field(
+        default_factory=list,
+        description=(
+            "Full list of USER-type editor subjects (dashboard owners) "
+            "after the operation. Any ROLE/GROUP-type editors on the "
+            "dashboard are not included here."
+        ),
+    )
+    dashboard_url: str | None = Field(None, description="URL to view the 
dashboard")
+    added_owner_ids: List[int] = Field(
+        default_factory=list,
+        description="User IDs actually added as owners by this call.",
+    )
+    removed_owner_ids: List[int] = Field(
+        default_factory=list,
+        description="User IDs actually removed from owners by this call.",
+    )
+    warnings: List[str] = Field(
+        default_factory=list,
+        description=(
+            "Non-fatal advisory messages, e.g. that a non-admin caller was "
+            "automatically re-added as owner after trying to remove "
+            "themselves."
+        ),
+    )

Review Comment:
   <div>
   
   
   <div id="suggestion">
   <div id="issue"><b>Extract duplicated error fields to base request 
class</b></div>
   <div id="fix">
   
   Found syntactic duplication in schemas.py. Error handling fields (`error`, 
`permission_denied`) and the `sanitize_error_for_llm_context` validator are 
duplicated across multiple request classes at lines 1145-1161, 1241-1257, and 
1345-1358. Recommend extracting to a base class to eliminate redundancy.
   </div>
   
   
   </div>
   
   
   
   
   <small><i>Code Review Run #871b93</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/mcp_service/dashboard/tool/test_manage_dashboard_owners.py:
##########
@@ -0,0 +1,408 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for the manage_dashboard_owners MCP tool.
+
+"Owners" are USER-type Subjects in the dashboard's ``editors`` list (the
+Subject-based model apache/superset#38831 introduced, replacing the legacy
+``owners`` relationship).
+"""
+
+from collections.abc import Iterator
+from unittest.mock import Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.subjects.types import SubjectType
+from superset.utils import json
+
+DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug"
+GET_OR_CREATE_USER_SUBJECT = 
"superset.subjects.utils.get_or_create_user_subject"
+POPULATE_SUBJECT_LIST = "superset.commands.utils.populate_subject_list"
+
+
[email protected]
+def mcp_server() -> object:
+    return mcp
+
+
[email protected](autouse=True)
+def mock_auth() -> Iterator[Mock]:
+    """Mock authentication for all tests in this module."""
+    with patch("superset.mcp_service.auth.get_user_from_request") as 
mock_get_user:
+        with patch("superset.security_manager.raise_for_editorship"):
+            mock_user = Mock()
+            mock_user.id = 1
+            mock_user.username = "admin"
+            mock_get_user.return_value = mock_user
+            yield mock_get_user
+
+
+def _mock_subject(id: int, user_id: int, label: str = "user") -> Mock:
+    subject = Mock()
+    subject.id = id
+    subject.type = SubjectType.USER
+    subject.user_id = user_id
+    subject.role_id = None
+    subject.label = label
+    subject.active = True
+    return subject
+
+
+def _mock_dashboard(
+    id: int = 42,
+    title: str = "Test Dashboard",
+    slug: str | None = "test-slug",
+    editors: list[Mock] | None = None,
+) -> Mock:
+    dashboard = Mock()
+    dashboard.id = id
+    dashboard.dashboard_title = title
+    dashboard.slug = slug
+    dashboard.editors = (
+        editors if editors is not None else [_mock_subject(100, 1, "admin")]
+    )
+    return dashboard
+
+
+class TestManageDashboardOwners:
+    @patch(POPULATE_SUBJECT_LIST)
+    @patch(GET_OR_CREATE_USER_SUBJECT)
+    @patch(DAO_GET)
+    @patch("superset.extensions.db.session")
+    @pytest.mark.asyncio
+    async def test_add_owner(
+        self,
+        mock_session: Mock,
+        mock_get: Mock,
+        mock_get_or_create: Mock,
+        mock_populate: Mock,
+        mcp_server: object,
+    ) -> None:
+        existing = _mock_subject(100, 1, "admin")

Review Comment:
   <div>
   
   
   <div id="suggestion">
   <div id="issue"><b>Extract duplicated mock setup code in tests</b></div>
   <div id="fix">
   
   Found syntactic duplication in test_manage_dashboard_owners.py. Mock setup 
code for owner tests is duplicated at lines 98-108 and 362-372. Recommend 
extracting to a shared fixture or helper function to reduce code duplication.
   </div>
   
   
   </div>
   
   
   
   
   <small><i>Code Review Run #871b93</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/mcp_service/dashboard/tool/test_manage_dashboard_certification.py:
##########
@@ -0,0 +1,283 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for the manage_dashboard_certification MCP tool."""
+
+from collections.abc import Iterator
+from unittest.mock import Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.utils import json
+
+DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug"
+
+
[email protected]

Review Comment:
   <div>
   
   
   <div id="suggestion">
   <div id="issue"><b>Extract shared test fixtures to reduce 
duplication</b></div>
   <div id="fix">
   
   Found syntactic duplication in test fixtures. The `mock_auth` fixture and 
`_mock_dashboard` helper are duplicated across 
test_manage_dashboard_certification.py (lines 32-49), 
test_manage_dashboard_owners.py (lines 40-57), and 
test_manage_dashboard_roles.py (lines 40-57). Consider extracting these into a 
shared fixtures module to improve maintainability.
   </div>
   
   
   </div>
   
   
   
   
   <small><i>Code Review Run #871b93</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



##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1062,6 +1062,312 @@ class UpdateDashboardResponse(BaseModel):
     )
 
 
+class ManageDashboardOwnersRequest(BaseModel):
+    """Request schema for explicit add/remove dashboard owner management.
+
+    Unlike ``update_dashboard``'s dropped ``owners`` field (a full-replacement
+    list with no safety guard, so an empty or partial list could silently
+    orphan a dashboard), this tool takes explicit add/remove operations and
+    rejects any change that would leave the dashboard with zero owners.
+
+    "Owners" here means USER-type entries in the dashboard's Subject-based
+    ``editors`` list (the ownership model apache/superset#38831 introduced,
+    replacing the legacy ``owners`` relationship). Any ROLE- or GROUP-type
+    editors already on the dashboard are left untouched by this tool.
+    """
+
+    identifier: int | str = Field(
+        ...,
+        description=(
+            "Dashboard ID (integer), UUID, or slug. Same identifier shape "
+            "accepted by ``get_dashboard_info``."
+        ),
+    )
+    add_owner_ids: List[int] = Field(
+        default_factory=list,
+        description=(
+            "User IDs to add as dashboard owners. Discover IDs with 
``find_users``."
+        ),
+    )
+    remove_owner_ids: List[int] = Field(
+        default_factory=list,
+        description=(
+            "User IDs to remove from dashboard owners. Rejected if it would "
+            "leave the dashboard with zero owners, or if an ID is not "
+            "currently an owner."
+        ),
+    )
+
+    @model_validator(mode="after")
+    def _validate_operations(self) -> "ManageDashboardOwnersRequest":
+        if not self.add_owner_ids and not self.remove_owner_ids:
+            raise ValueError(
+                "At least one of add_owner_ids or remove_owner_ids is 
required."
+            )
+        overlap: list[int] = sorted(
+            set(self.add_owner_ids) & set(self.remove_owner_ids)
+        )
+        if overlap:
+            raise ValueError(
+                "User IDs cannot appear in both add_owner_ids and "
+                f"remove_owner_ids: {overlap}."
+            )
+        return self
+
+
+class ManageDashboardOwnersResponse(BaseModel):
+    """Response schema for ``manage_dashboard_owners``."""
+
+    owners: List[SubjectInfo] = Field(

Review Comment:
   <div>
   
   
   <div id="suggestion">
   <div id="issue"><b>CWE-79: XSS via owner label</b></div>
   <div id="fix">
   
   Please add a `@field_validator('owners', mode='after')` on the `owners` 
field in `ManageDashboardOwnersResponse` to call 
`sanitize_for_llm_context(info.label, field_path=('owners', info.id))` for each 
`SubjectInfo`, filtering out any entries that sanitize to empty strings. (See 
also: [CWE-79](https://cwe.mitre.org/data/definitions/79.html))
   </div>
   
   
   </div>
   
   
   
   
   <small><i>Code Review Run #871b93</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/mcp_service/dashboard/tool/test_manage_dashboard_certification.py:
##########
@@ -0,0 +1,283 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for the manage_dashboard_certification MCP tool."""
+
+from collections.abc import Iterator
+from unittest.mock import Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.utils import json
+
+DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug"
+
+
[email protected]
+def mcp_server() -> object:
+    return mcp
+
+
[email protected](autouse=True)
+def mock_auth() -> Iterator[Mock]:
+    """Mock authentication for all tests in this module."""
+    with patch("superset.mcp_service.auth.get_user_from_request") as 
mock_get_user:
+        with patch("superset.security_manager.raise_for_editorship"):
+            mock_user = Mock()
+            mock_user.id = 1
+            mock_user.username = "admin"
+            mock_get_user.return_value = mock_user
+            yield mock_get_user
+
+
+def _mock_dashboard(
+    id: int = 42,
+    title: str = "Test Dashboard",
+    slug: str | None = "test-slug",
+    certified_by: str | None = None,
+    certification_details: str | None = None,
+) -> Mock:
+    dashboard = Mock()
+    dashboard.id = id
+    dashboard.dashboard_title = title
+    dashboard.slug = slug
+    dashboard.certified_by = certified_by
+    dashboard.certification_details = certification_details
+    return dashboard
+
+
+class TestManageDashboardCertification:
+    @patch(DAO_GET)
+    @patch("superset.extensions.db.session")
+    @pytest.mark.asyncio
+    async def test_set_certification(
+        self, mock_session: Mock, mock_get: Mock, mcp_server: object
+    ) -> None:
+        dash = _mock_dashboard()
+        mock_get.return_value = dash
+
+        async with Client(mcp_server) as client:
+            result = await client.call_tool(
+                "manage_dashboard_certification",
+                {
+                    "request": {
+                        "identifier": 42,
+                        "certified_by": "Data Platform Team",
+                        "certification_details": "Verified against 
source-of-truth.",
+                    }
+                },
+            )
+
+        assert dash.certified_by == "Data Platform Team"
+        assert dash.certification_details == "Verified against 
source-of-truth."
+        assert mock_session.commit.call_count >= 1
+        payload = json.loads(result.content[0].text)
+        # Response text is wrapped for LLM context (mirrors the read-path
+        # sanitization on DashboardInfo.certified_by), so check substring.
+        assert "Data Platform Team" in payload["certified_by"]
+        assert set(payload["changed_fields"]) == {
+            "certified_by",
+            "certification_details",
+        }
+
+    @patch(DAO_GET)
+    @patch("superset.extensions.db.session")
+    @pytest.mark.asyncio
+    async def test_set_certified_by_only(
+        self, mock_session: Mock, mock_get: Mock, mcp_server: object
+    ) -> None:
+        dash = _mock_dashboard(certification_details="Existing details")
+        mock_get.return_value = dash
+
+        async with Client(mcp_server) as client:
+            result = await client.call_tool(
+                "manage_dashboard_certification",
+                {"request": {"identifier": 42, "certified_by": "Analytics 
Guild"}},
+            )
+
+        assert dash.certified_by == "Analytics Guild"
+        assert dash.certification_details == "Existing details"
+        payload = json.loads(result.content[0].text)
+        assert payload["changed_fields"] == ["certified_by"]
+
+    @patch(DAO_GET)
+    @patch("superset.extensions.db.session")
+    @pytest.mark.asyncio
+    async def test_clear_with_empty_string(
+        self, mock_session: Mock, mock_get: Mock, mcp_server: object
+    ) -> None:
+        dash = _mock_dashboard(
+            certified_by="Old Team", certification_details="Old details"
+        )
+        mock_get.return_value = dash
+
+        async with Client(mcp_server) as client:
+            result = await client.call_tool(
+                "manage_dashboard_certification",
+                {
+                    "request": {
+                        "identifier": 42,
+                        "certified_by": "",
+                        "certification_details": "",
+                    }
+                },
+            )
+
+        assert dash.certified_by is None
+        assert dash.certification_details is None
+        payload = json.loads(result.content[0].text)
+        assert payload["certified_by"] is None
+        assert payload["certification_details"] is None
+
+    @patch(DAO_GET)
+    @pytest.mark.asyncio
+    async def test_no_fields_is_noop(self, mock_get: Mock, mcp_server: object) 
-> None:
+        dash = _mock_dashboard(certified_by="Existing")
+        mock_get.return_value = dash
+
+        async with Client(mcp_server) as client:
+            result = await client.call_tool(
+                "manage_dashboard_certification",
+                {"request": {"identifier": 42}},
+            )
+
+        assert dash.certified_by == "Existing"
+        payload = json.loads(result.content[0].text)
+        assert payload["changed_fields"] == []
+        assert any("No fields provided" in w for w in payload["warnings"])
+
+    @patch(DAO_GET)
+    @pytest.mark.asyncio
+    async def test_dashboard_not_found(
+        self, mock_get: Mock, mcp_server: object
+    ) -> None:
+        from superset.commands.dashboard.exceptions import 
DashboardNotFoundError
+
+        mock_get.side_effect = DashboardNotFoundError()
+
+        async with Client(mcp_server) as client:
+            result = await client.call_tool(
+                "manage_dashboard_certification",
+                {"request": {"identifier": 999999, "certified_by": "Team"}},

Review Comment:
   <div>
   
   
   <div id="suggestion">
   <div id="issue"><b>Extract duplicated test method for permission denied 
scenarios</b></div>
   <div id="fix">
   
   Found syntactic duplication across test files. The permission denied test 
scenario is duplicated in test_manage_dashboard_certification.py (lines 
177-198) and test_manage_dashboard_owners.py (lines 208-229). Consider 
extracting to a shared test utility or base class.
   </div>
   
   
   </div>
   
   
   
   
   <small><i>Code Review Run #871b93</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/mcp_service/dashboard/tool/test_manage_dashboard_roles.py:
##########
@@ -0,0 +1,320 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for the manage_dashboard_roles MCP tool.
+
+"Roles" are ROLE-type Subjects in the dashboard's ``viewers`` list (the
+Subject-based model apache/superset#38831 introduced, replacing the legacy
+``roles``/``DASHBOARD_RBAC`` relationship), gated by ``ENABLE_VIEWERS``.
+"""
+
+from collections.abc import Iterator
+from unittest.mock import Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.subjects.types import SubjectType
+from superset.utils import json
+
+DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug"
+SUBJECTS_FROM_ROLES = "superset.subjects.utils.subjects_from_roles"
+IS_FEATURE_ENABLED = "superset.is_feature_enabled"
+
+
[email protected]
+def mcp_server() -> object:
+    return mcp
+
+
[email protected](autouse=True)
+def mock_auth() -> Iterator[Mock]:
+    """Mock authentication for all tests in this module."""
+    with patch("superset.mcp_service.auth.get_user_from_request") as 
mock_get_user:
+        with patch("superset.security_manager.raise_for_editorship"):
+            mock_user = Mock()
+            mock_user.id = 1
+            mock_user.username = "admin"
+            mock_get_user.return_value = mock_user
+            yield mock_get_user
+
+
+def _mock_subject(id: int, role_id: int, label: str = "role") -> Mock:
+    subject = Mock()
+    subject.id = id
+    subject.type = SubjectType.ROLE
+    subject.user_id = None
+    subject.role_id = role_id
+    subject.label = label
+    subject.active = True
+    return subject
+
+
+def _mock_dashboard(
+    id: int = 42,
+    title: str = "Test Dashboard",
+    slug: str | None = "test-slug",
+    viewers: list[Mock] | None = None,
+) -> Mock:
+    dashboard = Mock()
+    dashboard.id = id
+    dashboard.dashboard_title = title
+    dashboard.slug = slug
+    dashboard.viewers = viewers if viewers is not None else []
+    return dashboard
+
+
+class TestManageDashboardRoles:
+    @patch(IS_FEATURE_ENABLED, return_value=True)
+    @patch(SUBJECTS_FROM_ROLES)
+    @patch(DAO_GET)
+    @patch("superset.extensions.db.session")
+    @pytest.mark.asyncio
+    async def test_add_role(
+        self,
+        mock_session: Mock,
+        mock_get: Mock,
+        mock_subjects_from_roles: Mock,
+        mock_flag: Mock,
+        mcp_server: object,
+    ) -> None:
+        new_subject = _mock_subject(200, 5, "Analyst")
+        dash = _mock_dashboard(viewers=[])
+        mock_get.return_value = dash
+        mock_subjects_from_roles.return_value = [new_subject]
+
+        async with Client(mcp_server) as client:
+            result = await client.call_tool(
+                "manage_dashboard_roles",
+                {"request": {"identifier": 42, "add_role_ids": [5]}},
+            )
+
+        mock_subjects_from_roles.assert_called_once_with([5])
+        assert dash.viewers == [new_subject]
+        assert mock_session.commit.call_count >= 1
+        payload = json.loads(result.content[0].text)
+        assert [r["id"] for r in payload["roles"]] == [200]
+        assert payload["added_role_ids"] == [5]
+        assert payload["viewers_enabled"] is True
+        assert payload["warnings"] == []

Review Comment:
   <div>
   
   
   <div id="suggestion">
   <div id="issue"><b>Missing database-error test for session.commit</b></div>
   <div id="fix">
   
   The commit at line 262 of `manage_dashboard_roles.py` is inside a 
`try/except SQLAlchemyError` block (lines 261-285) that performs rollback and 
returns a generic error — but this code path has no test coverage. Without it, 
a DB fault during commit silently fails to trigger rollback or produces an 
unhandled exception.
   </div>
   
   
   </div>
   
   
   
   
   <small><i>Code Review Run #dcfe00</i></small>
   </div><div>
   
   
   <div id="suggestion">
   <div id="issue"><b>Extract duplicated test setup helper function</b></div>
   <div id="fix">
   
   Found syntactic duplication in test_manage_dashboard_roles.py. The test 
setup and assertion pattern for `test_add_role` is duplicated at lines 88-105 
and 155-172. Similar patterns exist in other dashboard test files. Consider 
refactoring to use shared helper functions or parameterized tests.
   </div>
   
   
   </div>
   
   
   
   
   <small><i>Code Review Run #871b93</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]

Reply via email to