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.

Sign up for GitHub

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

fixed isnull, istrue, isfalse lookups/operators #156

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

fixed isnull, istrue, isfalse lookups/operators #156

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0dc9d6
fixed isnull, istrue, isfalse lookups/operators
eriktelepovsky Dec 20, 2021
4f425e3
removed select2("destroy") from inputs
eriktelepovsky Dec 20, 2021
ab4f9df
removed duplicated code
eriktelepovsky Jan 24, 2022
7f74f64
Merge branch 'modlinltd:develop' into develop
eriktelepovsky Jan 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 13 additions & 17 deletions advanced_filters/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,10 @@ def _build_query_dict(self, formdata=None):
if self.is_valid() and formdata is None:
formdata = self.cleaned_data
key = "{field}__{operator}".format(**formdata)
if formdata['operator'] == "isnull":
return {key: None}
elif formdata['operator'] == "istrue":
return {formdata['field']: True}
elif formdata['operator'] == "isfalse":
return {formdata['field']: False}

if formdata['operator'] in ["isnull", "istrue", "isfalse"]:
return {key: True if str(formdata['value']).lower() in ['1', 'true'] else False}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return {key: formdata['value']}

@staticmethod
Expand All @@ -115,6 +113,11 @@ def _parse_query_dict(query_data, model):
return query_data

parts = query_data['field'].split('__')
boolean_lookups = ['isnull', 'istrue', 'isfalse']
for boolean_lookup in boolean_lookups:
if query_data['field'].endswith(f'__{boolean_lookup}'):
query_data['operator'] = boolean_lookup
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if len(parts) < 2:
field = parts[0]
else:
Expand All @@ -132,18 +135,11 @@ def _parse_query_dict(query_data, model):
else:
mfield = mfield[-1] # get the field object

if query_data['value'] is None:
query_data['operator'] = "isnull"
elif query_data['value'] is True:
query_data['operator'] = "istrue"
elif query_data['value'] is False:
query_data['operator'] = "isfalse"
if isinstance(mfield, DateField):
# this is a date/datetime field
query_data['operator'] = "range" # default
else:
if isinstance(mfield, DateField):
# this is a date/datetime field
query_data['operator'] = "range" # default
else:
query_data['operator'] = operator # default
query_data['operator'] = operator # default

if isinstance(query_data.get('value'),
list) and query_data['operator'] == 'range':
Expand Down
2 changes: 1 addition & 1 deletion advanced_filters/static/advanced-filters/advanced-filters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ var OperatorHandlers = function($) {
$(this).off("change");
});
$('.form-row input.query-value').each(function() {
$(this).select2("destroy");
// $(this).select2("destroy");
});
};
};
Expand Down