search

plone / plone.app.content

Various views for Plone, such as folder_contents, as well as general content infrastructure, such as base classes and name choosers.
https://pypi.org/project/plone.app.content/
7 stars 32 forks source link

Pressing "Cancel" on delete_confirmation on the context of an image is broken #38

Closed ale-rt closed 8 years ago

ale-rt commented 9 years ago

The handle_cancel method:

image

I think we should change this to the default view.

vangheem commented 9 years ago

absolute_url should never return an image stream. Something else is happening.

vangheem commented 9 years ago

What version of plone 5 are you testing on? I can not reproduce on master.

ale-rt commented 9 years ago

I am using Plone-5.0b2.

absolute_url should never return an image stream. Something else is happening.

I see this:

Maybe some addon is changing the Image base view.

If I go here:

for the Image type I have @@display-file, which I think it is correct.

Will check this on master anyway.

ale-rt commented 9 years ago

I tried with buildout coredev and effectively I am not able to reproduce it anymore.

Perhaps someone introduced some javascript magic that fixed the popup.

Anyway I think this is still an issue.

If you go here:

Will try to provide a patch as soon as possible.

ale-rt commented 9 years ago

The situation is weird :)

With this patch:

diff --git a/plone/app/content/browser/actions.py b/plone/app/content/browser/actions.py
index cb95120..4a1e99d 100644
--- a/plone/app/content/browser/actions.py
+++ b/plone/app/content/browser/actions.py
@@ -40,6 +40,15 @@ class DeleteConfirmationForm(form.Form, LockingBase):
     template = ViewPageTemplateFile('templates/delete_confirmation.pt')
     enableCSRFProtection = True

+    def view_url(self):
+        ''' Facade to the homonymous plone_context_state method
+        '''
+        context_state = getMultiAdapter(
+            (self.context, self.request),
+            name='plone_context_state'
+        )
+        return context_state.view_url()
+
     @property
     def items_to_delete(self):
         catalog = getToolByName(self.context, 'portal_catalog')
@@ -72,7 +81,7 @@ class DeleteConfirmationForm(form.Form, LockingBase):
     @button.buttonAndHandler(
         _(u'label_cancel', default=u'Cancel'), name='Cancel')
     def handle_cancel(self, action):
-        self.request.response.redirect(self.context.absolute_url())
+        self.request.response.redirect(self.view_url())

 def valid_id(self):

the flow visting this url (the javascript-less behaviour):

is fixed.

But the popup breaks in an awkward way...

What happens is that the form (posted with an ajax call), get's the view (instead of the image) and then it goes to the image view...

image

vangheem commented 9 years ago

Yes, the modal should just be changed to close the modal if cancel is clicked.

vangheem commented 9 years ago

Wait, re-reading your comment, don't you want the image view? That's the correct behavior.

ale-rt commented 9 years ago

Thanks @vangheem for taking care :)

Wait, re-reading your comment, don't you want the image view? That's the correct behavior.

I do want the delete_confirmation to return a redirect to the view_url (e.g. http://localhost:8080/Plone/a.jpg/view) and not the absolute_url (e.g. http://localhost:8080/Plone/a.jpg). IMHO this should be the correct behaviour of that form.

With my patch it is working perfectly if you traverse to the form, e.g. going to http://localhost:8080/Plone/a.jpg/delete_confirmation and clicking cancel redirects you to http://localhost:8080/Plone/a.jpg/view.

But there is a problem when you use that view with pat-modal. This call:

window.parent.location.href = options.redirectToUrl.apply(self, [$action, response, options]);

sets the next target to the data-base-url read in the html, see:

Which for an image turns out to be the raw image.

Of course this method returns an empty baseUrl if the delete_confirmation returns the image (actual behaviour of master).

Note: as a side effect of this behaviour, images can weight several MBs...

vangheem commented 9 years ago

Ahh, that's your patch. Can you create a PR?

ale-rt commented 9 years ago

Yes, but I will add a check: if the request is an ajax one I will return a small string (empty? a json?), if it is not I will return the view_url.

ale-rt commented 9 years ago

Maybe it is also possible to make some modifications here:

but looking to pat-plone-modal options it does not seem possible to fix this behaviour.

vangheem commented 9 years ago

Mixing plain html forms with ajax modal forms is really hard and should be avoided if possible.

ale-rt commented 9 years ago

I agree, I wanted to avoid this. Do you see any other way to solve this issue?

Submitted the PR, I ran succesfully the tests on this package, but still waiting for ./bin/alltest

vangheem commented 9 years ago

Don't worry about runing ./bin/alltest locally. Just run the jenkins job for the PR: http://jenkins.plone.org/job/pull-request-5.0/118/

ale-rt commented 9 years ago

Thanks a lot for your support. Anyway I would think to a better solution :/ The issue is wide spread, for example the object_rename form is affected by the same problem.

My main concern is that we are making an ajax request that potentially downloads a high quality image.

I think that fixing this has implications on plone-pat-modal.

vangheem commented 9 years ago

Fixing this correctly could be:

1) re-implement forms with javascript instead of trying to make non-ajax forms work like this 2) try to make plone-pat-modal handle MORE cases and do this better. It's already insane what it tries to do because this problem is hard... 3) disable modals

If you can find a decent solution, that'd be great. I think plone 5.1 needs a plip to replace pat-plone-modal with a new implementation that is only meant to work with ajax and then we actually implement all the forms we want correctly.

pbauer commented 9 years ago

The issue is still there for files and images (in coredev). Steps to repoduce:

ale-rt commented 9 years ago

This is really bad!

There is an issue with pat-modal redirects. I am trying to fix that issue with this pull:

Anyway there is a more general issue (#42), After we find a valid solution for the delete action (and a valid solution, IMHO, means that if you click cancel the modal just disappears with no call to the server) we should apply it also to the other actions (e.g. rename).

vangheem commented 9 years ago

I think the real fix here is to use the "disableAjaxFormSubmit":true option on the modal options in portal_actions config

vangheem commented 9 years ago

Also see: https://github.com/plone/mockup/commit/413a1bdc71feaf1c1231d30fa3187fbcec865865

ale-rt commented 9 years ago

On Tue, Aug 11, 2015 at 3:04 PM, Nathan Van Gheem notifications@github.com wrote:

Also see: plone/mockup@413a1bd https://github.com/plone/mockup/commit/413a1bdc71feaf1c1231d30fa3187fbcec865865

I was playing with option in the ZMI, but I was never able to have a proper behavior for both the "Delete" and "Cancel" buttons. Maybe you have more experience than me and with five minutes of ZMI can fix

this.

Alessandro Pisa - RedTurtle Technology E-mail: alessandro.pisa@redturtle.it Web Site: http://www.redturtle.it Phone: +39 0532 1915958

vangheem commented 9 years ago

I screwed up and didn't mean to put this on master but this is the correct modal option fix: https://github.com/plone/Products.CMFPlone/commit/e57a314dba4b662cebda8f4528cacb25c1d7f721

formsUseAjax is not even a valid option.

Could you test with development mode active on the bundles? If we're good, I can build and commit the production resources.

ale-rt commented 9 years ago

Sure

ale-rt commented 9 years ago

I think e57a314 should be reverted: it fixes the cancel action, but not the delete one.

In http://localhost:8080/Plone/portal_actions/object_buttons/delete/manage_propertiesForm I see: image

With this setting if I click cancel it redirects me to http://localhost:8080/Plone/plone.png/view which is no longer there.

As said in my previous comment I was playing with several options in the ZMI but was never able to have a proper behavior for both actions.

Anyway if we revert e57a314 we are anyway quite close to the solution after:

it should be enough to rebuild the bundles, am I wrong?

vangheem commented 9 years ago

huh, it's working properly for me.

With this setting if I click cancel it redirects me to http://localhost:8080/Plone/plone.png/view which is no longer there.

Are you saying that it deletes it when you click cancel?

ale-rt commented 9 years ago

If I click cancel it deletes the image and the redirects to the image view, but of course I get a 404. I made the test with a fresh checkout and a brand new plone site.

vangheem commented 9 years ago

And with development mode enabled for the bundles?

vangheem commented 9 years ago

This is my behavior: https://www.dropbox.com/s/6thcbf68x6s1rja/toolbar-example.mov?dl=0

ale-rt commented 9 years ago

This is my:

vangheem commented 9 years ago

And you're in development mode for the bundles "plone" and "plone-logged-in"?

ale-rt commented 9 years ago

yes :) image

ale-rt commented 9 years ago

Thiis is what is happening: image

After getting a response from delete confirmation we want to go to the root of the Plone site, but later we are submitted to the view of the image (which has been deleted)

vangheem commented 9 years ago

I understand. I can't reproduce what you are seeing though.

ale-rt commented 9 years ago

Ok, I will try to refresh my development environment. Anyway thanks for your kind help!

ale-rt commented 8 years ago

Cannot reproduce this issue with a recent checkout