From aa3a31f014d6d035d3a2cd44f07d0b219a0b5e11 Mon Sep 17 00:00:00 2001 From: Austin Bennett Date: Fri, 11 Sep 2026 15:41:48 -0500 Subject: [PATCH] Fix the edit form so Save persists stage changes Changing an application's stage and pressing Save closed the form without saving the stage: JobsDatabase::updateJob() deliberately skips the stage column so every move lands in stage_history, but JobEditModel::save() never followed up with setStage(). It now does, as the CLI's update command always has. Also: - Validate that company and title are non-empty and say so, instead of saving a blank field - Always set lastError on failure, clear it on the next attempt, and bind the page's error message to it - Reload the form's values when jobsModel is assigned, since QML does not guarantee it is set before editingJobId - Bind the salary spin boxes to the model and write back only on user edits; guard the notes field the same way - Add jobeditmodeltest covering load order, edits, stage history, salaries round-tripping, adding, and validation errors Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BxDf7HqD1xPnsZP8wt3NTk --- autotests/CMakeLists.txt | 22 ++++ autotests/jobeditmodeltest.cpp | 177 ++++++++++++++++++++++++++++++++ src/jobeditmodel.cpp | 57 +++++++++- src/jobeditmodel.h | 2 + src/qml/ApplicationEditPage.qml | 22 ++-- 5 files changed, 267 insertions(+), 13 deletions(-) create mode 100644 autotests/jobeditmodeltest.cpp diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt index 84f32be..0f64e97 100644 --- a/autotests/CMakeLists.txt +++ b/autotests/CMakeLists.txt @@ -39,3 +39,25 @@ target_link_libraries(sankeylayouttest PRIVATE ) add_test(NAME sankeylayouttest COMMAND sankeylayouttest) + +add_executable(jobeditmodeltest + jobeditmodeltest.cpp + ../src/jobstage.cpp + ../src/jobsdatabase.cpp + ../src/jobsmodel.cpp + ../src/jobeditmodel.cpp + ../src/jobfieldcatalog.cpp +) + +target_include_directories(jobeditmodeltest PRIVATE ../src) + +target_link_libraries(jobeditmodeltest PRIVATE + Qt6::Core + Qt6::Gui + Qt6::Qml + Qt6::Sql + Qt6::Test + KF6::I18n +) + +add_test(NAME jobeditmodeltest COMMAND jobeditmodeltest) diff --git a/autotests/jobeditmodeltest.cpp b/autotests/jobeditmodeltest.cpp new file mode 100644 index 0000000..5e39ef8 --- /dev/null +++ b/autotests/jobeditmodeltest.cpp @@ -0,0 +1,177 @@ +/* + SPDX-FileCopyrightText: 2026 ToServeTheKing + + SPDX-License-Identifier: GPL-3.0-or-later +*/ + +#include "jobeditmodel.h" +#include "job.h" +#include "jobsdatabase.h" +#include "jobsmodel.h" + +#include +#include +#include + +using namespace Qt::Literals::StringLiterals; + +// JobsModel always opens JobsDatabase::defaultPath(), so each test points +// that at a fresh temporary file via the KAREER_DB_PATH override. +class JobEditModelTest : public QObject +{ + Q_OBJECT + +private Q_SLOTS: + void init() + { + m_dir = std::make_unique(); + QVERIFY(m_dir->isValid()); + qputenv("KAREER_DB_PATH", (m_dir->path() + u"/test.sqlite"_s).toUtf8()); + } + + // QML does not guarantee the order ApplicationEditPage's bindings assign + // editingJobId and jobsModel; the form must load the job either way. + void loadsJobWhenIdIsSetBeforeModel() + { + const int id = seedJob(); + + JobsModel jobs; + JobEditModel edit; + edit.setEditingJobId(id); + edit.setJobsModel(&jobs); + + QCOMPARE(valueOf(edit, u"company"_s).toString(), u"Acme Corp"_s); + QCOMPARE(valueOf(edit, u"title"_s).toString(), u"Engineer"_s); + QCOMPARE(valueOf(edit, u"salaryMin"_s).toInt(), 100000); + } + + void savesEditsToExistingJob() + { + const int id = seedJob(); + + JobsModel jobs; + JobEditModel edit; + edit.setEditingJobId(id); + edit.setJobsModel(&jobs); + + setValueOf(edit, u"title"_s, u"Senior Engineer"_s); + setValueOf(edit, u"notes"_s, u"Second round scheduled"_s); + QVERIFY2(edit.save(), qPrintable(edit.lastError())); + QVERIFY(edit.lastError().isEmpty()); + + JobsDatabase db; + const auto job = db.jobById(id); + QVERIFY(job.has_value()); + QCOMPARE(job->company, u"Acme Corp"_s); + QCOMPARE(job->title, u"Senior Engineer"_s); + QCOMPARE(job->notes, u"Second round scheduled"_s); + // Untouched salaries must survive the round trip through the form. + QCOMPARE(job->salaryMin, 100000); + QCOMPARE(job->salaryMax, -1); + } + + void stageChangeIsSavedWithHistory() + { + const int id = seedJob(); + + JobsModel jobs; + JobEditModel edit; + edit.setEditingJobId(id); + edit.setJobsModel(&jobs); + + setValueOf(edit, u"stage"_s, u"Interview"_s); + QVERIFY2(edit.save(), qPrintable(edit.lastError())); + + JobsDatabase db; + QCOMPARE(db.jobById(id)->stage, u"Interview"_s); + const auto transitions = db.stageTransitions(); + QCOMPARE(transitions.size(), 2); + QCOMPARE(transitions.last().fromStage, u"Applied"_s); + QCOMPARE(transitions.last().toStage, u"Interview"_s); + } + + void addsNewJob() + { + JobsModel jobs; + JobEditModel edit; + edit.setJobsModel(&jobs); + + setValueOf(edit, u"company"_s, u"Globex"_s); + setValueOf(edit, u"title"_s, u"Designer"_s); + QVERIFY2(edit.save(), qPrintable(edit.lastError())); + QCOMPARE(jobs.rowCount(), 1); + + JobsDatabase db; + const QList all = db.allJobs(); + QCOMPARE(all.size(), 1); + QCOMPARE(all.first().company, u"Globex"_s); + QCOMPARE(all.first().stage, u"Applied"_s); + } + + void missingCompanyIsReported() + { + JobsModel jobs; + JobEditModel edit; + edit.setJobsModel(&jobs); + + setValueOf(edit, u"title"_s, u"Designer"_s); + QVERIFY(!edit.save()); + QVERIFY(!edit.lastError().isEmpty()); + QCOMPARE(jobs.rowCount(), 0); + + // A later successful save clears the error. + setValueOf(edit, u"company"_s, u"Globex"_s); + QVERIFY(edit.save()); + QVERIFY(edit.lastError().isEmpty()); + } + + void saveWithoutModelReportsError() + { + JobEditModel edit; + QVERIFY(!edit.save()); + QVERIFY(!edit.lastError().isEmpty()); + } + +private: + int seedJob() + { + JobsDatabase db; + Job job; + job.company = u"Acme Corp"_s; + job.title = u"Engineer"_s; + job.dateApplied = QDate(2026, 6, 1); + job.salaryMin = 100000; + job.stage = u"Applied"_s; + if (!db.addJob(job)) { + qWarning() << db.lastError(); + return -1; + } + return job.id; + } + + static int rowOf(const JobEditModel &edit, const QString &fieldId) + { + for (int row = 0; row < edit.rowCount(); ++row) { + if (edit.data(edit.index(row), JobEditModel::FieldIdRole).toString() == fieldId) { + return row; + } + } + return -1; + } + + static QVariant valueOf(const JobEditModel &edit, const QString &fieldId) + { + return edit.data(edit.index(rowOf(edit, fieldId)), JobEditModel::ValueRole); + } + + static void setValueOf(JobEditModel &edit, const QString &fieldId, const QVariant &value) + { + edit.setValue(rowOf(edit, fieldId), value); + } + + std::unique_ptr m_dir; +}; + +QTEST_GUILESS_MAIN(JobEditModelTest) + +#include "jobeditmodeltest.moc" diff --git a/src/jobeditmodel.cpp b/src/jobeditmodel.cpp index 6e02444..66fdf55 100644 --- a/src/jobeditmodel.cpp +++ b/src/jobeditmodel.cpp @@ -6,6 +6,8 @@ #include "jobeditmodel.h" +#include + #include using namespace Qt::Literals::StringLiterals; @@ -96,6 +98,10 @@ void JobEditModel::setJobsModel(JobsModel *model) return; } m_jobsModel = model; + // QML does not guarantee editingJobId is assigned after jobsModel; if it + // came first, the resetValues() it triggered had no model to load from + // and only filled new-job defaults. + resetValues(); Q_EMIT jobsModelChanged(); } @@ -117,6 +123,7 @@ void JobEditModel::setEditingJobId(int id) void JobEditModel::resetValues() { m_values.clear(); + m_loadedStage.clear(); if (m_editingJobId < 0 || !m_jobsModel) { m_values[u"currency"_s] = u"USD"_s; @@ -138,6 +145,7 @@ void JobEditModel::resetValues() } m_values[field.id] = value; } + m_loadedStage = m_values.value(u"stage"_s).toString(); } if (rowCount() > 0) { @@ -170,7 +178,19 @@ void JobEditModel::setValue(int row, const QVariant &value) bool JobEditModel::save() { + setLastError(QString()); + if (!m_jobsModel) { + setLastError(i18n("Cannot save: no applications list is attached to this form.")); + return false; + } + + if (m_values.value(u"company"_s).toString().trimmed().isEmpty()) { + setLastError(i18n("Company is required.")); + return false; + } + if (m_values.value(u"title"_s).toString().trimmed().isEmpty()) { + setLastError(i18n("Job title is required.")); return false; } @@ -187,12 +207,39 @@ bool JobEditModel::save() } } - const bool ok = m_editingJobId < 0 ? m_jobsModel->addJob(fields) : m_jobsModel->updateJob(m_editingJobId, fields); - if (!ok) { - m_lastError = m_jobsModel->lastError(); - Q_EMIT lastErrorChanged(); + if (m_editingJobId < 0) { + if (!m_jobsModel->addJob(fields)) { + setLastError(m_jobsModel->lastError()); + return false; + } + return true; } - return ok; + + if (!m_jobsModel->updateJob(m_editingJobId, fields)) { + setLastError(m_jobsModel->lastError()); + return false; + } + + // updateJob() deliberately never writes the stage (so every stage change + // lands in stage_history); route a changed stage through setStage(). + const QString stage = fields.value(u"stage"_s).toString(); + if (!stage.isEmpty() && stage != m_loadedStage) { + if (!m_jobsModel->setStage(m_editingJobId, stage)) { + setLastError(m_jobsModel->lastError()); + return false; + } + m_loadedStage = stage; + } + return true; +} + +void JobEditModel::setLastError(const QString &error) +{ + if (m_lastError == error) { + return; + } + m_lastError = error; + Q_EMIT lastErrorChanged(); } bool JobEditModel::deleteJob() diff --git a/src/jobeditmodel.h b/src/jobeditmodel.h index 4e69098..387735b 100644 --- a/src/jobeditmodel.h +++ b/src/jobeditmodel.h @@ -74,10 +74,12 @@ Q_SIGNALS: private: void resetValues(); + void setLastError(const QString &error); JobsModel *m_jobsModel = nullptr; int m_editingJobId = -1; QHash m_values; QString m_lastError; + QString m_loadedStage; ///< Stage as loaded from the database, to detect stage changes on save. QList m_fields; }; diff --git a/src/qml/ApplicationEditPage.qml b/src/qml/ApplicationEditPage.qml index 43891e7..9952aef 100644 --- a/src/qml/ApplicationEditPage.qml +++ b/src/qml/ApplicationEditPage.qml @@ -56,11 +56,10 @@ Kirigami.ScrollablePage { Kirigami.Action { text: i18nc("@action:button", "Save") icon.name: "document-save" + // On failure, editModel.lastError is set and shown by errorLabel. onTriggered: { if (editModel.save()) { root.done(); - } else { - errorLabel.text = editModel.lastError; } } } @@ -76,6 +75,7 @@ Kirigami.ScrollablePage { Layout.rightMargin: Kirigami.Units.largeSpacing Layout.bottomMargin: Kirigami.Units.smallSpacing type: Kirigami.MessageType.Error + text: editModel.lastError visible: text.length > 0 } @@ -204,11 +204,11 @@ Kirigami.ScrollablePage { from: spinD.model.spinMin to: spinD.model.spinMax stepSize: 1000 - // One-time init, not a persistent binding: this also - // writes back on user edits, so binding "value" live to - // spinD.model.value would be a binding loop. - Component.onCompleted: value = spinD.model.value - onValueChanged: editModel.setValue(spinD.sourceRow, value) + // Live binding so values loaded after the delegate is + // created still show up. Writing back only on + // valueModified (user edits) avoids a binding loop. + value: spinD.model.value + onValueModified: editModel.setValue(spinD.sourceRow, value) } Kirigami.Separator { @@ -279,7 +279,13 @@ Kirigami.ScrollablePage { Layout.preferredHeight: Kirigami.Units.gridUnit * 5 wrapMode: TextEdit.Wrap text: areaD.value - onTextChanged: editModel.setValue(areaD.sourceRow, text) + // TextArea has no textEdited signal; skip the echo from + // the model-driven binding and only write real edits. + onTextChanged: { + if (text !== areaD.value) { + editModel.setValue(areaD.sourceRow, text); + } + } } Kirigami.Separator {