Closed Bug 882363 Opened 7 years ago Closed 7 years ago

Only enable Facebook functionality if the app id is part of the partner customization.

Categories

(Firefox OS Graveyard :: Gaia::Contacts, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed, b2g-v1.1hd fixed)

RESOLVED FIXED
blocking-b2g tef+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: fabrice, Assigned: arcturus)

References

Details

(Whiteboard: QARegressExclude)

Attachments

(1 file, 2 obsolete files)

Currently the facebook app id is hardcoded in apps/communications/contacts/oauth2/js/parameters.js, but this app id can't be shared with all partners.

We need to:
- move the app id to a customizable file, with no default value for now.
- disable all facebook visible features when no app is has been set up in the customization.
blocking-b2g: --- → tef?
Assignee: nobody → francisco.jordano
We can fix this for Buri devices, but aren't we past the time to fix this issue for Ikura devices?
Attached file Pointer to PR 10355 (obsolete) —
Following the necessary steps for partners to ask for their own facebook app id, we have been requested to move it to a file that is part of the customisation right now.

This first approach moves the appid to the config.json file already present in contacts.

We already have a mechanism to load this file and check if fb is enabled or not, now it has another step that is check if we have a appid set or not.

In order to work with the rest of elements we needed to ask the fb_init + config  js files to the rest of documents that are using the oauth 2.0 authentication against fb.

As well, used this patch to remove from the FTU the fb option if not present the appid.

We will need to check if everything works, (did a quick test on the contacts and ftu), but we will need to check synchronisation, post to wall, etc.

JMC we strongly need your advise here.

Thanks!
F.
Attachment #761796 - Flags: review?
Attachment #761796 - Flags: feedback?
Attachment #761796 - Flags: review?(jmcf)
Attachment #761796 - Flags: review?
Attachment #761796 - Flags: feedback?(fabrice)
Attachment #761796 - Flags: feedback?
Attachment #761796 - Flags: feedback?(yurenju.mozilla)
Hi folks, above is a POC to check the feasibility and how much work we need to port that.

I've sync with Jose Manuel this morning and we are preparing another patch to try to address this.

Same philosophy, moving the appid to the json file.

Thanks
Attached file Pointer to GH PR (obsolete) —
Attachment #761796 - Attachment is obsolete: true
Attachment #761796 - Flags: review?(jmcf)
Attachment #761796 - Flags: feedback?(yurenju.mozilla)
Attachment #761796 - Flags: feedback?(fabrice)
Attachment #761948 - Flags: review?(francisco.jordano)
We have been discussing with Yuren, and took a simplier approach.

We don't modify any code, we just add the two places where we setup the services:
config.json where we say if we enable facebook or not and
parameters.js where we specify the services keys

Now this is mapped to partners configuration files in the following way:

communications.json -> apps/communications/contacts/config.json
communications_services.json -> apps/communications/contacts/oauth2/js/parameters.js

Now changes are quite smaller and aligned with partner customisation rules:
https://wiki.mozilla.org/B2G/MarketCustomizations

Comments?

Thanks!
Attached file Pointer to PR 10355 v2
Attachment #761948 - Attachment is obsolete: true
Attachment #761948 - Flags: review?(francisco.jordano)
Attachment #761964 - Flags: review?(yurenju.mozilla)
Attachment #761964 - Flags: review?(jmcf)
Comment on attachment 761964 [details]
Pointer to PR 10355 v2

thanks Francisco!
Attachment #761964 - Flags: review?(jmcf) → review+
Comment on attachment 761964 [details]
Pointer to PR 10355 v2

r=yurenju if you remove parameters.js & config.json from git repository which will be generated in build time.
Attachment #761964 - Flags: review?(yurenju.mozilla) → review+
tef+. this is a blocker for shira release
blocking-b2g: tef? → tef+
Hi folks,

just one comment about this.

The configuration is separeted in two files, one that says if FB is enabled and the other one that contains the keys and endpoints.

With the current configuration in master we are enabling FB, but the appid is empty cause Mozilla needs to negotiate one, which will be lead to errors.

The Telefonica variant including the app id works perfectly.

Thanks
Asking for uplift to v1-train since this will be affected there.
Does the changes in this patch require followup bugs to make changes to the partner customization repositories?
1.1hd: ea195edff2f75cb9548404f732566f574ab56abe
Depends on: 883365
Please back this out. This just broke every possible branch Gaia is supported on for testing Facebook import functionality on non-customized builds.
Depends on: 883344
No longer depends on: 883365
(In reply to Jason Smith [:jsmith] from comment #18)
> Please back this out. This just broke every possible branch Gaia is
> supported on for testing Facebook import functionality on non-customized
> builds.

Actually, let's just quickly fix bug 883344.
Who provides the facebook app id? Phone manufacturer or service provider?
Flags: needinfo?(fabrice)
Can you please provide steps to verify this fix as we will blackbox test from the UI?
@Jeni,

right now the idea is to provide a default fb app id, right now there is a fake facebook id which will show a 'appid invalid', is the responsability of each partner to provide their own keys to guarantee the service.

Thanks,
F.
Whiteboard: QARegressExclude
Flags: needinfo?(fabrice)
You need to log in before you can comment on or make changes to this bug.