Closed Bug 1171141 Opened 6 years ago Closed 6 years ago

Fetch and update Firefox Account profile/avatar information

Categories

(Firefox for Android Graveyard :: Android Sync, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nalexander, Assigned: vivek, Mentored)

References

Details

Attachments

(1 file, 6 obsolete files)

There are few different ways we might do this.  We might run a background service (periodic or on-demand) to fetch Firefox Account profile data.  A lot of the code for fetching already exists; see https://bugzilla.mozilla.org/show_bug.cgi?id=1055264#c1.

We might ape Desktop, which has a WebChannel monitor that sees profile updates when the user interacts with accounts.firefox.com.  That's less mobile friendly, but is perhaps enough, as we push tickets like Bug 1161234 forward.

For now, a little on-demand service that writes in the format established by Bug 1161685 will move us forward.  We could add this fetch a few minutes after delayed startup, for example.
Attached patch WIP patch (obsolete) — Splinter Review
@Nick: This largely a WIP patch that I have been working on. It would be helpful if you can provide me with some review comments.
Attachment #8621302 - Flags: review?(nalexander)
Comment on attachment 8621302 [details] [diff] [review]
WIP patch

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

This looks a good start!

::: mobile/android/base/fxa/activities/FxAccountStatusActivity.java
@@ +202,5 @@
>      inflater.inflate(R.menu.fxaccount_status_menu, menu);
>      return super.onCreateOptionsMenu(menu);
>    };
> +
> +  /**

Consider making this an asynchronous method on the AndroidFxAccount.  That is, the AFxA gives you cached data if it has it and otherwise does background work to get data.  If it gives cached data, it periodically (once a day?) tries to freshen that data.

::: mobile/android/base/fxa/authenticator/AndroidFxAccount.java
@@ +68,5 @@
>    public static final String BUNDLE_KEY_BUNDLE_VERSION = "version";
>    public static final String BUNDLE_KEY_STATE_LABEL = "stateLabel";
>    public static final String BUNDLE_KEY_STATE = "state";
>  
> +  // Account type for profile

nit: full sentence.  And this should be the "Android authentication token type" or similar.

@@ +294,5 @@
>      return accountManager.getUserData(account, ACCOUNT_KEY_TOKEN_SERVER);
>    }
>  
> +  public String getProfileServerURI() {
> +    return "https://profile.accounts.firefox.com/v1/";

Future: store this in the account, like ACCOUNT_KEY_TOKEN_SERVER and friends.  I see you defined the constant.  We don't want to migrate accounts, so add a fallback to this default if it doesn't exist.  Also might be useful to add stage stuff, like below (in getOAuthServerURI).

@@ +648,5 @@
>    }
>  
> +
> +  public void setProfileJson(ExtendedJSONObject profileJson) {
> +    this.profileJson = profileJson;

Caching the profile here is reasonable, especially as you develop.  Consider serializing this JSON just as a string and re-loading it, like the other account data.

The point is that profile data wants to be stored in the Android account object (getUserData) etc, rather than SharedPreferences.  The difference is basically that the account data stays when you wipe your local storage; shared prefs get wiped.  The profile is not super valuable so we could refetch, but the getUserData approach should work.

::: mobile/android/base/fxa/tasks/FxAccountFetchProfileTask.java
@@ +1,1 @@
> +package org.mozilla.gecko.fxa.tasks;

nit: license headers.

@@ +9,5 @@
> +import org.mozilla.gecko.background.fxa.profile.FxAccountProfileClient10;
> +import org.mozilla.gecko.fxa.authenticator.AndroidFxAccount;
> +import org.mozilla.gecko.sync.ExtendedJSONObject;
> +
> +public class FxAccountFetchProfileTask extends AsyncTask<Void, Void, Void> {

AsyncTasks aren't very robust, since they're tied to UI.  Consider an IntentService for the actual work, like in http://stackoverflow.com/a/10335227.  (Or just doing the background work in AndroidFxAccount, and then adding this little task on top.)

@@ +13,5 @@
> +public class FxAccountFetchProfileTask extends AsyncTask<Void, Void, Void> {
> +  private static final String LOG_TAG = "FxAccountFetchProfileTask";
> +  private final FxAccountAbstractClient.RequestDelegate<ExtendedJSONObject> delegate;
> +  private final AndroidFxAccount androidFxAccount;
> +  private final FxAccountProfileClient10 client;

Oh hey!  This landed -- I had forgotten :)
Attachment #8621302 - Flags: review?(nalexander) → feedback+
Attached patch 1171141.patch (obsolete) — Splinter Review
Profile fetch network call now done through IntentService.
Stringified json response is written to per profile account.

@Nick: I'm yet to do some kind of rate limiting to fetch profile only once daily. I'll address that as a new patch.
Attachment #8621302 - Attachment is obsolete: true
Attachment #8621969 - Flags: review?(nalexander)
Comment on attachment 8621969 [details] [diff] [review]
1171141.patch

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

This is looking quite good.

I would like to standardize terminology: let's not refer to sync, because sync means something very particular on Android (and can also mean Firefox Sync).  So "FxAccountProfileService" throughout (since it will do more than fetch, eventually).

::: mobile/android/base/fxa/FxAccountConstants.java
@@ +20,5 @@
>  
>    public static final String STAGE_AUTH_SERVER_ENDPOINT = "https://stable.dev.lcip.org/auth/v1";
>    public static final String STAGE_TOKEN_SERVER_ENDPOINT = "https://stable.dev.lcip.org/syncserver/token/1.0/sync/1.5";
>    public static final String STAGE_OAUTH_SERVER_ENDPOINT = "https://oauth-stable.dev.lcip.org/v1";
> +  public static final String STAGE_PROFILE_SERVER_ENDPOINT = "https://latest.dev.lcip.org/profile/";

Have you tested this?  I expect it to need v1.

::: mobile/android/base/fxa/activities/FxAccountStatusActivity.java
@@ +232,5 @@
> +    @Override
> +    protected void onReceiveResult(int resultCode, Bundle bundle) {
> +      super.onReceiveResult(resultCode, bundle);
> +
> +      String resultData = bundle.getString(FxAccountProfileSyncService.RESULT_STRING);

This won't necessarily exist if you don't get RESULT_OK, right?  Be lazier, only fetch when required :)

@@ +242,5 @@
> +              statusFragment.updateProfileDetails();
> +            }
> +          }
> +          break;
> +        case FxAccountProfileSyncService.RESULT_NOT_OK:

Let's just log-and-ignore profile issues.  They could be transient, like offline access.

::: mobile/android/base/fxa/activities/FxAccountStatusFragment.java
@@ +396,5 @@
>      setCheckboxesEnabled(true);
>    }
>  
> +  public void updateProfileDetails() {
> +    // Update the icon, username and email in UI thread.

Let's look ahead a bit -- this will take the profile JSON as an ExtendedJSONObject.

::: mobile/android/base/fxa/authenticator/AndroidFxAccount.java
@@ +69,5 @@
>    public static final String BUNDLE_KEY_BUNDLE_VERSION = "version";
>    public static final String BUNDLE_KEY_STATE_LABEL = "stateLabel";
>    public static final String BUNDLE_KEY_STATE = "state";
>  
> +  // Account authentication token type for profile

nit: full sentences.

@@ +113,5 @@
>     * and then re-added.
>     */
>    protected static final ConcurrentHashMap<String, ExtendedJSONObject> perAccountBundleCache =
>        new ConcurrentHashMap<>();
> +  private ExtendedJSONObject profileJson;

This is unused -- even as a cache -- so kill it.

@@ +296,5 @@
>    }
>  
> +  public String getProfileServerURI() {
> +    final String profileURI = accountManager.getUserData(account, ACCOUNT_KEY_PROFILE_SERVER);
> +    return profileURI != null ? profileURI : FxAccountConstants.DEFAULT_PROFILE_SERVER_ENDPOINT;

nit: handle stage, like in getOAuthServerURI.

@@ +432,5 @@
>      Bundle userdata = new Bundle();
>      userdata.putString(ACCOUNT_KEY_ACCOUNT_VERSION, "" + CURRENT_ACCOUNT_VERSION);
>      userdata.putString(ACCOUNT_KEY_IDP_SERVER, idpServerURI);
>      userdata.putString(ACCOUNT_KEY_TOKEN_SERVER, tokenServerURI);
> +    userdata.putString(ACCOUNT_KEY_PROFILE_SERVER, "https://profile.accounts.firefox.com/v1/");

Pass this in as a parameter.  Yes, this is clunky.

@@ +649,5 @@
>      intent.putExtra(FxAccountConstants.ACCOUNT_DELETED_INTENT_ACCOUNT_AUTH_TOKENS, tokens.toArray(new String[tokens.size()]));
>      return intent;
>    }
>  
> +  public void setProfileJsonString(String profileJson) {

In general, it's better to keep everything typed.  So rather than take the string, take (and produce) an ExtendedJSONObject.  And then null check/convert to a string/etc in your function.

::: mobile/android/base/fxa/sync/FxAccountProfileSyncService.java
@@ +20,5 @@
> +import java.util.concurrent.Executors;
> +
> +public class FxAccountProfileSyncService extends IntentService {
> +  private static final String LOG_TAG = "FxAccountProfileSyncService";
> +  private static final Executor EXECUTOR_SERVICE = Executors.newSingleThreadExecutor();

Hmm, making this API require a new executor everywhere was not great.  Threads are not free -- I wonder if we should share an executor between similar parts of the FxA code.  Let's keep it like this for now.

@@ +22,5 @@
> +public class FxAccountProfileSyncService extends IntentService {
> +  private static final String LOG_TAG = "FxAccountProfileSyncService";
> +  private static final Executor EXECUTOR_SERVICE = Executors.newSingleThreadExecutor();
> +  public static final String KEY_AUTH_TOKEN = "auth_token";
> +  public static final String PROFILE_SERVER_URI = "profileServerURI";

Call these KEY_*, since that's what we're using them for.

@@ +25,5 @@
> +  public static final String KEY_AUTH_TOKEN = "auth_token";
> +  public static final String PROFILE_SERVER_URI = "profileServerURI";
> +  public static final String PROFILE_RESULT = "profile_json_result";
> +  public static final String RESULT_STRING = "RESULT_STRING";
> +  public static final int RESULT_OK = 1;

Use http://developer.android.com/reference/android/app/Activity.html#RESULT_OK and http://developer.android.com/reference/android/app/Activity.html#RESULT_CANCELED.

@@ +39,5 @@
> +    final String profileServerURI = intent.getStringExtra(PROFILE_SERVER_URI);
> +    final ResultReceiver resultReceiver = intent.getParcelableExtra(PROFILE_RESULT);
> +
> +    if (authToken == null || authToken.length() == 0) {
> +      Logger.warn(LOG_TAG, "Invalid Auth Token");

This should sendResult too.  Otherwise, how would the consumer know they did a bad thing?

@@ +44,5 @@
> +      return;
> +    }
> +
> +    if (profileServerURI == null || profileServerURI.length() == 0) {
> +      Logger.warn(LOG_TAG, "Invalid profile Server Endpoint");

Ditto.

@@ +48,5 @@
> +      Logger.warn(LOG_TAG, "Invalid profile Server Endpoint");
> +      return;
> +    }
> +
> +    // Network calls are asynchronous, acquire a latch to force it to be synchronous.

Why make this synchronous?  Intents are by nature asynchronous; it's fine to call sendResult whenever.

@@ +77,5 @@
> +        latch.countDown();
> +      }
> +    };
> +
> +    // We can safely reuse the same Executor service for all the requests as at the most only one request will be processed at a time..

It's more like:

// The Executor service orders request, so only one request will be processed at a time.

That is, it's not "safe because...", it's that the service was chosen to be safe.

@@ +84,5 @@
> +      client.profile(authToken, delegate);
> +      // Wait for the profile fetch completes.
> +      latch.await();
> +    } catch (Exception e) {
> +      Logger.error(LOG_TAG, "Got error syncing.", e);

Let's avoid the term syncing.  "Got exception fetching profile." is good.
Attachment #8621969 - Flags: review?(nalexander) → feedback+
Attached patch 1171141.patch (obsolete) — Splinter Review
Review nits addressed. As discussed over irc, profile fetching logic offloaded to AndroidFxAccount
Attachment #8621969 - Attachment is obsolete: true
Attachment #8624869 - Flags: review?(nalexander)
Attached patch 1171141.patch (obsolete) — Splinter Review
Update : Fallback check added for missing profile server URI while unpickling
Attachment #8624869 - Attachment is obsolete: true
Attachment #8624869 - Flags: review?(nalexander)
Attachment #8625632 - Flags: review?(nalexander)
Attached patch 1171141.patch (obsolete) — Splinter Review
Update: Profile fetching moved to background thread.
Attachment #8625632 - Attachment is obsolete: true
Attachment #8625632 - Flags: review?(nalexander)
Attachment #8625821 - Flags: review?(nalexander)
Comment on attachment 8625821 [details] [diff] [review]
1171141.patch

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

vivek and I discussed this, and its his opinion that the layered delegates through the IntentService/ResultReceiver is not clear.

So we're going to make AndroidFxAccount have an explicit caching API:
* getCachedProfileJSON (which might be null); and
* maybeUpdateProfileJSON (which will rate limit and possibly fetch fresh profile JSON).

The maybeUpdate method will dispatch to a background IntentService, which will LocalBroadcastManager to the app-at-large about fresh profile data.  So a UI consumer (like the main context menu/App, or the FxAccountStatusFragment) will get the cached data immediately, ask to get fresh data, and will listen for the Local Broadcast for updates after that.

::: mobile/android/base/fxa/FxAccountConstants.java
@@ +20,5 @@
>  
>    public static final String STAGE_AUTH_SERVER_ENDPOINT = "https://stable.dev.lcip.org/auth/v1";
>    public static final String STAGE_TOKEN_SERVER_ENDPOINT = "https://stable.dev.lcip.org/syncserver/token/1.0/sync/1.5";
>    public static final String STAGE_OAUTH_SERVER_ENDPOINT = "https://oauth-stable.dev.lcip.org/v1";
> +  public static final String STAGE_PROFILE_SERVER_ENDPOINT = "https://latest.dev.lcip.org/profile/v1/";

nit: kill trailing slash.

::: mobile/android/base/fxa/authenticator/AndroidFxAccount.java
@@ +712,5 @@
>      ContentResolver.setIsSyncable(account, BrowserContract.READING_LIST_AUTHORITY, 1);
>    }
>  
> +
> +  public void fetchProfileInformation(final Context context) {

Consider adding a flag to force fetching.

@@ +721,5 @@
> +    }
> +
> +    final String profileJsonString = accountManager.getUserData(account, ACCOUNT_KEY_PROFILE_AVATAR);
> +    if (profileJsonString == null) {
> +      ThreadUtils.postToBackgroundThread(new Runnable() {

This may need to be adjusted for android-sync.

@@ +732,5 @@
> +            if (authToken == null) {
> +              throw new RuntimeException("Couldn't get oauth token!  Aborting profile fetch.");
> +            }
> +          } catch (Exception e) {
> +            Logger.error(LOG_TAG, "Error fetching profile information.", e);

I like to add something like "; ignoring." to be clear when I'm not acting on exceptions.

@@ +749,5 @@
> +      Logger.info(LOG_TAG, "Cached Profile information retrieved from AccountManager.");
> +      try {
> +        profileJson = ExtendedJSONObject.parseJSONObject(profileJsonString);
> +      } catch (Exception e) {
> +        Logger.error(LOG_TAG, "Failed to parse profile json", e);

; ignoring.
Attachment #8625821 - Flags: review?(nalexander)
Attached patch 1171141.patch (obsolete) — Splinter Review
@Nick: As agreed, updates to the profile cache are pushed through LocalBroadcasts
Attachment #8625821 - Attachment is obsolete: true
Attachment #8625972 - Flags: review?(nalexander)
Comment on attachment 8625972 [details] [diff] [review]
1171141.patch

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

Logging data from the profile is PII (Personally Identif{ying,iable} Information), so use FxAccountUtils.pii for personal information.

vivek and I are co-located (\o/) and have talked about this a lot.  It's not perfect, but it's a great start!

::: mobile/android/base/fxa/activities/FxAccountStatusFragment.java
@@ +599,5 @@
>      scheduleAndUpdateLastSyncedTime();
>    }
>  
> +  private void updateProfileInformation() {
> +    if (AppConstants.MOZ_ANDROID_FIREFOX_ACCOUNT_PROFILES) {

Early abort: if (!...) { return };

@@ +645,5 @@
> +    // Read the profile information from json and Update the UI elements.
> +    ThreadUtils.postToUiThread(new Runnable() {
> +      @Override
> +      public void run() {
> +        if (AppConstants.Versions.feature11Plus) {

Explain why this is necessary.

::: mobile/android/base/fxa/authenticator/AndroidFxAccount.java
@@ +107,5 @@
>    private static final String PREF_KEY_LAST_SYNCED_TIMESTAMP = "lastSyncedTimestamp";
> +  public static final String PREF_KEY_LAST_PROFILE_FETCH_TIME = "lastProfilefetchTime";
> +  public static final String PREF_KEY_NUMBER_OF_PROFILE_FETCH = "numProfileFetch";
> +
> +  public static final long PROFILE_FETCH_RETRY_BACKOFF_DELTA_IN_MILLISECONDS = 24 * 60 * 60 * 1000;

Comments for both of these, please.

@@ +693,5 @@
> +      return neverFetched;
> +    }
> +  }
> +
> +  public boolean canScheduleProfileFetch() {

This shouldn't be public.

@@ +784,5 @@
> +   * @param isForceFetch boolean to isForceFetch fetch from the server.
> +   */
> +  public void maybeUpdateProfileJSON(final boolean isForceFetch) {
> +    final ExtendedJSONObject profileJson = getCachedProfileJSON();
> +    final Intent profileCachedIntent = new Intent();

Let's turn this intent maker into a helper.

@@ +795,5 @@
> +      return;
> +    }
> +
> +    final int attempts = getNumberOfProfileFetch();
> +    final long delta = System.currentTimeMillis() - getLastProfileFetchTimestamp();

extra now = System.currentTM and use it below as well.
Attachment #8625972 - Flags: review?(nalexander) → review+
Attached patch 1171141.patchSplinter Review
I cleaned the patch a bit as per your review comments.
Attachment #8625972 - Attachment is obsolete: true
Attachment #8626291 - Flags: review?(nalexander)
Assignee: nobody → vivekb.balakrishnan
Status: NEW → ASSIGNED
Comment on attachment 8626291 [details] [diff] [review]
1171141.patch

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

Landed!
Attachment #8626291 - Flags: review?(nalexander) → review+
woot! should we close this as resolved fixed?
Yes.  I don't know why this didn't get a mozilla-central push message.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 1231549
Product: Android Background Services → Firefox for Android
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.