Closed Bug 1574812 Opened 5 months ago Closed 5 months ago

Deploy CFR: Offer Fx Sync when a password is saved to Firefox 70+

Categories

(Firefox :: Messaging System, enhancement, P1, major)

Desktop
All
enhancement

Tracking

()

VERIFIED FIXED
Firefox 70
Tracking Status
firefox69 --- unaffected
firefox70 blocking verified
firefox71 --- verified

People

(Reporter: MattN, Assigned: nanj)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [skyline] [rca - Product Decision])

Attachments

(1 file)

After a user saves a login in Firefox, offer Firefox Sync. Bug 1570372 implemented the basics of the trigger and landed strings but this bug will test the layout and ensure that the feature works end-to-end with the appropriate images, strings, and trigger (without a host).

Relationships KPI Deck Slide | UX Spec (scroll to the very bottom)

[Tracking Requested - why for this release]: Blocker for Fx70.

{
  "data": {
    "weight": 100,
    "id": "SAVE_LOGIN",
    "frequency": {
      "lifetime": 3
    },
    "targeting": "isFxAEnabled == true && usesFirefoxSync == false && firefoxVersion >= 70",
    "template": "cfr_doorhanger",
    "last_modified": 1565907636313,
    "content": {
      "layout": "icon_and_message",
      "text": {
        "string_id": "cfr-doorhanger-sync-logins-body"
      },
      "icon": "chrome://browser/content/aboutlogins/icons/intro-illustration.svg",
      "buttons": {
        "secondary": [
          {
            "label": {
              "string_id": "cfr-doorhanger-extension-cancel-button"
            },
            "action": {
              "type": "CANCEL"
            }
          },
          {
            "label": {
              "string_id": "cfr-doorhanger-extension-never-show-recommendation"
            }
          },
          {
            "label": {
              "string_id": "cfr-doorhanger-extension-manage-settings-button"
            },
            "action": {
              "type": "OPEN_PREFERENCES_PAGE",
              "data": {
                "category": "general-cfrfeatures"
              }
            }
          }
        ],
        "primary": {
          "label": {
            "string_id": "cfr-doorhanger-sync-logins-ok-button"
          },
          "action": {
            "type": "OPEN_PREFERENCES_PAGE",
            "data": {
              "category": "sync"
            }
          }
        }
      },
      "bucket_id": "CFR_SAVE_LOGIN",
      "heading_text": {
        "string_id": "cfr-doorhanger-sync-logins-header"
      },
      "info_icon": {
        "label": {
          "string_id": "cfr-doorhanger-extension-sumo-link"
        },
        "sumo_path": "extensionrecommendations"
      },
      "notification_text": {
        "string_id": "cfr-doorhanger-extension-notification"
      },
      "category": "cfrFeatures"
    },
    "trigger": {
      "id": "newSavedLogin"
    }
  }
}

Relevant prefs:

  • To test the built-in CFR provider set browser.newtabpage.activity-stream.asrouter.providers.cfr to:
    • {"id":"cfr","enabled":true,"type":"local","localProvider":"CFRMessageProvider","bucket":"cfr","frequency":{"custom":[{"period":"daily","cap":1}]},"categories":["cfrAddons","cfrFeatures"],"updateCycleInMs":3600000}
  • To test telemetry set browser.ping-centre.log to true

Remote CFR docs

Flags: qe-verify+

MattN: is this something you can get into Nightly 70 this week or do you think is it going to end up as a beta uplift?

Flags: needinfo?(MattN+bmo)

There may be no work here if bug 1571022 handles it all. So far it seems like the patch there is adding the recommendation and I've asked if it will fix the assumption that the CFR trigger is for a specific site(s).

If we have to uplift that is probably fine since the strings landed in the first bug.

Flags: needinfo?(MattN+bmo)

Here's an update on the status. I tested the WIP patch from bug 1571022 at https://phabricator.services.mozilla.com/D43630 and it's working well (including telemetry) except for the image being only 32x32px (which I commented on).

I guess it would also be good to write a test to ensure that the trigger continues to work without hosts (a higher-level test than the one already landed for the trigger). Then I think we also need to somehow deploy this CFR recipe/rule to the remote settings server but I don't know the process for that.

