Commit 62cc484d authored by Volker Krause's avatar Volker Krause
Browse files

Give surveys a UUID

This protects tracking of already completed surveys against SQL auto id
reuse, making it safe to delete and re-create surveys.
parent 662f0619
......@@ -59,6 +59,7 @@
</table>
<table name="tbl_survey">
<column>col_id</column>
<column>col_uuid</column>
<column>col_product_id</column>
<column>col_name</column>
<column>col_url</column>
......@@ -66,6 +67,7 @@
<column>col_target</column>
<row>
<value>101</value>
<value>{962bbd80-7120-4f18-a4c0-5800fa323868}</value>
<value>2</value>
<value>myActiveSurvey</value>
<value>http://survey.example/active</value>
......@@ -74,6 +76,7 @@
</row>
<row>
<value>102</value>
<value>{bdfa82c5-238f-404b-a441-07ca3d3eff7f}</value>
<value>2</value>
<value>myInactiveSurvey</value>
<value>http://survey.example/inactive</value>
......
......@@ -29,6 +29,7 @@
#include <QObject>
#include <QSignalSpy>
#include <QStandardPaths>
#include <QUuid>
#include <limits>
......@@ -101,6 +102,7 @@ private slots:
// add new survey
Survey s;
s.setUuid(QUuid::createUuid());
s.setName(QStringLiteral("unitTestSurvey"));
s.setUrl(QUrl(QStringLiteral("http://www.kde.org")));
s.setActive(false);
......@@ -115,8 +117,6 @@ private slots:
QCOMPARE(reply->error(), QNetworkReply::NoError);
surveys = Survey::fromJson(reply->readAll());
QCOMPARE(surveys.size(), 1);
QVERIFY(surveys.at(0) != s); // id is different
s.setId(surveys.at(0).id());
QCOMPARE(surveys.at(0), s);
// update a survey
......@@ -165,7 +165,7 @@ private slots:
QVERIFY(reply->error() != QNetworkReply::NoError);
// update a non-existing survey
s.setId(std::numeric_limits<int>::max());
s.setUuid(QUuid::createUuid());
reply = RESTApi::updateSurvey(&client, s);
QVERIFY(waitForFinished(reply));
qDebug() << reply->readAll();
......
......@@ -29,12 +29,12 @@ class SurveyTest extends AbstractDatastoreTest
"url": "http://survey.example/",
"active": true,
"target": "screen[0].dpi > 100",
"id": 42
"uuid": "{9e529dfa-0213-413e-a1a8-8a9cea7d5a97}"
}');
$this->assertEquals($s->name, 'mySurvey');
$this->assertEquals($s->url, 'http://survey.example/');
$this->assertEquals($s->active, true);
$this->assertEquals($s->id, 42);
$this->assertEquals($s->uuid, '{9e529dfa-0213-413e-a1a8-8a9cea7d5a97}');
$this->assertEquals($s->target, 'screen[0].dpi > 100');
}
......@@ -44,14 +44,14 @@ class SurveyTest extends AbstractDatastoreTest
$s->name = 'mySurvey';
$s->url = 'http://survey.example/';
$s->active = true;
$s->id = 42;
$s->uuid = '{9e529dfa-0213-413e-a1a8-8a9cea7d5a97}';
$s->target = 'screen[0].dpi > 100';
$this->assertJsonStringEqualsJsonString(json_encode($s), '{
"uuid": "{9e529dfa-0213-413e-a1a8-8a9cea7d5a97}",
"name": "mySurvey",
"url": "http://survey.example/",
"active": true,
"id": 42,
"target": "screen[0].dpi > 100"
}');
}
......@@ -101,6 +101,7 @@ class SurveyTest extends AbstractDatastoreTest
{
$json = '{
"name": "newSurvey",
"uuid": "{9e529dfa-0213-413e-a1a8-8a9cea7d5a97}",
"url": "http://survey.example/new",
"active": true,
"target": "startCount.value > 10"
......@@ -111,7 +112,6 @@ class SurveyTest extends AbstractDatastoreTest
$s = Survey::fromJson($json);
$s->insert(self::$db, $p);
$this->assertGreaterThan(0, $s->id);
$surveys = Survey::activeSurveysForProduct(self::$db, $p);
$s = null;
......@@ -126,6 +126,7 @@ class SurveyTest extends AbstractDatastoreTest
$this->assertEquals($s->url, 'http://survey.example/new');
$this->assertEquals($s->active, true);
$this->assertEquals($s->target, 'startCount.value > 10');
$this->assertEquals($s->uuid, '{9e529dfa-0213-413e-a1a8-8a9cea7d5a97}');
}
public function testSurveyUpdate()
......@@ -134,7 +135,7 @@ class SurveyTest extends AbstractDatastoreTest
"name": "myChangedSurvey",
"url": "http://survey.example/changed",
"active": false,
"id": 101,
"uuid": "{962bbd80-7120-4f18-a4c0-5800fa323868}",
"target": "views[\"editor\"].ratio > 0.5"
}';
......@@ -162,7 +163,7 @@ class SurveyTest extends AbstractDatastoreTest
"name": "myInactiveSurvey",
"url": "http://survey.example/inactive",
"active": false,
"id": 102
"uuid": "{bdfa82c5-238f-404b-a441-07ca3d3eff7f}"
}';
$s = Survey::fromJson($json);
......
......@@ -24,6 +24,7 @@
#include <QSharedData>
#include <QString>
#include <QUrl>
#include <QUuid>
using namespace UserFeedback::Console;
......@@ -33,10 +34,10 @@ namespace Console {
class SurveyData : public QSharedData
{
public:
QUuid uuid;
QString name;
QUrl url;
QString target;
int id = -1;
bool active = false;
};
......@@ -53,7 +54,7 @@ bool Survey::operator==(const Survey& other) const
return d->name == other.d->name
&& d->url == other.d->url
&& d->target == other.d->target
&& d->id == other.d->id
&& d->uuid == other.d->uuid
&& d->active == other.d->active;
}
......@@ -62,14 +63,14 @@ bool Survey::operator!=(const Survey& other) const
return !(*this == other);
}
int Survey::id() const
QUuid Survey::uuid() const
{
return d->id;
return d->uuid;
}
void Survey::setId(int id)
void Survey::setUuid(const QUuid &id)
{
d->id = id;
d->uuid = id;
}
QString Survey::name() const
......@@ -115,7 +116,7 @@ void Survey::setTarget(const QString& target)
QByteArray Survey::toJson() const
{
QJsonObject obj;
obj.insert(QStringLiteral("id"), id());
obj.insert(QStringLiteral("uuid"), uuid().toString());
obj.insert(QStringLiteral("name"), name());
obj.insert(QStringLiteral("url"), url().toString());
obj.insert(QStringLiteral("active"), isActive());
......@@ -129,13 +130,7 @@ QVector<Survey> Survey::fromJson(const QByteArray &data)
foreach (const auto &v, QJsonDocument::fromJson(data).array()) {
const auto obj = v.toObject();
Survey survey;
const auto id = obj.value(QLatin1String("id"));
// TODO move this to a helper function
if (id.isDouble())
survey.setId(id.toInt(-1));
else if (id.isString())
survey.setId(id.toString().toInt());
survey.setUuid(QUuid(obj.value(QLatin1String("uuid")).toString()));
survey.setName(obj.value(QLatin1String("name")).toString());
survey.setUrl(QUrl(obj.value(QLatin1String("url")).toString()));
survey.setActive(obj.value(QLatin1String("active")).toBool());
......
......@@ -24,6 +24,7 @@
class QString;
class QUrl;
class QUuid;
namespace UserFeedback {
namespace Console {
......@@ -42,8 +43,8 @@ public:
bool operator==(const Survey &other) const;
bool operator!=(const Survey &other) const;
int id() const;
void setId(int id);
QUuid uuid() const;
void setUuid(const QUuid &id);
QString name() const;
void setName(const QString& name);
......
......@@ -22,6 +22,8 @@
#include <core/sample.h>
#include <core/survey.h>
#include <QUuid>
using namespace UserFeedback::Console;
QNetworkReply* RESTApi::checkSchema(RESTClient *client)
......@@ -78,10 +80,10 @@ QNetworkReply* RESTApi::createSurvey(RESTClient *client, const Product &p, const
QNetworkReply* RESTApi::updateSurvey(RESTClient *client, const Survey &s)
{
return client->put(QStringLiteral("surveys/") + QString::number(s.id()), s.toJson());
return client->put(QStringLiteral("surveys/") + s.uuid().toString(), s.toJson());
}
QNetworkReply* RESTApi::deleteSurvey(RESTClient *client, const Survey &s)
{
return client->deleteResource(QStringLiteral("surveys/") + QString::number(s.id()));
return client->deleteResource(QStringLiteral("surveys/") + s.uuid().toString());
}
......@@ -25,6 +25,7 @@
#include <QIcon>
#include <QPushButton>
#include <QUrl>
#include <QUuid>
using namespace UserFeedback::Console;
......@@ -51,6 +52,8 @@ Survey SurveyDialog::survey() const
s.setName(ui->name->text());
s.setUrl(QUrl(ui->url->text()));
s.setTarget(ui->targetExpression->toPlainText());
if (s.uuid().isNull())
s.setUuid(QUuid::createUuid());
return s;
}
......
......@@ -25,6 +25,7 @@
#include <QMessageBox>
#include <QNetworkReply>
#include <QUuid>
using namespace UserFeedback::Console;
......@@ -117,7 +118,7 @@ void SurveyEditor::deleteSurvey()
return;
const auto survey = selection.first().data(SurveyModel::SurveyRole).value<Survey>();
if (survey.id() < 0)
if (survey.uuid().isNull())
return;
auto reply = RESTApi::deleteSurvey(m_restClient, survey);
connect(reply, &QNetworkReply::finished, this, [this, reply]() {
......
......@@ -41,6 +41,7 @@
#include <QNetworkRequest>
#include <QSettings>
#include <QUrl>
#include <QUuid>
#include <algorithm>
#include <numeric>
......@@ -308,7 +309,7 @@ QVariant ProviderPrivate::sourceData(const QString& sourceName) const
bool ProviderPrivate::selectSurvey(const SurveyInfo &survey) const
{
qCDebug(Log) << "got survey:" << survey.url() << survey.target();
if (!survey.isValid() || completedSurveys.contains(QString::number(survey.id())))
if (!survey.isValid() || completedSurveys.contains(survey.uuid().toString()))
return false;
if (lastSurveyTime.addDays(surveyInterval) > QDateTime::currentDateTime())
......@@ -511,7 +512,7 @@ void Provider::setEncouragementInterval(int days)
void Provider::setSurveyCompleted(const SurveyInfo &info)
{
d->completedSurveys.push_back(QString::number(info.id()));
d->completedSurveys.push_back(info.uuid().toString());
d->lastSurveyTime = QDateTime::currentDateTime();
auto s = d->makeSettings();
......
......@@ -22,24 +22,18 @@
#endif
#include <QSharedData>
#include <QUrl>
#include <QUuid>
using namespace UserFeedback;
class UserFeedback::SurveyInfoData : public QSharedData
{
public:
SurveyInfoData();
int id;
QUuid uuid;
QUrl url;
QString target;
};
SurveyInfoData::SurveyInfoData()
: id(-1)
{
}
SurveyInfo::SurveyInfo() : d (new SurveyInfoData)
{
......@@ -53,17 +47,17 @@ SurveyInfo & SurveyInfo::operator=(const SurveyInfo&) = default;
bool SurveyInfo::isValid() const
{
return d->id >= 0 && d->url.isValid();
return !d->uuid.isNull() && d->url.isValid();
}
int SurveyInfo::id() const
QUuid SurveyInfo::uuid() const
{
return d->id;
return d->uuid;
}
void SurveyInfo::setId(int id)
void SurveyInfo::setUuid(const QUuid &id)
{
d->id = id;
d->uuid = id;
}
QUrl SurveyInfo::url() const
......@@ -90,7 +84,7 @@ SurveyInfo SurveyInfo::fromJson(const QJsonObject& obj)
{
SurveyInfo s;
#if QT_VERSION >= QT_VERSION_CHECK(5, 0, 0)
s.setId(obj.value(QLatin1String("id")).toInt());
s.setUuid(obj.value(QLatin1String("uuid")).toString());
s.setUrl(QUrl(obj.value(QLatin1String("url")).toString()));
s.setTarget(obj.value(QLatin1String("target")).toString());
#endif
......
......@@ -22,8 +22,11 @@
#include <QSharedDataPointer>
QT_BEGIN_NAMESPACE
class QJsonObject;
class QUrl;
class QUuid;
QT_END_NAMESPACE
namespace UserFeedback {
......@@ -46,10 +49,10 @@ public:
/*! Returns @c true if this survey has all necessary information to actually execute it. */
bool isValid() const;
/*! Internal unique id of the survey.
/*! Internal global unique id of the survey.
* Used to locally check if a user has completed the survey already.
*/
int id() const;
QUuid uuid() const;
/*! The URL to the survey website. */
QUrl url() const;
......@@ -58,7 +61,7 @@ public:
QString target() const;
///@cond internal
void setId(int id);
void setUuid(const QUuid &id);
void setUrl(const QUrl &url);
void setTarget(const QString &target);
static SurveyInfo fromJson(const QJsonObject &obj);
......
......@@ -154,13 +154,14 @@ public function post_surveys($productName)
/** Edit an existing survey. */
public function put_surveys($surveyId)
{
$surveyId = intval($surveyId);
if ($surveyId < 0)
Utils::httpError(400, "Invalid survey id.");
$surveyId = strval($surveyId);
if (strlen($surveyId) <= 0)
throw new RESTException('Invalid survey id.', 400);
$surveyData = file_get_contents('php://input');
$survey = Survey::fromJson($surveyData);
$survey->id = $surveyId;
$survey->uuid = $surveyId;
error_log("SURVEY UPDATE UUID:" . $surveyId);
$db = new DataStore();
$db->beginTransaction();
......@@ -173,9 +174,9 @@ public function put_surveys($surveyId)
public function delete_surveys($surveyId)
{
$survey = new Survey;
$survey->id = intval($surveyId);
if ($survey->id < 0)
Utils::httpError(400, "Invalid survey id.");
$survey->uuid = strval($surveyId);
if (strlen($survey->uuid) <= 0)
throw new RESTException('Invalid survey id.', 400);
$db = new DataStore();
$db->beginTransaction();
......
......@@ -44,6 +44,11 @@
"version": 7,
"sql": ["ALTER TABLE tbl_survey ADD COLUMN col_target VARCHAR"],
"mysql": ["ALTER TABLE tbl_survey ADD COLUMN col_target VARCHAR(255)"]
},
{
"version": 8,
"sql": ["ALTER TABLE tbl_survey ADD COLUMN col_uuid VARCHAR(40)"]
}
]
}
......@@ -23,17 +23,18 @@ require_once('restexception.php');
/** Represents a survey. */
class Survey
{
public $uuid = '';
public $name = '';
public $url = '';
public $id = -1;
public $active = false;
public $target = null;
private $id = -1;
/** Returns an array of all surveys for @p productName. */
public function surveysForProduct(Datastore $db, $productName)
{
$stmt = $db->prepare('
SELECT tbl_survey.col_id, tbl_survey.col_name, tbl_survey.col_url, tbl_survey.col_active, tbl_survey.col_target
SELECT tbl_survey.col_id, tbl_survey.col_uuid, tbl_survey.col_name, tbl_survey.col_url, tbl_survey.col_active, tbl_survey.col_target
FROM tbl_survey JOIN tbl_product ON (tbl_survey.col_product_id = tbl_product.col_id)
WHERE tbl_product.col_name = :productName
');
......@@ -44,6 +45,7 @@ class Survey
foreach ($stmt as $row) {
$s = new Survey();
$s->id = intval($row['col_id']);
$s->uuid = strval($row['col_uuid']);
$s->name = strval($row['col_name']);
$s->url = strval($row['col_url']);
$s->active = boolval($row['col_active']);
......@@ -57,7 +59,7 @@ class Survey
/** Returns an array of all active surveys for @p product. */
public function activeSurveysForProduct(Datastore $db, Product $product)
{
$stmt = $db->prepare('SELECT col_id, col_name, col_url, col_target FROM tbl_survey WHERE col_product_id = :productId AND col_active = :active');
$stmt = $db->prepare('SELECT col_id, col_uuid, col_name, col_url, col_target FROM tbl_survey WHERE col_product_id = :productId AND col_active = :active');
$stmt->bindValue(':productId', $product->id(), PDO::PARAM_INT);
$stmt->bindValue(':active', true, PDO::PARAM_BOOL);
$db->execute($stmt);
......@@ -66,6 +68,7 @@ class Survey
foreach ($stmt as $row) {
$s = new Survey();
$s->id = intval($row['col_id']);
$s->uuid = strval($row['col_uuid']);
$s->name = strval($row['col_name']);
$s->url = strval($row['col_url']);
$s->active = true;
......@@ -80,10 +83,11 @@ class Survey
public function insert(Datastore $db, Product $product)
{
$stmt = $db->prepare('
INSERT INTO tbl_survey (col_product_id, col_name, col_url, col_active, col_target)
VALUES (:productId, :name, :url, :active, :target)
INSERT INTO tbl_survey (col_product_id, col_uuid, col_name, col_url, col_active, col_target)
VALUES (:productId, :uuid, :name, :url, :active, :target)
');
$stmt->bindValue(':productId', $product->id(), PDO::PARAM_INT);
$stmt->bindValue(':uuid', $this->uuid, PDO::PARAM_STR);
$stmt->bindValue(':name', $this->name, PDO::PARAM_STR);
$stmt->bindValue(':url', $this->url, PDO::PARAM_STR);
$stmt->bindValue(':active', $this->active, PDO::PARAM_BOOL);
......@@ -101,21 +105,21 @@ class Survey
col_url = :url,
col_active = :active,
col_target = :target
WHERE col_id = :surveyId
WHERE col_uuid = :surveyUuid
');
$stmt->bindValue(':name', $this->name, PDO::PARAM_STR);
$stmt->bindValue(':url', $this->url, PDO::PARAM_STR);
$stmt->bindValue(':active', $this->active, PDO::PARAM_BOOL);
$stmt->bindValue(':target', $this->target, PDO::PARAM_STR);
$stmt->bindValue(':surveyId', $this->id, PDO::PARAM_INT);
$stmt->bindValue(':surveyUuid', $this->uuid, PDO::PARAM_STR);
$db->execute($stmt);
}
/** Delete this existing survey from storage. */
public function delete(Datastore $db)
{
$stmt = $db->prepare('DELETE FROM tbl_survey WHERE col_id = :surveyId');
$stmt->bindValue(':surveyId', $this->id, PDO::PARAM_INT);
$stmt = $db->prepare('DELETE FROM tbl_survey WHERE col_uuid = :surveyUuid');
$stmt->bindValue(':surveyUuid', $this->uuid, PDO::PARAM_STR);
$db->execute($stmt);
}
......@@ -123,10 +127,11 @@ class Survey
public static function fromJson($jsonString)
{
$jsonObj = json_decode($jsonString);
if (!property_exists($jsonObj, 'name') || !property_exists($jsonObj, 'url') || !property_exists($jsonObj, 'active'))
if (!property_exists($jsonObj, 'uuid') || !property_exists($jsonObj, 'name') || !property_exists($jsonObj, 'url') || !property_exists($jsonObj, 'active'))
throw new RESTException('Incomplete survey object.', 400);
$s = new Survey();
$s->uuid = strval($jsonObj->uuid);
$s->name = $jsonObj->name;
$s->url = strval($jsonObj->url);
$s->active = boolval($jsonObj->active);
......@@ -136,6 +141,8 @@ class Survey
$s->id = $jsonObj->id;
// verify
if (strlen($s->uuid) <= 0)
throw new RESTException('Empty survey UUID.', 400);
if (strlen($s->name) <= 0 || !is_string($s->name))
throw new RESTException('Empty product name.', 400);
if (!is_numeric($s->id))
......
......@@ -22,6 +22,7 @@
#include <QApplication>
#include <QPushButton>
#include <QStandardPaths>
#include <QUuid>
#include <QVBoxLayout>
using namespace UserFeedback;
......@@ -48,7 +49,7 @@ int main(int argc, char **argv)
layout->addWidget(button);
QObject::connect(button, &QPushButton::clicked, &provider, [&provider]() {
SurveyInfo info;
info.setId(42);
info.setUuid(QUuid(QStringLiteral("{9e529dfa-0213-413e-a1a8-8a9cea7d5a97}")));
info.setUrl(QUrl(QStringLiteral("https://www.kde.org/")));
emit provider.surveyAvailable(info);
});
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment