Skip to content

feat: support full user targeting on workflow USER TASK#171

Open
engalar wants to merge 2 commits intomendixlabs:mainfrom
engalar:fix/issue-169-workflow-target-users
Open

feat: support full user targeting on workflow USER TASK#171
engalar wants to merge 2 commits intomendixlabs:mainfrom
engalar:fix/issue-169-workflow-target-users

Conversation

@engalar
Copy link
Copy Markdown
Contributor

@engalar engalar commented Apr 10, 2026

Fixes #169

Summary

  • Parser now reads both legacy UserSource and current UserTargeting BSON fields, recognizing all five $Type variants (MicroflowUserTargeting, XPathUserTargeting, MicroflowGroupTargeting, XPathGroupTargeting, NoUserTargeting)
  • Added MicroflowGroupSource and XPathGroupSource SDK types with full BSON serialization/deserialization
  • MDL grammar extended with optional USERS/GROUPS modifier: TARGETING [USERS|GROUPS] MICROFLOW/XPATH
  • DESCRIBE output now correctly displays all targeting types including group targeting

Syntax

-- User targeting (existing, backward compatible)
TARGETING MICROFLOW Module.GetUsers
TARGETING XPATH '[System.UserRoles = ''[%UserRole_Manager%]'']'

-- Explicit user targeting (new)
TARGETING USERS MICROFLOW Module.GetUsers
TARGETING USERS XPATH '[...]'

-- Group targeting (new)
TARGETING GROUPS MICROFLOW Module.GetGroups
TARGETING GROUPS XPATH '[GroupType = ''Reviewers'']'

Test plan

  • 6 new parser tests covering all targeting types (user microflow/xpath, group microflow/xpath, no targeting, legacy UserSource)
  • mxcli check validates all syntax variants
  • Full test suite passes
  • Build succeeds

This was referenced Apr 10, 2026
Copy link
Copy Markdown
Collaborator

@ako ako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@engalar engalar force-pushed the fix/issue-169-workflow-target-users branch from b25df6d to e4ca07b Compare April 13, 2026 05:10
@github-actions
Copy link
Copy Markdown

AI Code Review

What Looks Good

  • The implementation correctly extends existing MDL syntax rather than creating new statements, maintaining consistency with current patterns
  • Reuses existing AST structure efficiently (same Microflow/XPath fields for both user and group targeting)
  • Follows the established pattern for optional modifiers in MDL grammar
  • Includes both USERS (explicit) and GROUPS options for clarity and explicit intent
  • Maintains backward compatibility with legacy UserSource targeting
  • Properly updates all layers of the stack: grammar → AST → visitor (implied) → executor → LSP
  • Includes DESCRIBE roundtrip consideration in formatting logic that outputs re-executable MDL
  • Updates LSP completions for proper IDE support in VS Code extension

Recommendation

The 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.
@engalar engalar force-pushed the fix/issue-169-workflow-target-users branch from e4ca07b to 5386fc4 Compare April 13, 2026 06:14
Copy link
Copy Markdown
Collaborator

@ako ako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 UserSource field is still read, with UserTargeting as 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 grammar was 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 new GROUPS modifier

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@engalar engalar requested a review from ako April 13, 2026 07:35
@ako
Copy link
Copy Markdown
Collaborator

ako commented Apr 13, 2026

Follow-up review after ef0f3e5

The new commit adds GROUPS to the keyword rule in MDLParser.g4 — correct fix for TestKeywordRuleCoverage. No issues with this change.

The substantive items from my previous review still apply:

  1. DESCRIBE asymmetry — user targeting outputs TARGETING MICROFLOW while group targeting outputs TARGETING GROUPS MICROFLOW. Consider emitting TARGETING USERS MICROFLOW for symmetry.
  2. Version comment — the UserSourceUserTargeting field name transition (Mendix 10.12+) deserves a comment in parser_workflow.go. (Partially addressed — the comments are there now but a version note would help.)
  3. Explicit NoUserTargeting case — still handled implicitly by the default branch in parseUserSource.
  4. Missing mdl-examples/doctype-tests/ — no working MDL examples for the new syntax variants.
  5. No version-gating — if group targeting requires Mendix 10.12+, should checkFeature() guard it?
  6. Quick reference not updateddocs/01-project/MDL_QUICK_REFERENCE.md doesn't mention the GROUPS modifier.

Otherwise LGTM — the core implementation is solid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support TARGET USERS clause on workflow USER TASK definitions

2 participants