Make IP address optional when reporting transactions

[ugexe]

No description provided.

[ugexe]

There seem to be tox failures in this branch that also exist on the latest commit in the main branch 🤔

[oschwald]

Would something like this work instead:

id_schema = Schema(
  {
    Required(
      Any("ip_address", "maxmind_id", "minfraud_id", "transaction_id"), 
      msg="The report must contain at least one of the following fields: 'ip_address', 'maxmind_id', 'minfraud_id', 'transaction_id'.",
    )
  },
  extra=ALLOW_EXTRA,
)

validate_report = All(validate_report_schema, id_schema)

It might also be good to prefix everything we don't intend to be public with an underscore.

[ugexe]

I'm not sure that we can. It'd have to be more like having a schema for validating the keys, and another schema for validating the values. For example this does not work:

_report_schema = Schema(
    {
        "chargeback_code": str,
        "ip_address": _ip_address,
        "maxmind_id": _maxmind_id,
        "minfraud_id": _uuid,
        "notes": str,
        "tag": _tag,
        "transaction_id": _transaction_id,
    },
)

_report_required_schema = Schema(
    {
        Required(
            Any("ip_address", "maxmind_id", "minfraud_id", "transaction_id"),
            msg="The report must contain at least one of the following fields: 'ip_address', 'maxmind_id', 'minfraud_id', 'transaction_id'.",
        ): object,
    },
    extra=ALLOW_EXTRA,
)

validate_report = All(_report_schema, _report_required_schema)

It gives errors for various tests like this:

./test_validation.py::TestReport::test_ip_address Failed: MultipleInvalid The report must contain at least one of the following fields: 'ip_address', 'maxmind_id', 'minfraud_id', 'transaction_id'. @ data['tag'] thrown for {'ip_address': '182.193.2.1', 'tag': 'chargeback'}
  File "/home/nlogan_maxmind_com/minfraud-api-python/tests/test_validation.py", line 66, in check_report_no_setup
    validate_report(report)

We can see ip_address was indeed sent but the validation is not seeing it. The following does work:

_report_schema = Schema(
    {
        "chargeback_code": str,
        "ip_address": _ip_address,
        "maxmind_id": _maxmind_id,
        "minfraud_id": _uuid,
        "notes": str,
        "tag": _tag,
        "transaction_id": _transaction_id,
    },
)

_report_required_schema = Schema(
    {
        "chargeback_code": object,
        "notes": object,
        "tag": object,
        Required(
            Any("ip_address", "maxmind_id", "minfraud_id", "transaction_id"),
            msg="The report must contain at least one of the following fields: 'ip_address', 'maxmind_id', 'minfraud_id', 'transaction_id'.",
        ): object,
    },
)

validate_report = All(_report_schema, _report_required_schema)

To me this suggests we would have to (re)define the optional fields in each schema used with All(...). alecthomas/voluptuous/issues/126#issuecomment-326487353 seems to align with that. I

[oschwald]

Weird. I would have expected that to work. I should have tested it. Your tests do pass for me with this though:

validate_report = Schema(
    {

        Any(Required("ip_address"), Required("maxmind_id"), Required("minfraud_id"), Required("transaction_id")): str, 
        "chargeback_code": str,
        "ip_address": _ip_address,
        "maxmind_id": _maxmind_id,
        "minfraud_id": _uuid,
        "notes": str,
        Required("tag"): _tag,
        "transaction_id": _transaction_id,
    },
)

I haven't looked too closely besides running the tests.

[ugexe]

That doesn't seem to give a validation error when only tag is supplied. I had to add a new test to catch this though

diff --git a/tests/test_validation.py b/tests/test_validation.py
index 87084ae..b905285 100644
--- a/tests/test_validation.py
+++ b/tests/test_validation.py
@@ -439,6 +439,7 @@ class TestReport(unittest.TestCase, ValidationBase):
             self.check_invalid_report({"tag": bad})

     def test_report_valid_identifier(self):
+        self.check_invalid_report_no_setup({"tag": "chargeback"})
         self.check_report_no_setup({"tag": "chargeback", "ip_address": "1.1.1.1"})
         self.check_report_no_setup({"tag": "chargeback", "minfraud_id": "58fa38d8-4b87-458b-a22b-f00eda1aa20d"})
         self.check_report_no_setup({"tag": "chargeback", "maxmind_id": "12345678"})

[oschwald]

It seems like there should be some way to do it, but I don't know that we need to dwell on that here. It would be worth adding that test case to this API and the other APIs if relevant.

[oschwald]

Missing a newline at the end of this file.

[oschwald]

Sorry, I didn't mean that we should add an underscore to these. I just meant the intermediate functions that you added that are only used in that file.

[oschwald]

This is probably bikeshedding, but expressing it as a set operation seems a bit simpler to me:

Suggested change

	optional_fields = ["ip_address", "maxmind_id", "minfraud_id", "transaction_id"]
	if not any(field in report for field in optional_fields):
	optional_fields = {"ip_address", "maxmind_id", "minfraud_id", "transaction_id"}
	if not optional_fields & report.keys():

[oschwald]

That said, maybe we should handle the empty string for these too like we do for the other APIs.

[ugexe]

The individual field validation that exists should already disallow empty strings for all of these fields. The only one we might need to check is minfraid_id to disallow the zero UUID