Pre-edit check lifecycle

[mleduc]

Hello all,

Currently, we have three pre-edit check:

• securityLevel: a warning when the edited page belong to an extension (can be an error with some configuration change)
• documentLock a warning when the edit page is currently locked by another user
• requiredRights a warning when the required rights analyzer finds some issue in the difference between the rights of the document author and the rights of the current user (this check is currently deactivated by default)

When a user edits a page and a check identifies an issue, an explanation message is displayed to the user instead of going directly to the edit page.
From there, the user can either cancel the edition, or force the edition.
When force is used on a page, this is currently stored for the remainder of the user’s session.

Once a page is forced, all the pre-edit check warnings are ignored (but the errors are always displayed).

Consequently, the user will never be warned if:

• another user locked the page
• the required rights changed because another user updated the document

Meaning that a user can for instance edit a page and create a conflict without being aware of it.

I propose to change the way the force mechanism works to allow each check to have it’s own lifecycle:
For a given document:

• securityLevel would stay with the same behavior
• documentLock I propose to only raise a warning again if the user locking the page is not the same as the one from the previous check
• requiredRights I propose to raise a warning again if the required rights analyzed on the page differs from the ones of the previous time the page was forced edit

At the architecture level, this means that each checker will be in charge of his own lifecycle, and of caching the information required to know if the new check can be skipped.

WDYT?

[surli]

@mleduc

securityLevel: a warning when the edited page belong to an extension (can be an error with some configuration change)

(offtopic)
The name of this check should probably be changed if that’s possible…
(/offtopic)

@mleduc

At the architecture level, this means that each checker will be in charge of his own lifecycle, and of caching the information required to know if the new check can be skipped.

+1

@mleduc

• securityLevel would stay with the same behavior
• documentLock I propose to only raise a warning again if the user locking the page is not the same as the one from the previous check

+1

@mleduc

requiredRights I propose to raise a warning again if the required rights analyzed on the page differs from the ones of the previous time the page was forced edit

Just to be sure: only if the set of rights differs, not if the location of the right analysis differs? e.g. if it’s been forced for some script right and a new xobject that requires script right is added it won’t need to be forced again?
I’m +0 for this.

[mleduc]

@surli

Just to be sure: only if the set of rights differs, not if the location of the right analysis differs?

I’ve updated the initial proposal to make it clearer. The lifecycle are always for a given document.
Meaning that if you try to edit page A, ignore lock by user U1, then edit page B and this page is also locked by user U1, you will get the warning again even though this is locked by the same user.

The same applies for requiredRights, you will not received a warning even if the required check returns results only:

1. this is for the same page
2. the results are strictly equals

[mflorea]

+1 overall, but I’m not sure about:

@mleduc

documentLock I propose to only raise a warning again if the user locking the page is not the same as the one from the previous check

I think you should always be warned that another user has already started editing the same page. If you force the lock you gain the lock, AFAIK, so if you reload the page or leave the edit mode and then get back you will go directly to edit mode unless someone else started editing while you were outside edit, which is what we want IMO.

Thanks,
Marius

[mleduc]

@mflorea

If you force the lock you gain the lock

ok, I wasn’t aware of this, so indeed always warning might be relevant in this case. Thanks

[tmortagne]

+1

@mflorea

I think you should always be warned that another user has already started editing the same page. If you force the lock you gain the lock, AFAIK, so if you reload the page or leave the edit mode and then get back you will go directly to edit mode unless someone else started editing while you were outside edit, which is what we want IMO.

+1 if that’s indeed the case that you get the lock

[mflorea]

@tmortagne

+1 if that’s indeed the case that you get the lock

For a standard page edit the lock is set in two places:

• in EditAction, when the edit mode is rendered, taking into account the force request parameter, see https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/EditAction.java#L319 , so if you enter edit mode with force=1 in the URL you steal the edit lock
• in LockAction which is called async by lock.js which we load on edit action; but lock.js doesn’t forward the force flag to the LockAction so it has no effect if the lock is held by someone else; I think it should forward the force request parameter, but in any case, when lock.js is executed the page is already locked by EditAction (it may be an issue when lock.js is used outside edit mode)

The in-place editor also forwards the force flag to the lock, so if you enter in-place editing with the force flag you steal the lock.

The problem is that getEditConfirmation() from edit_macros.vm caches the force flag in a session attribute which is not taken into account neither by EditAction nor by LockAction. This means that when you edit a page that is locked:

• first time you force the edit and steal the lock (as expected)
• then whenever you edit again the same page in the same servlet session you go directly to edit mode because of the cached force flag but you don’t steal the lock anymore… this needs to be fixed by taking the force_edit_* session attribute into account in LockAction and EditAction.

Thanks,
Marius