Skip to content

feat: Add a remove_filter flag to the jinja filter_values function and add a new get_filters function (see issue 13943 for more details)#14507

Merged
villebro merged 1 commit into
apache:masterfrom
CybercentreCanada:issue_13943
May 21, 2021
Merged

feat: Add a remove_filter flag to the jinja filter_values function and add a new get_filters function (see issue 13943 for more details)#14507
villebro merged 1 commit into
apache:masterfrom
CybercentreCanada:issue_13943

Conversation

@cccs-jc
Copy link
Copy Markdown
Contributor

@cccs-jc cccs-jc commented May 6, 2021

SUMMARY

Implementation of issue #13943.
Feature must be enabled with feature flag ENABLE_TEMPLATE_REMOVE_FILTERS.

SCREENSHOT

superset_remove_filter

@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2021

Codecov Report

Merging #14507 (7a63a3c) into master (96289e9) will increase coverage by 0.15%.
The diff coverage is 81.77%.

❗ Current head 7a63a3c differs from pull request most recent head b51aac5. Consider uploading reports for the commit b51aac5 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14507      +/-   ##
==========================================
+ Coverage   77.14%   77.30%   +0.15%     
==========================================
  Files         958      960       +2     
  Lines       48241    48775     +534     
  Branches     5636     6122     +486     
==========================================
+ Hits        37217    37707     +490     
- Misses      10823    10862      +39     
- Partials      201      206       +5     
Flag Coverage Δ
hive ?
mysql 81.39% <87.76%> (+0.33%) ⬆️
postgres 81.41% <87.76%> (+0.32%) ⬆️
presto ?
python 81.49% <87.76%> (-0.13%) ⬇️
sqlite 81.03% <87.23%> (+0.32%) ⬆️

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

Impacted Files Coverage Δ
superset-frontend/src/CRUD/Fieldset.jsx 85.71% <ø> (ø)
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (ø)
superset-frontend/src/SqlLab/actions/sqlLab.js 58.97% <ø> (ø)
...-frontend/src/SqlLab/components/SouthPane/state.ts 100.00% <ø> (ø)
...erset-frontend/src/SqlLab/components/SqlEditor.jsx 54.62% <ø> (ø)
...et-frontend/src/SqlLab/components/TableElement.jsx 89.02% <ø> (ø)
...ontend/src/common/hooks/apiResources/dashboards.ts 40.00% <0.00%> (-10.00%) ⬇️
...et-frontend/src/components/EditableTitle/index.tsx 83.95% <ø> (ø)
...rset-frontend/src/components/ErrorMessage/types.ts 100.00% <ø> (ø)
...src/components/FilterableTable/FilterableTable.tsx 82.26% <ø> (ø)
... and 183 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96289e9...b51aac5. Read the comment docs.

@cccs-jc cccs-jc changed the title Implementation issue 13943 feat: Implementation issue 13943 May 7, 2021
@cccs-jc cccs-jc force-pushed the issue_13943 branch 5 times, most recently from 9c4990d to a3c7eb2 Compare May 7, 2021 15:21
Comment thread superset/jinja_context.py Outdated
Comment thread superset/jinja_context.py Outdated
Comment thread superset/jinja_context.py Outdated
Comment thread superset/connectors/sqla/models.py Outdated
Comment thread superset/jinja_context.py Outdated
@mistercrunch
Copy link
Copy Markdown
Member

Nit: Let's give meaningful titles to PRs so that we can get a sense for the PR from the list of PRs without having to click through to the issue. The PR title also becomes the commit msg in master as we squash and merge.

@junlincc junlincc added the hold:testing! On hold for testing label May 8, 2021
@junlincc
Copy link
Copy Markdown
Member

junlincc commented May 8, 2021

thank you jc for the contribution! really excited about this new feature. could you add screenshot of ui changes, as well as manual test plan? added hold tag

