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