-
Notifications
You must be signed in to change notification settings - Fork 285
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
Only set creator when creating task #80
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.
Good find, and thanks for the PR! One change request, please.
todo/models.py
Outdated
@@ -80,6 +80,7 @@ class Task(models.Model): | |||
null=True, | |||
related_name="todo_created_by", | |||
on_delete=models.CASCADE, | |||
editable=False, |
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.
I don't agree with this change - Django docs: "If False, the field will not be displayed in the admin or any other ModelForm." We DO want it displayed in the admin, and in fact should be editable in the admin (superuser needs to be able to override this sort of thing). This change does not really really address the bug. And if we were to make a model change, you'd need to include the model migration in your pull request! But please remove this line.
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.
Good point. IIRC, form.is_valid()
returns False
if you leave that out, so I'll need to dig into the Django docs to figure out an alternate solution.
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.
I think you might have misread. This property only controls whether people can edit the field in the admin, and has nothing to do with whether data in a form conforms to requirements.
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.
I'll explain as best I understand; let me know if there are any errors. 🙂
- We want
created_by
to be changeable by the admin. - However, if we set
editable=False
, thencreated_by
won't show up in the admin.
Here's the issue I had.
Setup:
- Make the proposed changes to
task_edit.html
andlist_detail.py
. - Try to create a task.
Result: Task is not created.
I believe this is because the form validation is failing, but not sure. Going to experiment.
I figured out a simpler way to do it ( #81 ), so I'm closing this PR. |
I can reproduce the bug, good find. The bug is that the template sends in the created_by variable at all. But if you remove that line from the template, the task isn't created at all. If you add an The fix will involve 1) Remove that line from the template code - it shouldn't be there; 2) Modify the view code so that it sets created_by to request.user but ONLY if this is a new task. I can fix this, but want to give you the chance since it seems like you're an eager contributor :) As for "doing github wrong," yeah, you want to keep a single PR focused on a single issue. You should keep making changes to the same branch as the PR until it's accepted (there was no need to create a new PR for this - just keep changing your code until we agree it's right). But no worries - we can work on either branch. |
Thanks for the feedback! You said to "Modify the view code so that it sets created_by to request.user but ONLY if this is a new task." Are you thinking of modifying the form data here just before (Sidenote - I'm trying to figure out what flaws there might be in #81 since it appears to work. Is it the risk of a user intentionally modifying |
Not quite - I would do it right after this line, when you've got a handle on the task object. Note the argument
Note I'm moving created_date into this Also |
It looks like there are three places that save tasks: task_detail.py, AFAICT, is used if-and-only-if you are editing a task. So I think list_detail.py adds tasks and cannot edit existing tasks. Sidenote: I added another commit to remove the seemingly unneeded Thoughts? |
These changes will work great, thanks @james1293 . Last step here is to add a test. Would you like to do that, or should I? A good example of the test setup you'll need at |
I'll give it a shot next week, thanks for the specific suggestions! |
Test added. |
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.
Excellent test, thank you!
There was a bug:
In other words, "created_by" was acting more like "last modified by".
I'm still pretty new to Django, so please let me know if you have any feedback on this fix. Thanks!