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

adding edit fields mutate to graphql #894

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

DrillableBit
Copy link
Collaborator

allowing:
"bot data enabled"
"bot data publicly downloadable"
"bot zip publicly downloadable"
"id"
"name"

allowing:
"bot data enabled"
"bot data publicly downloadable"
"bot zip publicly downloadable"
"id"
"name"
Copy link

github-actions bot commented Jan 12, 2025

Playwright test Report

2 tests  ±0   2 ✔️ ±0   13s ⏱️ +3s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ±0 

Results for commit 03e50ab. ± Comparison against base commit a1be707.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 12, 2025

Test Report

117 tests  +4   25 ✔️  - 88   50s ⏱️ - 2m 47s
    1 suites ±0     0 💤 ±  0 
    1 files   ±0   76 +76   16 🔥 +16 

For more details on these failures and errors, see this check.

Results for commit 03e50ab. ± Comparison against base commit a1be707.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@lladdy lladdy left a comment

Choose a reason for hiding this comment

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

Good work @DrillableBit
Couple things to be addressed. Reach out to me for guidance if you want RE the tests.

from aiarena.graphql.types import BotType


class UpdateBotInput(CleanedInputType):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please flag any required fields here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the syntax might be graphene.String(required=True), but check this.

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually have a custom way of marking fields as required (and also validating their correctness), that produces output that's easier to handle on the frontend side (ie {"id": ["Required field"]} instead of just a text error).

Set it up for the UpdateBot mutation and wrote a test for it.



class UpdateBotInput(CleanedInputType):
name = graphene.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

The bot name cannot be updated by it's owner, so this should probably be removed (unless we want to make the decision to change this at this time).

input_class = UpdateBotInput

@classmethod
def perform_mutate(cls, info: graphene.ResolveInfo, input_object: UpdateBotInput):
Copy link
Contributor

Choose a reason for hiding this comment

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

I would feel much more comfortable from my end if there were tests associated with this to assert both that it works in the way it's intended, both now and into the future.

I have added some examples in test_mutations.py. Feel free to reach out to me for guidance and we can get it set up together.

Copy link
Contributor

Choose a reason for hiding this comment

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

I created a helper class for GraphQL tests and actually implemented the example tests, so that we have something to look at when writing GraphQL tests in the future

@@ -225,3 +226,8 @@ def __init_subclass_with_meta__(cls, _meta=None, required_fields=None, **options
_meta = CleanedInputTypeOptions(cls)
_meta.required_fields = required_fields
super().__init_subclass_with_meta__(_meta=_meta, **options)


def raise_if_annonymous_user(info):
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be good as part of a base class that all mutations (except the login one) use which always enforces authentication.
That way there's less risk we will accidentally forget to call this somewhere.

if bot.user.id != info.context.user.id:
raise GraphQLError("This is not your bot")

for attr, value in input_object.items():
Copy link
Contributor

Choose a reason for hiding this comment

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

A test to assert that extra fields cannot be included in the payload would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

3 participants