feat: support full user targeting on workflow USER TASK#171
feat: support full user targeting on workflow USER TASK#171engalar wants to merge 2 commits intomendixlabs:mainfrom
Conversation
ako
left a comment
There was a problem hiding this comment.
Clean fix with thoughtful backward compatibility.
What's good
Backward compatibility done right. Parser reads both legacy UserSource and current UserTargeting fields, mapping old and new $Type variants to the same SDK types — projects from any Mendix version round-trip.
Grammar extension is non-breaking. TARGETING (USERS | GROUPS)? keeps existing scripts working.
Full pipeline coverage across all 7 layers (SDK, parser, writer, grammar, AST, visitor, executor, formatter).
Good test coverage — 6 parser tests covering all targeting variants including the legacy field.
Concerns
XPathConstraint fallback may be reversed. Parser checks XPath first, then falls back to XPathConstraint, but the writer always emits XPathConstraint. If real Mendix BSON uses XPathConstraint, the XPath check is dead code; if both exist in different versions, the priority should match what's actually used.
Group source types may be missing fields. MicroflowGroupSource only stores Microflow. If group targeting has additional fields (e.g., context variable like user targeting may have), they'd be silently lost. Worth verifying against a real Studio Pro export.
MULTI USER TASK grammar duplication. Identical targeting blocks in both USER TASK and MULTI USER TASK rules — could be factored into a userTargetingClause rule. Not blocking.
Visitor's group detection is coarse. len(utCtx.AllGROUPS()) > 0 flags both microflow and xpath as group if either uses GROUPS. Probably impossible in practice (only one targeting clause allowed) but worth a guard.
No DESCRIBE roundtrip test. Tests cover parsing only. A test that creates → describes → re-parses would catch formatter mismatches.
LGTM — fix is correct, concerns are minor improvements.
b25df6d to
e4ca07b
Compare
AI Code ReviewWhat Looks Good
RecommendationThe PR appears to be well-implemented and follows all the necessary guidelines from CLAUDE.md. It addresses the full stack consistently, maintains backward compatibility, follows MDL design principles, and includes appropriate test coverage as claimed in the PR body. Approve this pull request. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
…labs#169) Parser now reads both legacy UserSource and current UserTargeting BSON fields, recognizing all five $Type variants: MicroflowUserTargeting, XPathUserTargeting, MicroflowGroupTargeting, XPathGroupTargeting, and NoUserTargeting. MDL syntax extended with optional USERS/GROUPS modifier: TARGETING [USERS|GROUPS] MICROFLOW/XPATH.
e4ca07b to
5386fc4
Compare
ako
left a comment
There was a problem hiding this comment.
PR Review: feat: support full user targeting on workflow USER TASK
Overall: Clean, well-scoped PR that adds group targeting support for workflow user tasks. The implementation follows existing patterns correctly and the test coverage is solid. A few items to address:
Issues
1. DESCRIBE output for existing user targeting types misses USERS modifier
The DESCRIBE formatter (cmd_workflows.go:426-433) outputs TARGETING MICROFLOW and TARGETING XPATH for existing user-targeting types, but the new syntax supports TARGETING USERS MICROFLOW. While backward-compatible (the grammar makes USERS/GROUPS optional), this creates an asymmetry: group targeting always outputs TARGETING GROUPS MICROFLOW but user targeting never outputs TARGETING USERS MICROFLOW. If a user reads DESCRIBE output, they might not realize the USERS modifier exists. Consider whether DESCRIBE should emit the explicit form for clarity once group targeting is in play.
2. Writer serializes UserTargeting but parser reads both UserSource and UserTargeting — field name inconsistency
The writer (writer_workflow.go) serializes into the UserTargeting BSON field (the current name), which is correct. The parser reads both UserSource (legacy) and UserTargeting (current) — also correct. However, there's no comment or constant explaining which Mendix versions use which field name. The PR description mentions "Mendix 10.12+" for UserTargeting — a brief comment in parser_workflow.go at the fallback would help future maintainers.
3. NoUserTargeting not explicitly handled in parseUserSource
The default case in parseUserSource (parser_workflow.go:598) returns &workflows.NoUserSource{} which implicitly handles Workflows$NoUserTargeting. This works, but adding it as an explicit case alongside NoUserSource would make the code self-documenting and prevent accidental regression if someone later adds logging for the default case.
4. Grammar allows both TARGETING USERS MICROFLOW and TARGETING USERS XPATH on the same task
The grammar rule has two separate optional clauses:
(TARGETING (USERS | GROUPS)? MICROFLOW qualifiedName)?
(TARGETING (USERS | GROUPS)? XPATH STRING_LITERAL)?
This means a user could write both on the same task. The visitor code would set Kind twice (once to microflow, then overwritten by xpath). This is pre-existing behavior, not introduced by this PR, but worth noting.
5. Missing BSON fields for group sources
MicroflowGroupTargeting and XPathGroupTargeting — the parser doesn't read XPath as a fallback field for MicroflowGroupTargeting (unlike the existing MicroflowBasedUserSource which reads MicroflowExpression as fallback). Are there any additional BSON fields on these types? If you've verified against real Studio Pro output, this is fine — but worth confirming.
Positives
- Backward compatibility: Legacy
UserSourcefield is still read, withUserTargetingas fallback — existing MPR files continue to work. - Test coverage: 6 well-structured tests covering all targeting types including legacy.
- Full-stack wiring: Grammar, visitor, AST, executor (read + write), parser, writer, SDK types, and LSP completions all updated consistently.
- Clean scope: Single feature, single commit — follows the PR checklist.
Checklist items to verify
-
make grammarwas run (generated files are included, looks correct) - No
mdl-examples/doctype-tests/test files for the new syntax variants — the PR checklist says MDL syntax changes should include working examples - No version-gating for group targeting — if this requires Mendix 10.12+, should
checkFeature()be called? - Quick reference (
docs/01-project/MDL_QUICK_REFERENCE.md) not updated with the newGROUPSmodifier
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Follow-up review after ef0f3e5The new commit adds The substantive items from my previous review still apply:
Otherwise LGTM — the core implementation is solid. |
Fixes #169
Summary
UserSourceand currentUserTargetingBSON fields, recognizing all five$Typevariants (MicroflowUserTargeting,XPathUserTargeting,MicroflowGroupTargeting,XPathGroupTargeting,NoUserTargeting)MicroflowGroupSourceandXPathGroupSourceSDK types with full BSON serialization/deserializationUSERS/GROUPSmodifier:TARGETING [USERS|GROUPS] MICROFLOW/XPATHSyntax
Test plan
mxcli checkvalidates all syntax variants