XMPP does not detect when rooms are private and cannot be joined

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aleth, Assigned: abdelrhman)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
IB fails silently trying to join such rooms. Pidgin seems to show errors in this case like "407: Registration Required".
Possibly part of capability discovery.
(Assignee)

Comment 1

4 years ago
Assignee: nobody → a.ahmed1026
Status: NEW → ASSIGNED
Attachment #8574111 - Flags: review?(aleth)
(Reporter)

Comment 2

4 years ago
Comment on attachment 8574111 [details] [diff] [review]
rev 1 - discover error in conversation and show it

Review of attachment 8574111 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message should be more specific (you don't discover all conversation errors, after all).
Maybe "Handle errors on attempting to join private XMPP rooms"?

::: chat/locales/en-US/xmpp.properties
@@ +46,5 @@
>  conversation.error.joinFailed=Could not join: %S
>  #   These are displayed in a conversation as a system error message.
>  conversation.error.remoteServerNotFound=Could not reach the recipient's server
>  conversation.error.unknownError=Unknown error
> +conversation.error.notAuthorized=Registration Required

How about "Registration required: You are not authorized to join this room."

Please move the string under the joinFailed string.

::: chat/protocols/xmpp/xmpp.jsm
@@ +992,5 @@
>  
>        // The join failed.
>        if (muc.left && aStanza.attributes["type"] == "error") {
> +        let error = this.parseError(aStanza);
> +        let message = "";

You don't need the = "" as you have a default in the switch statement.

@@ +993,5 @@
>        // The join failed.
>        if (muc.left && aStanza.attributes["type"] == "error") {
> +        let error = this.parseError(aStanza);
> +        let message = "";
> +        if (error) {

You don't need to check this as you know that type == "error" before the parseError call, so it's always true.

@@ +996,5 @@
> +        let message = "";
> +        if (error) {
> +          switch (error.condition) {
> +            case "not-authorized":
> +              message = _("conversation.error.notAuthorized");

Let's call this string "joinFailedNotAuthorized" so we can make the string shown to the user more specific.

@@ +1005,5 @@
> +          }
> +        }
> +        else {
> +          message = _("conversation.error.joinFailed", muc.name);
> +          this.ERROR("Failed to join MUC: " + aStanza.convertToString());

Move this to the default: case and change it to
this.ERROR(`Failed to join MUC due to unhandled ${error.condition} error.`);
The point of this is to make it clear in the debug log what needs to be implemented to better take care of this if it happens.
Attachment #8574111 - Flags: review?(aleth) → review-
(Assignee)

Comment 3

4 years ago
Attachment #8574111 - Attachment is obsolete: true
Attachment #8574141 - Flags: review?(aleth)
(Reporter)

Comment 4

4 years ago
Comment on attachment 8574141 [details] [diff] [review]
rev 2 - Handle errors on attempting to join private XMPP rooms

Review of attachment 8574141 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/xmpp/xmpp.jsm
@@ +1007,1 @@
>                           {system: true, error: true});

Does this fit on one line now? (<= 80 characters)
(Assignee)

Comment 5

4 years ago
(In reply to aleth [:aleth] from comment #4)
> Comment on attachment 8574141 [details] [diff] [review]
> rev 2 - Handle errors on attempting to join private XMPP rooms
> 
> Review of attachment 8574141 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/protocols/xmpp/xmpp.jsm
> @@ +1007,1 @@
> >                           {system: true, error: true});
> 
> Does this fit on one line now? (<= 80 characters)

Yes, it fits.
Attachment #8574141 - Attachment is obsolete: true
Attachment #8574141 - Flags: review?(aleth)
Attachment #8574150 - Flags: review?(aleth)
(Reporter)

Comment 6

4 years ago
Comment on attachment 8574150 [details] [diff] [review]
rev 3 - Handle errors on attempting to join private XMPP rooms

Review of attachment 8574150 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good now!
Attachment #8574150 - Flags: review?(aleth) → review+
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
(Reporter)

Updated

4 years ago
Blocks: 955167
https://hg.mozilla.org/comm-central/rev/8b047ada221f

Thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.