Manually triggered sync should ignore system account sync settings

RESOLVED FIXED in Firefox 57

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Grisha, Assigned: mehdisolamannejad, Mentored)

Tracking

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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.
(Reporter)

Updated

2 years ago
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
(Reporter)

Comment 2

2 years ago
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)
(Assignee)

Comment 3

2 years ago
Ok sure. But would you mentor me on this one as well?
Flags: needinfo?(mehdisolamannejad)
(Reporter)

Comment 4

2 years ago
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 hidden (mozreview-request)
(Reporter)

Comment 6

2 years ago
mozreview-review
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+
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 8

2 years ago
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
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.