feat: Enhance PaymentRequest with rich domain model , value objects and unit tests #4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a rich domain model around payment requests with encapsulated value objects, enforced validation, and accompanying unit tests.
- Defines value objects (Money, CardNumber, CVV, ExpiryDate, Payer, Card, BaseCurrency) with field validation.
- Implements
PaymentRequestentity behaviors (factory instantiation, setters, status updates) and validation. - Adds unit tests and a masking service for payment card fields.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Tests/Honamic.PayMaster.Abstractions.UnitTest/CardNumberTest.cs | Adds unit tests for CardNumber validation |
| src/Tests/Honamic.PayMaster.Abstractions.UnitTest/CardMaskingServiceTest.cs | Adds unit tests for CardMaskingService |
| src/Tests/Honamic.PayMaster.Abstractions.UnitTest/CVVTest.cs | Adds unit tests for CVV validation |
| src/PayMaster.sln | Registers the new unit test project and solution folder |
| src/Abstractions/PaymentProviders/ValueObjects/Payer.cs | Introduces Payer value object |
| src/Abstractions/PaymentProviders/ValueObjects/Money.cs | Introduces Money value object with arithmetic methods |
| src/Abstractions/PaymentProviders/ValueObjects/ExpiryDate.cs | Introduces ExpiryDate value object with expiry checks |
| src/Abstractions/PaymentProviders/ValueObjects/CardNumber.cs | Introduces CardNumber value object with length validation |
| src/Abstractions/PaymentProviders/ValueObjects/Card.cs | Introduces Card aggregate value object |
| src/Abstractions/PaymentProviders/ValueObjects/CVV.cs | Introduces CVV value object with length validation |
| src/Abstractions/PaymentProviders/ValueObjects/BaseCurrency.cs | Defines supported currencies and symbols |
| src/Abstractions/PaymentProviders/Services/CardMaskingService.cs | Implements card number masking logic |
| src/Abstractions/PaymentProviders/Exceptions/PaymentNotFoundException.cs | Adds PaymentNotFoundException |
| src/Abstractions/PaymentProviders/Exceptions/InvalidExpiryDateException.cs | Adds InvalidExpiryDateException |
| src/Abstractions/PaymentProviders/Exceptions/InvalidCurrencyException.cs | Adds InvalidCurrencyException |
| src/Abstractions/PaymentProviders/Exceptions/InvalidCardNumberException.cs | Adds InvalidCardNumberException |
| src/Abstractions/PaymentProviders/Exceptions/InvalidCVVException.cs | Adds InvalidCVVException |
| src/Abstractions/PaymentProviders/Exceptions/InvalidAmountException.cs | Adds InvalidAmountException |
| src/Abstractions/PaymentProviders/Exceptions/DuplicatePaymentException.cs | Adds DuplicatePaymentException |
| src/Abstractions/PaymentProviders/Entities/PaymentRequest.cs | Implements PaymentRequest entity and validation methods |
Comments suppressed due to low confidence (1)
src/Abstractions/PaymentProviders/ValueObjects/CardNumber.cs:36
- Validation only checks length; consider stripping whitespace and verifying all characters are digits and potentially applying the Luhn algorithm to prevent invalid card numbers.
return Regex.Replace(number, "@\\s+", "").Length == 16;
| /// <returns>A masked card number</returns> | ||
| public static string Mask(CardNumber cardNumber) | ||
| { | ||
| var visiblePart = cardNumber.Number.Substring(0 + cardNumber.Number.Length - UnmaskedDigits); |
There was a problem hiding this comment.
The 0 + in the Substring call is redundant; simplify to cardNumber.Number.Substring(cardNumber.Number.Length - UnmaskedDigits) for clarity.
Suggested change
| var visiblePart = cardNumber.Number.Substring(0 + cardNumber.Number.Length - UnmaskedDigits); | |
| var visiblePart = cardNumber.Number.Substring(cardNumber.Number.Length - UnmaskedDigits); |
Comment on lines
+71
to
+72
| if (!string.IsNullOrEmpty(mobileNumber) && mobileNumber.Length != 11) | ||
| throw new ArgumentException("Mobile number must be 11 digits.", nameof(mobileNumber)); |
There was a problem hiding this comment.
Mobile number is only length-checked; you should also enforce that it contains only digits (e.g. with a regex) to avoid invalid input.
Suggested change
| if (!string.IsNullOrEmpty(mobileNumber) && mobileNumber.Length != 11) | |
| throw new ArgumentException("Mobile number must be 11 digits.", nameof(mobileNumber)); | |
| if (!string.IsNullOrEmpty(mobileNumber)) | |
| { | |
| if (mobileNumber.Length != 11 || !Regex.IsMatch(mobileNumber, @"^\d{11}$")) | |
| throw new ArgumentException("Mobile number must be exactly 11 digits and contain only numeric characters.", nameof(mobileNumber)); | |
| } |
| { "AUD", "Australian dollar (A$)" }, | ||
| { "BRL", "Brazilian real (R$)" }, | ||
| { "CAD", "Canadian dollar (C$)" }, | ||
| { "CNY", "Chinese Renmenbi (¥)" }, |
There was a problem hiding this comment.
Typo in currency name: "Renmenbi" should be "Renminbi".
Suggested change
| { "CNY", "Chinese Renmenbi (¥)" }, | |
| { "CNY", "Chinese Renminbi (¥)" }, |
| throw new InvalidExpiryDateException($"{month}/{year} is not a valid expiry date"); | ||
| } | ||
|
|
||
| //TODO: it works only on forieng countries cards, should be globalized by culture |
There was a problem hiding this comment.
Typo in comment: "forieng" should be "foreign".
Suggested change
| //TODO: it works only on forieng countries cards, should be globalized by culture | |
| //TODO: it works only on foreign countries' cards, should be globalized by culture |
| throw new InvalidAmountException(); | ||
|
|
||
| if (string.IsNullOrEmpty(currency) || !BaseCurrency.CurrencySymbols.ContainsKey(currency.ToUpper())) | ||
| throw new InvalidCurrencyException(); |
There was a problem hiding this comment.
Throwing InvalidCurrencyException without a message can make debugging harder; consider including the invalid currency code in the exception message.
Suggested change
| throw new InvalidCurrencyException(); | |
| throw new InvalidCurrencyException($"Invalid currency code: '{currency}'."); |
| } | ||
|
|
||
| public void Validate() | ||
| { |
There was a problem hiding this comment.
Validation logic in Validate() duplicates the checks in Create; consider centralizing field validation in one place or reusing the factory checks to avoid drift.
Suggested change
| { | |
| { | |
| ValidateFields(); | |
| } | |
| private void ValidateFields() | |
| { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Createfor controlled instantiation.