Issue 3312 - Serve release update manifests for Adblock Browser

sitescripts/extensions/web/adblockbrowserUpdates.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2015 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
@@ skipping 31 matching lines @@
 
 """, autoescape=True)
 
 def _get_latest_build(builds_dir):
   latest_build = {"id": 0}
   for json_path in glob.glob(os.path.join(builds_dir, "adblockbrowser-*.json")):
     with open(json_path) as json_file:
       build_id = int(json.loads(json_file.read())["buildid"])
     if build_id > latest_build["id"]:
       latest_build["id"] = build_id
-      apk_path = re.sub(r"\.json$", ".apk", json_path)

[Sebastian Noack, 2015/11/13 22:07:39]

Checking for the file extension here is redundant. How about using
|os.path.splitext(json_path)[0] + ".apk"|?

[Felix Dahlke, 2015/11/14 00:41:05]

Done.

+      apk_path = os.path.splitext(json_path)[0] + ".apk"
       latest_build["path"] = os.path.join(builds_dir, apk_path)
   if latest_build["id"] == 0:
     return {}
   return latest_build
 
-def _render_manifest(build=None, devbuild=False):
+def _render_manifest(build=None, builds_url=None):
   if not build:
     return _MANIFEST_TEMPLATE.render()
 
-  if devbuild:
-    nightlies_url = get_config().get("extensions", "nightliesURL")
-    builds_url = "%s/adblockbrowser" % nightlies_url.rstrip("/")

[Sebastian Noack, 2015/11/13 22:07:39]

rstrip("/") is duplicated. How about moving it below the if/else?

[Felix Dahlke, 2015/11/14 00:41:05]

How would that look? Note that I need to call rstrip on the URL I get from
get_config().get(), only for release builds that's the same as builds_url.

-  else:
-    builds_url = get_config().get("extensions", "downloadsURL").rstrip("/")
   build_url = "%s/%s?update" % (builds_url, os.path.basename(build["path"]))
   with open(build["path"], "rb") as build_file:
     build_content = build_file.read()
   return _MANIFEST_TEMPLATE.render({
     "build": {
       "build_id": build["id"],
       "url": build_url,
       "hash_function": "SHA512",
       "hash_value": hashlib.sha512(build_content).hexdigest(),
       "size": len(build_content)
     }
   })
 
-def _get_update_manifest(current_build_id, devbuild):
-  if devbuild:
-    nightlies_dir = get_config().get("extensions", "nightliesDirectory")
-    builds_dir = os.path.join(nightlies_dir, "adblockbrowser")
-  else:
-    builds_dir = get_config().get("extensions", "downloadsDirectory")
-
+def _get_update_manifest(current_build_id, builds_dir, builds_url):
   if not os.path.isdir(builds_dir):
     return _render_manifest()
 
   latest_build = _get_latest_build(builds_dir)
   if not latest_build or current_build_id >= latest_build["id"]:
     return _render_manifest()
-  return _render_manifest(latest_build, devbuild)
+  return _render_manifest(latest_build, builds_url)
 
-def _handle_request(environ, start_response, devbuild):
+def _handle_request(environ, start_response, builds_dir, builds_url):
   params = parse_qs(environ.get("QUERY_STRING", ""))
   try:
     version = params.get("addonVersion", [""])[0]
     build_id = int(re.search(r"(\d+)$", version).group(1))
   except:
     start_response("400 Processing Error", [("Content-Type", "text/plain")])
     return ["Failed to parse addonVersion."]
-  manifest = _get_update_manifest(build_id, devbuild)
+  manifest = _get_update_manifest(build_id, builds_dir, builds_url)
   response = manifest.encode("utf-8")
   start_response("200 OK", [("Content-Type", "application/xml; charset=utf-8")])
   return [response]
 
 @url_handler("/adblockbrowser/updates.xml")
 def adblockbrowser_updates(environ, start_response):
-  return _handle_request(environ, start_response, False)

[Sebastian Noack, 2015/11/13 22:07:39]

I think the code flow would be simpler when you determine the paths here and
pass them to _handle_request rather than switching the logic inside
_handle_request based on a flag.

[Felix Dahlke, 2015/11/14 00:41:05]

Done.

+  config = get_config()
+
+  builds_dir = config.get("extensions", "downloadsDirectory")
+  builds_url = config.get("extensions", "downloadsURL").rstrip("/")
+
+  return _handle_request(environ, start_response, builds_dir, builds_url)
 
 @url_handler("/devbuilds/adblockbrowser/updates.xml")
 def adblockbrowser_devbuild_updates(environ, start_response):
-  return _handle_request(environ, start_response, True)
+  config = get_config()
+
+  nightlies_dir = config.get("extensions", "nightliesDirectory")
+  builds_dir = os.path.join(nightlies_dir, "adblockbrowser")
+
+  nightlies_url = config.get("extensions", "nightliesURL").rstrip("/")
+  builds_url = "%s/adblockbrowser" % nightlies_url
+
+  return _handle_request(environ, start_response, builds_dir, builds_url)