Opened on 09/03/2015 at 08:31:23 AM
Closed on 09/25/2015 at 12:28:23 PM
Last modified on 09/25/2015 at 12:30:25 PM
#2982 closed change (fixed)
Don't send a final notification to users that were in the test
| Reported by: | fhd | Assignee: | fhd |
|---|---|---|---|
| Priority: | P2 | Milestone: | |
| Module: | Sitescripts | Keywords: | |
| Cc: | Blocked By: | ||
| Blocking: | Platform: | Unknown / Cross platform | |
| Ready: | yes | Confidential: | no |
| Tester: | Ross | Verified working: | no |
| Review URL(s): |
https://codereview.adblockplus.org/29325919/ |
||
Description (last modified by fhd)
Background
We're running a test for the Adblock Browser promotion notification (see: https://codereview.adblockplus.org/29325400/). We want to ensure that users who have joined this test don't see the notification again when we show it to all users.
What to change
- Deliver a notification to users in group 0 if both title and message are present. (This is necessary because currently, no notification is ever returned for group 0, even if there is a title and message.)
- Ensure that all users are put in group 0 if all variants have sample set to 0.
Attachments (0)
Change History (15)
comment:1 Changed on 09/03/2015 at 08:31:52 AM by fhd
- Summary changed from Exclude users that saw the browser notification test from the real browser notification to Exclude users that saw the browser notification test from the real notification
comment:2 Changed on 09/03/2015 at 08:34:45 AM by fhd
- Summary changed from Exclude users that saw the browser notification test from the real notification to Don't send the browser notification to users that were in the test
comment:3 Changed on 09/03/2015 at 09:19:22 AM by fhd
- Review URL(s) modified (diff)
- Status changed from new to reviewing
comment:4 Changed on 09/03/2015 at 09:20:29 AM by fhd
- Owner set to fhd
comment:5 Changed on 09/03/2015 at 02:42:24 PM by fhd
- Component changed from Infrastructure to Sitescripts
- Description modified (diff)
- Review URL(s) modified (diff)
- Status changed from reviewing to reopened
- Summary changed from Don't send the browser notification to users that were in the test to Don't send a final notification to users that were in the test
comment:6 Changed on 09/04/2015 at 09:09:10 AM by fhd
- Review URL(s) modified (diff)
- Status changed from reopened to reviewing
comment:7 Changed on 09/09/2015 at 01:53:03 PM by fhd
- Tester changed from Unknown to Ross
comment:10 Changed on 09/24/2015 at 06:22:07 PM by Ross
- Verified working set
After actually applying the patch:
1: Working - Title and message are now included in group 0 responses.
2: Working - Always placed in group 0 with all samples set to 0 (unless already in a group).
issue-2982.patch
infrastructure-git / upstream / 5a8350483c0d0d3aadf7432bbdf34d92a7190582
comment:11 Changed on 09/24/2015 at 08:54:22 PM by fhd
- Resolution set to fixed
- Status changed from reviewing to closed
comment:12 Changed on 09/25/2015 at 12:22:54 PM by fhd
- Resolution fixed deleted
- Status changed from closed to reopened
Unfortunately I have noticed that, in practice, notifications without title and message are still being returned - with an empty title and message. The clients seem to ignore that, but I don't want to bet on that...
comment:13 Changed on 09/25/2015 at 12:25:23 PM by fhd
- Review URL(s) modified (diff)
- Status changed from reopened to reviewing
comment:14 Changed on 09/25/2015 at 12:28:23 PM by fhd
- Resolution set to fixed
- Status changed from reviewing to closed
Pushed the fix: https://hg.adblockplus.org/sitescripts/rev/7558e028fa23
comment:15 Changed on 09/25/2015 at 12:30:25 PM by fhd
- Verified working unset
Unchecking verified - the second patch hasn't been verified yet.
The initial idea here is too much of a hack, we can do better. Since the group designator is the cache key, we can move this logic to the handler.