Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make support of "ftw.subsite" more failsafe #29

Merged
merged 1 commit into from Nov 5, 2019

Conversation

@busykoala
Copy link
Contributor

busykoala commented Oct 24, 2019

In ftw/addressblock/contact/browser/contact.py the interface ISubsite is used. Therefore we should have ftw.subsite not only as a test requirement but also as a 'normal' requirement.

@busykoala busykoala requested a review from mbaechtold Oct 24, 2019
@busykoala busykoala self-assigned this Oct 24, 2019
@mbaechtold

This comment has been minimized.

Copy link
Member

mbaechtold commented Oct 24, 2019

Thank you for your contribution 😃

As I'm thinking this through, I tend to believe that it would probably better to make the code using ftw.subsite more flexible (by checking if ftw.subsite is available) instead of adding ftw.subsite as a direct dependency of ftw.addressblock.

Or at least make ftw.subsite only a dependency of the extra contact.

@maethu, what do you think about that?

@jone

This comment has been minimized.

Copy link
Member

jone commented Oct 25, 2019

Yes, ftw.addressblock should not depend on ftw.subsite.

@busykoala busykoala force-pushed the mo/fix_required branch 2 times, most recently from fddf7cf to 3d4ea6e Oct 25, 2019
@busykoala

This comment has been minimized.

Copy link
Contributor Author

busykoala commented Oct 25, 2019

@mbaechtold, the change now checks if the package is installed.

@busykoala

This comment has been minimized.

Copy link
Contributor Author

busykoala commented Nov 5, 2019

@mbaechtold ping ;-)

ftw/addressblock/contact/browser/contact.py Outdated Show resolved Hide resolved
@busykoala busykoala requested a review from jone Nov 5, 2019
Copy link
Member

mbaechtold left a comment

The implementation looks ok 👍

Could you please change the title of the pull request to something like Make support of "ftw.subsite" more failsafe.

Please also squash the two commits.

@busykoala busykoala changed the title Fix: add missing requirement. Make support of "ftw.subsite" more failsafe Nov 5, 2019
Check if ftw.subsite is installed before using its interface.
@busykoala busykoala force-pushed the mo/fix_required branch from 2e776c0 to 436fe28 Nov 5, 2019
@busykoala busykoala requested a review from mbaechtold Nov 5, 2019
@busykoala busykoala merged commit eb76ee8 into master Nov 5, 2019
2 checks passed
2 checks passed
CI Governor: test-plone-4.3.x.cfg Task #464059 succeeded
Details
CI Governor: test-plone-5.1.x.cfg Task #464060 succeeded
Details
@busykoala busykoala deleted the mo/fix_required branch Nov 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.