> On 17 Oct 2018, at 10:41, Simon Urli <simon.u...@xwiki.com> wrote:
>
>
>
> On 10/17/18 10:37 AM, Vincent Massol wrote:
>>> 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.
>
> 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
>>>>>>>>>> <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
>
> --
> Simon Urli
> Software Engineer at XWiki SAS
> simon.u...@xwiki.com
> More about us at http://www.xwiki.com
