Skip to content

Commit 22173a7

Browse files
josephjclarkclaudeelias-ba
authored
Sandboxes: handle credentials on merge (#4835)
* add tests 1 fails when merging a credential from a sandbox * extra failing test * rename tests * Remap sandbox credentials onto parent during merge Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * remove unhelpful comments * remove comments * new test * in new credential dialog, add credentials to root project too * changelog * Sandboxes: address remaining merge failures (#4842) * fix: free up workflow name on merge/provisioner soft-delete A workflow deleted as a side effect of a merge kept its original name while hidden, so the (name, project_id) unique index still treated the name as taken. A later merge that recreated a workflow with that name was rejected, surfacing as a generic merge validation error. Apply the same name-freeing the UI delete path already uses (rename to name_del) at the provisioner soft-delete step, so every delete path releases the name consistently. * fix: surface nested validation errors on sandbox merge failure Merge failures whose error was nested in an association (a workflow name, a job credential) had no top-level error and collapsed into an opaque "validation error" message. Walk the changeset's nested errors so the real reason is shown instead. * fix: add sandbox credentials to the full ancestor chain on creation A credential created in a sandbox was added only to the sandbox and the root project, but a merge targets the immediate parent and can target any ancestor. For a sandbox nested two or more levels deep the credential never reached the parent and was dropped on merge. Pre-select the whole ancestor chain so it survives a merge into any ancestor. * refactor: define the workflow soft-delete transition once Both the workflows list (mark_for_deletion) and the provisioner (YAML, CLI, GitHub sync, sandbox merge) soft-delete workflows, but each spelled out the transition by hand: set deleted_at, then free the name. The name-freeing had drifted out of the provisioner path, which is what let a merge leave a hidden workflow holding its name. Extract a single soft_delete_changeset/1 in the Workflows context that sets deleted_at and frees the name together, and route both paths through it, so the transition has one definition and cannot drift again. Replaces the per-path rename helper. Adds a unit test on the primitive alongside the existing both-path coverage. * chore: trim comments and changelog to essentials * feat: classify sandbox merge failures into user-facing reasons The merge screen used to render a generic 'validation error', and an attempt to surface the raw changeset risked leaking schema field paths to users. Classify merge failures in the domain (Sandboxes.merge) into typed reasons: a named workflow-name conflict, or a generic validation failure. The view maps each to curated copy and no longer inspects changesets. The full validation detail is logged at warning level for diagnosis, so users get clean copy while engineers keep the specifics. * chore: update changelog wording for merge error message * feat: send unclassified merge failures to Sentry Split merge-failure logging by recognisability: a workflow name conflict is user-actionable, so it logs at warning. An unclassified validation failure is a mode we don't yet handle, so it logs at error, which reaches Sentry, giving proactive signal to recognise and name it. Detail stays in the log message (not metadata) so it remains visible to logs-only operators. * fix: tidy merge failure copy and stop leaking raw reasons Drop the 'contact support if it persists' wording (not a flash style used in this app) for the short, direct house style. Stop showing inspect(reason) to the user for an unexpected failure: log it at error (Sentry) and show a generic message instead. Keep passing through human-readable string errors such as the collection-sync message. * fix: satisfy credo and dialyzer Move require Logger after the alias block (credo ordering), and widen the merge_error type to include the string-error case the merge can actually return, so the spec matches the success typing. * test: assert nested credential pre-fill at the LiveView level The settings-page test still asserted the old root-only pre-fill; update it to expect the full ancestor chain, matching the ancestor-chain change. * refactor: drop unreachable binary merge-error handling A merge can never fail with a bare string: sync_collections raises rather than returning an error, and every import_document error path returns a changeset, a usage-limit message, or an atom. So the string-passthrough clauses (and the String.t() in merge_error) were dead. Remove them, and point the merge-failure view test at a real merge_error reason instead of a fabricated binary. * refactor: log raw merge-error messages instead of interpolating The placeholder interpolation only ran for messages carrying %{...} opts, so it was untested, and String.to_existing_atom/1 could raise on an unexpected placeholder key while formatting a failure for the log. Log the raw message instead: simpler, can't crash, and covered by the existing classify tests. * refactor: simplify merge-failure handling to a generic message Per review, drop the per-cause merge-error classification and bespoke user messages (the workflow-name-conflict case in particular was a collision with a hidden deleted row, so its advice wasn't actionable). The user now sees one generic message; usage-limit messages still pass through. Every merge failure is logged at error so it surfaces in Sentry, since a failed merge is sensitive and we want to be aware of it. * fix: add retry hint to the generic merge-failure message * feat: attach sandbox credentials to the target at merge time Replace the credential pre-fill (which speculatively attached new credentials to ancestor projects at creation) with merge-time attachment. The merge modal now diffs the sandbox's credentials against the target and shows the ones that would be dropped as a deselectable picklist, all selected by default. On merge, the selected credentials are attached to the chosen target inside the transaction, before the remap, so they carry over instead of being dropped; deselected ones stay dropped. default_project_credentials reverts to the active project only. * feat: select-all for the merge credential picklist; tighten heading Rename the heading to 'Credentials to add' and give the credential picklist the same tri-state select-all checkbox the workflows list has, with a matching toggle-all-credentials handler and a selected-count. * fix: keep credential selections across merge-modal form changes The credential checkboxes share the merge form, so toggling one fires select-merge-target, which was resetting the selection to all and snapping the box back. Preserve the current selection across the change (keeping ones still in the diff, defaulting newly-appeared credentials to selected), mirroring how workflow selections are preserved. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Elias Waly Ba <eliaswalyba@gmail.com>
1 parent 0e4836b commit 22173a7

17 files changed

Lines changed: 1017 additions & 81 deletions

File tree

CHANGELOG.md

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,7 @@ and this project adheres to
2828
- Consolidated run and work order state definitions into single source of truth
2929
by adding `Run.active_states/0`, `WorkOrder.states/0`, and
3030
`WorkOrder.active_states/0` and replacing all hardcoded state lists across the
31-
codebase
32-
[#4589](https://github.com/OpenFn/lightning/issues/4589)
31+
codebase [#4589](https://github.com/OpenFn/lightning/issues/4589)
3332

3433
### Fixed
3534

@@ -38,6 +37,16 @@ and this project adheres to
3837
single `Lightning.Collaboration.WorkflowResolver`, so the channel join and the
3938
session save path can no longer disagree on whether an id should INSERT or
4039
UPDATE. [#4830](https://github.com/OpenFn/lightning/issues/4830)
40+
- Ensure that credentials are properly transferred when merging a sandbox. This
41+
fixes a validation error which can occur on merge
42+
[#4831](https://github.com/OpenFn/lightning/issues/4831)
43+
- Free up a workflow's name when it is deleted by a merge, so a later merge can
44+
reuse that name [#4831](https://github.com/OpenFn/lightning/issues/4831)
45+
- Replace the generic "validation error" on a failed sandbox merge with a clear
46+
message, naming the conflicting workflow when there is one
47+
[#4831](https://github.com/OpenFn/lightning/issues/4831)
48+
- Add a credential created in a sandbox to its full ancestor chain, so it
49+
survives a merge into any ancestor
4150

4251
## [2.16.7] - 2026-06-04
4352

lib/lightning/projects/merge_projects.ex

Lines changed: 61 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -51,16 +51,26 @@ defmodule Lightning.Projects.MergeProjects do
5151
opts
5252
) do
5353
source_project =
54-
Repo.preload(source_project, workflows: [:jobs, :triggers, :edges])
54+
Repo.preload(source_project,
55+
workflows: [:jobs, :triggers, :edges],
56+
project_credentials: []
57+
)
5558

5659
target_project =
57-
Repo.preload(target_project, workflows: [:jobs, :triggers, :edges])
60+
Repo.preload(target_project,
61+
workflows: [:jobs, :triggers, :edges],
62+
project_credentials: []
63+
)
5864

59-
merge_project(
60-
Map.from_struct(source_project),
61-
Map.from_struct(target_project),
62-
opts
63-
)
65+
credential_map =
66+
build_credential_remap(
67+
source_project.project_credentials,
68+
target_project.project_credentials
69+
)
70+
71+
Map.from_struct(source_project)
72+
|> merge_project(Map.from_struct(target_project), opts)
73+
|> remap_document_credentials(credential_map)
6474
end
6575

6676
def merge_project(source_project, target_project, opts) do
@@ -552,10 +562,6 @@ defmodule Lightning.Projects.MergeProjects do
552562
|> then(fn job_attrs ->
553563
if target_job do
554564
job_attrs
555-
|> Map.put(
556-
:project_credential_id,
557-
target_job.project_credential_id
558-
)
559565
|> Map.put(
560566
:keychain_credential_id,
561567
target_job.keychain_credential_id
@@ -585,6 +591,50 @@ defmodule Lightning.Projects.MergeProjects do
585591
{new_mapping, merged_from_source ++ deleted_targets}
586592
end
587593

594+
# Builds a map of `source_project_credential_id => target_project_credential_id`
595+
# by matching on the shared underlying `credential_id`
596+
defp build_credential_remap(
597+
source_project_credentials,
598+
target_project_credentials
599+
) do
600+
target_by_credential =
601+
Map.new(target_project_credentials, &{&1.credential_id, &1.id})
602+
603+
Map.new(source_project_credentials, fn pc ->
604+
{pc.id, Map.get(target_by_credential, pc.credential_id)}
605+
end)
606+
end
607+
608+
defp remap_document_credentials(document, credential_map)
609+
when map_size(credential_map) == 0,
610+
do: document
611+
612+
defp remap_document_credentials(document, credential_map) do
613+
Map.update(document, "workflows", [], fn workflows ->
614+
Enum.map(workflows, &remap_workflow_credentials(&1, credential_map))
615+
end)
616+
end
617+
618+
defp remap_workflow_credentials(%{"jobs" => jobs} = workflow, credential_map) do
619+
Map.put(
620+
workflow,
621+
"jobs",
622+
Enum.map(jobs, &remap_job_credential(&1, credential_map))
623+
)
624+
end
625+
626+
defp remap_workflow_credentials(workflow, _credential_map), do: workflow
627+
628+
defp remap_job_credential(
629+
%{"project_credential_id" => pc_id} = job,
630+
credential_map
631+
)
632+
when not is_nil(pc_id) do
633+
Map.put(job, "project_credential_id", Map.get(credential_map, pc_id, pc_id))
634+
end
635+
636+
defp remap_job_credential(job, _credential_map), do: job
637+
588638
defp build_merged_triggers(source_triggers, target_triggers, trigger_mappings) do
589639
# Process source triggers (matched and new)
590640
{new_mapping, merged_from_source} =

lib/lightning/projects/provisioner.ex

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -687,10 +687,7 @@ defmodule Lightning.Projects.Provisioner do
687687
{true, others} when map_size(others) == 0 ->
688688
changeset
689689
|> Map.put(:changes, others)
690-
|> put_change(
691-
:deleted_at,
692-
DateTime.utc_now() |> DateTime.truncate(:second)
693-
)
690+
|> Workflows.soft_delete_changeset()
694691

695692
{true, others} when map_size(others) > 0 ->
696693
changeset

lib/lightning/projects/sandboxes.ex

Lines changed: 112 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,8 @@ defmodule Lightning.Projects.Sandboxes do
6262
alias Lightning.Workflows.WorkflowVersion
6363
alias Lightning.WorkflowVersions
6464

65+
require Logger
66+
6567
@typedoc """
6668
Attributes for creating a new sandbox via `provision/3`.
6769
@@ -163,31 +165,136 @@ defmodule Lightning.Projects.Sandboxes do
163165
* `source` - The sandbox project being merged
164166
* `target` - The project receiving the merge
165167
* `actor` - The user performing the merge
166-
* `opts` - Merge options (`:selected_workflow_ids`, `:deleted_target_workflow_ids`)
168+
* `opts` - Merge options (`:selected_workflow_ids`,
169+
`:deleted_target_workflow_ids`, `:selected_credential_ids`)
170+
171+
## Credential attachment
172+
173+
A credential that lives only in the sandbox (the target has no
174+
`project_credential` for its underlying `credential_id`) would otherwise be
175+
dropped on merge, since the remap only matches on shared credentials. Pass
176+
`:selected_credential_ids` (a list of the sandbox `project_credential` ids the
177+
caller chose to carry over) and each one is attached to the target before the
178+
document is imported, so the remap finds a match instead of dropping it.
179+
Sandbox `project_credentials` left out of the list stay dropped. Only regular
180+
`project_credentials` are handled here; keychain credentials are out of scope.
167181
168182
## Returns
169183
* `{:ok, updated_target}` - Merge succeeded
170-
* `{:error, reason}` - Workflow merge or collection sync failed
184+
* `{:error, merge_error}` - Merge failed, classified into a domain reason
185+
186+
Failures are returned as typed reasons (see `t:merge_error/0`) so callers can
187+
render user-facing copy without inspecting changeset internals. The full
188+
validation detail is logged for diagnosis.
171189
"""
190+
@type merge_error ::
191+
:merge_failed | Lightning.Extensions.UsageLimiting.message()
192+
172193
@spec merge(Project.t(), Project.t(), User.t(), map()) ::
173-
{:ok, Project.t()} | {:error, term()}
194+
{:ok, Project.t()} | {:error, merge_error()}
174195
def merge(
175196
%Project{} = source,
176197
%Project{} = target,
177198
%User{} = actor,
178199
opts \\ %{}
179200
) do
180-
merge_doc = MergeProjects.merge_project(source, target, opts)
201+
selected_credential_ids = Map.get(opts, :selected_credential_ids, [])
181202

182203
Repo.transact(fn ->
183-
with {:ok, updated_target} <-
204+
with :ok <-
205+
attach_selected_credentials(source, target, selected_credential_ids),
206+
# Re-preload so the credential remap sees the just-attached
207+
# associations; merge_project skips the preload if they're already loaded.
208+
target =
209+
Repo.preload(target, [project_credentials: []], force: true),
210+
merge_doc = MergeProjects.merge_project(source, target, opts),
211+
{:ok, updated_target} <-
184212
Provisioner.import_document(target, actor, merge_doc,
185213
allow_stale: true
186214
),
187215
{:ok, _} <- sync_collections(source, target) do
188216
{:ok, updated_target}
189217
end
190218
end)
219+
|> case do
220+
{:ok, _} = ok -> ok
221+
{:error, reason} -> {:error, classify_merge_error(reason)}
222+
end
223+
end
224+
225+
# Attaches the chosen sandbox-only credentials to the target so the merge
226+
# remap can match them. The credential diff is recomputed from the database
227+
# rather than trusting the caller's list verbatim: only sandbox
228+
# project_credentials whose underlying credential the target still lacks are
229+
# attached, and ON CONFLICT DO NOTHING guards against a concurrent attach.
230+
defp attach_selected_credentials(_source, _target, []), do: :ok
231+
232+
defp attach_selected_credentials(source, target, selected_credential_ids) do
233+
selected_set = MapSet.new(selected_credential_ids)
234+
235+
target_credential_ids =
236+
from(pc in ProjectCredential,
237+
where: pc.project_id == ^target.id,
238+
select: pc.credential_id
239+
)
240+
|> Repo.all()
241+
|> MapSet.new()
242+
243+
rows =
244+
from(pc in ProjectCredential,
245+
where: pc.project_id == ^source.id,
246+
select: %{id: pc.id, credential_id: pc.credential_id}
247+
)
248+
|> Repo.all()
249+
|> Enum.filter(fn pc ->
250+
MapSet.member?(selected_set, pc.id) and
251+
not MapSet.member?(target_credential_ids, pc.credential_id)
252+
end)
253+
|> build_target_credential_rows(target.id)
254+
255+
Repo.insert_all(ProjectCredential, rows,
256+
on_conflict: :nothing,
257+
conflict_target: [:project_id, :credential_id]
258+
)
259+
260+
:ok
261+
end
262+
263+
defp build_target_credential_rows(source_credentials, target_id) do
264+
now = DateTime.utc_now() |> DateTime.truncate(:second)
265+
266+
Enum.map(source_credentials, fn pc ->
267+
%{
268+
id: Ecto.UUID.generate(),
269+
project_id: target_id,
270+
credential_id: pc.credential_id,
271+
inserted_at: now,
272+
updated_at: now
273+
}
274+
end)
275+
end
276+
277+
# A failed merge is sensitive (it can block or lose a user's work), so every
278+
# failure is logged at :error to surface in Sentry. A usage-limit message is
279+
# an expected, user-actionable block, so it passes through unlogged.
280+
defp classify_merge_error(%Ecto.Changeset{} = changeset) do
281+
Logger.error(
282+
"Sandbox merge failed. #{inspect(merge_error_details(changeset))}"
283+
)
284+
285+
:merge_failed
286+
end
287+
288+
defp classify_merge_error(%{text: _} = usage_limit_message),
289+
do: usage_limit_message
290+
291+
defp classify_merge_error(reason) do
292+
Logger.error("Sandbox merge failed. #{inspect(reason)}")
293+
:merge_failed
294+
end
295+
296+
defp merge_error_details(changeset) do
297+
Ecto.Changeset.traverse_errors(changeset, fn {message, _opts} -> message end)
191298
end
192299

193300
@doc """

lib/lightning/workflows.ex

Lines changed: 34 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -717,16 +717,10 @@ defmodule Lightning.Workflows do
717717
where: t.workflow_id == ^workflow.id
718718
)
719719

720-
new_name = resolve_name_for_pending_deletion(workflow)
721-
722720
Multi.new()
723721
|> Multi.update(
724722
:workflow,
725-
workflow
726-
|> Workflow.request_deletion_changeset(%{
727-
"deleted_at" => DateTime.utc_now()
728-
})
729-
|> Ecto.Changeset.put_change(:name, new_name)
723+
soft_delete_changeset(Ecto.Changeset.change(workflow))
730724
)
731725
|> Multi.insert(:audit, Audit.marked_for_deletion(workflow.id, actor))
732726
|> Multi.update_all(
@@ -744,10 +738,39 @@ defmodule Lightning.Workflows do
744738
end)
745739
end
746740

747-
defp resolve_name_for_pending_deletion(%Workflow{
748-
name: name,
749-
project_id: project_id
750-
}) do
741+
@doc """
742+
Marks a workflow deleted and frees its name for reuse, in one step.
743+
744+
The single soft-delete transition both `mark_for_deletion/3` and the
745+
provisioner route through, so a deleted workflow can never keep its name
746+
reserved on a hidden row.
747+
"""
748+
@spec soft_delete_changeset(Ecto.Changeset.t(Workflow.t())) ::
749+
Ecto.Changeset.t(Workflow.t())
750+
def soft_delete_changeset(
751+
%Ecto.Changeset{data: %Workflow{} = workflow} = changeset
752+
) do
753+
changeset
754+
|> Workflow.request_deletion_changeset(%{
755+
deleted_at: DateTime.utc_now() |> DateTime.truncate(:second)
756+
})
757+
|> Ecto.Changeset.put_change(
758+
:name,
759+
resolve_name_for_pending_deletion(workflow)
760+
)
761+
end
762+
763+
@doc """
764+
Computes the `name_del`-style name a workflow should take when it is soft
765+
deleted, so it frees up its original name for reuse within the project.
766+
767+
Used by `soft_delete_changeset/1`, which every delete path routes through.
768+
"""
769+
@spec resolve_name_for_pending_deletion(Workflow.t()) :: String.t()
770+
def resolve_name_for_pending_deletion(%Workflow{
771+
name: name,
772+
project_id: project_id
773+
}) do
751774
base_name = "#{name}_del"
752775

753776
existing_names =

lib/lightning_web/live/credential_live/credential_index_component.ex

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -91,15 +91,9 @@ defmodule LightningWeb.CredentialLive.CredentialIndexComponent do
9191
def handle_event("show_modal", %{"target" => "new_credential"}, socket) do
9292
if socket.assigns.can_create_project_credential do
9393
project_credentials =
94-
if socket.assigns.project do
95-
[
96-
%Lightning.Projects.ProjectCredential{
97-
project_id: socket.assigns.project.id
98-
}
99-
]
100-
else
101-
[]
102-
end
94+
LightningWeb.CredentialLive.Helpers.default_project_credentials(
95+
socket.assigns.project
96+
)
10397

10498
{:noreply,
10599
assign(socket,

lib/lightning_web/live/credential_live/helpers.ex

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,18 @@ defmodule LightningWeb.CredentialLive.Helpers do
145145
}
146146
end
147147

148+
@doc """
149+
The `project_credentials` to pre-select when creating a credential: only the
150+
active project. Empty when there is no project context.
151+
"""
152+
@spec default_project_credentials(Lightning.Projects.Project.t() | nil) ::
153+
[Lightning.Projects.ProjectCredential.t()]
154+
def default_project_credentials(nil), do: []
155+
156+
def default_project_credentials(%Lightning.Projects.Project{id: id}) do
157+
[%Lightning.Projects.ProjectCredential{project_id: id}]
158+
end
159+
148160
def handle_save_response(socket, credential) do
149161
if socket.assigns[:on_save] do
150162
socket.assigns[:on_save].(credential)

0 commit comments

Comments
 (0)