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

#473: Error message processing #474

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

guusdk
Copy link
Contributor

@guusdk guusdk commented Jun 16, 2016

This is a fix for #473 (further context in https://groups.google.com/forum/#!topic/candy-chat/plwovsjyx38 )

Displaying an error message should not depend on the XMPP error to contain a text element (as such element is optional).

This modification removes the 'info' type from the message that is displayed, to prevent XHTML processing problems. There might be cleaner fix for this, but that's beyond my javascript foo.

I'm wondering if this bit of the code behaves differently, when executed in the context of regular rooms, versus one-on-one/private chats. I've tested with the latter (using the muc#roomconfig_allowpm chatroom configuration option).

This is a fix for candy-chat#473

Displaying an error message should not depend on the XMPP error to
contain a text element (as such element is optional).
@benlangfeld
Copy link
Member

Excellent, thank you. Would you be able to add a test for this by any chance?

@guusdk
Copy link
Contributor Author

guusdk commented Jun 16, 2016

I'd love to, but I have already been stretching my javascript powers as-they-are. It might be best to have someone else do further modifications. I'm available for feedback in http://www.igniterealtime.org/support/group_chat.jsp - which is also the instance that suffers from the problem. Feel free to ping me there.

@guusdk
Copy link
Contributor Author

guusdk commented Jun 27, 2016

@benlangfeld could I tempt you to have a look at this? The old code is causing a lot of messages from being dropped without users being aware of that, which causes heaps of confusion. I'd love to roll out a fix, but I need some help to massage the bits and bytes in the right spot...

@benlangfeld
Copy link
Member

Unfortunately I'm very heavily loaded right now. I don't expect to get time to look at this until week beginning 11th July. If someone can figure out convincing test coverage before that, I could find the time to merge it.

@guusdk
Copy link
Contributor Author

guusdk commented Jul 12, 2016

So, 11th of July happened... :) _wink_wink_hint_hint*

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

Successfully merging this pull request may close these issues.

2 participants