You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/05/03 01:41:24 UTC

[GitHub] [airflow] Acehaidrey opened a new pull request #8681: Prevent clickable sorting on non sortable columns in TI view

Acehaidrey opened a new pull request #8681:
URL: https://github.com/apache/airflow/pull/8681


   The purpose of this PR is to address the error view we get once a user tries to click on columns that are properties and not sortable in the view. Currently the task instance view does not show all runs anyways and rather shows just the number of times it ran this task for a given execution date. We need to remove these from the order_columns to prevent them from being clickable and rather have them be static.
   
   Because this is a cosmetic change we have attached the UI shots from our instances within Pinterest and do not have any unit tests for this change.
   
   ---
   Make sure to mark the boxes below before creating PR: [x]
   
   - [x ] Description above provides context of the change
   - [ x] Unit tests coverage for changes (not needed for documentation changes)
   - [x ] Target Github ISSUE in description if exists
   - [x ] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
   - [x ] Relevant documentation is updated including usage instructions.
   - [x ] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
   
   ---
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
   Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Acehaidrey commented on a change in pull request #8681: Prevent clickable sorting on non sortable columns in TI view

Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on a change in pull request #8681:
URL: https://github.com/apache/airflow/pull/8681#discussion_r419658098



##########
File path: airflow/www/views.py
##########
@@ -2627,6 +2627,9 @@ class TaskInstanceModelView(AirflowModelView):
                     'unixname', 'priority_weight', 'queue', 'queued_dttm', 'try_number',
                     'pool', 'log_url']
 
+    # this is to remove the sorting option on these columns that raise `property has no attribute asc`
+    order_columns = [item for item in list_columns.copy() if item not in ['try_number', 'log_url']]

Review comment:
       I agree with you. Have removed the comment. And also removing the .copy(). Thanks for your help on this




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jhtimmins commented on a change in pull request #8681: Prevent clickable sorting on non sortable columns in TI view

Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #8681:
URL: https://github.com/apache/airflow/pull/8681#discussion_r419530061



##########
File path: airflow/www/views.py
##########
@@ -2627,6 +2627,9 @@ class TaskInstanceModelView(AirflowModelView):
                     'unixname', 'priority_weight', 'queue', 'queued_dttm', 'try_number',
                     'pool', 'log_url']
 
+    # this is to remove the sorting option on these columns that raise `property has no attribute asc`
+    order_columns = [item for item in list_columns.copy() if item not in ['try_number', 'log_url']]

Review comment:
       I think this can do w/out the comment, since `order_columns` has a specific meaning/purpose in Flask AppBuilder.
   
   Additionally, `.copy()` is unnecessary, as this list comprehension is already creating a new list.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #8681: Prevent clickable sorting on non sortable columns in TI view

Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #8681:
URL: https://github.com/apache/airflow/pull/8681#issuecomment-623730693


   Awesome work, congrats on your first merged pull request!
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] jhtimmins commented on a change in pull request #8681: Prevent clickable sorting on non sortable columns in TI view

Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #8681:
URL: https://github.com/apache/airflow/pull/8681#discussion_r419530061



##########
File path: airflow/www/views.py
##########
@@ -2627,6 +2627,9 @@ class TaskInstanceModelView(AirflowModelView):
                     'unixname', 'priority_weight', 'queue', 'queued_dttm', 'try_number',
                     'pool', 'log_url']
 
+    # this is to remove the sorting option on these columns that raise `property has no attribute asc`
+    order_columns = [item for item in list_columns.copy() if item not in ['try_number', 'log_url']]

Review comment:
       I think this can do w/out the comment, since `order_columns` has a specific meaning/purpose in Flask AppBuilder. Otherwise this looks good to me.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] Acehaidrey commented on pull request #8681: Prevent clickable sorting on non sortable columns in TI view

Posted by GitBox <gi...@apache.org>.
Acehaidrey commented on pull request #8681:
URL: https://github.com/apache/airflow/pull/8681#issuecomment-623731909


   Thank you team!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org