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

Can't add ManytoMany fields to a Page #231

Closed
bufke opened this issue May 5, 2014 · 25 comments
Closed

Can't add ManytoMany fields to a Page #231

bufke opened this issue May 5, 2014 · 25 comments

Comments

@bufke
Copy link
Contributor

bufke commented May 5, 2014

For example

class BlogPage(Page):
    body = RichTextField()
    categories = models.ManyToManyField(BlogCategory, blank=True)

One gets ValueError "<BlogPage: foo>" needs to have a value for field "blogpage" before this many-to-many relationship can be used.

It's caused by this
wagtail/django-modelcluster#3
https://github.com/torchbox/django-modelcluster/blob/master/modelcluster/forms.py#L260

I'll keep trying to think of a solution. Seems like a common use case as Wordpress implements categories as a M2M (you can pick multiple, but they are predefined unlike a tag).

@bufke
Copy link
Contributor Author

bufke commented May 6, 2014

I looked at it, but adding M2M support to modelcluster is beyond me. Here is a workaround for anyone with this issue.

You can create something that is database equivalent to a M2M in django by just making the "through" model yourself.

class BlogCategoryBlogPage(Orderable, models.Model):
    category = models.ForeignKey(BlogCategory, related_name="+")
    page = ParentalKey('gkblog.BlogPage', related_name='categories')
    panels = [
        FieldPanel('category'),
    ]

Add it as an inline - InlinePanel(BlogPage, 'categories', label="Categories"),

This works just like in Django admin. The UI is a bit more cumbersome using with method.

@tomdyson
Copy link
Contributor

@gasman - is this the best approach, or should we add M2M support to modelcluster?

@tomdyson
Copy link
Contributor

P.S. Thanks for raising this, @bufke!

@sixpearls
Copy link
Contributor

FWIW, I never tried making an M2M and went straight for the through model. The inline through model works great as an orderable M2M in my case, and is probably a more common use case.

When going to an "asset" like like wagtailimage.Image, it would be nice to use the ImageChooser edit_handler, but I'm not sure if that's possible at the moment. Making it possible to use those edit_handler may be a part of #245. This is actually a bigger issue for M2M an asset with a nice edit_handler (Image in this case). When using the M2M to the page, the inline edit_handler can use ImageChooser smoothly.

@gasman
Copy link
Collaborator

gasman commented Aug 27, 2014

@tomdyson Proper M2M support is definitely a desirable feature - creating the 'through' model manually is more of a workaround than a recommended approach. The lack of M2M support in django-modelcluster was also the root cause of #197 - under the hood Taggit's tags are an M2M relation, and at first glance it seemed that their behaviour was close enough to modelcluster's foreign key relations that they would Just Work... unfortunately that wasn't the case, hence the need for the workaround in #500.

@gasman gasman removed the Question label Aug 28, 2014
@mattots
Copy link

mattots commented Oct 9, 2014

I've just come up against exactly the scenario bufke raised in the initial comment - the need to provide an interface for adding categories to blog posts. The workaround using an inline is not very pretty, and tags don't quite fulfil this particular use case. Is there really no proper M2M support in Wagtail? If not, can I add a massive +1 for this to be addressed please.

@davecranwell davecranwell modified the milestone: Backlog Oct 14, 2014
@bufke
Copy link
Contributor Author

bufke commented Mar 5, 2015

I'm trying to determine if this is something I could fix. The main issue is M2M support in modelcluster. What's the scope of fixing something like that? I have experience with making django apps but it's not something I could commit weeks too either.

Another small component would be a UI widget but that seems straightforward.

@mhnbcu
Copy link

mhnbcu commented Mar 6, 2015

👍 for a fix on this.

Agree with @mattots that tags don't quite negate the need for M2M.

@gasman
Copy link
Collaborator

gasman commented Mar 6, 2015

@bufke Re the scope of this fix - unfortunately it's the sort of problem where figuring out the scope is 90% of the battle, so not really the sort of thing that I can give any pointers on. It would certainly involve digging into Django internals.

@bufke
Copy link
Contributor Author

bufke commented Mar 11, 2015

@gasman thanks I started reading through the modelcluster source. M2M would be a significant project for sure. I'm questioning the logic in supporting it actually. A M2M inherently affects multiple objects (pages perhaps). Would I ever want to edit/create M2M objects in the cluster way of keeping them in ram till saving to the database? I don't think I would.

Let's consider how wordpress does it - If I create a post, assign it to a new category, then save as a draft - the category is created and visible on other pages. That's probably fine.

Perhaps this would be best solved with a special M2M widget so the workaround doesn't look hacky. It's a pretty awkward UX right now due to using InlinePanel in an odd manner.

screenshot from 2015-03-11 14 59 21

What if we developed something like the django admin many to many widget. Maybe ManyToManyPanel. Then document how to use ManyToMany and note you must use a through table as to specify the ParentalKey. Finally I'd propose the documentation suggest making Category a snippet so the user can actually edit it.

If I can whip this idea together I'd be happy to submit upstream.

@gasman
Copy link
Collaborator

gasman commented Mar 17, 2015

@bufke I don't think we'd ever want to make the objects at the other end of an M2M relation editable as part of the page - that would cause all sorts of headaches if multiple pages had different draft edits of the same objects. The overall functionality would remain the same as it is with the workaround: being able to specify objects from an existing master list (defined elsewhere - e.g. in snippets) to be associated with this page. (We could still support adding / editing items without leaving the page editor, in the same sense that we currently do for images.)

The benefits of supporting M2M relations 'properly' would be:

  • a more suitable UI widget
  • a neater and more compact representation when serialized to JSON (e.g. when saving a page revision) - we'd store it as a list of IDs, rather than a list of through models that exist solely to hang an ID off.
  • Not having to design your database schema around Wagtail's requirements. An important design goal for Wagtail is that you should be able to design your schema in the way that best fits your data, not have it dictated by Wagtail's own implementation details.

Right now we have no ETA for getting that full solution implemented, so I'd be very happy to consider an approach that avoids the UI weirdness, as an interim solution. Thanks!

@gasman gasman removed their assignment Jun 2, 2015
@davecranwell davecranwell removed this from the Backlog milestone Jun 4, 2015
@benje
Copy link

benje commented Jun 30, 2015

thanks for the workaround @bufke

@bufke
Copy link
Contributor Author

bufke commented Oct 27, 2015

I'd be interested in starting a bounty on this issue. Maybe use bounty source? I could throw in $1000 for a good fix that got proper M2M support in modelcluster and a widget for wagtail.

@gasman
Copy link
Collaborator

gasman commented Oct 28, 2015

@bufke There's a PR in progress at wagtail/django-modelcluster#36 - if there were to be a bounty then it would be fair for it go to @theju (assuming it works as advertised, of course). Are retrospective bounties a thing...? :-)

It's currently waiting on Django 1.7 support, but at this point I reckon Django 1.7 is far enough in the past that we could reasonably ship this as a 1.8-only feature (provided that it doesn't break any existing 1.7 functionality). In that case, the only thing blocking this is me finding the time to review it all, since it's quite a complex update - I'll make this a priority for Wagtail 1.3.

@ababic
Copy link
Contributor

ababic commented Feb 17, 2016

Hi guys. I know it's difficult to know exactly when this will be sorted, due to the modelcluster dependency, but I wonder if it would be worth some work in the meantime, to try and at least solve some of the editing limitations that the lack of ManyToManyField presents?

For example, where you have a very simple Category model, and have already created a 'through' model to associate your page with categories, would it be super difficult to offer an alternative to InlinePanel, that would provide a checkbox list for you to select from? Or perhaps something more complicated (django's filter_horizontal widget) if there was a need for it.

Maybe it wouldn't be worth the work if 1.4 is coming soon?

@gasman
Copy link
Collaborator

gasman commented Feb 17, 2016

Hi @ababic,

A nice form field for M2M relations is something that we're going to want either way, and up to now I'd anticipated that it would be the second stage of development, after the modelcluster updates. However, if you think it's something that could usefully be done now, using 'through' models as an interim solution, then that would certainly be welcome.

On the other hand, it may be that with modelcluster's M2M support in place, the UI work simply reduces to "use ModelMultipleChoiceField"... in which case it probably is better to wait for modelcluster. That task is gradually rising to the top of my todo list, and will hopefully get done in the next two weeks or so.

@davejacobs
Copy link

That's great to hear! Can't wait for this.

@LiamBrenner
Copy link
Contributor

👍

@kofic
Copy link

kofic commented Jan 10, 2017

@gasman Is this available now?

@gasman
Copy link
Collaborator

gasman commented Jan 10, 2017

@kofic No (but I'm working on it) - there'll be an update here when there's any news.

@gasman gasman modified the milestones: 1.9, real-soon-now Jan 12, 2017
@gasman
Copy link
Collaborator

gasman commented Jan 12, 2017

This is now implemented in django-modelcluster: wagtail/django-modelcluster#62

From initial tests, it seems to be working nicely with Wagtail: just use ParentalManyToManyField instead of ManyToManyField, and specify it in a FieldPanel as normal.

Things that need to be done before the release of Wagtail 1.9:

  • release django-modelcluster 3.0 final and update wagtail's setup.py to require django-modelcluster>=3.0,<4.0
  • add a release note to document the updated django-modelcluster / django-taggit dependencies (since it's somewhat likely that people will have the old versions hard-coded in their requirements files)
  • ensure that M2M relations are covered by tests
  • document the use of ParentalManyToManyField, possibly through a new 'categories' section in the tutorial (which was dropped from Update Getting Started tutorial to include posts automatically, plus … #3125 (comment) because it was too messy to implement without real M2M support) as well as http://docs.wagtail.io/en/v1.8/topics/pages.html

@nickhudkins
Copy link

nickhudkins commented Jan 19, 2017

Before opening another ticket, which I believe would be a duplicate of: #2635, does the work being done here apply only to ParentalManyToManyField or will we be able to utilize this in contrib.modeladmin as well, for standard M2Ms and ForeignKeys. I understand that I could also just roll my own here, the use case being, a ForeignKey to a model with 100k rows destroys a select box. In the standard django-admin I have always used: https://github.com/lincolnloop/django-salmonella to get around this sort of problem.

Nevermind 👍 this is a non-wagtail specific issue. I'll go sleep now.

Thank you for all your work!

@gasman
Copy link
Collaborator

gasman commented Jan 19, 2017

Hi @nickhudkins - in retrospect I think I probably misinterpreted #2635. Since the only way to achieve M2M-like relations on pages was with 'through' models in an InlinePanel (it hadn't occurred to me that ManyToManyField does work on snippet / modeladmin models), I took it to be a request for a "better widget" than the through-model / InlinePanel setup.

Either way, the resolution is the same - now that we consistently support 'proper' M2Ms everywhere, the process for swapping in an alternative widget via FieldPanel is the same as for any other field type - and, as you observe, coming up with a better widget isn't a Wagtail-specific issue.

@kofic
Copy link

kofic commented Jan 30, 2017

This is awesome @gasman I will test it out

@gasman
Copy link
Collaborator

gasman commented Feb 3, 2017

Completed in #3305.

@gasman gasman closed this as completed Feb 3, 2017
@gasman gasman moved this from Planned for 1.9 to In 1.9 in Roadmap Feb 15, 2017
@gasman gasman removed this from In 1.9 in Roadmap May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests