[Proposal] Prevent users from renaming/move pages with XClass definition

classic Classic list List threaded Threaded
29 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Prevent users from renaming/move pages with XClass definition

vmassol
Administrator
Hi Simon,

> On 17 Oct 2018, at 10:12, Simon Urli <[hidden email]> 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 <[hidden email]> 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/PreventUserFromXClassRefactoring

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%20Application/antispam-error.png

 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.

>
>> 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?

>>> 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.

Thanks
-Vincent

>
> Simon
>> Thanks
>> -Vincent
>>>
>>> Simon
>>>
>>>
>>> On 9/26/18 10:27 AM, Simon Urli wrote:
>>>> Hi everyone,
>>>> ok trying to sum-up (I'm only talking about cases with XClass below, to simplify):
>>>>   - according to Vincent, we should completely prevent simple users to copy/move/rename and only allow advanced users to do it after a warning
>>>>   - according to Adel & Clément: preventing simple users will be useless as they can easily switch the advanced feature in their account
>>>>   - according to Marius copying a page/app is not necessarily harmful compared to moving/renaming and we should manage it differently.
>>>> I really don't know the practice of users on the field, but it looks to me that preventing simple users to do the action and telling them to ask an advanced user is actually a good trade-off:
>>>>  1. it will warn users that they might be doing something wrong
>>>>  2. it's not something completely blocking: either they ask for the admin/advanced user, or they know they can switch the advanced features by themselves, at their own risks
>>>> Now maybe we can only do the warning for the "copy" action.
>>>> WDYT?
>>>> Simon
>>>> On 9/25/18 11:36 AM, Vincent Massol wrote:
>>>>> Hi Marius,
>>>>>
>>>>>> On 25 Sep 2018, at 11:34, Marius Dumitru Florea <[hidden email]> wrote:
>>>>>>
>>>>>> On Sun, Sep 23, 2018 at 11:12 AM Vincent Massol <[hidden email]> wrote:
>>>>>>
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>>> On 21 Sep 2018, at 16:58, Simon Urli <[hidden email]> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/21/18 4:53 PM, Adel Atallah wrote:
>>>>>>>>> +1 for the warning, but I would not forbid simple users from renaming
>>>>>>>>> or moving pages but instead just hide the action (from the page menu).
>>>>>>>>
>>>>>>>> OK I should have written it: by "forbid" I meant:
>>>>>>>>
>>>>>>>> 1. Hide the action from the menu
>>>>>>>> 2. Return an error message if the user try to access the
>>>>>>> renaming/moving page (using forged URL)
>>>>>>>>
>>>>>>>> So you suggest we shouldn't do 2?
>>>>>>>
>>>>>>> So +1 to prevent/warn the user when doing a move/renaming
>>>>>>
>>>>>>
>>>>>>
>>>>>>> AND copy pages containing XClass definitions
>>>>>>
>>>>>>
>>>>>> FTR, copying a single page having an XClass definition is not dangerous (it
>>>>>> won't break the application that owns the page), as it only creates a new
>>>>>> class definition. Copying an entire application is not dangerous either.
>>>>>> The copy won't work like the original application (this justifies a warning
>>>>>> as it may fail the user expectations), but the original application will
>>>>>> still work. Renaming or moving an application is dangerous as it breaks the
>>>>>> application.
>>>>>
>>>>> Yes you’re correct. Unless the user does a copy + delete ;)
>>>>>
>>>>> Thanks
>>>>> -Vincent
>>>>>
>>>>>>
>>>>>>> (the message should list all such pages).
>>>>>>>
>>>>>>> -1 to hide the action from the menu (if you’re talking about the
>>>>>>> “Move/Rename” and “Copy" actions) because:
>>>>>>> 1) you get to choose whether you move/rename/copy children after you click
>>>>>>> the action
>>>>>>> 2) even when the current page has an XClass, the user wouldn't understand
>>>>>>> why he cannot see/click on the action. It’s better that he can do it but
>>>>>>> get an error message, explaining why and telling him that to contact an
>>>>>>> advanced users if he really needs to do it.
>>>>>>>
>>>>>>> Thanks
>>>>>>> -Vincent
>>>>>>>
>>>>>>>>
>>>>>>>>> On Fri, Sep 21, 2018 at 4:44 PM Simon Urli <[hidden email]>
>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> users might currently break their AWM application by renaming/moving
>>>>>>>>>> pages containing XClass definition.
>>>>>>>>>>
>>>>>>>>>> We need a proper refactoring operation to be able to properly do such
>>>>>>>>>> move/rename. But this feature might take a while to be completely
>>>>>>>>>> available.
>>>>>>>>>>
>>>>>>>>>> In the meantime I propose that we prevent users from renaming/moving
>>>>>>>>>> pages containing XClass.
>>>>>>>>>>
>>>>>>>>>> What I propose is the following:
>>>>>>>>>>   - Forbid completely *simple users* to rename/move pages containing
>>>>>>> XClass
>>>>>>>>>>   - Display a warning to *advanced users* when they perform such
>>>>>>>>>> operation: the same kind of warning we already have when performing
>>>>>>> edit
>>>>>>>>>> on XWiki pages
>>>>>>>>>>
>>>>>>>>>> WDYT?
>>>>>>>>>>
>>>>>>>>>> Simon
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Simon Urli
>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>> [hidden email]
>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>
>>>>>>>> --
>>>>>>>> Simon Urli
>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>> [hidden email]
>>>>>>>> More about us at http://www.xwiki.com
>>>>>
>>>
>>> --
>>> Simon Urli
>>> Software Engineer at XWiki SAS
>>> [hidden email]
>>> More about us at http://www.xwiki.com
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> [hidden email]
> More about us at http://www.xwiki.com

Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Prevent users from renaming/move pages with XClass definition

Simon Urli


On 10/17/18 10:22 AM, Vincent Massol wrote:

> Hi Simon,
>
>> On 17 Oct 2018, at 10:12, Simon Urli <[hidden email]> 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 <[hidden email]> 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/PreventUserFromXClassRefactoring
>
> 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%20Application/antispam-error.png
>
>   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.

Simon

>
> Thanks
> -Vincent
>
>>
>> Simon
>>> Thanks
>>> -Vincent
>>>>
>>>> Simon
>>>>
>>>>
>>>> On 9/26/18 10:27 AM, Simon Urli wrote:
>>>>> Hi everyone,
>>>>> ok trying to sum-up (I'm only talking about cases with XClass below, to simplify):
>>>>>    - according to Vincent, we should completely prevent simple users to copy/move/rename and only allow advanced users to do it after a warning
>>>>>    - according to Adel & Clément: preventing simple users will be useless as they can easily switch the advanced feature in their account
>>>>>    - according to Marius copying a page/app is not necessarily harmful compared to moving/renaming and we should manage it differently.
>>>>> I really don't know the practice of users on the field, but it looks to me that preventing simple users to do the action and telling them to ask an advanced user is actually a good trade-off:
>>>>>   1. it will warn users that they might be doing something wrong
>>>>>   2. it's not something completely blocking: either they ask for the admin/advanced user, or they know they can switch the advanced features by themselves, at their own risks
>>>>> Now maybe we can only do the warning for the "copy" action.
>>>>> WDYT?
>>>>> Simon
>>>>> On 9/25/18 11:36 AM, Vincent Massol wrote:
>>>>>> Hi Marius,
>>>>>>
>>>>>>> On 25 Sep 2018, at 11:34, Marius Dumitru Florea <[hidden email]> wrote:
>>>>>>>
>>>>>>> On Sun, Sep 23, 2018 at 11:12 AM Vincent Massol <[hidden email]> wrote:
>>>>>>>
>>>>>>>> Hi Simon,
>>>>>>>>
>>>>>>>>> On 21 Sep 2018, at 16:58, Simon Urli <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 9/21/18 4:53 PM, Adel Atallah wrote:
>>>>>>>>>> +1 for the warning, but I would not forbid simple users from renaming
>>>>>>>>>> or moving pages but instead just hide the action (from the page menu).
>>>>>>>>>
>>>>>>>>> OK I should have written it: by "forbid" I meant:
>>>>>>>>>
>>>>>>>>> 1. Hide the action from the menu
>>>>>>>>> 2. Return an error message if the user try to access the
>>>>>>>> renaming/moving page (using forged URL)
>>>>>>>>>
>>>>>>>>> So you suggest we shouldn't do 2?
>>>>>>>>
>>>>>>>> So +1 to prevent/warn the user when doing a move/renaming
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> AND copy pages containing XClass definitions
>>>>>>>
>>>>>>>
>>>>>>> FTR, copying a single page having an XClass definition is not dangerous (it
>>>>>>> won't break the application that owns the page), as it only creates a new
>>>>>>> class definition. Copying an entire application is not dangerous either.
>>>>>>> The copy won't work like the original application (this justifies a warning
>>>>>>> as it may fail the user expectations), but the original application will
>>>>>>> still work. Renaming or moving an application is dangerous as it breaks the
>>>>>>> application.
>>>>>>
>>>>>> Yes you’re correct. Unless the user does a copy + delete ;)
>>>>>>
>>>>>> Thanks
>>>>>> -Vincent
>>>>>>
>>>>>>>
>>>>>>>> (the message should list all such pages).
>>>>>>>>
>>>>>>>> -1 to hide the action from the menu (if you’re talking about the
>>>>>>>> “Move/Rename” and “Copy" actions) because:
>>>>>>>> 1) you get to choose whether you move/rename/copy children after you click
>>>>>>>> the action
>>>>>>>> 2) even when the current page has an XClass, the user wouldn't understand
>>>>>>>> why he cannot see/click on the action. It’s better that he can do it but
>>>>>>>> get an error message, explaining why and telling him that to contact an
>>>>>>>> advanced users if he really needs to do it.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> -Vincent
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On Fri, Sep 21, 2018 at 4:44 PM Simon Urli <[hidden email]>
>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>> users might currently break their AWM application by renaming/moving
>>>>>>>>>>> pages containing XClass definition.
>>>>>>>>>>>
>>>>>>>>>>> We need a proper refactoring operation to be able to properly do such
>>>>>>>>>>> move/rename. But this feature might take a while to be completely
>>>>>>>>>>> available.
>>>>>>>>>>>
>>>>>>>>>>> In the meantime I propose that we prevent users from renaming/moving
>>>>>>>>>>> pages containing XClass.
>>>>>>>>>>>
>>>>>>>>>>> What I propose is the following:
>>>>>>>>>>>    - Forbid completely *simple users* to rename/move pages containing
>>>>>>>> XClass
>>>>>>>>>>>    - Display a warning to *advanced users* when they perform such
>>>>>>>>>>> operation: the same kind of warning we already have when performing
>>>>>>>> edit
>>>>>>>>>>> on XWiki pages
>>>>>>>>>>>
>>>>>>>>>>> WDYT?
>>>>>>>>>>>
>>>>>>>>>>> Simon
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Simon Urli
>>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>>> [hidden email]
>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Simon Urli
>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>> [hidden email]
>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>
>>>>
>>>> --
>>>> Simon Urli
>>>> Software Engineer at XWiki SAS
>>>> [hidden email]
>>>> More about us at http://www.xwiki.com
>>
>> --
>> Simon Urli
>> Software Engineer at XWiki SAS
>> [hidden email]
>> More about us at http://www.xwiki.com
>

--
Simon Urli
Software Engineer at XWiki SAS
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Prevent users from renaming/move pages with XClass definition

vmassol
Administrator


> On 17 Oct 2018, at 10:31, Simon Urli <[hidden email]> wrote:
>
>
>
> On 10/17/18 10:22 AM, Vincent Massol wrote:
>> Hi Simon,
>>> On 17 Oct 2018, at 10:12, Simon Urli <[hidden email]> 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 <[hidden email]> 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/PreventUserFromXClassRefactoring
>> 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%20Application/antispam-error.png
>>  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.

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.

>
>>>
>>>> 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.

ok, so try this: http://playground.xwiki.org/xwiki/bin/delete/MyPage/?confirm=1

What happens?

>
>>>>> 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.

Yes, I agree.

Thanks
-Vincent

>
> Simon
>> Thanks
>> -Vincent
>>>
>>> Simon
>>>> Thanks
>>>> -Vincent
>>>>>
>>>>> Simon
>>>>>
>>>>>
>>>>> On 9/26/18 10:27 AM, Simon Urli wrote:
>>>>>> Hi everyone,
>>>>>> ok trying to sum-up (I'm only talking about cases with XClass below, to simplify):
>>>>>>   - according to Vincent, we should completely prevent simple users to copy/move/rename and only allow advanced users to do it after a warning
>>>>>>   - according to Adel & Clément: preventing simple users will be useless as they can easily switch the advanced feature in their account
>>>>>>   - according to Marius copying a page/app is not necessarily harmful compared to moving/renaming and we should manage it differently.
>>>>>> I really don't know the practice of users on the field, but it looks to me that preventing simple users to do the action and telling them to ask an advanced user is actually a good trade-off:
>>>>>>  1. it will warn users that they might be doing something wrong
>>>>>>  2. it's not something completely blocking: either they ask for the admin/advanced user, or they know they can switch the advanced features by themselves, at their own risks
>>>>>> Now maybe we can only do the warning for the "copy" action.
>>>>>> WDYT?
>>>>>> Simon
>>>>>> On 9/25/18 11:36 AM, Vincent Massol wrote:
>>>>>>> Hi Marius,
>>>>>>>
>>>>>>>> On 25 Sep 2018, at 11:34, Marius Dumitru Florea <[hidden email]> wrote:
>>>>>>>>
>>>>>>>> On Sun, Sep 23, 2018 at 11:12 AM Vincent Massol <[hidden email]> wrote:
>>>>>>>>
>>>>>>>>> Hi Simon,
>>>>>>>>>
>>>>>>>>>> On 21 Sep 2018, at 16:58, Simon Urli <[hidden email]> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 9/21/18 4:53 PM, Adel Atallah wrote:
>>>>>>>>>>> +1 for the warning, but I would not forbid simple users from renaming
>>>>>>>>>>> or moving pages but instead just hide the action (from the page menu).
>>>>>>>>>>
>>>>>>>>>> OK I should have written it: by "forbid" I meant:
>>>>>>>>>>
>>>>>>>>>> 1. Hide the action from the menu
>>>>>>>>>> 2. Return an error message if the user try to access the
>>>>>>>>> renaming/moving page (using forged URL)
>>>>>>>>>>
>>>>>>>>>> So you suggest we shouldn't do 2?
>>>>>>>>>
>>>>>>>>> So +1 to prevent/warn the user when doing a move/renaming
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> AND copy pages containing XClass definitions
>>>>>>>>
>>>>>>>>
>>>>>>>> FTR, copying a single page having an XClass definition is not dangerous (it
>>>>>>>> won't break the application that owns the page), as it only creates a new
>>>>>>>> class definition. Copying an entire application is not dangerous either.
>>>>>>>> The copy won't work like the original application (this justifies a warning
>>>>>>>> as it may fail the user expectations), but the original application will
>>>>>>>> still work. Renaming or moving an application is dangerous as it breaks the
>>>>>>>> application.
>>>>>>>
>>>>>>> Yes you’re correct. Unless the user does a copy + delete ;)
>>>>>>>
>>>>>>> Thanks
>>>>>>> -Vincent
>>>>>>>
>>>>>>>>
>>>>>>>>> (the message should list all such pages).
>>>>>>>>>
>>>>>>>>> -1 to hide the action from the menu (if you’re talking about the
>>>>>>>>> “Move/Rename” and “Copy" actions) because:
>>>>>>>>> 1) you get to choose whether you move/rename/copy children after you click
>>>>>>>>> the action
>>>>>>>>> 2) even when the current page has an XClass, the user wouldn't understand
>>>>>>>>> why he cannot see/click on the action. It’s better that he can do it but
>>>>>>>>> get an error message, explaining why and telling him that to contact an
>>>>>>>>> advanced users if he really needs to do it.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> -Vincent
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> On Fri, Sep 21, 2018 at 4:44 PM Simon Urli <[hidden email]>
>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>
>>>>>>>>>>>> users might currently break their AWM application by renaming/moving
>>>>>>>>>>>> pages containing XClass definition.
>>>>>>>>>>>>
>>>>>>>>>>>> We need a proper refactoring operation to be able to properly do such
>>>>>>>>>>>> move/rename. But this feature might take a while to be completely
>>>>>>>>>>>> available.
>>>>>>>>>>>>
>>>>>>>>>>>> In the meantime I propose that we prevent users from renaming/moving
>>>>>>>>>>>> pages containing XClass.
>>>>>>>>>>>>
>>>>>>>>>>>> What I propose is the following:
>>>>>>>>>>>>   - Forbid completely *simple users* to rename/move pages containing
>>>>>>>>> XClass
>>>>>>>>>>>>   - Display a warning to *advanced users* when they perform such
>>>>>>>>>>>> operation: the same kind of warning we already have when performing
>>>>>>>>> edit
>>>>>>>>>>>> on XWiki pages
>>>>>>>>>>>>
>>>>>>>>>>>> WDYT?
>>>>>>>>>>>>
>>>>>>>>>>>> Simon
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> Simon Urli
>>>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Simon Urli
>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>> [hidden email]
>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>
>>>>>
>>>>> --
>>>>> Simon Urli
>>>>> Software Engineer at XWiki SAS
>>>>> [hidden email]
>>>>> More about us at http://www.xwiki.com
>>>
>>> --
>>> Simon Urli
>>> Software Engineer at XWiki SAS
>>> [hidden email]
>>> More about us at http://www.xwiki.com
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> [hidden email]
> More about us at http://www.xwiki.com

Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Prevent users from renaming/move pages with XClass definition

Simon Urli


On 10/17/18 10:37 AM, Vincent Massol wrote:

>
>
>> On 17 Oct 2018, at 10:31, Simon Urli <[hidden email]> wrote:
>>
>>
>>
>> On 10/17/18 10:22 AM, Vincent Massol wrote:
>>> Hi Simon,
>>>> On 17 Oct 2018, at 10:12, Simon Urli <[hidden email]> 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 <[hidden email]> 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/PreventUserFromXClassRefactoring
>>> 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%20Application/antispam-error.png
>>>   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.
>
> 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.

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.

>
>>
>>>>
>>>>> 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.
>
> ok, so try this: http://playground.xwiki.org/xwiki/bin/delete/MyPage/?confirm=1
>
> What happens?

Coming back to you after I checked :)

>
>>
>>>>>> 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.
>
> Yes, I agree.
>
> Thanks
> -Vincent
>
>>
>> Simon
>>> Thanks
>>> -Vincent
>>>>
>>>> Simon
>>>>> Thanks
>>>>> -Vincent
>>>>>>
>>>>>> Simon
>>>>>>
>>>>>>
>>>>>> On 9/26/18 10:27 AM, Simon Urli wrote:
>>>>>>> Hi everyone,
>>>>>>> ok trying to sum-up (I'm only talking about cases with XClass below, to simplify):
>>>>>>>    - according to Vincent, we should completely prevent simple users to copy/move/rename and only allow advanced users to do it after a warning
>>>>>>>    - according to Adel & Clément: preventing simple users will be useless as they can easily switch the advanced feature in their account
>>>>>>>    - according to Marius copying a page/app is not necessarily harmful compared to moving/renaming and we should manage it differently.
>>>>>>> I really don't know the practice of users on the field, but it looks to me that preventing simple users to do the action and telling them to ask an advanced user is actually a good trade-off:
>>>>>>>   1. it will warn users that they might be doing something wrong
>>>>>>>   2. it's not something completely blocking: either they ask for the admin/advanced user, or they know they can switch the advanced features by themselves, at their own risks
>>>>>>> Now maybe we can only do the warning for the "copy" action.
>>>>>>> WDYT?
>>>>>>> Simon
>>>>>>> On 9/25/18 11:36 AM, Vincent Massol wrote:
>>>>>>>> Hi Marius,
>>>>>>>>
>>>>>>>>> On 25 Sep 2018, at 11:34, Marius Dumitru Florea <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>> On Sun, Sep 23, 2018 at 11:12 AM Vincent Massol <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Simon,
>>>>>>>>>>
>>>>>>>>>>> On 21 Sep 2018, at 16:58, Simon Urli <[hidden email]> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 9/21/18 4:53 PM, Adel Atallah wrote:
>>>>>>>>>>>> +1 for the warning, but I would not forbid simple users from renaming
>>>>>>>>>>>> or moving pages but instead just hide the action (from the page menu).
>>>>>>>>>>>
>>>>>>>>>>> OK I should have written it: by "forbid" I meant:
>>>>>>>>>>>
>>>>>>>>>>> 1. Hide the action from the menu
>>>>>>>>>>> 2. Return an error message if the user try to access the
>>>>>>>>>> renaming/moving page (using forged URL)
>>>>>>>>>>>
>>>>>>>>>>> So you suggest we shouldn't do 2?
>>>>>>>>>>
>>>>>>>>>> So +1 to prevent/warn the user when doing a move/renaming
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> AND copy pages containing XClass definitions
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> FTR, copying a single page having an XClass definition is not dangerous (it
>>>>>>>>> won't break the application that owns the page), as it only creates a new
>>>>>>>>> class definition. Copying an entire application is not dangerous either.
>>>>>>>>> The copy won't work like the original application (this justifies a warning
>>>>>>>>> as it may fail the user expectations), but the original application will
>>>>>>>>> still work. Renaming or moving an application is dangerous as it breaks the
>>>>>>>>> application.
>>>>>>>>
>>>>>>>> Yes you’re correct. Unless the user does a copy + delete ;)
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> -Vincent
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> (the message should list all such pages).
>>>>>>>>>>
>>>>>>>>>> -1 to hide the action from the menu (if you’re talking about the
>>>>>>>>>> “Move/Rename” and “Copy" actions) because:
>>>>>>>>>> 1) you get to choose whether you move/rename/copy children after you click
>>>>>>>>>> the action
>>>>>>>>>> 2) even when the current page has an XClass, the user wouldn't understand
>>>>>>>>>> why he cannot see/click on the action. It’s better that he can do it but
>>>>>>>>>> get an error message, explaining why and telling him that to contact an
>>>>>>>>>> advanced users if he really needs to do it.
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> -Vincent
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Sep 21, 2018 at 4:44 PM Simon Urli <[hidden email]>
>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>
>>>>>>>>>>>>> users might currently break their AWM application by renaming/moving
>>>>>>>>>>>>> pages containing XClass definition.
>>>>>>>>>>>>>
>>>>>>>>>>>>> We need a proper refactoring operation to be able to properly do such
>>>>>>>>>>>>> move/rename. But this feature might take a while to be completely
>>>>>>>>>>>>> available.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In the meantime I propose that we prevent users from renaming/moving
>>>>>>>>>>>>> pages containing XClass.
>>>>>>>>>>>>>
>>>>>>>>>>>>> What I propose is the following:
>>>>>>>>>>>>>    - Forbid completely *simple users* to rename/move pages containing
>>>>>>>>>> XClass
>>>>>>>>>>>>>    - Display a warning to *advanced users* when they perform such
>>>>>>>>>>>>> operation: the same kind of warning we already have when performing
>>>>>>>>>> edit
>>>>>>>>>>>>> on XWiki pages
>>>>>>>>>>>>>
>>>>>>>>>>>>> WDYT?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Simon
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Simon Urli
>>>>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> Simon Urli
>>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>>> [hidden email]
>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Simon Urli
>>>>>> Software Engineer at XWiki SAS
>>>>>> [hidden email]
>>>>>> More about us at http://www.xwiki.com
>>>>
>>>> --
>>>> Simon Urli
>>>> Software Engineer at XWiki SAS
>>>> [hidden email]
>>>> More about us at http://www.xwiki.com
>>
>> --
>> Simon Urli
>> Software Engineer at XWiki SAS
>> [hidden email]
>> More about us at http://www.xwiki.com
>

--
Simon Urli
Software Engineer at XWiki SAS
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Prevent users from renaming/move pages with XClass definition

vmassol
Administrator


> On 17 Oct 2018, at 10:41, Simon Urli <[hidden email]> wrote:
>
>
>
> On 10/17/18 10:37 AM, Vincent Massol wrote:
>>> On 17 Oct 2018, at 10:31, Simon Urli <[hidden email]> wrote:
>>>
>>>
>>>
>>> On 10/17/18 10:22 AM, Vincent Massol wrote:
>>>> Hi Simon,
>>>>> On 17 Oct 2018, at 10:12, Simon Urli <[hidden email]> 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 <[hidden email]> 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/PreventUserFromXClassRefactoring
>>>> 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%20Application/antispam-error.png
>>>>  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.
>> 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.
>
> 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

>>>
>>>>>
>>>>>> 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.
>> ok, so try this: http://playground.xwiki.org/xwiki/bin/delete/MyPage/?confirm=1
>> What happens?
>
> Coming back to you after I checked :)
>>>
>>>>>>> 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.
>> Yes, I agree.
>> Thanks
>> -Vincent
>>>
>>> Simon
>>>> Thanks
>>>> -Vincent
>>>>>
>>>>> Simon
>>>>>> Thanks
>>>>>> -Vincent
>>>>>>>
>>>>>>> Simon
>>>>>>>
>>>>>>>
>>>>>>> On 9/26/18 10:27 AM, Simon Urli wrote:
>>>>>>>> Hi everyone,
>>>>>>>> ok trying to sum-up (I'm only talking about cases with XClass below, to simplify):
>>>>>>>>   - according to Vincent, we should completely prevent simple users to copy/move/rename and only allow advanced users to do it after a warning
>>>>>>>>   - according to Adel & Clément: preventing simple users will be useless as they can easily switch the advanced feature in their account
>>>>>>>>   - according to Marius copying a page/app is not necessarily harmful compared to moving/renaming and we should manage it differently.
>>>>>>>> I really don't know the practice of users on the field, but it looks to me that preventing simple users to do the action and telling them to ask an advanced user is actually a good trade-off:
>>>>>>>>  1. it will warn users that they might be doing something wrong
>>>>>>>>  2. it's not something completely blocking: either they ask for the admin/advanced user, or they know they can switch the advanced features by themselves, at their own risks
>>>>>>>> Now maybe we can only do the warning for the "copy" action.
>>>>>>>> WDYT?
>>>>>>>> Simon
>>>>>>>> On 9/25/18 11:36 AM, Vincent Massol wrote:
>>>>>>>>> Hi Marius,
>>>>>>>>>
>>>>>>>>>> On 25 Sep 2018, at 11:34, Marius Dumitru Florea <[hidden email]> wrote:
>>>>>>>>>>
>>>>>>>>>> On Sun, Sep 23, 2018 at 11:12 AM Vincent Massol <[hidden email]> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi Simon,
>>>>>>>>>>>
>>>>>>>>>>>> On 21 Sep 2018, at 16:58, Simon Urli <[hidden email]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 9/21/18 4:53 PM, Adel Atallah wrote:
>>>>>>>>>>>>> +1 for the warning, but I would not forbid simple users from renaming
>>>>>>>>>>>>> or moving pages but instead just hide the action (from the page menu).
>>>>>>>>>>>>
>>>>>>>>>>>> OK I should have written it: by "forbid" I meant:
>>>>>>>>>>>>
>>>>>>>>>>>> 1. Hide the action from the menu
>>>>>>>>>>>> 2. Return an error message if the user try to access the
>>>>>>>>>>> renaming/moving page (using forged URL)
>>>>>>>>>>>>
>>>>>>>>>>>> So you suggest we shouldn't do 2?
>>>>>>>>>>>
>>>>>>>>>>> So +1 to prevent/warn the user when doing a move/renaming
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> AND copy pages containing XClass definitions
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> FTR, copying a single page having an XClass definition is not dangerous (it
>>>>>>>>>> won't break the application that owns the page), as it only creates a new
>>>>>>>>>> class definition. Copying an entire application is not dangerous either.
>>>>>>>>>> The copy won't work like the original application (this justifies a warning
>>>>>>>>>> as it may fail the user expectations), but the original application will
>>>>>>>>>> still work. Renaming or moving an application is dangerous as it breaks the
>>>>>>>>>> application.
>>>>>>>>>
>>>>>>>>> Yes you’re correct. Unless the user does a copy + delete ;)
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> -Vincent
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> (the message should list all such pages).
>>>>>>>>>>>
>>>>>>>>>>> -1 to hide the action from the menu (if you’re talking about the
>>>>>>>>>>> “Move/Rename” and “Copy" actions) because:
>>>>>>>>>>> 1) you get to choose whether you move/rename/copy children after you click
>>>>>>>>>>> the action
>>>>>>>>>>> 2) even when the current page has an XClass, the user wouldn't understand
>>>>>>>>>>> why he cannot see/click on the action. It’s better that he can do it but
>>>>>>>>>>> get an error message, explaining why and telling him that to contact an
>>>>>>>>>>> advanced users if he really needs to do it.
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> -Vincent
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> On Fri, Sep 21, 2018 at 4:44 PM Simon Urli <[hidden email]>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> users might currently break their AWM application by renaming/moving
>>>>>>>>>>>>>> pages containing XClass definition.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> We need a proper refactoring operation to be able to properly do such
>>>>>>>>>>>>>> move/rename. But this feature might take a while to be completely
>>>>>>>>>>>>>> available.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> In the meantime I propose that we prevent users from renaming/moving
>>>>>>>>>>>>>> pages containing XClass.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> What I propose is the following:
>>>>>>>>>>>>>>   - Forbid completely *simple users* to rename/move pages containing
>>>>>>>>>>> XClass
>>>>>>>>>>>>>>   - Display a warning to *advanced users* when they perform such
>>>>>>>>>>>>>> operation: the same kind of warning we already have when performing
>>>>>>>>>>> edit
>>>>>>>>>>>>>> on XWiki pages
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> WDYT?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Simon
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> Simon Urli
>>>>>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>>>>>
>>>>>>>>>>>> --
>>>>>>>>>>>> Simon Urli
>>>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Simon Urli
>>>>>>> Software Engineer at XWiki SAS
>>>>>>> [hidden email]
>>>>>>> More about us at http://www.xwiki.com
>>>>>
>>>>> --
>>>>> Simon Urli
>>>>> Software Engineer at XWiki SAS
>>>>> [hidden email]
>>>>> More about us at http://www.xwiki.com
>>>
>>> --
>>> Simon Urli
>>> Software Engineer at XWiki SAS
>>> [hidden email]
>>> More about us at http://www.xwiki.com
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> [hidden email]
> More about us at http://www.xwiki.com

Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Prevent users from renaming/move pages with XClass definition

