refactor: Deprecate ensure_user_is_set in favor of override_user #20502
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20502 +/- ##
==========================================
- Coverage 66.85% 66.85% -0.01%
==========================================
Files 1749 1749
Lines 65417 65416 -1
Branches 6906 6906
==========================================
- Hits 43736 43735 -1
Misses 19931 19931
Partials 1750 1750
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
john-bodley
force-pushed
the
john-bodley--refactor-ensure_user_is_set
branch
from
72227d0 to
72c71f3
Compare
| @@ -76,30 +67,35 @@ def load_chart_data_into_cache( | |||
| # pylint: disable=import-outside-toplevel | |||
| from superset.charts.data.commands.get_data_command import ChartDataCommand | |||
|
|
|||
| try: | |||
| ensure_user_is_set(job_metadata.get("user_id")) | |||
There was a problem hiding this comment.
I'm not certain whether this logic was correct, i.e., should we ensure that job_metadata.get("user_id") is the user performing said actions regardless of whether there is a current user set? Note the override_user context manager will reset back to the current user upon exit.
@benjreinhart it seems you authored #13878 and thus I was hoping you might be able to provide more context, i.e., on line #75 should it be override_user(user) rather than overide_user(user, force=False)?
| @@ -1453,23 +1453,27 @@ def get_user_id() -> Optional[int]: | |||
|
|
|||
|
|
|||
| @contextmanager | |||
| def override_user(user: Optional[User]) -> Iterator[Any]: | |||
| def override_user(user: Optional[User], force: bool = True) -> Iterator[Any]: | |||
There was a problem hiding this comment.
The force flag was added for parity, however per my previous comment I'm not sure it's needed.
| assert not hasattr(mock_g, "user") | ||
|
|
||
| with override_user(user): | ||
| mock_g.user = None |
|
@zhaoyongjie I introduced |
…che#20502) Co-authored-by: John Bodley <[email protected]>
mistercrunch
added
🏷️ bot
SUMMARY
The
ensure_user_is_setmethod shares similar logic tooverride_user. It has logic to only override the user if not set—which I gather is to ensure that it doesn't permanently change the user, however if no user was previously set, setting of a user will persist.This PR updates the
override_userlogic to also include aforceflag to only override if non-set enabling theensure_user_is_setmethod to be removed.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION