Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#5950 closed change (fixed)

Add privacy friendly acceptable ads documentation link

Reported by: wspee Assignee: ferris
Priority: P3 Milestone:
Module: Infrastructure Keywords:
Cc: Blocked By:
Blocking: #5949 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29590597/

Description (last modified by wspee)

Background

We need to add Privacy friendly Acceptable Ads documentation link for the launch of the new options page

What to change

Add the documentation link redirects as specified in https://bitbucket.org/adblockplus/spec/pull-requests/87/specified-link-for-privacy-friendly/diff

Note

This is a duplicate of what #5869 was supposed to be, but since #5869 ended up implementing something different all together I'm keeping this.

Change History (19)

comment:1 Changed 2 years ago by saroyanm

  • Blocking 5949 added

comment:2 Changed 2 years ago by saroyanm

  • Cc matze ferris added

@matze, @ferris: Please let me know if you prefer this ticket to be moved into the hub.

comment:3 Changed 2 years ago by saroyanm

  • Description modified (diff)

comment:4 Changed 2 years ago by saroyanm

  • Description modified (diff)

comment:5 Changed 2 years ago by saroyanm

  • Description modified (diff)

comment:6 Changed 2 years ago by saroyanm

  • Description modified (diff)

comment:7 Changed 2 years ago by saroyanm

  • Description modified (diff)

comment:8 Changed 2 years ago by saroyanm

  • Description modified (diff)

comment:9 Changed 2 years ago by wspee

  • Cc matze ferris removed

@matze, ferris
No need to do anything here. I'll provide a patch :) (Feel free to re-cc in case you are still interested in this issue).

comment:10 Changed 2 years ago by wspee

  • Description modified (diff)

comment:11 Changed 2 years ago by wspee

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

comment:12 Changed 2 years ago by ferris

  • Owner set to ferris

comment:13 follow-up: Changed 2 years ago by matze

  • Priority changed from Unknown to P3
  • Type changed from defect to change

Seems like we need some clarification here:

  1. Ticket #5869 did not directly result in "something different all together", but was closed prematurely with the most important bit missing: The adblockplus.org link (legacy format) to the adblockplus.to / eyeo.to (new format) redirect.
  2. Ticket #5958 is invalid, a product of miscommunication which came as a result from the missing link explained above. Just like the Note section in the ticket description here.
  3. Either the specification has changed between #5869 and the review for this ticket here, or the target link was never correct. Either way, it's what is addressed in said review. But...
  4. We still need to bridge the legacy and new implementations, for which there is no patch-set yet.

Note that this could have been avoided by just not using our new campaign links features for this one. Maybe this would have been the better option - not only to avoid all of this hassle, but also because there is no added benefit from using both the legacy and the new version here, maybe even quite the contrary:

  • We'll analyze both redirecting logs anyway, at least as long as the legacy ones aren't included with the new ones entirely.
  • There are different purposes and stakeholders for both entities (though there's some overlap), e.g. our communications department for eyeo.to vs. a potpourri of folks for adblockplus.org/redirect.
  • Thus we could very well consider both entities to be (or should be) unrelated.
  • Also the spec states very clearly what the team expected. We (operations department / infrastructure) should never derive from such resources without explicit prior alignment and especially without explicit necessity.

comment:14 in reply to: ↑ 13 Changed 2 years ago by wspee

Replying to matze:

Seems like we need some clarification here:

  1. Ticket #5869 did not directly result in "something different all together", but was closed prematurely with the most important bit missing: The adblockplus.org link (legacy format) to the adblockplus.to / eyeo.to (new format) redirect.

What has been implemented in #5869 is useless for us (=we can literally not use the redirect that has been added, see below) and I don't think it makes sense to connect #5869 and #5950, see below.

  1. Ticket #5958 is invalid, a product of miscommunication which came as a result from the missing link explained above. Just like the Note section in the ticket description here.

If you don't want to connect #5869 and #5950 #5958 is not a duplicate of #5950. #5958 is supposed to revert the changes introduced in #5869, see below.

  1. Either the specification has changed between #5869 and the review for this ticket here, or the target link was never correct. Either way, it's what is addressed in said review. But...

Yes the specification has changed. It was suggested that using a more specific anchor tag is preferred (#privacy vs #privacy-friendly-acceptable-ads).

  1. We still need to bridge the legacy and new implementations, for which there is no patch-set yet.

No, we (abp ui/the extension abp) don't need the bridge between the legacy and new redirect service. In fact redirects only in the new redirect service are useless to us.

To elaborate:

What you call the legacy system I call documentation link, see documentation link spec.

It's a feature that consists of two parts a function called get_documentation_link (or smth) in the abp extenion and the adblockplus.org/redirect url. The function takes the name of the link and returns the url to the redirect url e.g.:

> get_documentation_link('adblock_plus_report_issue')
"https://adblockplus.org/redirect?link=adblock_plus_report_issue"

Fetching https://adblockplus.org/redirect?link=adblock_plus_report_issue then redirects to target url

$ curl -k  -v https://adblockplus.org/redirect?link=adblock_plus_report_issue
< HTTP/2 302 
< location: https://forums.lanik.us/viewforum.php?f=64

This is done to be able to update the target url without having to release a new version of abp.

Now we could chain the two systems behind each other like so:

> get_documentation_link('adblock_plus_report_issue')
"https://adblockplus.org/redirect?link=adblock_plus_report_issue"

Fetching https://adblockplus.org/redirect?link=adblock_plus_report_issue then redirects to target url

$ curl -k  -v https://adblockplus.org/redirect?link=adblock_plus_report_issue
< HTTP/2 302 
< location: https://eyeo.to/adblockplus/adblock_plus_report_issue
$ curl -k  -v https://eyeo.to/adblockplus/adblock_plus_report_issue
< HTTP/2 302 
< location: https://forums.lanik.us/viewforum.php?f=64

But I would not like to do this this for a single redirect. I would be happy to work towards migrating the documentation link feature to a new backend but this should be done for all links at once and independent of changes to the links it self i.e. not in this issue.

The next ABP release is scheduled for the 6. of November and we need this change by then (really the sooner the better because this link yields a 404 for our nightly users).

I think for now we should land the patch https://codereview.adblockplus.org/29590597/ as is, revert the changes done in #5869 and then if you are interested (we are not really, for us it works fine as is) work towards migrating the whole documentation link feature to a new redirect backend in a separate ticket.

Hope that clarifies a few things, let me know what you think, thanks :).

comment:15 Changed 2 years ago by wspee

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

comment:16 Changed 2 years ago by wspee

  • Resolution fixed deleted
  • Status changed from closed to reopened

Nevermind

comment:18 Changed 2 years ago by ferris

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

comment:19 Changed 2 years ago by matze

Since I link to this ticket in documentation here's some more context when landing here:

What has been implemented in #5869 is useless for us (=we can literally not use the redirect that has been added, see below) and I don't think it makes sense to connect #5869 and #5950, see below.

No, we (abp ui/the extension abp) don't need the bridge between the legacy and new redirect service. In fact redirects only in the new redirect service are useless to us.

I have not a single clue why my clarification is rejected when the following elaboration is perfectly in line with what I wrote. The only piece of actual rationale I found against the suggested plan (which I just made to not reject Ferris' initial implementation entirely) translates roughly into "dislike when done for a single link only", which I did not suggest anywhere anyway (a bridge is not usually built for a single entity only).

However, after I ended the discussion by accepting the patch-set (which, yes and again, could and should have ended this endeavor in the first place), the bridge was built, and should now render said patch-set obsolete.

Note: See TracTickets for help on using tickets.