codeant-ai-for-open-source[bot] commented on code in PR #41133:
URL: https://github.com/apache/superset/pull/41133#discussion_r3507548007
##########
tests/unit_tests/charts/test_dashboard_filter_context.py:
##########
@@ -563,6 +563,178 @@ def
test_get_dashboard_filter_context_out_of_scope_filter_excluded(
assert ctx.filters[0].id == "f1"
+def _build_dashboard_mock(
+ mock_db: MagicMock,
+ filter_config: list[dict[str, Any]],
+ chart_ids: list[int],
+) -> MagicMock:
+ """Wire a dashboard MagicMock with the given filters and chart ids."""
+ metadata = {"native_filter_configuration": filter_config}
+ dashboard = MagicMock()
+ dashboard.id = 1
+ dashboard.slices = [MagicMock(id=cid) for cid in chart_ids]
+ dashboard.json_metadata = json.dumps(metadata)
+ dashboard.position_json = json.dumps(SAMPLE_POSITION_JSON)
+ (
+
mock_db.session.query.return_value.filter_by.return_value.one_or_none.return_value
+ ) = dashboard
+ return dashboard
+
+
+@patch("superset.charts.data.dashboard_filter_context._check_dashboard_access")
+@patch("superset.charts.data.dashboard_filter_context.db")
+def test_active_data_mask_overrides_default(
+ mock_db: MagicMock,
+ mock_check_access: MagicMock,
+) -> None:
+ """An active filter value replaces the saved default."""
+ filter_config = [
+ _make_filter(
+ flt_id="f1",
+ name="Region",
+ scope_root=["ROOT_ID"],
+ default_value=["US"],
+ target_column="region",
+ ),
+ ]
+ _build_dashboard_mock(mock_db, filter_config, [10])
+
+ active_data_mask = {
+ "f1": {
+ "extraFormData": {
+ "filters": [{"col": "region", "op": "IN", "val": ["FR", "DE"]}]
+ }
+ }
+ }
+ ctx = get_dashboard_filter_context(
+ dashboard_id=1, chart_id=10, active_data_mask=active_data_mask
+ )
+
+ assert ctx.filters[0].status == DashboardFilterStatus.APPLIED
+ assert ctx.extra_form_data["filters"][0]["val"] == ["FR", "DE"]
+
+
+@patch("superset.charts.data.dashboard_filter_context._check_dashboard_access")
+@patch("superset.charts.data.dashboard_filter_context.db")
+def test_active_data_mask_empty_clears_default(
+ mock_db: MagicMock,
+ mock_check_access: MagicMock,
+) -> None:
+ """An empty active extraFormData clears the filter; the default is NOT
used."""
+ filter_config = [
+ _make_filter(
+ flt_id="f1",
+ name="Region",
+ scope_root=["ROOT_ID"],
+ default_value=["US"],
+ target_column="region",
+ ),
+ ]
+ _build_dashboard_mock(mock_db, filter_config, [10])
+
+ active_data_mask: dict[str, Any] = {"f1": {"extraFormData": {}}}
+ ctx = get_dashboard_filter_context(
+ dashboard_id=1, chart_id=10, active_data_mask=active_data_mask
+ )
+
+ assert ctx.filters[0].status == DashboardFilterStatus.NOT_APPLIED
+ assert "filters" not in ctx.extra_form_data
+
+
+@patch("superset.charts.data.dashboard_filter_context._check_dashboard_access")
+@patch("superset.charts.data.dashboard_filter_context.db")
+def test_active_data_mask_absent_filter_falls_back_to_default(
+ mock_db: MagicMock,
+ mock_check_access: MagicMock,
+) -> None:
+ """A filter not present in the mask keeps its saved default."""
+ filter_config = [
+ _make_filter(
+ flt_id="f1",
+ name="Region",
+ scope_root=["ROOT_ID"],
+ default_value=["US"],
+ target_column="region",
+ ),
+ ]
+ _build_dashboard_mock(mock_db, filter_config, [10])
+
+ # Mask only references some other filter id
+ active_data_mask: dict[str, Any] = {"f2": {"extraFormData": {"filters":
[]}}}
+ ctx = get_dashboard_filter_context(
+ dashboard_id=1, chart_id=10, active_data_mask=active_data_mask
+ )
+
+ assert ctx.filters[0].status == DashboardFilterStatus.APPLIED
+ assert ctx.extra_form_data["filters"][0]["val"] == ["US"]
+
+
+@patch("superset.charts.data.dashboard_filter_context._check_dashboard_access")
+@patch("superset.charts.data.dashboard_filter_context.db")
+def test_active_data_mask_applies_despite_default_to_first_item(
+ mock_db: MagicMock,
+ mock_check_access: MagicMock,
+) -> None:
+ """
+ defaultToFirstItem filters cannot be resolved from saved config, but when
the
+ frontend supplies a concrete active value it is applied.
+ """
+ filter_config = [
+ _make_filter(
+ flt_id="f1",
+ name="City",
+ scope_root=["ROOT_ID"],
+ default_to_first_item=True,
+ target_column="city",
+ ),
+ ]
+ _build_dashboard_mock(mock_db, filter_config, [10])
+
+ active_data_mask = {
+ "f1": {
+ "extraFormData": {"filters": [{"col": "city", "op": "IN", "val":
["NYC"]}]}
+ }
+ }
Review Comment:
**Suggestion:** Add a concrete type hint to this new variable so the added
test code remains fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This added local mapping is unannotated even though it is a concrete
variable that can be typed. The type-hint rule applies here.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d746512c412b462894a1d01adb463852&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d746512c412b462894a1d01adb463852&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/test_dashboard_filter_context.py
**Line:** 693:697
**Comment:**
*Custom Rule: Add a concrete type hint to this new variable so the
added test code remains fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=3452506ae4ca7affe0c3a82f26c86968b53fc1a7fa757428c56c2a614e539d77&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=3452506ae4ca7affe0c3a82f26c86968b53fc1a7fa757428c56c2a614e539d77&reaction=dislike'>👎</a>
##########
tests/unit_tests/charts/test_dashboard_filter_context.py:
##########
@@ -563,6 +563,178 @@ def
test_get_dashboard_filter_context_out_of_scope_filter_excluded(
assert ctx.filters[0].id == "f1"
+def _build_dashboard_mock(
+ mock_db: MagicMock,
+ filter_config: list[dict[str, Any]],
+ chart_ids: list[int],
+) -> MagicMock:
+ """Wire a dashboard MagicMock with the given filters and chart ids."""
+ metadata = {"native_filter_configuration": filter_config}
+ dashboard = MagicMock()
+ dashboard.id = 1
+ dashboard.slices = [MagicMock(id=cid) for cid in chart_ids]
+ dashboard.json_metadata = json.dumps(metadata)
+ dashboard.position_json = json.dumps(SAMPLE_POSITION_JSON)
+ (
+
mock_db.session.query.return_value.filter_by.return_value.one_or_none.return_value
+ ) = dashboard
+ return dashboard
+
+
+@patch("superset.charts.data.dashboard_filter_context._check_dashboard_access")
+@patch("superset.charts.data.dashboard_filter_context.db")
+def test_active_data_mask_overrides_default(
+ mock_db: MagicMock,
+ mock_check_access: MagicMock,
+) -> None:
+ """An active filter value replaces the saved default."""
+ filter_config = [
+ _make_filter(
+ flt_id="f1",
+ name="Region",
+ scope_root=["ROOT_ID"],
+ default_value=["US"],
+ target_column="region",
+ ),
+ ]
+ _build_dashboard_mock(mock_db, filter_config, [10])
+
+ active_data_mask = {
+ "f1": {
+ "extraFormData": {
+ "filters": [{"col": "region", "op": "IN", "val": ["FR", "DE"]}]
+ }
+ }
+ }
+ ctx = get_dashboard_filter_context(
+ dashboard_id=1, chart_id=10, active_data_mask=active_data_mask
+ )
+
+ assert ctx.filters[0].status == DashboardFilterStatus.APPLIED
+ assert ctx.extra_form_data["filters"][0]["val"] == ["FR", "DE"]
+
+
+@patch("superset.charts.data.dashboard_filter_context._check_dashboard_access")
+@patch("superset.charts.data.dashboard_filter_context.db")
+def test_active_data_mask_empty_clears_default(
+ mock_db: MagicMock,
+ mock_check_access: MagicMock,
+) -> None:
+ """An empty active extraFormData clears the filter; the default is NOT
used."""
+ filter_config = [
+ _make_filter(
+ flt_id="f1",
+ name="Region",
+ scope_root=["ROOT_ID"],
+ default_value=["US"],
+ target_column="region",
+ ),
+ ]
+ _build_dashboard_mock(mock_db, filter_config, [10])
+
+ active_data_mask: dict[str, Any] = {"f1": {"extraFormData": {}}}
+ ctx = get_dashboard_filter_context(
+ dashboard_id=1, chart_id=10, active_data_mask=active_data_mask
+ )
+
+ assert ctx.filters[0].status == DashboardFilterStatus.NOT_APPLIED
+ assert "filters" not in ctx.extra_form_data
+
+
+@patch("superset.charts.data.dashboard_filter_context._check_dashboard_access")
+@patch("superset.charts.data.dashboard_filter_context.db")
+def test_active_data_mask_absent_filter_falls_back_to_default(
+ mock_db: MagicMock,
+ mock_check_access: MagicMock,
+) -> None:
+ """A filter not present in the mask keeps its saved default."""
+ filter_config = [
+ _make_filter(
+ flt_id="f1",
+ name="Region",
+ scope_root=["ROOT_ID"],
+ default_value=["US"],
+ target_column="region",
+ ),
+ ]
+ _build_dashboard_mock(mock_db, filter_config, [10])
+
+ # Mask only references some other filter id
+ active_data_mask: dict[str, Any] = {"f2": {"extraFormData": {"filters":
[]}}}
+ ctx = get_dashboard_filter_context(
+ dashboard_id=1, chart_id=10, active_data_mask=active_data_mask
+ )
+
+ assert ctx.filters[0].status == DashboardFilterStatus.APPLIED
+ assert ctx.extra_form_data["filters"][0]["val"] == ["US"]
+
+
+@patch("superset.charts.data.dashboard_filter_context._check_dashboard_access")
+@patch("superset.charts.data.dashboard_filter_context.db")
+def test_active_data_mask_applies_despite_default_to_first_item(
+ mock_db: MagicMock,
+ mock_check_access: MagicMock,
+) -> None:
+ """
+ defaultToFirstItem filters cannot be resolved from saved config, but when
the
+ frontend supplies a concrete active value it is applied.
+ """
+ filter_config = [
+ _make_filter(
+ flt_id="f1",
+ name="City",
+ scope_root=["ROOT_ID"],
+ default_to_first_item=True,
+ target_column="city",
+ ),
+ ]
+ _build_dashboard_mock(mock_db, filter_config, [10])
+
+ active_data_mask = {
+ "f1": {
+ "extraFormData": {"filters": [{"col": "city", "op": "IN", "val":
["NYC"]}]}
+ }
+ }
+ ctx = get_dashboard_filter_context(
+ dashboard_id=1, chart_id=10, active_data_mask=active_data_mask
+ )
+
+ assert ctx.filters[0].status == DashboardFilterStatus.APPLIED
+ assert ctx.extra_form_data["filters"][0]["val"] == ["NYC"]
+
+
+@patch("superset.charts.data.dashboard_filter_context._check_dashboard_access")
+@patch("superset.charts.data.dashboard_filter_context.db")
+def test_active_data_mask_out_of_scope_filter_still_excluded(
+ mock_db: MagicMock,
+ mock_check_access: MagicMock,
+) -> None:
+ """An active value for an out-of-scope filter does not leak into the
chart."""
+ filter_config = [
+ _make_filter(
+ flt_id="f1",
+ name="Out-of-scope",
+ scope_root=["TABS-nonexistent"],
+ target_column="status",
+ ),
+ ]
+ _build_dashboard_mock(mock_db, filter_config, [10])
+
+ active_data_mask = {
+ "f1": {
+ "extraFormData": {
+ "filters": [{"col": "status", "op": "IN", "val": ["active"]}]
+ }
+ }
+ }
Review Comment:
**Suggestion:** Provide a type annotation for this newly added mapping
variable to satisfy the type-hint rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is another newly added untyped local variable in Python test code. It
can be annotated, so it violates the type-hint requirement.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=37a363742cb14fb7b0129a0f16e10f96&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=37a363742cb14fb7b0129a0f16e10f96&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/charts/test_dashboard_filter_context.py
**Line:** 723:729
**Comment:**
*Custom Rule: Provide a type annotation for this newly added mapping
variable to satisfy the type-hint rule.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=3a9691a20d4ac25b415bd9d3f50f5bca3835d70e25165844561307ab3e2c4d19&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41133&comment_hash=3a9691a20d4ac25b415bd9d3f50f5bca3835d70e25165844561307ab3e2c4d19&reaction=dislike'>👎</a>
--
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]
