Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: remove access_type from curation API #3080

Merged
merged 8 commits into from
Aug 25, 2022
Merged

fix: remove access_type from curation API #3080

merged 8 commits into from
Aug 25, 2022

Conversation

Bento007
Copy link
Contributor

@Bento007 Bento007 commented Aug 10, 2022 •
edited
Loading

Reviewers

Functional: @danieljhegeman

Readability: @ebezzi

Changes

  • remove access_type from the curation_api
  • modify how we determine what filters to apply when listing collections. This is to accommodate the changes needed to support the removal of the access_type. It also reduces the number of collections returned by database to only the ones the user can view.
  • Fix the error message when curator is used by not a super curator

QA steps (optional)

  • updated tests to use access_type.
  • update tests to handle new errors return when using the curator parameter

Notes for Reviewer

blocked by #3089

@Bento007 Bento007 self-assigned this Aug 10, 2022
@Bento007 Bento007 changed the title remove access_type from curation API fix: remove access_type from curation API Aug 11, 2022
danieljhegeman
danieljhegeman previously requested changes Aug 14, 2022
View reviewed changes
Copy link
Contributor

@danieljhegeman danieljhegeman left a comment

Choose a reason for hiding this comment

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

We're now running into the problem with my design choice from a while back for the reshape method 😅

See comment about breaking Super Curator #3

backend/corpora/lambdas/api/v1/curation/collections/common.py Outdated Show resolved Hide resolved
@Bento007
Merge branch 'main' into tsmith/2997-remove-access-type
0cc9506 
# Conflicts:
#	backend/config/curation-api.yml
#	tests/unit/backend/corpora/api_server/curator/collection/test_curation_collections.py
@Bento007
remove is_allowed check from reshape_for_curation_api
1ad3e07
nayib-jose-gloria
nayib-jose-gloria reviewed Aug 25, 2022
View reviewed changes
backend/corpora/lambdas/api/v1/curation/collections/actions.py Outdated
else:
result = owner_or_allowed(token_info)
if result:
filters.append(DbCollection.owner == result)
Copy link
Contributor

Choose a reason for hiding this comment

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

to accommodate Super Curator #3 outlined here, I think the owner filter should only be appended if the user is not a super curator. Maybe turn this else block into an elif not is_super_curator(token_info)

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, owner_or_allowed already returns None for superusers. Maybe could use a rename, but that's out of scope for this ticket

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave this open as a non-blocking suggestion in case you do think of a better name, because it isn't clear just reading a call to owner_or_allowed what it actually returns. but admittedly, I can't come up with anything particularly great off the top of my head

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change the name to owner_filter and added a comment.

nayib-jose-gloria
nayib-jose-gloria approved these changes Aug 25, 2022
View reviewed changes
Copy link
Contributor

@nayib-jose-gloria nayib-jose-gloria left a comment

Choose a reason for hiding this comment

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

The change itself looks good! I left a few non-blocking suggestions for readability / code clean-up, but I recognize those issues didn't originate with this fix. We can also revisit later, or you can let me know if you disagree

@Bento007 Bento007 enabled auto-merge (squash) August 25, 2022 21:35
@Bento007 Bento007 dismissed danieljhegeman’s stale review August 25, 2022 21:57

unavailable to review

