Opened 5 years ago

Closed 3 months ago

#13223 closed Bug (duplicate)

ValueError with inline and save as new

Reported by: lucalenardi Owned by: igors
Component: contrib.admin Version: 1.1
Severity: Normal Keywords: admin, save-as-new
Cc: kevin1024, andrew@…, igor@…, samstudio8@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Given two models that are related to each other with a foreign key, as follows:

class Post(models.Model):
    title = models.CharField()
    slug = models.CharField(max_length=200, unique = True)

class Block(models.Model):
    post = models.ForeignKey(Post)
    slug = models.CharField(max_length=200, unique = True)

Suppose you have set save_as = true in the admin.py for the class PostAdmin. When you try to create a new Post, generated from an existing one with the "save as new" button after correcting some validation errors, the following exception is being generated:
ValueError: invalid literal for int() with base 10: ''

If you take a look at the request POST parameters, you could find some empty values for the block_set-N-post block_set-N-id (FK and id).

To replicate the error, follow this sequence:

  1. Create a new object with at least one inline
  2. Save it as new with some validation errors
  3. Try to save it again, after correcting the errors of the form

Attachments (6)

13223.diff (4.8 KB) - added by kevin1024 4 years ago.
13223.v2.diff (4.9 KB) - added by kevin1024 4 years ago.
13223.templatefix.diff (5.0 KB) - added by prestontimmons 4 years ago.
Patch that modifies add_view to maintain the _saveasnew parameter
13223-1.diff (6.1 KB) - added by ramiro 4 years ago.
Patch using the fix strategy proposed by contributor prestontimmons with enhanced tests
13223-2.diff (6.8 KB) - added by ramiro 4 years ago.
Patch updated to current trunk status, still needs work
13223.newview-prototype.diff (8.9 KB) - added by igors 4 years ago.
Prototype for an intermediate clone/ view

Download all attachments as: .zip

Change History (32)

comment:1 Changed 5 years ago by kmtracey

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

#10448 was an earlier ticket in the same area, but it does not seem to have addressed the inline issue.

comment:2 Changed 5 years ago by lucalenardi

The error appears to be related with the value of save_as_new in options.py around line 730.

When the form is submitted the first time, it is set to True, while the second time, after the validation errors, it is set to False since it actually creates a new object. Forcing the value to True, the object is being saved without exceptions (as expected).

comment:3 follow-up: Changed 5 years ago by gptvnt

  • Owner changed from nobody to gptvnt
  • Status changed from new to assigned

comment:4 Changed 5 years ago by paulos

