Introduce the notion of async action in Live Data

Development
#1

Hello all,

Livetable has a notion of async action, allowing to perform declaratively the following steps:

  1. the user click on the action
  2. a request to the action’s url is called in the background (with a loading message)
  3. the success/failure status of the actions is presented to the user
  4. in case of success the table is reloaded

I propose to introduce the same notion for Live Data.

Note: the intent for now is to only cover the simple case where no special error handling is required, and where a single http request is enough (i.e., moving a page, where several steps are required, is not covered by this proposal)

Developer

The goal is to make it easy for developers to declare a action are asynchronous, by declaring four properties:

  1. async: true
  2. loadingMessage/successMessage/failureMessage: localized messages for the different steps of the async process. Optionally we could provide generic messages, but I’d don’t think that’s really great for users.
{
  "meta": {
    "actions": [
      {
        "id": "delete",
        // ...Other usual action descriptor properties here
        "async": true,
        "loadingMessage": "Deletion in progress.",
        "successMessage": "Page successfully deleted.",
        "failureMessage": "A problem occurred while deleting the page."
      }
    ]
  }
}

Implementation

You can see an implementation draft in this PR: XWIKI-21944: Allow live data action to be async by manuelleduc · Pull Request #2937 · xwiki/xwiki-platform · GitHub

Conclusion

WDYT?

#2

In general +1. What I’m wondering is if there is any way to specify that the action should be a POST request, potentially with some request body. Using GET requests for actions that modify anything on the server is a bad practice that, among other things, makes protecting against CSRF much harder, and we should stop encouraging this bad practice in XWiki.

1 Like
#3

Hi @mleduc,

Looks good. I’m wondering if we can’t make it a bit more generic, by preparing the path for other type of (more complex) actions. For instance instead of

"async": true

we could have maybe (taking also @MichaelHamann comment into account):

"type": "asyncGet"

WDYT?

Also, especially for delete action we may need a:

"confirmationMessage": "Are you sure you want to delete this page?"

Thanks,
Marius

#4

Indeed GET shouldn’t be the default. I guess we might want to allow it for legacy use cases, but it shouldn’t be the default,

Based on your feedbacks, an alternative design could be:

  • async is now an object, when the object is defined the action is considered as asynchronous. When the key is not defined, the action is not async
  • action is now explict, with no default value
  • confirmationMessage is an optional key, allowing to define a confirmation message for “dangerous” actions, as proposed by @mflorea
  • body is an optional key, allowing to define the body of the request as propose by @MichaelHamann Example:
{
  "meta": {
    "actions": [
      {
        "id": "delete",
        "async": {
          "action": "POST",
          "confirmationMessage": "Are you sure you want to delete the page?",
          "loadingMessage": "Deletion in progress.",
          "successMessage": "Page successfully deleted.",
          "failureMessage": "A problem occurred while deleting the page.",
          "body": ""
        }
      }
    ]
  }
}
#5

+1 with the latest version, now IMO all messages should only be translations keys.

#6

Definitely, I did not mention it, but messages are always translated server-side before being directly served in the locale on the users. Avoiding an extra request to fetch translations later on.

#7

The last version looks good, +1.

I’m just wondering if plain text really covers all use cases, or if the content of those messages should be some more generic HTML content.

#8

I get the value of supporting this. But, I don’t know I want to. Supporting something else than plain text opens the door to security issues if we don’t handle escaping well.
As I said, I’m aiming at supporting simple cases for now.
And, it’s always possible to fall back to a dedicated Javascript code for handling custom actions on LD buttons if the async option is too limited.

#9

OK, then I guess it’s good enough, yes.

#10

PR XWIKI-21944: Allow live data action to be async by manuelleduc · Pull Request #2937 · xwiki/xwiki-platform · GitHub updated accordingly (except for missing tests).

#11

I’m not sure if this would work as-is, I think for the body to work we would also need a content type. For example, to indicate that the content is JSON.

#12

Indeed, I guess we can safely make it a limitation must be a json body for a first version?
My thinking is that we don’t want to reproduce all the subtlety of the fetch api in the async config.
Here too, devs needing more control can always fall back to a custom handling.

#13

+1 for the updated proposal.

Thanks,
Marius

#14

The PR is now ready for review XWIKI-21944: Allow live data action to be async by manuelleduc · Pull Request #2937 · xwiki/xwiki-platform · GitHub

In addition to what I proposed above, I introduced a “headers” parameter, which can take an optional map of HTTP headers. When not defined, the default parameters of the fetch API are used.

You can find an example of use in the docker test XWIKI-21944: Allow live data action to be async by manuelleduc · Pull Request #2937 · xwiki/xwiki-platform · GitHub