-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add Certified filter to Datasets #20136
Conversation
6c10299
to
bd66b8f
Compare
Codecov Report
@@ Coverage Diff @@
## master #20136 +/- ##
==========================================
- Coverage 66.45% 66.29% -0.16%
==========================================
Files 1721 1721
Lines 64497 64532 +35
Branches 6805 6811 +6
==========================================
- Hits 42860 42782 -78
- Misses 19905 20017 +112
- Partials 1732 1733 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
bd66b8f
to
166b352
Compare
/testenv up |
@eschutho Container image not yet published for this PR. Please try again when build is complete. |
@eschutho Ephemeral environment creation failed. Please check the Actions logs for details. |
This is great! Can you add a test? @AAfghahi wrote some nice tests for his database filters here that you could prob use for inspiration: https://github.com/apache/superset/pull/19051/files#diff-b94e273d2b41d3fe8a552cbf34cfa59ab9495154d16823b411470ae782cea38a |
Also I was going to test what happens if the word "certification" is in the warning for example, and if so, we may want to lengthen the search string to |
82063af
to
b4224db
Compare
b4224db
to
9a368cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
/testenv up |
"sql": [DatasetIsNullOrEmptyFilter], | ||
"id": [DatasetCertifiedFilter], | ||
} | ||
search_columns = ["id", "database", "owners", "sql", "table_name"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this limits search to only these columns, is that the intention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this with my filter PR and it broke a lot of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i don't set the search columns, the new filter won't be referenced. So i'm explicitly setting it here just like the other API classes
superset/datasets/filters.py
Outdated
name = _("Is certified") | ||
arg_name = "dataset_is_certified" | ||
|
||
def apply(self, query: Query, value: Any) -> Query: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like value here is just a boolean?
@jinghua-qa Ephemeral environment spinning up at http://52.25.126.26:8080. Credentials are |
914b360
to
bc0480f
Compare
bc0480f
to
169022a
Compare
/testenv up |
@jinghua-qa Ephemeral environment spinning up at http://35.89.165.229:8080. Credentials are |
LGTM |
Ephemeral environment shutdown and build artifacts deleted. |
Screen.Recording.2022-05-19.at.6.29.02.PM.mov
SUMMARY
Allowing users to know filter by
Dataset.extra.certification
. Using is like check in the ORM to validateBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION