Merge pull request #86 from matty0ung/dbFixes

Db fixes etc
Brewken · Oct 1, 2024 · 28f5738 · 28f5738
2 parents c600e4b + e3dbc4c
commit 28f5738
Show file tree

Hide file tree

Showing 115 changed files with 2,580 additions and 2,432 deletions.
diff --git a/images/iconMash.svg b/images/iconMash.svg
diff --git a/resources.qrc b/resources.qrc
@@ -82,6 +82,7 @@
       <file alias="images/grain2glass.svg"           >images/grain2glass.svg</file>
       <file alias="images/help-contents.png"         >images/help-contents.png</file>
       <file alias="images/hydrometer.svg"            >images/hydrometer.svg</file>
+      <file alias="images/iconMash.svg"              >images/iconMash.svg</file>
       <file alias="images/kbruch.png"                >images/kbruch.png</file>
       <file alias="images/mashpaddle.svg"            >images/mashpaddle.svg</file>
       <file alias="images/merge.png"                 >images/merge.png</file>

diff --git a/src/MainWindow.cpp b/src/MainWindow.cpp
@@ -1671,7 +1671,7 @@ void MainWindow::treeActivated(const QModelIndex &index) {
             {
                Water * w = active->getItem<Water>(index);
                if (w) {
-                  this->pimpl->m_waterEditor->setWater(ObjectStoreWrapper::getSharedFromRaw(w));
+                  this->pimpl->m_waterEditor->setEditItem(ObjectStoreWrapper::getSharedFromRaw(w));
                   this->pimpl->m_waterEditor->show();
                }
             }
@@ -2506,15 +2506,11 @@ void MainWindow::editYeastOfSelectedYeastAddition() {
    return;
 }
 
-void MainWindow::newRecipe()
-{
-   QString name = QInputDialog::getText(this, tr("Recipe name"),
-                                          tr("Recipe name:"));
-   QVariant defEquipKey = PersistentSettings::value(PersistentSettings::Names::defaultEquipmentKey, -1);
-   QObject* selection = sender();
-
-   if( name.isEmpty() )
+void MainWindow::newRecipe() {
+   QString const name = QInputDialog::getText(this, tr("Recipe name"), tr("Recipe name:"));
+   if (name.isEmpty()) {
       return;
+   }
 
    std::shared_ptr<Recipe> newRec = std::make_shared<Recipe>(name);
 
@@ -2526,17 +2522,31 @@ void MainWindow::newRecipe()
       return;
    }
    ObjectStoreWrapper::insert(newRec);
-   std::shared_ptr<Boil> newBoil = std::make_shared<Boil>(QString("Boil for").arg(name));
+
+   //
+   // .:TODO:. For the moment, we still assume that every Recipe has a Boil and a Fermentation.  Also, for the moment,
+   // we also create a new boil and fermentation for every Recipe.  In time, I'd like to extend the UI so that, when you
+   // input the name for your new Recipe, you also select Mash, Boil, Fermentation (all either from "List of existing"
+   // or "Make new").
+   //
+   std::shared_ptr<Boil> newBoil = std::make_shared<Boil>(tr("Automatically-created Boil for %1").arg(name));
+   // NB: Recipe::setBoil will ensure Boil is stored in the database
    newRec->setBoil(newBoil);
-   ObjectStoreWrapper::insert(newBoil);
+   // Since we're auto-creating a Boil, it might as well start out with the "standard" profile
+   newBoil->ensureStandardProfile();
+
+   std::shared_ptr<Fermentation> newFermentation = std::make_shared<Fermentation>(tr("Automatically-created Fermentation for %1").arg(name));
+   // NB: Recipe::setFermentation will ensure Fermentation is stored in the database
+   newRec->setFermentation(newFermentation);
 
    // Set the following stuff so everything appears nice
    // and the calculations don't divide by zero... things like that.
    newRec->setBatchSize_l(18.93); // 5 gallons
    newBoil->setPreBoilSize_l(23.47);  // 6.2 gallons
    newRec->setEfficiency_pct(70.0);
 
-   // we need a valid key, so insert the recipe before we add equipment
+   // We need a valid key, so insert the recipe before we add equipment
+   QVariant const defEquipKey = PersistentSettings::value(PersistentSettings::Names::defaultEquipmentKey, -1);
    if (defEquipKey != -1) {
       auto equipment = ObjectStoreWrapper::getById<Equipment>(defEquipKey.toInt());
       // I really want to do this before we've written the object to the
@@ -2549,8 +2559,9 @@ void MainWindow::newRecipe()
       }
    }
 
-   // a new recipe will be put in a folder if you right click on a recipe or
+   // A new recipe will be put in a folder if you right click on a recipe or
    // folder. Otherwise, it goes into the main window?
+   QObject* selection = this->sender();
    if (selection) {
       TreeView* sent = qobject_cast<TreeView*>(tabWidget_Trees->currentWidget()->focusWidget());
       if (sent) {

diff --git a/src/WaterDialog.cpp b/src/WaterDialog.cpp
@@ -294,12 +294,12 @@ void WaterDialog::setRecipe(Recipe *rec) {
       spinBox_spargeRO->setValue( QVariant(m_spargeRO * 100).toInt());
 
       baseProfileButton->setWater(this->m_base);
-      m_base_editor->setWater(this->m_base);
+      m_base_editor->setEditItem(this->m_base);
       // all of the magic to set the sliders happens in newTotals(). So don't do it twice
    }
    if (this->m_target && this->m_target != this->m_base) {
       targetProfileButton->setWater(this->m_target);
-      m_target_editor->setWater(this->m_target);
+      m_target_editor->setEditItem(this->m_target);
 
       this->setDigits();
    }
@@ -327,7 +327,7 @@ void WaterDialog::update_baseProfile(int selected) {
       qDebug() << Q_FUNC_INFO << "Made base child" << *this->m_base << "from parent" << parent;
 
       baseProfileButton->setWater(this->m_base);
-      m_base_editor->setWater(this->m_base);
+      m_base_editor->setEditItem(this->m_base);
       newTotals();
    }
    return;
@@ -351,7 +351,7 @@ void WaterDialog::update_targetProfile(int selected) {
       qDebug() << Q_FUNC_INFO << "Made target child" << *this->m_target << "from parent" << parent;
 
       targetProfileButton->setWater(this->m_target);
-      m_target_editor->setWater(this->m_target);
+      m_target_editor->setEditItem(this->m_target);
 
       this->setDigits();
    }

diff --git a/src/database/DatabaseSchemaHelper.cpp b/src/database/DatabaseSchemaHelper.cpp
@@ -844,7 +844,7 @@ namespace {
          {QString("ALTER TABLE water ADD COLUMN iron_ppm      %1").arg(db.getDbNativeTypeName<double>())},
          {QString("ALTER TABLE water ADD COLUMN nitrate_ppm   %1").arg(db.getDbNativeTypeName<double>())},
          {QString("ALTER TABLE water ADD COLUMN nitrite_ppm   %1").arg(db.getDbNativeTypeName<double>())},
-         {QString("ALTER TABLE water ADD COLUMN flouride_ppm  %1").arg(db.getDbNativeTypeName<double>())},
+         {QString("ALTER TABLE water ADD COLUMN flouride_ppm  %1").arg(db.getDbNativeTypeName<double>())}, // Should have been fluoride_ppm!
          //
          // Equipment: Extended and additional fields for BeerJSON.  This includes changing a lot of column names as
          // BeerJSON essentially has a record per vessel ("HLT", "Mash Tun", etc)
@@ -2188,6 +2188,7 @@ namespace {
          case 12:
             ret &= migrate_to_13(database, sqlQuery);
             break;
+         // TODO: On next DB update, correct water.flouride_ppm to water.fluoride_ppm
          default:
             qCritical() << QString("Unknown version %1").arg(oldVersion);
             return false;
@@ -2302,7 +2303,7 @@ bool DatabaseSchemaHelper::migrate(Database & database, int oldVersion, int newV
    // By the magic of RAII, this will abort if we exit this function (including by throwing an exception) without
    // having called dbTransaction.commit().  (It will also turn foreign keys back on either way -- whether the
    // transaction is committed or rolled back.)
-   DbTransaction dbTransaction{database, connection, DbTransaction::DISABLE_FOREIGN_KEYS};
+   DbTransaction dbTransaction{database, connection, "Migrate", DbTransaction::DISABLE_FOREIGN_KEYS};
 
    for ( ; oldVersion < newVersion && ret; ++oldVersion ) {
       ret &= migrateNext(database, oldVersion, connection);

diff --git a/src/database/DbTransaction.cpp b/src/database/DbTransaction.cpp
@@ -1,5 +1,5 @@
 /*======================================================================================================================
- * database/DbTransaction.cpp is part of Brewken, and is copyright the following authors 2021-2022:
+ * database/DbTransaction.cpp is part of Brewken, and is copyright the following authors 2021-2024:
  *   • Matt Young <[email protected]>
  *
  * Brewken is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License
@@ -19,11 +19,15 @@
 #include <QSqlError>
 
 #include "database/Database.h"
+#include "Logging.h"
 
-
-DbTransaction::DbTransaction(Database & database, QSqlDatabase & connection, DbTransaction::SpecialBehaviours specialBehaviours) :
+DbTransaction::DbTransaction(Database & database,
+                             QSqlDatabase & connection,
+                             QString const nameForLogging,
+                             DbTransaction::SpecialBehaviours specialBehaviours) :
    database{database},
    connection{connection},
+   nameForLogging{nameForLogging},
    committed{false},
    specialBehaviours{specialBehaviours} {
    // Note that, on SQLite at least, turning foreign keys on and off has to happen outside a transaction, so we have to
@@ -33,9 +37,12 @@ DbTransaction::DbTransaction(Database & database, QSqlDatabase & connection, DbT
    }
 
    bool succeeded = this->connection.transaction();
-   qDebug() << Q_FUNC_INFO << "Database transaction begin: " << (succeeded ? "succeeded" : "failed");
+   qDebug() <<
+      Q_FUNC_INFO << "Database transaction" << this->nameForLogging << "begin: " << (succeeded ? "succeeded" : "failed");
    if (!succeeded) {
-      qCritical() << Q_FUNC_INFO << "Unable to start database transaction:" << connection.lastError().text();
+      qCritical() <<
+         Q_FUNC_INFO << "Unable to start database transaction" << this->nameForLogging << ":" << connection.lastError().text();
+      qCritical().noquote() << Q_FUNC_INFO << Logging::getStackTrace();
    }
    return;
 }
@@ -44,9 +51,11 @@ DbTransaction::~DbTransaction() {
    qDebug() << Q_FUNC_INFO;
    if (!committed) {
       bool succeeded = this->connection.rollback();
-      qDebug() << Q_FUNC_INFO << "Database transaction rollback: " << (succeeded ? "succeeded" : "failed");
+      qDebug() <<
+         Q_FUNC_INFO << "Database transaction" << this->nameForLogging << "rollback: " << (succeeded ? "succeeded" : "failed");
       if (!succeeded) {
-         qCritical() << Q_FUNC_INFO << "Unable to rollback database transaction:" << connection.lastError().text();
+         qCritical() <<
+            Q_FUNC_INFO << "Unable to rollback database transaction" << this->nameForLogging << ":" << connection.lastError().text();
       }
    }
 
@@ -59,9 +68,11 @@ DbTransaction::~DbTransaction() {
 
 bool DbTransaction::commit() {
    this->committed = connection.commit();
-   qDebug() << Q_FUNC_INFO << "Database transaction commit: " << (this->committed ? "succeeded" : "failed");
+   qDebug() <<
+      Q_FUNC_INFO << "Database transaction" << this->nameForLogging << "commit: " << (this->committed ? "succeeded" : "failed");
    if (!this->committed) {
-      qCritical() << Q_FUNC_INFO << "Unable to commit database transaction:" << connection.lastError().text();
+      qCritical() <<
+         Q_FUNC_INFO << "Unable to commit database transaction" << this->nameForLogging << ":" << connection.lastError().text();
    }
    return this->committed;
 }
diff --git a/src/database/DbTransaction.h b/src/database/DbTransaction.h
@@ -1,5 +1,5 @@
 /*======================================================================================================================
- * database/DbTransaction.h is part of Brewken, and is copyright the following authors 2021:
+ * database/DbTransaction.h is part of Brewken, and is copyright the following authors 2021-2024:
  *   • Matt Young <[email protected]>
  *
  * Brewken is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License
@@ -34,7 +34,10 @@ class DbTransaction {
    /**
     * \brief Constructing a \c DbTransaction will start a DB transaction
     */
-   DbTransaction(Database & database, QSqlDatabase & connection, SpecialBehaviours specialBehaviours = NONE);
+   DbTransaction(Database & database,
+                 QSqlDatabase & connection,
+                 QString const nameForLogging = "???",
+                 SpecialBehaviours specialBehaviours = NONE);
 
    /**
     * \brief When a \c DbTransaction goes out of scope and its destructor is called, the transaction started in the
@@ -53,6 +56,9 @@ class DbTransaction {
    Database & database;
    // This is intended to be a short-lived object, so it's OK to store a reference to a QSqlDatabase object
    QSqlDatabase & connection;
+   // This is useful for diagnosing problems such as
+   // 'Unable to start database transaction: "cannot start a transaction within a transaction Unable to begin transaction"'
+   QString const nameForLogging;
    bool committed;
    int specialBehaviours;
 

diff --git a/src/database/DefaultContentLoader.cpp b/src/database/DefaultContentLoader.cpp
@@ -156,6 +156,8 @@ DefaultContentLoader::UpdateResult DefaultContentLoader::updateContentIfNecessar
                                 allRecipesBeforeImport.begin(), allRecipesBeforeImport.end(),
                                 std::back_inserter(newlyImportedRecipes));
             qDebug() << Q_FUNC_INFO << newlyImportedRecipes.size() << "newly imported Recipes";
+            // TODO: It would be neat, at some point, to to have a mechanism for setting a property on multiple objects
+            //       of the same type, so that we could do it in a single DB update.
             for (auto recipe : newlyImportedRecipes) {
                recipe->setFolder(FOLDER_FOR_SUPPLIED_RECIPES);
             }

diff --git a/src/database/ObjectStore.cpp b/src/database/ObjectStore.cpp
@@ -1409,7 +1409,9 @@ void ObjectStore::loadAll(Database * database) {
    //
    // .:TBD:. In theory we don't need a transaction if we're _only_ reading data...
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Load All %1").arg(*this->pimpl->primaryTable.tableName)};
 
    //
    // Using QSqlTableModel would save us having to write a SELECT statement, however it is a bit hard to use it to
@@ -1710,7 +1712,9 @@ int ObjectStore::insert(std::shared_ptr<QObject> object) {
    // Start transaction
    // (By the magic of RAII, this will abort if we return from this function without calling dbTransaction.commit()
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Insert %1").arg(*this->pimpl->primaryTable.tableName)};
 
    int primaryKey = this->pimpl->insertObjectInDb(connection, *object, false);
 
@@ -1749,7 +1753,9 @@ void ObjectStore::update(std::shared_ptr<QObject> object) {
    // Start transaction
    // (By the magic of RAII, this will abort if we return from this function without calling dbTransaction.commit()
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Update %1").arg(*this->pimpl->primaryTable.tableName)};
 
    //
    // Construct the SQL, which will be of the form
@@ -1875,7 +1881,11 @@ void ObjectStore::updateProperty(QObject const & object, BtStringConst const & p
    // Start transaction
    // (By the magic of RAII, this will abort if we return from this function without calling dbTransaction.commit()
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{
+      *this->pimpl->database,
+      connection,
+      QString("Update property %1 on %2").arg(*propertyName).arg(*this->pimpl->primaryTable.tableName)
+   };
 
    if (!this->pimpl->updatePropertyInDb(connection, object, propertyName)) {
       // Something went wrong.  Bailing out here will abort the transaction and avoid sending the signal.
@@ -1918,7 +1928,9 @@ std::shared_ptr<QObject> ObjectStore::defaultHardDelete(int id) {
    qDebug() << Q_FUNC_INFO << "Hard delete" << this->pimpl->m_className << "#" << id;
    auto object = this->pimpl->allObjects.value(id);
    QSqlDatabase connection = this->pimpl->database->sqlDatabase();
-   DbTransaction dbTransaction{*this->pimpl->database, connection};
+   DbTransaction dbTransaction{*this->pimpl->database,
+                               connection,
+                               QString("Hard delete %1").arg(*this->pimpl->primaryTable.tableName)};
 
    // We'll use this in a couple of places below
    QVariant primaryKey{id};

diff --git a/src/database/ObjectStoreTyped.cpp b/src/database/ObjectStoreTyped.cpp
@@ -582,7 +582,8 @@ namespace {
          {ObjectStore::FieldType::Double, "iron_ppm"     , PropertyNames::Water::iron_ppm        },
          {ObjectStore::FieldType::Double, "nitrate_ppm"  , PropertyNames::Water::nitrate_ppm     },
          {ObjectStore::FieldType::Double, "nitrite_ppm"  , PropertyNames::Water::nitrite_ppm     },
-         {ObjectStore::FieldType::Double, "flouride_ppm" , PropertyNames::Water::flouride_ppm    },
+         // .:TODO:. We should correct the typo in this column name (copy-and-paste from BeerJSON
+         {ObjectStore::FieldType::Double, "flouride_ppm" , PropertyNames::Water::fluoride_ppm    },
       }
    };
 
@@ -1041,7 +1042,7 @@ bool WriteAllObjectStoresToNewDb(Database & newDatabase, QSqlDatabase & connecti
    // having called dbTransaction.commit().  (It will also turn foreign keys back on either way -- whether the
    // transaction is committed or rolled back.)
    //
-   DbTransaction dbTransaction{newDatabase, connectionNew, DbTransaction::DISABLE_FOREIGN_KEYS};
+   DbTransaction dbTransaction{newDatabase, connectionNew, "Write All", DbTransaction::DISABLE_FOREIGN_KEYS};
 
    for (ObjectStore const * objectStore : getAllObjectStores()) {
       if (!objectStore->writeAllToNewDb(newDatabase, connectionNew)) {

diff --git a/src/editors/BoilEditor.cpp b/src/editors/BoilEditor.cpp
@@ -22,44 +22,23 @@
 #include "model/Boil.h"
 #include "model/Recipe.h"
 
-BoilEditor::BoilEditor(QWidget* parent) :
+BoilEditor::BoilEditor(QWidget* parent, QString const editorName) :
    QDialog(parent),
-   EditorWithRecipeBase<BoilEditor, Boil>() {
+   EditorBase<BoilEditor, Boil, BoilEditorOptions>(editorName) {
    this->setupUi(this);
    this->postSetupUiInit(
       {
-       EDITOR_FIELD(Boil, label_name       , lineEdit_name       , PropertyNames::NamedEntity::name     ),
-       EDITOR_FIELD(Boil, label_description, textEdit_description, PropertyNames::Boil::description     ),
-       EDITOR_FIELD(Boil, label_preBoilSize, lineEdit_preBoilSize, PropertyNames::Boil::preBoilSize_l, 2),
-       EDITOR_FIELD(Boil, label_notes      , textEdit_notes      , PropertyNames::Boil::notes           )
+       EDITOR_FIELD_NORM(Boil, label_name       , lineEdit_name       , NamedEntity::name     ),
+       EDITOR_FIELD_NORM(Boil, label_description, textEdit_description, Boil::description     ),
+       EDITOR_FIELD_NORM(Boil, label_preBoilSize, lineEdit_preBoilSize, Boil::preBoilSize_l, 2),
+       EDITOR_FIELD_NORM(Boil, label_notes      , textEdit_notes      , Boil::notes           ),
       }
    );
 
-   // NB: label_description / textEdit_description don't need initialisation here as neither is a smart field
-   // NB: label_notes / textEdit_notes don't need initialisation here as neither is a smart field
-///   SMART_FIELD_INIT(BoilEditor, label_name       , lineEdit_name       , Boil, PropertyNames::NamedEntity::name  );
-///   SMART_FIELD_INIT(BoilEditor, label_preBoilSize, lineEdit_preBoilSize, Boil, PropertyNames::Boil::preBoilSize_l, 2);
-
-///   connect(this, &QDialog::accepted, this, &BoilEditor::saveAndClose);
-///   connect(this, &QDialog::rejected, this, &BoilEditor::closeEditor );
-
-///   this->connectSignalsAndSlots();
    return;
 }
 
 BoilEditor::~BoilEditor() = default;
 
-void BoilEditor::writeFieldsToEditItem() {
-   return;
-}
-
-void BoilEditor::writeLateFieldsToEditItem() {
-   return;
-}
-
-void BoilEditor::readFieldsFromEditItem([[maybe_unused]] std::optional<QString> propName) {
-   return;
-}
-
 // Insert the boilerplate stuff that we cannot do in EditorWithRecipeBase
-EDITOR_WITH_RECIPE_COMMON_CODE(BoilEditor)
+EDITOR_COMMON_CODE(Boil)