Closed Bug 1401336 Opened 7 years ago Closed 7 years ago

Manually triggered sync should ignore system account sync settings

Categories

(Firefox for Android Graveyard :: Firefox Accounts, enhancement, P3)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: Grisha, Assigned: mehdisolamannejad, Mentored)

References

Details

Attachments

(1 file)

In addition to "sync not triggered", the pull-to-refresh animation gets stuck in a spinning state.

We've discussed having user-triggered syncs ignore such settings in https://bugzilla.mozilla.org/show_bug.cgi?id=802749#c45 and in https://bugzilla.mozilla.org/show_bug.cgi?id=802749#c49

The consensus was that settings _should_ be ignored. As such, this behaviour will be fixed in 57 as part of Bug 802749.

I'm filing this bug to make finding this issue easier, and also to consider if we want to land and uplift a fix for this for 56.
Actually, Bug 802749 is going to wait until 58.

This _should_ be a pretty straightforward patch, so I'll consider landing it for 57 by itself.
Summary: Pull-to-refresh on synced devices list doesn't trigger a sync if account sync is disabled in Android settings → Manually triggered sync should ignore system account sync settings
Let me know if you'd like to land part of your work from Bug 802749 here instead, and then simplify your patch from Bug 802749.
Flags: needinfo?(mehdisolamannejad)
Ok sure. But would you mentor me on this one as well?
Flags: needinfo?(mehdisolamannejad)
Sure thing. Simply carrying over parts of your patch from Bug 802749 should do the trick here. Specifically for this bug, we want to change requestImmediateSync to accept "ignoreSettings" boolean, set the SYNC_EXTRAS_IGNORE_SETTINGS flag, and modify all of consumers of requestImmediateSync to specify if their requests should ignore settings.

What will remain in Bug 802749 is the network checking & other logic in onPerformSync, and the Sync Preference changes.
Assignee: nobody → mehdisolamannejad
Mentor: gkruglov
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8910249 [details]
Bug 1401336 - Make user-initiated syncs ignore account sync settings.  Kruglov

https://reviewboard.mozilla.org/r/181736/#review187202

Looks good, thanks!

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:685
(Diff revision 1)
>    /**
>     * Request an immediate sync.  Use this to sync as soon as possible in response to user action.
>     *
>     * @param stagesToSync stage names to sync; can be null to sync <b>all</b> known stages.
>     * @param stagesToSkip stage names to skip; can be null to skip <b>no</b> known stages.
> +   * @param ignoreSettings whether we should check preferences for syncing over metered connections.

This comment needs to change to be more generic.

Perhaps,

"Whether we should check if syncing is allowed via in-app or system settings."
Attachment #8910249 - Flags: review?(gkruglov) → review+
(In reply to :Grisha Kruglov from comment #6)
> Comment on attachment 8910249 [details]
> Bug 1401336 - Make user-initiated syncs ignore account sync settings. 
> Kruglov
> 
> https://reviewboard.mozilla.org/r/181736/#review187202
> 
> Looks good, thanks!
> 
> :::
> mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/
> AndroidFxAccount.java:685
> (Diff revision 1)
> >    /**
> >     * Request an immediate sync.  Use this to sync as soon as possible in response to user action.
> >     *
> >     * @param stagesToSync stage names to sync; can be null to sync <b>all</b> known stages.
> >     * @param stagesToSkip stage names to skip; can be null to skip <b>no</b> known stages.
> > +   * @param ignoreSettings whether we should check preferences for syncing over metered connections.
> 
> This comment needs to change to be more generic.
> 
> Perhaps,
> 
> "Whether we should check if syncing is allowed via in-app or system
> settings."

I'm marking this as fixed, but I'll edit it in Bug 802749.

Thank you for the review.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e740ebb21e46
Make user-initiated syncs ignore account sync settings. r=Grisha Kruglov
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e740ebb21e46
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: