Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Messages both in local and servers archive appear twice #467

Closed
1st8 opened this issue Sep 8, 2015 · 8 comments
Closed

Messages both in local and servers archive appear twice #467

1st8 opened this issue Sep 8, 2015 · 8 comments

Comments

@1st8
Copy link
Contributor

1st8 commented Sep 8, 2015

Hi,

when using the new MAM feature, messages stored both in local and server archive appear twice.
I remember vaguely that this is a known issue, but I couldn't find one here.

Do you have a possible solution in mind that I could implement and submit as PR?
I would otherwise try to merge the messages from the two archives by ID, if thats possible, will take a look later.
Or turning off the local archive if MAM is active would be easy too, wouldn't it?

Cheers!

@jcbrand
Copy link
Member

jcbrand commented Sep 8, 2015

There have been issues in the past concerning duplicate messages, but that was before MAM support was added, so I doubt that they are related.

This case should already be covered in the MAM code. There must be something special or specific about your usecase.

Currently I have no idea why this would happen, so unfortunately no suggestions for you.

@1st8
Copy link
Contributor Author

1st8 commented Sep 8, 2015

Ok I'll debug it and report back here, thanks.

@1st8
Copy link
Contributor Author

1st8 commented Sep 30, 2015

Looks like it can be fixed by explicitly setting message_archiving: 'always', although this is already configured on the server.

@1st8 1st8 closed this as completed Sep 30, 2015
@1st8
Copy link
Contributor Author

1st8 commented Sep 30, 2015

Whoops, still an issue.

ChatBoxView#showMessage is called once with each param:

archive_id: "c15ceae9-88f0-44d2-ab9b-e53f80558a23"
chat_state: "active"
delayed: true
fullname: "Oliver"
id: "10990045-0037-21b5-f9d5-a0cbddf58982"
message: "Kannst du das Problem mit der Nachrichtendopplung noch reproduzieren? Ich schaffe es leider gerade nicht"
msgid: "1443604483090"
sender: "me"
time: "2015-09-30T09:14:43Z"
fullname: "Christoph Geschwind"
id: "a9cf96f7-96d9-50c6-0294-a9d11e831d02"
message: "Kannst du das Problem mit der Nachrichtendopplung noch reproduzieren? Ich schaffe es leider gerade nicht"
sender: "me"
time: "2015-09-30T11:14:43+02:00"

The message has been sent once from me to Oliver.
Can you already tell something from the different format of the params?

fullname is different, time is once UTC and once with timezone, msgid and archive_id are only present in one call, id is different for both (probably stanza id, one from the real message, the other from the server archive response?)

I'll continue to look into it, the error occured after continuing a disconnected bosh session.

@1st8 1st8 reopened this Sep 30, 2015
1st8 pushed a commit to 1st8/converse.js that referenced this issue Sep 30, 2015
@1st8
Copy link
Contributor Author

1st8 commented Sep 30, 2015

Could you take a look at the referenced commit above? (1st8@a06f0d2)

I think the problem was, that outgoing messages had their msgid assigned when they were sent over the wire, but not when they were put into the storage, which happens before.
The commit fixes this by moving msgid generation into the model, thus ensuring that every Message object has a unique msgid.
The changed sendMessage method now expects a Message object and takes the message and msgid from it.

For some extra information, I added the msgid as data attribute on the rendered chat message.
Might be useful for debugging purposes or getting the Message object from its DOM representation.

Please let me know what you think, I will test the changes with my colleagues in the meantime.

@jcbrand
Copy link
Member

jcbrand commented Oct 2, 2015

Hi @1st8, thanks for debugging this issue!

Your change looks good. If you're satisfied on your side that it works, please make a pull request.

@1st8
Copy link
Contributor Author

1st8 commented Oct 2, 2015

Hey @jcbrand

I would rather not rush it and give it a few more days of internal production use next week, pull request will be prepared after that, okay?

Cheers!

@jcbrand
Copy link
Member

jcbrand commented Oct 5, 2015

No problem.

1st8 pushed a commit to 1st8/converse.js that referenced this issue Oct 12, 2015
1st8 pushed a commit to 1st8/converse.js that referenced this issue Oct 12, 2015
jcbrand added a commit that referenced this issue Oct 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants