Opened 7 months ago

Closed 5 months ago

Last modified 4 months ago

#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):

https://codereview.adblockplus.org/29805597/

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

Change History (19)

comment:1 Changed 7 months ago by arthur

  • Cc arthur added

comment:2 Changed 7 months ago by hfiguiere

  • Blocked By 6689 added
  • Blocking 6689 removed
  • Description modified (diff)

comment:3 Changed 6 months ago by hfiguiere

  • Blocked By 6731 added

comment:4 Changed 6 months ago by hfiguiere

  • Blocked By 6731 removed
  • Description modified (diff)

comment:5 Changed 6 months ago by hfiguiere

  • Cc sebastian kzar added
  • Description modified (diff)
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:6 Changed 6 months ago by mjethani

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?

comment:7 Changed 6 months ago by hfiguiere

  • Component changed from Adblock-Plus-for-Chromium to Platform

comment:9 Changed 6 months ago by hfiguiere

  • Description modified (diff)

comment:10 follow-up: Changed 5 months ago 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: Changed 5 months ago 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 5 months ago 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: Changed 5 months ago 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 5 months ago 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 5 months ago by hfiguiere

Filed issue #6783

comment:16 Changed 5 months ago by hfiguiere

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:17 Changed 5 months ago by hfiguiere

  • Sensitive unset

comment:18 Changed 5 months ago by hfiguiere

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next

comment:19 Changed 4 months ago 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

Note: See TracTickets for help on using tickets.