fix(Tags filter): Filter assets by tag ID

[Vitor-Avila]

SUMMARY

#28570 updated SelectTagsValue and tagToSelectOption. These were also used by the Tags filter in the CRUD views (dashboards, charts and saved queries), so these filters stopped working (as they're now configured to use the tag.id as the value (rather than the tag.name which is expected by the API filter).

This PR introduces a new filter operator for these assets to be able to filter tags by ID. I decided not to update the existing filter (that filters by name) and instead create a new one, to avoid a breaking change in the API.

I also removed the TAGGING_SYSTEM FF check in the SavedQueries API, for consistency with the Dashboard (#23909) and Chart (#24060) APIs.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

Screen.Recording.2024-06-29.at.8.47.31.PM.mov

After

After.mov

TESTING INSTRUCTIONS

Added integration tests for both API filters.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags: TAGGING_SYSTEM
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.36%. Comparing base (76d897e) to head (dc78646).
Report is 966 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #29412      +/-   ##
==========================================
+ Coverage   60.48%   70.36%   +9.87%     
==========================================
  Files        1931     1970      +39     
  Lines       76236    78610    +2374     
  Branches     8568     8973     +405     
==========================================
+ Hits        46114    55312    +9198     
+ Misses      28017    21100    -6917     
- Partials     2105     2198      +93     

Flag	Coverage Δ
hive	49.15% <90.00%> (-0.02%)	⬇️
javascript	58.05% <ø> (+0.33%)	⬆️
mysql	77.01% <100.00%> (?)
postgres	77.12% <100.00%> (?)
presto	53.73% <90.00%> (-0.07%)	⬇️
python	83.68% <100.00%> (+20.19%)	⬆️
sqlite	76.60% <100.00%> (?)
unit	59.72% <100.00%> (+2.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hughhhh]

Can we write a small unit test to verify the query is properly writing the query we want?

I think something like this would be good for the base class

superset/tests/unit_tests/databases/filters_test.py

Line 116 in 7f3c8ef

filtered_query = filter_.apply(query, None)

[Vitor-Avila]

Hey @hughhhh -- just added unit tests for the BaseTagFilter -- the ChartTagNameFilter, ChartTagIdFilter, DashboardTagNameFilter, DashboardTagIdFilter, SavedQueryTagNameFilter and SavedQueryTagIdFilter should have integration tests already 🙌

Let me know if you have any other concerns or feedback 🙏

[betodealmeida]

Nice! I left a recommendation about splitting the BaseTagFilter class into two, if you think it's not a lot of additional work — it might make the code easier to follow.

[betodealmeida]

I think it would be cleaner to define a new BaseTagIdFilter, and rename this one to BaseTagNameFilter, to prevent the check in apply.

[Vitor-Avila]

that makes sense! I just pushed a new commit with this change. I also ended up moving these two filters to superset.tag.filters as I believe it makes more sense -- let me know your thoughts 🙏