Closed
Bug 1446445
Opened 7 years ago
Closed 7 years ago
[Normandy] Recipes using normandy.isFirstRun are not executed during first run
Categories
(Firefox :: Normandy Client, defect, P1)
Firefox
Normandy Client
Tracking
()
VERIFIED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | + | verified |
firefox61 | --- | verified |
People
(Reporter: aflorinescu, Assigned: mythmon)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
Gijs
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
[PreRequisites:]
1. Set the app.normandy.dev_mode preference to true to run recipes immediately on startup.
2. Set the app.normandy.logging.level preference to 0 to enable more logging.
3. Set the security.content.signature.root_hash preference to DB:74:CE:58:E4:F9:D0:9E:E0:42:36:BE:6C:C5:C4:F6:6A:E7:74:7D:C0:21:42:7A:03:BC:2F:57:0C:8B:9B:90.
4. Set the preference value for app.normandy.api_url set to https://normandy.stage.mozaws.net/api/v1
[Steps:]
1. Open staging Control center and create any type of recipe with the following filter:
normandy.isFirstRun
2. Open Beta 60 or Nightly 61.
[Actual Result:]
Recipe created at step 1 is fetched but not executed.
[Expected Result:]
Filter should return true and recipe created at step 1 fetched and executed.
Reporter | ||
Comment 1•7 years ago
|
||
Forgot to add at step 2, Beta 60 and Nightly 61 should be opened with a new profile and the pre-requisites should already be defined in prefs.js, in order to be able to reproduce the issue.
Reporter | ||
Comment 2•7 years ago
|
||
[Additional Note:] app.normandy.dev_mode true or false makes no difference: recipe still doesn't execute.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mcooper
Status: NEW → ASSIGNED
Priority: -- → P1
How's the patch moving?
Flags: needinfo?(mcooper)
Assignee | ||
Comment 5•7 years ago
|
||
The patch is pending review from Gijs. I believe it is ready to go.
Gijs, is there anything blocking this on your end I can help with? This is a major bug that we're going to need to uplift to Beta 60 to support bug 1436113. I'd much rather it land soon than have to back out the toolkit component migrations.
Flags: needinfo?(mcooper) → needinfo?(gijskruitbosch+bugs)
Comment 6•7 years ago
|
||
(In reply to Mike Cooper [:mythmon] from comment #5)
> The patch is pending review from Gijs. I believe it is ready to go.
There's no review request, so I didn't think it was...
Flags: needinfo?(gijskruitbosch+bugs)
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8960352 [details]
Bug 1446445 - app.normandy.first_run should default to true
https://reviewboard.mozilla.org/r/229110/#review237578
::: toolkit/components/normandy/lib/ClientEnvironment.jsm:206
(Diff revision 1)
> const addons = await Addons.getAll();
> return Utils.keyBy(addons, "id");
> });
>
> XPCOMUtils.defineLazyGetter(environment, "isFirstRun", () => {
> - return Preferences.get("app.normandy.first_run");
> + return Services.prefs.get("app.normandy.first_run", true);
Services.prefs.get doesn't exist. You want Services.prefs.getBoolPref, probably?
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8960352 [details]
Bug 1446445 - app.normandy.first_run should default to true
https://reviewboard.mozilla.org/r/229110/#review237578
> Services.prefs.get doesn't exist. You want Services.prefs.getBoolPref, probably?
(Throughout this set of changes)
Assignee | ||
Comment 9•7 years ago
|
||
> There's no review request, so I didn't think it was...
Odd. Sorry about that, I guess I confused myself with review board. It seems even though I flagged you for review on review board it didn't sync to Bugzilla.
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
tracking-firefox60:
--- → +
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8960352 [details]
Bug 1446445 - app.normandy.first_run should default to true
https://reviewboard.mozilla.org/r/229110/#review238136
I'm a little bit concerned none of the below cause test failures (or did the tests not get run?), but r=me with these fixed, I guess...
::: toolkit/components/normandy/lib/ClientEnvironment.jsm:83
(Diff revision 2)
> XPCOMUtils.defineLazyGetter(environment, "userId", () => {
> - let id = Preferences.get("app.normandy.user_id", "");
> + let id = Services.prefs.getCharPref("app.normandy.user_id", "");
> if (!id) {
> // generateUUID adds leading and trailing "{" and "}". strip them off.
> id = generateUUID().toString().slice(1, -1);
> - Preferences.set("app.normandy.user_id", id);
> + Services.prefs.set("app.normandy.user_id", id);
setCharPref
::: toolkit/components/normandy/lib/ClientEnvironment.jsm:146
(Diff revision 2)
> }
> return null;
> });
>
> XPCOMUtils.defineLazyGetter(environment, "syncSetup", () => {
> - return Preferences.isSet("services.sync.username");
> + return Services.prefs.isSet("services.sync.username");
`prefHasUserValue`, I think?
Attachment #8960352 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
Pushed by mcooper@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aeeb277faf1d
app.normandy.first_run should default to true r=Gijs
Comment 15•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Reporter | ||
Updated•7 years ago
|
Flags: qe-verify+
QA Contact: adrian.florinescu
Assignee | ||
Comment 16•7 years ago
|
||
Once this fix is verified on Nightly, I plan to request uplift of this to Beta 60.
Comment 17•7 years ago
|
||
Managed to reproduce the initial issue on 61.0a1 (2018-03-16) using the steps provided by Adrian. I can confirm that the bug is verified fixed on 61.0a1 (2018-04-02), using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12.6.
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 18•7 years ago
|
||
Comment on attachment 8960352 [details]
Bug 1446445 - app.normandy.first_run should default to true
Approval Request Comment
[Feature/Bug causing the regression]: bug 1436113
[User impact if declined]: First run studies will never run
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Done already, STR in comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It is a simple change that is easy to verify and has limited side effects
[String changes made/needed]: None
Attachment #8960352 -
Flags: approval-mozilla-beta?
Comment 19•7 years ago
|
||
Comment on attachment 8960352 [details]
Bug 1446445 - app.normandy.first_run should default to true
normandy regression fix, verified in nightly, approved for 60.0b10
Attachment #8960352 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•7 years ago
|
||
bugherder uplift |
Comment 21•7 years ago
|
||
The fix is also successfully applied on 60.0b10 (20180404171943) across platforms (Windows 10 x64, Ubuntu 16.04 x86 and macOS 10.13.3).
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•