-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add deletion feature for comments #660
base: dev
Are you sure you want to change the base?
Conversation
def get_username(self, obj): | ||
return obj.user.username |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this help with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get the username who's commenting on a source. The username of the user is one of the parameters that is going to be posted with the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few clarifying questions but mostly good.
def get_permissions(self): | ||
if self.action == "destroy": | ||
return [permissions.IsAuthenticated()] | ||
return super().get_permissions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only for "destroy"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we wouldn't want anyone else other than the user who posts a comment or the admin to delete a comment. Not really necessary to grab permissions while posting because anyone the user who posts a comment is going to validated through login.
sde_collections/views.py
Outdated
if request.user == comment.user or request.user.is_staff: | ||
return super().destroy(request, *args, **kwargs) | ||
else: | ||
return Response(status=status.HTTP_403_FORBIDDEN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make it that only the user who created the comment can delete it.
function deleteComment(element) { | ||
var commentId = element.getAttribute('data-comment-id'); | ||
$.ajax({ | ||
url: `/api/comments/${commentId}/`, | ||
type: 'DELETE', | ||
headers: { 'X-CSRFToken': csrftoken }, | ||
success: function(result) { | ||
$(element).closest('.comment').remove(); | ||
}, | ||
error: function(xhr, status, error) { | ||
console.error('Comment deletion failed:', error); | ||
} | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@@ -123,6 +123,9 @@ <h1>{{ collection.name }}</h1> | |||
<strong>{{ comment.user.username }}</strong> | |||
<span>{{ comment.created_at|date:"M. d, Y, P" }}</span> | |||
<p>{{ comment.text }}</p> | |||
{% if user.is_authenticated and user == comment.user or user.is_staff %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Business logic repeated here.
<script type="text/javascript"> | ||
var csrftoken = '{{ csrf_token }}'; | ||
</script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works.
|
||
def destroy(self, request, *args, **kwargs): | ||
comment = self.get_object() | ||
if request.user == comment.user: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we'd want the admin to have privileges to delete a comment, but this works too because the user alone owns the responsibility of the comments posted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@code-geek A few changes have been made based on the suggestions. Can you take a look at them?
@bishwaspraveen This feels like useful code that we never merged. Let's clean up conflicts and we can review/merge. |
1 similar comment
@bishwaspraveen This feels like useful code that we never merged. Let's clean up conflicts and we can review/merge. |
@code-geek I'll work on these conflicts and let you know. This is useful code. |
No description provided.