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

Problem in tiles with images #36

Closed lyralemos closed 4 years ago

lyralemos commented 4 years ago

Follow up for this forum post.

When you a have a NamedBlobImage as part of your tile, like this:

class IMyTile(model.Schema):
    image = NamedBlobImage(
        title = u"Image",
        required = True
    )

class MyTile(tiles.PersistentTile):
    """PersistentTile"""

While editing the tile using the default @@edit-tile view the image is lost in the process. I found the problem was here:

https://github.com/plone/plone.app.tiles/blob/495b45d5d3a9e68ba61424f628895960e0c615c6/plone/app/tiles/browser/edit.py#L84 The extractData method returns a dict like this: {'image': <NOT_CHANGED>}

Then, when applying changes to the dataManager the original value is lost, here: https://github.com/plone/plone.app.tiles/blob/495b45d5d3a9e68ba61424f628895960e0c615c6/plone/app/tiles/browser/edit.py#L97-L100

One possible solution would be to merge the current tile data with the new one, like this:

form.applyChanges(self, new_data, data)
for group in self.groups:
    form.applyChanges(group, new_data, data)

# Merge new data with the current to avoid removing images
current_data = self.getContent()
current_data.update(new_data)

dataManager.set(current_data)

Is this solution acceptable? Or can it affect some other situation?

adrianschulz commented 4 years ago

Same issue here. With which Plone version did you get this bug?

lyralemos commented 4 years ago

Plone 5.x

adrianschulz commented 4 years ago

So I think I found the change which led to this issue. With https://github.com/plone/plone.formwidget.namedfile/pull/42 you get NOT_CHANGE as value (before it was the file/image). So going back to plone.formwidget.namedfile 2.0.9 could also solve the problem.

mauritsvanrees commented 4 years ago

I confirm that going back to plone.formwidget.namedfile 2.0.9 on Plone 5.2.2 solves it for me, as workaround.

I will have a look at the plone.app.tiles PR.

mauritsvanrees commented 4 years ago

Since https://github.com/plone/plone.formwidget.namedfile/pull/42 has this unintended side effect, let me think about that one some more. cc @flipmcf so the person who wrote that merged PR is in the loop.

The value of a widget is calculated in z3c.form in field.py, which is basically two things:

  1. We have raw = widget.extract(). For the namedfile widget, when the action is 'no change', it means this code. There, self.value is the current named blob file, and it is returned.

  2. z3c.form continues and does value = interfaces.IDataConverter(widget).toFieldValue(raw). So the formwidget namedfile converter is called, which is the part changed by the formwidget PR, especially this part. Originally toFieldValue returned the given raw value, which is the current blob file. After the PR, if returns NOT_CHANGED.

So at first the extract method of plone.formwidget.namedfile realizes that there is no change, so it returns the original blob file. And then toFieldValue undoes this by returning NOT_CHANGED. This suggests a bit that the PR may not be doing the right thing. But the FileUploadDataConverter from z3c.form has similar code, detecting that no new file is uploaded and then returning NOT_CHANGED. So looks like this is working as intended.

If https://github.com/plone/plone.formwidget.namedfile/pull/42 has more unintended side effects, then maybe it needs to be reverted anyway. But for now it seems okay.

Anyway, PR #38 by @lyralemos seems okay and works for me. I would do it a bit simpler, with just a one-line change:

# new_data = {}
new_data = self.getContent()

I can do that in a new PR. But seems we first have to fix the Travis tests.... :-(

mauritsvanrees commented 4 years ago

Just for clarity, with some added print statements, this is the current data on a (simplified) tile:

[('_plone.scales',
  {'2311e168-3703-40a3-889b-675a7e582897': {'mimetype': 'image/jpeg', 'uid': '2311e168-3703-40a3-889b-675a7e582897', 'modified': 1597845460.544773, 'height': 768, 'width': 576, 'fieldname': 'image', 'key': (('fieldname', 'image'), ('height', 768), ('width', 768)), 'data': <plone.namedfile.file.NamedBlobImage object at 0x116346250 oid 0x1ee372 in <Connection at 11088e510>>}}),
 ('style', 'feature-wrapper'),
 ('image',
  <plone.namedfile.file.NamedBlobImage object at 0x116343e50 oid 0x1ee36d in <Connection at 11088e510>>),
 ]

So there are image and style fields, and some scale annotations.

The submitted form data is:

[('style', 'feature-wrapper'),
 ('image', <NOT_CHANGED>),
]

If we save only this, then the original image is gone AND the scales are gone. That is the problem we try to solve here.

And even with the original plone.formwidget.namedfile before the PR 42, the scales were gone. So the new code from #38 or my one-liner help for that.

mauritsvanrees commented 4 years ago

Fixe released in 3.2.0.