Proof-of-concept: Link version to corresponding announcement

Proof-of-concept: Link version to corresponding announcement

[Sebastian Noack, 2016-02-05 14:19:00]

As discussed on IRC, this is a proof-of-concept for linking the version number
on the options page directly to the corresponding release announcement. If we
agree on an approach I file an issue.

Patch Set 1 loads the Atom feed for the releases section when the option page
loads linking the version number to the corresponding announcement post. If no
release announcement has been found the version number won't be linked.

Patch Set 2 always links the version number, except for development builds. Once
you click on the link the Atom feed is loaded, and if a corresponding
announcement post is found it will be navigated to, otherwise we fallback to the
releases overview.

Let me know what you think.

[Thomas Greiner, 2016-02-05 14:24:37]

It's better than scraping the HTML content.

Given the complexity of the implementation, the low significance of the feature
and the sufficient usefulness of simply linking to the releases blog, I'd prefer
not to go forward with this approach.

https://codereview.adblockplus.org/29335794/diff/29335795/messageResponder.js
File messageResponder.js (left):

https://codereview.adblockplus.org/29335794/diff/29335795/messageResponder.js...
messageResponder.js:168: else if (message.what == "addonVersion")
Nice. I wanted to make this more generic in the future anyway so I'm glad that
you feel the same.

https://codereview.adblockplus.org/29335794/diff/29335795/options.js
File options.js (right):

https://codereview.adblockplus.org/29335794/diff/29335795/options.js#newcode655
options.js:655:
fetch("https://adblockplus.org/atom/?section=releases&limit=100").then(function(response)
We shouldn't ping our servers everytime someone opens the options page. Maybe we
could do it once for each version to find the corresponding blog post. However,
even then I'm still seeing issues with our privacy policy and with the
reliability of this interface since we'd have to keep this URL alive
indefinitely.

Taking all of that into consideration we'd end up with something like this:

1. User clicks on link
2. We check whether we already know the link to the blog post for this version
   If not:
     a. Fetch list of blog posts
     b. Remove stored link to blog post for previous version (if any)
     c. Determine and store link to the corresponding blog post (preferrably not
dependent on patterns in blog post title or URL)
3. Open blog post

Obviously, that'd still be inferior to having a dynamic redirect on the server
side à la
"https://adblockplus.org/redirect?link=releases&lang=de&application=chrome&addonVersion=1.10.1".

https://codereview.adblockplus.org/29335794/diff/29335795/options.js#newcode671
options.js:671: if (lowerCaseTitleText.split(/\W+/).indexOf(info.application) ==
-1)
That doesn't seem too reliable because it's more of a coincidence that the
application name is part of the blog post title.

[Wladimir Palant, 2016-02-05 14:29:45]

NOT LGTM, see below.

https://codereview.adblockplus.org/29335794/diff/29335795/options.js
File options.js (right):

https://codereview.adblockplus.org/29335794/diff/29335795/options.js#newcode655
options.js:655:
fetch("https://adblockplus.org/atom/?section=releases&limit=100").then(function(response)
On 2016/02/05 14:24:37, Thomas Greiner wrote:
> Obviously, that'd still be inferior to having a dynamic redirect on the server
> side à la
>
"https://adblockplus.org/redirect?link=releases&lang=de&application=chrome&addonVersion=1.10.1".

Indeed. If we need this feature we need to set up a redirect for each release -
everything else would be wrong.

[Sebastian Noack, 2016-02-05 14:37:18]

https://codereview.adblockplus.org/29335794/diff/29335795/options.js
File options.js (right):

https://codereview.adblockplus.org/29335794/diff/29335795/options.js#newcode655
options.js:655:
fetch("https://adblockplus.org/atom/?section=releases&limit=100").then(function(response)
On 2016/02/05 14:29:44, Wladimir Palant wrote:
> On 2016/02/05 14:24:37, Thomas Greiner wrote:
> > Obviously, that'd still be inferior to having a dynamic redirect on the
server
> > side à la
> >
>
"https://adblockplus.org/redirect?link=releases&lang=de&application=chrome&addonVersion=1.10.1".
> 
> Indeed. If we need this feature we need to set up a redirect for each release
-
> everything else would be wrong.

Please note the alternative approach in patch set 2. The request there is only
sent when the link is clicked. With a fallback through a redirect to the release
overview.

This should address all concerns. We wouldn't send a request no matter what, but
only when the user is about to navigate to our website anyway. Also, if for some
reasons the blog will be taken off or moved in the future, or if there simply
isn't an announcement that matches, we fallback to the releases overview through
a redirect by our server.

https://codereview.adblockplus.org/29335794/diff/29335795/options.js#newcode671
options.js:671: if (lowerCaseTitleText.split(/\W+/).indexOf(info.application) ==
-1)
On 2016/02/05 14:24:37, Thomas Greiner wrote:
> That doesn't seem too reliable because it's more of a coincidence that the
> application name is part of the blog post title.

I guess we could omit that check as well. Not sure if that would be more
reliable though.

[Wladimir Palant, 2016-02-05 14:46:13]

Just to explain: I said NOT LGTM not because of the server load (which is also a
concern of course but that's beyond the point). Scraping our Atom feed is a very
bad idea in the first place. We do not now whether we will be using Textpattern
in future (it has its share of issues), we don't know whether we will use it
under the same URL (it really belongs under blog.adblockplus.org or something
along these lines), we don't know whether we will still name/categorize the
release announcements the same in future (as the products converge the current
categorization tends to make less sense). We should not make this kind of
assumptions.

We have our redirector URL on the server, it is meant to receive all incoming
links from the extensions. That's what we should stick to. How we would add the
release links to it is a different question. Worst case: manually, one more
point on the release checklist. On option would be adding a custom field to
Textpattern blog posts to indicate that it is a release announcement - this one
should be added to redirects automatically then. More options might be possible.

[Sebastian Noack, 2016-02-05 15:09:05]

On 2016/02/05 14:46:13, Wladimir Palant wrote:
> Just to explain: I said NOT LGTM not because of the server load (which is also
a
> concern of course but that's beyond the point). Scraping our Atom feed is a
very
> bad idea in the first place. We do not now whether we will be using
Textpattern
> in future (it has its share of issues), we don't know whether we will use it
> under the same URL (it really belongs under http://blog.adblockplus.org or
something
> along these lines), we don't know whether we will still name/categorize the
> release announcements the same in future (as the products converge the current
> categorization tends to make less sense). We should not make this kind of
> assumptions.

Well, with the approach in patch set 2, this wouldn't be a big deal. As
mentioned in my previous comment, if anything changes on the server side, old
builds out there would just fall back to the releases overview or whatever the
server redirects them to. Without this change, this is the behavior, no matter
what.

> We have our redirector URL on the server, it is meant to receive all incoming
> links from the extensions. That's what we should stick to. How we would add
the
> release links to it is a different question. Worst case: manually, one more
> point on the release checklist. On option would be adding a custom field to
> Textpattern blog posts to indicate that it is a release announcement - this
one
> should be added to redirects automatically then. More options might be
possible.

I have to admit, that a redirector script on the server might be more elegant.
So if anything changes on the server side, we can adjust the logic there. If
textpattern has support for custom fields, and to filter programmatically filter
for them in a structured way, even better. Otherwise, I wouldn't mind using the
same logic I implemented here on the server side.