-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Checkbox before clear task confirmation to prevent rerun of tasks in 'Running' states. #56351
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
Checkbox before clear task confirmation to prevent rerun of tasks in 'Running' states. #56351
Conversation
…nning, up_for_retry, or restarting state.
…n confirmation dialogue.
…and ui still not communicating. MIGHT revert to previous commit.
…that will not run the instance until the instance is not running.
…o router. Followed suggestion about attribute access.
…nningTasks in dags.json in the en locale.
…ent_running_tasks' addition to ti, instead, it is passed as a parameter in clearTaskInstance.
…Error code 400 is still being raised in the router.
…f trying to clear a running task.
…escript-eslint/no-unsafe-assignment
jason810496
left a comment
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.
The unit test of airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_instances.py still need fix. Thanks!
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_instances.py
Outdated
Show resolved
Hide resolved
…est to count prevent_running_task.
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_instances.py
Outdated
Show resolved
Hide resolved
jason810496
left a comment
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.
Nice! LGMT on backend side if CI run successfully.
bbovenzi
left a comment
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.
Can you please upload new gifs and screenshots with the clear and then confirm dialog?
|
Hello! This comment is a friendly ping for a review whenever someone is free. Sorry for the sudden notification. |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
…'Running' states. (apache#56351) * Added a confirm dialog modal that appears only when the task is in running, up_for_retry, or restarting state. * Removed unnecessary comments * Added clearTask texts in dags.json in the EN locale. Used translate in confirmation dialogue. * (NON-WORKING/ Experimental) added a flag in taskinstance.py. backend and ui still not communicating. MIGHT revert to previous commit. * (EXPERIMENTAL) Removed the confirm pop-up and replaced with checkbox that will not run the instance until the instance is not running. * Added mirgin-right auto for the checkbox * Change isRunning to task_confirmed_running. Moved the HTTPException to router. Followed suggestion about attribute access. * Made the checkbox checked on default. is_running_message set to true by default. * Added the code 400 to line 688. * Cleaned up the code and attempted to adress the error in the tests, e712. * Made sure the date variable is not null and changed the string in the value error * Changed the hardcoded english into an i18n translate. Added preventRunningTasks in dags.json in the en locale. * Renamed the is_running_message to prevent_running_tasks. Removed prevent_running_tasks' addition to ti, instead, it is passed as a parameter in clearTaskInstance. * Added a custom airflow exception for clearing running task instance. Error code 400 is still being raised in the router. * Added retry 0 in use clearTaskInstances. * Added a conditional for error toast that states the task is running if trying to clear a running task. * added check for null in error.detail * removed unnecessary imports in task_instances.py * made the prevent_running_task optional to pass checks * Changed prevent_running_task to Optional[bool]=None to be optional in test check * Attempt to resolve static checks which are unsafe assignment and calls in useClearTaskInstances, dateTimeUtils, and dateTimeUtils.test. * Added flag for QUEUED and SCHEDULED in taskinstances.py * Added a modal for QUEUED and SCHEDULED states. * Added the text of the modal in the en locale. * Changed the contents of the modal to include the user that started the task and the time when it was started. * Changed taskinstance.py according to the comments, and changed the modal behavior to only trigger the toast when task is running, not in queued or scheduled. * Attempt to resolve static checks * Attempt to resolve assertion errors of prevent_running_task = false in tests. * Attempt to fix the restarting state returned by clear_task_instances method. * 2nd Attempt to resolve static checks * Used common:error.defaultMessage for the default error toast. Added useState to avoid the modal rendering for a split second when its not supposed to. * Resolve the dateTimeUtils.getRelativeTime not returning an empty string when date is undefined. 3rd Attempt to solve static checks. * Attempt to resolve 7 eslint static checks error. Shortened conditional codes. * Resolving eslint errors in UseClearTaskInstance and ClearTaskInstanceConfirmationDialog. Implemented suggested changes. * Attempt to resolve detail in useClearTaskInstances not triggering typescript-eslint/no-unsafe-assignment * Attempt to resolve static checks. * Removed kwargs inspect, added back prevent_running_task, and edited test to count prevent_running_task. * Changed error code 400 to 409. Added the pre-commit modification. * Fixed missing detail in the error toast.
…'Running' states. (apache#56351) * Added a confirm dialog modal that appears only when the task is in running, up_for_retry, or restarting state. * Removed unnecessary comments * Added clearTask texts in dags.json in the EN locale. Used translate in confirmation dialogue. * (NON-WORKING/ Experimental) added a flag in taskinstance.py. backend and ui still not communicating. MIGHT revert to previous commit. * (EXPERIMENTAL) Removed the confirm pop-up and replaced with checkbox that will not run the instance until the instance is not running. * Added mirgin-right auto for the checkbox * Change isRunning to task_confirmed_running. Moved the HTTPException to router. Followed suggestion about attribute access. * Made the checkbox checked on default. is_running_message set to true by default. * Added the code 400 to line 688. * Cleaned up the code and attempted to adress the error in the tests, e712. * Made sure the date variable is not null and changed the string in the value error * Changed the hardcoded english into an i18n translate. Added preventRunningTasks in dags.json in the en locale. * Renamed the is_running_message to prevent_running_tasks. Removed prevent_running_tasks' addition to ti, instead, it is passed as a parameter in clearTaskInstance. * Added a custom airflow exception for clearing running task instance. Error code 400 is still being raised in the router. * Added retry 0 in use clearTaskInstances. * Added a conditional for error toast that states the task is running if trying to clear a running task. * added check for null in error.detail * removed unnecessary imports in task_instances.py * made the prevent_running_task optional to pass checks * Changed prevent_running_task to Optional[bool]=None to be optional in test check * Attempt to resolve static checks which are unsafe assignment and calls in useClearTaskInstances, dateTimeUtils, and dateTimeUtils.test. * Added flag for QUEUED and SCHEDULED in taskinstances.py * Added a modal for QUEUED and SCHEDULED states. * Added the text of the modal in the en locale. * Changed the contents of the modal to include the user that started the task and the time when it was started. * Changed taskinstance.py according to the comments, and changed the modal behavior to only trigger the toast when task is running, not in queued or scheduled. * Attempt to resolve static checks * Attempt to resolve assertion errors of prevent_running_task = false in tests. * Attempt to fix the restarting state returned by clear_task_instances method. * 2nd Attempt to resolve static checks * Used common:error.defaultMessage for the default error toast. Added useState to avoid the modal rendering for a split second when its not supposed to. * Resolve the dateTimeUtils.getRelativeTime not returning an empty string when date is undefined. 3rd Attempt to solve static checks. * Attempt to resolve 7 eslint static checks error. Shortened conditional codes. * Resolving eslint errors in UseClearTaskInstance and ClearTaskInstanceConfirmationDialog. Implemented suggested changes. * Attempt to resolve detail in useClearTaskInstances not triggering typescript-eslint/no-unsafe-assignment * Attempt to resolve static checks. * Removed kwargs inspect, added back prevent_running_task, and edited test to count prevent_running_task. * Changed error code 400 to 409. Added the pre-commit modification. * Fixed missing detail in the error toast.




Why
This pull request is a revised version of the pull request #55660 and so it is also based on issue #54379
Why another PR?
Due to the changes made in the previous pull request ( #55660 ), its design has deviated from the original PR. Therefore, it necessitated a change in PR following the new design and interaction with the user.
This PR's goal is still the same compared to the last one, however, its execution is now a bit different as the modal is removed and is replaced by a checkbox. The checkbox still serves the same purpose to prevent a running task instance from clearing, but without necessitating the user to read another prompt/ pop-up, and urging the user to make the decision before the dialog confirm instead of after.
What
Difference of this PR compared to PR #55660 :
This PR uses the same code for the backend flag from #55660 to enable the functionalities of the checkbox.
Here is a video example of how the checkbox works:
chrome_VkmgN56gJw.mp4
Here is what happens when the checkbox is unchecked and goes back to the default behavior:
chrome_wbJuSnkObl.mp4