[statistics] Fix join ordering; renames and comments

8 files changed, 29 insertions, 17 deletions

diff --git a/modules-available/statistics/api.inc.php b/modules-available/statistics/api.inc.php
index fc0b2afe..7aa6de34 100644
--- a/modules-available/statistics/api.inc.php
+++ b/modules-available/statistics/api.inc.php
@@ -338,7 +338,7 @@ if ($type[0] === '~') {
 					$hwid = $hwids[$screen['name']];
 				} else {
 					$hwid = (int)Database::insertIgnore('statistic_hw', 'hwid',
-						array('hwtype' => DeviceType::SCREEN, 'hwname' => $screen['name']));
+						array('hwtype' => HardwareInfo::SCREEN, 'hwname' => $screen['name']));
 					$hwids[$screen['name']] = $hwid;
 				}
 				// Now add new entries
@@ -384,7 +384,7 @@ if ($type[0] === '~') {
 				Database::exec("UPDATE machine_x_hw x, statistic_hw h
 						SET x.disconnecttime = UNIX_TIMESTAMP()
 						WHERE x.machineuuid = :uuid AND x.hwid = h.hwid AND h.hwtype = :type AND x.disconnecttime = 0",
-					array('uuid' => $uuid, 'type' => DeviceType::SCREEN));
+					array('uuid' => $uuid, 'type' => HardwareInfo::SCREEN));
 			} else {
 				// Some screens connected, make sure old entries get removed
 				Database::exec("UPDATE machine_x_hw x, statistic_hw h
@@ -393,7 +393,7 @@ if ($type[0] === '~') {
 						AND x.machineuuid = :uuid", array(
 					'pairs' => $keepPair,
 					'uuid' => $uuid,
-					'type' => DeviceType::SCREEN,
+					'type' => HardwareInfo::SCREEN,
 				));
 			}
 		}
