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

Add UserDeleteView #934

Open
wants to merge 2 commits into
base: cs-pkg-cat-edit
Choose a base branch
from
Open

Add UserDeleteView #934

wants to merge 2 commits into from
+114 −1

Conversation

Oksamies
Copy link
Contributor

@Oksamies Oksamies commented Dec 8, 2023

No description provided.

@Oksamies Graphite App
Copy link
Contributor Author

Oksamies commented Dec 8, 2023 •
edited
Loading

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@Oksamies Oksamies mentioned this pull request Dec 8, 2023
Closed
@codecov Codecov
Copy link

codecov bot commented Dec 8, 2023 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (cs-pkg-cat-edit@3d21030). Learn more about missing BASE report.

❗ Current head e27c47d differs from pull request most recent head 1d0e07f

Please upload reports for the commit 1d0e07f to get more accurate results.

Files Patch % Lines
django/thunderstore/social/views.py 66.66% 1 Missing ⚠️
Additional details and impacted files 
@@                Coverage Diff                 @@
##             cs-pkg-cat-edit     #934   +/-   ##
==================================================
  Coverage                   ?   93.15%           
==================================================
  Files                      ?      300           
  Lines                      ?     8764           
  Branches                   ?      780           
==================================================
  Hits                       ?     8164           
  Misses                     ?      494           
  Partials                   ?      106

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

@Oksamies Oksamies force-pushed the add-delete-user-endpoint branch 6 times, most recently from 73f4b22 to 6d7cb43 Compare December 12, 2023 10:08
MythicManiac
MythicManiac requested changes Dec 12, 2023
View reviewed changes
django/thunderstore/api/cyberstorm/views/user.py Outdated
data=request.data,
)
if form.is_valid():
request.user.delete()
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved under the form if we want to use the form as the proper way to perform this action (which I'd say is the easiest option). The alternative would be some dedicated function that gets called for user deletes (and does not reside in any API module, but some module where it logically makes the most sense to exist in).

So I'd suggest adding a method to the form that performs the deletion action and refactoring the previous form handler to call that as well (since it uses a FormView base class, it might even call it automatically already).

django/thunderstore/api/cyberstorm/views/user.py Outdated
operation_id="cyberstorm.user.delete",
tags=["cyberstorm"],
)
def post(self, request, username, format=None):
Copy link
Member

Choose a reason for hiding this comment

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

Could use type annotations (at least for the username) + you can get rid of format entirely

django/thunderstore/api/cyberstorm/views/user.py Outdated
raise PermissionDenied("You can only delete your own account")
form = DeleteAccountForm(
user=request.user,
data=request.data,
Copy link
Member

Choose a reason for hiding this comment

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

Use the serializer validated data

django/thunderstore/api/cyberstorm/views/user.py Outdated
Comment on lines 40 to 42
CyberstormUserDeleteResponseSerialiazer(
{"username": request.user.username}
).data
Copy link
Member

Choose a reason for hiding this comment

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

Just returning a response with no body is sufficient, no need to use a serializer or anything.

django/thunderstore/api/cyberstorm/views/user.py Outdated
from thunderstore.api.utils import conditional_swagger_auto_schema
from thunderstore.social.views import DeleteAccountForm

User = get_user_model()
Copy link
Member

Choose a reason for hiding this comment

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

Why is this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Left over, weird that IDE didn't complain about it.

django/thunderstore/api/cyberstorm/views/user.py Outdated
Comment on lines 31 to 32
if request.user.username != username:
raise PermissionDenied("You can only delete your own account")
Copy link
Member

Choose a reason for hiding this comment

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

Given this constraint exists, I feel like this view should not take the URL path parameter at all and instead just treat it as one of the "current user" APIs that are around, e.g. /current-user/delete/. No need to add this condition at all

django/thunderstore/api/cyberstorm/views/user.py
username = serializers.CharField()


class UserDeleteAPIView(APIView):
Copy link
Member

Choose a reason for hiding this comment

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

You're missing permission classes (specifically at least the IsAuthenticated should be set to this view)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on if #953 was merged, I'll either add it or not add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the new views should now have IsAuthenticated

@Oksamies Oksamies force-pushed the add-delete-user-endpoint branch 4 times, most recently from 7c4a8e5 to 67fb284 Compare December 13, 2023 16:13
@Oksamies Oksamies changed the base branch from master to cs-pkg-cat-edit June 24, 2024 13:52
This was referenced Jun 24, 2024
Open
Open
Open
Open
Open
Merged
Open
Merged
Open
Merged
Open
Closed
This was referenced Jun 24, 2024
Open
Closed
@Oksamies Oksamies mentioned this pull request Jun 24, 2024
Open
This was referenced Jul 18, 2024
Open
Open
@Oksamies Oksamies mentioned this pull request Aug 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MythicManiac MythicManiac MythicManiac requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Oksamies @MythicManiac