fix!: Correct between filtering (#22918)
* fix: Remove incorrect fallback If you do +1 on date it will also start considering next date. This was only done to accomodate date filter on datetime fields. Which also doesn't really work. * refactor: simplify fieldtype detection * fix!: Correct datetime fallbacks for between filters * chore: remove unncessary test This is very specific and introduces flake when the job tests run before it.
This commit is contained in:
Ankush Menat
GitHub
parent
a643098a26
commit
385fa8aaef
4 changed files with 93 additions and 36 deletions
|
|
@ -68,15 +68,6 @@ class TestScheduledJobType(FrappeTestCase):
|
|||
self.assertFalse(job.is_event_due(get_datetime("2019-01-31 23:59:59")))
|
||||
|
||||
def test_cron_job(self):
|
||||
# Daily but offset by 45 minutes
|
||||
job = frappe.get_doc(
|
||||
"Scheduled Job Type",
|
||||
dict(method="frappe.core.doctype.log_settings.log_settings.run_log_clean_up"),
|
||||
)
|
||||
self.assertEqual(
|
||||
job.next_execution,
|
||||
add_to_date(None, days=1).replace(hour=0, minute=45, second=0, microsecond=0),
|
||||
)
|
||||
# runs every 15 mins
|
||||
job = frappe.get_doc("Scheduled Job Type", dict(method="frappe.oauth.delete_oauth2_data"))
|
||||
job.db_set("last_execution", "2019-01-01 00:00:00")
|
||||
|
|
|
|||
|
|
@ -3,10 +3,10 @@
|
|||
"""build query for doclistview and return results"""
|
||||
|
||||
import copy
|
||||
import datetime
|
||||
import json
|
||||
import re
|
||||
from collections import Counter
|
||||
from datetime import datetime
|
||||
|
||||
import frappe
|
||||
import frappe.defaults
|
||||
|
|
@ -21,7 +21,6 @@ from frappe.model.utils import is_virtual_doctype
|
|||
from frappe.model.utils.user_settings import get_user_settings, update_user_settings
|
||||
from frappe.query_builder.utils import Column
|
||||
from frappe.utils import (
|
||||
add_to_date,
|
||||
cint,
|
||||
cstr,
|
||||
flt,
|
||||
|
|
@ -30,7 +29,7 @@ from frappe.utils import (
|
|||
get_timespan_date_range,
|
||||
make_filter_tuple,
|
||||
)
|
||||
from frappe.utils.data import sbool
|
||||
from frappe.utils.data import DateTimeLikeObject, get_datetime, getdate, sbool
|
||||
|
||||
LOCATE_PATTERN = re.compile(r"locate\([^,]+,\s*[`\"]?name[`\"]?\s*\)", flags=re.IGNORECASE)
|
||||
LOCATE_CAST_PATTERN = re.compile(
|
||||
|
|
@ -853,7 +852,7 @@ class DatabaseQuery:
|
|||
value = frappe.db.format_date(f.value)
|
||||
fallback = "'0001-01-01'"
|
||||
|
||||
elif (df and df.fieldtype == "Datetime") or isinstance(f.value, datetime):
|
||||
elif (df and df.fieldtype == "Datetime") or isinstance(f.value, datetime.datetime):
|
||||
value = frappe.db.format_datetime(f.value)
|
||||
fallback = f"'{FallBackDateTimeStr}'"
|
||||
|
||||
|
|
@ -1247,10 +1246,18 @@ def has_any_user_permission_for_doctype(doctype, user, applicable_for):
|
|||
|
||||
|
||||
def get_between_date_filter(value, df=None):
|
||||
"""Handle datetime filter bounds for between filter values.
|
||||
|
||||
If date is passed but fieldtype is datetime then
|
||||
from part is converted to start of day and to part is converted to end of day.
|
||||
If any of filter part (to or from) are missing then:
|
||||
start or end of current day is assumed as fallback.
|
||||
If fieldtypes match with filter values then:
|
||||
no change is applied.
|
||||
"""
|
||||
return the formattted date as per the given example
|
||||
[u'2017-11-01', u'2017-11-03'] => '2017-11-01 00:00:00.000000' AND '2017-11-04 00:00:00.000000'
|
||||
"""
|
||||
|
||||
fieldtype = df and df.fieldtype or "Datetime"
|
||||
|
||||
from_date = frappe.utils.nowdate()
|
||||
to_date = frappe.utils.nowdate()
|
||||
|
||||
|
|
@ -1260,18 +1267,35 @@ def get_between_date_filter(value, df=None):
|
|||
if len(value) >= 2:
|
||||
to_date = value[1]
|
||||
|
||||
if not df or (df and df.fieldtype == "Datetime"):
|
||||
to_date = add_to_date(to_date, days=1)
|
||||
# if filter value is date but fieldtype is datetime:
|
||||
if fieldtype == "Datetime":
|
||||
from_date = _convert_type_for_between_filters(from_date, set_time=datetime.time())
|
||||
to_date = _convert_type_for_between_filters(to_date, set_time=datetime.time(23, 59, 59, 999999))
|
||||
|
||||
if df and df.fieldtype == "Datetime":
|
||||
data = "'{}' AND '{}'".format(
|
||||
frappe.db.format_datetime(from_date),
|
||||
frappe.db.format_datetime(to_date),
|
||||
)
|
||||
# If filter value is already datetime, do nothing.
|
||||
if fieldtype == "Datetime":
|
||||
cond = f"'{frappe.db.format_datetime(from_date)}' AND '{frappe.db.format_datetime(to_date)}'"
|
||||
else:
|
||||
data = f"'{frappe.db.format_date(from_date)}' AND '{frappe.db.format_date(to_date)}'"
|
||||
cond = f"'{frappe.db.format_date(from_date)}' AND '{frappe.db.format_date(to_date)}'"
|
||||
|
||||
return data
|
||||
return cond
|
||||
|
||||
|
||||
def _convert_type_for_between_filters(
|
||||
value: DateTimeLikeObject, set_time: datetime.time
|
||||
) -> datetime.datetime:
|
||||
if isinstance(value, str):
|
||||
if " " in value.strip():
|
||||
value = get_datetime(value)
|
||||
else:
|
||||
value = getdate(value)
|
||||
|
||||
if isinstance(value, datetime.datetime):
|
||||
return value
|
||||
elif isinstance(value, datetime.date):
|
||||
return datetime.datetime.combine(value, set_time)
|
||||
|
||||
return value
|
||||
|
||||
|
||||
def get_additional_filter_field(additional_filters_config, f, value):
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@ from frappe.custom.doctype.property_setter.property_setter import make_property_
|
|||
from frappe.database.utils import DefaultOrderBy
|
||||
from frappe.desk.reportview import get_filters_cond
|
||||
from frappe.handler import execute_cmd
|
||||
from frappe.model.db_query import DatabaseQuery
|
||||
from frappe.model.db_query import DatabaseQuery, get_between_date_filter
|
||||
from frappe.permissions import add_user_permission, clear_user_permissions_for_doctype
|
||||
from frappe.query_builder import Column
|
||||
from frappe.tests.utils import FrappeTestCase
|
||||
|
|
@ -286,7 +286,7 @@ class TestDBQuery(FrappeTestCase):
|
|||
event1 = create_event(starts_on="2016-07-05 23:59:59")
|
||||
event2 = create_event(starts_on="2016-07-06 00:00:00")
|
||||
event3 = create_event(starts_on="2016-07-07 23:59:59")
|
||||
event4 = create_event(starts_on="2016-07-08 00:00:01")
|
||||
event4 = create_event(starts_on="2016-07-08 00:00:00")
|
||||
|
||||
# if the values are not passed in filters then event should be filter as current datetime
|
||||
data = DatabaseQuery("Event").execute(filters={"starts_on": ["between", None]}, fields=["name"])
|
||||
|
|
@ -298,27 +298,58 @@ class TestDBQuery(FrappeTestCase):
|
|||
filters={"starts_on": ["between", ["2016-07-06", "2016-07-07"]]}, fields=["name"]
|
||||
)
|
||||
|
||||
self.assertTrue({"name": event2.name} in data)
|
||||
self.assertTrue({"name": event3.name} in data)
|
||||
self.assertTrue({"name": event1.name} not in data)
|
||||
self.assertTrue({"name": event4.name} not in data)
|
||||
self.assertIn({"name": event2.name}, data)
|
||||
self.assertIn({"name": event3.name}, data)
|
||||
self.assertNotIn({"name": event1.name}, data)
|
||||
self.assertNotIn({"name": event4.name}, data)
|
||||
|
||||
# if only one value is passed in the filter
|
||||
data = DatabaseQuery("Event").execute(
|
||||
filters={"starts_on": ["between", ["2016-07-07"]]}, fields=["name"]
|
||||
)
|
||||
|
||||
self.assertTrue({"name": event3.name} in data)
|
||||
self.assertTrue({"name": event4.name} in data)
|
||||
self.assertTrue({"name": todays_event.name} in data)
|
||||
self.assertTrue({"name": event1.name} not in data)
|
||||
self.assertTrue({"name": event2.name} not in data)
|
||||
self.assertIn({"name": event3.name}, data)
|
||||
self.assertIn({"name": event4.name}, data)
|
||||
self.assertIn({"name": todays_event.name}, data)
|
||||
self.assertNotIn({"name": event1.name}, data)
|
||||
self.assertNotIn({"name": event2.name}, data)
|
||||
|
||||
# test between is formatted for creation column
|
||||
data = DatabaseQuery("Event").execute(
|
||||
filters={"creation": ["between", ["2016-07-06", "2016-07-07"]]}, fields=["name"]
|
||||
)
|
||||
|
||||
def test_between_filters_date_bounds(self):
|
||||
date_df = frappe._dict(fieldtype="Date")
|
||||
datetime_df = frappe._dict(fieldtype="Datetime")
|
||||
today = frappe.utils.nowdate()
|
||||
|
||||
# No filters -> assumes today
|
||||
cond = get_between_date_filter("", date_df)
|
||||
self.assertQueryEqual(cond, f"'{today}' AND '{today}'")
|
||||
|
||||
# One filter assumes "from" bound and to is today
|
||||
start = "2021-01-01"
|
||||
cond = get_between_date_filter([start], date_df)
|
||||
self.assertQueryEqual(cond, f"'{start}' AND '{today}'")
|
||||
|
||||
# both date filters are applied
|
||||
start = "2021-01-01"
|
||||
end = "2022-01-02"
|
||||
cond = get_between_date_filter([start, end], date_df)
|
||||
self.assertQueryEqual(cond, f"'{start}' AND '{end}'")
|
||||
|
||||
# single date should include entire day
|
||||
start = "2021-01-01"
|
||||
cond = get_between_date_filter([start, start], datetime_df)
|
||||
self.assertQueryEqual(cond, f"'{start} 00:00:00.000000' AND '{start} 23:59:59.999999'")
|
||||
|
||||
# datetime field on datetime type should remain same
|
||||
start = "2021-01-01 01:01:00"
|
||||
end = "2022-01-02 12:23:43"
|
||||
cond = get_between_date_filter([start, end], datetime_df)
|
||||
self.assertQueryEqual(cond, f"'{start}.000000' AND '{end}.000000'")
|
||||
|
||||
def test_ignore_permissions_for_get_filters_cond(self):
|
||||
frappe.set_user("test2@example.com")
|
||||
self.assertRaises(frappe.PermissionError, get_filters_cond, "DocType", dict(istable=1), [])
|
||||
|
|
|
|||
|
|
@ -86,6 +86,17 @@ class FrappeTestCase(unittest.TestCase):
|
|||
|
||||
return BeautifulSoup(code, "html.parser").prettify(formatter=None)
|
||||
|
||||
def normalize_sql(self, query: str) -> str:
|
||||
"""Formats SQL consistently so simple string comparisons can work on them."""
|
||||
import sqlparse
|
||||
|
||||
return (
|
||||
sqlparse.format(query.strip(), keyword_case="upper", reindent=True, strip_comments=True),
|
||||
)
|
||||
|
||||
def assertQueryEqual(self, first: str, second: str):
|
||||
self.assertEqual(self.normalize_sql(first), self.normalize_sql(second))
|
||||
|
||||
@contextmanager
|
||||
def assertQueryCount(self, count):
|
||||
queries = []
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue