Save messages without body (XEP-0333, XEP-0184, XEP-0085,...)

[mightymop]

fix #166
close #130

enable/disable feature:

Output on admin console and pdf:

[mightymop]

When will this PR be reviewed / merged?

[mightymop]

PR is now up to date with master

[guusdk]

Rebased to the latest master again, and added a changelog entry.

[guusdk]

I've got a number of concerns regarding this:

• the placement of the checkbox on the admin console makes me wonder if the chat marker setting applies to both types of messages (one-on-one and MUC).
• We might want something similar for other message stanzas that don't have a body (eg: XEP-0184: Message Delivery Receipts, XEP-0085: Chat State Notifications)
• The code to distill the type of marker from a message seems overly complex. The end result is hardly used.
• The newly added field org.jivesoftware.openfire.archive.ConversationEvent#marker doesn't seem to be used. Does that mean that this functionality is broken in a cluster?
• and, somewhat trivial: the MARKERTYPE enum shouldn't have an all-caps name.
• the second non-null check in if (message.getBody() != null||(message.getBody()==null&&this.conversationManager.isChatmarkerArchivingEnabled())) is unneeded
• the naming of ConversationEvent.chatmarkerMessageReceived suggests that this is an event handler, but it's not. This duplicates a similarly misnamed, pre-existing org.jivesoftware.openfire.archive.ConversationEvent#chatMessageReceived though.
• I'm not often one to comment on whitespace, but the (lack of) indentation of the four lines in this if-statement is very confusing:

if (markertype!=ChatMarker.MARKERTYPE.NONE)
{
    eventsQueue.addChatEvent(conversationManager.getConversationKey(sender, receiver),
    ConversationEvent.chatmarkerMessageReceived(sender, receiver,markertype,
    conversationManager.isMessageArchivingEnabled() ? message.toXML() : null,
    new Date()));
}

My suggestion would be to redesign this functionality, to become something like: "log message stanzas without bodies if it includes metadata for:" (and then offer a checkbox for all types of child elements that we want to support).

[mightymop]

yes i have stumbled about an issue in converse js (missing body in retraction message) which would also applies to your concerns... i am going to redesign this PR in the next days :)

[mightymop]

Okay i have refactored the whole PR... please review it :)

[mightymop]

I've got a number of concerns regarding this:

• the placement of the checkbox on the admin console makes me wonder if the chat marker setting applies to both types of messages (one-on-one and MUC).
• We might want something similar for other message stanzas that don't have a body (eg: XEP-0184: Message Delivery Receipts, XEP-0085: Chat State Notifications)
• The code to distill the type of marker from a message seems overly complex. The end result is hardly used.
• The newly added field org.jivesoftware.openfire.archive.ConversationEvent#marker doesn't seem to be used. Does that mean that this functionality is broken in a cluster?
• and, somewhat trivial: the MARKERTYPE enum shouldn't have an all-caps name.
• the second non-null check in if (message.getBody() != null||(message.getBody()==null&&this.conversationManager.isChatmarkerArchivingEnabled())) is unneeded
• the naming of ConversationEvent.chatmarkerMessageReceived suggests that this is an event handler, but it's not. This duplicates a similarly misnamed, pre-existing org.jivesoftware.openfire.archive.ConversationEvent#chatMessageReceived though.
• I'm not often one to comment on whitespace, but the (lack of) indentation of the four lines in this if-statement is very confusing:

if (markertype!=ChatMarker.MARKERTYPE.NONE)
{
    eventsQueue.addChatEvent(conversationManager.getConversationKey(sender, receiver),
    ConversationEvent.chatmarkerMessageReceived(sender, receiver,markertype,
    conversationManager.isMessageArchivingEnabled() ? message.toXML() : null,
    new Date()));
}

My suggestion would be to redesign this functionality, to become something like: "log message stanzas without bodies if it includes metadata for:" (and then offer a checkbox for all types of child elements that we want to support).

review plz :)

[Neustradamus]

@mightymop: One year after your changes, have you received a review from @guusdk?

[mightymop]

@mightymop: One year after your changes, have you received a review from @guusdk?

nope

[guusdk]

I have rebased this to the current main branch head.

[guusdk]

This bitmasking implementation is overly complex (it is hard to reason over). I don't think it is doing what it intends to do: the emptyMessageType of the stanza is compared with just one bitmask value.

[guusdk]

This complex code is duplicated in four places. That will make maintaining this code more difficult.

[guusdk]

I've fixed and refactored this.

[guusdk]

I identified at least one bug, and several inconsistencies. I've committed a number of changes that each address some concerns that I had. The current state of the code is untested. I particularly worry how this behaves in a cluster.

Additional thought: maybe we should support for XEP-0334: Message Processing Hints. Server-side rules are generally good, but they aren't discoverable and may lag behind. If a new XEP is written tomorrow and we do/don't want that in MAM, hints can help with that. On the other hand, I'm wondering if having some clients having their empty-bodied messages stored in MAM, is introducing more confusion than having either all or none of them do that. That probably only applies to MUC archives though.

[guusdk]

I'm now noticing more code duplication, in the sense that most of the configuration is duplicated for MUC vs. non-MUC messages. That also makes me wonder if my previous refactoring has taken that into account correctly.

In all, I'm disliking the over-complication of this solution: both in the implementation, as well as the UI options that are presented to the end-user.