Skip to content

Commit

Permalink
Solving a bug on cooking - request object is not modified
Browse files Browse the repository at this point in the history
  • Loading branch information
@bloodbare
bloodbare committed Oct 28, 2014
1 parent ccfcc5e commit ffb3c70
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 4 deletions.
9 changes: 6 additions & 3 deletions Products/CMFPlone/resources/browser/cook.py
View file
Expand Up @@ -199,8 +199,10 @@ def cookWhenChangingSettings(context, bundle):
# Let's join all css and js
css_file = ""
js_file = ""
# siteUrl = getSite().absolute_url()
siteUrl = getSite().absolute_url()

request = getRequest()
original_request = request.clone()
for package in bundle.resources:
if package in resources:
resource = resources[package]
Expand Down Expand Up @@ -289,5 +291,6 @@ def cookWhenChangingSettings(context, bundle):
folder.writeFile(resource_filepath, fi)
bundle.last_compilation = datetime.now()

# import transaction
# transaction.commit()
setRequest(original_request)
import transaction
transaction.commit()

This comment has been minimized.

Sign in to view

Copy link
@davisagli

davisagli Nov 2, 2014

Member

This transaction commit is the cause of the current test failures in plone.app.linkintegrity. /cc @tisto

What is supposed to happen in this test is:
a. The test tries to delete an item.
b. p.a.linkintegrity catches the delete event, and if there are integrity errors it raises LinkIntegrityNotificationException.
b. An special exception view for LinkIntegrityNotificationException renders the confirmation page.
c. Since an exception was raised, the publisher aborts the transaction and the object does not actually get deleted.

However, because this is the first page rendered by the browser, rendering the exception view triggers bundle compilation, and so this line commits the transaction with the deleted items!

Can we remove this transaction commit? Why is it needed?
2 changes: 1 addition & 1 deletion Products/CMFPlone/static/plone-compiled.css
View file

Large diffs are not rendered by default.

4 comments on commit ffb3c70

@mister-roboto
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TESTS FAILED
Mr.roboto url : http://jenkins.plone.org/roboto/get_info?push=757740070bd44228bfe24f5a410b4069
plone-5.0-python-2.7 [FAILURE]

@bloodbare
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that may be the cause, I saw that linkintegrity needs a transaction abort to avoid deleting it, just I thought it was doing the transaction abort before cooking is done.

The reason of that commit is: if the last compilation time is earlier than last installation time we cook the resources once (when is production so its possible there is some addon that has added staff). The problem resides that last_compilation time is not stored on the registry until it finish the transaction and that leads to compiles the legacy js/css for each bundles (which is the same). I can try to refactor it so it does not happen.

@tisto
Copy link
Member

@tisto tisto commented on ffb3c70 Nov 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bloodbare When I remove the transaction commit, the test isolation problems in p.a.event are gone (though, there are still two other test failures). I think we should try to remove that transaction commit if possible.

@thet I investigated the problem further and @davisagli put me on the right track. test_event_view.py in p.a.event uses the integration testing layer and calls 'restrictedTraverse', which seem to trigger the 'transaction commit' from @bloodbare's commit above.

Here is my (preliminary) p.a.event fix:

plone/plone.app.event@97206eb

I'm not sure how this is supposed to work. If we keep the 'transaction commit' in the resource registy, we have to make sure that every test that uses restrictedTraverse also uses the functional test layer. Not sure if this is what we want. It is strange though that this only occurs in p.a.event/p.a.contenttypes.

@davisagli
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not restrictedTraverse that does it, it's actually calling the view (since that ends up including the resource registry viewlets)

Please sign in to comment.