|
| 1 | +# Code Review Notes: jira ScopeConfigId fix / ExtraJQL ProjectName feature |
| 2 | + |
| 3 | +Review date: 2026-07-30. Scope: two branches based on `main` @ `14a4e5bcd`: |
| 4 | + |
| 5 | +- `fix/jira-blueprint-scope-config` (commit `1a07d1062`) |
| 6 | +- `feature/expose-devlake-project-extrajql-variable` (commit `90239e1d9`) |
| 7 | + |
| 8 | +This file records what was found and fixed, so future agents don't have to |
| 9 | +re-derive the same context from scratch. All fixes described below were |
| 10 | +applied as uncommitted working-tree changes on their respective branch as of |
| 11 | +this review; check `git log`/`git status` on each branch to see whether |
| 12 | +they've since been committed. |
| 13 | + |
| 14 | +## fix/jira-blueprint-scope-config |
| 15 | + |
| 16 | +**What it does:** `makeDataSourcePipelinePlanV200` in |
| 17 | +`backend/plugins/jira/api/blueprint_v200.go` now threads the resolved |
| 18 | +`ScopeConfigId` into `JiraTaskOptions`, the way `ArgoCD`/`Linear` already do |
| 19 | +in their own `blueprint_v200.go`. Previously Jira relied solely on a runtime |
| 20 | +fallback in `impl.go`'s `PrepareTaskData` that re-looks-up the board by |
| 21 | +`connection_id + board_id` and copies `scope.ScopeConfigId` if the task |
| 22 | +option was zero. That fallback is fragile (e.g. it does nothing if |
| 23 | +`op.BoardId == 0`, and is generally an indirect way to get a value that's |
| 24 | +already known at plan-build time), so passing it explicitly is a real, |
| 25 | +justified fix and matches established plugin convention. |
| 26 | + |
| 27 | +**Findings (fixed):** |
| 28 | +1. `gofmt` failure — the new `ScopeConfigId` struct field broke alignment |
| 29 | + of the `JiraTaskOptions{}` literal. The repo's `.golangci.yaml` enables |
| 30 | + the `gofmt` formatter and CI runs `golangci-lint run`, so this would have |
| 31 | + failed CI. Fixed by running `gofmt -w`. |
| 32 | +2. Missing regression test — sibling plugins (`linear`, `bitbucket`, |
| 33 | + `gitlab`, `azuredevops_go`) all have a `blueprint_v200_test.go`; Linear's |
| 34 | + even has `TestMakePipelinePlanV200PassesScopeConfigId`, testing exactly |
| 35 | + this scenario. Jira had no such test. Added |
| 36 | + `backend/plugins/jira/api/blueprint_v200_test.go` modeled on Linear's, |
| 37 | + covering: ScopeConfig.ID takes priority over scope.ScopeConfigId, and |
| 38 | + scope.ScopeConfigId is used when ScopeConfig is nil/unconfigured. |
| 39 | + |
| 40 | +## feature/expose-devlake-project-extrajql-variable |
| 41 | + |
| 42 | +**What it does:** Exposes the DevLake *project* name (the logical grouping |
| 43 | +of scopes across data sources) as `{{.ProjectName}}` inside the `ExtraJQL` |
| 44 | +scope-config template, alongside the existing `{{.BoardName}}` / |
| 45 | +`{{.BoardId}}`. |
| 46 | + |
| 47 | +**Important design change made during this review — read before touching |
| 48 | +`ProjectName` resolution again:** |
| 49 | + |
| 50 | +The original implementation resolved `ProjectName` in `impl.go`'s |
| 51 | +`PrepareTaskData` by reverse-looking-up `project_mapping WHERE |
| 52 | +table='boards' AND row_id=<domain id of the board>`. This is broken for the |
| 53 | +feature's actual stated purpose (per the requester): **the same Jira board |
| 54 | +attached to multiple Devlake projects, each project filtering its own |
| 55 | +tickets via ExtraJQL.** `project_mapping`'s primary key is |
| 56 | +`(project_name, table, row_id)`, so a shared board has one row *per |
| 57 | +project*, and the reverse lookup can't tell which project the *current |
| 58 | +pipeline run* belongs to — it just grabs an arbitrary/first row. That means |
| 59 | +every project sharing a board would get the same (wrong, for all but one of |
| 60 | +them) `{{.ProjectName}}` value, silently. |
| 61 | + |
| 62 | +We looked at fixing this "properly" by threading `projectName` through |
| 63 | +`plugin.DataSourcePluginBlueprintV200.MakeDataSourcePipelinePlanV200(...)`, |
| 64 | +the interface every datasource plugin implements — but that's a framework |
| 65 | +interface change with a large blast radius (~15+ plugins), and was |
| 66 | +rejected as too big for this fix. |
| 67 | + |
| 68 | +**Actual fix implemented (small blast radius, no interface changes):** |
| 69 | +`backend/server/services/blueprint_makeplan_v200.go`'s `GeneratePlanJsonV200` |
| 70 | +already receives `projectName` as a parameter — it's the only place in the |
| 71 | +framework where "this pipeline run belongs to project X" and "these are its |
| 72 | +scopes" are known together unambiguously. After building `sourcePlans` (the |
| 73 | +per-connection plans returned by each plugin's |
| 74 | +`MakeDataSourcePipelinePlanV200`), it now generically injects |
| 75 | +`task.Options["projectName"] = projectName` into every task, for every |
| 76 | +plugin, when `projectName != ""`. This is safe because the mapstructure |
| 77 | +decoding used everywhere (`Decode`/`DecodeMapStruct`) silently ignores |
| 78 | +unknown map keys by default — plugins that don't declare a matching struct |
| 79 | +field simply never see it. (Note: `dora`/`refdiff` already manually put a |
| 80 | +`"projectName"` key into their own task options today, via |
| 81 | +`MakeMetricPluginPipelinePlanV200`'s own `projectName` parameter — this |
| 82 | +generic injection follows the same naming convention, just for |
| 83 | +`DataSourcePluginBlueprintV200` plugins that don't get `projectName` on |
| 84 | +their interface.) |
| 85 | + |
| 86 | +On the Jira side this actually *simplified* the implementation: |
| 87 | +- `backend/plugins/jira/tasks/task_data.go`: `JiraOptions` gained a |
| 88 | + `ProjectName string` field (mapstructure tag `projectName,omitempty`), |
| 89 | + populated purely by decoding task options — no plugin-side logic needed. |
| 90 | +- `backend/plugins/jira/impl/impl.go`: `PrepareTaskData` now just does |
| 91 | + `ProjectName: op.ProjectName` when building `JiraTaskData`. The |
| 92 | + `project_mapping`/`crossdomain`/`didgen` reverse-lookup code and its |
| 93 | + imports were deleted entirely — there's no more ambiguity to handle. |
| 94 | +- Test coverage: `backend/server/services/blueprint_makeplan_v200_test.go` |
| 95 | + gained `TestMakePlanV200InjectsProjectNameIntoDataSourceTaskOptions`, |
| 96 | + asserting the injected key lands in a data-source plugin's task options |
| 97 | + alongside its own options, using `mock.Anything` for the "org" plugin's |
| 98 | + `MapProject` call (re-registering "org" per-test to avoid cross-test mock |
| 99 | + state leaking via the global `plugin` registry — see that file for why). |
| 100 | + |
| 101 | +**Other findings (fixed):** |
| 102 | +1. `gofmt` failure — `JqlTemplateData` struct and the `JqlTemplateData{}` |
| 103 | + literal in `issue_collector.go` used tabs for alignment instead of |
| 104 | + spaces; new `ProjectName` field wasn't aligned either. Fixed with |
| 105 | + `gofmt -w`. |
| 106 | +2. Zero test coverage for the new variable — `issue_collector_test.go`'s |
| 107 | + existing `Test_renderExtraJQL` test has a `makeData` helper whose third |
| 108 | + parameter (`_ string`) was unused and looked purpose-built for exactly |
| 109 | + this addition, but nothing wired it up. Fixed: the helper now sets |
| 110 | + `ProjectName` from that parameter, and new subtests exercise |
| 111 | + `{{.ProjectName}}` substitution (present and empty/unmapped board). This |
| 112 | + part of the test is still valid after the `ProjectName`-resolution |
| 113 | + redesign above, since it tests `renderExtraJQL` given an already-built |
| 114 | + `JiraTaskData`, independent of how `ProjectName` gets populated upstream. |
| 115 | +3. Config-UI help tooltip |
| 116 | + (`config-ui/src/plugins/register/jira/transformation.tsx`) documented |
| 117 | + only `{{.BoardName}}`/`{{.BoardId}}`. Updated to also mention |
| 118 | + `{{.ProjectName}}` so the feature is discoverable from the UI. |
| 119 | + |
| 120 | +## Verification performed |
| 121 | + |
| 122 | +- `go build ./plugins/jira/...`, `./server/services/...` on both branches |
| 123 | + (fix branch built in an isolated git worktree at the time of review). |
| 124 | +- `go test ./plugins/jira/api/... ./plugins/jira/tasks/... ./plugins/jira/impl/... ./server/services/...` |
| 125 | + on both branches. Pre-existing `plugins/jira/e2e` requires `E2E_DB_URL` |
| 126 | + and is expected to fail/skip outside a real DB — unrelated to these |
| 127 | + changes. `go build ./...` at the repo root also fails on unrelated |
| 128 | + pre-existing issues in this sandbox (no `libgit2` for `gitextractor`; |
| 129 | + `plugins/org` and other plugin packages are `main` packages meant to be |
| 130 | + built with `-buildmode=plugin`, not as ordinary binaries) — neither is a |
| 131 | + regression from these branches. |
| 132 | +- `gofmt -l` on all touched files, before and after fixes, on both |
| 133 | + branches. |
| 134 | +- Generating `backend/mocks/{core,helpers}` via `mockery` was required to |
| 135 | + run `server/services` tests locally (`make mock`, or the two `mockery |
| 136 | + --recursive ...` commands in `backend/Makefile`); that directory is |
| 137 | + gitignored and not checked in. |
0 commit comments