Kate, can you help me with next steps to get this deployed for 70? Will that be done in another bug at the same time as the other Fx70 CFR rules/recipes? Thanks.

Flags: needinfo?(khudson)

As far as I can tell the try...catch was originally added around various sendAsyncMessage calls but then when some logic got moved to a helper, the try...catch didn't move along with it. This try...catch made it very annoying to debug an issue with bug 1570372 because it swallowed errors that were getting thrown. It doesn't seem like a good idea to swallow all errors for large pieces of code.

Pushed by elee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb40acd1ac7f
Don't try...catch routeMessageToTarget, only the sendAsyncMessage. r=Mardak

Oops, didn't realize the try/catch patch was attached to this bug.

Keywords: leave-open

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #3)

Here's an update on the status. I tested the WIP patch from bug 1571022 at https://phabricator.services.mozilla.com/D43630 and it's working well (including telemetry) except for the image being only 32x32px (which I commented on).

Update on the image: It's now 64x64 in the latest patch and rgaddis is fine with that for 70 so that issue is resolved.

Keywords: leave-open
Depends on: 1578209
Depends on: 1578236

In order to deploy this to 70, you can file a new bug in the Messaging System component with a title like "Deploy XYZ message for Firefox 70..." including the targeting / content as a comment or an doc/attachment, whatever is easiest. In order to QA, we can either deploy to staging or just ship it to nightly.

We'll see it in triage but feel free to needinfo :nanj directly to help with deployment

Flags: needinfo?(khudson)

I think we can use this bug since we didn't end up needing to do any more work for the CFR here. I believe comment 0 has the message to deploy unless there have been schema changes since then. Nan, can you deploy this for Nightly and then we can QA it and then deploy to Beta 70?

Thanks

Flags: needinfo?(najiang)
Summary: CFR: Offer Fx Sync when a password is saved → Deploy CFR: Offer Fx Sync when a password is saved to Firefox 70+
Whiteboard: [skyline] [blocked on bug 1571022 and triggers without hosts] → [skyline]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #11)

I think we can use this bug since we didn't end up needing to do any more work for the CFR here. I believe comment 0 has the message to deploy unless there have been schema changes since then. Nan, can you deploy this for Nightly and then we can QA it and then deploy to Beta 70?

Sure thing!

To be clear, we usually deploy the new CFR to our stage environment for QA. Would that be OK in this case? If we wanted to do a staged rollout, such as nightly goes first, then beta, and release. We'd have to do that by adding a release channel filter in the CFR targeting criteria.

Flags: needinfo?(najiang)

That's fine with me. From comment 10 I didn't know which was easier.

Assignee: MattN+bmo → najiang

This CFR has already been deployed to both stage and prod.

Note that we've updated the targeting to "targeting": "isFxAEnabled == true && usesFirefoxSync == false && firefoxVersion >= 70" so that it only triggers for Firefox 70 and after.

Thanks, I didn't know that. I guess this is ready for QA testing then.

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Flags: needinfo?(cosmin.muntean)
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

I have verified this issue with the latest Firefox Beta (70.0b6 Build ID - 20190912160217) Firefox Nightly (71.0a1 Build ID - 20190916093313) installed, on Windows 10 x64, Arch Linux and Mac 10.14.6. I can confirm that the "CFR Sync Logins" recommendation is correctly triggered without any additional configuration.

However, we will also start running our Full Functional test suite for the feature and will send the Pre-Release preliminary status report until Sept 27.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(cosmin.muntean)
Depends on: 1584949

This bug has been identified as part of a pilot on determining root causes of blocking and dot release drivers.

It needs a root-cause set for it. Please see the list at https://docs.google.com/document/d/1FFEGsmoU8T0N8R9kk-MXWptOPtXXXRRIe4vQo3_HgMw/.

Add the root cause as a whiteboard tag in the form [rca - <cause> ] and remove the rca-needed keyword.

If you have questions, please contact :tmaity.

Keywords: rca-needed
Keywords: rca-needed
Whiteboard: [skyline] → [skyline] [rca - Product Decision]
You need to log in before you can comment on or make changes to this bug.