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