You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Vitor-Avila (via GitHub)" <gi...@apache.org> on 2024/03/01 17:36:50 UTC
[PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Vitor-Avila opened a new pull request, #27364:
URL: https://github.com/apache/superset/pull/27364
### SUMMARY
The current behavior for updating assets via the API is inconsistent:
* When updating dashboards, the `owners` information wouldn't be modified unless it's part of the update payload
* When updating datasets and charts, the `owners` information would be lost unless if included in the update payload
This PR centralizes this logic to ensure consistency across charts, dashboards, datasets and reports (also DRYer).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
### TESTING INSTRUCTIONS
Tests added. To test through the UI:
1. Create a new chart.
2. Update its description via the API.
3. Validate the `owners` configuration was not affected.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] 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
- [ ] 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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #27364:
URL: https://github.com/apache/superset/pull/27364#issuecomment-1981674265
hey @michael-s-molina would you mind cherry-picking this PR into the 4.0 (or next possible) release?
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1509524651
##########
superset/commands/base.py:
##########
@@ -70,3 +70,17 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]:
:returns: Final list of owners
"""
return populate_owners(owner_ids, default_to_user=False)
+
+ @staticmethod
+ def update_owner_list(
+ current_owners: list[User],
+ new_owners: Optional[list[int]],
+ ) -> list[User]:
+ """
+ Handle list of owners for update events.
+
+ :param current_owners: list of current owners
+ :param new_owners: list of new owners specified in the update payload
+ :returns: Final list of owners
+ """
+ return update_owner_list(current_owners, new_owners)
Review Comment:
I was kind of confused thinking that the function was invoking itself to later realize that it's actually calling an utility function with the same name. Would you mind doing something like:
```
from superset.commands import utils
utils.populate_owners(owner_ids, default_to_user=True)
utils.update_owner_list(current_owners, new_owners)
```
or renaming the utility functions?
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1509509777
##########
superset/commands/utils.py:
##########
@@ -62,6 +62,24 @@ def populate_owners(
return owners
+def update_owner_list(
+ current_owners: list[User],
+ new_owners: list[int] | None,
+) -> list[User]:
+ """
+ Helper function for update commands, to properly handle the owners list.
+ Preserve the previous configuration unless included in the update payload.
+
+ :param current_owners: list of current owners
+ :param new_owners: list of new owners specified in the update payload
+ :returns: Final list of owners
+ """
+ owners_ids = (
+ [owner.id for owner in current_owners] if new_owners is None else new_owners
Review Comment:
Sure! I'm going to create a `tests/unit_tests/commands/test_utils.py` file. I found some other unit test files using `pytest` so I'll go with that.
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1509344856
##########
superset/commands/utils.py:
##########
@@ -62,6 +62,24 @@ def populate_owners(
return owners
+def update_owner_list(
+ current_owners: list[User],
+ new_owners: list[int] | None,
+) -> list[User]:
+ """
+ Helper function for update commands, to properly handle the owners list.
+ Preserve the previous configuration unless included in the update payload.
+
+ :param current_owners: list of current owners
+ :param new_owners: list of new owners specified in the update payload
+ :returns: Final list of owners
+ """
+ owners_ids = (
+ [owner.id for owner in current_owners] if new_owners is None else new_owners
Review Comment:
can you add this small unit test that will basically verify the that `populate_owners` will be called with the proper values?
Something like this will do:
```
import unittest
from unittest.mock import patch
from your_module import update_owner_list, populate_owners, User
class TestUpdateOwnerList(unittest.TestCase):
@patch('your_module.populate_owners')
def test_update_owner_list(self, mock_populate_owners):
# Define test data
current_owners = [User(id=1), User(id=2), User(id=3)]
new_owners = [4, 5, 6]
# Call the function under test
update_owner_list(current_owners, new_owners)
# Assert that populate_owners was called with the correct values
mock_populate_owners.assert_called_once_with(new_owners, default_to_user=False)
@patch('your_module.populate_owners')
def test_update_owner_list_with_none_new_owners(self, mock_populate_owners):
# Define test data
current_owners = [User(id=1), User(id=2), User(id=3)]
new_owners = None
# Call the function under test
update_owner_list(current_owners, new_owners)
# Assert that populate_owners was called with the correct values
expected_ids = [owner.id for owner in current_owners]
mock_populate_owners.assert_called_once_with(expected_ids, default_to_user=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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on PR #27364:
URL: https://github.com/apache/superset/pull/27364#issuecomment-1983390744
> hey @michael-s-molina would you mind cherry-picking this PR into the 4.0 (or next possible) release?
All PRs with the `fix` prefix are cherry-picked every day. You don't need to request their addition 😉
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1509883902
##########
superset/commands/base.py:
##########
@@ -70,3 +70,17 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]:
:returns: Final list of owners
"""
return populate_owners(owner_ids, default_to_user=False)
+
+ @staticmethod
+ def update_owner_list(
+ current_owners: list[User],
+ new_owners: Optional[list[int]],
+ ) -> list[User]:
+ """
+ Handle list of owners for update events.
+
+ :param current_owners: list of current owners
+ :param new_owners: list of new owners specified in the update payload
+ :returns: Final list of owners
+ """
+ return update_owner_list(current_owners, new_owners)
Review Comment:
I had that same reaction when started looking into this haha I kept it this flow because this was how `populate_owners` was implemented. I'll implement your first suggestion in a future 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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1509884797
##########
superset/commands/utils.py:
##########
@@ -62,6 +62,24 @@ def populate_owners(
return owners
+def update_owner_list(
+ current_owners: list[User],
+ new_owners: list[int] | None,
+) -> list[User]:
+ """
+ Helper function for update commands, to properly handle the owners list.
+ Preserve the previous configuration unless included in the update payload.
+
+ :param current_owners: list of current owners
+ :param new_owners: list of new owners specified in the update payload
+ :returns: Final list of owners
+ """
+ owners_ids = (
+ [owner.id for owner in current_owners] if new_owners is None else new_owners
Review Comment:
@hughhhh just added the unit tests 🙌 let me know if you have any feedback
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1509339929
##########
superset/commands/base.py:
##########
@@ -70,3 +70,17 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]:
:returns: Final list of owners
"""
return populate_owners(owner_ids, default_to_user=False)
+
+ @staticmethod
+ def update_owner_list(
Review Comment:
why are we wrapping this as `staticmethod`?
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1511298027
##########
superset/commands/utils.py:
##########
@@ -62,6 +62,25 @@ def populate_owners(
return owners
+def update_owner_list(
Review Comment:
nit: this is a pure function, but the name implies it's updating something but it's not, since it returns a newly created `list[User]`. why not call it `generate_owners` or `compute_owners`
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #27364:
URL: https://github.com/apache/superset/pull/27364#issuecomment-1973617477
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `33.33333%` with `8 lines` in your changes are missing coverage. Please review.
> Project coverage is 59.69%. Comparing base [(`371f2ab`)](https://app.codecov.io/gh/apache/superset/commit/371f2ab851eee46cd4ab3c5428c8061430a288ad?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`5519f19`)](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 7 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/commands/dashboard/update.py](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGFzaGJvYXJkL3VwZGF0ZS5weQ==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/utils.py](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvdXRpbHMucHk=) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/base.py](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvYmFzZS5weQ==) | 75.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/chart/update.py](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvY2hhcnQvdXBkYXRlLnB5) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/dataset/update.py](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGF0YXNldC91cGRhdGUucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/report/update.py](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvcmVwb3J0L3VwZGF0ZS5weQ==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #27364 +/- ##
==========================================
- Coverage 69.61% 59.69% -9.92%
==========================================
Files 1908 1908
Lines 74550 74552 +2
Branches 8317 8317
==========================================
- Hits 51896 44505 -7391
- Misses 20601 27994 +7393
Partials 2053 2053
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/27364/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [mysql](https://app.codecov.io/gh/apache/superset/pull/27364/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/27364/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/27364/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.74% <33.33%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/27364/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.32% <33.33%> (-20.57%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/27364/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/27364/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `56.51% <33.33%> (+<0.01%)` | :arrow_up: |
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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/27364?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1509339929
##########
superset/commands/base.py:
##########
@@ -70,3 +70,17 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]:
:returns: Final list of owners
"""
return populate_owners(owner_ids, default_to_user=False)
+
+ @staticmethod
+ def update_owner_list(
Review Comment:
why are we wrapping this as `staticmethod`?
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #27364:
URL: https://github.com/apache/superset/pull/27364#issuecomment-1977852881
@michael-s-molina @hughhhh let me know if you have any other feedback, or if you think this one is good to merge. Thanks!
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "hughhhh (via GitHub)" <gi...@apache.org>.
hughhhh commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1509344856
##########
superset/commands/utils.py:
##########
@@ -62,6 +62,24 @@ def populate_owners(
return owners
+def update_owner_list(
+ current_owners: list[User],
+ new_owners: list[int] | None,
+) -> list[User]:
+ """
+ Helper function for update commands, to properly handle the owners list.
+ Preserve the previous configuration unless included in the update payload.
+
+ :param current_owners: list of current owners
+ :param new_owners: list of new owners specified in the update payload
+ :returns: Final list of owners
+ """
+ owners_ids = (
+ [owner.id for owner in current_owners] if new_owners is None else new_owners
Review Comment:
can you add this small unit test that will basically verify the that `populate_owners` will be called with the proper values?
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1511596842
##########
superset/commands/utils.py:
##########
@@ -62,6 +62,25 @@ def populate_owners(
return owners
+def update_owner_list(
Review Comment:
that makes sense! Updated the functions to `compute_owners` (in the class), and `compute_owner_list` (in `utils`).
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1509883902
##########
superset/commands/base.py:
##########
@@ -70,3 +70,17 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]:
:returns: Final list of owners
"""
return populate_owners(owner_ids, default_to_user=False)
+
+ @staticmethod
+ def update_owner_list(
+ current_owners: list[User],
+ new_owners: Optional[list[int]],
+ ) -> list[User]:
+ """
+ Handle list of owners for update events.
+
+ :param current_owners: list of current owners
+ :param new_owners: list of new owners specified in the update payload
+ :returns: Final list of owners
+ """
+ return update_owner_list(current_owners, new_owners)
Review Comment:
I had that same reaction when started looking into this haha I kept it this flow because this was how `populate_owners` was imported. I'll implement your first suggestion in a future 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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on PR #27364:
URL: https://github.com/apache/superset/pull/27364#issuecomment-1983411933
that's amazing! thanks @michael-s-molina 🙏
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar merged PR #27364:
URL: https://github.com/apache/superset/pull/27364
--
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
Re: [PR] fix(API): Updating assets via the API should preserve ownership configuration [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #27364:
URL: https://github.com/apache/superset/pull/27364#discussion_r1511166827
##########
superset/commands/base.py:
##########
@@ -70,3 +70,17 @@ def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]:
:returns: Final list of owners
"""
return populate_owners(owner_ids, default_to_user=False)
+
+ @staticmethod
+ def update_owner_list(
+ current_owners: list[User],
+ new_owners: Optional[list[int]],
+ ) -> list[User]:
+ """
+ Handle list of owners for update events.
+
+ :param current_owners: list of current owners
+ :param new_owners: list of new owners specified in the update payload
+ :returns: Final list of owners
+ """
+ return update_owner_list(current_owners, new_owners)
Review Comment:
ended up renaming the functions to avoid importing `utils` entirely.
--
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