If inline formset contains an image or file field, form will always fail to validate on save_as (#14760).

comment:5 in reply to: ↑ 3 Changed 5 years ago by paulos

Replying to gptvnt:

I think the most reliable way to correct this is creating a new admin URL like:

{{app_name}}/{{model_name}}/{{id}}/save_as/

or

{{app_name}}/{{model_name}}/clone/{{id}}/

comment:6 Changed 5 years ago by sirpengi

Unless a new admin URL is created, the only way to get the current add_view form to behave is to replace the values for INITIAL_FORMS and TOTAL_FORMS, as well as the fk_field value. The INITIAL_FORMS is the cause of the ValueError of the ticket (setting it to 0 makes things happy), and the TOTAL_FORMS/fk_field mess with the form if you try to add any new inlines while you are fixing the validation errors. The TOTAL_FORMS needs to be set to the old INITIAL_FORMS value, and the fk_field needs to be blank.

Last edited 5 years ago by sirpengi (previous) (diff)

comment:7 Changed 4 years ago by lukeplant

  • Type set to Bug

comment:8 Changed 4 years ago by lukeplant

  • Severity set to Normal

Changed 4 years ago by kevin1024

comment:9 Changed 4 years ago by kevin1024

  • Cc kevin1024 added
  • Easy pickings unset
  • Has patch set

OK, I spent some time hacking on this today. Here is a patch. Be gentle; it's my first contribution to Django. :)

comment:10 Changed 4 years ago by prestontimmons

  • Patch needs improvement set

Hi Kevin,

Thanks for putting a patch together. I tested it out. It applies cleanly to revision 16181 and seems to fix the issue here, but the changes to django/forms/formsets.py introduce problems into the model formsets:

ERROR: test_add_form_deletion_when_invalid (modeltests.model_formsets.tests.DeletionTests)
    ...
AttributeError: 'PoetFormFormSet' object has no attribute 'save_as_new'

I'm marking this one as needing improvement.

comment:11 Changed 4 years ago by anonymous

  • Patch needs improvement unset

OK, let's try this again.

Changed 4 years ago by kevin1024

comment:12 Changed 4 years ago by prestontimmons

Thanks, Kevin. I verified your new patch fixes the problem.

I'm hesitant to mark it ready for check-in. The "save_as_new" property is really a part of BaseInlineFormSet. Adding a check for it in BaseFormSet seems wrong. It might be more appropriate to put this fix in a custom "_management_form" method on the BaseInlineFormSet instead.

I'm also attaching a separate patch. Instead of modifying the formset classes it modifies the add_view method to post the value "_saveasnew" on subsequent requests, even when form errors appear. When the form is redisplayed the admin switches from the change_view to the add_view and loses this piece of information in the process.

I'm not sure what the best solution is.

Changed 4 years ago by prestontimmons

Patch that modifies add_view to maintain the _saveasnew parameter

comment:13 Changed 4 years ago by prestontimmons

After some more testing of 13223.v2.diff, I found one problem with it. Since it doesn't bind posted data to the management form, when the form is redisplayed it loses inline data if you add more blocks than the initial form count at the same time you use "save as new".

The newer patch that maintains "_saveasnew" in the template doesn't suffer from that problem.

comment:14 follow-up: Changed 4 years ago by candlerb

  • UI/UX unset

I have a related problem:

  • model admin has save_as = True
  • browse to existing model, click "Save as new"
  • the model fails to validate (e.g. because of a failed uniqueness constraint)
  • when the form is redisplayed with the error, I see "Save and add another" instead of "Save as new"

This behaviour still remains after applying 13223.templatefix.diff (onto released django-1.3), although I've not tried any of the other patches in this thread.

(Platform: python 2.6.5, Ubuntu 10.04.2 LTS)

comment:15 Changed 4 years ago by Andrew Macgregor <andrew@…>

  • Cc andrew@… added

comment:16 in reply to: ↑ 14 Changed 4 years ago by ramiro

Replying to candlerb:

I have a related problem:

  • model admin has save_as = True
  • browse to existing model, click "Save as new"
  • the model fails to validate (e.g. because of a failed uniqueness constraint)
  • when the form is redisplayed with the error, I see "Save and add another" instead of "Save as new"

This behaviour still remains after applying 13223.templatefix.diff (onto released django-1.3), although I've not tried any of the other patches in this thread.

This would be #5681 -- In particular, look at this comment for some discussion about why that behavior isn't wrong IMHO.

comment:17 Changed 4 years ago by ramiro

  • Owner gptvnt deleted
  • Status changed from assigned to new

I've updated the patch contributed by prestontimmons with a more complete test case that simulates a complete workflow as originally reported.

Changed 4 years ago by ramiro

Patch using the fix strategy proposed by contributor prestontimmons with enhanced tests

Changed 4 years ago by ramiro

Patch updated to current trunk status, still needs work

comment:18 Changed 4 years ago by ramiro

  • Patch needs improvement set

I've attached an updated version of the patch that simply allows it to apply cleanly in trunk.

But the OP in ticket #17417 (slightly related to this one) makes a good point regarding the current buggy behavior that isn't fixed by this patch:

If you press "Save as New" without changing the slug field a ValidationError? is raised, and the form is displayed to the user with a warning (as it should). However, the url is that of the old entity ...

That means that at that point the user is presented with the admin add view/form for a new instance of the model, pre-filled with values of the original instance (the one whose edit view/form she was looking at when she decided to press the Save as new button) but the URL in the browser address bar hasn't changed.

IMHO this is semantically incorrect and puts comment:5 in a new perspective. Perhaps we need to implement an intermediate view as suggested there?

Last edited 4 years ago by ramiro (previous) (diff)

comment:19 Changed 4 years ago by igors

  • Owner set to igors
  • Status changed from new to assigned

I was wondering how this intermediate view could be implemented.

My idea is to replace the "Save as New" button for a link to a clone/ view. This view would present a pre-filled form with the data from the original object you'd like to clone. It's (very) similar to add_view , but using initial to fill the form based on the original object.
The formsets are also pre-filled using initial .

All of this happens on GET. POSTing is the same as add_view's POST.

This is the same to call add_view passing initial values as GET parameters. But I think a clone/ url is much cleaner.

Let me know if I'm on the right track so I can continue with the implementation. It's ugly with lot's of duplication and incomplete at this moment, the patch is just a prototype. But I need some feedback before moving on...

Last edited 4 years ago by igors (previous) (diff)

Changed 4 years ago by igors

Prototype for an intermediate clone/ view

comment:20 Changed 4 years ago by igors

  • Cc igor@… added

comment:21 Changed 3 years ago by igors

I'm pushing small commits to my git fork: https://github.com/igorsobreira/django/commits/master (if anyone is interesting in follow or review step-by-step).

When I'm done I'll attach the full patch here.

comment:22 Changed 2 years ago by anonymous

Did anyone else pick this up? The github link above has disappeared to try and pick up his line of work.

comment:23 Changed 2 years ago by dakinsloss

Did anyone else pick this up? The github link above has disappeared to try and pick up his line of work.

comment:24 Changed 2 years ago by igors

Sorry for the broken link @dakinsloss. Since the review/feedback was running to slow I ended up hacking it as an external app.

https://github.com/RealGeeks/django-modelclone

It's basically the intermediate 'clone' view, same form as 'add_view' but already pre-populated.

comment:25 Changed 2 years ago by samstudio8@…

  • Cc samstudio8@… added

comment:26 Changed 3 months ago by timgraham

  • Resolution set to duplicate
  • Status changed from assigned to closed

Duplicate of #23387 which has been fixed in master (1.9) by only showing the "Save as new" button if it's selected and there are validation errors.

Note: See TracTickets for help on using tickets.
Back to Top