Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#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/
https://codereview.adblockplus.org/29328658/

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

  1. 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.)
  2. Ensure that all users are put in group 0 if all variants have sample set to 0.

Change History (15)

comment:1 Changed 4 years ago 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 4 years ago 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 4 years ago by fhd

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:4 Changed 4 years ago by fhd

  • Owner set to fhd

comment:5 Changed 4 years ago 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

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.

comment:6 Changed 4 years ago by fhd

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:7 Changed 4 years ago by fhd

  • Tester changed from Unknown to Ross

comment:8 Changed 4 years ago by fhd

  • Description modified (diff)

comment:9 Changed 4 years ago by fhd

  • Description modified (diff)

comment:10 Changed 4 years ago 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 4 years ago by fhd

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:12 Changed 4 years ago 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 4 years ago by fhd

  • Review URL(s) modified (diff)
  • Status changed from reopened to reviewing

comment:14 Changed 4 years ago by fhd

  • Resolution set to fixed
  • Status changed from reviewing to closed
Last edited 4 years ago by fhd (previous) (diff)

comment:15 Changed 4 years ago by fhd

  • Verified working unset

Unchecking verified - the second patch hasn't been verified yet.

Note: See TracTickets for help on using tickets.