Rules engine portal alignment#135
Conversation
… rules engine. Also switched the Portal to post to the queue on submit. Some other random refactoring and fixes.
Deployments
|
… with the outcome of the engine.
dfe-lance
left a comment
There was a problem hiding this comment.
Nice one — the alignment work here is solid, and the flow-config alignment test is a great guard. Before this merges I've got a few asks,
mainly to de-risk the merge with 285959_Split-out-Rules-Engine-worker (which removes the Azure Storage Queue architecture entirely, so the
worker-loop side of this PR is the bit that will collide hard).
-
Lift ProcessMessageBodyAsync out of RulesEngineWorker into a transport-neutral service.
Everything valuable in that method (parse → map → evaluate → record outcome → ticket, plus the record-before-Zendesk/idempotent-retry
ordering) is independent of how the message arrives. If it lives as an injectable IRequestProcessor.ProcessAsync(string body,
CancellationToken) in Application, with RulesEngineWorker reduced to a poll loop around it, then my branch's consumers can call the same
service and the merge conflict on the worker disappears instead of being a modify/delete archaeology job. Small refactor now, saves a big
one later. -
Land as two squashed commits rather than one.
One for the portal/rules alignment (mapper, AnswerFieldMap, RequestDocument, journey changes, tests) and one for the Azure queue wiring
(worker loop, Program.cs registrations, RequestQueueClient). The first will merge into my branch nearly clean; the second I'll be resolving
by taking my side — having them separated makes that mechanical. -
One config key for the queue name.
The PR has RulesEngineQueue (Web) and RulesEngineOptions:QueueName (worker) for the same queue. We've been bitten by duplicated queue
options before (#126 — web ended up binding neither and submit broke at runtime). Could we have a single shared key/options class? -
Three small code points worth fixing in this PR:
- DecisionOutcomeRepository uses ExecuteUpdateAsync, which skips SaveChanges — so the audit interceptor never fires and decisions land on
ChangeRequests with no audit entry, despite WorkerCurrentUserService existing to attribute them. Tracked update + save would keep the audit
trail (idempotency unaffected). - In RuleContextMapper, an unrecognised value for a RadioFanOut question sets every fan-out field to false — a combination the UI can't
produce, and it reads as definite "no" rather than "don't know". Suggest unlisted values map to Unknown (as TranslatedQuestions already
does) so crafted POSTs fail to Scrutiny, not to all-false. - {field}_code for autocomplete answers is taken from the form unvalidated, so the human-visible Value and the engine-routed RawValue can
disagree. Worth resolving the code server-side from the selected option rather than trusting the hidden input. - (Tiny: stray using AngleSharp; in Web/Program.cs.)
-
After this merges, can we treat the RequestDocument JSON as frozen unless coordinated? It'll carry across both queue implementations, so
any further renames or new required properties are a wire-format break on both sides. -
Separately — worth five minutes on a call: my branch already persists decisions on the Postgres side, and this PR adds
Outcome/OutcomeKey/MatchedRuleId to ChangeRequests. Let's agree the ChangeRequests columns are the canonical record the portal reads, so I
align to that once during the merge rather than us carrying two stores.
One deploy check: the worker now needs the Postgres connection string in its environment (AddPersistenceDependencies) and there's no
terraform change in the PR — the worker has no probes, so a missing conn string shows up as a crash-loop at deploy.
Uh oh!
There was an error while loading. Please reload this page.