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) <[email protected]> Claude-Session: https://claude.ai/code/session_01BxDf7HqD1xPnsZP8wt3NTk
This commit is contained in:
Claude Opus 5
@@ -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)
|
||||
|
||||
@@ -0,0 +1,177 @@
|
||||
/*
|
||||
SPDX-FileCopyrightText: 2026 ToServeTheKing <[email protected]>
|
||||
|
||||
SPDX-License-Identifier: GPL-3.0-or-later
|
||||
*/
|
||||
|
||||
#include "jobeditmodel.h"
|
||||
#include "job.h"
|
||||
#include "jobsdatabase.h"
|
||||
#include "jobsmodel.h"
|
||||
|
||||
#include <QTemporaryDir>
|
||||
#include <QtTest>
|
||||
#include <memory>
|
||||
|
||||
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<QTemporaryDir>();
|
||||
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<Job> 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<QTemporaryDir> m_dir;
|
||||
};
|
||||
|
||||
QTEST_GUILESS_MAIN(JobEditModelTest)
|
||||
|
||||
#include "jobeditmodeltest.moc"
|
||||
+52
-5
@@ -6,6 +6,8 @@
|
||||
|
||||
#include "jobeditmodel.h"
|
||||
|
||||
#include <KLocalizedString>
|
||||
|
||||
#include <QDate>
|
||||
|
||||
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 ok;
|
||||
return true;
|
||||
}
|
||||
|
||||
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()
|
||||
|
||||
@@ -74,10 +74,12 @@ Q_SIGNALS:
|
||||
|
||||
private:
|
||||
void resetValues();
|
||||
void setLastError(const QString &error);
|
||||
|
||||
JobsModel *m_jobsModel = nullptr;
|
||||
int m_editingJobId = -1;
|
||||
QHash<QString, QVariant> m_values;
|
||||
QString m_lastError;
|
||||
QString m_loadedStage; ///< Stage as loaded from the database, to detect stage changes on save.
|
||||
QList<JobFieldCatalog::Field> m_fields;
|
||||
};
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user