(MIDRC-1276) Add pre-filtering in funnel query for listing tasks - #164
(MIDRC-1276) Add pre-filtering in funnel query for listing tasks#164george42-ctds wants to merge 7 commits into
Conversation
|
The style in this PR agrees with This formatting comment was generated automatically by a script in uc-cdis/wool. |
Coverage Report for CI Build 29771115552Coverage increased (+0.5%) to 83.598%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions17 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
Integration Tests on Kind
|
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
Integration Tests on Kind
|
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
| TAGS_HIDDEN_FROM_USER.remove("_AUTHZ") | ||
|
|
||
|
|
||
| def get_auth_string_for_user(user_id: str) -> str: |
There was a problem hiding this comment.
| def get_auth_string_for_user(user_id: str) -> str: | |
| def get_authz_string_for_user(user_id: str) -> str: |
| # filter out tasks the current user does not have access to | ||
| listed_tasks["tasks"] = [ |
There was a problem hiding this comment.
We should not need to get the user's access and filter the tasks when we're already filtering on the _AUTHZ tag. But that means we'd 100% trust the TES server to filter tasks properly.
I'd say let's leave it in, but add a comment stating this^?
| @router.get("/tasks/", status_code=HTTP_200_OK, include_in_schema=False) | ||
| async def list_tasks(request: Request, auth=Depends(Auth)) -> dict: | ||
| async def list_tasks( | ||
| request: Request, auth=Depends(Auth), all: str | None = None |
There was a problem hiding this comment.
The all param needs to be documented
Example of how to do it here
Integration Tests on Kind
|
| raise HTTPException(e.code, e.message) | ||
|
|
||
| # filter out tasks the current user does not have access to | ||
| # this should be redundant if all is None but is included in case pre-filtering fails |
There was a problem hiding this comment.
| # this should be redundant if all is None but is included in case pre-filtering fails | |
| # this should be redundant if all is None but is included in case server-side filtering fails |
| request: Request, | ||
| auth=Depends(Auth), | ||
| all: str = Query( | ||
| None, description="Get all TES tasks without pre-filtering on _AUTHZ" |
There was a problem hiding this comment.
The description should be user-friendly because that's what goes into the auto-generated swagger doc. Maybe something like "If true, retrieves all the TES tasks you may have access to, instead of just your own. Default: false"?
Integration Tests on Kind
|
Integration TestsTest summary after running integration tests
Test summary after rerunning failed integration tests
Please find the detailed integration test report here Please find the detailed integration test report after rerunning failed tests here Please find the Github Action logs here |
Integration Tests on Kind
|
3 participants
JIRA ticket:MIDRC-1276
New Features
Breaking Changes
Bug Fixes
tag_keyandtag_valueto funnel task query to get only tasks for user. Optionalallflag allows getting all tasks in funnel query.Improvements
Dependency updates
Deployment changes