Skip to content

Parameterize whether input is required for AirflowDateTimePickerWidget#31141

Closed
xuganyu96 wants to merge 1 commit intoapache:mainfrom
xuganyu96:parameterize-airflow-datetime-picker-widget
Closed

Parameterize whether input is required for AirflowDateTimePickerWidget#31141
xuganyu96 wants to merge 1 commit intoapache:mainfrom
xuganyu96:parameterize-airflow-datetime-picker-widget

Conversation

@xuganyu96
Copy link
Contributor

@xuganyu96 xuganyu96 commented May 9, 2023

I'd like to use Airflow's own datetime picker widget, especially when writing web view plugins with FAB. With the existing implementation, the input is always required, but sometimes I have use cases where a datetime value is optional. This pull request made a simple change that parameterize whether the input tag will have the required attribute when instantiating an instance of this widget.

A default True is assigned so that the original behavior is preserved.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label May 9, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented May 9, 2023

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

Copy link
Contributor

@utkarsharma2 utkarsharma2 May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xuganyu96 Thanks for the contribution! :)

I like the idea of making datetime picker optional, but I foresee an issue, I have not gone through the code(@bbovenzi would be able to confirm :) ), but there can be multiple ways you can enforce an input field to be required, adding the required attribute is one of them. So I think adding just this code won't guarantee the behavior for all occurrences. It would be much nicer if we can see in a single PR the change and its usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback.

there can be multiple ways you can enforce an input field to be required

Could you please clarify what other ways you are referring to? The only other place I can think of is with wtforms where the forms are validated, but I think these are separate layers of validation that should remain independent.

class PluginWebviewForm(FlaskForm):
    required_at_frontend = DateTimeWithTimezoneField(
        "Datetime required at frontend",
        widget=AirflowDateTimePickerWidget(),
    )

    required_at_backend = DateTimeWithTimezoneField(
        "Datetime required at form validation",
        widget=AirflowDateTimePickerWidget(),
        validators=[InputRequired()],
    )

    optional_datetime = DateTimeWithTimezoneField(
        "Datetime optional",
        widget=AirflowDateTimePickerWidget(input_required=False),
        validators=[Optional()],
    )

What prompted me to make this change is the third field optional_datetime in the form where I want to collect an optional datetime.

I did a quick search and found that AirflowDateTimePicker originally did not contain the "required" attribute at the input tag. Issue #15976 and pull request #18602 changed the widget so that the required attribute is always present, but I think that's too inflexible, and my change aims to make it more flexible.

if we can see in a single PR the change and its usage

Is this comment sufficient, or should I produce documentation, as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can enforce the required clause by just JS as well but again I'm not entirely sure if there are any scenarios in the airflow codebase where that is the case.

IMO, if you plan to use the optional datetime picker anywhere in the code it would give more context if you make those changes in the same PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback.

I am not entirely familiar with using JS to control attributes of an HTML tag. My intended use case is "outside" the Airflow codebase, such as with developing plugins. For relatively simple plugins it would be great to use widgets with Flask/Jinja2 and not have to work with JS, which is why I want to parameterize the required attribute with Python instead of anywhere else.

@xuganyu96 xuganyu96 requested a review from utkarsharma2 May 9, 2023 16:59
@xuganyu96 xuganyu96 force-pushed the parameterize-airflow-datetime-picker-widget branch 2 times, most recently from e257490 to 274778c Compare May 15, 2023 18:15
@xuganyu96 xuganyu96 force-pushed the parameterize-airflow-datetime-picker-widget branch from 274778c to 79c9b72 Compare May 23, 2023 03:36
@xuganyu96 xuganyu96 requested a review from pierrejeambrun as a code owner May 23, 2023 03:36
@pierrejeambrun
Copy link
Member

Can you rebase to fix the static check issue please ?

@xuganyu96 xuganyu96 force-pushed the parameterize-airflow-datetime-picker-widget branch from 79c9b72 to 5ee765d Compare May 25, 2023 19:53
@xuganyu96 xuganyu96 force-pushed the parameterize-airflow-datetime-picker-widget branch from 5ee765d to 39e7ead Compare June 10, 2023 23:05
@xuganyu96 xuganyu96 closed this Jul 16, 2023
@xuganyu96 xuganyu96 deleted the parameterize-airflow-datetime-picker-widget branch July 16, 2023 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants