EasyFix: Fix Error 500 when sorting users list by fullname.

Review Request #10155 — Created Sept. 21, 2018 and discarded

Information

Djblets
release-1.0.x

Reviewers

When appending ?sort=fullname to /users/, ReviewBoard attempts to query and
sort on 'fullname' which is not a correct field and causes an Error 500 to
occur. Although most manual sort queries will be discarded as improper,
fullname is allowed as a sort field because it corresponds to a Column name on
the page. This has been fixed by detecting the special case for 'fullname' in
grids.py and if 'fullname' is in the sort query, it is removed and replaced
with 'first_name' and 'last_name' which are its composite substrings.

Tested with Review Board 3.0.x by manually attempting ?sort=fullname and ?sort=-fullname on /users/, as well as sorting by fullname and username, and going back and forth between the fullname sort cases and username sort cases. I encountered no issues on my testing once the fix was applied.

Description From Last Updated

Can you update your description to explain what the bug was and how your change fixes it? See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea for …

daviddavid

David touched upon an important point, the location of where the change should be made. This change is too broad, …

chipx86chipx86

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E111 indentation is not a multiple of four

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

F821 undefined name 'F'

reviewbotreviewbot

F821 undefined name 'F'

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

Remove this blank line.

brenniebrennie

And this one.

brenniebrennie

This comment doesn't explain why we're oing this. Also comments should be full sentences (i.e., end with a period).

brenniebrennie

And this one.

brenniebrennie

I think this was accidentally added to your patch. Can you please remove this from the commit, and re-post?

mike_conleymike_conley

Please delete this file from your review request. You can do git rm package-lock.json and git commit --amend -C HEAD

brenniebrennie

W293 blank line contains whitespace

reviewbotreviewbot

I just noticed what file this is in. The DataGrid class should be entirely generic--it's easily possible that someone could …

daviddavid

There's no need for the u prefix. We use Unicode strings by default.

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

flake8

gojeffcho
mike_conley
  1. 
      
  2. package-lock.json (Diff revision 2)
     
     

    I think this was accidentally added to your patch. Can you please remove this from the commit, and re-post?

  3. 
      
gojeffcho
brennie
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 2)
     
     

    Remove this blank line.

  3. djblets/datagrid/grids.py (Diff revision 2)
     
     

    And this one.

  4. djblets/datagrid/grids.py (Diff revision 2)
     
     

    This comment doesn't explain why we're oing this. Also comments should be full sentences (i.e., end with a period).

  5. djblets/datagrid/grids.py (Diff revision 2)
     
     

    And this one.

  6. package-lock.json (Diff revision 2)
     
     

    Please delete this file from your review request. You can do git rm package-lock.json and git commit --amend -C HEAD

    1. Sorry this review was from yesterday -- I just forgot to publish before we left.

  7. 
      
gojeffcho
mike_conley
  1. Hey Jeff, I re-opened some of Barret's issues because those newlines still look like they're in the latest revision.

  2. 
      
gojeffcho
Review request changed

Change Summary:

Remove unnecessary linebreaks.

Commit:

-4f7533dac2f6b25bf1ccab45ab2f8c27971c84af
+ac151aeea0be8d0f240b09a22bf01ac975589a30

Diff:

Revision 5 (+10)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

gojeffcho
david
  1. 
      
  2. Can you update your description to explain what the bug was and how your change fixes it? See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea for our guide on this.

    You should also list the bug number in the "Bugs" field, and not in the description.

  3. 
      
gojeffcho
gojeffcho
david
  1. 
      
  2. djblets/datagrid/grids.py (Diff revision 6)
     
     
     
     
     
     
     
     
     

    I just noticed what file this is in. The DataGrid class should be entirely generic--it's easily possible that someone could use this for a model that does include a column named fullname.

    Can we instead make the change in Review Board's reviewboard.datagrids.grids.UsersDataGrid? We can add a precomute_objects override that first makes necessary changes to self.sort_list and then calls the superclass' version.

  3. 
      
chipx86
  1. 
      
  2. David touched upon an important point, the location of where the change should be made. This change is too broad, as DataGrids are used for many things.

    Another thing that's important is the design of the change. This change is somewhat of a "bandaid." It's addressing a symptom of the problem, but the real problem is that we were trying to allow sorting on the fullname field (which does not have sortable=True, so it shouldn't even be possible), when it's not really a field. It's working around an improper setting. Albeit in an interesting way -- turning it into two sortable fields.

    So let's boil down the real issues:

    1. We're allowing values in ?sort= that are not valid sortable columns. This needs to be fixed (and have a unit test proving it).
    2. We want to allow sorting on the Full Name header, but we want something more specialized with the sorting (sorting by first name and then last name).

    Issue #1 -- Bad Sort Values

    The fix would be to add some validation where we're processing self.request.GET.get('sort', ...) in grids.py. It needs to be updated to make sure that each value in there is something we can sort on. Each must be something that a column could return. So an approach would be to first loop through all columns being shown on the datagrid that have sortable=True and then fetch the sortable database field from each, put those all in a set(), and then ignore any value in ?sort= that aren't in that set. This fixes not only ?sort=fullname, but ?sort=whatever.

    Issue #2 -- Smarter Sorting of Full Names

    Let's make a proper design for this, rather than working around a limitation in the current design. Columns can specify the database field they'd like to use for sorting through Column.get_sort_field(), which defaults to returning self.db_field. If you search, you'll find where we call get_sort_field(), and how it places the result in the list of fields to sort with. How about we update this to allow returning a list/tuple? Then we can have:

    class FullNameColumn(...):
        ...
    
        def get_sort_field(self):
            return ('first_name', 'last_name')
    

    The code that create the sort list can check if it got multiple items from this call, and add each independently to the sort list.

    This would require a change in Djblets to provide this capability (plus a matching unit test), and a second change in Review Board to define this method for the column.

    Summary

    That's three review requests: 1 for the sorting fix (fix + unit test), 1 for the Djblets multi-field sorting (implementation + unit test), 1 for defining FullNameColumn.get_sort_field in Review Board.

    Sound good? I can go over this with you on Slack if it helps.

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

    There's no need for the u prefix. We use Unicode strings by default.

  4. 
      
gojeffcho
Review request changed

Status: Discarded

Change Summary:

This has spun off into two changes, the first of which is 10168.

Loading...