4002 sandboxes UI to select workflows on merge#4518
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4518 +/- ##
==========================================
- Coverage 89.55% 89.48% -0.07%
==========================================
Files 425 425
Lines 20307 20422 +115
==========================================
+ Hits 18185 18274 +89
- Misses 2122 2148 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@doc-han could you attach a screenshot please? |
@brandonjackson I've attached some visuals to the description |
|
Thanks! Looks great. Can you make these copy changes as well while you're working on this component? cc @theroinaochieng |
alright |
|
I haven't checked the PR but I'd love to only suggest workflows that have changed in the sandbox. Unchanged workflows should be deselected. Maybe even greyed out? (less sure on that so for now I think just deselect please) When running QA on this, please can we verify:
Maybe worth remembering in test that there are several types of change that we might see
If this branch fails any of these tests I think we need to create a new issue because main will fail too. I just want to run these tests while we have focus on it |
|
@doc-han if a workflow is deleted from the sandbox, it needs to count as a change (type |
josephjclark
left a comment
josephjclark
left a comment
There was a problem hiding this comment.
Here's how it looks
Looks fantastic to me!
I've still got "only select changed workflows by default" on my wishlist. But this is a huge improvement and I think we should spin that out into a new issue.
@theroinaochieng I could use help getting a technical review on this, then I'd like to ship it. Huge user benefit.
midigofrank
left a comment
midigofrank
left a comment
There was a problem hiding this comment.
Hey @doc-han , great work here. I've left 2 refactor change requests. Also I see you have used the capture operator, &, quite a lot. I'm not a huge fan of them because they conceal what their source is. Given that the value could be anything, it is better if you name the variable
| |> optimistic_lock(:lock_version) | ||
| |> validate_required([:id]) | ||
| |> maybe_mark_for_deletion() | ||
| |> maybe_soft_delete_workflow() |
| &(&1.id == (default_target && default_target.value)) | ||
| ) | ||
|
|
||
| source_workflows = |
There was a problem hiding this comment.
Could we move this to a private function?
| target_workflows | ||
| |> Enum.reject(fn wf -> MapSet.member?(source_workflow_names, wf.name) end) | ||
| |> Enum.map(fn wf -> | ||
| %{ |
There was a problem hiding this comment.
I'm seeing this map built by hand in different places. This is slightly brittle because someone might forget to the change in all 3 places. Could you move this to a private function? Or maybe you could define a struct at the top:
defmodule LightningWeb.SandboxLive.Index do
defmodule MergeWorkflow do
defstruct [:id, :name, :is_diverged, :is_new, :is_deleted]
end
...
end 4 participants
Description
This PR allows user to select the specific workflows they want to merge during a sandbox merge
Closes #4002
Validation steps
Additional notes for the reviewer
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)