Added filtering of sorting columns that are not actual database fields (e.x. fullname)

Review Request #9497 — Created Jan. 19, 2018 and discarded

Information

Djblets
master

Reviewers

This fixes a crash throwing a FieldError in DataGrid. The crash is caused by specifying a sort column in the url that is a method (rather than a field; For example "fullname" is a method on User (as get_full_name()) and no table column for this field actually exists). The method is resolved from the field name, and added to the queryset's orderby call, which eventually returns with a FieldError (since no column called get_full_name exists).

DataGrid now ignores these columns and logs a warning to the console (just like it does when sorting by non-existing columns).

Did manual testing on a mock database with various sort columns.

Removed a unit test that no longer was relevant (since it tested the behaviour of get_sort_column on these invalid columns, and it no longer gets called).
Added a (passing) unit test that makes sure no exception is thrown and a warning is printed about trying to sort the grid by a column that isn't in the db table.

Description From Last Updated

I realized there's a flaw with the approach here. It's assuming we're only able to sort by fields on the …

chipx86chipx86

The bug number should go in the Bugs field, instead of having a link in the description.

chipx86chipx86

The text for the summary, description, and testing done need to be more in line with our standards described at …

chipx86chipx86

Make sure you close any open issues as you fix them in your code, so that you don't leave any …

chipx86chipx86

E501 line too long (140 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (107 > 79 characters)

reviewbotreviewbot

E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

W191 indentation contains tabs

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W191 indentation contains tabs

reviewbotreviewbot

W191 indentation contains tabs

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

I know the old code was like this, but there should be a blank line before the start of a …

chipx86chipx86

No blank line here.

chipx86chipx86

Much more can fit on each line.

chipx86chipx86

This doesn't match the docstring format for our tests. It should be more like the old docstring. They're in this …

chipx86chipx86

Ideally, we should always use single quotes. We can also import the logger we care about directly from the module, …

chipx86chipx86

Instead of calls[-1], use last_call.

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

rpolyano
Review request changed
Change Summary:

Fixed flake8 errors

Commit:
d074501728857ad187cbbc25c21a5d491cfbe007
4f4b644b8a7ac63bae53351847306d05fd64d103

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

rpolyano
chipx86
  1. 
      
  2. Show all issues

    I realized there's a flaw with the approach here. It's assuming we're only able to sort by fields on the main model being worked with, but that's not the case. We can sort by related fields as well. For instance, if we were working with review requests:

    ?sort=submitter.username
    

    In this case, we'd be sorting based on the username of the submitter, which clearly is not a field directly on the model. That case is important, and must be allowed. (Unit tests should probably be added to make sure it continues to work correctly.)

    1. Is this allowed currently? It does not appear to be implemented as self.get_column('owner.username') returns None (and is thus ignored) in /r/?sort=owner.username

  3. Show all issues

    The bug number should go in the Bugs field, instead of having a link in the description.

  4. Show all issues

    The text for the summary, description, and testing done need to be more in line with our standards described at https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/.

  5. Show all issues

    Make sure you close any open issues as you fix them in your code, so that you don't leave any open or accidentally close any that turned out not to be fixed. Very important while going through code review.

  6. djblets/datagrid/grids.py (Diff revision 3)
     
     
     
    Show all issues

    I know the old code was like this, but there should be a blank line before the start of a new block (like for).

  7. djblets/datagrid/grids.py (Diff revision 3)
     
     
     
     
    Show all issues

    No blank line here.

  8. djblets/datagrid/grids.py (Diff revision 3)
     
     
     
     
    Show all issues

    Much more can fit on each line.

  9. djblets/datagrid/tests.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    This doesn't match the docstring format for our tests. It should be more like the old docstring. They're in this format:

    """One-line-summary."""
    

    Or:

    """One-line summary broken across multiple
    lines.
    """
    

    Note that this format is only for unit test docstrings. Normal docstrings follow a differnet format, documented in Notion.

  10. djblets/datagrid/tests.py (Diff revision 3)
     
     
    Show all issues

    Ideally, we should always use single quotes.

    We can also import the logger we care about directly from the module, instaed of calling logging.getLogger() here.

  11. djblets/datagrid/tests.py (Diff revision 3)
     
     
    Show all issues

    Instead of calls[-1], use last_call.

  12. 
      
rpolyano
rpolyano
rpolyano
david
Review request changed
Status:
Discarded