[Framework-Team] PLIP145 Locking - Review

[Raphael Ritz]

Alec Mitchell schrieb:
> On 9/25/06, Helge Tesdal <tesdal at plonesolutions.com> wrote:
Hi Helge, hi Alec,

thanks for taking a look at this.
I take the liberty to add some comments inline below.
>>
>> 2. Test setup
>> =============
>> I wasn't actually able to test the review bundle now with Zope 2.10 from
>> SVN, so I'm basing this review on the demonstration during Archipelago
>> sprint and looking at code.
>
> I wasn't able to test it either, but I looked at the code extensively.
>
Because we screwed up the bundle or because of lack of time?
I recall having had a problem with JS on Zope 2.10.
Maybe this is still broken in the bundle? (We'll check that)

Also note that it is not really straight-forward to see the effect.
If you just set-up and use a default Plone site you will not
notice any change. It really is a multi-user feature, i.e.,
only if one user is editing an item and some other user who
is also allowed to modifiy the item is visiting this item s/he
will see an effect. You really need two simultaneous browser
sessions with different users authenticaed for this to become
visible (logging out as one and later logging in as the other
won't tell you anything as the logout will also remove the
lock set).
>> 3. How does it work
>> ===================
>> When editing content, an event is fired, and an event handler sets a
>> WebDAV lock to prevent concurrent editing. When editing ends or is
>> cancelled, an event is fired, and an event handler removes the WebDAV
>> lock. I believe you can also remove stale locks.
That's correct.
>>
>> 4. State of code
>> ================
>> Implemented using view, events and event handlers. Not a lot of code 
>> (or I
>> missed some code), fairly easy to get an overview of and modify.
>
it really is not much code
> The existing code is a bit too invasive into Archetypes for my liking.
really?
> I think there are quite a few places where the CA could be used
> instead of patching AT.  Of course this is partly because things like
> events fired on add and edit didn't exist when the code was written,
> and neither did the potential of content-providers and viewlets.
that's also correct.
> Hopefully instead of patches into AT, we can add extension points in
> AT as needed and use those for this product.
if they were there of course!

First of all, wrt to code location: we needed to put the code somewhere
and as we didn't want to introduce yet another package just for two
event definitions and appropriate subscribers we placed them into
Archetypes. This could be changed to any other place, of course.
Note that neither the events nor their subscribers are in any way
Archetypes or even Plone specific.

WRT "patching" BaseObject: really the only change to BaseObject
is to use the 'post_edit_hook" to fire the event signalling that edit
is finished. If there is another more appropriate event by now,
we can of course subscribe to that one right away and be done
without touching BaseObject. (but five month ago when we
implemented this there was no such event in place).

The trickiest part I think is setting the lock the moment the edit
process starts. For now, we've chosen to tie this to the rendering
of the edit form. As I'm not aware of any (planned) event
to be fired on rendering 'base_edit' (or any edit form for that matter)
we decided to trigger the event firing from base_edit (a skin template
after all) right away. We needed to touch 'base_edit' anyway because
it explicitly checks for the WebDAV locks and marks the item as
locked if so set. Now, since we are re-using the WebDAV locks we
need to destinguish between the user holding the lock and any other
user. So we needed to change 'base_edit' to do a more clever lock
checking anyway.

Other things like the update of the status message or the lock release
on browsing away from the form without pressing save or cancel
are indeed just boldly put into the AT templates because it was our
understanding that this is something that should be implemented
using our upcoming AJAX framework anyway which also wasn't
decided upon at the time we implemented this.

I agree that this part is Archetypes-specific and while it is true
that this does not readily apply to Zope 3-based content items
it might not be that easy to come up with a generic way to
signal the begin of an editing process. In particular wrt the
planned inline editing ...
In my LockingWorkflow product I actually (ab)use the formcontroller
to first invoke a custom method to set the lock before handing over to
the edit form but this would fail for inline editing as well.

>
>> 5. Suggested improvements
>> =========================
>> This needs to be made compatible and aware of iterate. Event naming
>> conventions has to be harmonized with iterate. 
Sure if we agree to go with iterate (as it seems by now).
But please understand that at the sprint when we wrote
this there was no mention of iterate or anything like that
in sight. And when it comes to naming conventions: I
couldn't care less. I'm horrible and usually fail miserably
whan it comes to inventing names - that's what you get
when you confront a Belgium and a German with English ;-)
>> Furthermore, it should not
>> be possible to easily remove the lock set by iterate on the published
>> content (or locks set by any other applications for that matter). Check
>> the comments section of the PLIP page for specifics.
>
the ease with which you can currently remove/steal the lock was
explicitly introduced on Alex request but again, at that time we
didn't even think of things like iterate.
> Yes there is some real overlap between these two, and we need to make
> sure they fit together well.  The same goes for CMFEditions and the
> existing history viewing support (though I neglected to mention it in
> my comments there), the fancy ajax code for history support can
> probably easily be grafted into CMFEditions to ensure we have a
> consistent user experience.
>
BTW: if shipping with CMFEditions/iterate: did we already decide
whether to enable versioning/staging per default or whether we
leave this as an option to the site admin? In case of the latter the
TTW locking might need to support and provide different policies
wrt lock setting and removing.
>> 6. Recommendation
>> =================
>> +1 to including this in Plone 3 - provided the suggested improvements 
>> are
>> done.
>
> I'm +0 for now because of my reservations about adding so much usecase
> specific code directly into the Archetypes framework.
Alec, could you maybe elaborate a bit here in light of my comments above?
> I think its
> very important that we begin to consider the potential importance of
> non-AT content, and not hinder plone by tying more core functionality,
> which should be available to all types if possible, to that framework.
Sure. Do we have an event that gets fired the moment an editing
process starts? (rendering an edit form or the user choosing to
start some in-line editing process)? If so, we are done.
If not, we should introduce and fire such an event.
(suggestions on who to actually do this are welcome!)
Again, at the time we've considered AT to be "the framework".
But I agree with Alec that we should aim for a more generic solution.
> However, I'd be surprised if the team that created this code wouldn't
> be willing and capable of decoupling it from AT.
>
Correct ;-)

Raphael
> Alec
>
> _______________________________________________
> Framework-Team mailing list
> Framework-Team at lists.plone.org
> http://lists.plone.org/mailman/listinfo/framework-team