1236133 - Disable facebook prpl

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[aleth [:aleth]]

Facebook's XMPP gateway, long officially deprecated, doesn't appear to work anymore.

We should probably disable the prpl for 45 (and possibly for 38.6).

Ideally there'd be an explanatory error message in the account manager for existing accounts, but we've missed the string freeze. Not sure if this is serious enough to attempt to work around that.

Comment 1

[Kent James (:rkent)]

If the gateway just stopped working, it seems to me that users are already not able to use this feature. So while we should disable it, a relnote should be enough without needing any late strings.

Comment 2

[Patrick Cloke [:clokep]]

rkent: If we remove the "Facebook Chat" protocol type then it just shows up as an "Unknown protocol" account, which will confuse users that something disappeared. That's why aleth is asking about strings.

So yes, the gateway stopped working, users are not able to use the feature but it still tries to connect and gives an error saying the password is wrong. This is confusing to users.

Comment 3

[Kent James (:rkent)]

True I don't really understand the issues here very well, but I assume that aleth added me to the CC list to get an opinion on strings, so I gave the best I could. Is anyone really pushing for a late string change for this?

Comment 4

[Patrick Cloke [:clokep]]

We discussed this briefly on IRC and it seems the options are:
* Remove the Facebook prpl which will give an awkward error message about TB not knowing what prpl-facebook is.
* Land a (really) late string
* Try to steal a string from somewhere else and immediately error out. (https://dxr.mozilla.org/comm-central/source/chat/locales/en-US/irc.properties#161 and https://dxr.mozilla.org/comm-central/source/chat/locales/en-US/xmpp.properties#22 were suggested)

None of these seem to be good solutions.

Comment 5

[Florian Quèze [:florian]]

(In reply to Patrick Cloke [:clokep] from comment #4)
> We discussed this briefly on IRC and it seems the options are:
> * Remove the Facebook prpl which will give an awkward error message about TB
> not knowing what prpl-facebook is.
> * Land a (really) late string
> * Try to steal a string from somewhere else and immediately error out.
> (https://dxr.mozilla.org/comm-central/source/chat/locales/en-US/irc.
> properties#161 and
> https://dxr.mozilla.org/comm-central/source/chat/locales/en-US/xmpp.
> properties#22 were suggested)
> 
> None of these seem to be good solutions.

Right, there doesn't seem to be any good solution. You forgot to list the solution "0. Do nothing and let the current prpl fail to connect with a misleading error message."

I think solution 1 (remove the prpl and let it fail with a cryptic error message) is about as bad as 0, just with more effort.

I don't know how feasible solution 2 is. Obviously that would be better for users.

Solution 3 seems a reasonable compromise. Eg. with the "%S is temporarily unavailable." string, with %S replaced with "Facebook Chat" (which we already have from the existing prpl), and a more descriptive non-localized message ERROR()'ed to the Error Console.

Comment 6

[aleth [:aleth]]

Attachment: Screen Shot.png

This is what happens if you just remove the prpl. I guess that's not as good as "Facebook temporarily unavailable"?

Comment 7

[aleth [:aleth]]

Attachment: Disable facebook prpl

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8713808 [details] [diff] [review]
Disable facebook prpl

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

::: chat/chat-prefs.js
@@ +82,5 @@
>  pref("chat.irc.automaticList", true);
>  // Disable Skype until it can be tested further.
>  pref("chat.prpls.prpl-skype.disable", true);
> +// Disable Facebook as the XMPP gateway no longer exists.
> +pref("chat.prpls.prpl-facebook.disable", true);

Just to remember how this works, it will show up only for users with that have that account but not in the wizard?

::: chat/protocols/facebook/facebook.js
@@ +22,5 @@
>  FacebookAccount.prototype = {
>    __proto__: XMPPAccountPrototype,
>    get canJoinChat() { return false; },
>    connect: function() {
> +    this.WARN("Facebook officially deprecated its XMPP gateway on April 30th, " +

Might make more sense to point to this bug so we can update it, etc. if we need to.

::: im/locales/en-US/chrome/instantbird/accountWizard.properties
@@ +17,4 @@
>  topProtocol.prpl-twitter.description=Stay up to date with your Twitter timeline
>  topProtocol.prpl-aim.description=Chat with your buddies on AOL Instant Messenger
>  topProtocol.prpl-yahoo.description=Chat with friends using Yahoo! Messenger
> +topProtocol.prpl-irc.description=Join IRC chat rooms

This would be super late strings, right? Or are you thinking it's "OK" because these are separate per locale?

Comment 9

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #8)
> > +// Disable Facebook as the XMPP gateway no longer exists.
> > +pref("chat.prpls.prpl-facebook.disable", true);
> 
> Just to remember how this works, it will show up only for users with that
> have that account but not in the wizard?

Yes, this makes getProtocols() ignore it.

> ::: chat/protocols/facebook/facebook.js
> @@ +22,5 @@
> >  FacebookAccount.prototype = {
> >    __proto__: XMPPAccountPrototype,
> >    get canJoinChat() { return false; },
> >    connect: function() {
> > +    this.WARN("Facebook officially deprecated its XMPP gateway on April 30th, " +
> 
> Might make more sense to point to this bug so we can update it, etc. if we
> need to.

You mean a link to the bug? Or a comment in the code with the bug bumber instead of the WARN? Please suggest an updated string.

> ::: im/locales/en-US/chrome/instantbird/accountWizard.properties
> @@ +17,4 @@
> >  topProtocol.prpl-twitter.description=Stay up to date with your Twitter timeline
> >  topProtocol.prpl-aim.description=Chat with your buddies on AOL Instant Messenger
> >  topProtocol.prpl-yahoo.description=Chat with friends using Yahoo! Messenger
> > +topProtocol.prpl-irc.description=Join IRC chat rooms
> 
> This would be super late strings, right? Or are you thinking it's "OK"
> because these are separate per locale?

These strings are IB-only, and IB...umm...isn't shipping right now.

Comment 10

[aleth [:aleth]]

Comment on attachment 8713808 [details] [diff] [review]
Disable facebook prpl

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

::: im/locales/en-US/chrome/instantbird/accountWizard.properties
@@ +7,5 @@
>  # to the target language / location and comma delimited.
>  # Exceeding 4 protocols may cause scrolling. A list of the
>  # available protocols can be found at
>  #     https://wiki.instantbird.org/Protocol_Identifiers
> +topProtocol.list=prpl-gtalk,prpl-twitter,prpl-aim,prpl-yahoo,prpl-irc

Hmm, I wonder if it should be list2 here to notify localisers?

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8713808 [details] [diff] [review]
Disable facebook prpl

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

(In reply to aleth [:aleth] from comment #10)
> Comment on attachment 8713808 [details] [diff] [review]
> ::: im/locales/en-US/chrome/instantbird/accountWizard.properties
> @@ +7,5 @@
> >  # to the target language / location and comma delimited.
> >  # Exceeding 4 protocols may cause scrolling. A list of the
> >  # available protocols can be found at
> >  #     https://wiki.instantbird.org/Protocol_Identifiers
> > +topProtocol.list=prpl-gtalk,prpl-twitter,prpl-aim,prpl-yahoo,prpl-irc
> 
> Hmm, I wonder if it should be list2 here to notify localisers?

I'm not sure we usually rev that when adding/removing from it.

Comment 12

[aleth [:aleth]]

> > Might make more sense to point to this bug so we can update it, etc. if we
> > need to.
> 
> You mean a link to the bug? Or a comment in the code with the bug bumber
> instead of the WARN? Please suggest an updated string.

Did you ask for a comment or a better WARN string?

Comment 13

[aleth [:aleth]]

Attachment: Disable facebook prpl

Also makes the pref act on the topProtocols just in case some l10n misses the change. I changed the WARN text to refer to the 'find a replacement for xmpp' bug.

Comment 14

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/4f1b3e5af8efdae0712600fe10017f7355cd6eaa
Bug 1236133 - Disable facebook prpl. r=clokep a=aleth for testing in IB CLOSED TREE DONTBUILD

Comment 15

[aleth [:aleth]]

Comment on attachment 8714021 [details] [diff] [review]
Disable facebook prpl

[Approval Request Comment]
Regression caused by: Facebook
User impact if declined: Incorrect connection error ("password incorrect")
Testing completed (on c-c, etc.): Needs a couple of days testing, so might not make b1
Risk to taking this patch (and alternatives if risky): Note the string change in this patch does not affect TB

Comment 16

[Jorg K (CEST = GMT+2)]

Aurora (TB 46):
https://hg.mozilla.org/releases/comm-aurora/rev/41bd20a67a02

Comment 18

[Kent James (:rkent)]

Comment on attachment 8714021 [details] [diff] [review]
Disable facebook prpl

http://hg.mozilla.org/releases/comm-beta/rev/2d0a5e8bd2ea

Comment 19

[aleth [:aleth]]

Comment on attachment 8714021 [details] [diff] [review]
Disable facebook prpl

If someone could verify this works in TB nightlies with existing fb chat accounts, we should consider uplifting this to esr38 too.

Comment 20

[Kent James (:rkent)]

Comment on attachment 8714021 [details] [diff] [review]
Disable facebook prpl

This has 4 different hunk failures on esr38, which is more than I am willing to port. Can you please prepare an esr38 version of this patch?