[Django] #27280: can-order: we don't use initial data
Django
--------------------------------------+--------------------
Reporter: Kifsif | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
https://docs.djangoproject.com/en/1.10/topics/forms/formsets/#can-order
{{{
>>> data = {
... 'form-TOTAL_FORMS': '3',
... 'form-INITIAL_FORMS': '2',
... 'form-MAX_NUM_FORMS': '',
... 'form-0-title': 'Article #1',
... 'form-0-pub_date': '2008-05-10',
... 'form-0-ORDER': '2',
... 'form-1-title': 'Article #2',
... 'form-1-pub_date': '2008-05-11',
... 'form-1-ORDER': '1',
... 'form-2-title': 'Article #3',
... 'form-2-pub_date': '2008-05-01',
... 'form-2-ORDER': '0',
... }
>>> formset = ArticleFormSet(data, initial=[
... {'title': 'Article #1', 'pub_date': datetime.date(2008, 5, 10)},
... {'title': 'Article #2', 'pub_date': datetime.date(2008, 5, 11)},
... ])
}}}
We don't use initial data here. It overburdens the documentation.
Offer: formset = ArticleFormSet(data).
--
Ticket URL: <https://code.djangoproject.com/ticket/27280>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Kifsif | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I'm not sure about that suggestion. The initial is needed for the previous
code block on the page and generally you should pass the same `initial`
when processing formset data so that `form.has_changed()` works properly.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:1>
Django
Reporter: Kifsif | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Kifsif):
Replying to [comment:1 Tim Graham]:
> I'm not sure about that suggestion. The initial is needed for the
previous code block on the page and generally you should pass the same
`initial` when processing formset data so that `form.has_changed()` works
properly.
Does "initial" illustrate "can_order"? If not, it misleads the user of the
documentation. If one sees a parameter in the example, one supposes that
it is vitally necessary for the method they are interested in. This is not
a book. This is documentation.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Kifsif | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => invalid
Comment:
Without initial data, the formset will output 1 empty form which isn't
very useful for giving examples of ordering and deleting.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Kifsif):
Replying to [comment:3 Tim Graham]:
> Without initial data, the formset will output 1 empty form which isn't
very useful for giving examples of ordering and deleting.
In this case how can a formset with data for 3 forms produce one empty
form?
In the initial we have meat for 2 forms. And how can it fill the third
one?
I have modelled the situation:
https://bitbucket.org/Kifsif/formsets/commits/f5e05aa61bcfe4be776bd1acc436589d27c78d29
I can't see that it produces an empty form?
I think that in vain you closed the ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Check the output of the first part of the example without initial data:
{{{
for form in formset:
... print(form.as_table())
}}}
Only 1 blank form appears.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Kifsif):
Replying to [comment:5 Tim Graham]:
> Check the output of the first part of the example without initial data:
> {{{
> for form in formset:
> ... print(form.as_table())
> }}}
> Only 1 blank form appears.
Hm. I'm starting to understand you.
This in your opinion is the first part of the example:
{{{
>>> from django.forms import formset_factory
>>> from myapp.forms import ArticleForm
>>> ArticleFormSet = formset_factory(ArticleForm, can_delete=True)
>>> formset = ArticleFormSet(initial=[
... {'title': 'Article #1', 'pub_date': datetime.date(2008, 5, 10)},
... {'title': 'Article #2', 'pub_date': datetime.date(2008, 5, 11)},
... ])
>>> for form in formset:
... print(form.as_table())
}}}
And this is the second:
{{{
>>> data = {
... 'form-TOTAL_FORMS': '3',
... 'form-INITIAL_FORMS': '2',
... 'form-MAX_NUM_FORMS': '',
... 'form-0-title': 'Article #1',
... 'form-0-pub_date': '2008-05-10',
... 'form-0-DELETE': 'on',
... 'form-1-title': 'Article #2',
... 'form-1-pub_date': '2008-05-11',
... 'form-1-DELETE': '',
... 'form-2-title': '',
... 'form-2-pub_date': '',
... 'form-2-DELETE': '',
... }
>>> formset = ArticleFormSet(data, initial=[
... {'title': 'Article #1', 'pub_date': datetime.date(2008, 5, 10)},
... {'title': 'Article #2', 'pub_date': datetime.date(2008, 5, 11)},
... ])
>>> [form.cleaned_data for form in formset.deleted_forms]
}}}
So what? What about the first part if the second part overriddes it
completely.
Well. The first part is ok. I didn't mention it in the ticket.
The second part is erroneous. Because it provides initial data and doesn't
make use of them. This is exactly what I mean: it misleads the user as
s/he may think initial data is necessary here.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
It is necessary. Going back to my first comment on the ticket: "you should
pass the same `initial` when processing formset data so that
`form.has_changed()` works properly."
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Kifsif):
Replying to [comment:7 Tim Graham]:
> It is necessary. Going back to my first comment on the ticket: "you
should pass the same `initial` when processing formset data so that
`form.has_changed()` works properly."
Please, look here. Answer one particular question: is initial data
misleading here or not? If it is, let's change something.
I think it is. We are illustrating can_order/can delete. This means that
we should isolate and demonstrate these two particular phenomena.
has_changed() was demonstrated somewhere earlier. But almost immediately
after that demonstration we can see:
formset = ArticleFormSet(data)
It was 3-5 lines later. Namely here:
https://docs.djangoproject.com/en/1.10/topics/forms/formsets
/#understanding-the-managementform
Then again formset = ArticleFormSet(data) here:
https://docs.djangoproject.com/en/1.10/topics/forms/formsets/#custom-
formset-validation
And close to the end of the article we are demonstrating can_delete. And
somehow we can't help but insert initial data. For some reason we remember
that the article should be consistent with the first "has_changed()"
example.
Once more: this example is misleading. You closed the ticket. Let it be at
least open for other people to express their opinion.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Here's a try at [https://github.com/django/django/pull/7308 a
clarification] to explain why `initial` should be provided.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: 1.10
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Michael, did you take a look at my pull request? Does it help?
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Michael | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michael):
Replying to [comment:10 Tim Graham]:
> Michael, did you take a look at my pull request? Does it help?
Pardon, I somehow failed to notice your update for this ticket.
As far as I can see, you added:
{{{
"Whenever processing a formset submission, you should pass the same
``initial``
that you used to populate the formset so that the formset can detect which
forms were changed by the user. For example, you might have something
like:
``ArticleFormSet(request.POST, initial=[...])``.
}}}
I don't like wording. "Whenever processing a formset you should pass the
same "initial"..." This seems to be even more confusing. I meant that
"initial" is not necessary at all. And now in the documentation there will
be a string about transmitting initial data to the formset whenever we use
it.
Well, the ticket has invalid status. Maybe we shouldn't discuss it?
Invalid is invalid.
If you have any suspicion that the ticket is somehow valid, maybe the
status should be changed? Let other people suggest something.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Michael | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Thanks, I updated the wording on the pull request. Feel free to comment on
the pull request if it's still unclear.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Michael | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michael):
Replying to [comment:12 Tim Graham]:
> Thanks, I updated the wording on the pull request. Feel free to comment
on the pull request if it's still unclear.
Let me comment in this ticket. I would say, the wording is really unclear:
{{{
If you use an ``initial`` for displaying a formset, you should pass the
same
``initial`` when processing that formset's submission so that the formset
can
detect which forms were changed by the user. For example, you might have
something like: ``ArticleFormSet(request.POST, initial=[...])``.
}}}
1. If one passes initial, it does not matter that s/he should pass the
same initial again. Maybe one wants just show initial data.
2. To the best of my ability the only case I can imagine when one needs
both initial and data is for has_changed() method. But you are writing
documentation for "Using initial data with a formset" section. Whereas
has_changed() is in "Formset validation" section (by the way, illustrated
very badly).
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Michael | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Marten Kenbeek):
Replying to [comment:13 Michael]:
> Replying to [comment:12 Tim Graham]:
> > Thanks, I updated the wording on the pull request. Feel free to
comment on the pull request if it's still unclear.
>
> Let me comment in this ticket. I would say, the wording is really
unclear:
>
> {{{
> If you use an ``initial`` for displaying a formset, you should pass the
same
> ``initial`` when processing that formset's submission so that the
formset can
> detect which forms were changed by the user. For example, you might have
> something like: ``ArticleFormSet(request.POST, initial=[...])``.
> }}}
>
> 1. If one passes initial, it does not matter that s/he should pass the
same initial again. Maybe one wants just to show initial data.
> 2. To the best of my ability the only case I can imagine when one needs
both initial and data is for has_changed() method. But you are writing
documentation for "Using initial data with a formset" section. Whereas
has_changed() is in "Formset validation" section (by the way, illustrated
not very well, I think).
>
>
The aim of documentation is not just to explain a concept, but also to
show best practices, and these best practices should be used consistently
across all documentation. While it is not always technically necessary to
pass the same initial data when submitting a form or formset, failure to
do so consistently will give surprising behaviour when it ''is''
necessary. A quick look through the code shows that disabled fields
require the initial data on submission to work properly, and that model
formsets need the initial data to determine which forms should be saved
and which shouldn't. I'm sure I've missed a thing or two.
So I completely agree with Tim that the current use of initial in the
documentation shouldn't be changed. If it is confusing, it should be
cleared up by explaining ''why'' the initial data should be passed when
submitting the form. Removing the initial data when submitting the form in
this example will only create more confusion down the line as people copy
the example and expect it to "just work", only to encounter surprising and
incorrect behaviour when building upon that code.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Michael | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
"If one passes initial, it does not matter that s/he should pass the same
initial again."
This is the incorrect point in your understanding that my patch tries to
clarify. I could amend it to say something like "so that the formset can,
among other things, detect which forms were changed..." if it helps to
drive home the point that passing `initial` to the submitted formset is
important. I don't know that it's important to go into an exhaustive
discussion of all the places where `initial` might be used in a submitted
formset, but Marten's comment is correct in pointing out another place
where it's used.
Feel free to suggest an alternate wording if you find the current proposal
unclear.
--
Ticket URL: <https://code.djangoproject.com/ticket/27280#comment:15>