-
Notifications
You must be signed in to change notification settings - Fork 15.5k
Allow preloaded hitl options and params through search params #54783
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
Conversation
Thanks for adding this feature. That totally makes sense. Just I am not 100% good with the choice of implementation. Similar we had in Airflow 2.x for the trifger form (and I just noticed that this feature is not implemented in 3.x... so need to clean docs...) - if you take alook to https://airflow.apache.org/docs/apache-airflow/stable/core-concepts/params.html:
Can you make it for the feature here the same, so that no |
Not sure whether it's ideal in this case as we have both |
I also have the concern about that case. Since we already preserve the IMO The preload behavior in this PR is scoped only to |
I do not understand your concerns here. (I mean the technical reason, maybe due to mis-understanding) - can you elaborate the reason a bit more? I would propose to make parameter input passing consistent to minimize mental load, encoding a params dict in a single value is a lot of mental load if somebody wants to prepare a URL, also quotes and special chars need to be encoded. besides this proper docs are needed describing this to users as interface, else somebody need to be expert in source tree in order to make a parameterized call. |
The main reason is that we're not just adding parameter input, but also the option we want to preload. If there's only parameter input, I'd say we should go with |
Mhm, I do not understand about the "options" - this is what the user needs to confirm anyway? We decided not making a 1-click solution so the "2 click solution" here with pre-populated fields would anyway require to select the user to click on "option1", "option2", "option3" manually. What would be pre-selectable for the user with passing the "option"? Is this not the core idea of the second click the user needs to make? Still if mis-understood the "options", we could make this a special field naming like "_options" and/or reserve the key such that if the form has the same it is just not possilbe to pre-populate. Even if this is a restriction it is much easier to be able to pass values to the form w/o the need to build and envode a JSON dict in a URL as param value. |
The demo video seems to miss the "options" part. But the idea is that whenever we receive ![]() The users still need to click the
I don't think I have enough frontend knowledge to judge whether decoding a json dict from a frontend perspective is complicated, so I'll leave this to @guan404ming . From the backend perspective, there will be a utility method once this is merged, so users typically don't need to deal with JSON encoding. If we decide to go with |
Ah, okay, so passing the "options" would be only beneficial if multiple options should be pre-selected, as single options would render as a button directly. My argument is mostly not about the decoding (this can be done as code, I am sure this is not a big challenge) but about the complexity somebody need to invest to create a URL with the right parameters to pre-select. Making a URL with |
From what I last discussed with @guan404ming , single options will work this way (like multiple options)if users are using this URL this way.
My issue is that |
I created this table to summarize the discussion. Personally, I find Jens's concern reasonable if we try to think from user’s perspective. The flat query style feels more comfortable and approachable. Especially since some users of this feature may not be technical. I think reserving
I initially implemented it this way. However, after manual testing, I found it was not consistent with our original one-click design in single option. Therefore, I switched to preloading with default options and I think it would be more consistent and user-friendly. Next, I will adjust the preload implementation and update the demo with screenshots covering:
|
d1f236b
to
ae80b63
Compare
Two update in url
.../required_actions?_options=option 1,option 2&key1=value1&key2=value2...
Screen.Recording.2025-08-24.at.3.55.55.PM.mov
Screen.Recording.2025-08-24.at.3.54.40.PM.mov
Screen.Recording.2025-08-24.at.3.52.22.PM.mov |
I feel we probably need a
I probably need to check whether |
@guan404ming / @Lee-W thanks for the discussion! My concerns are fixed by the rework and looks very good for me!
That is really a limitation... that would be a + for having JSON support... mhm. I'd think it would be OK to assume no One option as resolution could be to offer to support JSON as well as plain text style... but might be un-needed complexity if nobody is using/needing it. Would make it easy for users to consume and still would leave the option for the nice cases... I would defer this to a future if really somebody needs it. |
Thanks for jumping in on the discussion and helping us improve this feature! 🙌 I totally agree that restricting |
Here is the version with a respond button for single option preload case and I think I don't have strong opinion here since both of them look good to me. First one persist the consistent one-click ux and second one make the user flow more natural. What do you think? Screen.Recording.2025-08-24.at.5.31.30.PM.mov
Agree with this. |
I am not sure about the implementation complexity that you need to have the "single option" then in a dropdown and having a respond button... smells a bit more complex. Also UX would be a nit in-consistent. No strong feeling on this but I feel the previous way that still 3 buttons are presented but the pre-select is highlighted is a nit more consistent. Especially looking at a "Reject"/"Approve" scenario (Where in the current HITL Example because of the timeout the Reject is always proposed as default, if you post a link with a 2-click Approve option then approve would be highlighted. |
No strong opinion either. Then I'll let @guan404ming decide what's the best 🙂 |
The implementation itself wouldn’t be too complex, since our FlexibleForm can render fields based on the passed value type.
I would personally prefer to stick with the current approach (highlight the preload options) for now for more consistent UX but we can revisit and adjust later based on user feedback. Thanks for all discussion to make this feature better! 🚀 |
ae80b63
to
a7cff08
Compare
I think a simple We should probably have a "Submit" button on more forms. The number of clicks is less important than it being clear what you're about to do. I agree that "Approve" should probably get the default main button styling. |
a7cff08
to
a67e8d2
Compare
Related PR
Closes: #54506
How
Screen.Recording.2025-08-21.at.10.32.04.PM.mov
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in airflow-core/newsfragments.