> On 17 Oct 2018, at 10:31, Simon Urli <simon.u...@xwiki.com> wrote:
>
>
>
> On 10/17/18 10:22 AM, Vincent Massol wrote:
>> Hi Simon,
>>> On 17 Oct 2018, at 10:12, Simon Urli <simon.u...@xwiki.com> wrote:
>>>
>>> Hi Vincent and all,
>>>
>>> On 10/17/18 9:41 AM, Vincent Massol wrote:
>>>> Hi Simon,
>>>>> On 16 Oct 2018, at 17:43, Simon Urli <simon.u...@xwiki.com> wrote:
>>>>>
>>>>> Hello everyone,
>>>>>
>>>>> I'm coming back on this proposal as the work is going on, to basically
>>>>> propose to dropping the warning on copy action.
>>>>>
>>>>> I try to sum up why in the following.
>>>>>
>>>>> When implementing the proposal, I was adviced to use an event listener,
>>>>> observing the deleting event for informing the user if he were doing a
>>>>> refactoring on a document containing an XClass.
>>>>> This work is already implemented and working for Moving/Renaming pages
>>>>> (which involve deleting the old page) and of course deleting.
>>>> The nice part about the listener is that it works for all use cases:
>>>> * rename/move from the UI
>>>> * rename/move from scripts
>>>> * etc
>>>
>>> To ease the discussion I just created a design page with some screenshot of
>>> my current work. Then you can see what it looks like for the user:
>>> https://design.xwiki.org/xwiki/bin/view/Proposal/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
>>>>>>>> <mariusdumitru.flo...@xwiki.com> wrote:
>>>>>>>>
>>>>>>>> On Sun, Sep 23, 2018 at 11:12 AM Vincent Massol <vinc...@massol.net>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Hi Simon,
>>>>>>>>>
>>>>>>>>>> On 21 Sep 2018, at 16:58, Simon Urli <simon.u...@xwiki.com> 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 <simon.u...@xwiki.com>
>>>>>>>>> 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
>>>>>>>>>>>> simon.u...@xwiki.com
>>>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Simon Urli
>>>>>>>>>> Software Engineer at XWiki SAS
>>>>>>>>>> simon.u...@xwiki.com
>>>>>>>>>> More about us at http://www.xwiki.com
>>>>>>>
>>>>>
>>>>> --
>>>>> Simon Urli
>>>>> Software Engineer at XWiki SAS
>>>>> simon.u...@xwiki.com
>>>>> More about us at http://www.xwiki.com
>>>
>>> --
>>> Simon Urli
>>> Software Engineer at XWiki SAS
>>> simon.u...@xwiki.com
>>> More about us at http://www.xwiki.com
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> simon.u...@xwiki.com
> More about us at http://www.xwiki.com
