Remove hacks to "hide" technical events notifications when grouping them

Development
,
#1

Hi everyone,

this is yet another proposal related to notifications. I’d like to propose another technical behavioural change related to how events are grouped by default to form composite events, and then to be displayed.

Right now the default algorithm for grouping events is based on 3 criteria:

  1. the type of the events
  2. the document involved in the event
  3. the groupId of the event

The two first criteria are not difficult to understand: we group events that are of similar type and targeting same document basically. The third one is more complicated, and that’s the one I’d like to get rid of: right now we might group recordable events that are not of the same type, if they have been triggered during the same activity.
For me this is the hack for hiding the fact that we are wrongly triggering technical recordable events not related to the actual activity we’re doing.

For example, if you’re adding an annotation to a document, right now 3 recordable events (aka notifications events) are triggered:

  1. first, the update recordable event
  2. second, a comment recordable event (because annotation are stored as comment)
  3. third, an annotation recordable event

Right now, the default algorithm consists in grouping those 3 events, even if their type is unrelated. Worst thing is that since we’re mixing unrelated events in the CompositeEvent class, we also need a hack for checking the type to take the latest type and ignore the “update”/“create” technical types (see: CompositeEvent. Also it complexify the code for grouping the events, see the dedicated condition.
And finally we also need to handle this at template levels, since else we’d show all events in the details even the unrelated ones, see the macro we have

So my proposal would be to:

  1. Remove all those hacks and condition, so the default grouping algorithm would be only based on type and document
  2. Improve our listeners in XS to only trigger the RecordableEvents when the activity related to the event is done: i.e. don’t trigger an update recordable, if it’s for an actual comment or annotation, by checking in the listener the context of the event, and probably by creating some dedicated folded events

The pro of my proposal is that it would simplify a lot maintaining notifications as we’ll stop having weird exceptions, and it also simplify writing/customizing templates.
The cons is that there will be a change in behaviour that will impact extensions: I expect that some extensions do rely on that behaviour for their notifications, because they don’t use folded events and they update pages.

It’s a big cons, I don’t have a magical solution for it, except reviewing extension that uses custom notifications (AFAIR, there’s not that many), so we could even start by doing that.

WDYT?

#2

I agree with the need to remove the hack you are mentioning, but not with the proposed solution. We already have a way to hide technical events which are just implementation details of a user action (like posting an annotation): from what I understand, all we need to making the annotation event folded and the comment/document events are going to be ignored (by the way the comment event itself most probably need to be a folded event too so that posting a comment ignore document modifications).

#3

We’re on the same page, my idea is to use the folded event mechanism for that, and indeed right now the DocumentEventListener does ignore folded events. So all we need to do is indeed to create the appropriate begin/end events.

Also the annotation event here was an example: the plan is to review events in XS to do ensure we’re not missing anything, so yes to do the same also for comment. But we might have other events to change too.

#4

I’m sure we do, yes. Note that it’s often more about adding new events than changing an event : Begin/EndFoldEvent can only be sent by an API which knows what it’s doing, but we still need to receive a CommentAddedEvent event when someone adds a comment directly through the object editor.

#5

So a quick check on xwiki-contrib shows that we have the following list of extensions using notifications:

  • blog (using notifcations defined through xobject and that’s the only one using this mechanism here): needs to be reviewed
  • activitypub: needs to be reviewed
  • security-adivsory: needs to be reviewed
  • publication-workflow-review: needs to be reviewed
  • replication: not impacted according to @tmortagne
  • notification-word: at early stage of dev anyway, not even released yet
  • changerequest: most events are already folded, but needs to be reviewed
#6

Have you thought about the idea of moving all the hackish code into legacy and introducing a component role to decide with impl to use (the legacy one or the new one) so that admins could decide to go back to the old behaviour if they’re using custom code or extensions using that old behavior?

#7

A possibility in 15.5 is to provide a grouping strategy specifically for that. But that only concerns the java code, the idea here is also to allow removing hackish code from templates, and to ensure that default behaviour doesn’t contain anymore those hacks.

[Edit: And note that the proposal only target 15.5.x+]

#8

It’s also possible to have legacy templates (they can be located in the legacy jar).

#9

I opened Loading... to proceed on this.