fix(jtk): support unassigning issues via --assignee none#187
Conversation
Accept "none", "null", or empty string as assignee values to clear the assignee field. Works through both --assignee flag and --field assignee=none. jtk issues update PROJ-123 --assignee none jtk issues update PROJ-123 --field assignee=null Closes #177
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 8343ef6
Approved with 2 non-blocking suggestions below. Address at your discretion.
Summary
| Reviewer | Findings |
|---|---|
| database:sql-reviewer | 1 |
| harness-engineering:architecture-reviewer | 1 |
database:sql-reviewer (1 findings)
💡 Suggestion - tools/jtk/api/fields_test.go:231
FormatFieldValue test cases for the new null-user path cover 'none' and 'null' but not the empty string case (
value: ""), which is also handled by the code at fields.go:114. The assignee_test.go TestIsNullValue tests cover empty string thoroughly — adding at least one empty-string case here would close the gap.
harness-engineering:architecture-reviewer (1 findings)
💡 Suggestion - tools/jtk/api/fields.go:113
Null-value detection logic (TrimSpace + ToLower, checking 'none'/'null'/empty) is duplicated identically in
isNullValue()in assignee.go and inline here inFormatFieldValue(). Since theissuespackage already importsapi, consider exportingIsNullValuefrom theapipackage and calling it from both sites. If the set of sentinel values ever changes, both locations must be updated otherwise.
2 info-level observations excluded. Run with --verbose to include.
Completed in 4m 41s | $0.72
| Field | Value |
|---|---|
| Reviewers | hybrid-synthesis, database:sql-reviewer, security:code-auditor, harness-engineering:knowledge-reviewer, harness-engineering:enforcement-reviewer, harness-engineering:architecture-reviewer, harness-engineering:legibility-reviewer |
| Reviewed by | pr-review-daemon · monit-pr-reviewer |
| Duration | 4m 41s (Reviewers: 54s · Synthesis: 1m 08s) |
| Cost | $0.72 |
| Tokens | 375.9k in / 18.4k out |
| Turns | 54 |
tools/jtk/api/fields.go
Outdated
| } | ||
| return []string{value} | ||
| case "user": | ||
| lower := strings.ToLower(strings.TrimSpace(value)) |
There was a problem hiding this comment.
🔵 Low (harness-engineering:architecture-reviewer): Null-value detection logic (TrimSpace + ToLower, checking 'none'/'null'/empty) is duplicated identically in isNullValue() in assignee.go and inline here in FormatFieldValue(). Since the issues package already imports api, consider exporting IsNullValue from the api package and calling it from both sites. If the set of sentinel values ever changes, both locations must be updated otherwise.
| @@ -231,6 +231,30 @@ func TestFormatFieldValue(t *testing.T) { | |||
| value: "abc123", | |||
There was a problem hiding this comment.
🔵 Low (database:sql-reviewer): FormatFieldValue test cases for the new null-user path cover 'none' and 'null' but not the empty string case (value: ""), which is also handled by the code at fields.go:114. The assignee_test.go TestIsNullValue tests cover empty string thoroughly — adding at least one empty-string case here would close the gap.
TDD AssessmentOverall: PASS — test coverage is solid for the scope of this change. What's tested
What's not tested
VerdictThe unit-level coverage of both new functions ( |
4d0d4b5 to
f5b893a
Compare
Summary
none,null, or empty string as assignee values to sendnullto Jira API--assigneeflag and--field assignee=noneisNullValue()helper for consistent null detectionFormatFieldValue()to returnnilfor user fields with null valuesCloses #177
Examples
Test plan
isNullValue(none, null, empty, case variants)FormatFieldValuewith null user valuesmake build && make testpasses