Nice work gesha.
I have one general proposal: The license text should be extracted from
the build info file and put in the separate file in the same directory
as the build info file.
Few comments on the code also:
>
> @@ -102,4 +102,5 @@
> RewriteCond %{REQUEST_URI} !^/licenses/.*$
> RewriteCond %{ENV:LP_DOWNLOAD_DIR}/EULA.txt !-f
> RewriteCond %{ENV:LP_DOWNLOAD_DIR}/OPEN-EULA.txt !-f
> +RewriteCond %{ENV:LP_DOWNLOAD_DIR}/BUILD-INFO.txt !-f
> RewriteRule .* - [R=403,L]
A comment above this block should be updated to match the new Build info
change.
> === added file 'licenses/BuildInfo.php'
> --- licenses/BuildInfo.php 1970-01-01 00:00:00 +0000
> +++ licenses/BuildInfo.php 2012-05-28 09:45:23 +0000
> @@ -0,0 +1,151 @@
> + +
> +class BuildInfo
> +{
> + private $text_array = array();
> + private $fields_defined = array("Format-Version", "Files-Pattern",
> + "Build-Name", "Theme", "License-Type", "OpenID-Launchpad-Teams",
> + "Collect-User-Data", "License-Text");
> + private $multiline_vars = array("License-Text");
> + private $search_path = '';
> +
I like using __construct method and do all the class attribute setup in
there, that way is more readable.
Also, since your build file is something always unique to the build info
instance, you can make it an attribute of the class, and then just
assign it during class initialization. Something like:
$bi = BuildInfo("build_info.txt");
$bi->readFile();
...
> + public function readFile($fn)
> + {
> + $this->search_path = dirname($fn);
> + $field = '';
> + $fp_read = 0;
> + if (is_dir($fn) or !is_file($fn) or filesize($fn) == 0) return false; is
> + $file = fopen($fn, "r") or exit("Unable to open file $fn!");
There are exceptions in PHP as well, I started using them in the project
so I think it should be good idea that we stick with that.
> + // Get the 'Format-Version' field.
> + $line = fgets($file);
> + $fields = explode(":", $line, "2");
This is good, you ensure that second and any other ":" char is ignored.
...
> + // Get the rest fileds.
> + while(!feof($file)) {
> + if ($fp_read) {
> + print_r($fields);
Is this some leftover debugging? Please remove.
> + $this->text_array[$fields[0]] = trim($fields[1]);
> + $fp_read = 0;
> + }
> + $line = fgets($file);
> + if (trim($line) == "")
> + continue;
> + $fields = explode(":", $line, "2");
> + if ($fields[0] == "Files-Pattern") {
> + $tmp_arr = array();
> + $fp = trim($fields[1]);
> + while(!feof($file)) {
> + $line = fgets($file);
> + if (trim($line) == "")
> + continue;
> + $fields = explode(":", $line, "2");
> + if (in_array($fields[0], $this->multiline_vars)) {
> + $field = $fields[0];
> + if (isset($fields[1]))
> + $tmp_arr[$fields[0]] = trim($fields[1]);
> + while(!feof($file)) {
> + $line = fgets($file);
> + if (trim($line) == "")
> + continue;
> + $fields = explode(":", $line, "2");
> + if(in_array($fields[0], $this->fields_defined)) {
> + if ($fields[0] == "Files-Pattern") {
> + fseek($file, -(strlen($line)), SEEK_CUR);
> + break 2;
> + }
> + break;
> + }
> + $tmp_arr[$field] = $tmp_arr[$field].
> + "\n".rtrim($line);
> + }
> +
> + }
> + if (isset($fields[1])) {
> + $tmp_arr[$fields[0]] = trim($fields[1]);
> + }
> + }
> + $this->text_array[$fp] = $tmp_arr;
> + unset($tmp_arr);
> + }
> + }
> + fclose($file);
> +
> + return true;
> + }
> +
First, the code above is not commented at all and was very hard to read.
I think this will change anyway if you apply the license text extraction
from file so I will not comment it further just now. Three nested
'while' loops certainly don't look good when you basically need to go
through file only once.
> + private function getInfoForFile($fname) {
> + foreach (array_keys($this->text_array) as $key)
> + if ($key != 'Format-Version') {
> + $files = glob($this->search_path."/".$key);
> + foreach ($files as $file)
> + if ($file == $this->search_path."/".$fname)
> + return $this->text_array[$key];
> + }
> + return array();
> + }
> +
> + public function getFormatVersion()
> + {
> + if (array_key_exists('Format-Version', $this->text_array))
> + return $this->text_array["Format-Version"];
> + else
> + return false;
> + }
> +
> + public function getBuildName($fname)
> + {
> + $info = $this->getInfoForFile($fname);
> + if (array_key_exists('Build-Name', $info))
> + return $info["Build-Name"];
> + else
> + return false;
> + }
> +
> + public function getTheme($fname)
> + {
> + $info = $this->getInfoForFile($fname);
> + if (array_key_exists('Theme', $info))
> + return $info["Theme"];
> + else
> + return false;
> + }
> +
> + public function getLicenseType($fname)
> + {
> + $info = $this->getInfoForFile($fname);
> + if (array_key_exists('License-Type', $info))
> + return $info["License-Type"];
> + else
> + return false;
> + }
> +
> + public function getLaunchpadTeams($fname)
> + {
> + $info = $this->getInfoForFile($fname);
> + if (array_key_exists('OpenID-Launchpad-Teams', $info))
> + return $info["OpenID-Launchpad-Teams"];
> + else
> + return false;
> + }
> +
> + public function getCollectUserData($fname)
> + {
> + $info = $this->getInfoForFile($fname);
> + if (array_key_exists('Collect-User-Data', $info))
> + return $info["Collect-User-Data"];
> + else
> + return false;
> + }
> +
> + public function getLicenseText($fname)
> + {
> + $info = $this->getInfoForFile($fname);
> + if (array_key_exists('License-Text', $info))
> + return $info["License-Text"];
> + else
> + return false;
> + }
> +}
>
> === modified file 'licenses/license.php'
> --- licenses/license.php 2012-05-11 08:23:42 +0000
> +++ licenses/license.php 2012-05-28 09:45:23 +0000
> @@ -1,6 +1,7 @@
>
> require_once("LicenseHelper.php");
> +require_once("BuildInfo.php");
>
> $down = $_COOKIE["downloadrequested"];
> $host = $_SERVER["HTTP_HOST"];
> @@ -9,55 +10,79 @@
> $fn = $doc.$down; // Filename on server
> $flist = array();
> $eula = '';
> +$bi_found = 0;
>
> if (preg_match("/.*openid.*/", $fn) or preg_match("/.*restricted.*/", $fn) or preg_match("/.*private.*/", $fn)) {
> - LicenseHelper::redirect_with_status($down, $domain, 200);
> + LicenseHelper::redirect_with_status($down, $domain, 200);
> }
>
> if (file_exists($fn) and LicenseHelper::checkFile($fn)) { // Requested download is file
> - $search_dir = dirname($fn);
> - $repl = dirname($down);
> - $name_only = array(basename($down), '');
> + $search_dir = dirname($fn);
> + $repl = dirname($down);
> + $name_only = array(basename($down), '');
> } elseif (is_dir($fn)) { // Requested download is directory
> - $search_dir = $fn;
> - $repl = $down;
> - $name_only = array();
> + $search_dir = $fn;
> + $repl = $down;
> + $name_only = array();
> } else { // Requested download not found on server
> - LicenseHelper::redirect_with_status($down, $domain, 404);
> + LicenseHelper::redirect_with_status($down, $domain, 404);
> }
>
> -$flist = LicenseHelper::getFilesList($search_dir);
> -
> -if (!empty($name_only)) {
> - $pattern = "/^".$name_only[0]."\.EULA\.txt.*/";
> - $eula = LicenseHelper::findFileByPattern($flist, $pattern);
> +if (file_exists($search_dir."/BUILD-INFO.txt")) {
> + $bi_found = 1;
> + $bi = new BuildInfo();
> + $bi->readFile($search_dir."/BUILD-INFO.txt");
> + $theme = $bi->getTheme($name_only[0]);
> + $lic_type = $bi->getLicenseType($name_only[0]);
> + $lic_text = $bi->getLicenseText($name_only[0]);
> +} else {
> + $flist = LicenseHelper::getFilesList($search_dir);
> + if (!empty($name_only)) {
> + $pattern = "/^".$name_only[0]."\.EULA\.txt.*/";
> + $eula = LicenseHelper::findFileByPattern($flist, $pattern);
> + }
> }
>
> if (LicenseHelper::checkFile($fn)) {
> - if (LicenseHelper::checkFile($doc."/".$repl."/".$eula)) { // Special EULA found
> - $theme = LicenseHelper::getTheme($eula, $down);
> - } elseif (LicenseHelper::checkFile($doc."/".$repl."/EULA.txt")) { // No special EULA found
> - $theme = LicenseHelper::getTheme("EULA.txt", $down);
> - } elseif (LicenseHelper::findFileByPattern($flist, "/.*EULA.txt.*/")) {
> - // If file is requested but no special EULA for it and no EULA.txt is present,
> - // look for any EULA and if found decide that current file is not protected.
> - LicenseHelper::redirect_with_status($down, $domain, 200);
> - } else {
> - LicenseHelper::redirect_with_status($down, $domain, 403);
> - }
> + if ($bi_found) {
> + if ($lic_type == 'open')
> + LicenseHelper::redirect_with_status($down, $domain, 200);
> + elseif (($theme != false) or ($lic_text != false))
> + $template_content = file_get_contents($doc."/licenses/".$theme.".html");
> + else
> + LicenseHelper::redirect_with_status($down, $domain, 403);
> + } else {
> + if (LicenseHelper::checkFile($doc."/".$repl."/".$eula)) {
> + // Special EULA found
> + $theme = LicenseHelper::getTheme($eula, $down);
> + } elseif (LicenseHelper::checkFile($doc."/".$repl."/EULA.txt")) {
> + // No special EULA found
> + $theme = LicenseHelper::getTheme("EULA.txt", $down);
> + } elseif (LicenseHelper::findFileByPattern($flist, "/.*EULA.txt.*/")) {
> + // If file is requested but no special EULA for it and no EULA.txt
> + // is present, look for any EULA and if found decide that current
> + // file is not protected.
> + LicenseHelper::redirect_with_status($down, $domain, 200);
> + } else {
> + LicenseHelper::redirect_with_status($down, $domain, 403);
> + }
> + $template_content = file_get_contents($doc."/licenses/".$theme.".html");
> + $lic_text = file_get_contents($doc."/licenses/".$theme.".txt");
> + }
> } elseif (is_dir($fn)) {
> - if (empty($flist) or LicenseHelper::findFileByPattern($flist, "/.*EULA.txt.*/")) { // Directory contains only subdirs or any EULA
> - LicenseHelper::redirect_with_status($down, $domain, 200);
> - } else { // No special EULA, no EULA.txt, no OPEN-EULA.txt found
> - LicenseHelper::redirect_with_status($down, $domain, 403);
> - }
> + if (empty($flist)
> + or LicenseHelper::findFileByPattern($flist, "/.*EULA.txt.*/")
> + or $bi_found) {
> + // Directory contains only subdirs or any EULA or BUILD-INFO.txt
> + LicenseHelper::redirect_with_status($down, $domain, 200);
> + } else {
> + // No special EULA, no EULA.txt, no OPEN-EULA.txt found
> + LicenseHelper::redirect_with_status($down, $domain, 403);
> + }
> } else {
> - LicenseHelper::redirect_with_status($down, $domain, 403);
> + LicenseHelper::redirect_with_status($down, $domain, 403);
> }
>
> -$template_content = file_get_contents($doc."/licenses/".$theme.".html");
> -$eula_content = file_get_contents($doc."/licenses/".$theme.".txt");
> -
> -$out = str_replace("EULA.txt", $eula_content, $template_content);
> +$out = str_replace("EULA.txt", $lic_text, $template_content);
> echo $out;
> ?>
Indentation changes makes the diff harder to read. Please keep in mind
to do this kind of work in 2 check-in please.
> === added file 'tests/BUILD-INFO.txt'
> --- tests/BUILD-INFO.txt 1970-01-01 00:00:00 +0000
> +++ tests/BUILD-INFO.txt 2012-05-28 09:45:23 +0000
> @@ -0,0 +1,18 @@
> +Format-Version: 0.1
> +Files-Pattern: *.txt
> +Build-Name: landing-snowball
> +Theme: stericsson
> +License-Type: open
> +OpenID-Launchpad-Teams: linaro,non-linaro
> +Collect-User-Data: yes
> +License-Text: IMPORTANT — PLEASE READ THE FOLLOWING AGREEMENT CAREFULLY.
> +
> + THIS IS A LEGALLY BINDING AGREEMENT BETWEEN YOU, an individual or a
> + legal entity, (“LICENSEE”) AND SAMSUNG ELECTRONICS CO.,
> + LTD. (“SAMSUNG”). BY CLICKING THE "ACCEPT" BUTTON, OR BY DOWNLOADING OR
> + INSTALLING OR OTHERWISE USING THE SOFTWARE, YOU AGREE TO BE BOUND BY THE
> + TERMS OF THIS AGREEMENT. IF YOU DO NOT AGREE TO THE TERMS OF THIS
> + AGREEMENT OR ARE NOT AUTHORISED TO ACCEPT AND AGREE TO THE TERMS OF THE
> + AGREEMENT ON BEHALF OF YOUR LEGAL ENTITY, DO NOT DOWNLOAD, INSTALL OR
> + OTHERWISE USE THE SOFTWARE.
> +
>
> === added file 'tests/BuildInfoTest.php'
> --- tests/BuildInfoTest.php 1970-01-01 00:00:00 +0000
> +++ tests/BuildInfoTest.php 2012-05-28 09:45:23 +0000
> @@ -0,0 +1,124 @@
> + +
> +require_once("licenses/BuildInfo.php");
> +
> +class BuildInfoTest extends PHPUnit_Framework_TestCase
> +{
> +
> + private $temp_filename;
> + private $good_bi;
> + private $bi;
> + private $fname;
> +
> + public function __construct()
> + {
> + $this->good_bi = new BuildInfo();
> + $this->good_bi->readFile("tests/BUILD-INFO.txt");
> + $this->temp_filename = tempnam(sys_get_temp_dir(), "build-info");
> + $this->bi = new BuildInfo();
> + $this->bi->readFile($this->temp_filename);
> + $this->fname = "BUILD-INFO.txt";
> + }
> +
This is good, why wasn't it applied for BuildInfo.php as well?
> + /**
> + * Running readFile on a directory fnameh returns false.
> + */
> + public function test_readFile_nonFile()
> + {
> + $this->assertFalse(BuildInfo::readFile(dirname(__FILE__)));
> + }
> +
> + /**
> + * Running readFile on a nonexistent file returns false.
> + */
> + public function test_readFile_nonexistentFile()
> + {
> + $this->assertFalse(BuildInfo::readFile("nonexistent.file"));
> + }
> +
> + /**
> + * Running readFile on a regular file returns true.
> + */
> + public function test_readFile_file()
> + {
> + $bi = new BuildInfo();
> + $this->assertTrue($bi->readFile("tests/BUILD-INFO.txt"));
> + }
> +
Yes it does. But it also sets the 'text-array' variable with some very
important data. This should be tested as well securing that array is not
empty, the format of the array (it's multidimensional) etc.
> + /**
> + * Running 'get' functions on an empty fields returns false.
> + */
> + public function test_getFormatVersion_empty()
> + {
> + $this->assertFalse($this->bi->getFormatVersion());
> + }
> +
> + /**
> + * Running 'get' functions on non-empty fields returns string value.
> + */
> + public function test_getFormatVersion()
> + {
> + $this->assertInternalType('string', $this->good_bi->getFormatVersion());
> + }
> +
> + public function test_getBuildName_empty()
> + {
> + $this->assertFalse($this->bi->getBuildName($this->fname));
> + }
> +
> + public function test_getBuildName()
> + {
> + $this->assertInternalType('string', $this->good_bi->getBuildName($this->fname));
> + }
> +
> + public function test_getTheme_empty()
> + {
> + $this->assertFalse($this->bi->getTheme($this->fname));
> + }
> +
> + public function test_getTheme()
> + {
> + $this->assertInternalType('string', $this->good_bi->getTheme($this->fname));
> + }
> +
> + public function test_getLicenseType_empty()
> + {
> + $this->assertFalse($this->bi->getLicenseType($this->fname));
> + }
> +
> + public function test_getLicenseType()
> + {
> + $this->assertInternalType('string', $this->good_bi->getLicenseType($this->fname));
> + }
> +
> + public function test_getCollectUserData_empty()
> + {
> + $this->assertFalse($this->bi->getCollectUserData($this->fname));
> + }
> +
> + public function test_getCollectUserData()
> + {
> + $this->assertInternalType('string', $this->good_bi->getCollectUserData($this->fname));
> + }
> +
> + public function test_getLaunchpadTeams_empty()
> + {
> + $this->assertFalse($this->bi->getLaunchpadTeams($this->fname));
> + }
> +
> + public function test_getLaunchpadTeams()
> + {
> + $this->assertInternalType('string', $this->good_bi->getLaunchpadTeams($this->fname));
> + }
> +
> + public function test_getLicenseText_empty()
> + {
> + $this->assertFalse($this->bi->getLicenseText($this->fname));
> + }
> +
> + public function test_getLicenseText()
> + {
> + $this->assertInternalType('string', $this->good_bi->getLicenseText($this->fname));
> + }
> +}
> +?>
This is not enough. Testing that all the parsed values are not empty or
strings will not make sure that our code is 100% working. One solution
is to make a mock BUILD_INFO file and test the exact values you have put
there. Or use the existing one from our trunk if it's possible.
> === added file 'tests/test_build_info.py'
> --- tests/test_build_info.py 1970-01-01 00:00:00 +0000
> +++ tests/test_build_info.py 2012-05-28 09:45:23 +0000
> @@ -0,0 +1,40 @@
> +import os
> +import tempfile
> +import subprocess
> +import xml.etree.ElementTree
> +
> +from testtools import TestCase
> +from testtools.matchers import Equals
> +from testtools.matchers import AllMatch
> +
> +from tests.test_click_through_license import CommandNotFoundException
> +
> +
> +class BuildInfoTest(TestCase):
> + '''Tests for executing the BuildInfo PHP Unit tests'''
> +
> + def setUp(self):
> + super(BuildInfoTest, self).setUp()
> +
> + self.build_info_xml_path = tempfile.mkstemp()[1]
> + if subprocess.Popen(['phpunit', '--log-junit',
> + self.build_info_xml_path, 'tests/BuildInfoTest'],
> + stdout=open('/dev/null', 'w'),
> + stderr=subprocess.STDOUT).wait():
> + raise CommandNotFoundException("phpunit command not found. Please "
> + "install phpunit package and rerun tests.")
> + self.build_info_xml_data = xml.etree.ElementTree.parse(self.build_info_xml_path)
> +
> + def tearDown(self):
> + super(BuildInfoTest, self).tearDown()
> + if os.path.exists(self.build_info_xml_path):
> + os.unlink(self.build_info_xml_path)
> +
> + def test_run_buildinfo_tests(self):
> + self.assertThat(
> + [
> + self.build_info_xml_data.getroot()[0].attrib['failures'],
> + self.build_info_xml_data.getroot()[0].attrib['errors']
> + ],
> + AllMatch(Equals("0"))
> + )
This file should be removed. The 'test_php_unit.py' should contain code
for all the php unit tests whichever class they test. If you have
question how to merge these two, let me know.
> === modified file 'tests/test_click_through_license.py'
> --- tests/test_click_through_license.py 2012-05-17 18:44:59 +0000
> +++ tests/test_click_through_license.py 2012-05-28 09:45:23 +0000
> @@ -36,6 +36,9 @@
> per_file_ste_test_file = '/android/images/snowball-blob.txt'
> per_file_not_protected_test_file = '/android/images/MANIFEST'
> dirs_only_dir = '/android/~linaro-android/'
> +build_info_samsung_test_file = '/android/build-info/origen-blob.txt'
> +build_info_ste_test_file = '/android/build-info/snowball-blob.txt'
> +build_info_not_protected_test_file = '/android/build-info/panda-open.txt'
>
>
> class Contains(object):
> @@ -295,3 +298,25 @@
> search = "Not Found"
> testfile = fetcher.get(host + not_found_test_file)
> self.assertThat(testfile, Contains(search))
> +
> + def test_build_info_non_protected_file(self):
> + search = "This is always available."
> + testfile = fetcher.get(host + build_info_not_protected_test_file)
> + self.assertThat(testfile, Contains(search))
> +
> + def test_build_info_accept_license_samsung_file(self):
> + search = "This is protected with click-through Samsung license."
> + testfile = fetcher.get(host + build_info_samsung_test_file)
> + fetcher.close()
> + if os.path.exists("%s/cookies.txt" % docroot):
> + os.rename("%s/cookies.txt" % docroot,
> + "%s/cookies.samsung" % docroot)
> + self.assertThat(testfile, Contains(search))
> +
> + def test_build_info_accept_license_ste_file(self):
> + search = "This is protected with click-through ST-E license."
> + testfile = fetcher.get(host + build_info_ste_test_file)
> + fetcher.close()
> + if os.path.exists("%s/cookies.txt" % docroot):
> + os.rename("%s/cookies.txt" % docroot, "%s/cookies.ste" % docroot)
> + self.assertThat(testfile, Contains(search))
>
This is ok.