Skip to content
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

Disabled edit button in task instances list view page #20659

Merged
merged 31 commits into from
Jan 5, 2022

Conversation

subkanthi
Copy link
Contributor

@subkanthi subkanthi commented Jan 4, 2022

closes: #20655
Screen Shot 2022-01-04 at 5 59 45 PM

Disabled edit button in List Task Instance page, tested all the action buttons.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

subkanthi and others added 28 commits November 22, 2021 13:36
@subkanthi
Copy link
Contributor Author

Will this change allow clearing the TIs now from the List TIs page?

Can you confirm, please?

Yes it does.
Screen Shot 2022-01-05 at 6 52 05 AM

@uranusjr uranusjr merged commit 1cff451 into apache:main Jan 5, 2022
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Apr 11, 2022
@dmishh
Copy link

dmishh commented Aug 15, 2022

Code changes in this PR removed all bulk actions for TIs except for Delete. This is a huge mistake https://github.com/apache/airflow/pull/20659/files#diff-917b70d2661fae8322c538a4f60b9d4ba755556036fe0c527eb0411a6ac60f2eL4370

Airflow 2.2.3 - TI's bulk actions in place (https://github.com/apache/airflow/blob/2.2.3/airflow/www/views.py#L4227)
Airflow 2.3.3 - TI's bulk actions are missing (https://github.com/apache/airflow/blob/2.3.3/airflow/www/views.py#L5011)

@dmishh
Copy link

dmishh commented Aug 15, 2022

More than that, the Edit action is available in 2.3.3. So this PR just removed a very useful feature of TI bulk clearing.
cc @kaxil @uranusjr @bbovenzi

@ashb
Copy link
Member

ashb commented Aug 15, 2022

@dmishh You could ask for this back in a much much nicer way. Please be considerate of the volunteer time of OSS authors

https://xkcd.com/1172/

@dmishh
Copy link

dmishh commented Aug 15, 2022

@ashb I highly appreciate everyone's time and effort, but what's wrong with calling mistake a mistake? The "huge" correlates with the fact that @kaxil directly asked (which is a fantastic call) whether this will break the clearing functionality and the answer was "no, it won't".

@potiuk
Copy link
Member

potiuk commented Aug 15, 2022

@ashb I highly appreciate everyone's time and effort, but what's wrong with calling mistake a mistake? The "huge" correlates with the fact that @kaxil directly asked (which is a fantastic call) whether this will break the clearing functionality and the answer was "no, it won't".

@dmishh - let me explain what I think @ashb had in mind (or at least how I feel it as it is close to what I feel when I read your comment). The way you expressed it, looked like the whole change was a huge mistake. I believe (also knowing Ash) he did not complain on naming things a mistake but about your way of communicating and asking for it.

For example If you had started your first comment with "I highly appreciate everyone's time and effort and I see how much this PR improved, but it also mistakenly removed this and that" - that would be much nicer way to say what you possibly wanted to say (looking at your secondd comment you do appreiciate the work but it was not seen from the first comment).

It's very difficult to express the feelings in a written form when you write a comment in PR and I think learning it is an art.

@dmishh
Copy link

dmishh commented Aug 15, 2022

@potiuk @ashb Folks, I really didn't want to hurt anyone's feelings. Contributing to open source is a pure act of love - giving away your energy for the sake of creating some good stuff. I know what it takes, and I genuinely appreciate everyone's effort in creating free software.

My directness might sound a bit harsh, especially reading the comment without the details below, but I did it with respect and appreciation. I was as precise as possible when describing an issue - in my opinion, all the changes in this PR should be amended: the PR removed an important feature and the main piece of code that was supposed to do what was required is reverted now (in 2.3.3 at least). Also, the way the initial problem was fixed is something like "the wheel doesn't spin - let's remove it instead of lubricating".

@potiuk
Copy link
Member

potiuk commented Aug 26, 2022

It's easy to criticise. Much more difficult to express it in the way that shows respect while doing it. The last comment shows respect. The first did not. That's as simple as that. Try better next time. Contributing is a live-long learning (for me for sure) so you have a chance to show respect next time when you criticise something. No hard feelings.

eejbyfeldt pushed a commit to eejbyfeldt/airflow that referenced this pull request Apr 12, 2023
The action was disable in apache#20659
which resolved apache#20655. The issue
only mentions that the edit is broken and should be disabled. So it seem
like the disabling of the clear action was unintentional.

Also based on the discussion in the PR
apache#20655 further reinforces this.
That the author believed it still worked could be explain by that using
a user with role `Admin` the action was still available and therefore
one could easily make a mistake believing it still worked as expected.

This PR reenables it action and modifies and existing test case to also
verify that clearing is possible using a user with the role `User`.
potiuk pushed a commit that referenced this pull request Apr 22, 2023
* Reenable clear on TaskInstanceModelView for role User

The action was disable in #20659
which resolved #20655. The issue
only mentions that the edit is broken and should be disabled. So it seem
like the disabling of the clear action was unintentional.

Also based on the discussion in the PR
#20655 further reinforces this.
That the author believed it still worked could be explain by that using
a user with role `Admin` the action was still available and therefore
one could easily make a mistake believing it still worked as expected.