Simon Urli


On 10/17/18 10:43 AM, Vincent Massol wrote:

>
>
>> On 17 Oct 2018, at 10:41, Simon Urli <[hidden email]> wrote:
>>
>>
>>
>> On 10/17/18 10:37 AM, Vincent Massol wrote:
>>>> On 17 Oct 2018, at 10:31, Simon Urli <[hidden email]> wrote:
>>>>
>>>>
>>>>
>>>> On 10/17/18 10:22 AM, Vincent Massol wrote:
>>>>> Hi Simon,
>>>>>> On 17 Oct 2018, at 10:12, Simon Urli <[hidden email]> 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 <[hidden email]> 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/PreventUserFromXClassRefactoring
>>>>> 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%20Application/antispam-error.png
>>>>>   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.
>>> 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.
>>
>> 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?

No, to be precise here we are talking about a MoveJob which extends
AbstractEntityJobWithChecks (same for DeleteJob and RenameJob): when the
job starts a method runInternal() is called, which performs a check by
creating a DeletingEvent (as some documents might be deleted) and
propagating it to the observationManager.

Then it's catched by the listener and processed: only after this step,
the job will be really started.

>
> Thanks
> -Vincent
>
>>>>
>>>>>>
>>>>>>> 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.
>>> ok, so try this: http://playground.xwiki.org/xwiki/bin/delete/MyPage/?confirm=1
>>> What happens?

When doing:
localhost:8080/xwiki/bin/delete/Menu/?confirm=1

I first get a warning because of the secret token, and if I ask to
continue I get redirected to the delete page, so I got the warning as
for renaming/moving pages.

Now concerning scripts, I was wrong in my previous answer: we check in
the listener if the job is interactive or not, and if it's not
interactive we skip the listener.
Now we might improve this behaviour later.

Simon

>>
>> Coming back to you after I checked :)
>>>>
>>>>>>>> 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.
>>> Yes, I agree.
>>> Thanks
>>> -Vincent
>>>>
>>>> Simon
>>>>> Thanks
>>>>> -Vincent
>>>>>>
>>>>>> Simon
>>>>>>> Thanks
>>>>>>> -Vincent
>>>>>>>>
>>>>>>>> Simon
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/26/18 10:27 AM, Simon Urli wrote:
>>>>>>>>> Hi everyone,
>>>>>>>>> ok trying to sum-up (I'm only talking about cases with XClass below, to simplify):
>>>>>>>>>    - according to Vincent, we should completely prevent simple users to copy/move/rename and only allow advanced users to do it after a warning
>>>>>>>>>    - according to Adel & Clément: preventing simple users will be useless as they can easily switch the advanced feature in their account
>>>>>>>>>    - according to Marius copying a page/app is not necessarily harmful compared to moving/renaming and we should manage it differently.
>>>>>>>>> I really don't know the practice of users on the field, but it looks to me that preventing simple users to do the action and telling them to ask an advanced user is actually a good trade-off:
>>>>>>>>>   1. it will warn users that they might be doing something wrong
>>>>>>>>>   2. it's not something completely blocking: either they ask for the admin/advanced user, or they know they can switch the advanced features by themselves, at their own risks
>>>>>>>>> Now maybe we can only do the warning for the "copy" action.
>>>>>>>>> WDYT?
>>>>>>>>> Simon
>>>>>>>>> On 9/25/18 11:36 AM, Vincent Massol wrote:
>>>>>>>>>> Hi Marius,
>>>>>>>>>>
>>>>>>>>>>> On 25 Sep 2018, at 11:34, Marius Dumitru Florea <[hidden email]> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Sun, Sep 23, 2018 at 11:12 AM Vincent Massol <[hidden email]> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi Simon,
>>>>>>>>>>>>
>>>>>>>>>>>>> On 21 Sep 2018, at 16:58, Simon Urli <[hidden email]> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 9/21/18 4:53 PM, Adel Atallah wrote:
>>>>>>>>>>>>>> +1 for the warning, but I would not forbid simple users from renaming
>>>>>>>>>>>>>> or moving pages but instead just hide the action (from the page menu).
>>>>>>>>>>>>>
>>>>>>>>>>>>> OK I should have written it: by "forbid" I meant:
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. Hide the action from the menu
>>>>>>>>>>>>> 2. Return an error message if the user try to access the
>>>>>>>>>>>> renaming/moving page (using forged URL)
>>>>>>>>>>>>>
>>>>>>>>>>>>> So you suggest we shouldn't do 2?
>>>>>>>>>>>>
>>>>>>>>>>>> So +1 to prevent/warn the user when doing a move/renaming
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> AND copy pages containing XClass definitions
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> FTR, copying a single page having an XClass definition is not dangerous (it
>>>>>>>>>>> won't break the application that owns the page), as it only creates a new
>>>>>>>>>>> class definition. Copying an entire application is not dangerous either.
>>>>>>>>>>> The copy won't work like the original application (this justifies a warning
>>>>>>>>>>> as it may fail the user expectations), but the original application will
>>>>>>>>>>> still work. Renaming or moving an application is dangerous as it breaks the
>>>>>>>>>>> application.
>>>>>>>>>>
>>>>>>>>>> Yes you’re correct. Unless the user does a copy + delete ;)
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>> -Vincent
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> (the message should list all such pages).
>>>>>>>>>>>>
>>>>>>>>>>>> -1 to hide the action from the menu (if you’re talking about the
>>>>>>>>>>>> “Move/Rename” and “Copy" actions) because:
>>>>>>>>>>>> 1) you get to choose whether you move/rename/copy children after you click
>>>>>>>>>>>> the action
>>>>>>>>>>>> 2) even when the current page has an XClass, the user wouldn't understand
>>>>>>>>>>>> why he cannot see/click on the action. It’s better that he can do it but
>>>>>>>>>>>> get an error message, explaining why and telling him that to contact an
>>>>>>>>>>>> advanced users if he really needs to do it.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> -Vincent
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Fri, Sep 21, 2018 at 4:44 PM Simon Urli <[hidden email]>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> users might currently break their AWM application by renaming/moving
>>>>>>>>>>>>>>> pages containing XClass definition.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> We need a proper refactoring operation to be able to properly do such
>>>>>>>>>>>>>>> move/rename. But this feature might take a while to be completely
>>>>>>>>>>>>>>> available.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> In the meantime I propose that we prevent users from renaming/moving
>>>>>>>>>>>>>>> pages containing XClass.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> What I propose is the following:
>>>>>>>>>>>>>>>    - Forbid completely *simple users* to rename/move pages containing
>>>>>>>>>>>> XClass
>>>>>>>>>>>>>>>    - Display a warning to *advanced users* when they perform such
>>>>>>>>>>>>>>> operation: the same kind of warning we already have when performing
>>>>>>>>>>>> edit
>>>>>>>>>>>>>>> on XWiki pages
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> WDYT?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Simon
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>> Simon Urli
>>>>>>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>>>>>>
>>>>>>>>>>>>> --
>>>>>>>>>>>>> Simon Urli
>>>>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Simon Urli
>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>> [hidden email]
>>>>>>>> More about us at http://www.xwiki.com
>>>>>>
>>>>>> --
>>>>>> Simon Urli
>>>>>> Software Engineer at XWiki SAS
>>>>>> [hidden email]
>>>>>> More about us at http://www.xwiki.com
>>>>
>>>> --
>>>> Simon Urli
>>>> Software Engineer at XWiki SAS
>>>> [hidden email]
>>>> More about us at http://www.xwiki.com
>>
>> --
>> Simon Urli
>> Software Engineer at XWiki SAS
>> [hidden email]
>> More about us at http://www.xwiki.com
>

