Improve CrudModel code quality

tests/integration/CMakeLists.txt

@@ -5,6 +5,7 @@ add_executable(IntegrationTests
     tst_DatabaseSizes.cpp
     tst_DataGridMapping.cpp
     tst_DataGridUpdate.cpp
+    tst_CrudModelBehavior.cpp
     ${CMAKE_SOURCE_DIR}/pglab/crud/CrudModel.cpp
     ${CMAKE_SOURCE_DIR}/pglab/crud/DataGridWidget.cpp
     ${CMAKE_SOURCE_DIR}/pglab/OpenDatabase.cpp

tests/integration/DataGridTest.h

@@ -9,7 +9,11 @@
 #include "crud/DataGridWidget.h"
 #include "Pgsql_Connection.h"
 
+#include <QCoreApplication>
+#include <QDeadlineTimer>
 #include <QSortFilterProxyModel>
+#include "catalog/PgClassContainer.h"
+#include "catalog/PgDatabaseCatalog.h"
 #include <QUrl>
 #include <memory>
 #include <optional>
@@ -41,6 +45,26 @@ protected:
 		model->setResult(db, result);
 	}
 
+	/// Shows a whole table (CRUD mode) and waits for the asynchronous load to finish
+	bool showTable(const QString &name)
+	{
+		db = openDb();
+		auto table = db->catalog()->classes()->findIf([&name](const PgClass &c)
+			{ return c.fullyQualifiedQuotedObjectName().endsWith("." + name); });
+		if (!table)
+			return false;
+		model = std::make_unique<CrudModel>();
+		widget = std::make_unique<DataGridWidget>();
+		widget->setSourceModel(model.get());
+		bool reset = false;
+		QObject::connect(model.get(), &QAbstractItemModel::modelReset, model.get(), [&reset] { reset = true; });
+		model->setConfig(db, *table);
+		QDeadlineTimer deadline(10000);
+		while (!reset && !deadline.hasExpired())
+			QCoreApplication::processEvents(QEventLoop::AllEvents, 50);
+		return reset;
+	}
+
 	/// Editable flag of every result column, as seen through the widget's proxy
 	std::vector<bool> editableColumns()
 	{

tests/integration/fixtures/schema.sql

@@ -31,3 +31,30 @@ CREATE TABLE grid_nopk (
     y text
 );
 INSERT INTO grid_nopk (x, y) VALUES (1, 'a'), (2, 'b');
+
+CREATE TABLE crud_item (
+    id serial PRIMARY KEY,
+    name text NOT NULL,
+    qty integer,
+    flag boolean,
+    born date,
+    tag text DEFAULT 'dflt'
+);
+INSERT INTO crud_item (name, qty, flag, born) VALUES
+    ('a', 1, true, '2020-01-02'),
+    ('b', 2, false, NULL),
+    ('c', NULL, NULL, '2021-03-04'),
+    ('d', 4, true, NULL),
+    ('e', 5, false, NULL);
+
+CREATE TABLE crud_empty (
+    id serial PRIMARY KEY,
+    v text
+);
+
+CREATE TABLE crud_gen (
+    id integer GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
+    a integer,
+    b integer GENERATED ALWAYS AS (a * 2) STORED
+);
+INSERT INTO crud_gen (a) VALUES (1), (2);

tests/integration/tst_CrudModelBehavior.cpp

@@ -0,0 +1,656 @@
+// Characterization tests for CrudModel, they lock in the behavior of the model
+// so it can be refactored safely.
+#include "DataGridTest.h"
+
+#include "CustomDataRole.h"
+#include "Pgsql_oids.h"
+
+#include <QApplication>
+#include <QBrush>
+#include <QDate>
+#include <QTimer>
+
+namespace {
+
+/// Failed saves show a modal message box, this closes it so the test can continue
+void autoCloseModalDialogs(int triesLeft = 100)
+{
+	if (auto w = QApplication::activeModalWidget())
+		w->close();
+	else if (triesLeft > 0)
+		QTimer::singleShot(20, [triesLeft] { autoCloseModalDialogs(triesLeft - 1); });
+}
+
+/// Records the values of CrudModel::pendingChangesChanged
+struct PendingSpy {
+	explicit PendingSpy(CrudModel *m)
+	{
+		QObject::connect(m, &CrudModel::pendingChangesChanged, m, [this](bool b) { values.push_back(b); });
+	}
+	std::vector<bool> values;
+};
+
+/// Counts the emissions of a signal
+struct CountSpy {
+	template <typename Sender, typename Func>
+	CountSpy(Sender *sender, Func signal)
+	{
+		QObject::connect(sender, signal, sender, [this] { ++n; });
+	}
+	int count() const { return n; }
+	int n = 0;
+};
+
+// crud_item columns
+constexpr int ColId = 0;
+constexpr int ColName = 1;
+constexpr int ColQty = 2;
+constexpr int ColFlag = 3;
+constexpr int ColBorn = 4;
+constexpr int ColTag = 5;
+
+class CrudModelBehaviorTest : public DataGridTest {
+protected:
+	QModelIndex idx(int row, int resultCol) const { return model->index(row, resultCol + 1); }
+
+	QVariant cell(int row, int resultCol, int role = Qt::DisplayRole) const
+	{
+		return model->data(idx(row, resultCol), role);
+	}
+
+	bool edit(int row, int resultCol, const QString &value)
+	{
+		return model->setData(idx(row, resultCol), value, Qt::EditRole);
+	}
+
+	/// Saves while auto closing any error dialog
+	bool save()
+	{
+		QTimer::singleShot(0, [] { autoCloseModalDialogs(); });
+		return model->saveChanges();
+	}
+
+	/// Row of crud_item with the given name, -1 if not shown
+	int rowOfName(const QString &name) const
+	{
+		for (int r = 0; r < model->rowCount(); ++r)
+			if (cell(r, ColName, Qt::EditRole).toString() == name)
+				return r;
+		return -1;
+	}
+
+	QStringList names() const
+	{
+		QStringList l;
+		for (int r = 0; r < model->rowCount(); ++r)
+			l << cell(r, ColName, Qt::EditRole).toString();
+		return l;
+	}
+
+	void reload()
+	{
+		bool reset = false;
+		QObject::connect(model.get(), &QAbstractItemModel::modelReset, model.get(), [&reset] { reset = true; });
+		model->loadData();
+		QDeadlineTimer deadline(10000);
+		while (!reset && !deadline.hasExpired())
+			QCoreApplication::processEvents(QEventLoop::AllEvents, 50);
+		ASSERT_TRUE(reset);
+	}
+};
+
+} // namespace
+
+// ---- loading, headers, row number column
+
+TEST_F(CrudModelBehaviorTest, WholeTableLoadsRowsPlusEmptyNewRow)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_EQ(model->rowCount(), 6);
+	EXPECT_EQ(model->columnCount(), 7);
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(names(), (QStringList{"a", "b", "c", "d", "e", ""}));
+	for (int c = 0; c <= ColTag; ++c)
+		EXPECT_FALSE(cell(5, c).isValid()) << c;
+}
+
+TEST_F(CrudModelBehaviorTest, EmptyTableHasOnlyTheNewRow)
+{
+	ASSERT_TRUE(showTable("crud_empty"));
+	EXPECT_EQ(model->rowCount(), 1);
+}
+
+TEST_F(CrudModelBehaviorTest, QueryResultHasNoNewRow)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	EXPECT_EQ(model->rowCount(), 3);
+}
+
+TEST_F(CrudModelBehaviorTest, HeaderShowsNameAndType)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_EQ(model->headerData(0, Qt::Horizontal).toString(), "#");
+	for (int c = 0; c <= ColTag; ++c)
+	{
+		auto text = model->headerData(c + 1, Qt::Horizontal, Qt::DisplayRole).toString();
+		EXPECT_EQ(text.count('\n'), 1) << c;
+		EXPECT_FALSE(text.section('\n', 1).isEmpty()) << c;
+	}
+	EXPECT_EQ(columnName(ColId), "id");
+	EXPECT_EQ(columnName(ColName), "name");
+	EXPECT_EQ(columnName(ColTag), "tag");
+	EXPECT_FALSE(model->headerData(1, Qt::Horizontal, Qt::EditRole).isValid());
+}
+
+TEST_F(CrudModelBehaviorTest, RowNumberColumnData)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	for (int r = 0; r < model->rowCount(); ++r)
+	{
+		auto i = model->index(r, 0);
+		EXPECT_EQ(model->data(i, Qt::DisplayRole).toInt(), r + 1);
+		EXPECT_EQ(model->data(i, CustomSortRole).toInt(), r + 1);
+		EXPECT_EQ(model->data(i, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::int4_oid));
+		EXPECT_FALSE(model->data(i, Qt::EditRole).isValid());
+		EXPECT_FALSE(model->data(i, Qt::ForegroundRole).isValid());
+	}
+	EXPECT_FALSE(model->setData(model->index(0, 0), "5", Qt::EditRole));
+	EXPECT_FALSE(model->hasPendingChanges());
+}
+
+// ---- data roles
+
+TEST_F(CrudModelBehaviorTest, DisplayAndEditRoles)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_EQ(cell(0, ColName).toString(), "a");
+	EXPECT_EQ(cell(0, ColQty).toString(), "1");
+	EXPECT_EQ(cell(0, ColQty, Qt::EditRole).toString(), "1");
+	EXPECT_EQ(cell(0, ColBorn).toString(), "2020-01-02");
+	EXPECT_EQ(cell(0, ColTag).toString(), "dflt");
+	// NULL
+	EXPECT_FALSE(cell(1, ColBorn).isValid());
+	EXPECT_FALSE(cell(1, ColBorn, Qt::EditRole).isValid());
+	// Roles that are not handled
+	EXPECT_FALSE(cell(0, ColName, Qt::ToolTipRole).isValid());
+	EXPECT_FALSE(cell(0, ColName, Qt::ForegroundRole).isValid());
+}
+
+TEST_F(CrudModelBehaviorTest, BoolDisplaysAsBool)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto t = cell(0, ColFlag);
+	EXPECT_EQ(t.typeId(), QMetaType::Bool);
+	EXPECT_TRUE(t.toBool());
+	auto f = cell(1, ColFlag);
+	EXPECT_EQ(f.typeId(), QMetaType::Bool);
+	EXPECT_FALSE(f.toBool());
+	EXPECT_FALSE(cell(2, ColFlag).isValid());
+	EXPECT_EQ(cell(0, ColFlag, Qt::EditRole).toString(), "t");
+}
+
+TEST_F(CrudModelBehaviorTest, DisplayTruncatesButEditDoesNot)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(0, ColName, QString(300, 'x')));
+	EXPECT_EQ(cell(0, ColName, Qt::DisplayRole).toString().size(), 256);
+	EXPECT_EQ(cell(0, ColName, Qt::EditRole).toString().size(), 300);
+}
+
+TEST_F(CrudModelBehaviorTest, SortRoleGivesTypedValues)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto i = cell(0, ColQty, CustomSortRole);
+	EXPECT_EQ(i.typeId(), QMetaType::Int);
+	EXPECT_EQ(i.toInt(), 1);
+	EXPECT_EQ(cell(0, ColBorn, CustomSortRole).toDate(), QDate(2020, 1, 2));
+	EXPECT_EQ(cell(0, ColBorn, CustomSortRole).typeId(), QMetaType::QDate);
+	auto b = cell(0, ColFlag, CustomSortRole);
+	EXPECT_EQ(b.typeId(), QMetaType::Bool);
+	EXPECT_TRUE(b.toBool());
+	EXPECT_EQ(cell(0, ColName, CustomSortRole).toString(), "a");
+	EXPECT_FALSE(cell(1, ColBorn, CustomSortRole).isValid());
+	EXPECT_FALSE(cell(2, ColQty, CustomSortRole).isValid());
+}
+
+TEST_F(CrudModelBehaviorTest, DataTypeRole)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_EQ(cell(0, ColId, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::int4_oid));
+	EXPECT_EQ(cell(0, ColName, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::text_oid));
+	EXPECT_EQ(cell(0, ColFlag, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::bool_oid));
+	EXPECT_EQ(cell(0, ColBorn, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::date_oid));
+	// also for the new row
+	EXPECT_EQ(cell(5, ColQty, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::int4_oid));
+}
+
+TEST_F(CrudModelBehaviorTest, ReadOnlyColumnsAreGrayInQueryMode)
+{
+	showQuery("SELECT id, name || 'x' AS e FROM grid_parent");
+	EXPECT_FALSE(cell(0, 0, Qt::ForegroundRole).isValid());
+	auto brush = cell(0, 1, Qt::ForegroundRole);
+	ASSERT_TRUE(brush.isValid());
+	EXPECT_EQ(brush.value<QBrush>().color(), QColor(Qt::gray));
+}
+
+// ---- flags
+
+TEST_F(CrudModelBehaviorTest, FlagsOfGeneratedAndIdentityColumns)
+{
+	ASSERT_TRUE(showTable("crud_gen"));
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{false, true, false}));
+	auto f = model->flags(model->index(0, 0));
+	EXPECT_TRUE(f & Qt::ItemIsSelectable);
+	EXPECT_TRUE(f & Qt::ItemIsEnabled);
+	EXPECT_FALSE(f & Qt::ItemIsEditable);
+}
+
+TEST_F(CrudModelBehaviorTest, FlagsOfWholeTable)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_EQ(editableColumns(), (std::vector<bool>(6, true)));
+}
+
+TEST_F(CrudModelBehaviorTest, ExpressionColumnIsNotEditable)
+{
+	showQuery("SELECT id, name || 'x' AS e FROM grid_parent");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{true, false}));
+}
+
+TEST_F(CrudModelBehaviorTest, TableWithoutPkIsNotEditable)
+{
+	ASSERT_TRUE(showTable("grid_nopk"));
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{false, false}));
+}
+
+// ---- setData
+
+TEST_F(CrudModelBehaviorTest, SetDataRejectsOtherRoles)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_FALSE(model->setData(idx(0, ColName), "x", Qt::DisplayRole));
+	EXPECT_FALSE(model->hasPendingChanges());
+}
+
+TEST_F(CrudModelBehaviorTest, SetDataShowsPendingValueAndEmitsDataChanged)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	CountSpy spy(model.get(), &QAbstractItemModel::dataChanged);
+	ASSERT_TRUE(edit(0, ColName, "changed"));
+	EXPECT_EQ(spy.count(), 1);
+	EXPECT_TRUE(model->hasPendingChanges());
+	EXPECT_EQ(cell(0, ColName).toString(), "changed");
+	EXPECT_EQ(cell(1, ColName).toString(), "b");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 1"), "a");
+}
+
+TEST_F(CrudModelBehaviorTest, EmptyStringBecomesNullAndQuotesBecomeEmptyString)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(0, ColTag, ""));
+	ASSERT_TRUE(edit(1, ColTag, "''"));
+	EXPECT_FALSE(cell(0, ColTag).isValid());
+	EXPECT_TRUE(cell(1, ColTag).isValid());
+	EXPECT_EQ(cell(1, ColTag).toString(), "");
+	ASSERT_TRUE(save());
+
+	EXPECT_EQ(dbValue("SELECT tag FROM crud_item WHERE name = 'a'"), std::nullopt);
+	EXPECT_EQ(dbValue("SELECT tag FROM crud_item WHERE name = 'b'"), "");
+	EXPECT_EQ(dbValue("SELECT tag FROM crud_item WHERE name = 'c'"), "dflt");
+}
+
+TEST_F(CrudModelBehaviorTest, RepeatedEditsOfSameCellKeepLastValue)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(0, ColName, "one"));
+	ASSERT_TRUE(edit(0, ColName, "two"));
+	ASSERT_TRUE(edit(1, ColName, "three"));
+	EXPECT_EQ(spy.values, (std::vector<bool>{true}));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false}));
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 1"), "two");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 2"), "three");
+}
+
+// ---- saving updates
+
+TEST_F(CrudModelBehaviorTest, SavedUpdateIsShownAfterwards)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(2, ColQty, "42"));
+	ASSERT_TRUE(save());
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(cell(2, ColQty).toString(), "42");
+	EXPECT_EQ(cell(2, ColName).toString(), "c");
+	EXPECT_EQ(cell(2, ColBorn).toString(), "2021-03-04");
+	EXPECT_EQ(model->rowCount(), 6);
+	EXPECT_EQ(dbValue("SELECT qty FROM crud_item WHERE name = 'c'"), "42");
+}
+
+TEST_F(CrudModelBehaviorTest, SaveWithoutChangesIsOk)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	EXPECT_TRUE(model->saveChanges());
+	EXPECT_TRUE(spy.values.empty());
+}
+
+TEST_F(CrudModelBehaviorTest, RevertThenEditAgain)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(0, ColName, "discarded"));
+	model->revertChanges();
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(cell(0, ColName).toString(), "a");
+	model->revertChanges();	// nothing to revert, no signal
+	ASSERT_TRUE(edit(0, ColName, "kept"));
+	EXPECT_EQ(cell(0, ColName).toString(), "kept");
+	ASSERT_TRUE(save());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false, true, false}));
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 1"), "kept");
+}
+
+TEST_F(CrudModelBehaviorTest, RevertDoesNotRemoveSavedValues)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(0, ColName, "saved"));
+	ASSERT_TRUE(save());
+	ASSERT_TRUE(edit(0, ColName, "unsaved"));
+	model->revertChanges();
+	EXPECT_EQ(cell(0, ColName).toString(), "saved");
+}
+
+TEST_F(CrudModelBehaviorTest, PartialFailureKeepsOnlyFailingRowPending)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(0, ColName, "ok"));
+	ASSERT_TRUE(edit(1, ColName, ""));	// NULL violates NOT NULL
+	ASSERT_TRUE(edit(2, ColName, "never saved"));
+	EXPECT_FALSE(save());
+
+	EXPECT_TRUE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true}));
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 1"), "ok");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 2"), "b");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 3"), "c");
+	EXPECT_EQ(cell(0, ColName).toString(), "ok");
+	EXPECT_FALSE(cell(1, ColName).isValid());	// still shows the pending value
+
+	// Fixing the failing row lets the rest be saved
+	ASSERT_TRUE(edit(1, ColName, "fixed"));
+	ASSERT_TRUE(save());
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false}));
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 2"), "fixed");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 3"), "never saved");
+}
+
+TEST_F(CrudModelBehaviorTest, UpdateOfRowChangedElsewhereFails)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	conn().query("DELETE FROM crud_item WHERE id = 1");
+	ASSERT_TRUE(edit(0, ColName, "gone"));
+	EXPECT_FALSE(save());
+	EXPECT_TRUE(model->hasPendingChanges());
+}
+
+// ---- reload
+
+TEST_F(CrudModelBehaviorTest, ReloadClearsPendingChangesAndShowsDbState)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(0, ColName, "pending"));
+	conn().query("INSERT INTO crud_item (name) VALUES ('f')");
+	PendingSpy spy(model.get());
+	reload();
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{false}));
+	EXPECT_EQ(model->rowCount(), 7);
+	EXPECT_EQ(names(), (QStringList{"a", "b", "c", "d", "e", "f", ""}));
+}
+
+TEST_F(CrudModelBehaviorTest, ReloadWithoutPendingChangesEmitsNoSignal)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	reload();
+	EXPECT_TRUE(spy.values.empty());
+	EXPECT_EQ(model->rowCount(), 6);
+}
+
+// ---- insert
+
+TEST_F(CrudModelBehaviorTest, InsertNewRow)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(5, ColName, "new"));
+	ASSERT_TRUE(edit(5, ColQty, "7"));
+	EXPECT_EQ(spy.values, (std::vector<bool>{true}));
+	ASSERT_TRUE(save());
+
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false}));
+	EXPECT_EQ(model->rowCount(), 7);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "6");
+	EXPECT_EQ(dbValue("SELECT qty FROM crud_item WHERE name = 'new'"), "7");
+
+	// RETURNING * brings the defaults into the model
+	EXPECT_EQ(cell(5, ColName).toString(), "new");
+	EXPECT_EQ(cell(5, ColQty).toString(), "7");
+	EXPECT_EQ(cell(5, ColTag).toString(), "dflt");
+	EXPECT_EQ(cell(5, ColId).toString(), *dbValue("SELECT id FROM crud_item WHERE name = 'new'"));
+	EXPECT_FALSE(cell(5, ColBorn).isValid());
+
+	// and a fresh empty row is appended
+	EXPECT_EQ(names(), (QStringList{"a", "b", "c", "d", "e", "new", ""}));
+	EXPECT_FALSE(cell(6, ColId).isValid());
+}
+
+TEST_F(CrudModelBehaviorTest, InsertTwoRowsInARow)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(5, ColName, "n1"));
+	ASSERT_TRUE(save());
+	ASSERT_TRUE(edit(6, ColName, "n2"));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(model->rowCount(), 8);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "7");
+	EXPECT_EQ(names(), (QStringList{"a", "b", "c", "d", "e", "n1", "n2", ""}));
+}
+
+TEST_F(CrudModelBehaviorTest, InsertIntoEmptyTable)
+{
+	ASSERT_TRUE(showTable("crud_empty"));
+	ASSERT_TRUE(edit(0, 1, "first"));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(model->rowCount(), 2);
+	EXPECT_EQ(dbValue("SELECT v FROM crud_empty"), "first");
+	EXPECT_EQ(cell(0, 0).toString(), "1");
+}
+
+TEST_F(CrudModelBehaviorTest, InsertWithExplicitNull)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(5, ColName, "nulltag"));
+	ASSERT_TRUE(edit(5, ColTag, ""));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(dbValue("SELECT tag FROM crud_item WHERE name = 'nulltag'"), std::nullopt);
+	EXPECT_FALSE(cell(5, ColTag).isValid());
+}
+
+TEST_F(CrudModelBehaviorTest, FailedInsertKeepsRowPending)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(5, ColQty, "9"));	// name is NOT NULL and missing
+	EXPECT_FALSE(save());
+	EXPECT_TRUE(model->hasPendingChanges());
+	EXPECT_EQ(model->rowCount(), 6);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "5");
+	EXPECT_EQ(cell(5, ColQty).toString(), "9");
+
+	ASSERT_TRUE(edit(5, ColName, "now valid"));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(model->rowCount(), 7);
+	EXPECT_EQ(dbValue("SELECT qty FROM crud_item WHERE name = 'now valid'"), "9");
+}
+
+TEST_F(CrudModelBehaviorTest, EditingOnlyThenRevertingNewRowDoesNotInsert)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(5, ColName, "temp"));
+	model->revertChanges();
+	EXPECT_FALSE(cell(5, ColName).isValid());
+	EXPECT_EQ(model->rowCount(), 6);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "5");
+}
+
+// ---- delete
+
+TEST_F(CrudModelBehaviorTest, RemoveSingleRow)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	CountSpy spy(model.get(), &QAbstractItemModel::rowsRemoved);
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(1, 1)});
+	EXPECT_TRUE(ok) << msg.toStdString();
+	EXPECT_EQ(spy.count(), 1);
+	EXPECT_EQ(names(), (QStringList{"a", "c", "d", "e", ""}));
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "4");
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item WHERE name = 'b'"), "0");
+	EXPECT_EQ(model->data(model->index(4, 0), Qt::DisplayRole).toInt(), 5);
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveContiguousRange)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(1, 3)});
+	EXPECT_TRUE(ok) << msg.toStdString();
+	EXPECT_EQ(names(), (QStringList{"a", "e", ""}));
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "2");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveNonContiguousRanges)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(0, 1), IntegerRange<int>(2, 2)});
+	EXPECT_TRUE(ok) << msg.toStdString();
+	EXPECT_EQ(names(), (QStringList{"b", "e", ""}));
+	EXPECT_EQ(dbValue("SELECT string_agg(name, ',' ORDER BY name) FROM crud_item"), "b,e");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveRowsOverloadForViews)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_TRUE(model->removeRows(0, 2));
+	EXPECT_EQ(names(), (QStringList{"c", "d", "e", ""}));
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "3");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveNothingIsOk)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto [ok, msg] = model->removeRows({});
+	EXPECT_TRUE(ok);
+	EXPECT_EQ(model->rowCount(), 6);
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveDropsPendingEditsOfRemovedRows)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(1, ColName, "edited then removed"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(1, 1)});
+	EXPECT_TRUE(ok);
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false}));
+}
+
+TEST_F(CrudModelBehaviorTest, PendingEditsOfOtherRowsSurviveRemoval)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(3, ColName, "d edited"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(0, 3)});
+	EXPECT_TRUE(ok);
+	EXPECT_TRUE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true}));
+	EXPECT_EQ(names(), (QStringList{"d edited", "e", ""}));
+
+	ASSERT_TRUE(save());
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 4"), "d edited");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 5"), "e");
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "2");
+}
+
+TEST_F(CrudModelBehaviorTest, EditAfterRemovalUpdatesTheRightRow)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(std::get<0>(model->removeRows({IntegerRange<int>(0, 3)})));
+	ASSERT_TRUE(edit(1, ColName, "e edited"));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 5"), "e edited");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 4"), "d");
+}
+
+TEST_F(CrudModelBehaviorTest, InsertAfterRemoval)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(std::get<0>(model->removeRows({IntegerRange<int>(0, 1)})));
+	ASSERT_TRUE(edit(model->rowCount() - 1, ColName, "after delete"));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(names(), (QStringList{"b", "c", "d", "e", "after delete", ""}));
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "5");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveOutOfRangeFails)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(5, 5)});
+	EXPECT_FALSE(ok);
+	EXPECT_FALSE(msg.isEmpty());
+	EXPECT_EQ(model->rowCount(), 6);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "5");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveFromQueryResultFails)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(0, 1)});
+	EXPECT_FALSE(ok);
+	EXPECT_FALSE(msg.isEmpty());
+	EXPECT_EQ(model->rowCount(), 3);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM grid_parent"), "3");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveFromTableWithoutPkFails)
+{
+	ASSERT_TRUE(showTable("grid_nopk"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(0, 1)});
+	EXPECT_FALSE(ok);
+	EXPECT_FALSE(msg.isEmpty());
+	EXPECT_EQ(model->rowCount(), 3);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM grid_nopk"), "2");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveRowReferencedByForeignKeyFails)
+{
+	conn().query("ALTER TABLE grid_child ADD FOREIGN KEY (parent_id) REFERENCES grid_parent (id)");
+	ASSERT_TRUE(showTable("grid_parent"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(0, 1)});
+	EXPECT_FALSE(ok);
+	EXPECT_EQ(model->rowCount(), 4);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM grid_parent"), "3");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveFromCompositePkTable)
+{
+	ASSERT_TRUE(showTable("grid_composite"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(1, 1)});
+	EXPECT_TRUE(ok) << msg.toStdString();
+	EXPECT_EQ(dbValue("SELECT count(*) FROM grid_composite"), "2");
+	EXPECT_EQ(dbValue("SELECT count(*) FROM grid_composite WHERE a = 1 AND b = 2"), "0");
+}