@Bento007 Bento007 merged commit 6844294 into main Aug 25, 2022
@Bento007 Bento007 deleted the tsmith/2997-remove-access-type branch August 25, 2022 21:57
danieljhegeman pushed a commit that referenced this pull request Aug 30, 2022
@Bento007 @nayib-jose-gloria
@ashin-czi @tihuan
chore: deploy staging <- main (#3213)
f92570d 
* fix: remove self example from openapi (#3173)

* chore: cherry pick changes from #3172 surface dataset batch_condition in curation API (#3177)

* feat: surface dataset batch_condition in curation API

* resolve merge conflict

* fix: document upload links that are suported (#3176)

* Document the types of links that are supported
* Update documentation

* docs: Renaming scExpression to Gene Expression (#3091)

* docs: Renaming scExpression to Gene Expression

- changing route to gene-expression instead of scExpression

* chore(fe): upgrade to nextjs12 blueprint4 node16 (#3119)

* chore(fe): upgrade to nextjs12 blueprint4 node16

* upgrade packages to resolve vulns

* fix(lint): lint stuff

* fix(lint): lint stuff

* restore css usage

* update Dockerfile

* clean up css

* fix bp4 css change

* update docsite image css

* fix: remove access_type from curation API (#3080)

* remove access_type form curation API

* remove is_allowed check from reshape_for_curation_api

* update openapi

* Fix the error message when curator is used by not a super curator

* fix errors

* Move all reshaping collection code into reshape_for_curation_api.

* fix: change the datasets response shape (#3178)

change name to title in dataset
change is_primary_data to list of boolean in dataset

* fix(curation api):Remove h5ad suffix requirements (#3151)

- remove .h5ad suffix from uploads and curator tags
- check for embedded UUID in curator tag

* fix: backend/Makefile local db improvements, migration README (#3029)

* fix: backend/Makefile local db improvements, migration README

- Rename the targets for loading data and loading schema into local test
  db
- Adapt the commands for the above two scripts to actually work (??)

* doc updates

* additional doc update

* style: remove unused regex and consolidate curator tag logic (#3200)

Co-authored-by: Trent Smith <1429913+Bento007@users.noreply.github.com>
Co-authored-by: Nayib Gloria <55710092+nayib-jose-gloria@users.noreply.github.com>
Co-authored-by: ashin-czi <109984998+ashin-czi@users.noreply.github.com>
Co-authored-by: Timmy Huang <tihuan@users.noreply.github.com>
@danieljhegeman danieljhegeman mentioned this pull request Sep 1, 2022
Merged
danieljhegeman pushed a commit that referenced this pull request Sep 1, 2022
@Bento007 @nayib-jose-gloria
@ashin-czi @tihuan
chore: deploy staging <- main (#3213) (#3221)
b05fde4 
* fix: remove self example from openapi (#3173)

* chore: cherry pick changes from #3172 surface dataset batch_condition in curation API (#3177)

* feat: surface dataset batch_condition in curation API

* resolve merge conflict

* fix: document upload links that are suported (#3176)

* Document the types of links that are supported
* Update documentation

* docs: Renaming scExpression to Gene Expression (#3091)

* docs: Renaming scExpression to Gene Expression

- changing route to gene-expression instead of scExpression

* chore(fe): upgrade to nextjs12 blueprint4 node16 (#3119)

* chore(fe): upgrade to nextjs12 blueprint4 node16

* upgrade packages to resolve vulns

* fix(lint): lint stuff

* fix(lint): lint stuff

* restore css usage

* update Dockerfile

* clean up css

* fix bp4 css change

* update docsite image css

* fix: remove access_type from curation API (#3080)

* remove access_type form curation API

* remove is_allowed check from reshape_for_curation_api

* update openapi

* Fix the error message when curator is used by not a super curator

* fix errors

* Move all reshaping collection code into reshape_for_curation_api.

* fix: change the datasets response shape (#3178)

change name to title in dataset
change is_primary_data to list of boolean in dataset

* fix(curation api):Remove h5ad suffix requirements (#3151)

- remove .h5ad suffix from uploads and curator tags
- check for embedded UUID in curator tag

* fix: backend/Makefile local db improvements, migration README (#3029)

* fix: backend/Makefile local db improvements, migration README

- Rename the targets for loading data and loading schema into local test
  db
- Adapt the commands for the above two scripts to actually work (??)

* doc updates

* additional doc update

* style: remove unused regex and consolidate curator tag logic (#3200)

Co-authored-by: Trent Smith <1429913+Bento007@users.noreply.github.com>
Co-authored-by: Nayib Gloria <55710092+nayib-jose-gloria@users.noreply.github.com>
Co-authored-by: ashin-czi <109984998+ashin-czi@users.noreply.github.com>
Co-authored-by: Timmy Huang <tihuan@users.noreply.github.com>

Co-authored-by: Trent Smith <1429913+Bento007@users.noreply.github.com>
Co-authored-by: Nayib Gloria <55710092+nayib-jose-gloria@users.noreply.github.com>
Co-authored-by: ashin-czi <109984998+ashin-czi@users.noreply.github.com>
Co-authored-by: Timmy Huang <tihuan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nayib-jose-gloria nayib-jose-gloria nayib-jose-gloria approved these changes

@danieljhegeman danieljhegeman Awaiting requested review from danieljhegeman

@ebezzi ebezzi Awaiting requested review from ebezzi

Assignees

@Bento007 Bento007

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Bento007 @danieljhegeman @nayib-jose-gloria