Opened on 05/25/2018 at 03:00:00 PM
Closed on 07/11/2018 at 07:29:22 PM
Last modified on 08/07/2018 at 08:22:55 AM
#6699 closed change (fixed)
Support more than one default subscription
| Reported by: | hfiguiere | Assignee: | |
|---|---|---|---|
| Priority: | Unknown | Milestone: | Adblock-Plus-3.3-for-Chrome-Opera-Firefox |
| Module: | Platform | Keywords: | circumvention |
| Cc: | arthur, sebastian, kzar | Blocked By: | #6689 |
| Blocking: | Platform: | Unknown / Cross platform | |
| Ready: | no | Confidential: | no |
| Tester: | Ross | Verified working: | yes |
| Review URL(s): | |||
Description (last modified by hfiguiere)
Background
We want to introduce another default subscription list. The whole rationale is at http://hub.eyeo.com/issues/11069
Currently the subscription init code only handle having one default subscription:
https://hg.adblockplus.org/adblockpluschrome/file/tip/lib/subscriptionInit.js
The chooseFilterSubscription() function will pick the subscription that match the language, and if more than one a random amongst them.
Then we can update subscriptions.xml (see issue #6689)
What to change
- Fix the logic in subscriptionInit.js so that we can have two different default subscriptions in subscriptions.xml: we return an "ads" and a "circumvention" subscription.
Update dependencies to have a new subscriptions.xml from adblockplus core as provided by issue #6689.(Issue #6784)
Note
Changes in the UI are handled by #6731
Attachments (0)
Change History (19)
comment:1 Changed on 05/25/2018 at 03:05:31 PM by arthur
- Cc arthur added
comment:2 Changed on 05/25/2018 at 03:16:23 PM by hfiguiere
- Blocked By 6689 added
- Blocking 6689 removed
- Description modified (diff)
comment:3 Changed on 06/05/2018 at 02:23:07 PM by hfiguiere
- Blocked By 6731 added
comment:4 Changed on 06/05/2018 at 02:25:53 PM by hfiguiere
- Blocked By 6731 removed
- Description modified (diff)
comment:5 Changed on 06/13/2018 at 03:38:14 AM by hfiguiere
comment:6 Changed on 06/13/2018 at 09:19:29 AM by mjethani
comment:7 Changed on 06/13/2018 at 12:18:51 PM by hfiguiere
- Component changed from Adblock-Plus-for-Chromium to Platform
comment:8 Changed on 06/19/2018 at 09:19:30 PM by abpbot
A commit referencing this issue has landed:
Issue 6699 - Support the "circumvention" filter list as default subscription
comment:10 follow-up: ↓ 12 Changed on 07/10/2018 at 05:07:18 AM by mjethani
@hfiguiere with reference to changeset d6214de663e0, do we really need the type property on the Subscription class? I have already added it in Core, but now I'm wondering if it is really necessary. I don't really need it for Snippets, I could probably just use the URL of the subscription.
comment:11 follow-up: ↓ 13 Changed on 07/10/2018 at 06:15:53 AM by mjethani
We might have to rethink the above patch. If the user is already subscribed to a non-default list, the default lists won't get added. Is this the intended behavior?
comment:12 in reply to: ↑ 10 Changed on 07/10/2018 at 11:54:04 AM by hfiguiere
Replying to mjethani:
@hfiguiere with reference to changeset d6214de663e0, do we really need the type property on the Subscription class? I have already added it in Core, but now I'm wondering if it is really necessary. I don't really need it for Snippets, I could probably just use the URL of the subscription.
I need it to identify the subscription. I don't consider using the URL the proper way. We could move the list to a different location, and in that case we'd just need to change the subscriptions.xml.
Similarly I also use the type field in the UI to identify it.
comment:13 in reply to: ↑ 11 ; follow-up: ↓ 14 Changed on 07/10/2018 at 01:19:16 PM by hfiguiere
Replying to mjethani:
We might have to rethink the above patch. If the user is already subscribed to a non-default list, the default lists won't get added. Is this the intended behavior?
If we want to subscribe existing users to that new list, I think we should add something in the "upgrade" path. I admit, I missed that.
comment:14 in reply to: ↑ 13 Changed on 07/11/2018 at 03:09:55 PM by mjethani
Replying to hfiguiere:
Replying to mjethani:
We might have to rethink the above patch. If the user is already subscribed to a non-default list, the default lists won't get added. Is this the intended behavior?
If we want to subscribe existing users to that new list, I think we should add something in the "upgrade" path. I admit, I missed that.
OK, the existing users really matter a lot so I hope we're able to add this list for them, preferably by default.
comment:15 Changed on 07/11/2018 at 04:58:07 PM by hfiguiere
Filed issue #6783
comment:16 Changed on 07/11/2018 at 07:29:22 PM by hfiguiere
- Description modified (diff)
- Resolution set to fixed
- Status changed from reviewing to closed
comment:17 Changed on 07/11/2018 at 07:30:01 PM by hfiguiere
- Sensitive unset
comment:18 Changed on 07/12/2018 at 01:51:34 PM by hfiguiere
- Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
comment:19 Changed on 08/07/2018 at 08:22:55 AM by Ross
- Tester changed from Unknown to Ross
- Verified working set
Done. Tested with #6783.
ABP 3.2.2102
Chrome 66 / 55 / 49 / Windows 10
Firefox 61 / 55 / 51
Doesn't this also need a change in core to add a type property to the Subscription class?
Also, shouldn't the module here be Platform?