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

FormCollection self.instance bug due to FormCollection.full_clean() #159

Open
codematsing opened this issue Aug 9, 2024 · 3 comments
Open

Comments

@codematsing
Copy link

codematsing commented Aug 9, 2024

Given

#models.py
class ParentModel(self):
    name = models.CharField
    
class ChildModel(self):
    parent = models.ForeignKey(Parent, related_name='children', on_delete=models.CASCADE)
    name = models.CharField

#forms.py
class ChildForm(ModelForm):
  class Meta:
    model = ChildModel()
    fields = '__all__'

class ParentChildFormCollection(FormCollection):
    child_formset = ChildForm()

class UpdateView(SingleObjectMixin, FormCollectionView):
    model = ParentModel
    collection_class = ParentChildFormCollection()

    def get_initial():
      parent_object = self.get_object()
      initial = [
        {"child_form": {child.name for child in parent.children.all()}}
      ]
      return initial

Expectation:

#forms.py
class ParentChildFormCollection():
  def save(self):
    logger.info(self.instance.name)
    logger.info(type(self.instance))
    ...
  • Expectation self.instance.name should consistently be the parent_object.name passed in FormCollectionView.get_form_kwargs()

Actual Results:

  • self.instance.name would refer to the last object child.name after FormCollection.full_clean()

Observations:

# in class FormCollection
    def full_clean(self):
       ...
        logger.info(instance) #print parent_name
        if holder.is_valid():
            valid_holders[name] = holder
            logger.info(instance) #prints_child_name
      ...

Note:
I'll to replicate this in django-formset using the repo. I'll get back to you on this as I'm swamped at the moment.

@codematsing codematsing changed the title BaseFormCollection self.instance bug due to FormCollection.full_clean() FormCollection self.instance bug due to FormCollection.full_clean() Aug 9, 2024
codematsing added a commit to codematsing/django-formset-debug that referenced this issue Aug 9, 2024
@codematsing
Copy link
Author

codematsing commented Aug 9, 2024

Replicated:
releases/1.5...codematsing:django-formset-debug:releases/1.5

How to test:

  • Go to: http://127.0.0.1:8000/bootstrap/companydepartmentformset
  • Check CompanyDepartmentUpdateView.form_kwargs. It replicates the functionality of UpdateViews passing a form_kwargs["instance"] to Form
  • Expectation is that the first Company Instance in the database will be used but retained throughout the process since Company data is not changed in saving mechanism
  • type more than one department name in form and submit. For my case, I added once department with name="new"
  • Check console logs

image

Notes:
I did add my workaround so that form collection functionality will not interfere with self.instance data but I bet there's a better approach to this

image

Why is this important:

  • I believe there are a lot of usecases where you would want to use a FormCollection on an UpdateView.
  • Since native django uses ModelForm.instance to pass UpdateView.object to form, UpdateView.object data would be accidentally changed due to integration of FormCollectionView

How I stumbled upon this issue

  • My saving implementation for a model with similar relationship as Company ||--o{ Department would go as follow
class CompanyDepartmentUpdateView(SingleObjectMixin, FormCollectionView):
     model = Company
     collection_class = CompanyDepartmentFormset

     def form_collection_valid(self, form_collection):
          form_collection.save()

class FormCollection(FormCollection):
     department_formset = DepartmentForm()

     def save(self):
         for department_kwargs in self.cleaned_data['department_formset']:
               Department.objects.create(company=self.instance, **deparment_kwargs)

@jrief
Copy link
Owner

jrief commented Aug 9, 2024

Thanks for creating a runnable demo, appreciate that. I'll have a look at it.

@codematsing
Copy link
Author

Sure thing! I really love using your library and happy to help on improving on it!

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

No branches or pull requests

2 participants