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.

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

Merged
merged 4 commits into from
Jul 30, 2019
Merged

Only set creator when creating task #80

merged 4 commits into from
Jul 30, 2019

Conversation

james1293
Copy link
Contributor

There was a bug:

  1. UserA creates task
  2. UserB saves task
  3. UserB is now the "created_by".

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!

Copy link
Owner

@shacker shacker left a 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,
Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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.

Copy link
Contributor Author

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. 🙂

  1. We want created_by to be changeable by the admin.
  2. However, if we set editable=False, then created_by won't show up in the admin.

Here's the issue I had.

Setup:

  1. Make the proposed changes to task_edit.html and list_detail.py.
  2. 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.

@james1293
Copy link
Contributor Author

I figured out a simpler way to do it ( #81 ), so I'm closing this PR.
As always, let me know if I'm doing GitHub wrong.

@james1293 james1293 closed this Jul 22, 2019
@shacker
Copy link
Owner

shacker commented Jul 23, 2019

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 else block to if form.is_valid() in the TaskList detail view, you can see the error coming through.

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.

@shacker shacker reopened this Jul 23, 2019
@james1293
Copy link
Contributor Author

james1293 commented Jul 23, 2019

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 form.is_valid(), or something else? If it's ok with you, I would like to see the solution you have in mind if/when you get the chance.

(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 created_by ?)

@shacker
Copy link
Owner

shacker commented Jul 24, 2019

Not quite - I would do it right after this line, when you've got a handle on the task object. Note the argument commit=False which means the form bound to a model object but did not push it to the database. And that means it doesn't yet have an ID! So you can do:

if not new_task.id:
    new_task.created_by = request.user
    new_task.created_date = timezone.now()
    ....

Note I'm moving created_date into this if since it's suffering the same bug, so you can fix them both at once.

Also new_task is a bad name for the var, since this is for handling new OR existing tasks. So I would rename new_task to task.

@james1293
Copy link
Contributor Author

james1293 commented Jul 24, 2019

It looks like there are three places that save tasks:

  1. external_add.py
  2. task_detail.py
  3. list_detail.py

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 new_task.created_date = timezone.now() (This mimics external_add.py, which also doesn't set created_date. It looks like the model handles setting created_date because default=timezone.now.)

Thoughts?

@shacker
Copy link
Owner

shacker commented Jul 26, 2019

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 todo/tests/test_views.py::test_no_javascript_in_task_note , which shows how to post data to a form "as" a user. So your test should make a task owned by user1 (the users are created in conftest.py), then user2 will log in and make a change to it, then you can assert that the created_by is unchanged. Are you up for it?

@james1293
Copy link
Contributor Author

I'll give it a shot next week, thanks for the specific suggestions!

@james1293
Copy link
Contributor Author

Test added.

Copy link
Owner

@shacker shacker left a 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!

@shacker shacker merged commit 2d40ef4 into shacker:master Jul 30, 2019
@james1293 james1293 deleted the only-set-creator-creation branch July 30, 2019 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants