jrief / django-formset

The missing widgets and form manipulation library for Django
https://django-formset.fly.dev/
MIT License
317 stars 30 forks source link

Editing model with many-to-one relationship #110

Open agseaton opened 7 months ago

agseaton commented 7 months ago

The documentation describes how to set up a form collection for creating a new entry with a one-to-many relationship (https://django-formset.fly.dev/model-collections/#one-to-many-relations).

However, it does not describe how to use a FormCollection to edit an existing entry. Would you be able to provide an example of how to do this (ideally similar to https://django-formset.fly.dev/model-form/#complete-crud-view)?

jrief commented 7 months ago

I'm unsure if I understand your question, but forms can be created from models as described here: https://django-formset.fly.dev/model-collections/#one-to-one-relations this also applies to one-to-many-relations

The important method to remember is model_to_dict which recursively traverses the the tree of related models and builds a dictionary then used to initialize the collection of forms.

agseaton commented 7 months ago

Let me give a concrete example. Let's say we have the following pair of models as per the documentation:

class Company(models.Model):
    name = models.CharField(verbose_name="Company name", max_length=50)

class Department(models.Model):
    name = models.CharField(verbose_name="Department name", max_length=50)
    company = models.ForeignKey(Company, on_delete=models.CASCADE)

    class Meta:
        unique_together = ['name', 'company']

The documentation describes how to write a form that will allow the user to create a company and a series of new linked departments.

What I'd like to do instead is to create a form that will not only do this, but that can also be used to edit existing objects. In other words, similar to the CRUD example but for FormCollections. I understand that I need to implement model_to_dict functions, but it isn't immediately obvious to me how to do this, and from what I can see this isn't all that is required.

In particular, how should I represent the child objects? A list of dicts, or a dict containing a list? What should the dict keys be? Which Form/FormCollection classes need to implement these methods? It seems to me that the FormCollectionView needs modifying too, so if that's true, what changes need to be made there?

If you'd be able to provide a complete working example of this, it would help clear up some of these ambiguities. I've been trying to figure this out for some time by referring to the various examples you've provided and by trial and error, but I'm just getting more and more confused!

jrief commented 7 months ago

I believe the best idea is that you fork this project and add one of the existing models/collections to your implementation. Then I'll have a look at how that problem can be solved. Btw., I'm always interested in exotic use-cases, so yours seems to play in that league.

It might however take some time, because I'm currently working on other stuff.

agseaton commented 7 months ago

Sure, it might take me a little while to find some time to do this, but I'm planning to give it a try. I'll post back here once I have something I can share.

lsmoker commented 6 months ago

I would also like to know how to update/view existing parent/child data using form collections - not just add new objects (database table rows).

jrief commented 6 months ago

Did you try the methods construct_instance and model_to_dict. Here is a working demo: https://django-formset.fly.dev/bootstrap/company

the code to run that demo is provided in testapp.

lsmoker commented 6 months ago

Yes, that's it. My attempts with my own models haven't worked out, but I'll keep at it.

lsmoker commented 6 months ago

I see that the FormCollection classes in company.py only have retrieve_instance methods not construct_instance and model_to_dict. I have tried overriding all 3 of those methods in my child FormCollection but they are not called when the page loads...

jrief commented 6 months ago

did you use the formset.views.EditCollectionView?

lsmoker commented 6 months ago

I did. I think the problem I'm having relates to my child model's ForeignKey field - I did not use the related_name parameter which means the reverse relation becomes something like book_set rather than explicitly named as books which your examples do. Monday I'll do more work on it.

jrief commented 6 months ago

You may send me the link to your (public) repository and I will try to reproduce it.

lsmoker commented 6 months ago

I did learn that I need to set the related_name on the child model for that form collection to load existing rows (as I mentioned previously). Here is the repo that can demonstrate that -> https://github.com/lsmoker/django-formset

In testapp/models/bundle.py, I have a line commented out that doesn't have related_name specified for the bundle field. In testapp/forms/bundle.py, I have a line commented out that uses opportunity_set (the default manager name if related_name is not used). I was wrongly assuming(!) opportunity_set would work. If you switch these 2 lines, you can see that the Opportunity rows will not be loaded.

Maybe the documentation could make it clear that related_name needs to be specified in this situation and even explain how the "magic" of it works...

SamuelJennings commented 3 months ago

I encountered this issue as well while trying to create a form collection for a one-to-many relationship. Here's what I discovered. Everything works fine ONLY if the name of the declared holder is equivalent to the reverse accessor of the related field on the parent model. For example

# models.py
class Department(models.Model):
    company = models.ForeignKey(Company, on_delete=models.CASCADE)

# forms.py
class CompanyCollection(FormCollection):
    # this doesn't work!
    # departments = DepartmentCollection()

    # this works!
    department_set = DepartmentCollection()

and also,

# models.py
class Department(models.Model):
    company = models.ForeignKey(Company, on_delete=models.CASCADE, related_name="departments")

# forms.py
class CompanyCollection(FormCollection):
    # this works now!
    departments = DepartmentCollection()

Why?

At the moment, the default implementation of model_to_dict tries to fetch the related manager and queryset like this:

# formset/collection.py 

class BaseFormCollection(HolderMixin, RenderableMixin):

    def model_to_dict(...):
        ...
        if related_manager := getattr(instance, holder._name, None):
            ...

holder._name is automatically set based on the declared attribute on the parent holder. Ergo, the attribute name of the related form collection MUST be the same as the reverse accessor on the instance.

So...

I don't think the current implementation is necessarily a bad thing but it probably needs to be documented. Perhaps something could be changed to allow a little more flexibility with naming and declaring child form collections but I would leave that to you.

jrief commented 3 months ago

Thanks @SamuelJennings for pointing this out. I'll add this to the documentation. If you want, you can create a pull request and write it yourself.

SamuelJennings commented 3 months ago

Sure thing, I’d be happy to. I’ll try get it done by the end of the week.

jrief commented 2 months ago

@SamuelJennings have you been able to document this setting?

SamuelJennings commented 2 months ago

I wrote it but then ran into some Git related issues. I'll sort it out tomorrow.