diff --git a/modules-available/statistics/hooks/config-tgz.inc.php b/modules-available/statistics/hooks/config-tgz.inc.php
index 0a43e115..b732fd7a 100644
--- a/modules-available/statistics/hooks/config-tgz.inc.php
+++ b/modules-available/statistics/hooks/config-tgz.inc.php
@@ -4,7 +4,7 @@ $res = Database::simpleQuery('SELECT h.hwname FROM statistic_hw h'
 	. " INNER JOIN statistic_hw_prop p ON (h.hwid = p.hwid AND p.prop = :projector)"
 	. " WHERE h.hwtype = :screen ORDER BY h.hwname ASC", array(
 	'projector' => 'projector',
-	'screen' => DeviceType::SCREEN,
+	'screen' => HardwareInfo::SCREEN,
 ));
 
 if ($res !== false) { // CHeck this in case we're running on old DB during update
diff --git a/modules-available/statistics/inc/devicetype.inc.php b/modules-available/statistics/inc/devicetype.inc.php
deleted file mode 100644
index a01ec310..00000000
--- a/modules-available/statistics/inc/devicetype.inc.php
+++ /dev/null
@@ -1,7 +0,0 @@
-<?php
-
-class DeviceType
-{
-	const SCREEN = 'SCREEN';
-
-}
diff --git a/modules-available/statistics/inc/hardwareinfo.inc.php b/modules-available/statistics/inc/hardwareinfo.inc.php
index 9ccf88be..6ca070ee 100644
--- a/modules-available/statistics/inc/hardwareinfo.inc.php
+++ b/modules-available/statistics/inc/hardwareinfo.inc.php
@@ -12,6 +12,7 @@ class HardwareInfo
 	const PCI_DEVICE = 'PCI_DEVICE';
 	const HDD = 'HDD';
 	const CPU = 'CPU';
+	const SCREEN = 'SCREEN';
 
 	/**
 	 * Get a KCL modification string for the given machine, enabling GVT, PCI passthrough etc.
diff --git a/modules-available/statistics/inc/hardwarequery.inc.php b/modules-available/statistics/inc/hardwarequery.inc.php
index 525e05e8..00a9bbc9 100644
--- a/modules-available/statistics/inc/hardwarequery.inc.php
+++ b/modules-available/statistics/inc/hardwarequery.inc.php
@@ -131,10 +131,11 @@ class HardwareQuery
 	 */
 	public function buildQuery(string $groupBy = ''): string
 	{
+		$groupConcat = !empty($groupBy) && $groupBy !== 'mxhw.machinehwid';
 		$columns = [];
 		foreach ($this->columns as $column) {
 			if ($column instanceof HardwareQueryColumn) {
-				$column->generate($this->joins, $columns, $this->args);
+				$column->generate($this->joins, $columns, $this->args, $groupConcat);
 			} else {
 				$columns[] = $column;
 			}
diff --git a/modules-available/statistics/inc/hardwarequerycolumn.inc.php b/modules-available/statistics/inc/hardwarequerycolumn.inc.php
index 71793008..d073530b 100644
--- a/modules-available/statistics/inc/hardwarequerycolumn.inc.php
+++ b/modules-available/statistics/inc/hardwarequerycolumn.inc.php
@@ -11,6 +11,7 @@ class HardwareQueryColumn
 	private $alias;
 	private $conditions = [];
 	private $params = [];
+	private $classId;
 
 	private static function getId(): string
 	{
@@ -19,13 +20,19 @@ class HardwareQueryColumn
 
 	public function __construct(bool $global, string $column, string $alias = null)
 	{
+		$this->classId = ++self::$id;
 		$this->global = $global;
 		$this->tableAlias = self::getId();
 		$this->virtualColumnName = $column;
 		$this->alias = $alias ?? $column;
 	}
 
-	public function generate(array &$joins, array &$columns, array &$params)
+	/**
+	 * Add necessary conditions, joins, columns to final SQL arrays. To be called
+	 * from HardwareQuery::buildQuery().
+	 * @param bool $groupConcat whether to add distinct GROUP_CONCAT to column.
+	 */
+	public function generate(array &$joins, array &$columns, array &$params, bool $groupConcat)
 	{
 		if ($this->global) {
 			$srcTable = 'shw';
@@ -46,7 +53,11 @@ class HardwareQueryColumn
 		// INNER JOIN, so the result will be empty if the condition doesn't match.
 		$type = count($this->conditions) === 1 ? 'LEFT' : 'INNER';
 		$joins[] = "$type JOIN $table $tid ON (" . implode(' AND ', $this->conditions) . ")";
-		$columns[] = "$tid.`value` AS `{$this->alias}`";
+		if ($groupConcat) {
+			$columns[] = "Group_Concat(DISTINCT $tid.`value` SEPARATOR ', ') AS `{$this->alias}`";
+		} else {
+			$columns[] = "$tid.`value` AS `{$this->alias}`";
+		}
 		$params += $this->params;
 	}
 
@@ -60,7 +71,13 @@ class HardwareQueryColumn
 	{
 		$valueCol = ($op === '<' || $op === '>' || $op === '<=' || $op === '>=') ? 'numeric' : 'value';
 		if ($other instanceof HardwareQueryColumn) {
-			$this->conditions[] = "{$this->tableAlias}.`$valueCol` $op {$other->tableAlias}.`$valueCol`";
+			$cond = "{$this->tableAlias}.`$valueCol` $op {$other->tableAlias}.`$valueCol`";
+			// Don't reference a column of a table that hasn't been joined yet
+			if ($this->classId > $other->classId) {
+				$this->conditions[] = $cond;
+			} else {
+				$other->conditions[] = $cond;
+			}
 		} else {
 			$pid = self::getId();
 			$this->conditions[] = "{$this->tableAlias}.`$valueCol` $op (:$pid)";
diff --git a/modules-available/statistics/pages/machine.inc.php b/modules-available/statistics/pages/machine.inc.php
index 019b0d8d..6ea0ce28 100644
--- a/modules-available/statistics/pages/machine.inc.php
+++ b/modules-available/statistics/pages/machine.inc.php
@@ -229,7 +229,7 @@ class SubPage
 			. " LEFT JOIN machine_x_hw_prop p ON (m.machinehwid = p.machinehwid AND p.prop = 'resolution')"
 			. " LEFT JOIN statistic_hw_prop q ON (m.hwid = q.hwid AND q.prop = 'projector')"
 			. " WHERE m.machineuuid = :uuid",
-			array('screen' => DeviceType::SCREEN, 'uuid' => $uuid));
+			array('screen' => HardwareInfo::SCREEN, 'uuid' => $uuid));
 		$client['screens'] = array();
 		$ports = array();
 		foreach ($res as $row) {
diff --git a/modules-available/statistics/pages/projectors.inc.php b/modules-available/statistics/pages/projectors.inc.php
index 97a21ebd..91abfe9c 100644
--- a/modules-available/statistics/pages/projectors.inc.php
+++ b/modules-available/statistics/pages/projectors.inc.php
@@ -49,7 +49,7 @@ class SubPage
 			. " INNER JOIN statistic_hw_prop p ON (h.hwid = p.hwid AND p.prop = :projector)"
 			. " WHERE h.hwtype = :screen ORDER BY h.hwname ASC", array(
 			'projector' => 'projector',
-			'screen' => DeviceType::SCREEN,
+			'screen' => HardwareInfo::SCREEN,
 		));
 		$data = array(
 			'projectors' => $res->fetchAll()