--
Simon Urli
Software Engineer at XWiki SAS
[hidden email]
More about us at http://www.xwiki.com
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Prevent users from renaming/move pages with XClass definition

vmassol
Administrator


> On 17 Oct 2018, at 11:08, Simon Urli <[hidden email]> wrote:

[snip]

>>>>>>> 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%20Application/antispam-error.png
>>>>>>  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.
>>>> 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.
>>>
>>> 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?
>
> No, to be precise here we are talking about a MoveJob which extends AbstractEntityJobWithChecks (same for DeleteJob and RenameJob): when the job starts a method runInternal() is called, which performs a check by creating a DeletingEvent (as some documents might be deleted) and propagating it to the observationManager.
>
> Then it's catched by the listener and processed: only after this step, the job will be really started.

ok cool, I don’t fully understand the implementation but I don’t need to ;) (I can check the code if I need).

All that’s important is that the user can cancel before the refactoring starts and that’s great!

>> Thanks
>> -Vincent
>>>>>
>>>>>>>
>>>>>>>> 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.
>>>> ok, so try this: http://playground.xwiki.org/xwiki/bin/delete/MyPage/?confirm=1
>>>> What happens?
>
> When doing:
> localhost:8080/xwiki/bin/delete/Menu/?confirm=1
>
> I first get a warning because of the secret token, and if I ask to continue I get redirected to the delete page, so I got the warning as for renaming/moving pages.

Ok, I was trying to find a UI way to not go through the rename/move warning but I don’t think that’s possible ;-)

What I wanted to show is that right now we don’t have a nice UI when an event is cancelled and it bubble up in the UI (which is what you can see on the Antispam app screensht).

But it doesn’t matter in this case since it always go through the job and thus we won’t have it and for scripts there’s no UI anyway so it’s normal to get an exception.

> Now concerning scripts, I was wrong in my previous answer: we check in the listener if the job is interactive or not, and if it's not interactive we skip the listener.
> Now we might improve this behaviour later.

Ok. It might be good to open a jira so that we don’t forget the use case.

Thanks
-Vincent

