17 Oct
2018
17 Oct
'18
10:43 a.m.
On 17 Oct 2018, at 10:41, Simon Urli
<simon.urli(a)xwiki.com> wrote:
On 10/17/18 10:37 AM, Vincent Massol wrote:
Actually you don't have to rollback: the listener is catching the event before doing
the refactoring, so the user has a chance to edit the list of pages to refactor before the
refactor begins.
Before any page in the set has been moved/renamed?
Are we running 2 jobs? One job to perform the check on all pages and another job to
perform the refactoring?
Thanks
-Vincent
On 17 Oct
2018, at 10:31, Simon Urli <simon.urli(a)xwiki.com> wrote:
On 10/17/18 10:22 AM, Vincent Massol wrote:
Yes and I
remember mentioning that for me it’s too late… How do you rollback the move/rename if you
find an XClass after having renamed/moved 100 pages? And yes there’s the same problem with
the other refactoring (and I already mentioned the problem) but we need to improve at some
point and not continue doing it in a suboptimal way. We need to put ourselves in the shoes
of the user. Hi Simon,
> On 17 Oct 2018, at 10:12, Simon Urli <simon.urli(a)xwiki.com> wrote:
>
> Hi Vincent and all,
>
> On 10/17/18 9:41 AM, Vincent Massol wrote:
>> Hi Simon,
>>> On 16 Oct 2018, at 17:43, Simon Urli <simon.urli(a)xwiki.com> wrote:
>>>
>>> Hello everyone,
>>>
>>> I'm coming back on this proposal as the work is going on, to basically
propose to dropping the warning on copy action.
>>>
>>> I try to sum up why in the following.
>>>
>>> When implementing the proposal, I was adviced to use an event listener,
observing the deleting event for informing the user if he were doing a refactoring on a
document containing an XClass.
>>> This work is already implemented and working for Moving/Renaming pages (which
involve deleting the old page) and of course deleting.
>> The nice part about the listener is that it works for all use cases:
>> * rename/move from the UI
>> * rename/move from scripts
>> * etc
>
> To ease the discussion I just created a design page with some screenshot of my
current work. Then you can see what it looks like for the user:
https://design.xwiki.org/xwiki/bin/view/Proposal/PreventUserFromXClassRefac…
Ok cool so it seems you have it implemented at both the job level and the listener
level.
>
>> The bad parts are:
>> 1) right now we don’t provide a nice UI when an event is cancelled. AFAIR we just
display a stack trace in the UI which is definitely not good enough. Are you improving
this part?
>
> I reused the existing UI which does not look so bad IMO (see the screenshot in the
design page).
This is what happens in the AntiSpam app when the event is cancelled (ie when it finds
some spam in the doc):
https://extensions.xwiki.org/xwiki/bin/download/Extension/AntiSpam%20Tool%2…
As you can see it’s not really user-friendly.
Maybe you don’t get this because you’re running inside a job? But in this case I don’t
understand why you need a listener since you check for XClass before you start the
move/rename.
I must be missing something.
I don't check before I start the move/rename, I'm checking after the job already
started. I reuse exactly the same mechanism as the one used when refactoring a page that
belongs to an extension: https://jira.xwiki.org/browse/XWIKI-14591. 
>
>> 2) this is after the fact. Imagine that you’re renaming a set of pages and among
them there are several coming from an app. It’ll work fine on pages not having an XClass
(like moving the page having an XObject of that XClass) and then failing down the line on
the page having the XClass. That’s a problem because the xobject page will be wrongly
moved, since it doesn’t make sense that it’s moved if the other pages of the app are not
moved. Generally speaking you’ll have a bad state that is not easy to rollback.
>> This is why for me the check also has to be done in the move/rename UI and verify
that among the list of pages there are none with XClass and if so prevent moving/renaming
any page.
>> This is not in contradiction with the listener but the more important (from a
usage POV) is the check in the move/rename UI and not the listener which is a more
advanced use case.
>
> There might be a misunderstanding here: I use the listener to check the event that
are fired during the rename/move. As you can see in my screenshot, I got the warning in
the move/refactoring UI.
This listener is registered only during rename/move?
What happens if I write a script that moves/renames pages with XClass?
Nop the listener is globally registered, so I assume it would be triggered when running a
script too. >>> Now going back on "Copy"
the page, it's another job as I cannot rely on a "Deleting" event. I checked
quickly and I don't think I really could rely on other events for this: basically
copying is about creating a document and updating its content, and I don't think we
want to rely on those event for this mechanism.
>>>
>>> So unless you have another proposal to handle this case, I propose to simply
drop it.
>>>
>>> Do you agree?
>> AFAIK if you copy a set of pages with pages having an XClass in it, then the
copied pages won’t work so we shouldn’t drop this. We should just implement the protection
at the UI level (ie the copy action), same as for rename/move and not implement the
listener part (ie not support the script use case).
>
>
> I don't agree: the pages would work, but if they contain XObject they won't
use the copied XClass, only the older one.
> So for me the issue is not exactly the same: the problematic is not about copying an
XClass here, but a couple XClass + XObject. More difficult to detect and to handle IMO.
I still think we should handle it with a warning to explain this. It’s easy for me, we
just need to check for XClass and warn that any copied pages having a link to this XClass
will no longer point to it but will keep pointing to the original XClass. Easy to do.
I can handle it only on the UI side then.