This PR reenables it action and modifies and existing test case to also
verify that clearing is possible using a user with the role `User`.

* Add back other set state actions

* fix static checks

---------

Co-authored-by: eladkal <[email protected]>
ephraimbuddy pushed a commit that referenced this pull request May 8, 2023
* Reenable clear on TaskInstanceModelView for role User

The action was disable in #20659
which resolved #20655. The issue
only mentions that the edit is broken and should be disabled. So it seem
like the disabling of the clear action was unintentional.

Also based on the discussion in the PR
#20655 further reinforces this.
That the author believed it still worked could be explain by that using
a user with role `Admin` the action was still available and therefore
one could easily make a mistake believing it still worked as expected.

This PR reenables it action and modifies and existing test case to also
verify that clearing is possible using a user with the role `User`.

* Add back other set state actions

* fix static checks

---------

Co-authored-by: eladkal <[email protected]>
(cherry picked from commit b140c44)
ahidalgob pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Jun 12, 2023
* Reenable clear on TaskInstanceModelView for role User

The action was disable in apache/airflow#20659
which resolved apache/airflow#20655. The issue
only mentions that the edit is broken and should be disabled. So it seem
like the disabling of the clear action was unintentional.

Also based on the discussion in the PR
apache/airflow#20655 further reinforces this.
That the author believed it still worked could be explain by that using
a user with role `Admin` the action was still available and therefore
one could easily make a mistake believing it still worked as expected.

This PR reenables it action and modifies and existing test case to also
verify that clearing is possible using a user with the role `User`.

* Add back other set state actions

* fix static checks

---------

Co-authored-by: eladkal <[email protected]>
GitOrigin-RevId: b140c4473335e4e157ff2db85148dd120c0ed893
ahidalgob pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 7, 2023
* Reenable clear on TaskInstanceModelView for role User

The action was disable in apache/airflow#20659
which resolved apache/airflow#20655. The issue
only mentions that the edit is broken and should be disabled. So it seem
like the disabling of the clear action was unintentional.

Also based on the discussion in the PR
apache/airflow#20655 further reinforces this.
That the author believed it still worked could be explain by that using
a user with role `Admin` the action was still available and therefore
one could easily make a mistake believing it still worked as expected.

This PR reenables it action and modifies and existing test case to also
verify that clearing is possible using a user with the role `User`.

* Add back other set state actions

* fix static checks

---------

Co-authored-by: eladkal <[email protected]>
(cherry picked from commit b140c4473335e4e157ff2db85148dd120c0ed893)
GitOrigin-RevId: 056aaffb759d94ccdf2ce49422b892d6836c28ef
ahidalgob pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request May 15, 2024
* Reenable clear on TaskInstanceModelView for role User

The action was disable in apache/airflow#20659
which resolved apache/airflow#20655. The issue
only mentions that the edit is broken and should be disabled. So it seem
like the disabling of the clear action was unintentional.

Also based on the discussion in the PR
apache/airflow#20655 further reinforces this.
That the author believed it still worked could be explain by that using
a user with role `Admin` the action was still available and therefore
one could easily make a mistake believing it still worked as expected.

This PR reenables it action and modifies and existing test case to also
verify that clearing is possible using a user with the role `User`.

* Add back other set state actions

* fix static checks

---------

Co-authored-by: eladkal <[email protected]>
GitOrigin-RevId: b140c4473335e4e157ff2db85148dd120c0ed893
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 19, 2024
* Reenable clear on TaskInstanceModelView for role User

The action was disable in apache/airflow#20659
which resolved apache/airflow#20655. The issue
only mentions that the edit is broken and should be disabled. So it seem
like the disabling of the clear action was unintentional.

Also based on the discussion in the PR
apache/airflow#20655 further reinforces this.
That the author believed it still worked could be explain by that using
a user with role `Admin` the action was still available and therefore
one could easily make a mistake believing it still worked as expected.

This PR reenables it action and modifies and existing test case to also
verify that clearing is possible using a user with the role `User`.

* Add back other set state actions

* fix static checks

---------

Co-authored-by: eladkal <[email protected]>
GitOrigin-RevId: b140c4473335e4e157ff2db85148dd120c0ed893
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Nov 8, 2024
* Reenable clear on TaskInstanceModelView for role User

The action was disable in apache/airflow#20659
which resolved apache/airflow#20655. The issue
only mentions that the edit is broken and should be disabled. So it seem
like the disabling of the clear action was unintentional.

Also based on the discussion in the PR
apache/airflow#20655 further reinforces this.
That the author believed it still worked could be explain by that using
a user with role `Admin` the action was still available and therefore
one could easily make a mistake believing it still worked as expected.

This PR reenables it action and modifies and existing test case to also
verify that clearing is possible using a user with the role `User`.

* Add back other set state actions

* fix static checks

---------

Co-authored-by: eladkal <[email protected]>
GitOrigin-RevId: b140c4473335e4e157ff2db85148dd120c0ed893
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edit Task Instance Page does not save updates.
8 participants