lims: give a refused form submission a path to the operator, and leave one dispatch path per action - #314
Merged
Merged
Conversation
Every schema-driven form on both lims surfaces submits through
`submitIfValid`, which reports *both* outcomes on one signal --
`replyReceived(actionType, ok, payload)`, with the model's own `what()`
as the payload when `ok` is false. The bridges relayed it faithfully.
No file under `gui/qml/` handled it.
The `failed`/`lastError` pair the two views *do* bind is fed only from
`SamplePresenter::reportError` / `ResultPresenter::reportError`, which
the typed invokables use and `submitIfValid` never touches. So the
operator clicked Submit, the model refused, and the screen did not
change: an over-precise reading (decision 7), an `exactlyOneOf`
violation (decision 6), a four-eyes refusal (decision 16), an unknown
qualifier or dilution code (decision 18) and a rejected conflict
resolution all landed nowhere. Those refusals are the rung's thesis.
Pinned first, at the level that can observe it. `test_lims_qml_surface.cpp`
gains a case that runs the QML-surface audit with no exemptions and
requires that no finding names `replyReceived`; before this change it
failed with exactly the two lines the defect predicts:
lims::gui::SampleBridge::replyReceived is a signal no scanned .qml
handles (bound as 'sampleBridge')
lims::gui::ResultBridge::replyReceived is a signal no scanned .qml
handles (bound as 'resultBridge')
Fixed by binding `replyReceived` in the two views, not by routing
`ok == false` into `lastError` inside the bridges. That is the shape
every other rung with a `replyReceived` uses -- bookmarks'
`BookmarkListView.qml`, pastebin's `Main.qml` and polls' `VoteView.qml`
each carry the same `Connections { function onReplyReceived(...) }`
block -- and it is the only one of the two that keeps what the signal
carries: which action replied, whether it succeeded, and the payload.
Folding a failure into `lastError` would discard the action name and the
success half, leave `replyReceived` permanently unbound (so the audit
would need a forever-exemption for it, which is the opposite of what
morph#287 is for), and give the two views no place to clear a form only
once its submission was actually accepted.
`test_lims_qml_bridges.cpp` gains the matching bridge-level regression:
a real model refusal (rework on a `registered` sample) driven through
`submitIfValid` comes back as `replyReceived(type, false, message)`
carrying the model's own words, not a generic failure. The two
`replyReceived` entries leave the morph#287 backlog list, which the
audit itself enforces -- an exemption for a member QML does bind is a
finding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both bridges published a typed `Q_INVOKABLE` *and* a schema-driven form
for the same action. No QML called the typed half: `SampleView.qml`
submits RegisterClient/RegisterSample/ReturnForRework/RejectSample
through `submitIfValid` (lines 71, 91, 176, 191) and
`ResultEntryView.qml` submits CaptureConcentration/ResolveConflict the
same way (lines 99, 200). Two paths to one action is two places for the
behaviour to differ, and here they already did.
`captureReading(qlonglong, double, QString, double)` is the case worth
naming. It converted the reading with `Concentration::fromDouble` at the
field's *declared* precision, so it silently rounded an over-precise
reading that the form path submits exactly and the model refuses (README
decision 7) -- the refusal was unreachable from the GUI through it. It
was also the only `double` on this rung's QML surface, directly against
the convention `gui_lib/lims_qml_conversions.hpp` states: a `Quantity`
crosses as `valueText` + `hasValue`, never as a number a binding could
round.
Deleted, therefore:
SampleBridge bound, rejectSample, returnForRework
ResultBridge bound, sampleAttached, resultCaptured, conflictResolved,
captureReading, captureQualifier, resolveConflict
plus `SamplePresenter::rejectSample`/`returnForRework`,
`ResultPresenter::captureReading`/`captureQualifier`/`dispatchCapture`/
`resolveConflict` and the two presenter signals only those emitted --
nothing else called any of them.
`resultVerified` is *not* in that class, and the issue's pre-sort had it
wrong: its emitter `verifyResult` is bound by QML
(ResultEntryView.qml:201), so deleting the signal would have left a live
control whose success nothing reports. The view now handles it and
re-reads the listing, which is how the four-eyes verification reaches the
table at all -- previously the row stayed unchanged until somebody
pressed "Refresh results".
The exemption list shrinks from 15 to 2, and neither survivor is a
backlog entry. `SamplePresenter::submitIfValid` routes both registration
actions to the plain, key-less `_creator` handler, so through the form
path `RegisterClient` never emits `clientRegistered` (leaving the
`clientId` property SampleView.qml binds at -1) and `RegisterSample`
never leaves the *shared* handler attached (the follow-up `GetSample`
fails with "handler not bound"). Both were reproduced against the real
bridges before the exemptions were written; both are stated as
mechanisms a test can falsify, and the audit reports the exemption as
stale the moment either stops being true.
The two capture cases in `test_lims_qml_bridges.cpp` now drive
`submitIfValid` with a `glz::write_json` body -- the path the screen
uses -- and take their quiescence from `resultsListed`, which is also
what pins Main.qml's own `openSample` + `refreshResults` ordering.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lims was the last GUI rung still hand-rolling this: bookmarks, kanban,
ledger, pastebin and polls all call `morph::ladder::gui::errorText`, and
these four handlers were the same `try { rethrow } catch (const
std::exception&)` three-liner with **no `catch (...)` arm**.
That omission is not cosmetic. Per `examples/common/gui/error_text.hpp`,
these bodies run as `Completion` error callbacks, so an exception that is
not a `std::exception` escaping one unwinds out of the callback rather
than reaching the user -- the process goes down instead of the screen
showing a message. `errorText` handles that arm (and a null
`exception_ptr`) and is covered by its own suite,
`examples/common/testkit/test_error_text.cpp`.
Four sites, two per presenter: `reportError` (the typed invokables'
channel) and `submitIfValid`'s error arm (the schema-driven forms'
channel, which the views now handle -- see the previous two commits).
The visible text is unchanged for a `std::exception`: `what()`, decoded
as UTF-8.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
Every failed form submission in the lims GUI was silently swallowed. The presenters emit
replyReceived(actionType, false, what())on the error arm; the bridges forward it but feedlastErroronly fromfailed(); and QML boundreplyReceivednowhere.So the operator clicked Submit and got nothing back for an over-precise reading (decision 7), an
exactlyOneOfviolation (decision 6), a four-eyes refusal (decision 16), an unknown qualifier code (decision 18), or a conflict-resolution refusal — the rung's entire thesis.Pinned first, by running the surface audit with zero exemptions so the finding is structural rather than hand-asserted:
The fix shape, and why
replyReceivedis bound in the two QML views rather than foldingok == falseintolastError. Every other rung with areplyReceivedbinds it in QML (bookmarks/gui/qml/BookmarkListView.qml:184,pastebin/gui/qml/Main.qml:96,polls/gui/qml/VoteView.qml:98). Folding intolastErrorwould discard the action name and the success half, leavereplyReceivedneeding a permanent exemption, and give the views nowhere to clear a form only once it was accepted. Ledger'slastError-only shape is not a counter-example — ledger has noreplyReceived.#287 — reduced from 15 exemptions to 2, and the issue's pre-sort was backwards
Do not act on #287 as written. It calls
rejectSample/returnForRework/resolveConflict"missing controls" — the controls exist as schema-driven forms — and callsreplyReceived"plumbing to delete", which is the fix above.Deleted as genuinely redundant second dispatch paths:
rejectSample,returnForRework,captureReading,captureQualifier,resolveConflict, plusboundon both bridges and three signals whose only emitters were those calls.Deleting
captureReadingalso removes the onlydoubleon the QML surface. It tookdouble reading, double factorand converted withConcentration::fromDoubleat the declared precision — silently rounding a reading the form path submits exactly and the model refuses, making decision 7's refusal unreachable from the GUI.Three members the corrected pre-sort still got wrong, each verified rather than trusted:
resultVerifiedis not dead. Its emitterverifyResultis QML-bound, and nothing refreshed the table afterwards — a recorded four-eyes verification never appeared until someone pressed Refresh. Deleting it would have cemented that. Bound instead.registerClientis not redundant. It is the only dispatch emittingclientRegistered, hence the only thing that sets theclientIdthatSampleView.qml:77displays. Reproduced: after a form-path registration,clientId()is still-1.registerSampleis not redundant. It is the only dispatch that leaves the shared handler attached. Reproduced: the follow-upGetSamplefails withhandler not bound.Those last two are the surviving exemptions, and they are keyed to #309 — the schema-driven registration path is broken. Fixing that is what takes #287 to zero.
Also
Both presenters now route errors through the shared
morph::ladder::gui::errorText. lims was the only GUI rung not using it, and its four hand-rolled handlers had nocatch (...)arm — in aCompletionerror callback that takes the process down rather than reaching the user.Verification
ladder_lims_tests: 830 assertions / 128 cases (baseline on master: 821 / 126).ctest -L ladder-lims: 100% of 128. Clean under-Weverything -Werror;clang-formatclean;qmllintwarning count identical to a stashed baseline.Not run: coverage, sanitizers, non-lims targets.
This branch makes #309's failures visible. Form-path registration will now show a red
handler not boundwhere it previously failed silently. That is correct behaviour surfacing a pre-existing defect — not a regression introduced here — but it will read as one if #309 is not fixed close behind.Part of #304 (§A3). Reduces #287 to two exemptions; #309 closes it.
🤖 Generated with Claude Code