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.

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

feat: Validate transfer transaction data #10

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

apexDev37
Copy link

Description

This PR introduces patches to increase developer feedback and consequently user experience.

The narrative field on the transaction data used in the transfer module is truncated by default for strings that exceed 22 characters. No documentation or info about this behavior exists to inform developers concerning a field used in the SMS notification for a successful transaction. This branch introduces a patch to raise if a user attempts to use a string exceeding the limit to avoid incomplete or obscure data passed to the customer/end-user.

Fixes: n/a

Below are examples of truncated narratives in the SMS notification from Intasend on a successful transaction for a live account.

truncated-narrative-airtime
truncated-narrative-mpesa-b2c

Decisions

This section outlines notable considerations for developers/maintainers.

  • Alternatively, logging a warning (non-blocking) may be more favorable to raising an exception (blocking). However, this requires configuring logging and that users observe these logs.

Testing

The following unit test was written to validate the function responsible for raising the exception.

"""Throw away tests to validate transactions data """

import unittest

from intasend.transfers import _validate_transaction_data
from intasend.exceptions import NarrativeExceedsLengthLimit


class TestValidationTransactionData(unittest.TestCase):
    """Test individual transaction data is valid"""

    def setUp(self) -> None:
        self.transactions = [
            {"name": "test-name", "account": "0722222222", "amount": 10, "narrative": "fees"},
            {"name": "test-name", "account": "0722222222", "amount": 10000, "narrative": "fees"},
        ]
        return super().setUp()

    def test_should_raise_if_narrative_length_exceeds_expected_limit(self) -> None:
        # Given
        self.transactions[0]["narrative"] = "Exceeds max length of 22 chars"

        with self.assertRaises(NarrativeExceedsLengthLimit):  # Then
            _validate_transaction_data(self.transactions)  # When


if __name__ == "__main__":
    unittest()

Note: The test module was omitted from the PR given no tests are included in this package.

Additional

General

  • [Issues] The following areas for improvement were noted when working with this repo locally. Maintainers can check each item for approval and I can follow up with a subsequent PR to integrate the recommendations.

Recommended change improvements:
Check if applicable:

  • (docs) Update transaction fields account and amount to use the correct type.
    The docs and example code for transaction data specify the fields account and amount as type int - which will result in a 400 BAD REQUEST response - instead of the correct and expected type, str.
  • (style) Format Python code, example, and docs code snippets with black.
    The codebase uses a custom format that is difficult to read. Using a recommended and opinionated formatter, such as black, ensures that the src resembles the majority of Python code, improving readability.
  • (feat) Raise if the transaction field amount is below the minimum required, 10.
    This applies the same validation logic used for the narrative field to increase user feedback.

Add logic to inform user on narrative data exceeding length limit.

[Note]
- This func can be extended to handle additional validation checks
Move check from `airtime` to common entry-point, `send_money`,
given that transaction data is required by other services.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant