Choice of the "Allow to skip the recycle bin" configuration location

Development
#1

In the context of XWIKI-17401, we need to introduce a new configuration defining if advanced users are allowed to choose during a page removal if the page is sent to the recycle bin or permanently deleted.

While another thread discuss the underlying implications of whether or not deleting a page is a refactoring operation (see this forum topic), I think we all agree that from a user standpoint, this configuration cannot be placed in a “Refactoring” section in the administration.
Consequently, we need to find a better location for this configuration.

Since this question is the last point blocking the merge of the PR related to this evolution, I’d like to reach an agreement in order to let it be integrated in the CI and have the time to fix the potential issue that could occur.

In conclusion, I am proposing the following configuration location of this configuration (Category / Section):

  • Location 1: Content / Pages
  • Location 2: Editing / Pages
  • Location 3: Content / Refactoring
  • Location 4: Editing / Refactoring
  • Location 5: Refactoring / Pages

Of course proposal for other locations are encouraged.

I propose to leave this vote open until Thursday 10.

Thanks

#2

Well sorry, but personally it wouldn’t chock me to have a “Refactoring” section. It’s just that this section shouldn’t be in the “Other” category IMO.

Now I really find “Pages” section name too vague. So personally I’d go to “Editing / Refactoring”: it’s a place where we could put later some other options like allowing to update backlinks on wiki farm or other things related to refactoring operations.

#3

I’ve added 3 new options (n° 3, 4 and 5).

#4

We need to be sure that the “allow to skip the recycle bin” configuration option is an option of the refactoring module and not an option of the model. I don’t recall where (I think in a github code review) but I commented about this already but I don’t remember other’s opinions so I’m restarting the discussion here.

I’m ambivalent on my side since the CRUD + move/rename operations on pages are APIs of the model right now and these APIs call the refactoring module internally to perform the action. This means that currently the main APIs for this are inside the model and thus it would make sense to have the config property also in the model configuration. Now, the refactoring module also exposes a Script Service (and indirectly through Jobs for Java, but not a very easy to use API). So basically we have several API locations to do the same things, hence the confusion for me and why it’s hard to decide where the config property should be located. More specifically we have 2 main locations for APIs:

  • When inside a script, it’s the refactoring module with the exposed Script Service
  • When in Java, it’s the model APIs (accessed through the XWiki and XWikiDocument classes)

I guess we have 2 options:

  • Consider that the config option should go in the same module as where the API is. For java API this is in the oldcore and thus CoreConfiguration (However, maybe should be in the model module since this is where any new model would go in the future).
  • Consider that the config option should go in the module where it’s used and that’s the refactoring module. This means that we don’t pass the config as a parameter of the various refactoring operations. I’m not sure about that and I think it would make more sense that the refactoring module has all its config passed to the methods as it’s already done for lots of existing configurations.

Thus I don’t believe that locating the config option in the refactoring module is a good idea at all. I much prefer to continue seeing the refactoring module as a utility module.

WDYT?

#5

From the architectural point of view:
In light of those arguments, I’m ok with placing the configuration in xwiki-platform-oldcore.

From a user experience point of view, the debate is not the same and @surli expressed above his arguments to see the page or configuration section named Refactoring.

WDYT is the best location for the configuration page in the administration?

#6

I remember having the same kind of discussion when working on rename and when needed to decide where to put the API and some other things. IMO the problem is that, as I understood it, the general direction is to stop putting things in old-core, so the various refactoring operations should go in Refactoring module, but at the same time old core still contains important API for the model, so we still want to have those operations available there.
I do think that we should discuss about the future direction we want for those specific APIs of the model in the future: to keep them there, or to move them, and if so, where.

Personally I would push for keeping simple direct “core” APIs in old core (so for example, a delete in old core would be a full delete only possible with PR), while complex operations should be all moved in Refactoring: a delete there could be a move to the recycle bin, or a real delete depending on the configuration.

IMO it would clarify both our usage and the future direction of those modules. And in that spirit, I still don’t think that this new configuration is a “core” configuration: it’s a configuration for a complex refactoring operation. The fact that it’s currently partly used in old core for now, is something that should be fixed by improving the module in the future.

#7

Yes, which is why I mentioned the model module too as an option.

It’s not that it still contains important API for the model, it is where the full model is (the model is the classes representing the wiki concepts: Document, Space, Wiki, Object, Classes, ec).

We already discussed that in the past and this is why we introduced a model module. This is the location for the new model in the future. The only problem is that it’s hard to move stuff there if the model API is not defined yet. Now, we did start defining the new Model API, it’s available here:

So what we need to do on this topic is a sprint to push this to master and start using the new API from the model module a bit everywhere and add new stuff in this module rather than in oldcore.

For me the refactoring module is probably a temporary module that is there because we didn’t have a proper model module and we wanted to extract stuff out of oldcore. We might want to remove it once we have a model module containing the model. Or we may want to keep it but just to do complex refactoring operations, i.e. not for doing the create/delete/move/rename of pages but to perform additional operations (find impacted pages and suggest modifications to them). The API could accept a document or document ref for ex and return an iterator of documents with changes to them (but not persisted), and let the model do the persistence, as it should.

Note: I’ve just realized that what I said in my previous reply above was not correct. I was thinking about something like:

        PermanentlyDeleteRequest deleteRequest =
            refactoring.getRequestFactory().createPermanentlyDeleteRequest(Collections.emptyList());
        deleteRequest.setInteractive(isAsync(context.getRequest()));
        deleteRequest.setCheckAuthorRights(false);
        deleteRequest.setAllowSkippingRecycleBin(true):

But this doesn’t make sense since this config is for the UI and not for the API :slight_smile:

So for me the situation is pretty clear: this config should go where the UI is. Right now the delete UI is a template located in the flamingo skin. This changes the discussion to: where should the UIs for create/delete/rename/move operations be located. IMO in a model UI module. That’s because I see the refactoring module as a helper module that is a helper to the model (i.e. it’s controlled and called by the model). BTW the script service inside the refactoring module should probably be deprecated in favor of a script service in the model module in the future once we have the model APIs there.

#8

Not sure I follow you there. For me the config was for both UI and API. And by API I mean the Script Service API for performing refactoring operations.

That’s pretty much how it currently works indeed, but IMO in the future it should be the contrary: the refactoring module should be the entry point for end-users and it should control the model.

We should never allow users to perform low-level operations on the model itself. Only developers with PR should be allowed to do that.

#9

Actually I agree with you here!

  • The model should do its model work, and CRUD operations are part of the model for sure. So page delete, creation, updates are in the model.
  • The refactoring module should do additional operations related only to refactoring (and page creation, deletion, updates are NOT refactorings). In other words the refactoring module should not do any DB actions, that’s the model that should do them (and use the store api to do them in practice).
  • So indeed the refactoring module should be the entry point for refactoring operations.
  • To summarize:
    • Basic CRUD operations: entry point = model script service
    • Complex refactorings: entry point = refactoring script service (calls the model to do the basic operations)

So this doesn’t help us decide where the Menus go… The problem is that the menus are both for simple operations and for refactoring operations.

Now, the “allow skipping the recycle bin” feature is definitely not a refactoring operation. It’s a basic configuration of page deletion and something that must be implemented in the model. Specifically the delete page code should have an IF to decide whether to send the doc to the bin or not.

WDYT?

#10

Yeah and for me that is specifically Refactoring responsibility. It’s an end-user action to delete a document and it ends up in the Refactoring module which will decide how to process this end-user action: should it really perform a delete (and then send it to the model) or should it performs actually a move to the recycle bin (and then use its own move API, or send it also to the module depending how we handle this).

So I’m still standing on my opinion here: this configuration should go on Refactoring.

#11

Ok that’s interesting. For me there’s no doubt that this is not a refactoring in the traditional usage for the word.

So it raises the interesting question of: What is the definition of the refactoring module and what is a refactoring in the xwiki sense.

We need a precise definition or we’ll never know way to put where. With your very large definition, anything that is not a single call to store.delete(doc) is a refactoring. For example deleting the xobjects or deleting the attachments would be a refactoring. And it doesn’t make much sense to me since it leads to a very anemic model. See https://www.martinfowler.com/bliki/AnemicDomainModel.html

I would like to propose another definition which I believe is non-ambiguous: We call an operation a refactoring, when it acts on more than one element of its type. In the case of a document refactoring, it’s when the action implies several documents (or sub-elements of a document, still the same type).

Some examples:

  • A move/delete refactoring will analyze the content of the document to check if there are other documents linking to the current one and modify them if need be.
  • I don’t consider that the deletion of xobjects when you delete a doc is a refactoring because it doesn’t act on another document or another element (the xobjects are part of the current document). So any operations that acts on the current element is not a refactoring to me.

Of course, this is restrictive since for example we wouldn’t put in the refactoring module things like:

  • Transform all the wiki word of a document into links (it wouldn’t match the definition since it acts on the same element, the document). Note that it doesn’t mean that because it’s not a “refactoring” in that module sense, that it has to go into the model. It can go elsewhere (in this example this is a transformation located in platform-rendering-transformations).

I honestly don’t know if this definition is good or not but we need a definition and a precise one if possible.

I really don’t get the logic that because it’s an end user action, it has to go in the refactoring module.

Since it’s an end user action it has to go in the UI logic obviously. And that logic will do:

  • If the “allow skipping the recycle bin” is set, then offer an option in the UI to display a “send to recycle bin option?”.
  • If the user has checked the “don’t send to the recycle bin”, then it’ll call the code to do it
  • If the user hasn’t checked it, it’ll call the code do it.

Note: it could decide to call the same code in the end if that code is made generic to support both cases but that doesn’t change the logic that it’s the UI logic that is in charge of deciding what to call (in term of MVC, it’s the Controller part - In our case the Controller part is in velocity in the pages or templates). This is why I was saying above that the “allow skipping the recycle bin” is a UI config parameter. What will be passed to downstream calls is not “allow skipping the recycle bin” but whether the document should be sent to the recycle bin or not.

#12

Vincent:

I wonder if you’re not mixing the “allow skipping the recycle bin” configuration option with the value decided by the user for a “skip the recycle bin” checkbox?
I don’t get why you don’t consider it a UI config option

Simon:

yeah but it’s this API purpose to compute this. The user could say “I want this doc to not been sent in recycle bin (i.e. to be fully deleted)”, but the admin could have set the config to not allow that. So it’s the API role to check if it’s allowed or not and to perform the proper action.

Vincent:

ah yes, for the SS API indeed, for permission checks when the user is a simple user (for advanced user we probably always allow it). But not for the java refactoring API. I understand now what you meant, thanks. So indeed, it’s not just used for the UI, it’s also used in some case for the permissions checks. So there are 2 different needs for this param:

  • it’s needed at the UI level definitely (so it cannot be a refactoring config parameter - unless the UI is moved to refactoring)
  • it’s needed at the refactoring level for permission checks (since refactoring depends on oldcore/model, it can get the configuration from the model) - it’s also possible to have a component for the permission check and that check implemented in model/oldcore to cleanly decouple it from the refactoring so that refactoring doesn’t need it at all.
#13

After discussing with Simon, we propose:

  • Commit it now in the refactoring module.
  • We still need to decide if it’s the right place and decide what we mean by refactoring. We need to do it before 12.8 is released (afterwards, it would be a bit late and we’d need to deprecate/move the configuration - for the API it’s ok since it’ll be Unstable for a while).
  • The Admin UI location, we propose Content > Delete which has the advantage of being more clear for the user + we won’t need to change if the move the property to the model for example.
#14

To summarize the discussion regarding the right place for the skip the recycle bin configuration (isRecycleBinSkippingActivated).

Three places has been proposed:

  • In xwiki-platform-oldcore:
    • Probably not the best place since the goal is to gradually extract things out of it.
  • In xwiki-platform-model:
    • In this case that implies to introduced two new modules xwiki-platform-model-api and xwiki-platform-model-ui
  • In xwiki-platform-refactoring-ui

The isRecycleBinSkippingActivated is currently used in two places:

  1. In xwiki-platform-refactoring-api to validate that the user is allowed to skip the recycle bin
  2. In delete.vm from xwiki-platform-oldcore to adapt the UI when the user is allowed to skip the recycle bin (using the RefactoringScriptService).

To decide between xwiki-platform-model and xwiki-platform-refactoring-ui is to decide whether the delete operation is a refactoring operation.

Starting for this definition of a refactoring.

I would argue that performing a document deletion involves the synchronized manipulations of multiple documents (for instance, translations or sub-documents) in a single transaction, that must be kept consistent.
In addition, is might be better to place the configuration in the refactoring module since this is where is is currently used.

WDYT?

#15

Reply here: https://matrix.to/#/!ikPtGZaGWtyblizzlR:matrix.xwiki.com/$160102087415191ztCny:matrix.xwiki.com?via=matrix.org&via=matrix.xwiki.com&via=t2bot.io

EDIT: Copying my conclusion from the chat:

So to conclude:

  1. These are hard questions and cannot be solved in 1 day
  2. They require someone to make a precise and consistent (ie includes all topics) proposal
  3. It’s not going to happen anytime soon as I don’t think anyone has the itch to do so. As a consquence what has been done will remain where it is now and will need to go through deprecation and uspporting several config names in the future if we move it

I don’t see any choice than not doing anything and leaving it as it is now :slight_smile: