users can delete their course#533
Conversation
WalkthroughThis pull request adds a custom Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant T as Template
participant V as delete_course View
participant M as Course Model
participant S as Storage
U->>T: Submit deletion confirmation form
T->>V: POST deletion request (with course slug)
alt Authorized
V->>M: Invoke custom delete() method
M->>S: If image exists, delete image (save=False)
M->>M: Delete Course instance
V->>U: Redirect to profile page
else Unauthorized
V->>U: Display error: "You are not authorized to delete this course."
end
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
web/models.py(1 hunks)web/templates/courses/delete_confirm.html(1 hunks)web/views.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/models.py (1)
web/forms.py (5)
save(245-283)save(594-619)save(704-721)save(1424-1437)save(1616-1631)
🪛 Ruff (0.8.2)
web/models.py
315-315: Missing return type annotation for public function delete
Add return type annotation: None
(ANN201)
315-315: Missing type annotation for *args
(ANN002)
315-315: Missing type annotation for **kwargs
(ANN003)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Tests
- GitHub Check: build-and-test
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/templates/courses/delete_confirm.html (1)
1-36: Overall Template Quality and CSRF Handling.
The template maintains a clear structure and consistent styling that aligns with the project’s design guidelines. Notably, the issue from previous revisions regarding duplicate{% csrf_token %}instances has been resolved, as only one token is now present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
web/templates/courses/delete_confirm.html(1 hunks)web/views.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
web/views.py
923-923: Logging statement uses f-string
(G004)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Tests
- GitHub Check: build-and-test
🔇 Additional comments (7)
web/templates/courses/delete_confirm.html (3)
16-20: Enhanced Form Submission Configuration.
The<form>tag now includes theactionattribute using{% url 'delete_course' course.id %}, which ensures that deletion requests are correctly routed to the proper endpoint. Additionally, the presence of the hidden input field forcourse_idand a single{% csrf_token %}adheres to Django best practices.
22-31: Clear and Accessible Action Buttons.
The design of the Cancel link and Delete Course button is clear. The Cancel link effectively redirects users to the course details page via{% url 'course_detail' course.slug %}, while the Delete Course button is styled to emphasize the destructive nature of the action.
11-15: User Confirmation Prompt Clarity.
The confirmation message clearly informs the user that deleting the course (i.e., "{{ course.title }}") is irreversible, thereby reducing the risk of accidental deletion.web/views.py (4)
909-909: Good addition of a clear docstring.The addition of a docstring clearly explains the function's dual purpose of handling both course and image deletion, improving code maintainability.
914-914: Improved error message for unauthorized access.The updated error message "You are not authorized to delete this course" is more professional and generic than the previous message, providing better clarity to users.
918-924: Well-implemented image deletion with proper error handling.This code properly checks if an image exists before attempting deletion and includes error handling for potential file system issues. The warning is appropriately logged if deletion fails, which aids in debugging.
# Delete the course image if it exists if course.image and os.path.isfile(course.image.path): try: os.remove(course.image.path) except (PermissionError, OSError) as e: logger.warning(f"Failed to delete image for course {course.slug}: {e}")🧰 Tools
🪛 Ruff (0.8.2)
923-923: Logging statement uses f-string
(G004)
928-928: Good post-deletion redirect.After successful deletion, redirecting to the profile page is appropriate as it returns the user to a relevant page in the application flow.
IITI-tushar
force-pushed
the
users-delete-course
branch
from
f9cdf0a to
917b0b7
Compare
IITI-tushar
force-pushed
the
users-delete-course
branch
from
917b0b7 to
e51f610
Compare
IITI-tushar
force-pushed
the
users-delete-course
branch
from
e51f610 to
13eb036
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web/views.py (1)
967-984: 🧹 Nitpick (assertive)Good implementation of image deletion before course removal.
The function now properly handles image cleanup during course deletion with:
- A clear docstring explaining the purpose
- An improved error message for unauthorized access
- Checks if an image exists and removes it from the filesystem
While this works well, consider adding error handling for the file deletion operation to prevent potential issues with file permissions:
# Delete the course image if it exists if course.image and os.path.isfile(course.image.path): - os.remove(course.image.path) + try: + os.remove(course.image.path) + except (PermissionError, OSError) as e: + logger.warning(f"Failed to delete image for course {course.slug}: {e}")web/models.py (1)
315-318: 🧹 Nitpick (assertive)Good implementation for proper resource cleanup.
This method ensures that any associated image files are properly removed from storage when a Course instance is deleted, preventing orphaned files in the filesystem.
To align with static analysis recommendations, consider adding type annotations:
- def delete(self, *args, **kwargs): + def delete(self, *args: Any, **kwargs: Any) -> None: if self.image: self.image.delete(save=False) super().delete(*args, **kwargs)You would need to add
from typing import Anyat the top of the file.🧰 Tools
🪛 Ruff (0.8.2)
315-315: Missing return type annotation for public function
deleteAdd return type annotation:
None(ANN201)
315-315: Missing type annotation for
*args(ANN002)
315-315: Missing type annotation for
**kwargs(ANN003)
web/templates/courses/delete_confirm.html (1)
16-20:⚠️ Potential issueForm implementation looks good but contains a duplicate CSRF token.
The form action is correctly set to target the delete_course view with the course ID, and the hidden input field properly passes the course identifier to the backend.
There's a duplicate CSRF token in the form (line 21). Since Django only requires one token per form, remove the duplicate:
<form method="post" action="{% url 'delete_course' course.id %}" class="space-y-6"> {% csrf_token %} <input type="hidden" name="course_id" value="{{ course.id }}" /> - {% csrf_token %} <div class="flex justify-center space-x-4">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
web/models.py(1 hunks)web/templates/courses/delete_confirm.html(1 hunks)web/templates/courses/detail.html(1 hunks)web/views.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/views.py (1)
web/models.py (4)
Course(233-332)image(1246-1251)delete(315-318)delete(472-478)
🪛 Ruff (0.8.2)
web/models.py
315-315: Missing return type annotation for public function delete
Add return type annotation: None
(ANN201)
315-315: Missing type annotation for *args
(ANN002)
315-315: Missing type annotation for **kwargs
(ANN003)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Run Tests
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/models.py (1)
315-318: Great implementation of proper resource cleanup!The override of the
deletemethod ensures that any associated image files are properly removed from storage when aCourseinstance is deleted, preventing orphaned files in the filesystem. This is good practice for resource management.Consider adding type annotations to align with the static analysis recommendations:
- def delete(self, *args, **kwargs): + def delete(self, *args: Any, **kwargs: Any) -> None: if self.image: self.image.delete(save=False) super().delete(*args, **kwargs)You would need to add
from typing import Anyat the top of the file.🧰 Tools
🪛 Ruff (0.8.2)
315-315: Missing return type annotation for public function
deleteAdd return type annotation:
None(ANN201)
315-315: Missing type annotation for
*args(ANN002)
315-315: Missing type annotation for
**kwargs(ANN003)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
web/models.py(1 hunks)web/templates/courses/detail.html(1 hunks)web/views.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/views.py (1)
web/models.py (4)
Course(233-332)image(1248-1253)delete(315-318)delete(472-478)
🪛 Ruff (0.8.2)
web/models.py
315-315: Missing return type annotation for public function delete
Add return type annotation: None
(ANN201)
315-315: Missing type annotation for *args
(ANN002)
315-315: Missing type annotation for **kwargs
(ANN003)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Tests
- GitHub Check: build-and-test
🔇 Additional comments (3)
web/views.py (3)
967-970: Docstring and spacing improvements enhance readability.The addition of a clear docstring and proper spacing improves code readability and maintainability.
972-973: Improved error message for unauthorized access.The error message is now more clear and professional, which improves the user experience.
983-983: Appropriate redirection after course deletion.Redirecting to the profile page after deleting a course provides a clear user flow.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
web/templates/courses/delete_confirm.html(1 hunks)web/templates/courses/detail.html(1 hunks)web/views.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/views.py (1)
web/models.py (3)
Course(233-332)delete(315-318)delete(472-478)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Run Tests
🔇 Additional comments (2)
web/templates/courses/delete_confirm.html (1)
16-18:Details
✅ Verification successful
Confirm the form action matches your URL pattern name and parameter
The addition ofaction="{% url 'delete_course' course.slug %}"ensures the form submits to the delete view, but please verify that your URLconf declares a
delete_courseroute accepting aslug(notid).Run this script to locate and inspect the pattern in your URL definitions:
🏁 Script executed:
#!/bin/bash # Verify the delete_course URL pattern exists and uses <slug:slug> rg -n "delete_course" -A2Length of output: 1203
Confirmed
delete_courseURL pattern uses<slug:slug>
The URLconf inweb/urls.pydefines:path("courses/<slug:slug>/delete/", views.delete_course, name="delete_course")and the view signature
delete_course(request, slug)matches. The form’saction="{% url 'delete_course' course.slug %}"is correct.web/views.py (1)
965-982: Well-implemented course deletion function.The changes to the
delete_coursefunction properly handle course deletion by:
- Adding a clear docstring explaining the purpose
- Improving the error message for unauthorized access
- Relying on the model's
deletemethod to handle image deletion automatically- Including a clarifying comment about how the course image gets removed
This implementation correctly addresses the prior feedback by eliminating redundant image deletion logic which could have caused errors.
🧰 Tools
🪛 Ruff (0.8.2)
966-966: Missing return type annotation for public function
delete_course(ANN201)
966-966: Missing type annotation for function argument
request(ANN001)
966-966: Missing type annotation for function argument
slug(ANN001)
IITI-tushar
force-pushed
the
users-delete-course
branch
from
b6f706c to
2469edf
Compare
2 participants
fixes : issue#532
Summary by CodeRabbit
Summary by CodeRabbit