bito-code-review[bot] commented on code in PR #21769:
URL: https://github.com/apache/superset/pull/21769#discussion_r3455127865
##########
tests/integration_tests/queries/saved_queries/commands_tests.py:
##########
@@ -57,9 +57,10 @@ def tearDown(self):
db.session.commit()
super().tearDown()
+ @patch("superset.security.manager.g")
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Dead mock parameter</b></div>
<div id="fix">
The `mock_security_g` parameter is never consumed by the test. The only
authorization check in the saved-queries export path is
`security_manager.can_access_all_queries()` in `SavedQueryFilter.apply()` (line
91 of `saved_queries/filters.py`), which reads `g.user` from
`superset.queries.saved_queries.filters.g` — the already-patched module. The
separate `superset.security.manager.g` patch is dead code; its `user` attribute
is never read by any called function.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- tests/integration_tests/queries/saved_queries/commands_tests.py
+++ tests/integration_tests/queries/saved_queries/commands_tests.py
@@ -57,9 +57,8 @@
db.session.commit()
super().tearDown()
- @patch("superset.security.manager.g")
@patch("superset.queries.saved_queries.filters.g")
- def test_export_query_command(self, mock_filter_g, mock_security_g):
- mock_filter_g.user = mock_security_g.user =
security_manager.find_user("admin")
+ def test_export_query_command(self, mock_filter_g):
+ mock_filter_g.user = security_manager.find_user("admin")
command = ExportSavedQueriesCommand([self.example_query.id])
```
</div>
</details>
</div>
<small><i>Code Review Run #0b9308</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-frontend/src/pages/SavedQueryList/index.tsx:
##########
@@ -589,6 +605,26 @@ function SavedQueryList({
),
paginate: true,
},
+ {
+ Header: t('Created by'),
+ key: 'created_by',
+ id: 'created_by',
+ input: 'select',
+ operator: FilterOperator.RelationOneMany,
+ unfilteredLabel: t('All'),
+ fetchSelects: createFetchRelated(
+ 'saved_query',
+ 'created_by',
+ createErrorHandler(errMsg =>
+ t(
+ 'An error occurred while fetching created_by values: %s',
+ errMsg,
+ ),
+ ),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing toast notification for created_by fetch
errors</b></div>
<div id="fix">
The `created_by` filter error handler calls `t()` directly without invoking
`addDangerToast`, meaning errors will be silent. Compare with the `database`
filter at line 548 which properly calls `addDangerToast(errMsg)`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset-frontend/src/pages/SavedQueryList/index.tsx
+++ b/superset-frontend/src/pages/SavedQueryList/index.tsx
@@ -615,10 +615,10 @@ export default function SavedQueryList({
fetchSelects: createFetchRelated(
'saved_query',
'created_by',
- createErrorHandler(errMsg =>
- t(
+ createErrorHandler(errMsg =>
+ addDangerToast(t(
'An error occurred while fetching created_by values: %s',
errMsg,
- ),
+ )),
),
user,
),
```
</div>
</details>
</div>
<small><i>Code Review Run #0b9308</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]
