Skip to content

Add Bill Pay & Vendor Management API support with tests#173

Open
mdunnpexcard wants to merge 3 commits intomasterfrom
126209-vendor-endpoints
Open

Add Bill Pay & Vendor Management API support with tests#173
mdunnpexcard wants to merge 3 commits intomasterfrom
126209-vendor-endpoints

Conversation

@mdunnpexcard
Copy link
Copy Markdown
Contributor

Comprehensive SDK update: adds enums, models, and client methods for PEX Bill Pay and Vendor Management APIs, including vendor and bill CRUD, approval, payment processing, and document handling. Includes a full end-to-end integration test and configuration support for secure, environment-specific testing. Also updates token model for richer authentication context.

Comprehensive SDK update: adds enums, models, and client methods for PEX Bill Pay and Vendor Management APIs, including vendor and bill CRUD, approval, payment processing, and document handling. Includes a full end-to-end integration test and configuration support for secure, environment-specific testing. Also updates token model for richer authentication context.
@devin-ai-integration
Copy link
Copy Markdown
Contributor

Code Review Summary

This is a comprehensive PR that adds Bill Pay & Vendor Management API support. The implementation follows existing SDK patterns well, and the integration test serves as excellent living documentation. However, I've identified several issues that should be addressed before merging.

Critical Issues

1. Enum Serialization Concern
The SDK's PexJsonSettings in PexClientExtensions.cs does not include StringEnumConverter, meaning enums serialize as numeric values by default. This affects:

  • BillPaymentMethodType (VendorCard=0, ACH=1) - no explicit values defined
  • VendorStatus, VendorStatusTrigger, VendorStatusUpdateTrigger - have explicit numeric values

Please confirm the API expects numeric enum values in request bodies. If the API expects string values (e.g., "ACH" instead of 1), this will cause silent failures.

2. Query Parameter Casing Inconsistency
Existing SDK methods use PascalCase for query params (IncludePendings, StartDate, AttachmentLinkType). The new code is inconsistent:

  • GetVendors uses camelCase: vendorStatuses, vendorStatusTriggers, pageIndex, pageSize
  • SearchBills uses PascalCase: CreatedDateFrom, VendorId, Page, PageSize

This inconsistency suggests at least one of these is incorrect. Please verify against the API documentation.

3. Decimal ToString() Without Culture Specification
In SearchBills, decimal values are converted without culture specification:

requestUriQueryParams.Add("AmountFrom", request.AmountFrom.Value.ToString());
requestUriQueryParams.Add("AmountTo", request.AmountTo.Value.ToString());

This will produce incorrect values in locales that use commas as decimal separators (e.g., 100,50 instead of 100.50). Use ToString(CultureInfo.InvariantCulture).

Security Concerns

4. Missing .gitignore Entry
The .gitignore file does not include appsettings.local.json. While the test code correctly documents that credentials should go in this file, without the gitignore entry, developers could accidentally commit real API tokens.

5. Bank Account Data in Logs
AddVendorBankAccountRequestModel contains sensitive fields (BankAccountNumber, BankRoutingNumber). Please verify that TerseLoggingHandler or any HTTP logging middleware does not log request bodies for these endpoints, or that sensitive fields are redacted.

Backwards Compatibility

6. Interface Breaking Change
Adding 30+ methods to IPexApiClient is a breaking change for any consumer who has implemented this interface (custom wrappers, test fakes, etc.). Consider:

  • Bumping to a major version
  • Creating separate interfaces (IPexVendorClient, IPexBillPayClient) that PexApiClient also implements
  • Using extension methods instead of interface methods

7. TokenDataModel Change (Minor)
Adding nullable PlatformAccountId property is backwards compatible - existing consumers will simply see null if the server doesn't return it.

Code Quality

8. Property Naming Inconsistency

  • CreateVendorRequestModel.WebSite vs VendorModel.Website - inconsistent casing of "Site"/"site"

9. Use of object Type
BillTagValueModel.Value and BillTagValueResponseModel.TagOptionValue use object type, which erodes type safety. If this matches existing tag patterns in the SDK, it's acceptable for consistency. Otherwise, consider JToken or strongly-typed alternatives.

Test Concerns

10. Integration Test Configuration
The test is well-documented but could accidentally run in CI if a token is configured. Consider:

  • Adding [Fact(Skip = "Integration test - requires manual configuration")] by default
  • Using an environment variable check instead of just checking for placeholder token

Minor Suggestions

  • The integration test output formatting is excellent for documentation purposes
  • Consider disposing HttpRequestMessage, MultipartFormDataContent, and StreamContent objects (though this matches existing SDK patterns)

Overall, this is solid work. The main blockers are the enum serialization verification, query param casing consistency, and the decimal culture issue. The interface breaking change should also be discussed with the team regarding versioning strategy.

Updated query parameter names to PascalCase for API consistency.
Switched "AmountFrom" and "AmountTo" to use invariant culture
formatting. Added System.Globalization import for support.
@mdunnpexcard
Copy link
Copy Markdown
Contributor Author

Devin AI review comments have been addressed in second commit.

@@ -0,0 +1,6 @@
{
"PexApi": {
"BaseUrl": "https://sand-coreapi.pexcard.com",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sandbox-coreapi.pexcard.com

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok updated to sandbox

Changed the PexApi BaseUrl in appsettings.json from sand-coreapi.pexcard.com to sandbox-coreapi.pexcard.com to use the correct sandbox environment.
@mdunnpexcard mdunnpexcard requested a review from tybaker December 18, 2025 17:58
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.

2 participants