fixed isnull, istrue, isfalse lookups/operators#156
fixed isnull, istrue, isfalse lookups/operators#156eriktelepovsky wants to merge 4 commits intomodlinltd:developfrom
Conversation
asfaltboy
left a comment
asfaltboy
left a comment
There was a problem hiding this comment.
Thanks for submitting a PR! I left a few questions below
Any chance you could add some test case/s to assert this change will fix the issue?
advanced_filters/forms.py
Outdated
| boolean_lookups = ['isnull', 'istrue', 'isfalse'] | ||
| for boolean_lookup in boolean_lookups: | ||
| if query_data['field'].endswith(f'__{boolean_lookup}'): | ||
| query_data['operator'] = boolean_lookup |
There was a problem hiding this comment.
This seems identical to looking up the operator in AdvancedFilterQueryForm.OPERATORS (as on line 124 below). Could you explain what this does differently, and what scenario this will address?
There was a problem hiding this comment.
Correct, should be the same. I don't remember the purpose of these lines of mine but I tested it again and it works exactly as your code so we can delete this part. Thanks for double check.
| return {formdata['field']: False} | ||
|
|
||
| if formdata['operator'] in ["isnull", "istrue", "isfalse"]: | ||
| return {key: True if str(formdata['value']).lower() in ['1', 'true'] else False} |
There was a problem hiding this comment.
Is this functionally identical to before, or did something change here? Or is the change that isnull operator should return {use key: True}?
From a readability perspective, I think I prefer the previous code
There was a problem hiding this comment.
Yes, this is the most important part. Otherwise it returns 'True' value (string) instead of True (boolean), which is incorrect and produces false SQL statement. That's the reason why I iterate only boolean operators ("isnull", "istrue", "isfalse") and not all of the AdvancedFilterQueryForm.OPERATORS.
Except of that. It allows to combine values like following:
isnull: True
isnull: False
istrue: True
istrue: False
isfalse: True
isfalse: False
which can be useful as well.
|
@asfaltboy while I understand that tests are important: |
3 participants
Fixed building query for
isnull,istrueandisfalselookups as well as initialising form.