Only set creator when creating task

[james1293]

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!

[james1293]

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.

[shacker]

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.

[james1293]

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]

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]

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]

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]

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

[james1293]

Test added.