From 24b92e1e8dfd895dcfda40bb76d40c132f977607 Mon Sep 17 00:00:00 2001 From: Milad <63479762+MRyderOC@users.noreply.github.com> Date: Sat, 18 May 2024 16:39:10 -0500 Subject: [PATCH 1/4] Remove strioct string passing --- src/dfcx_scrapi/builders/fulfillments.py | 39 +++++++++--------------- 1 file changed, 14 insertions(+), 25 deletions(-) diff --git a/src/dfcx_scrapi/builders/fulfillments.py b/src/dfcx_scrapi/builders/fulfillments.py index f3dea89f..e9fc09c7 100644 --- a/src/dfcx_scrapi/builders/fulfillments.py +++ b/src/dfcx_scrapi/builders/fulfillments.py @@ -15,7 +15,7 @@ # limitations under the License. import logging -from typing import Dict +from typing import List, Dict, Any from google.cloud.dialogflowcx_v3beta1.types import Fulfillment from google.cloud.dialogflowcx_v3beta1.types import ResponseMessage @@ -208,12 +208,12 @@ def add_response_message( def add_parameter_presets( self, - parameter_map: Dict[str, str] + parameter_map: Dict[str, Any] ) -> Fulfillment: """Set parameter values before executing the webhook. Args: - parameter_map (Dict[str, str]): + parameter_map (Dict[str, Any]): A dictionary that represents parameters as keys and the parameter values as it's values. A `None` value clears the parameter. @@ -225,13 +225,10 @@ def add_parameter_presets( # Type error checking if isinstance(parameter_map, dict): - if not all(( - isinstance(key, str) and isinstance(val, str) - for key, val in parameter_map.items() - )): + if not all(isinstance(k, str) for k in parameter_map.keys()): raise ValueError( "Only strings are allowed as" - " dictionary keys and values in parameter_map." + " dictionary keys in parameter_map." ) for param, val in parameter_map.items(): self.proto_obj.set_parameter_actions.append( @@ -247,15 +244,14 @@ def add_parameter_presets( def remove_parameter_presets( self, - parameter_map: Dict[str, str] + parameters: List[str] ) -> Fulfillment: """Remove parameter values from the fulfillment. Args: - parameter_map (Dict[str, str]): - A dictionary that represents parameters as keys - and the parameter values as it's values. - A `None` value clears the parameter. + parameters (List[str]): + A list of parameters that should be removed from the fulfillment. + Only strings are allowed. Returns: A Fulfillment object stored in proto_obj @@ -263,22 +259,15 @@ def remove_parameter_presets( self._check_proto_obj_attr_exist() # Type error checking - if isinstance(parameter_map, dict): - if not all(( - isinstance(key, str) and isinstance(val, str) - for key, val in parameter_map.items() - )): + if isinstance(parameters, list): + if not all(isinstance(p, str) for p in parameters): raise ValueError( - "Only strings are allowed as" - " dictionary keys and values in parameter_map." + "Only strings are allowed as in parameters." ) new_params = [] for param in self.proto_obj.set_parameter_actions: - if ( - param.parameter in parameter_map and - param.value == parameter_map[param.parameter] - ): + if param.parameter in parameters: continue new_params.append(param) @@ -287,7 +276,7 @@ def remove_parameter_presets( return self.proto_obj else: raise ValueError( - "parameter_map should be a dictionary." + "parameter_map should be a list of strings." ) From d70f715674721d0517ea145f6df418cc644d0dcd Mon Sep 17 00:00:00 2001 From: MRyderOC Date: Wed, 22 May 2024 20:15:53 -0500 Subject: [PATCH 2/4] Solve a potential bug and add some tests for FulfilmmentBuilder --- src/dfcx_scrapi/builders/fulfillments.py | 2 +- .../dfcx_scrapi/builders/test_fulfillments.py | 54 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 tests/dfcx_scrapi/builders/test_fulfillments.py diff --git a/src/dfcx_scrapi/builders/fulfillments.py b/src/dfcx_scrapi/builders/fulfillments.py index e9fc09c7..f2410be4 100644 --- a/src/dfcx_scrapi/builders/fulfillments.py +++ b/src/dfcx_scrapi/builders/fulfillments.py @@ -163,7 +163,7 @@ def create_new_proto_obj( "tag is required when webhook is specified." ) # `overwrite` parameter error checking - if self.proto_obj and not overwrite: + if self.proto_obj is not None and not overwrite: raise UserWarning( "proto_obj already contains a Fulfillment." " If you wish to overwrite it, pass overwrite as True." diff --git a/tests/dfcx_scrapi/builders/test_fulfillments.py b/tests/dfcx_scrapi/builders/test_fulfillments.py new file mode 100644 index 00000000..056299ae --- /dev/null +++ b/tests/dfcx_scrapi/builders/test_fulfillments.py @@ -0,0 +1,54 @@ +"""Test Class for FulfillmentBuilder in SCRAPI's builder package.""" + +# Copyright 2023 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import pytest + +from dfcx_scrapi.builders.fulfillments import Fulfillment +from dfcx_scrapi.builders.fulfillments import FulfillmentBuilder + + +def test_create_new_proto_obj(): + fb = FulfillmentBuilder() + assert fb.proto_obj is None + + fb.create_new_proto_obj() + assert isinstance(fb.proto_obj, Fulfillment) + assert fb.proto_obj.return_partial_responses == False + assert fb.proto_obj.messages == [] + + with pytest.raises(UserWarning): + fb.create_new_proto_obj(return_partial_responses=True) + + fb.create_new_proto_obj(return_partial_responses=True, overwrite=True) + assert fb.proto_obj.return_partial_responses == True + + +def test_create_new_proto_obj_with_webhook(): + fb = FulfillmentBuilder() + custom_wbhk = ( + "projects/sample_project_id/locations/sample_location_id" + "/agents/sample_agent_id/webhooks/sample_webhook_id") + fb.create_new_proto_obj( + webhook=custom_wbhk, + tag="new_tag" + ) + assert fb.proto_obj.webhook == custom_wbhk + assert fb.proto_obj.tag == "new_tag" + + with pytest.raises(ValueError): + fb.create_new_proto_obj(webhook=custom_wbhk, overwrite=True) + + From 97a67b4b993d63469ec0538a39c355c5b862b169 Mon Sep 17 00:00:00 2001 From: MRyderOC Date: Thu, 23 May 2024 19:25:54 -0500 Subject: [PATCH 3/4] typo fix and add more tests for add_parameter_presets method --- src/dfcx_scrapi/builders/fulfillments.py | 4 ++-- .../dfcx_scrapi/builders/test_fulfillments.py | 21 +++++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/dfcx_scrapi/builders/fulfillments.py b/src/dfcx_scrapi/builders/fulfillments.py index f2410be4..6b2194d0 100644 --- a/src/dfcx_scrapi/builders/fulfillments.py +++ b/src/dfcx_scrapi/builders/fulfillments.py @@ -228,7 +228,7 @@ def add_parameter_presets( if not all(isinstance(k, str) for k in parameter_map.keys()): raise ValueError( "Only strings are allowed as" - " dictionary keys in parameter_map." + " dictionary keys in `parameter_map`." ) for param, val in parameter_map.items(): self.proto_obj.set_parameter_actions.append( @@ -238,7 +238,7 @@ def add_parameter_presets( return self.proto_obj else: raise ValueError( - "parameter_map should be a dictionary." + "`parameter_map` should be a dictionary." ) diff --git a/tests/dfcx_scrapi/builders/test_fulfillments.py b/tests/dfcx_scrapi/builders/test_fulfillments.py index 056299ae..864c98e0 100644 --- a/tests/dfcx_scrapi/builders/test_fulfillments.py +++ b/tests/dfcx_scrapi/builders/test_fulfillments.py @@ -52,3 +52,24 @@ def test_create_new_proto_obj_with_webhook(): fb.create_new_proto_obj(webhook=custom_wbhk, overwrite=True) +def test_add_parameter_presets_with_valid_dict(): + valid_param_map = {"p1": "v1", "p2": 123, "p3": True, "p4": None} + fb = FulfillmentBuilder() + fb.create_new_proto_obj() + fb.add_parameter_presets(valid_param_map) + + for p in fb.proto_obj.set_parameter_actions: + assert p.parameter in valid_param_map + assert p.value == valid_param_map[p.parameter] + + +def test_add_parameter_presets_with_invalid_dict(): + invalid_param_map = {"p1": "v1", 123: "p2"} + fb = FulfillmentBuilder() + fb.create_new_proto_obj() + with pytest.raises(ValueError): + fb.add_parameter_presets(invalid_param_map) + + # passing a list instead of dict + with pytest.raises(ValueError): + fb.add_parameter_presets(list(invalid_param_map.keys())) From 1b602964a3d5e45221a536f8fa3e326823c411e9 Mon Sep 17 00:00:00 2001 From: MRyderOC Date: Fri, 24 May 2024 11:37:15 -0500 Subject: [PATCH 4/4] Add more tests for the FulfillmentBuilder --- .../dfcx_scrapi/builders/test_fulfillments.py | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/tests/dfcx_scrapi/builders/test_fulfillments.py b/tests/dfcx_scrapi/builders/test_fulfillments.py index 864c98e0..af54f79a 100644 --- a/tests/dfcx_scrapi/builders/test_fulfillments.py +++ b/tests/dfcx_scrapi/builders/test_fulfillments.py @@ -26,14 +26,14 @@ def test_create_new_proto_obj(): fb.create_new_proto_obj() assert isinstance(fb.proto_obj, Fulfillment) - assert fb.proto_obj.return_partial_responses == False + assert fb.proto_obj.return_partial_responses is False assert fb.proto_obj.messages == [] with pytest.raises(UserWarning): fb.create_new_proto_obj(return_partial_responses=True) fb.create_new_proto_obj(return_partial_responses=True, overwrite=True) - assert fb.proto_obj.return_partial_responses == True + assert fb.proto_obj.return_partial_responses is True def test_create_new_proto_obj_with_webhook(): @@ -62,6 +62,13 @@ def test_add_parameter_presets_with_valid_dict(): assert p.parameter in valid_param_map assert p.value == valid_param_map[p.parameter] + new_params_map = {"n1": 12, "n2": "v2", "n3": 1.2} + fb.add_parameter_presets(new_params_map) + all_params_map = {**valid_param_map, **new_params_map} + for p in fb.proto_obj.set_parameter_actions: + assert p.parameter in all_params_map + assert p.value == all_params_map[p.parameter] + def test_add_parameter_presets_with_invalid_dict(): invalid_param_map = {"p1": "v1", 123: "p2"} @@ -73,3 +80,38 @@ def test_add_parameter_presets_with_invalid_dict(): # passing a list instead of dict with pytest.raises(ValueError): fb.add_parameter_presets(list(invalid_param_map.keys())) + + +def test_remove_parameter_presets_single_param(): + fb = FulfillmentBuilder() + fb.create_new_proto_obj() + params_map = {"p1": "v1", "p2": 123, "p3": True, "p4": None} + fb.add_parameter_presets(parameter_map=params_map) + fb.remove_parameter_presets(["p1"]) + params_map.pop("p1") + for p in fb.proto_obj.set_parameter_actions: + assert p.parameter in params_map + assert p.value == params_map[p.parameter] + + +def test_remove_parameter_presets_multi_params(): + fb = FulfillmentBuilder() + fb.create_new_proto_obj() + params_map = {"p1": "v1", "p2": 123, "p3": True, "p4": None} + fb.add_parameter_presets(parameter_map=params_map) + fb.remove_parameter_presets(["p1", "p4"]) + params_map.pop("p1") + params_map.pop("p4") + for p in fb.proto_obj.set_parameter_actions: + assert p.parameter in params_map + assert p.value == params_map[p.parameter] + + +def test_has_webhook(): + fb = FulfillmentBuilder() + fb.create_new_proto_obj() + assert not fb.has_webhook() + + fb.create_new_proto_obj( + webhook="sample_webhook_id", tag="some_tag", overwrite=True) + assert fb.has_webhook()