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.

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

GraphQL Mutations #966

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from
Open

GraphQL Mutations #966

wants to merge 24 commits into from

Conversation

nevoodoo
Copy link
Collaborator

A continuation of #686 - created this fresh branch because there were too many merge conflicts to deal with. Feedback and changes requested in #686 should be incorporated here.

@nevoodoo nevoodoo mentioned this pull request Sep 30, 2024
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 75.73964% with 123 lines in your changes missing coverage. Please review.

Project coverage is 81.70%. Comparing base (ab02bd2) to head (098b3a2).

Files with missing lines Patch % Lines
api/graphql/mutations/project.py 36.95% 29 Missing ⚠️
api/graphql/mutations/sample.py 52.17% 22 Missing ⚠️
api/graphql/mutations/cohort.py 73.23% 19 Missing ⚠️
api/graphql/mutations/participant.py 57.77% 19 Missing ⚠️
api/graphql/mutations/sequencing_group.py 65.51% 10 Missing ⚠️
api/graphql/mutations/family.py 58.82% 7 Missing ⚠️
api/graphql/mutations/analysis_runner.py 84.21% 6 Missing ⚠️
api/graphql/mutations/__init__.py 90.47% 4 Missing ⚠️
api/graphql/mutations/analysis.py 92.85% 4 Missing ⚠️
api/graphql/mutations/assay.py 92.85% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #966      +/-   ##
==========================================
- Coverage   81.75%   81.70%   -0.06%     
==========================================
  Files         184      188       +4     
  Lines       15969    16454     +485     
==========================================
+ Hits        13056    13443     +387     
- Misses       2913     3011      +98     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nevoodoo nevoodoo marked this pull request as draft October 1, 2024 00:17
@nevoodoo nevoodoo marked this pull request as ready for review October 14, 2024 23:14
Copy link
Contributor

@dancoates dancoates left a comment

Choose a reason for hiding this comment

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

I've left some specific comments but there are a few general things that I think will need updating in this too.

  • I know it was my idea but I think wrapping the mutations in a project to implement the permissions was actually not a great idea. It means that we can only restrict to full write permissions but some mutations like cohorts and comments actually allow for a lower level of permissions. After thinking about it some more, I think the best approach is to remove all the permissions checks from graphql and only have them in layers.
  • The return values of mutations need to be consistent and should return the thing that was updated/created. This makes the mutations a ton easier to use in the frontend as you don't need to make an extra request to get the updated item. It's also important that they return the exact same type as if you were to request the item, not a type like AssayUpsertType. This is due to the way Apollo handles its normalized cache. For things like deletes we can probably just return True or None, just need to settle on a standard and stick with it.
  • Also need to be consistent with inputs, it seems like in most cases the mutations are accepting a single input type, or in some cases the id and an input type. I think that's fine but we should be consistent and make sure that we don't have mutations that have a ton of inputs.

api/graphql/mutations/analysis_runner.py Outdated Show resolved Hide resolved
api/graphql/mutations/analysis.py Outdated Show resolved Hide resolved
api/graphql/mutations/analysis.py Show resolved Hide resolved
api/graphql/mutations/analysis.py Outdated Show resolved Hide resolved
api/graphql/mutations/analysis.py Outdated Show resolved Hide resolved
api/graphql/mutations/analysis_runner.py Outdated Show resolved Hide resolved
api/graphql/types/__init__.py Outdated Show resolved Hide resolved
api/graphql/mutations/sequencing_group.py Outdated Show resolved Hide resolved
api/graphql/mutations/sample.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants