-
Notifications
You must be signed in to change notification settings - Fork 868
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
'Solution' #897
base: master
Are you sure you want to change the base?
'Solution' #897
Conversation
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.
Thank you for your hard work on this project! However, there are a few critical issues that need to be addressed before we can proceed:
-
Incorrect Model Usage in Views: The
ManufacturerCreateView
,ManufacturerUpdateView
, andManufacturerDeleteView
are incorrectly using theCar
model instead of theManufacturer
model. This is a critical issue as it affects the core functionality of your application. -
Missing Cancel Button: The
manufacturer_confirm_delete.html
andcar_confirm_delete.html
templates are missing aCancel
button, which is a requirement. -
Incorrect URL Naming: The URLs in
car_detail.html
should use hyphens instead of underscores to meet the task requirements. -
Unnecessary Fields Attribute: The
fields
attribute inCarDeleteView
andManufacturerDeleteView
should be removed as it is not applicable for delete views. -
Code Style: Ensure there is a blank line at the end of all files to adhere to code style guidelines.
Please address these issues, and feel free to reach out if you need any help. Once these changes are made, your code will be in great shape! Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
taxi/views.py
Outdated
|
||
|
||
class ManufacturerCreateView(LoginRequiredMixin, generic.CreateView): | ||
model = Car |
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.
The ManufacturerCreateView
should use the Manufacturer
model instead of the Car
model. This is likely a copy-paste error.
taxi/views.py
Outdated
|
||
|
||
class ManufacturerUpdateView(LoginRequiredMixin, generic.UpdateView): | ||
model = Car |
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.
The ManufacturerUpdateView
should use the Manufacturer
model instead of the Car
model. This is likely a copy-paste error.
taxi/views.py
Outdated
|
||
|
||
class ManufacturerDeleteView(LoginRequiredMixin, generic.DeleteView): | ||
model = Car |
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.
The ManufacturerDeleteView
should use the Manufacturer
model instead of the Car
model. This is likely a copy-paste error.
taxi/views.py
Outdated
|
||
class CarDeleteView(LoginRequiredMixin, generic.DeleteView): | ||
model = Car | ||
fields = "__all__" |
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.
The fields
attribute is not applicable for DeleteView
. You should remove this line from the CarDeleteView
.
taxi/views.py
Outdated
|
||
class ManufacturerDeleteView(LoginRequiredMixin, generic.DeleteView): | ||
model = Car | ||
fields = "__all__" |
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.
The fields
attribute is not applicable for DeleteView
. You should remove this line from the ManufacturerDeleteView
.
{% load crispy_forms_filters%} | ||
{% block content %} | ||
<h1>Delete Car</h1> | ||
<p> Are you sure you want delete car: {{ car }} ? </p> |
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.
There is a typo in the sentence: 'Are you sure you want delete car: {{ car }} ?'. It should be 'Are you sure you want to delete the car: {{ car }}?'
{% block content %} | ||
<h1>Delete Car</h1> | ||
<p> Are you sure you want delete car: {{ car }} ? </p> | ||
<p><i> All drivers with this cae also will be deleted </i>></p> |
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.
There is a typo in the sentence: 'All drivers with this cae also will be deleted'. It should be 'All drivers with this car will also be deleted'.
@@ -1,12 +1,13 @@ | |||
{% extends "base.html" %} | |||
|
|||
{% block content %} | |||
<h1>Car list</h1> | |||
<h1>Car list 111</h1> |
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.
The heading 'Car list 111' seems to include an unintended '111'. It should probably be 'Car List'.
{% load crispy_forms_filters%} | ||
{% block content %} | ||
<h1>Delete Manufacturer</h1> | ||
<p> Are you sure you want delete manufacturer: {{ manufacturer }} ? </p> |
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.
There is a typo in the sentence: 'Are you sure you want delete manufacturer: {{ manufacturer }} ?'. It should be 'Are you sure you want to delete the manufacturer: {{ manufacturer }}?'
{% block content %} | ||
<h1>Delete Manufacturer</h1> | ||
<p> Are you sure you want delete manufacturer: {{ manufacturer }} ? </p> | ||
<p><i> All drivers with this cae also will be deleted </i>></p> |
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.
There is a typo in the sentence: 'All drivers with this cae also will be deleted'. It should be 'All drivers with this car will also be deleted'.
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.
Great job on addressing the issues from the previous review! 🎉 Your code now meets the task requirements and constraints. However, there are a few areas to improve:
-
URL Naming Convention: Ensure that all URLs use hyphens instead of underscores. This applies to several places, such as
'taxi:manufacturer-delete'
,'taxi:manufacturer-update'
,'taxi:car-detail'
,'car-delete'
, and'car-update'
. -
Typographical Errors: Correct the typo in the text from 'cae' to 'car' in the message: "All drivers with this car also will be deleted."
-
success_url
inCarDeleteView
: Make sure thesuccess_url
redirects to the car list view usingreverse_lazy("taxi:car-list")
instead of the car detail view.
These adjustments are crucial for the functionality and readability of your code. Keep up the good work, and make sure to address these points to enhance your learning experience. Remember, learning from these reviews will help you become a better developer. 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
taxi/views.py
Outdated
|
||
class CarDeleteView(LoginRequiredMixin, generic.DeleteView): | ||
model = Car | ||
success_url = reverse_lazy("taxi:car-detail") |
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.
The success_url
for CarDeleteView
should redirect to the car list view, not the car detail view. Change it to reverse_lazy("taxi:car-list")
.
{% block content %} | ||
<h1>Delete Car</h1> | ||
<p> Are you sure you want delete car: {{ car }} ? </p> | ||
<p><i> All drivers with this cae also will be deleted </i>></p> |
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.
There is a typo in the text: 'cae' should be 'car'. Correct it to 'All drivers with this car also will be deleted'.
@@ -9,4 +9,7 @@ <h1>Drivers</h1> | |||
<li>{{ driver.username }} ({{ driver.first_name }} {{ driver.last_name }})</li> | |||
{% endfor %} | |||
</ul> | |||
|
|||
<a href="{% url 'car-update' car.id %}">Update</a> |
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.
The URL name should use hyphens instead of underscores. Change 'car-update'
to 'car-update'
.
@@ -9,4 +9,7 @@ | |||
<li>{{ driver.username }} ({{ driver.first_name }} {{ driver.last_name }})</li> | |||
{% endfor %} | |||
</ul> | |||
|
|||
<a href="{% url 'car-update' car.id %}">Update</a> | |||
<a href="{% url 'car-delete' car.id %}">Delete</a> |
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.
The URL name should use hyphens instead of underscores. Change 'car-delete'
to 'car-delete'
.
{% if car_list %} | ||
<ul> | ||
{% for car in car_list %} | ||
<li> | ||
<a href="{% url "taxi:car-detail" pk=car.id %} ">{{ car.id }}</a> | ||
<a href="{% url 'taxi:car-detail' pk=car.id %} ">{{ car.id }}</a> |
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.
The URL name should use hyphens instead of underscores. Change 'taxi:car-detail'
to 'taxi:car-detail'
.
{% block content %} | ||
<h1>Delete Manufacturer</h1> | ||
<p> Are you sure you want delete manufacturer: {{ manufacturer }} ? </p> | ||
<p><i> All drivers with this cae also will be deleted </i>></p> |
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.
There is a typo in the text: 'cae' should be 'car'. Correct it to 'All drivers with this car also will be deleted'.
@@ -23,6 +25,12 @@ <h1>Manufacturer List | |||
<td> | |||
{{ manufacturer.country }} | |||
</td> | |||
<td> | |||
<a href="{% url 'taxi:manufacturer-update' pk=manufacturer.id %}">UPDATE</a> |
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.
The URL name should use hyphens instead of underscores. Change 'taxi:manufacturer-update'
to 'taxi:manufacturer-update'
.
<a href="{% url 'taxi:manufacturer-update' pk=manufacturer.id %}">UPDATE</a> | ||
</td> | ||
<td> | ||
<a href="{% url 'taxi:manufacturer-delete' pk=manufacturer.id %}">DELETE</a> |
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.
The URL name should use hyphens instead of underscores. Change 'taxi:manufacturer-delete'
to 'taxi:manufacturer-delete'
.
No description provided.