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
fix: native annotations #10037
fix: native annotations #10037
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10037 +/- ##
==========================================
- Coverage 68.91% 68.85% -0.06%
==========================================
Files 584 584
Lines 31047 31056 +9
Branches 3180 3180
==========================================
- Hits 21395 21384 -11
- Misses 9543 9563 +20
Partials 109 109
Continue to review full report at Codecov.
|
I propose merging this as-is, and adding more comprehensive tests later if we refactor annotations (I hadn't worked extensively with the annotation code before, but it seems to be in need of some refactoring at some point). |
@@ -103,7 +103,7 @@ class AnnotationLayerModelView(SupersetModelView): # pylint: disable=too-many-a | |||
add_title = _("Add Annotation Layer") | |||
edit_title = _("Edit Annotation Layer") | |||
|
|||
list_columns = ["name", "descr"] | |||
list_columns = ["id", "name", "descr"] |
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.
This will make the CRUD view slightly uglier with the id
, but is necessary based on discussions with @dpgaspar
* fix: native annotations * Add test * Add comment to test
SUMMARY
Native annotations are currently broken due to regressions in recent PRs:
BEFORE
Bug number 1: RLS breaks annotations as they only apply to SQL datasources if
ENABLE_ROW_LEVEL_SECURITY
is enabled:Bug number 2: The
id
of the annotation layer was dropped in #9888 , hence can't be selected:AFTER
TEST PLAN
Local testing + new test + CI
ADDITIONAL INFORMATION