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

implement create region in graphql #1449

Merged
merged 9 commits into from
Aug 16, 2024
Merged

Conversation

sudan45
Copy link
Contributor

@sudan45 sudan45 commented Apr 3, 2024

Addresses

Changes

  • implemented graphql of create region

This PR doesn't introduce any:

  • temporary files, auto-generated files or secret keys
  • n+1 queries
  • flake8 issues
  • print
  • typos
  • unwanted comments

This PR contains valid:

  • tests
  • permission checks (tests here too)
  • translations

susilnem
susilnem previously approved these changes Apr 3, 2024
@sudan45 sudan45 force-pushed the features/add-region-mutation branch from 3743ad3 to 5e1b56a Compare April 4, 2024 09:53
@sudan45 sudan45 mentioned this pull request Apr 8, 2024
9 tasks
@sudan45 sudan45 self-assigned this Apr 8, 2024
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/tests/test_mutations.py Show resolved Hide resolved
apps/geo/tests/test_mutations.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/mutations.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Show resolved Hide resolved
@sudan45 sudan45 force-pushed the features/add-region-mutation branch from 5e1b56a to df9b4fa Compare April 8, 2024 10:38
@sudan45 sudan45 removed their assignment Apr 9, 2024
@sudan45 sudan45 requested review from thenav56 and k9845 April 9, 2024 06:09
@sudan45 sudan45 changed the base branch from develop to project/rest-to-graphql April 9, 2024 06:11
@sudan45 sudan45 dismissed susilnem’s stale review April 9, 2024 06:11

The base branch was changed.

@sudan45 sudan45 marked this pull request as draft April 9, 2024 09:20
@sudan45 sudan45 force-pushed the features/add-region-mutation branch from 80c9696 to 7dc8d5c Compare April 9, 2024 09:23
@sudan45 sudan45 marked this pull request as ready for review April 9, 2024 09:30
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
schema.graphql Outdated Show resolved Hide resolved
Copy link
Member

@susilnem susilnem left a comment

Choose a reason for hiding this comment

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

Let's verify with the client If field project is mandatory or not.

apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
@sudan45 sudan45 changed the base branch from project/rest-to-graphql to develop April 30, 2024 06:02
@sudan45 sudan45 force-pushed the features/add-region-mutation branch 3 times, most recently from 94d0af6 to 528ba78 Compare April 30, 2024 09:19
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
@sudan45 sudan45 force-pushed the features/add-region-mutation branch 3 times, most recently from f8de780 to 6d317f3 Compare July 30, 2024 09:03
@sudan45 sudan45 marked this pull request as ready for review July 31, 2024 05:10
@sudan45 sudan45 requested a review from thenav56 July 31, 2024 05:10
@sudan45 sudan45 force-pushed the features/add-region-mutation branch 2 times, most recently from c8c7600 to 54f6302 Compare August 13, 2024 05:51
apps/geo/mutations.py Outdated Show resolved Hide resolved
apps/geo/mutations.py Outdated Show resolved Hide resolved
apps/geo/mutations.py Outdated Show resolved Hide resolved
apps/geo/mutations.py Show resolved Hide resolved
apps/geo/schema.py Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/serializers.py Show resolved Hide resolved
apps/geo/serializers.py Outdated Show resolved Hide resolved
apps/geo/tests/test_mutations.py Outdated Show resolved Hide resolved
apps/geo/tests/test_mutations.py Outdated Show resolved Hide resolved
apps/geo/tests/test_mutations.py Outdated Show resolved Hide resolved
apps/geo/tests/test_mutations.py Outdated Show resolved Hide resolved
schema.graphql Show resolved Hide resolved
@sudan45 sudan45 force-pushed the features/add-region-mutation branch 3 times, most recently from b7e7eef to 4d2873c Compare August 14, 2024 06:05
Comment on lines 88 to 93
if admin_level_qs is None:
error_data.append('AdminLevel does\'t exist')
elif admin_level_qs.region.created_by != info.context.user:
error_data.append('Region Owner can delete admin level ')
elif admin_level_qs.region.is_published:
error_data.append('Published region can\'t be changed. Please contact')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
if admin_level_qs is None:
error_data.append('AdminLevel does\'t exist')
elif admin_level_qs.region.created_by != info.context.user:
error_data.append('Region Owner can delete admin level ')
elif admin_level_qs.region.is_published:
error_data.append('Published region can\'t be changed. Please contact')
if admin_level_qs is None:
error_data.append("AdminLevel does't exist")
elif admin_level_qs.region.created_by_id != info.context.user.id:
error_data.append('Only region owner can delete admin level ')
elif admin_level_qs.region.is_published:
error_data.append("Published region can't be changed. Please contact system admin")

Comment on lines 94 to 102
else:
admin_level_qs.delete()
return DeleteAdminLevel(errors=None, ok=True)
return DeleteAdminLevel(errors=[
dict(
field='nonFieldErrors',
messages=error_data
)
], ok=False)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
else:
admin_level_qs.delete()
return DeleteAdminLevel(errors=None, ok=True)
return DeleteAdminLevel(errors=[
dict(
field='nonFieldErrors',
messages=error_data
)
], ok=False)
if error_data:
return DeleteAdminLevel(errors=[
dict(
field='nonFieldErrors',
messages=error_data
)
], ok=False)
admin_level.delete()
return DeleteAdminLevel(errors=None, ok=True)

region = data.get('region', (self.instance and self.instance.region))
if not region.can_modify(self.context['request'].user):
raise serializers.ValidationError('You don\'t have the access to the region or region is published')
return data
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirm with @subinasr to check error response in required format

@sudan45 sudan45 force-pushed the features/add-region-mutation branch from 4d2873c to f243f01 Compare August 14, 2024 08:57
@thenav56
Copy link
Member

@subinasr Please squash and merge this hae... Lots of comments are review fixes.

* Update testcase  of create region
* Update testcase of region mutation
* Update testcase of region mutation
* Update testcase and refactor
* Update testcase  of create region
* Update testcase of region mutation
* Update testcase of region mutation
* Update testcase and refactor
* Optimize testcases and error messages
@AdityaKhatri AdityaKhatri force-pushed the features/add-region-mutation branch from f243f01 to 3013525 Compare August 16, 2024 06:18
@AdityaKhatri AdityaKhatri merged commit dd2d05a into develop Aug 16, 2024
7 checks passed
@AdityaKhatri AdityaKhatri deleted the features/add-region-mutation branch August 16, 2024 06:22
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.

7 participants