> Simon
>>>
>>> Coming back to you after I checked :)
>>>>>
>>>>>>>>> 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.
>>>> Yes, I agree.
>>>> Thanks
>>>> -Vincent
>>>>>
>>>>> Simon
>>>>>> Thanks
>>>>>> -Vincent
>>>>>>>
>>>>>>> Simon
>>>>>>>> Thanks
>>>>>>>> -Vincent
>>>>>>>>>
>>>>>>>>> Simon
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 9/26/18 10:27 AM, Simon Urli wrote:
>>>>>>>>>> Hi everyone,
>>>>>>>>>> ok trying to sum-up (I'm only talking about cases with XClass below, to simplify):
>>>>>>>>>>   - according to Vincent, we should completely prevent simple users to copy/move/rename and only allow advanced users to do it after a warning
>>>>>>>>>>   - according to Adel & Clément: preventing simple users will be useless as they can easily switch the advanced feature in their account
>>>>>>>>>>   - according to Marius copying a page/app is not necessarily harmful compared to moving/renaming and we should manage it differently.
>>>>>>>>>> I really don't know the practice of users on the field, but it looks to me that preventing simple users to do the action and telling them to ask an advanced user is actually a good trade-off:
>>>>>>>>>>  1. it will warn users that they might be doing something wrong
>>>>>>>>>>  2. it's not something completely blocking: either they ask for the admin/advanced user, or they know they can switch the advanced features by themselves, at their own risks
>>>>>>>>>> Now maybe we can only do the warning for the "copy" action.
>>>>>>>>>> WDYT?
>>>>>>>>>> Simon
>>>>>>>>>> On 9/25/18 11:36 AM, Vincent Massol wrote:
>>>>>>>>>>> Hi Marius,
>>>>>>>>>>>
>>>>>>>>>>>> On 25 Sep 2018, at 11:34, Marius Dumitru Florea <[hidden email]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Sun, Sep 23, 2018 at 11:12 AM Vincent Massol <[hidden email]> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Simon,
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 21 Sep 2018, at 16:58, Simon Urli <[hidden email]> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 9/21/18 4:53 PM, Adel Atallah wrote:
>>>>>>>>>>>>>>> +1 for the warning, but I would not forbid simple users from renaming
>>>>>>>>>>>>>>> or moving pages but instead just hide the action (from the page menu).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> OK I should have written it: by "forbid" I meant:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 1. Hide the action from the menu
>>>>>>>>>>>>>> 2. Return an error message if the user try to access the
>>>>>>>>>>>>> renaming/moving page (using forged URL)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So you suggest we shouldn't do 2?
>>>>>>>>>>>>>
>>>>>>>>>>>>> So +1 to prevent/warn the user when doing a move/renaming
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> AND copy pages containing XClass definitions
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> FTR, copying a single page having an XClass definition is not dangerous (it
>>>>>>>>>>>> won't break the application that owns the page), as it only creates a new
>>>>>>>>>>>> class definition. Copying an entire application is not dangerous either.
>>>>>>>>>>>> The copy won't work like the original application (this justifies a warning
>>>>>>>>>>>> as it may fail the user expectations), but the original application will
>>>>>>>>>>>> still work. Renaming or moving an application is dangerous as it breaks the
>>>>>>>>>>>> application.
>>>>>>>>>>>
>>>>>>>>>>> Yes you’re correct. Unless the user does a copy + delete ;)
>>>>>>>>>>>
>>>>>>>>>>> Thanks
>>>>>>>>>>> -Vincent
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> (the message should list all such pages).
>>>>>>>>>>>>>
>>>>>>>>>>>>> -1 to hide the action from the menu (if you’re talking about the
>>>>>>>>>>>>> “Move/Rename” and “Copy" actions) because:
>>>>>>>>>>>>> 1) you get to choose whether you move/rename/copy children after you click
>>>>>>>>>>>>> the action
>>>>>>>>>>>>> 2) even when the current page has an XClass, the user wouldn't understand
>>>>>>>>>>>>> why he cannot see/click on the action. It’s better that he can do it but
>>>>>>>>>>>>> get an error message, explaining why and telling him that to contact an
>>>>>>>>>>>>> advanced users if he really needs to do it.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>> -Vincent
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Fri, Sep 21, 2018 at 4:44 PM Simon Urli <[hidden email]>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> users might currently break their AWM application by renaming/moving
>>>>>>>>>>>>>>>> pages containing XClass definition.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> We need a proper refactoring operation to be able to properly do such
>>>>>>>>>>>>>>>> move/rename. But this feature might take a while to be completely
>>>>>>>>>>>>>>>> available.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In the meantime I propose that we prevent users from renaming/moving
>>>>>>>>>>>>>>>> pages containing XClass.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> What I propose is the following:
>>>>>>>>>>>>>>>>   - Forbid completely *simple users* to rename/move pages containing
>>>>>>>>>>>>> XClass
>>>>>>>>>>>>>>>>   - Display a warning to *advanced users* when they perform such
>>>>>>>>>>>>>>>> operation: the same kind of warning we already have when performing
>>>>>>>>>>>>> edit
>>>>>>>>>>>>>>>> on XWiki pages
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> WDYT?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Simon
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>> Simon Urli
>>>>>>>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --
>>>>>>>>>>>>>> Simon Urli
>>>>>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Simon Urli
>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>> [hidden email]
>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>
>>>>>>> --
>>>>>>> Simon Urli
>>>>>>> Software Engineer at XWiki SAS
>>>>>>> [hidden email]
>>>>>>> More about us at http://www.xwiki.com
>>>>>
>>>>> --
>>>>> Simon Urli
>>>>> Software Engineer at XWiki SAS
>>>>> [hidden email]
>>>>> More about us at http://www.xwiki.com
>>>
>>> --
>>> Simon Urli
>>> Software Engineer at XWiki SAS
>>> [hidden email]
>>> More about us at http://www.xwiki.com
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> [hidden email]
> More about us at http://www.xwiki.com

Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Prevent users from renaming/move pages with XClass definition

Thomas Mortagne
Administrator
On Wed, Oct 17, 2018 at 11:29 AM Vincent Massol <[hidden email]> wrote:

>
>
>
> > On 17 Oct 2018, at 11:08, Simon Urli <[hidden email]> wrote:
>
> [snip]
>
> >>>>>>> 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%20Application/antispam-error.png
> >>>>>>  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.
> >>>> 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.
> >>>
> >>> 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?
> >
> > No, to be precise here we are talking about a MoveJob which extends AbstractEntityJobWithChecks (same for DeleteJob and RenameJob): when the job starts a method runInternal() is called, which performs a check by creating a DeletingEvent (as some documents might be deleted) and propagating it to the observationManager.
> >
> > Then it's catched by the listener and processed: only after this step, the job will be really started.
>
> ok cool, I don’t fully understand the implementation but I don’t need to ;) (I can check the code if I need).

You could summarize it that way: there is two main steps in the
delete/rename/move jobs
* preparation: find out the "concerned pages" and associate each of
them with a boolean indicating if it should be taken into account or
not, at the end of this step an event is fired to give a chance to any
listener to modify that list (for example what Simon is doing in his
listener is injecting a question in the current job and modify the
list based on the answer)
* apply: the action is applied to all the pages in the list with true

