On Wed, Oct 17, 2018 at 11:29 AM Vincent Massol <vinc...@massol.net> wrote: > > > > > On 17 Oct 2018, at 11:08, Simon Urli <simon.u...@xwiki.com> 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 > >>>>>>>>>>>> <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 > > > > -- > > Simon Urli > > Software Engineer at XWiki SAS > > simon.u...@xwiki.com > > More about us at http://www.xwiki.com > -- Thomas Mortagne
