Merge pull request #78 from ServiceNow/scratch/fix-validate-filter-list-task

Do not assume = is the only operator in filter conditions

Author: jardinetsouffleton

src/browsergym/workarena/tasks/list.py

@@ -101,6 +101,11 @@
 
 
 class ServiceNowListTask(AbstractServiceNowTask):
+    OPERATOR_EQUALS = "="
+    OPERATOR_NOT_EQUALS = "!="
+    OPERATOR_STARTSWITH = "STARTSWITH"
+    OPERATOR_ISEMPTY = "ISEMPTY"
+    OPERATOR_EMPTYSTRING = "EMPTYSTRING"
 
     @classmethod
     def all_configs(cls) -> List[dict]:
@@ -777,6 +782,9 @@ def validate(
         list_info = self._extract_list_info(page)
         current_query = list_info["query"]
 
+        if not current_query:
+            return 0, False, "", {"message": "There are no filters yet."}
+
         # Replace "new query" statements with the standard OR separator
         current_query = current_query.replace("^NQ", "^OR")
 
@@ -789,24 +797,74 @@ def validate(
             current_sep = "^"
 
         if current_kind != self.filter_kind:
-            return 0, False, "", {"message": "The kind of filter used is incorrect."}
+            return (
+                0,
+                False,
+                "",
+                {"message": f"The kind of filter used is incorrect: {current_query}."},
+            )
 
         # Extract the query pieces for validation
         current_query = current_query.split(current_sep)
 
         # Validate query length is ok
         if len(current_query) != self.filter_len:
-            return 0, False, "", {"message": "Incorrect number of filter conditions."}
+            return (
+                0,
+                False,
+                "",
+                {"message": f"Incorrect number of filter conditions: {current_query}."},
+            )
+
+        # Parse column names, operators, and values
+        current_columns, current_operators, current_values = [], [], []
+
+        # Note that this is not exhaustive. If/when other operators are added, this will have to be updated.
+        for predicate in current_query:
+            if self.OPERATOR_EMPTYSTRING in predicate:
+                current_columns.append(predicate.replace(self.OPERATOR_EMPTYSTRING, "").strip())
+                current_operators.append("=")
+                current_values.append("")
+            elif self.OPERATOR_ISEMPTY in predicate:
+                current_columns.append(predicate.replace(self.OPERATOR_ISEMPTY, "").strip())
+                current_operators.append("=")
+                current_values.append("")
+            elif any(
+                unsupported_operator in predicate
+                for unsupported_operator in [self.OPERATOR_NOT_EQUALS, self.OPERATOR_STARTSWITH]
+            ):
+                return (
+                    0,
+                    False,
+                    "",
+                    {"message": f"Unexpected operator in filter condition: {current_query}."},
+                )
+            elif self.OPERATOR_EQUALS in predicate:
+                col, val = predicate.split(self.OPERATOR_EQUALS, 1)
+                current_columns.append(col.strip())
+                current_operators.append("=")
+                current_values.append(val.strip())
+            else:
+                return (
+                    0,
+                    False,
+                    "",
+                    {"message": f"Unexpected operator in filter condition: {current_query}."},
+                )
 
-        # Validate query columns are ok
-        current_columns = [x.split("=")[0] for x in current_query]
         if set(current_columns) != set(self.filter_columns):
-            return 0, False, "", {"message": "Incorrect filter columns."}
+            return (
+                0,
+                False,
+                "",
+                {
+                    "message": f"Incorrect filter columns: {set(current_columns)}. Expected: {set(self.filter_columns)}."
+                },
+            )
 
         # Validate query values are ok
         # This is the tricky part because we need to expand the values to their display values
         # We also need to handle the case where the value is a reference
-        current_values = [x.split("=")[1] for x in current_query]
 
         # Handle filtering across multiple rows
         if len(set(current_columns)) < len(current_columns):
@@ -856,9 +914,21 @@ def validate(
 
         # Validate the values
         if set(current_values) != set(self.filter_values):
-            return 0, False, "", {"message": "Incorrect filter values."}
+            return (
+                0,
+                False,
+                "",
+                {
+                    "message": f"Incorrect filter values {set(current_values)}. Expected: {set(self.filter_values)}."
+                },
+            )
 
-        return 1, True, "Nice work, thank you!", {"message": "Correct filter."}
+        return (
+            1,
+            True,
+            "Nice work, thank you!",
+            {"message": f"Correct filter: {list_info['query']}."},
+        )
 
 
 class ExtractListInfoTask(ServiceNowListTask):

tests/test_filter_list_task.py

@@ -0,0 +1,81 @@
+import pytest
+from playwright.sync_api import Page
+from browsergym.workarena.tasks.list import FilterIncidentListTask
+
+
+# These are all the same ways to express the same filter query (empty string) in ServiceNow.
+@pytest.mark.parametrize(
+    "query",
+    [
+        "assigned_toEMPTYSTRING^short_descriptionISEMPTY^description=This is a beautiful incident",
+        "assigned_toISEMPTY^short_descriptionEMPTYSTRING^description=This is a beautiful incident^",
+        "assigned_toISEMPTY^short_descriptionISEMPTY^description=This is a beautiful incident",
+        "assigned_toEMPTYSTRING^short_descriptionEMPTYSTRING^description=This is a beautiful incident^",
+        "assigned_to=^short_description=^description=This is a beautiful incident",
+    ],
+)
+@pytest.mark.slow
+def test_validate_filter_list_task(page: Page, query):
+    fixed_config = {
+        "filter_columns": [
+            "short_description",
+            "assigned_to",
+            "description",
+        ],
+        "filter_kind": "AND",
+        "filter_values": [
+            "",
+            "",
+            "This is a beautiful incident",
+        ],
+    }
+    task = FilterIncidentListTask(seed=1, fixed_config=fixed_config)
+    _, _ = task.setup(page=page)
+    query = query.replace("^", r"%5E").replace("=", r"%3D")
+    task.page.goto(
+        task.instance.snow_url
+        + rf"/now/nav/ui/classic/params/target/incident_list.do?sysparm_query={query}"
+    )
+    reward, done, _, info = task.validate(page, [])
+    task.teardown()
+    assert done is True and reward == 1.0 and "Correct filter" in info["message"]
+
+
+# Different ways in which the filter is wrong
+@pytest.mark.parametrize(
+    "query, expected_message",
+    [
+        ("", "There are no filters yet"),
+        ("assignment_groupEMPTYSTRING", "Incorrect number of filter conditions"),
+        (
+            "assigned_toEMPTYSTRING^short_description!=Description",
+            "Unexpected operator in filter condition",
+        ),
+        ("assigned_toEMPTYSTRING^short_description=Description", "Incorrect filter columns"),
+        ("assigned_toISEMPTY^description=My Description", "Incorrect filter values"),
+    ],
+)
+@pytest.mark.slow
+def test_invalid_filter_list_task(page: Page, query, expected_message):
+    fixed_config = {
+        "filter_columns": [
+            "assigned_to",
+            "description",
+        ],
+        "filter_kind": "AND",
+        "filter_values": [
+            "",
+            "Description",
+        ],
+    }
+    task = FilterIncidentListTask(seed=1, fixed_config=fixed_config)
+    _, _ = task.setup(page=page)
+    query = query.replace("^", r"%5E").replace("=", r"%3D")
+    task.page.goto(
+        task.instance.snow_url
+        + f"/now/nav/ui/classic/params/target/incident_list.do?sysparm_query={query}"
+    )
+    reward, done, _, info = task.validate(page, [])
+    task.teardown()
+    print(info["message"])
+    assert done is False and reward == 0.0 and expected_message in info["message"]