search

plone / plone.app.contentlisting

Generic way to make listings of Plone content
2 stars 8 forks source link

In RealContentListingObject.__getattr__ check attribute existence without acquisition but return the attribute with acquisition in case it is a method that needs acquisition. #48

Closed gbastien closed 1 year ago

gbastien commented 2 years ago

Hi @mauritsvanrees @baekholt

See #47

I adapted code so __getattr__ on a contentlisting object checks attribute existence on the object without acquisition but returns it with acquisition so when it is a method, calling it will have acquisition.

This change breaks test_special_getattr_from_object, but actually I think the test is wrong. It does not defer to the brain, but actually calls absolute_url (OFS/Traversable.py(75)absolute_url()) and fails because object does not have acquisition. You can pdb in test_special_getattr_from_object without my change to see that. So I would adapt this test that will now return the absolute_url, but this changes the current behavior, so?

Thank you for review and feedback.

Gauthier

mister-roboto commented 2 years ago

@gbastien thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

gbastien commented 2 years ago

@jenkins-plone-org please run jobs

gbastien commented 2 years ago

Hi @davisagli @mauritsvanrees

I take advantage of the PloneConf sprint to work on this issue.

I removed Title monkeypatch as the absolute_url method is there and uses acquisition.

Now the test_special_getattr_from_object is broken, the assumption of the test is wrong.

What should we do?

Thank you for follow up.

Gauthier

gbastien commented 2 years ago

@jenkins-plone-org please run jobs

jensens commented 1 year ago

@jenkins-plone-org please run jobs

gbastien commented 1 year ago

Hi @jensens @davisagli @mauritsvanrees

as you can see, the test_special_getattr_from_object is broken "as expected". It was correct before because acquisition was not there and absolute_url needs acquisition.

Thank you for review ;-)

Gauthier

davisagli commented 1 year ago

@gbastien Sorry, I see that you asked what to do with the test and no one answered. Please update the test to match the changed behavior.

gbastien commented 1 year ago

@gbastien Sorry, I see that you asked what to do with the test and no one answered. Please update the test to match the changed behavior.

Right thank you I will do that!

gforcada commented 1 year ago

I can't really say how critical is this fix, but 6.0 is getting closer ™️ and if only a few iterations on the code are needed, maybe is a good time to try to finish it? 👍🏾

jensens commented 1 year ago

@jenkins-plone-org please run jobs