-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add empty body as default new input on Job inspector #2982
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2982 +/- ##
==========================================
- Coverage 91.61% 91.60% -0.01%
==========================================
Files 346 346
Lines 12744 12745 +1
==========================================
Hits 11675 11675
- Misses 1069 1070 +1 ☔ View full report in Codecov by Sentry. |
46975f9
to
a45073d
Compare
a45073d
to
882d1f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jyeshe , great job!
I'm a little concerned about the multiple if
statements introduced. I think we should only have this in one place, in the changeset: https://github.com/OpenFn/lightning/blob/main/lib/lightning/workorders/manual.ex#L35
...
|> validate_required([:project, :job, :created_by, :workflow])
|> then(fn changeset ->
if get_change(changeset, :body) |> is_nil() do
put_change(changeset, :body, "{}")
else
changeset
end
end)
|> remove_body_if_dataclip_present()
...
Also, is there a reason why we can't always just build the manual workorder with the created_by: current_user
. The user who is logged in is the only person who can initiate this right? I don't understand why we conditionally pass this in.
|> then(fn attrs -> | ||
attrs = | ||
if body == "{}" do | ||
Map.put(attrs, :created_by, current_user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding the created_by
only if body == {}
?
I can see in the above block we are already passing in a user: current_user
, is it any different from created_by
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the user accepts the suggestion of the empty body, the user is the creator of the dataclip. This is not needed when the user types the input but without typing it is.
I would refactor the existing validation that checks about this created_by
but once this is a huge module that handle many cases I am leaving it do it after some prioritization.
Hi @midigofrank I appreciate your concern once this is a module that handles tens of possibilities.
It is passed because the user is not explicitly typing an input but accepting the suggestion when creating a work order with it. |
@jyeshe I get that the user is not explicitly setting the input, but does it really matter now that we have a default input anyway. |
It's required field. Here is the validation I was referring to: But I agree with you that we should find a way to make it more maintainable. Not only in regards to the creation but a full refactor breaking the LV into components and helpers. |
Aaah, I see. Can't we just change that line to |
That's what I wanted but do you remember the request (: ? But to be honest, the more I tested the more I saw have many scenarios the editor can handle so I think it's a responsible take to just satisfy the required condition instead of ignore it. |
af6e308
to
9906c96
Compare
@stuartc as you're resuming this, please notice my last commit shows that with an empty body on the schema it is not shown on the input and the create button is disabled: If you feel that you are also spending extra time on this I would suggest us to go with the commit 882d1f0 that handles the empty body in explicit way satisfying the requirement. I would only complement it by adding the created_by to always be set. |
75aad56
to
a6debde
Compare
Description
This PR adds an empty body
{}
for the creation of new input on the job inspector.Closes #2952
Validation steps
{}
is suggested as basic dataclip for start and to create a new work order.Additional notes for the reviewer
Notice that if you copy a link of a run to follow it and this run has a wiped input dataclip the empty dataclip is NOT suggested to honor the fact that this run does NOT have any dataclip.
AI Usage
Please disclose how you've used AI in this work (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
:owner
,:admin
,:editor
,:viewer
)