Conversation
…tomers and payment methods
…andling and logging
…ing, and optional deletion
…ing and security best practices
… payment method management, and webhook configuration
…method setup and verification
…TTP client methods
0xrafasec
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b976f0b035
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export type Transaction = Request & { | ||
| export interface Transaction { | ||
| provider: string; | ||
| source: Source; |
There was a problem hiding this comment.
Payment.Transaction now hard-codes source to the shared Source type, which only allows capture_method: "automatic" and drops provider-specific fields that were previously carried through by Request & .... If the API returns valid responses for flows like PagarMe manual capture (or provider-specific response echoes), consumers will get false TypeScript errors and lose accurate typing on Payment.Response.
Useful? React with 👍 / 👎.
| const email = `test_${timestamp}@example.com`; | ||
| const response = await customers.create({ | ||
| email, | ||
| first_name: 'John', |
There was a problem hiding this comment.
to create stripe customer we need only email, if we want to create the test for pagarme change the title of the test. I suggest keep the old test for stripe as we prioritizing this for US clients
| const response = await customers.create({ | ||
| email, | ||
| country_code, | ||
| first_name: 'Jane', |
There was a problem hiding this comment.
to create stripe connected account we need only email and country code. I suggest keep the old test for stripe as we prioritizing this for US clients
| if (createResponse.ok) { | ||
| testCustomerId = createResponse.value.data.id as string; | ||
| // Fallback: find or create a test customer | ||
| const listResponse = await customers.list({ limit: 1 }); |
There was a problem hiding this comment.
I think storing paymentCustomerId in an environment variable is unnecessary since we can fetch the customer list directly from the provider.
Instead, we could apply appropriate filters to retrieve a more specific customer when needed. This would make the implementation cleaner and reduce dependency on environment-based configuration.
| const email = `pm_test_${Date.now()}@example.com`; | ||
| const createResponse = await customers.create({ | ||
| email, | ||
| first_name: 'Test', |
There was a problem hiding this comment.
this will create pagarme customer
| const documentNumber = `${timestamp}`.padStart(11, '0').substring(0, 11); | ||
|
|
||
| // Customer data | ||
| const customerData = { |
There was a problem hiding this comment.
this will create pagarme customer
fahmidareem3
left a comment
fahmidareem3
left a comment
There was a problem hiding this comment.
Thanks for all the great improvements! Just one thing to address per Tahseen's feedback: the customer test changes add document_type/document_number fields, which route to PagarMe instead of Stripe. Since we're currently focused on US clients, could we keep the original Stripe-only fields (email for customer, email + country_code for connected account)? Happy to discuss if there's a reason for the change.
| "should add a PIX payment method", | ||
| async () => { | ||
| if (!testCustomerId) { | ||
| console.warn("Skipping: testCustomerId not available"); |
There was a problem hiding this comment.
Consider using beforeAll with fail() or Jest's test.skip() conditionally instead of throwing in each test. Current approach causes all dependent tests to fail with the same error message.
|
Let's discuss together |
3 participants
This pull request introduces a range of improvements across CI/CD workflows, integration tests, documentation, and configuration. The most notable changes include stricter CI filtering, enhanced integration test reliability, the introduction of a detailed changelog, and updates to environment and engine requirements.
The PR was created to implement changes on issue #29 and improve the quality.
CI/CD Workflow Improvements:
@oaknetwork/contractsplaceholder package from CI build, test, and lint steps to prevent unnecessary processing and failures. [1] [2] [3]continue-on-errorfrom the lint step, making lint failures block PRs.10.9.2in the release workflow for deterministic builds.pnpmto>=10.0.0inpackage.json.Integration Test Reliability and Configuration:
PAYMENT_CUSTOMER_IDto.env-sampleand support for reading it in test config, allowing integration tests to use a stable customer for payment method tests. [1] [2] [3]PaymentMethodServiceto throw explicit errors instead of silently skipping tests when prerequisites are missing, improving test reporting and reliability. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]first_name,last_name,document_type,document_number). [1] [2]Documentation and Changelog:
CHANGELOG.mddocumenting all notable changes, breaking changes, new features, fixes, removals, and migration guidance.Other Notable Changes:
These changes collectively improve build reliability, test accuracy, developer experience, and documentation clarity.