[WIP] Move PasswordResetTool to CMFPlone #1799
Conversation
|
I know that some addons are (ab)using PasswordResetTool for other similar tasks. But I can not find code importing from there, just using the tool instance. So this should be not a problem to merge once finished. |
tomgross
force-pushed
the
tomgross-mergepwresettool
branch
from
afe8759
to
72aa556
Compare
| return self.manage_overview(manage_tabs_message="Returning username check turned %s" % m) | ||
|
|
||
| def __init__(self): | ||
| self._requests = {} |
There was a problem hiding this comment.
While we're moving the password tool, let's change this to a BTree. We had a customer with a site that had self-registration turned on and was getting spam accounts created, and the database grew rapidly because each new user saved a new copy of this entire dict.
There was a problem hiding this comment.
Sounds like a good idea. I'll look into this
| mail_text = self.registered_notify_template( | ||
| self, self.REQUEST, member=member, reset=reset, email=email, | ||
| charset=encoding) | ||
| registered_notify_template = getMultiAdapter( |
There was a problem hiding this comment.
This sort of lookup would be more readable if we had a "getView" utility function in CMFPlone.
There was a problem hiding this comment.
On the first glance this sounds tempting but getting the multi-adapter is well documented and changing it would lead to different patterns all over the place.
There was a problem hiding this comment.
Yeah, I realize the suggestion is outside the scope of this pull request. But I think it's something we should consider changing in all those places. :)
| @@ -31,7 +31,7 @@ | |||
| <required tool_id="portal_migration" | |||
| class="Products.CMFPlone.MigrationTool.MigrationTool"/> | |||
| <required tool_id="portal_password_reset" | |||
| class="Products.PasswordResetTool.PasswordResetTool.PasswordResetTool"/> | |||
| class="Products.CMFPlone.PasswordResetTool.PasswordResetTool"/> | |||
There was a problem hiding this comment.
Remember this will require an upgrade step in plone.app.upgrade.
There was a problem hiding this comment.
OK. I guess the goal should be to get rid of the tool all together, since the methods can go to the view classes and the storage to the Plone site directly.
| @@ -10,7 +10,8 @@ | |||
| from Testing.ZopeTestCase import ZopeDocFileSuite | |||
|
|
|||
|
|
|||
| UNITTESTS = ['messages.txt', 'mails.txt', 'emaillogin.txt', 'translate.txt'] | |||
| UNITTESTS = ['messages.txt', 'mails.txt', 'emaillogin.txt', 'translate.txt', | |||
| 'pwreset_browser.txt'] | |||
There was a problem hiding this comment.
Are you up for converting the doctest to unit tests?
There was a problem hiding this comment.
No. But this way I make sure the tests in pwreset_browser.txt are not run twice. They are already called by test_passwordreset with a custom testsetup. The name UNITTESTS is misleading. It really is IGNORE_THIS_DOCS_FROM_TESTS
Turn the doctests in unit tests. It makes it far more easy to write and maintain them.
Reset and re-enable ``define`` and ``require`` for the ``plone-legacy`` bundle in development mode. Fixes issues with legacy scripts having RequireJS integration in development mode. In Production mode, resetting and re-enabling is done in the compiled bundle.
…/getNotAddableTypes.py
|
Superseded by #1809 |
Move PasswordResetTool to CMFPlone since there are interdependencies anyway.
This PR additionally does:
With this PR merged Products.PasswordResetTool is no longer necessary.