search

plone / plone.app.tiles

Plone UI integration for plone.tiles
http://pypi.python.org/pypi/plone.app.tiles
Other
6 stars 7 forks source link

RelationField's don't seem to be persisted properly #6

Closed danmur closed 8 years ago

danmur commented 8 years ago

I came across this creating a custom widget for selecting relations (for RelationChoice fields) but it seems to happen using the default widget too. The symptom is the same as this SO post: http://stackoverflow.com/questions/27278300/relationlist-wrong-field-value-in-tile-schema

The objects stored in the tiles data are not RelationValues, but content objects, and become broken after a server restart.

That post gave some good hints, as far as I can tell the form handling for tiles is all done in plone.app.tiles rather than in z3c.form's applyForm(), so the correct IDataManager is never used.

Anyway, long story short, I've added this code to what seems like a logical place and it works for my widget and for the default one also. Branch is here:

https://github.com/hatchddigital/plone.app.tiles/commits/relationfield-handling

If this seems like a reasonable change let me know and I'll create a pull request, or if anyone has any hints or pointers as to a better fix I would appreciate that :).

datakurre commented 8 years ago

Thanks for the report. Plone.app.tiles uses z3c.form properly, so this was surprising. It's probably minor fix, but we must add a test to break it again. Probably we should have used UUIDs instead of relation values, but now that would require a slow migration step. I recall that even with proper data manager all relation catalog magic did not work properly.

On 28. huhtikuuta 2016 klo 7.05 +0300, Daniel Murraynotifications@github.com, wrote:

I came across this creating a custom widget for selecting relations (for RelationChoice fields) but it seems to happen using the default widget too. The symptom is the same as this SO post: http://stackoverflow.com/questions/27278300/relationlist-wrong-field-value-in-tile-schema

The objects stored in the tiles data are not RelationValues, but content objects, and become broken after a server restart.

That post gave some good hints, as far as I can tell the form handling for tiles is all done in plone.app.tiles rather than in z3c.form's applyForm(), so the correct IDataManager is never used.

Anyway, long story short, I've added this code to what seems like a logical place and it works for my widget and for the default one also. Branch is here:

https://github.com/hatchddigital/plone.app.tiles/commits/relationfield-handling

If this seems like a reasonable change let me know and I'll create a pull request, or if anyone has any hints or pointers as to a better fix I would appreciate that :).

— You are receiving this because you are subscribed to this thread. Reply to this email directly orview it on GitHub(https://github.com/plone/plone.app.tiles/issues/6)

krissik commented 8 years ago

I have the same problem but I am using the default widget. I have worked around it by implementing a subclass of PersistentTileDataManager which converts Objects to RelationValues. But I would be happy to have a better solution. I have tested danmurs fix but the widget does not show selected items again.

@datakurre: Can you explain in more detail what`s the problem with relation catalog magic is or point me somewhere where it is explained? Thanks!

datakurre commented 8 years ago

@KrissiK zc.relation is the package providing the zc relations catalog and all API for the "new" relations.

I recall, that because relationvalues in tiles cannot be traversed (they are inside annotation, in a persistent list), some relations catalog API does not work properly. Yet, the simplest use-case of storing and reading references should work. But you should always take account that relation may become broken when its target gets removed.

datakurre commented 8 years ago

Anyway, this is definitely a bug, and this was supposed to be fixed a long time ago with a special data manager (as mentioned in stackoverflow): https://github.com/plone/plone.app.relationfield/commit/cd18d5cb2a4dceb37135a441ad6e820ae954cf81

danmur commented 8 years ago

@KrissiK I'll check that and see what's going on. I didn't too too much testing of this (e.g. only tried a RelationChoice, not a list), wanted to check first I was on the right track. @datakurre I'll add tests and clean up.

datakurre commented 8 years ago

Thanks! Adding proper ZCA registrations for dict and dict list datamanagers from p.a.relationfield should be enough. Yet, still puzzled why the existing registrations didn't work.

On 29. huhtikuuta 2016 klo 3.08 +0300, Daniel Murraynotifications@github.com, wrote:

@KrissiK(https://github.com/KrissiK)I'll check that and see what's going on. I didn't too too much testing of this (e.g. only tried a RelationChoice, not a list), wanted to check first I was on the right track.@datakurre(https://github.com/datakurre)I'll add tests and clean up.

— You are receiving this because you were mentioned. Reply to this email directly orview it on GitHub(https://github.com/plone/plone.app.tiles/issues/6#issuecomment-215598350)

datakurre commented 8 years ago

@KrissiK, could you share the ZCML registration you used for your manager?

On 29. huhtikuuta 2016 klo 3.08 +0300, Daniel Murraynotifications@github.com, wrote:

@KrissiK(https://github.com/KrissiK)I'll check that and see what's going on. I didn't too too much testing of this (e.g. only tried a RelationChoice, not a list), wanted to check first I was on the right track.@datakurre(https://github.com/datakurre)I'll add tests and clean up.

— You are receiving this because you were mentioned. Reply to this email directly orview it on GitHub(https://github.com/plone/plone.app.tiles/issues/6#issuecomment-215598350)

danmur commented 8 years ago

The registrations that are there are correct as far as I can tell (I didn't have to change any). It's just that the forms for the tiles don't seem to go through any code path that uses z3c.form's IDataManager; regular forms go through here: https://github.com/zopefoundation/z3c.form/blob/master/src/z3c/form/form.py#L35

and this was the only place I could find that did this processing. I'm not very familiar with the internals of z3c.form but perhaps this applyForm() could be called in p.a.tiles so that the logic need not be duplicated. Will have a look this weekend :)

datakurre commented 8 years ago

@danmur You are correct. I'm sorry that I did not believe you at first on that. I was so sure that I had used plone.app.tiles' forms with relations before.

If you have change to look into this, please do.

Probably, both add and edit form needs a fix:

Maybe simply replacing

    dataManager.set(data)

with

    dataManager.set(applyForm(self, data, data))

or

    content = {}
    dataManager.set(applyForm(self, content, data))
    dataManager.data(content)

would do the trick.

krissik commented 8 years ago

@damur Indeed - I have tested with a RelationList @datakurre This is in my configure.zcml <adapter factory=".tiles.MyPersistentTileDataManager" />

It would be great if using Relations in tiles work out of the box - thanks for your work!

danmur commented 8 years ago

Hey just fyi I haven't forgotten about this, we just ran into a bunch of other problems using RelationFields with data grids :(. Anyhow, I'll have a pull request tomorrow with the fix for this specific issue along with tests.

datakurre commented 8 years ago

This should now have been fixed by https://github.com/plone/plone.app.tiles/pull/10