Closed Bug 1652392 Opened 4 years ago Closed 3 years ago

"weave:service:login:change" notification is poorly named and has unclear semantics

Categories

(Firefox :: Sync, task, P3)

task

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox90 --- fixed

People

(Reporter: markh, Assigned: bdk)

Details

Attachments

(2 files)

This notification is sent whenever we fetch a new sync token, whether successful or not. Various things observe this notification assuming it does something a little like it describes - eg, the old ecosystem telemetry impl would submit the ping on this notification, and existing sync telemetry sets a scalar in the main ping under the assumption that some account state is actually changing - but it's not. It is sent on the first sync, and every 2 hours after that (assuming token expiry is still 2 hours)

Worse, it's sync specific - users with fxa but not sync will never get it. We could rename it, but I don't think any use-cases in the tree actually need anything new so should be removed.

Blocking bug 1635659 as that currently uses this notification, but should not (I guess it technically need not block it so long as the refactor doesn't use it, but we might as well clean this up.

The severity field is not set for this bug.
:lina, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(lina)
Severity: -- → N/A
Type: defect → task
Flags: needinfo?(lina)
Priority: -- → P3

I think telemetry might rely on this given how we get the hashed uid from the token (ie, every time we get, or fail to get, a token is a reasonable time for telemetry to check if the account has changed). This can probably be refactored, but I don't think it's worthwhile at the moment - we should instead just wait until ecosystem telemetry kills the need for the hashed uid entirely.

Assignee: markh → nobody
No longer blocks: 1635659
Status: ASSIGNED → NEW
Summary: "weave:service:login:change" notification is useless and poorly named → "weave:service:login:change" notification is poorly named and has unclear semantics
Assignee: nobody → bdeankawamura
Status: NEW → ASSIGNED

Uploaded a new phabricator patch to change the name of the notification, rather than remove it.

There were a couple failures in the treehearder run: https://treeherder.mozilla.org/jobs?revision=3895cf7754f94f3dce911264e863ff0f23500058&repo=try, but when I run mach test toolkit/components/passwordmgr/test/mochitest/test_autocomplete_autofill_related_realms_no_dupes locally the tests succeed.

Pushed by mhammond@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0304c3d548cf rename "weave:service:login:change" notification. r=markh
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: