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

Do not start QueueProcesserThread on config update. #18

Merged
merged 4 commits into from May 28, 2019

Conversation

jugmac00
Copy link
Member

@jugmac00 jugmac00 commented May 28, 2019

... as this is unexpected.

I'd be very happy to also cover the if REQUEST idiom, if somebody could point me to

  • how to make a simple REQUEST object for testing (which I probably could figure out myself)
  • and especially how to satisfy the security restrictions, see below

With a super simple dummy request object(request = {}) I get this test error, which I do not know how to handle.

AccessControl.unauthorized.Unauthorized: You are not allowed to access 'manage_page_header' in this context

This is the first pull request (for #14) - once accepted, I will make another one for the 2.13 branch, as I need a backport for my Zope app, which currently still runs on 2.13.

... as "domain.com" is a real domain, and "example.com" should be used
for both documenation and testing purposes.

modified:   src/Products/MailHost/tests/testMailHost.py
modified:   src/Products/MailHost/tests/testMailHost.py
... as this is unexpected.

modified:   src/Products/MailHost/MailHost.py
modified:   src/Products/MailHost/tests/testMailHost.py
modified:   src/Products/MailHost/tests/testMailHost.py
@jugmac00 jugmac00 requested review from dataflake and icemac May 28, 2019 14:00
@d-maurer
Copy link
Contributor

d-maurer commented May 28, 2019 via email

@dataflake
Copy link
Member

I wish you would not mix a completely unrelated cleanup of the email addresses used in the tests into this PR. Now you have a huge diff you're asking people to look at where very little actually concerns the issue in question. Please do not do that in the future. It makes reviewing unneccessarily hard. Thank you.

@dataflake
Copy link
Member

It is somewhat pointless to test the ZMI redirect at the end. That code has not changed except for moving a single line around. Furthermore, the redirect is completely unrelated to the actual issue you're fixing here. I'm going to merge this as-in.

@dataflake dataflake merged commit 690746b into master May 28, 2019
@dataflake dataflake deleted the fix-make-changes branch May 28, 2019 23:28
@icemac
Copy link
Member

icemac commented May 29, 2019

An additional entry in the change log would be nice.

@jugmac00
Copy link
Member Author

@d-maurer Thanks a lot for pointing me to Testing.ZopeTestCase.ZopeTestCase which I did not know until now!

@dataflake Thanks for merging!

When I work on code, I usually look for what Martin Fowler calls opportunistic refactoring or Robert Martin calls the boyscout rule = "always leave the code behind in a better state than you found it".

I am well aware that reviews and therefore commits should be easy scannable.

By the way... Did you notice that I put the domain name change into a separate commit?
c17e511
Although there are many changes, Github offers even inline highlighting, so the review of the domain name change should not take more than one minute.

Anyway, if you prefer smaller pull requests, I will do so.

@jugmac00
Copy link
Member Author

@icemac Thanks for the hint! Please see #19

@dataflake
Copy link
Member

I do not look at separate commit. I look at the final change set.

You're welcome to use whatever technique you want, but if you end up with a large change set you're making life hard for the reviewer. We're all volunteers here who spend their spare time.

You can always create separate PRs for unrelated changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants