Skip to content

chore: Refactor ownership - #21938

Merged
john-bodley merged 1 commit into
apache:masterfrom
john-bodley:john-bodley--deprecate-actor
Nov 2, 2022
Merged

chore: Refactor ownership#21938
john-bodley merged 1 commit into
apache:masterfrom
john-bodley:john-bodley--deprecate-actor

Conversation

@john-bodley

Copy link
Copy Markdown
Member

SUMMARY

A follow up to #20499 where the actor field was deprecated given this is simply the current logged in user.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@codecov

codecov Bot commented Oct 25, 2022

Copy link
Copy Markdown

Codecov Report

Merging #21938 (4d546c7) into master (9a063ab) will increase coverage by 11.37%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master   #21938       +/-   ##
===========================================
+ Coverage   55.55%   66.92%   +11.37%     
===========================================
  Files        1806     1806               
  Lines       69140    69156       +16     
  Branches     7393     7393               
===========================================
+ Hits        38409    46284     +7875     
+ Misses      28822    20963     -7859     
  Partials     1909     1909               
Flag Coverage Δ
hive 52.92% <50.00%> (+<0.01%) ⬆️
mysql 78.37% <100.00%> (?)
postgres 78.43% <100.00%> (?)
presto 52.83% <50.00%> (+<0.01%) ⬆️
python 81.49% <100.00%> (+23.54%) ⬆️
sqlite 76.92% <100.00%> (?)
unit 51.08% <50.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
superset/datasets/api.py 87.39% <100.00%> (+38.21%) ⬆️
superset/datasets/commands/duplicate.py 90.90% <100.00%> (+61.79%) ⬆️
superset/config.py 91.69% <0.00%> (+0.63%) ⬆️
superset/views/database/views.py 31.36% <0.00%> (+0.90%) ⬆️
superset/sql_parse.py 96.49% <0.00%> (+1.16%) ⬆️
superset/db_engine_specs/__init__.py 85.71% <0.00%> (+1.19%) ⬆️
superset/tables/models.py 67.53% <0.00%> (+1.29%) ⬆️
superset/exceptions.py 90.43% <0.00%> (+1.73%) ⬆️
superset/reports/models.py 100.00% <0.00%> (+2.00%) ⬆️
superset/explore/utils.py 100.00% <0.00%> (+2.27%) ⬆️
... and 285 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@john-bodley
john-bodley force-pushed the john-bodley--deprecate-actor branch from 62a662e to 4d546c7 Compare October 26, 2022 00:01
Comment thread superset/datasets/api.py Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was wrong. It was passing in a List[int] whereas the class signature expected User. I'm somewhat perplexed as to why Mypy didn't report this an error. See this Slack thread for details.

exceptions.append(DatasetExistsValidationError(table_name=duplicate_name))

try:
owners = self.populate_owners(self._actor)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

See the previous comment. I gather Mypy didn't barf because it was being passed List[int] as opposed to User per the class __init__ method.

Note, per here there's no need to specify [g.user.id] given that it defaults to the current user if not specified.

@villebro villebro left a comment

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 tried to think of any reason for this to be passed through, but couldn't come up with any valid reason, so I must assume it's not needed. And if this does indeed break something we need to add an IT for it anyway. LGTM.

@john-bodley
john-bodley merged commit 4a330a3 into apache:master Nov 2, 2022
@john-bodley
john-bodley deleted the john-bodley--deprecate-actor branch November 2, 2022 08:05
@mistercrunch mistercrunch added the 🚢 2.1.3 First shipped in 2.1.3 label Feb 18, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 First shipped in 2.1.0 and removed 🚢 2.1.3 First shipped in 2.1.3 labels Mar 13, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
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/S 🚢 2.1.0 First shipped in 2.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants