diff --git a/CODE_REVIEW_NOTES.md b/CODE_REVIEW_NOTES.md new file mode 100644 index 00000000000..8bb4370e24c --- /dev/null +++ b/CODE_REVIEW_NOTES.md @@ -0,0 +1,137 @@ +# Code Review Notes: jira ScopeConfigId fix / ExtraJQL ProjectName feature + +Review date: 2026-07-30. Scope: two branches based on `main` @ `14a4e5bcd`: + +- `fix/jira-blueprint-scope-config` (commit `1a07d1062`) +- `feature/expose-devlake-project-extrajql-variable` (commit `90239e1d9`) + +This file records what was found and fixed, so future agents don't have to +re-derive the same context from scratch. All fixes described below were +applied as uncommitted working-tree changes on their respective branch as of +this review; check `git log`/`git status` on each branch to see whether +they've since been committed. + +## fix/jira-blueprint-scope-config + +**What it does:** `makeDataSourcePipelinePlanV200` in +`backend/plugins/jira/api/blueprint_v200.go` now threads the resolved +`ScopeConfigId` into `JiraTaskOptions`, the way `ArgoCD`/`Linear` already do +in their own `blueprint_v200.go`. Previously Jira relied solely on a runtime +fallback in `impl.go`'s `PrepareTaskData` that re-looks-up the board by +`connection_id + board_id` and copies `scope.ScopeConfigId` if the task +option was zero. That fallback is fragile (e.g. it does nothing if +`op.BoardId == 0`, and is generally an indirect way to get a value that's +already known at plan-build time), so passing it explicitly is a real, +justified fix and matches established plugin convention. + +**Findings (fixed):** +1. `gofmt` failure — the new `ScopeConfigId` struct field broke alignment + of the `JiraTaskOptions{}` literal. The repo's `.golangci.yaml` enables + the `gofmt` formatter and CI runs `golangci-lint run`, so this would have + failed CI. Fixed by running `gofmt -w`. +2. Missing regression test — sibling plugins (`linear`, `bitbucket`, + `gitlab`, `azuredevops_go`) all have a `blueprint_v200_test.go`; Linear's + even has `TestMakePipelinePlanV200PassesScopeConfigId`, testing exactly + this scenario. Jira had no such test. Added + `backend/plugins/jira/api/blueprint_v200_test.go` modeled on Linear's, + covering: ScopeConfig.ID takes priority over scope.ScopeConfigId, and + scope.ScopeConfigId is used when ScopeConfig is nil/unconfigured. + +## feature/expose-devlake-project-extrajql-variable + +**What it does:** Exposes the DevLake *project* name (the logical grouping +of scopes across data sources) as `{{.ProjectName}}` inside the `ExtraJQL` +scope-config template, alongside the existing `{{.BoardName}}` / +`{{.BoardId}}`. + +**Important design change made during this review — read before touching +`ProjectName` resolution again:** + +The original implementation resolved `ProjectName` in `impl.go`'s +`PrepareTaskData` by reverse-looking-up `project_mapping WHERE +table='boards' AND row_id=`. This is broken for the +feature's actual stated purpose (per the requester): **the same Jira board +attached to multiple Devlake projects, each project filtering its own +tickets via ExtraJQL.** `project_mapping`'s primary key is +`(project_name, table, row_id)`, so a shared board has one row *per +project*, and the reverse lookup can't tell which project the *current +pipeline run* belongs to — it just grabs an arbitrary/first row. That means +every project sharing a board would get the same (wrong, for all but one of +them) `{{.ProjectName}}` value, silently. + +We looked at fixing this "properly" by threading `projectName` through +`plugin.DataSourcePluginBlueprintV200.MakeDataSourcePipelinePlanV200(...)`, +the interface every datasource plugin implements — but that's a framework +interface change with a large blast radius (~15+ plugins), and was +rejected as too big for this fix. + +**Actual fix implemented (small blast radius, no interface changes):** +`backend/server/services/blueprint_makeplan_v200.go`'s `GeneratePlanJsonV200` +already receives `projectName` as a parameter — it's the only place in the +framework where "this pipeline run belongs to project X" and "these are its +scopes" are known together unambiguously. After building `sourcePlans` (the +per-connection plans returned by each plugin's +`MakeDataSourcePipelinePlanV200`), it now generically injects +`task.Options["projectName"] = projectName` into every task, for every +plugin, when `projectName != ""`. This is safe because the mapstructure +decoding used everywhere (`Decode`/`DecodeMapStruct`) silently ignores +unknown map keys by default — plugins that don't declare a matching struct +field simply never see it. (Note: `dora`/`refdiff` already manually put a +`"projectName"` key into their own task options today, via +`MakeMetricPluginPipelinePlanV200`'s own `projectName` parameter — this +generic injection follows the same naming convention, just for +`DataSourcePluginBlueprintV200` plugins that don't get `projectName` on +their interface.) + +On the Jira side this actually *simplified* the implementation: +- `backend/plugins/jira/tasks/task_data.go`: `JiraOptions` gained a + `ProjectName string` field (mapstructure tag `projectName,omitempty`), + populated purely by decoding task options — no plugin-side logic needed. +- `backend/plugins/jira/impl/impl.go`: `PrepareTaskData` now just does + `ProjectName: op.ProjectName` when building `JiraTaskData`. The + `project_mapping`/`crossdomain`/`didgen` reverse-lookup code and its + imports were deleted entirely — there's no more ambiguity to handle. +- Test coverage: `backend/server/services/blueprint_makeplan_v200_test.go` + gained `TestMakePlanV200InjectsProjectNameIntoDataSourceTaskOptions`, + asserting the injected key lands in a data-source plugin's task options + alongside its own options, using `mock.Anything` for the "org" plugin's + `MapProject` call (re-registering "org" per-test to avoid cross-test mock + state leaking via the global `plugin` registry — see that file for why). + +**Other findings (fixed):** +1. `gofmt` failure — `JqlTemplateData` struct and the `JqlTemplateData{}` + literal in `issue_collector.go` used tabs for alignment instead of + spaces; new `ProjectName` field wasn't aligned either. Fixed with + `gofmt -w`. +2. Zero test coverage for the new variable — `issue_collector_test.go`'s + existing `Test_renderExtraJQL` test has a `makeData` helper whose third + parameter (`_ string`) was unused and looked purpose-built for exactly + this addition, but nothing wired it up. Fixed: the helper now sets + `ProjectName` from that parameter, and new subtests exercise + `{{.ProjectName}}` substitution (present and empty/unmapped board). This + part of the test is still valid after the `ProjectName`-resolution + redesign above, since it tests `renderExtraJQL` given an already-built + `JiraTaskData`, independent of how `ProjectName` gets populated upstream. +3. Config-UI help tooltip + (`config-ui/src/plugins/register/jira/transformation.tsx`) documented + only `{{.BoardName}}`/`{{.BoardId}}`. Updated to also mention + `{{.ProjectName}}` so the feature is discoverable from the UI. + +## Verification performed + +- `go build ./plugins/jira/...`, `./server/services/...` on both branches + (fix branch built in an isolated git worktree at the time of review). +- `go test ./plugins/jira/api/... ./plugins/jira/tasks/... ./plugins/jira/impl/... ./server/services/...` + on both branches. Pre-existing `plugins/jira/e2e` requires `E2E_DB_URL` + and is expected to fail/skip outside a real DB — unrelated to these + changes. `go build ./...` at the repo root also fails on unrelated + pre-existing issues in this sandbox (no `libgit2` for `gitextractor`; + `plugins/org` and other plugin packages are `main` packages meant to be + built with `-buildmode=plugin`, not as ordinary binaries) — neither is a + regression from these branches. +- `gofmt -l` on all touched files, before and after fixes, on both + branches. +- Generating `backend/mocks/{core,helpers}` via `mockery` was required to + run `server/services` tests locally (`make mock`, or the two `mockery + --recursive ...` commands in `backend/Makefile`); that directory is + gitignored and not checked in. diff --git a/backend/plugins/jira/impl/impl.go b/backend/plugins/jira/impl/impl.go index b27ea9daa66..ac07653fd57 100644 --- a/backend/plugins/jira/impl/impl.go +++ b/backend/plugins/jira/impl/impl.go @@ -255,6 +255,10 @@ func (p Jira) PrepareTaskData(taskCtx plugin.TaskContext, options map[string]int ApiClient: jiraApiClient, JiraServerInfo: *info, Board: scope, + // op.ProjectName is injected by services.GeneratePlanJsonV200 at + // blueprint-plan-generation time, when the running Devlake project is + // known unambiguously. + ProjectName: op.ProjectName, } return taskData, nil diff --git a/backend/plugins/jira/tasks/issue_collector.go b/backend/plugins/jira/tasks/issue_collector.go index fe1939b721b..49d18904c5d 100644 --- a/backend/plugins/jira/tasks/issue_collector.go +++ b/backend/plugins/jira/tasks/issue_collector.go @@ -112,8 +112,9 @@ func CollectIssues(taskCtx plugin.SubTaskContext) errors.Error { // JqlTemplateData holds the variables available inside an ExtraJQL template. // Users reference these with Go template syntax, e.g. `{{.BoardName}}`. type JqlTemplateData struct { - BoardId uint64 // numeric ID of the connected Jira board - BoardName string // display name of the connected Jira board + BoardId uint64 // numeric ID of the connected Jira board + BoardName string // display name of the connected Jira board + ProjectName string // Devlake project name associated with the Jira board scope } // renderExtraJQL executes the ExtraJQL scope-config field as a Go text/template, @@ -133,7 +134,8 @@ func renderExtraJQL(tmplStr string, data *JiraTaskData) (string, errors.Error) { } vars := JqlTemplateData{ - BoardId: data.Options.BoardId, + BoardId: data.Options.BoardId, + ProjectName: data.ProjectName, } if data.Board != nil { vars.BoardName = data.Board.Name diff --git a/backend/plugins/jira/tasks/issue_collector_test.go b/backend/plugins/jira/tasks/issue_collector_test.go index 50c66947c59..501a7246e98 100644 --- a/backend/plugins/jira/tasks/issue_collector_test.go +++ b/backend/plugins/jira/tasks/issue_collector_test.go @@ -143,10 +143,11 @@ func Test_buildFilterJQL(t *testing.T) { } func Test_renderExtraJQL(t *testing.T) { - makeData := func(boardId uint64, boardName string, _ string) *JiraTaskData { + makeData := func(boardId uint64, boardName string, projectName string) *JiraTaskData { return &JiraTaskData{ - Options: &JiraOptions{BoardId: boardId}, - Board: &models.JiraBoard{BoardId: boardId, Name: boardName}, + Options: &JiraOptions{BoardId: boardId}, + Board: &models.JiraBoard{BoardId: boardId, Name: boardName}, + ProjectName: projectName, } } @@ -181,6 +182,18 @@ func Test_renderExtraJQL(t *testing.T) { data: &JiraTaskData{Options: &JiraOptions{BoardId: 1}, Board: nil}, want: `project = ""`, }, + { + name: "ProjectName substitution", + tmpl: `labels = "{{.ProjectName}}"`, + data: makeData(1, "My Board", "My Devlake Project"), + want: `labels = "My Devlake Project"`, + }, + { + name: "board with no mapped Devlake project falls back to empty ProjectName", + tmpl: `labels = "{{.ProjectName}}"`, + data: makeData(1, "My Board", ""), + want: `labels = ""`, + }, { name: "invalid template returns error", tmpl: `project = "{{.Unclosed"`, diff --git a/backend/plugins/jira/tasks/task_data.go b/backend/plugins/jira/tasks/task_data.go index 3505bf162e8..a49b3a8f3f7 100644 --- a/backend/plugins/jira/tasks/task_data.go +++ b/backend/plugins/jira/tasks/task_data.go @@ -31,6 +31,13 @@ type JiraOptions struct { ScopeConfig *models.JiraScopeConfig `json:"scopeConfig" mapstructure:"scopeConfig"` ScopeConfigId uint64 `json:"scopeConfigId" mapstructure:"scopeConfigId"` PageSize int `json:"pageSize" mapstructure:"pageSize"` + // ProjectName is the Devlake project this pipeline run belongs to. It is + // injected generically into every plugin's task options by + // services.GeneratePlanJsonV200, not set by Jira's own blueprint plan + // builder, since that's the only place in the framework where the + // running project is known unambiguously (a board scope can be attached + // to more than one Devlake project). + ProjectName string `json:"projectName" mapstructure:"projectName,omitempty"` } type JiraTaskData struct { @@ -39,6 +46,7 @@ type JiraTaskData struct { JiraServerInfo models.JiraServerInfo FilterId string Board *models.JiraBoard + ProjectName string } type JiraApiParams models.JiraApiParams diff --git a/backend/server/services/blueprint_makeplan_v200.go b/backend/server/services/blueprint_makeplan_v200.go index bd89d7de3e2..73ee897ad76 100644 --- a/backend/server/services/blueprint_makeplan_v200.go +++ b/backend/server/services/blueprint_makeplan_v200.go @@ -70,6 +70,23 @@ func GeneratePlanJsonV200( } } + // let every task know which Devlake project it's running for, so plugins + // can use it (e.g. Jira's ExtraJQL {{.ProjectName}}) without needing + // project context threaded through their own blueprint-plan-building + // logic. Unrecognized map keys are ignored by mapstructure decoding, so + // this is safe for plugins that don't care about it. + if projectName != "" { + for _, plan := range sourcePlans { + for _, stage := range plan { + for _, task := range stage { + if task.Options != nil { + task.Options["projectName"] = projectName + } + } + } + } + } + // skip collectors if skipCollectors { for i, plan := range sourcePlans { diff --git a/backend/server/services/blueprint_makeplan_v200_test.go b/backend/server/services/blueprint_makeplan_v200_test.go index 22b53b53257..0b3943ae97f 100644 --- a/backend/server/services/blueprint_makeplan_v200_test.go +++ b/backend/server/services/blueprint_makeplan_v200_test.go @@ -29,6 +29,7 @@ import ( mockplugin "github.com/apache/incubator-devlake/mocks/core/plugin" "github.com/apache/incubator-devlake/plugins/org/tasks" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" ) func TestMakePlanV200(t *testing.T) { @@ -101,3 +102,45 @@ func TestMakePlanV200(t *testing.T) { assert.Equal(t, expectedPlan, plan) } + +// TestMakePlanV200InjectsProjectNameIntoDataSourceTaskOptions verifies that +// GeneratePlanJsonV200 stamps the running Devlake project's name into every +// data-source task's options, without requiring the plugin's own +// MakeDataSourcePipelinePlanV200 implementation to know about it. This is +// what lets a scope shared by multiple projects (e.g. a Jira board added to +// two different Devlake projects) resolve the correct project per pipeline +// run instead of guessing from a many-to-one reverse lookup. +func TestMakePlanV200InjectsProjectNameIntoDataSourceTaskOptions(t *testing.T) { + const projectName = "TestMakePlanV200InjectsProjectName-project" + jiraName := "TestMakePlanV200InjectsProjectName-jira" + connId := uint64(1) + scopes := []*coreModels.BlueprintScope{{ScopeId: "jira:JiraBoard:1:1"}} + outputPlan := coreModels.PipelinePlan{ + { + {Plugin: jiraName, Options: map[string]interface{}{"boardId": float64(1)}}, + }, + } + jira := new(mockplugin.CompositeDataSourcePluginBlueprintV200) + jira.On("MakeDataSourcePipelinePlanV200", connId, scopes).Return(outputPlan, []plugin.Scope(nil), nil) + plugin.RegisterPlugin(jiraName, jira) + + // GeneratePlanJsonV200 also calls the "org" plugin's ProjectMapper + // whenever projectName != "". Re-register it here (overwriting any + // registration from other tests in this package) with permissive + // matchers, since this test doesn't care about project_mapping. + org := new(mockplugin.CompositeProjectMapper) + org.On("MapProject", mock.Anything, mock.Anything).Return(coreModels.PipelinePlan{}, nil) + plugin.RegisterPlugin("org", org) + + connections := []*coreModels.BlueprintConnection{ + {PluginName: jiraName, ConnectionId: connId, Scopes: scopes}, + } + + plan, err := GeneratePlanJsonV200(projectName, connections, nil, false) + assert.Nil(t, err) + assert.Equal(t, 1, len(plan)) + assert.Equal(t, 1, len(plan[0])) + assert.Equal(t, projectName, plan[0][0].Options["projectName"]) + // the plugin's own option is preserved alongside the injected one + assert.Equal(t, float64(1), plan[0][0].Options["boardId"]) +} diff --git a/config-ui/src/plugins/register/jira/transformation.tsx b/config-ui/src/plugins/register/jira/transformation.tsx index bbfe760dba0..3db9116785b 100644 --- a/config-ui/src/plugins/register/jira/transformation.tsx +++ b/config-ui/src/plugins/register/jira/transformation.tsx @@ -269,7 +269,7 @@ const renderCollapseItems = ({ label={ <> Extra JQL - + } extra='Tip: use Go template variables to make this dynamic, e.g. owner = "{{.BoardName}}"'