@cccs-jc cccs-jc force-pushed the issue_13943 branch 4 times, most recently from fc116f4 to 8eaecac Compare May 8, 2021 16:51
@cccs-jc cccs-jc changed the title feat: Implementation issue 13943 feat: Add a remove_filter flag to the jinja filter_values function and add a new get_filters function (see issue 13943 for more details) May 8, 2021
@junlincc junlincc removed the hold:testing! On hold for testing label May 10, 2021
@junlincc
Copy link
Copy Markdown
Member

thanks for adding video :) !

@villebro @zhaoyongjie is this gonna partially solve the "jinja filter status not applied on dashboard" problem?

@junlincc
Copy link
Copy Markdown
Member

/testenv up

@junlincc junlincc added the hold:testing! On hold for testing label May 10, 2021
@github-actions
Copy link
Copy Markdown
Contributor

@junlincc Ephemeral environment spinning up at http://34.211.207.26:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@cccs-jc
Copy link
Copy Markdown
Contributor Author

cccs-jc commented May 11, 2021

is this gonna partially solve the "jinja filter status not applied on dashboard" problem?

@junlincc it does in the sense that you can declare a column which you intend to only use as a filter. You can then handle that filter in your jinja template and mark it as remote_filter=True.

To prevent users from picking the column as a select column. We uncheck the Is Dimension flag in the dataset UI.

@cccs-jc cccs-jc force-pushed the issue_13943 branch 4 times, most recently from 6c5d4a2 to fe9df0c Compare May 12, 2021 20:57
@junlincc junlincc removed the hold:testing! On hold for testing label May 13, 2021
@cccs-jc
Copy link
Copy Markdown
Contributor Author

cccs-jc commented May 19, 2021

@villebro all tests pass. I think it's ready to be merged.

@junlincc
Copy link
Copy Markdown
Member

thank you so much for addressing the comments! @villebro please do help unblock jc 🙏

Copy link
Copy Markdown
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM - Let me give this a test run and approve if all is ok! 👍

Copy link
Copy Markdown
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work JC, really great feature and works really well! Left a few additional comments, with one functional question relating to casting val to str. Let me know what you think.

Comment thread superset/jinja_context.py Outdated
Comment thread superset/jinja_context.py Outdated
Comment thread superset/jinja_context.py Outdated
Comment thread superset/jinja_context.py Outdated
Copy link
Copy Markdown
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some last minute type nits

Comment thread superset/jinja_context.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we need to assume val can be Optional[Union[Any, List[Any]]]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you okay with Union[None, Any, List[Any]]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Comment thread superset/jinja_context.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the return value is List[Any]

Comment thread superset/jinja_context.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: List[Any]

Implementation issue 13943
Copy link
Copy Markdown
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the hard work on this awesome feature JC!

@villebro villebro merged commit 590fe20 into apache:master May 21, 2021
@github-actions
Copy link
Copy Markdown
Contributor

Ephemeral environment shutdown and build artifacts deleted.

cccs-jc added a commit to CybercentreCanada/superset that referenced this pull request May 27, 2021
…e#14507)

Implementation issue 13943

Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
cccs-jc pushed a commit to CybercentreCanada/superset that referenced this pull request May 30, 2021
cccs-jc pushed a commit to CybercentreCanada/superset that referenced this pull request May 30, 2021
cccs-joel pushed a commit to CybercentreCanada/superset that referenced this pull request May 31, 2021
…e#14507)

Implementation issue 13943

Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Sep 2, 2021
…e#14507)

Implementation issue 13943

Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Sep 8, 2021
…e#14507)

Implementation issue 13943

Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
…e#14507)

Implementation issue 13943

Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
…e#14507)

Implementation issue 13943

Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
…e#14507)

Implementation issue 13943

Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 First shipped in 1.3.0 labels Mar 12, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
…e#14507)

Implementation issue 13943

Co-authored-by: cccs-jc <cccs-jc@cyber.gc.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 1.3.0 First shipped in 1.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants