You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/07/27 18:44:04 UTC

[GitHub] [superset] cccs-Dustin opened a new pull request, #20892: [WIP] feat: Add dataset tagging to the back-end

cccs-Dustin opened a new pull request, #20892:
URL: https://github.com/apache/superset/pull/20892

   <!---
   Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
   Example:
   fix(dashboard): load charts correctly
   -->
   
   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   In the current version of the master branch, the back-end only supports tagging for 'Charts', 'Dashboards' and 'Saved Queries'. However, this PR would add the back-end support for tagging 'Datasets'. This is just the first of at least 2 PR's, the second of which will introduce new API endpoints as well as add dataset tagging to the UI.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   N/A
   
   ### TESTING INSTRUCTIONS
   <!--- Required! What steps can be taken to manually verify the changes? -->
   N/A
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [X] Required feature flags: TAGGING_SYSTEM
   - [ ] Changes UI
   - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
     - [ ] Migration is atomic, supports rollback & is backwards-compatible
     - [ ] Confirm DB migration upgrade and downgrade tested
     - [ ] Runtime estimates and downtime expectations provided
   - [X] Introduces new feature or API
   - [ ] Removes existing feature or API
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] cccs-Dustin closed pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
cccs-Dustin closed pull request #20892: feat: Add dataset tagging to the back-end
URL: https://github.com/apache/superset/pull/20892


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro merged pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
villebro merged PR #20892:
URL: https://github.com/apache/superset/pull/20892


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on a diff in pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on code in PR #20892:
URL: https://github.com/apache/superset/pull/20892#discussion_r977393647


##########
superset/migrations/versions/2018-07-26_11-10_c82ee8a39623_add_implicit_tags.py:
##########
@@ -33,7 +33,7 @@
 from sqlalchemy import Column, DateTime, Enum, ForeignKey, Integer, String
 from sqlalchemy.ext.declarative import declarative_base, declared_attr
 
-from superset.models.tags import ObjectTypes, TagTypes
+from superset.tags.models import ObjectTypes, TagTypes

Review Comment:
   cool, makes sense to me



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a diff in pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20892:
URL: https://github.com/apache/superset/pull/20892#discussion_r973016585


##########
superset/tags/models.py:
##########
@@ -116,13 +119,15 @@ class ObjectUpdater:
 
     @classmethod
     def get_owners_ids(
-        cls, target: Union["Dashboard", "FavStar", "Slice"]
+        cls, target: Union["Dashboard", "FavStar", "Slice", "Query", "SqlaTable"]

Review Comment:
   Let's get rid of these quotation marks by doing the ol' `from __future__ import annotations`



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on a diff in pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on code in PR #20892:
URL: https://github.com/apache/superset/pull/20892#discussion_r977351949


##########
superset/migrations/versions/2018-07-26_11-10_c82ee8a39623_add_implicit_tags.py:
##########
@@ -33,7 +33,7 @@
 from sqlalchemy import Column, DateTime, Enum, ForeignKey, Integer, String
 from sqlalchemy.ext.declarative import declarative_base, declared_attr
 
-from superset.models.tags import ObjectTypes, TagTypes
+from superset.tags.models import ObjectTypes, TagTypes

Review Comment:
   not related with this PR, but ideally we should place the Enums here also, to avoid this problem for example and move away from native Enums on the DB in favour of strings (if an Enum changes no new db migration is needed)
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] cccs-Dustin commented on a diff in pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
cccs-Dustin commented on code in PR #20892:
URL: https://github.com/apache/superset/pull/20892#discussion_r973036102


##########
tests/integration_tests/fixtures/tags.py:
##########
@@ -0,0 +1,31 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import pytest
+
+from superset.tags.core import clear_sqla_event_listeners, register_sqla_event_listeners
+from tests.integration_tests.test_app import app
+
+
+@pytest.fixture
+def tag_sqla_event_listeners():
+    with app.app_context():
+        app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = True
+        register_sqla_event_listeners()
+        yield
+        app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = False
+        clear_sqla_event_listeners()

Review Comment:
   Done! I added this change to the most recent commit



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] cccs-Dustin closed pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
cccs-Dustin closed pull request #20892: feat: Add dataset tagging to the back-end
URL: https://github.com/apache/superset/pull/20892


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a diff in pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20892:
URL: https://github.com/apache/superset/pull/20892#discussion_r977388292


##########
superset/migrations/versions/2018-07-26_11-10_c82ee8a39623_add_implicit_tags.py:
##########
@@ -33,7 +33,7 @@
 from sqlalchemy import Column, DateTime, Enum, ForeignKey, Integer, String
 from sqlalchemy.ext.declarative import declarative_base, declared_attr
 
-from superset.models.tags import ObjectTypes, TagTypes
+from superset.tags.models import ObjectTypes, TagTypes

Review Comment:
   > Looks good! One question, why the move of `/superset/models/tags.py` to `/superset/tags/models.py`?
   > 
   > Note: We are currently moving and deprecating all endpoints from `/supeset` to `/api/v1`
   
   @dpgaspar I requested this move, as I feel we should move all models from the generic `/superset/models` dumping ground to the relevant context, in this case `/superset/tags`. The same applies to `utils` and other modules - IMO a util which is specific to a certain context, say `tags`, should be placed under `/superset/tags/utils.py` rather than `/superset/utils/tags.py`. A good example of this is `key_value` which follows this pattern. I got this inspiration from our frontend code organization SIP (see #13632), which has done wonders for the frontend codebase readability.



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
villebro commented on PR #20892:
URL: https://github.com/apache/superset/pull/20892#issuecomment-1255907069

   > Could you add some unit tests to make sure that the commands are working as expected.
   
   FYI @AAfghahi I removed the change request from your review as the requested tests have been added to the PR


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov[bot] commented on pull request #20892: [WIP] feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20892:
URL: https://github.com/apache/superset/pull/20892#issuecomment-1197362066

   # [Codecov](https://codecov.io/gh/apache/superset/pull/20892?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#20892](https://codecov.io/gh/apache/superset/pull/20892?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (485f3f0) into [master](https://codecov.io/gh/apache/superset/commit/77db0651d819f4bda367fc59a4e95954cb0929e1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (77db065) will **decrease** coverage by `11.48%`.
   > The diff coverage is `42.59%`.
   
   > :exclamation: Current head 485f3f0 differs from pull request most recent head 9d9dce5. Consider uploading reports for the commit 9d9dce5 to get more accurate results
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #20892       +/-   ##
   ===========================================
   - Coverage   66.25%   54.77%   -11.49%     
   ===========================================
     Files        1758     1758               
     Lines       67048    67083       +35     
     Branches     7118     7118               
   ===========================================
   - Hits        44423    36745     -7678     
   - Misses      20808    28521     +7713     
     Partials     1817     1817               
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | hive | `53.23% <45.23%> (-0.01%)` | :arrow_down: |
   | mysql | `?` | |
   | postgres | `?` | |
   | presto | `53.13% <45.23%> (-0.01%)` | :arrow_down: |
   | python | `57.82% <45.23%> (-23.74%)` | :arrow_down: |
   | sqlite | `?` | |
   | unit | `50.23% <33.33%> (-0.02%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/20892?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/reports/actions/reports.js](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3JlcG9ydHMvYWN0aW9ucy9yZXBvcnRzLmpz) | `39.39% <0.00%> (ø)` | |
   | [...rontend/src/visualizations/TimeTable/TimeTable.jsx](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Zpc3VhbGl6YXRpb25zL1RpbWVUYWJsZS9UaW1lVGFibGUuanN4) | `0.00% <0.00%> (ø)` | |
   | [superset/common/tags.py](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29tbW9uL3RhZ3MucHk=) | `0.00% <0.00%> (ø)` | |
   | [superset/dashboards/filters.py](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9maWx0ZXJzLnB5) | `48.75% <ø> (-45.00%)` | :arrow_down: |
   | [superset/sql\_parse.py](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3BhcnNlLnB5) | `95.90% <ø> (-1.17%)` | :arrow_down: |
   | [superset/views/tags.py](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvdGFncy5weQ==) | `42.55% <25.00%> (-7.45%)` | :arrow_down: |
   | [...tend/plugins/plugin-chart-table/src/TableChart.tsx](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtdGFibGUvc3JjL1RhYmxlQ2hhcnQudHN4) | `38.31% <40.00%> (ø)` | |
   | [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `77.70% <40.00%> (-12.64%)` | :arrow_down: |
   | [superset/models/tags.py](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvbW9kZWxzL3RhZ3MucHk=) | `78.51% <71.42%> (-0.44%)` | :arrow_down: |
   | [superset/sqllab/command.py](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsbGFiL2NvbW1hbmQucHk=) | `85.24% <77.77%> (-0.97%)` | :arrow_down: |
   | ... and [292 more](https://codecov.io/gh/apache/superset/pull/20892/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] cccs-Dustin commented on pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
cccs-Dustin commented on PR #20892:
URL: https://github.com/apache/superset/pull/20892#issuecomment-1228555015

   > Could you add some unit tests to make sure that the commands are working as expected.
   
   Just re-opened the PR after adding in some integration tests to verify that the added code works as expected.


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] cccs-Dustin commented on pull request #20892: [WIP] feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
cccs-Dustin commented on PR #20892:
URL: https://github.com/apache/superset/pull/20892#issuecomment-1239421265

   Hi @dpgaspar & @betodealmeida! I am tagging you on this comment as @michael-s-molina said you two were who I should go to with this question about Pytest.
   
   I am currently working on adding the new back-end code for tagging datasets, however, when it comes to the integration tests, I am running into a brick wall so to speak. I am trying to mock the `TAGGING_SYSTEM` feature flag in my tests, however, it does not seem to be working. When I manually go into the `superset > connectors > sqla > models.py` (Dataset) file around like 2542 where it checks if the flag is enabled, if I take that check out, the test passes. Same thing with all of the other objects I am testing (`superset > models ? core.py` (Saved Query - line 850), `superset > models > dashboard.py` (Dashboard - line 458), and `superset > models > slice.py` (Chart - line 371)).
   
   I have tried all sorts of different methods to mock the feature flag (as you can see by all the commits on my PR: https://github.com/apache/superset/pull/20892), but I have not been successful as of yet.
   
   Michael did mention this, but I am still not sure how to fix the tests: "I think using @with_feature_flags(TAGGING_SYSTEM=True) like the other tests would be sufficient. I think it’s failing because the events in models.py are being registered when the module is loaded, which happens before the tests’ decorators/mocks. At that time, the flag is false"


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] cccs-Dustin commented on pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
cccs-Dustin commented on PR #20892:
URL: https://github.com/apache/superset/pull/20892#issuecomment-1208490437

   Closing this PR so that I can add unit tests to it. I will create a new PR once that is done.


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a diff in pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20892:
URL: https://github.com/apache/superset/pull/20892#discussion_r975479590


##########
tests/integration_tests/fixtures/tags.py:
##########
@@ -0,0 +1,31 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import pytest
+
+from superset.tags.core import clear_sqla_event_listeners, register_sqla_event_listeners
+from tests.integration_tests.test_app import app
+
+
+@pytest.fixture
+def with_tagging_system_feature():
+    with app.app_context():
+        app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = True
+        register_sqla_event_listeners()
+        yield
+        app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = False
+        clear_sqla_event_listeners()

Review Comment:
   Just to be on the safe side, I'd maybe make it so that this works even in case `TAGGING_SYSTEM` is made to default to `True`. Something like this:
   ```suggestion
   @pytest.fixture
   def with_tagging_system_feature():
       with app.app_context():
           is_enabled = app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"]
           if not is_enabled:
               app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = True
               register_sqla_event_listeners()
               yield
               app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = False
               clear_sqla_event_listeners()
   ```



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a diff in pull request #20892: [WIP] feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #20892:
URL: https://github.com/apache/superset/pull/20892#discussion_r973020369


##########
tests/integration_tests/fixtures/tags.py:
##########
@@ -0,0 +1,31 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import pytest
+
+from superset.tags.core import clear_sqla_event_listeners, register_sqla_event_listeners
+from tests.integration_tests.test_app import app
+
+
+@pytest.fixture
+def tag_sqla_event_listeners():
+    with app.app_context():
+        app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = True
+        register_sqla_event_listeners()
+        yield
+        app.config["DEFAULT_FEATURE_FLAGS"]["TAGGING_SYSTEM"] = False
+        clear_sqla_event_listeners()

Review Comment:
   Nit: could we rename this `with_tagging_system_feature` or something similar to indicate that it's in fact temporarily 1. flipping the feature flag and 2. enabling the tagging event listeners?



##########
superset/tags/models.py:
##########
@@ -116,13 +119,15 @@ class ObjectUpdater:
 
     @classmethod
     def get_owners_ids(
-        cls, target: Union["Dashboard", "FavStar", "Slice"]
+        cls, target: Union["Dashboard", "FavStar", "Slice", "Query", "SqlaTable"]

Review Comment:
   Let's get rid of these quotation marks by doing the ol' `from __future__ import annotations



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] cccs-Dustin commented on a diff in pull request #20892: feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
cccs-Dustin commented on code in PR #20892:
URL: https://github.com/apache/superset/pull/20892#discussion_r973046005


##########
superset/tags/models.py:
##########
@@ -116,13 +119,15 @@ class ObjectUpdater:
 
     @classmethod
     def get_owners_ids(
-        cls, target: Union["Dashboard", "FavStar", "Slice"]
+        cls, target: Union["Dashboard", "FavStar", "Slice", "Query", "SqlaTable"]

Review Comment:
   Done! I added this change to the most recent commit



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on pull request #20892: [WIP] feat: Add dataset tagging to the back-end

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on PR #20892:
URL: https://github.com/apache/superset/pull/20892#issuecomment-1239460250

   > Hi @dpgaspar & @betodealmeida! I am tagging you on this comment as @michael-s-molina said you two were who I should go to with this question about Pytest.
   > 
   > I am currently working on adding the new back-end code for tagging datasets, however, when it comes to the integration tests, I am running into a brick wall so to speak. I am trying to mock the `TAGGING_SYSTEM` feature flag in my tests, however, it does not seem to be working. When I manually go into the `superset > connectors > sqla > models.py` (Dataset) file around like 2542 where it checks if the flag is enabled, if I take that check out, the test passes. Same thing with all of the other objects I am testing (`superset > models ? core.py` (Saved Query - line 850), `superset > models > dashboard.py` (Dashboard - line 458), and `superset > models > slice.py` (Chart - line 371)).
   > 
   > I have tried all sorts of different methods to mock the feature flag (as you can see by all the commits on this PR), but I have not been successful as of yet.
   > 
   > Michael did mention this, but I am still not sure how to fix the tests: "I think using @with_feature_flags(TAGGING_SYSTEM=True) like the other tests would be sufficient. I think it’s failing because the events in models.py are being registered when the module is loaded, which happens before the tests’ decorators/mocks. At that time, the flag is false"
   > 
   > Would you be able to take a look at this PR and see if you can help me with mocking the feature flag in these integration tests?
   
   I think @michael-s-molina is right. Feature flags evaluation should not happen at module load time, try moving those conditional sqlalchemy event listeners to https://github.com/apache/superset/blob/c4b6fc5a6a93b5912bef863d7279a213599cda2f/superset/initialization/__init__.py#L583
   


-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org