Re: [Framework-Team] PLIP #240 ready for review
On Jan 17, 2009, at 12:04 AM, Andrew Burkhalter wrote: PLIP 240 (Improve locking configurability) has been implemented. You can get the review buildout from: https://svn.plone.org/svn/plone/review/plip240-locking-improvements/ i've added my review notes in http://dev.plone.org/plone/changeset/24800 in short: i'm +1 for merging. minor improvements are possible, but otherwise another really nice PLIP implementation from andrew david. cheers, andi -- zeidler it consulting - http://zitc.de/ - i...@zitc.de friedelstraße 31 - 12047 berlin - telefon +49 30 25563779 pgp key at http://zitc.de/pgp - http://wwwkeys.de.pgp.net/ plone 3.2rc1 released! -- http://plone.org/products/plone/ PGP.sig Description: This is a digitally signed message part ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #240 ready for review
Erik Rose wrote: I actually first implemented it exactly that way (even called it IRefreshableLockable), then wondered if the complexity was worth it. I can go either way, but would like to hear some other opinions of best practice in a case like this. For what a non-3.x-FWT opinion is worth, I'd rather program against a proper IRefreshableLockable interface. Or, if nobody's implementing it in the wild, it'd be even better (less complex) to revise ILockable. I'd recommend to stay away from revising ILockable as this isn't necessarily even Plone specific. IIRC Jeff and I where trying to follow the at-the-time default way in which Zope handled DAV locks when we wrote this. Raphael Cheers, Erik ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #240 ready for review
I actually first implemented it exactly that way (even called it IRefreshableLockable), then wondered if the complexity was worth it. I can go either way, but would like to hear some other opinions of best practice in a case like this. For what a non-3.x-FWT opinion is worth, I'd rather program against a proper IRefreshableLockable interface. Or, if nobody's implementing it in the wild, it'd be even better (less complex) to revise ILockable. Cheers, Erik ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #240 ready for review
On Tue, Jan 27, 2009 at 18:12, Erik Rose psuc...@grinchcentral.com wrote: I actually first implemented it exactly that way (even called it IRefreshableLockable), then wondered if the complexity was worth it. I can go either way, but would like to hear some other opinions of best practice in a case like this. For what a non-3.x-FWT opinion is worth, I'd rather program against a proper IRefreshableLockable interface. Or, if nobody's implementing it in the wild, it'd be even better (less complex) to revise ILockable. ILockable is used in the wild; I've got implementations where TTW locking is disabled altogether for example, by implementing ILockable as no-op operations. -- Martijn Pieters ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #240 ready for review
Ok, I reviewed this plip and did some tests with ttw locking and indeed it seems to work. I leave all the technical review stuff to people with brains. I have a remark for the help text below Enable locking for through- the-web edits though. I think that the line Disabling locking here will only affect users editing content through the Plone web UI. Content edited via WebDAV clients will still be subject to locking. by itself isn't explaining much what this setting does. I want to know why I would want to enable or disable this option. So a +1 but please add a bit more explanation to the switch in site props. On 17 jan 2009, at 00:04, Andrew Burkhalter wrote: PLIP 240 (Improve locking configurability) has been implemented. You can get the review buildout from: https://svn.plone.org/svn/plone/review/plip240-locking-improvements/ I'm going to paste the contents of https://svn.plone.org/svn/plone/review/plip240-locking-improvements/PLIP_240_README.txt below for ease of any discussion. Please take careful note of the Other section below. Thanks! Andrew (sending on behalf of David) PLIP 240: Improve locking configurability = This review buildout presents the changes that have been made in order to implement PLIP 240. The goal of this PLIP is to give more flexibility in configuring Plone's lock-on-edit behavior. One of the original aims of this PLIP was to reduce the pain that has been occurring in the Plone 3.x series when items get inadvertently locked and then accidentally left in that state. A late-October fix in Archetypes (http://dev.plone.org/archetypes/changeset/10163, which made it into Plone 3.2) already helped greatly with this by making sure that the unlockOnFormUnload.js script also runs for inline edits. However, that script doesn't work in Webkit-based browsers, nor in case of things like browser crashes. So we have gone ahead with the plan to change the default lock timeout to 5 minutes and keep it alive via a periodic AJAX call, as well as adding a global lock-on-edit on/off switch. The unlockOnFormUnload.js also continues its old behavior, as it is still useful when it works. Summary of changes -- * plone.locking: - Changed the default timeout for through-the-web locks to 10 minutes. - Added a refresh_lock method to the ILockable interface and to the default implementation, TTWLockable - Added a publishable refresh_lock method to the plone_lock_operations browser view - Made the lock method from ITTWLockable check the lock_on_ttw_edit setting * Plone: - Added lock_on_ttw_edit property to site_properties, with a default value of True to match the behavior of Plone 3.3 - Adjusted unlockOnFormUnload.js to also call the refresh_lock method every 5 minutes (for forms with the enableLockProtection class only) - Make sure the 'removeLockProtection' KSS action fires when someone cancels inline editing, to stop calling the periodic refresh_lock * plone.app.controlpanel: - Added 'Enable locking' setting to the site configlet, which adjusts the lock_on_ttw_edit property - Changed the SiteControlPanelAdapter to also adapt ILockSettings from plone.locking so that plone.locking can look up the value of the setting without introducing a hard dependency of plone.locking on other parts of Plone. (This does introduce a dependency of plone.app.controlpanel on plone.locking, but that seems less bad.) * plone.app.kss: - Fixed the broken implementation of the 'removeLockProtection' KSS action Summary of test assertions -- X default timeout is 10 minutes for TTW X site creation adds 'lock_on_ttw_edit' to site properties (value: True) X - (and migration) X plone.locking.lockable.TTWLockable has a refresh_lock method that updates the lock time X there is a configlet which can edit the lock_on_ttw_edit property Not tested yet but will be before merging: - the lock_on_ttw_edit property is actually taken into account by content (even if old locks remain from before the property was turned off) Tested manually (because I wasn't sure how to automate it): - The periodic AJAX call from unlockOnFormUnload.js actually updates the lock expiration time every five minutes. (This is easiest to test by changing the time to 5 seconds and watching the Firebug console.) - Locking, and the auto-refresh behavior, is correctly activated/deactivated when inline editing begins/ends. I only had time to test on Firefox and Safari, so if the reviewers can check other browsers I'd appreciate it. Needed documentation changes * Documentation of the locking feature should note the new setting in the site configlet for enabling/disabling locking. * Any existing documentation of lock timeouts should be adjusted to note the new behavior. Needed translations
Re: [Framework-Team] PLIP #240 ready for review
On Jan 25, 2009, at 4:09 AM, Martijn Pieters wrote: On Sat, Jan 17, 2009 at 00:04, Andrew Burkhalter andrewburkhal...@gmail.com wrote: PLIP 240 (Improve locking configurability) has been implemented. You can get the review buildout from: https://svn.plone.org/svn/plone/review/plip240-locking-improvements/ This PLIP gets a +1 from me. I've looked at the code changes for the 4 affected packages, ran the tests and tested the functionality manually. I have but one remark: the ILockable interface has been extended requiring users of the interface to test if the additional method is actually present in implementors. This test smells icky to me; better to assume the method is there (and risk breaking 3rd party implementations), or define a new interface that extends ILockable (IRefreshableLockable?). Then register LockingOperations for that interface instead or test for IRefreshableLockable instead of a hasattr test. However, I feel that removing that icky smell is not a prerequisite for acceptance, just advice to improve what otherwise is an excellent implementation. I actually first implemented it exactly that way (even called it IRefreshableLockable), then wondered if the complexity was worth it. I can go either way, but would like to hear some other opinions of best practice in a case like this. peace, David Glick Web Developer ONE/Northwest New tools and strategies for engaging people in protecting the environment http://www.onenw.org davidgl...@onenw.org work: (206) 286-1235 x32 mobile: (206) 679-3833 Subscribe to ONEList, our email newsletter! Practical advice for effective online engagement http://www.onenw.org/full_signup ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #240 ready for review
On Jan 16, 2009, at 3:04 PM, Andrew Burkhalter wrote: Other - There is a weird edge case in the following scenario: 1. Manager turns off locking. 2. User A starts editing a document 3. Manager turns locking back on 4. User A clicks the edit tab again 5. User B views the document The document should show as locked to User B, but it doesn't. We didn't have time to track this down or compare to current Plone behavior, but will do so before merging this PLIP. However, it should be noted that if the user in step 4 above hits view and then the edit tab (rather than just hitting edit an additional time) the locking viewlet behaves as expected (e.g. the item is listed as locked) for user B in step 5. All initial analysis of our added code from the debugger behaved as expected, so more investigation is needed on our part to determine if this was introduced by us. Hey, just wanted to note that Andrew and I investigated this, discovered it was due to a previously existing bug in the unlockOnFormUnload.js script, and implemented a fix in the branch for PLIP #240. In the process we also discovered a related bug which results in items getting unlocked following a validation error even though someone is still editing. We'll port the fix for that issue to the 3.2 branch and trunk regardless of the decision on PLIP 240 as a whole. peace, David Glick Web Developer ONE/Northwest New tools and strategies for engaging people in protecting the environment http://www.onenw.org davidgl...@onenw.org work: (206) 286-1235 x32 mobile: (206) 679-3833 Subscribe to ONEList, our email newsletter! Practical advice for effective online engagement http://www.onenw.org/full_signup ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
Re: [Framework-Team] PLIP #240 ready for review
David Glick wrote: [..] Hey, just wanted to note that Andrew and I investigated this, discovered it was due to a previously existing bug in the unlockOnFormUnload.js script, and implemented a fix in the branch for PLIP #240. In the process we also discovered a related bug which results in items getting unlocked following a validation error even though someone is still editing. We'll port the fix for that issue to the 3.2 branch and trunk regardless of the decision on PLIP 240 as a whole. Great! Obviously this is fine with me as this is just a bug fix. Thanks for looking into this, Raphael ___ Framework-Team mailing list Framework-Team@lists.plone.org http://lists.plone.org/mailman/listinfo/framework-team
[Framework-Team] PLIP #240 ready for review
PLIP 240 (Improve locking configurability) has been implemented. You can get the review buildout from: https://svn.plone.org/svn/plone/review/plip240-locking-improvements/ I'm going to paste the contents of https://svn.plone.org/svn/plone/review/plip240-locking-improvements/PLIP_240_README.txt below for ease of any discussion. Please take careful note of the Other section below. Thanks! Andrew (sending on behalf of David) PLIP 240: Improve locking configurability = This review buildout presents the changes that have been made in order to implement PLIP 240. The goal of this PLIP is to give more flexibility in configuring Plone's lock-on-edit behavior. One of the original aims of this PLIP was to reduce the pain that has been occurring in the Plone 3.x series when items get inadvertently locked and then accidentally left in that state. A late-October fix in Archetypes (http://dev.plone.org/archetypes/changeset/10163, which made it into Plone 3.2) already helped greatly with this by making sure that the unlockOnFormUnload.js script also runs for inline edits. However, that script doesn't work in Webkit-based browsers, nor in case of things like browser crashes. So we have gone ahead with the plan to change the default lock timeout to 5 minutes and keep it alive via a periodic AJAX call, as well as adding a global lock-on-edit on/off switch. The unlockOnFormUnload.js also continues its old behavior, as it is still useful when it works. Summary of changes -- * plone.locking: - Changed the default timeout for through-the-web locks to 10 minutes. - Added a refresh_lock method to the ILockable interface and to the default implementation, TTWLockable - Added a publishable refresh_lock method to the plone_lock_operations browser view - Made the lock method from ITTWLockable check the lock_on_ttw_edit setting * Plone: - Added lock_on_ttw_edit property to site_properties, with a default value of True to match the behavior of Plone 3.3 - Adjusted unlockOnFormUnload.js to also call the refresh_lock method every 5 minutes (for forms with the enableLockProtection class only) - Make sure the 'removeLockProtection' KSS action fires when someone cancels inline editing, to stop calling the periodic refresh_lock * plone.app.controlpanel: - Added 'Enable locking' setting to the site configlet, which adjusts the lock_on_ttw_edit property - Changed the SiteControlPanelAdapter to also adapt ILockSettings from plone.locking so that plone.locking can look up the value of the setting without introducing a hard dependency of plone.locking on other parts of Plone. (This does introduce a dependency of plone.app.controlpanel on plone.locking, but that seems less bad.) * plone.app.kss: - Fixed the broken implementation of the 'removeLockProtection' KSS action Summary of test assertions -- X default timeout is 10 minutes for TTW X site creation adds 'lock_on_ttw_edit' to site properties (value: True) X - (and migration) X plone.locking.lockable.TTWLockable has a refresh_lock method that updates the lock time X there is a configlet which can edit the lock_on_ttw_edit property Not tested yet but will be before merging: - the lock_on_ttw_edit property is actually taken into account by content (even if old locks remain from before the property was turned off) Tested manually (because I wasn't sure how to automate it): - The periodic AJAX call from unlockOnFormUnload.js actually updates the lock expiration time every five minutes. (This is easiest to test by changing the time to 5 seconds and watching the Firebug console.) - Locking, and the auto-refresh behavior, is correctly activated/deactivated when inline editing begins/ends. I only had time to test on Firefox and Safari, so if the reviewers can check other browsers I'd appreciate it. Needed documentation changes * Documentation of the locking feature should note the new setting in the site configlet for enabling/disabling locking. * Any existing documentation of lock timeouts should be adjusted to note the new behavior. Needed translations --- - title and description for the new schema field in plone.app.controlpanel's site.py Backwards compatibility --- * Custom ILockable adapters should continue to operate without error, though the lock refresh will not actually take place unless they are adjusted to implement the new refresh_lock method of the ILockable interface. * Any custom monkey patches to plone.locking.lockable.MAXTIMEOUT to change the timeout value will break. * Any customizations of the unlockOnFormUnload.js script will have to be updated. Other - There is a weird edge case in the following scenario: 1. Manager turns off locking. 2. User A starts editing a