>
> All that’s important is that the user can cancel before the refactoring starts and that’s great!
>
> >> Thanks
> >> -Vincent
> >>>>>
> >>>>>>>
> >>>>>>>> 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.
> >>>> ok, so try this: http://playground.xwiki.org/xwiki/bin/delete/MyPage/?confirm=1
> >>>> What happens?
> >
> > When doing:
> > localhost:8080/xwiki/bin/delete/Menu/?confirm=1
> >
> > I first get a warning because of the secret token, and if I ask to continue I get redirected to the delete page, so I got the warning as for renaming/moving pages.
>
> Ok, I was trying to find a UI way to not go through the rename/move warning but I don’t think that’s possible ;-)
>
> What I wanted to show is that right now we don’t have a nice UI when an event is cancelled and it bubble up in the UI (which is what you can see on the Antispam app screensht).
>
> But it doesn’t matter in this case since it always go through the job and thus we won’t have it and for scripts there’s no UI anyway so it’s normal to get an exception.
>
> > Now concerning scripts, I was wrong in my previous answer: we check in the listener if the job is interactive or not, and if it's not interactive we skip the listener.
> > Now we might improve this behaviour later.
>
> Ok. It might be good to open a jira so that we don’t forget the use case.
>
> Thanks
> -Vincent
>
> > Simon
> >>>
> >>> Coming back to you after I checked :)
> >>>>>
> >>>>>>>>> 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.
> >>>> Yes, I agree.
> >>>> Thanks
> >>>> -Vincent
> >>>>>
> >>>>> Simon
> >>>>>> Thanks
> >>>>>> -Vincent
> >>>>>>>
> >>>>>>> Simon
> >>>>>>>> Thanks
> >>>>>>>> -Vincent
> >>>>>>>>>
> >>>>>>>>> Simon
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On 9/26/18 10:27 AM, Simon Urli wrote:
> >>>>>>>>>> Hi everyone,
> >>>>>>>>>> ok trying to sum-up (I'm only talking about cases with XClass below, to simplify):
> >>>>>>>>>>   - according to Vincent, we should completely prevent simple users to copy/move/rename and only allow advanced users to do it after a warning
> >>>>>>>>>>   - according to Adel & Clément: preventing simple users will be useless as they can easily switch the advanced feature in their account
> >>>>>>>>>>   - according to Marius copying a page/app is not necessarily harmful compared to moving/renaming and we should manage it differently.
> >>>>>>>>>> I really don't know the practice of users on the field, but it looks to me that preventing simple users to do the action and telling them to ask an advanced user is actually a good trade-off:
> >>>>>>>>>>  1. it will warn users that they might be doing something wrong
> >>>>>>>>>>  2. it's not something completely blocking: either they ask for the admin/advanced user, or they know they can switch the advanced features by themselves, at their own risks
> >>>>>>>>>> Now maybe we can only do the warning for the "copy" action.
> >>>>>>>>>> WDYT?
> >>>>>>>>>> Simon
> >>>>>>>>>> On 9/25/18 11:36 AM, Vincent Massol wrote:
> >>>>>>>>>>> Hi Marius,
> >>>>>>>>>>>
> >>>>>>>>>>>> On 25 Sep 2018, at 11:34, Marius Dumitru Florea <[hidden email]> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>> On Sun, Sep 23, 2018 at 11:12 AM Vincent Massol <[hidden email]> wrote:
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Hi Simon,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> On 21 Sep 2018, at 16:58, Simon Urli <[hidden email]> wrote:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> On 9/21/18 4:53 PM, Adel Atallah wrote:
> >>>>>>>>>>>>>>> +1 for the warning, but I would not forbid simple users from renaming
> >>>>>>>>>>>>>>> or moving pages but instead just hide the action (from the page menu).
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> OK I should have written it: by "forbid" I meant:
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> 1. Hide the action from the menu
> >>>>>>>>>>>>>> 2. Return an error message if the user try to access the
> >>>>>>>>>>>>> renaming/moving page (using forged URL)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> So you suggest we shouldn't do 2?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> So +1 to prevent/warn the user when doing a move/renaming
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> AND copy pages containing XClass definitions
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> FTR, copying a single page having an XClass definition is not dangerous (it
> >>>>>>>>>>>> won't break the application that owns the page), as it only creates a new
> >>>>>>>>>>>> class definition. Copying an entire application is not dangerous either.
> >>>>>>>>>>>> The copy won't work like the original application (this justifies a warning
> >>>>>>>>>>>> as it may fail the user expectations), but the original application will
> >>>>>>>>>>>> still work. Renaming or moving an application is dangerous as it breaks the
> >>>>>>>>>>>> application.
> >>>>>>>>>>>
> >>>>>>>>>>> Yes you’re correct. Unless the user does a copy + delete ;)
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks
> >>>>>>>>>>> -Vincent
> >>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> (the message should list all such pages).
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> -1 to hide the action from the menu (if you’re talking about the
> >>>>>>>>>>>>> “Move/Rename” and “Copy" actions) because:
> >>>>>>>>>>>>> 1) you get to choose whether you move/rename/copy children after you click
> >>>>>>>>>>>>> the action
> >>>>>>>>>>>>> 2) even when the current page has an XClass, the user wouldn't understand
> >>>>>>>>>>>>> why he cannot see/click on the action. It’s better that he can do it but
> >>>>>>>>>>>>> get an error message, explaining why and telling him that to contact an
> >>>>>>>>>>>>> advanced users if he really needs to do it.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Thanks
> >>>>>>>>>>>>> -Vincent
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> On Fri, Sep 21, 2018 at 4:44 PM Simon Urli <[hidden email]>
> >>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Hi all,
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> users might currently break their AWM application by renaming/moving
> >>>>>>>>>>>>>>>> pages containing XClass definition.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> We need a proper refactoring operation to be able to properly do such
> >>>>>>>>>>>>>>>> move/rename. But this feature might take a while to be completely
> >>>>>>>>>>>>>>>> available.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> In the meantime I propose that we prevent users from renaming/moving
> >>>>>>>>>>>>>>>> pages containing XClass.
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> What I propose is the following:
> >>>>>>>>>>>>>>>>   - Forbid completely *simple users* to rename/move pages containing
> >>>>>>>>>>>>> XClass
> >>>>>>>>>>>>>>>>   - Display a warning to *advanced users* when they perform such
> >>>>>>>>>>>>>>>> operation: the same kind of warning we already have when performing
> >>>>>>>>>>>>> edit
> >>>>>>>>>>>>>>>> on XWiki pages
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> WDYT?
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Simon
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> --
> >>>>>>>>>>>>>>>> Simon Urli
> >>>>>>>>>>>>>>>> Software Engineer at XWiki SAS
> >>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>> More about us at http://www.xwiki.com
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> --
> >>>>>>>>>>>>>> Simon Urli
> >>>>>>>>>>>>>> Software Engineer at XWiki SAS
> >>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>> More about us at http://www.xwiki.com
> >>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>> Simon Urli
> >>>>>>>>> Software Engineer at XWiki SAS
> >>>>>>>>> [hidden email]
> >>>>>>>>> More about us at http://www.xwiki.com
> >>>>>>>
> >>>>>>> --
> >>>>>>> Simon Urli
> >>>>>>> Software Engineer at XWiki SAS
> >>>>>>> [hidden email]
> >>>>>>> More about us at http://www.xwiki.com
> >>>>>
> >>>>> --
> >>>>> Simon Urli
> >>>>> Software Engineer at XWiki SAS
> >>>>> [hidden email]
> >>>>> More about us at http://www.xwiki.com
> >>>
> >>> --
> >>> Simon Urli
> >>> Software Engineer at XWiki SAS
> >>> [hidden email]
> >>> More about us at http://www.xwiki.com
> >
> > --
> > Simon Urli
> > Software Engineer at XWiki SAS
> > [hidden email]
> > More about us at http://www.xwiki.com
>


--
Thomas Mortagne
Reply | Threaded
Open this post in threaded view
|

Re: [Proposal] Prevent users from renaming/move pages with XClass definition

vmassol
Administrator


> On 17 Oct 2018, at 11:37, Thomas Mortagne <[hidden email]> wrote:
>
> On Wed, Oct 17, 2018 at 11:29 AM Vincent Massol <[hidden email]> wrote:
>>
>>
>>
>>> On 17 Oct 2018, at 11:08, Simon Urli <[hidden email]> wrote:
>>
>> [snip]
>>
>>>>>>>>> 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%20Application/antispam-error.png
>>>>>>>> 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.
>>>>>> 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.
>>>>>
>>>>> 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?
>>>
>>> No, to be precise here we are talking about a MoveJob which extends AbstractEntityJobWithChecks (same for DeleteJob and RenameJob): when the job starts a method runInternal() is called, which performs a check by creating a DeletingEvent (as some documents might be deleted) and propagating it to the observationManager.
>>>
>>> Then it's catched by the listener and processed: only after this step, the job will be really started.
>>
>> ok cool, I don’t fully understand the implementation but I don’t need to ;) (I can check the code if I need).
>
> You could summarize it that way: there is two main steps in the
> delete/rename/move jobs
> * preparation: find out the "concerned pages" and associate each of
> them with a boolean indicating if it should be taken into account or
> not, at the end of this step an event is fired to give a chance to any
> listener to modify that list (for example what Simon is doing in his
> listener is injecting a question in the current job and modify the
> list based on the answer)
> * apply: the action is applied to all the pages in the list with true

Thanks for the explanations. It’s clear now.

Thanks
-Vincent

[snip]


12