[WIP] Stripe Payment #679
[WIP] Stripe Payment #679
Conversation
change 'nf-payment' to 'payment'
WalkthroughThis pull request introduces comprehensive Stripe payment integration across the application. The changes span both the API and client-side components, adding support for payment blocks in forms, OAuth connection with Stripe, and client-side payment processing. Key additions include environment configurations, OAuth drivers, payment block validation rules, localization support, and Vue components for handling payment interactions. Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🔭 Outside diff range comments (1)
api/app/Http/Controllers/Settings/OAuthProviderController.php (1)
Line range hint
39-53: Consider adding error handling for OAuth failures.The
handleRedirectmethod should include try-catch blocks to handle potential OAuth failures gracefully. Also consider validating token expiry and implementing refresh token logic.public function handleRedirect(OAuthProviderService $service) { + try { $driverUser = $service->getDriver()->getUser(); $provider = OAuthProvider::query() ->updateOrCreate( [ 'user_id' => Auth::id(), 'provider' => $service, 'provider_user_id' => $driverUser->getId(), ], [ 'access_token' => $driverUser->token, 'refresh_token' => $driverUser->refreshToken, 'name' => ($driverUser->getName()) ? $driverUser->getName() : $driverUser->getNickname(), 'email' => $driverUser->getEmail(), 'scopes' => $driverUser->approvedScopes ] ); return OAuthProviderResource::make($provider); + } catch (\Exception $e) { + \Log::error('OAuth redirect failed', [ + 'error' => $e->getMessage(), + 'provider' => $service + ]); + return response()->json([ + 'message' => 'Failed to connect with the service provider' + ], 500); + } }
🧹 Nitpick comments (21)
api/app/Rules/PaymentBlockConfigurationRule.php (3)
30-34: Fine-tune the single-payment-block constraint.
Currently, the rule forbids multiple payment blocks outright. If you foresee multi-payment-block features in the future, consider making this limit configurable or conditionally allowing more than one payment block.
48-51: Enforce consistent currency casing between validation and Stripe usage.
Here, the currency is validated in its uppercase form, while the controller sets it to lowercase when creating the PaymentIntent. This is fine if the config keys are uppercase. Otherwise, ensure both places use the same approach (either strictly uppercase or lowercase) to avoid confusion or misconfigurations.- if (!isset($this->field['currency']) || !in_array(strtoupper($this->field['currency']), array_keys(config('services.stripe.currencies')))) { + if (!isset($this->field['currency']) + || !in_array(strtolower($this->field['currency']), array_map('strtolower', array_keys(config('services.stripe.currencies'))))) { $fail('Currency must be a valid currency'); return; }
54-57: Validate and trim the Stripe account ID.
If there's a chance of whitespace or unexpected characters, consider trimming the string or applying a stricter validation pattern to prevent user errors.- if (!isset($this->field['stripe_account_id']) || empty($this->field['stripe_account_id'])) { + if (empty(trim($this->field['stripe_account_id'] ?? ''))) { $fail('Stripe account is required'); return; }api/app/Http/Controllers/Forms/FormPaymentController.php (2)
19-24: Return 403 for non-public forms if they exist.
Here, you’re returning a 404 response if the form’s workspace is null or the form is not public. Returning a 403 (Forbidden) might be more consistent with typical API design for existing but unauthorized resources.- return $this->error(['message' => 'Form not found.'], 404); + return $this->error(['message' => 'Form not accessible.'], 403);
51-64: Consider multi-account support or dynamic account keys.
Currently, you set a singleStripe::setApiKey(config('cashier.secret'));for all payments, while specifying'stripe_account'for the connected account. If each user can have a unique Stripe secret, you may need to retrieve credentials from the provider record instead.Do you want me to open a follow-up issue clarifying support for multiple Stripe accounts and credential management?
client/components/forms/PaymentInput.client.vue (3)
20-22: Provide additional success details.
When the payment succeeds, users may appreciate more detail than a generic success message, such as the paid amount and transaction ID.<p>{{ $t('forms.payment.success') }}</p> + <p> + {{ $t('forms.payment.you_paid') }} {{ currency }} {{ amount }}. + {{ $t('forms.payment.transaction_id') }}: {{ stripeState?.intentId }} + </p>
83-85: Remove or replace console logging in production.
Console logs can clutter production logs and potentially leak internal states. Consider switching to a more formal logging strategy or removing them entirely in production builds.- console.error('Stripe initialization failed') + // Use a formal logger or remove in production
130-140: Consider watching currency and amount changes.
You’re already resetting the card whendirectionorlocalechanges. Ifcurrencyoramountcan change dynamically, consider watching those too, so that the display or behavior remains in sync.client/components/open/forms/OpenForm.vue (2)
358-367: Consider consolidating thebusystate logic insubmitForm.
Lines 363 and 366 both setthis.dataForm.busy = false. Atry/finallyblock or unified approach could reduce duplication while preserving short-circuit returns upon failure.async submitForm() { if (!this.isAutoSubmit && this.formPageIndex !== this.fieldGroups.length - 1) return this.dataForm.busy = true try { if (!await this.doPayment()) { return } // ... further logic ... } finally { - this.dataForm.busy = false } }
525-538: Validate client-side errors early innextPageto avoid double processing.
Currently, the payment is processed first, followed by form validation. If validation fails frequently, an unnecessary payment attempt might be triggered. Consider validating form fields before initiating payment to prevent wasted payment calls.client/composables/useStripeElements.js (1)
32-34: Improve email validation.The current regex is too permissive and may accept invalid email addresses.
Consider using a more robust email validation:
- if(!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(state.value.cardHolderEmail)) { + if(!/^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/.test(state.value.cardHolderEmail)) {client/components/settings/ProviderModal.vue (1)
39-39: Consider disabling click events during loading state.The service cards remain clickable while loading, which could lead to multiple connection attempts.
- class="mr-2 bg-gray-50 border border-gray-200 rounded-md transition-colors p-4 pb-2 items-center justify-center w-[170px] h-[110px] flex flex-col relative" + class="mr-2 bg-gray-50 border border-gray-200 rounded-md transition-colors p-4 pb-2 items-center justify-center w-[170px] h-[110px] flex flex-col relative" + :class="{ + 'pointer-events-none': loading, + 'hover:bg-blue-50 group cursor-pointer': service.enabled && !loading, + 'cursor-not-allowed': !service.enabled || loading + }"client/components/open/forms/fields/components/PaymentFieldOptions.vue (2)
47-57: Improve help link accessibility and UX.The help link uses a hash (#) as href and relies solely on click handler. This isn't optimal for accessibility.
<a - target="#" + href="javascript:void(0)" + role="button" + aria-label="Learn about collecting payments" class="text-gray-500 cursor-pointer" @click.prevent="crisp.openHelpdesk()" >
78-83: Consider user's locale for default currency.The default currency is hardcoded to USD. Consider using the user's locale to set a more appropriate default currency.
+ const userLocale = navigator.language || 'en-US' + const userCurrency = new Intl.NumberFormat(userLocale, { style: 'currency', currency: 'USD' }).resolvedOptions().currency + if(props.field?.currency === undefined || props.field?.currency === null) { - props.field.currency = 'USD' + props.field.currency = userCurrency }api/config/services.php (1)
90-130: Consider moving currencies to a dedicated config file.The currencies list is quite extensive. Consider moving it to a dedicated configuration file (e.g.,
config/currencies.php) to improve maintainability and reusability.client/stores/working_form.js (1)
99-104: LGTM! Consider enhancing the error message.The validation logic correctly ensures only one payment block per form. Consider making the error message more informative by explaining why only one payment block is allowed.
- useAlert().error('Only one payment block is allowed per form') + useAlert().error('Only one payment block is allowed per form to maintain a single payment flow')api/app/Http/Requests/UserFormRequest.php (1)
74-74: Add PHPDoc to document the payment validation rule.The PaymentBlockConfigurationRule is a complex validation rule that deserves documentation explaining its purpose and requirements.
+ // Validates form block types and ensures payment blocks are properly configured 'properties.*.type' => ['required', new PaymentBlockConfigurationRule($this->properties)],client/i18n/lang/pa.json (1)
43-47: Consider adding more payment-related translations.While the basic payment fields are covered, consider adding translations for common payment-related messages such as:
- Payment errors/failures
- Card validation messages
- Processing status
- Currency formatting
Example additions for the English locale:
"payment": { "amount_to_pay": "Amount to pay", "success": "Payment successful", "name_on_card": "Name on card", "billing_email": "Billing email address", + "card_error": "Your card was declined", + "processing": "Processing payment...", + "invalid_card": "Invalid card details", + "currency_format": "${amount}", + "minimum_amount": "Minimum payment amount is ${amount}" }client/i18n/lang/bn.json (1)
44-45: Improve clarity of Bengali payment translations.Consider these improvements:
- "amount_to_pay": Change "প্রদত্ত পরিমাণ" to "প্রদেয় পরিমাণ" (payable amount)
- "success": Change "প্রদত্ত পরিমাণ প্রদান সফল" to "পেমেন্ট সফল" (payment successful)
client/data/blocks_types.json (1)
160-161: Consider using purple color scheme for consistency.The payment block currently uses green colors (bg-green-100/text-green-900), while other payment-related blocks like "number" use purple. Consider using "bg-purple-100" and "text-purple-900" for visual consistency with related blocks.
- "bg_class": "bg-green-100", - "text_class": "text-green-900", + "bg_class": "bg-purple-100", + "text_class": "text-purple-900",client/i18n/lang/en.json (1)
43-47: Consider standardizing the success message format.The translations look good, but consider revising "Payment successful" to "Payment was successful" to maintain consistency with typical transaction confirmation messages.
- "success": "Payment successful", + "success": "Payment was successful",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api/composer.lockis excluded by!**/*.lockclient/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (48)
api/.env.example(1 hunks)api/app/Http/Controllers/Content/FeatureFlagsController.php(1 hunks)api/app/Http/Controllers/Forms/FormPaymentController.php(1 hunks)api/app/Http/Controllers/Settings/OAuthProviderController.php(1 hunks)api/app/Http/Requests/UserFormRequest.php(2 hunks)api/app/Integrations/OAuth/Drivers/OAuthStripeDriver.php(1 hunks)api/app/Integrations/OAuth/OAuthProviderService.php(1 hunks)api/app/Providers/AppServiceProvider.php(2 hunks)api/app/Rules/PaymentBlockConfigurationRule.php(1 hunks)api/composer.json(2 hunks)api/config/services.php(1 hunks)api/routes/api.php(2 hunks)client/.env.example(1 hunks)client/components/forms/PaymentInput.client.vue(1 hunks)client/components/open/forms/OpenCompleteForm.vue(2 hunks)client/components/open/forms/OpenForm.vue(3 hunks)client/components/open/forms/OpenFormField.vue(3 hunks)client/components/open/forms/fields/components/FieldOptions.vue(4 hunks)client/components/open/forms/fields/components/PaymentFieldOptions.vue(1 hunks)client/components/open/tables/OpenTable.vue(2 hunks)client/components/open/tables/components/OpenPayment.vue(1 hunks)client/components/settings/ProviderModal.vue(2 hunks)client/composables/useStripeElements.js(1 hunks)client/data/blocks_types.json(1 hunks)client/i18n/lang/ar.json(1 hunks)client/i18n/lang/bn.json(1 hunks)client/i18n/lang/de.json(1 hunks)client/i18n/lang/en.json(1 hunks)client/i18n/lang/es.json(1 hunks)client/i18n/lang/fr.json(1 hunks)client/i18n/lang/hi.json(1 hunks)client/i18n/lang/ja.json(1 hunks)client/i18n/lang/jv.json(1 hunks)client/i18n/lang/ko.json(1 hunks)client/i18n/lang/mr.json(1 hunks)client/i18n/lang/pa.json(1 hunks)client/i18n/lang/pt.json(1 hunks)client/i18n/lang/ru.json(1 hunks)client/i18n/lang/ta.json(1 hunks)client/i18n/lang/te.json(1 hunks)client/i18n/lang/tr.json(1 hunks)client/i18n/lang/ur.json(1 hunks)client/i18n/lang/vi.json(1 hunks)client/i18n/lang/zh.json(1 hunks)client/package.json(2 hunks)client/runtimeConfig.js(1 hunks)client/stores/oauth_providers.js(3 hunks)client/stores/working_form.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (34)
api/app/Rules/PaymentBlockConfigurationRule.php (1)
23-24: Consider validating the attribute structure more robustly.
You're extracting thefieldIndexfrom the second segment of$attributeassuming"properties.<index>.field". If any unexpected format is passed to the validator, you might get an undefined offset. Consider adding a defensive check around this extraction.api/app/Http/Controllers/Forms/FormPaymentController.php (1)
27-33: Ensure the presence of exactly one payment block is enforced upstream.
Because you're already validating payment blocks inPaymentBlockConfigurationRule, this additional check is logically consistent. However, if you later allow multiple payment blocks, you’ll need to adjust logic here. Keep the domain rules consistent across both the controller and the validation rule.api/app/Integrations/OAuth/OAuthProviderService.php (1)
7-7: Add robust testing and error handling for new provider.
The new Stripe case and driver look good from a structural standpoint. Ensure each branch is tested to confirm it correctly retrieves credentials and handles errors.Use a script to locate test coverage for
OAuthProviderServiceandOAuthStripeDriver:Also applies to: 12-12, 16-18
client/components/open/tables/components/OpenPayment.vue (1)
1-16: Verify the payment button props and conditional rendering.
The template conditionally renders the “View Payment” button based onvalue. Ensure thatvalueis consistently an object with the necessary data.client/runtimeConfig.js (1)
34-35: LGTM! Verify environment setup.The addition of
STRIPE_PUBLISHABLE_KEYfollows the established pattern and correctly uses the nullish coalescing operator.Run this script to verify the environment setup:
api/app/Http/Controllers/Content/FeatureFlagsController.php (1)
31-33: LGTM! Verify currencies configuration.The Stripe currencies feature flag is correctly integrated and follows the established pattern.
Run this script to verify the currencies configuration:
✅ Verification successful
Stripe currencies configuration is properly implemented ✓
The currencies configuration in
config/services.phpis correctly defined with a comprehensive list of supported currencies, and proper validation is implemented inPaymentBlockConfigurationRule.phpto ensure only valid currencies are accepted.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Stripe currencies configuration # Test: Check if currencies are defined in services config echo "Checking services.php for Stripe currencies configuration..." rg "currencies.*=>.*\[" config/services.php -A 10Length of output: 242
Script:
#!/bin/bash # Find all potential config files and search for Stripe configurations echo "Looking for config files..." fd -e php -e yaml -e yml -e json config/ echo -e "\nSearching for Stripe configuration patterns..." rg -l "stripe" --type php --type yaml --type json echo -e "\nSearching for currency configurations..." rg -i "currenc" --type php --type yaml --type jsonLength of output: 69654
Script:
#!/bin/bash # Check the Stripe currencies configuration and validation echo "Checking services.php content..." cat api/config/services.php echo -e "\nChecking currency validation..." cat api/app/Rules/PaymentBlockConfigurationRule.phpLength of output: 6863
client/composables/useStripeElements.js (1)
1-13: Add cleanup method for sensitive data.The singleton pattern retains sensitive payment data in memory.
[security]
Add a cleanup method:let state = null // singleton pattern export const useStripeElements = () => { if (!state) { state = ref({ isLoaded: false, stripe: null, elements: null, card: null, cardHolderName: '', cardHolderEmail: '', - intentId: null + intentId: null, + isProcessing: false }) } + + const cleanup = () => { + state.value.cardHolderName = ''; + state.value.cardHolderEmail = ''; + state.value.intentId = null; + if (state.value.card) { + state.value.card.clear(); + } + }api/config/services.php (1)
87-89: Verify Stripe environment variables are properly configured.Ensure these environment variables are documented in
.env.exampleand deployment guides.✅ Verification successful
✓ Stripe environment variables are properly documented
All required Stripe OAuth variables are documented in
api/.env.example, along with additional Stripe configuration variables for both production and test environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Stripe environment variables are documented grep -A 3 "STRIPE_" .env.exampleLength of output: 78
Script:
#!/bin/bash # Search for env example files fd -H "\.env" --type f # Search for environment setup documentation in common locations fd -e md -e txt | xargs grep -l -i "environment" || trueLength of output: 155
Script:
#!/bin/bash # Check API env example file for Stripe variables cat api/.env.example | grep -i "stripe"Length of output: 395
client/components/open/forms/OpenCompleteForm.vue (2)
236-237: LGTM: Stripe state initializationThe Stripe state is properly initialized using the
useStripeElementscomposable in the setup method.
293-293: LGTM: Async form submissionThe
submitFormmethod has been properly converted to async to support payment processing while maintaining existing functionality.client/components/open/forms/OpenFormField.vue (1)
Line range hint
211-229: LGTM: Payment field component mappingThe payment field type is properly mapped to the PaymentInput component, maintaining consistency with other field mappings.
api/routes/api.php (1)
25-25: LGTM: Payment controller importThe FormPaymentController import is properly placed with other form-related controllers.
client/components/open/forms/fields/components/FieldOptions.vue (4)
197-200: LGTM: Payment field options componentThe PaymentFieldOptions component is properly integrated into the template with correct prop passing.
612-612: LGTM: Payment options importThe PaymentFieldOptions import is properly placed with other component imports.
620-620: LGTM: Component registrationPaymentFieldOptions is properly registered in the components object.
636-636: LGTM: Payment field type configurationPayment field type is correctly added to typesWithoutPlaceholder array, maintaining consistency with other special field types.
api/.env.example (1)
98-100: Consider grouping Stripe-related environment variables.The file already contains other Stripe-related variables (STRIPE_KEY, STRIPE_SECRET). Consider grouping all Stripe configurations together for better maintainability.
Apply this diff to reorganize:
STRIPE_KEY= STRIPE_SECRET= +STRIPE_CLIENT_ID= +STRIPE_CLIENT_SECRET= +STRIPE_REDIRECT_URI=http://localhost:3000/settings/connections/callback/stripe STRIPE_PROD_DEFAULT_PRODUCT_ID=Additionally, ensure that:
- The production environment uses HTTPS for the redirect URI
- The OAuth scope is properly configured in the Stripe dashboard
✅ Verification successful
Group Stripe-related environment variables for better organization
The suggestion to group Stripe configurations is valid. The current scattered layout could be improved by consolidating all Stripe-related variables. The proposed diff is correct.
Regarding the security considerations:
- The HTTP protocol in the redirect URI is acceptable for the example file as it's only used for local development
- Production URLs should be configured with HTTPS in the actual environment variables, not in the example file
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check the complete .env.example file echo "=== Checking .env.example content ===" cat api/.env.example # Look for production environment files echo -e "\n=== Searching for production environment files ===" fd -t f -e example -e prod -e production env # Search for Stripe OAuth integration code echo -e "\n=== Searching for Stripe OAuth integration code ===" rg -l "stripe.*oauth|oauth.*stripe" -g "!{*.lock,*.md}"Length of output: 2298
Script:
#!/bin/bash # Check client environment example echo "=== Checking client/.env.example content ===" cat client/.env.exampleLength of output: 405
client/i18n/lang/zh.json (1)
43-47: LGTM! Chinese translations are accurate and consistent.The payment-related translations are grammatically correct and use standard e-commerce terminology in Chinese.
client/i18n/lang/ko.json (1)
43-47: LGTM! Korean translations are accurate and consistent.The payment-related translations are grammatically correct and use standard e-commerce terminology in Korean.
client/i18n/lang/ja.json (1)
43-47: LGTM! Japanese translations are accurate and consistent.The payment-related translations are grammatically correct and use standard e-commerce terminology in Japanese.
api/app/Http/Controllers/Settings/OAuthProviderController.php (1)
50-50: LGTM! Good fallback mechanism.The fallback to
getNickname()whengetName()is falsy is a good practice, especially when dealing with OAuth providers that might not always return a name.client/i18n/lang/hi.json (1)
43-47: LGTM! Hindi translations are accurate.The payment-related translations are semantically correct and maintain consistency with the English terms.
client/i18n/lang/tr.json (1)
43-47: LGTM! Turkish translations are accurate.The payment-related translations are semantically correct and maintain consistency with the English terms.
client/i18n/lang/mr.json (1)
43-47: LGTM! Marathi translations are accurate.The payment-related translations are semantically correct and maintain consistency with the English terms.
client/i18n/lang/ru.json (1)
43-47: LGTM: Russian translations are accurateThe payment-related translations are grammatically correct and contextually appropriate for Russian users.
client/i18n/lang/pt.json (1)
43-47: LGTM: Portuguese translations are accurateThe payment-related translations are grammatically correct and contextually appropriate for Portuguese users.
client/i18n/lang/es.json (1)
43-47: LGTM: Spanish translations are accurateThe payment-related translations are grammatically correct and contextually appropriate for Spanish users.
client/package.json (1)
45-45: Consider updating Stripe-related package versions.The current package versions might have compatibility or security issues:
@stripe/[email protected]should be updated to the latest v2.x series[email protected]is relatively old and might need updatingAlso applies to: 79-79
client/i18n/lang/ta.json (1)
43-47: LGTM! Payment translations are complete and well-structured.The Tamil translations for payment-related terms are accurate and consistent with payment processing terminology.
client/i18n/lang/de.json (1)
43-47: LGTM! German payment translations are accurate.The translations maintain consistency with standard German payment terminology.
client/i18n/lang/fr.json (1)
43-47: LGTM! French payment translations are properly formatted.The translations accurately reflect standard French e-commerce terminology.
client/data/blocks_types.json (1)
155-163: LGTM! The payment block configuration follows the established structure.The configuration is well-structured and includes all necessary fields with appropriate values.
client/i18n/lang/ar.json (1)
43-47: LGTM! Arabic translations are well-structured.The payment-related translations are properly formatted with correct RTL text and maintain consistent JSON structure.
client/i18n/lang/vi.json (1)
43-47: LGTM! Vietnamese translations are well-structured.The payment-related translations are properly formatted with correct diacritical marks and maintain consistent JSON structure.
| 'amount' => (int) ($paymentBlock['amount'] * 100), // Stripe requires amount in cents | ||
| 'currency' => strtolower($paymentBlock['currency']), | ||
| 'payment_method_types' => ['card'], | ||
| 'metadata' => [ | ||
| 'form_id' => $form->id, | ||
| 'workspace_id' => $form->workspace_id, | ||
| 'form_name' => $form->title, | ||
| ], | ||
| ], [ | ||
| 'stripe_account' => $provider->provider_user_id | ||
| ]); | ||
|
|
||
| Log::info('Payment intent created', [ | ||
| 'form_id' => $form->id, | ||
| 'intent' => $intent | ||
| ]); | ||
|
|
||
| if ($intent->id) { | ||
| return $this->success([ | ||
| 'intent' => ['id' => $intent->id, 'secret' => $intent->client_secret] | ||
| ]); | ||
| } else { | ||
| return $this->error(['message' => 'Failed to create payment intent']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use round or int to handle fractional amounts.
Casting using (int) floors the amount. If a user enters $10.99, it will become 1099 which is correct, but $10.999 becomes 1099 instead of 1100. Decide whether to round or floor intentionally.
- 'amount' => (int) ($paymentBlock['amount'] * 100),
+ 'amount' => (int) round($paymentBlock['amount'] * 100),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'amount' => (int) ($paymentBlock['amount'] * 100), // Stripe requires amount in cents | |
| 'currency' => strtolower($paymentBlock['currency']), | |
| 'payment_method_types' => ['card'], | |
| 'metadata' => [ | |
| 'form_id' => $form->id, | |
| 'workspace_id' => $form->workspace_id, | |
| 'form_name' => $form->title, | |
| ], | |
| ], [ | |
| 'stripe_account' => $provider->provider_user_id | |
| ]); | |
| Log::info('Payment intent created', [ | |
| 'form_id' => $form->id, | |
| 'intent' => $intent | |
| ]); | |
| if ($intent->id) { | |
| return $this->success([ | |
| 'intent' => ['id' => $intent->id, 'secret' => $intent->client_secret] | |
| ]); | |
| } else { | |
| return $this->error(['message' => 'Failed to create payment intent']); | |
| } | |
| 'amount' => (int) round($paymentBlock['amount'] * 100), // Stripe requires amount in cents | |
| 'currency' => strtolower($paymentBlock['currency']), | |
| 'payment_method_types' => ['card'], | |
| 'metadata' => [ | |
| 'form_id' => $form->id, | |
| 'workspace_id' => $form->workspace_id, | |
| 'form_name' => $form->title, | |
| ], | |
| ], [ | |
| 'stripe_account' => $provider->provider_user_id | |
| ]); | |
| Log::info('Payment intent created', [ | |
| 'form_id' => $form->id, | |
| 'intent' => $intent | |
| ]); | |
| if ($intent->id) { | |
| return $this->success([ | |
| 'intent' => ['id' => $intent->id, 'secret' => $intent->client_secret] | |
| ]); | |
| } else { | |
| return $this->error(['message' => 'Failed to create payment intent']); | |
| } |
| async doPayment() { | ||
| // If there is a payment block, process the payment | ||
| const { state: stripeState, processPayment } = useStripeElements() | ||
| const paymentBlock = this.currentFields.find(field => field.type === 'payment') | ||
| if (paymentBlock && !stripeState.value.intentId && (paymentBlock.required || !stripeState.value.card._empty)) { | ||
| try { | ||
| // Process the payment | ||
| const result = await processPayment(this.form.slug, paymentBlock.required) | ||
| console.log('result', result) | ||
| if (result && result?.error) { | ||
| this.dataForm.errors.set(paymentBlock.id, result.error.message) | ||
| useAlert().error(result.error.message) | ||
| return false | ||
| } | ||
|
|
||
| if (result?.paymentIntent?.status === 'succeeded') { | ||
| stripeState.value.intentId = result.paymentIntent.id | ||
| useAlert().success('Thank you! Your payment is successful.') | ||
| return true | ||
| } | ||
| useAlert().error('Something went wrong. Please try again.') | ||
| } catch (error) { | ||
| console.error(error) | ||
| useAlert().error(error?.message || 'Payment failed') | ||
| } | ||
| return false | ||
| } | ||
| return true | ||
| }, |
There was a problem hiding this comment.
Avoid logging sensitive payment data to the console.
The console statement at line 555 may leak payment details or error responses. Remove or mask sensitive information to protect user privacy and security.
- console.log('result', result)
+ // console.log('result', result) // Remove or mask sensitive data in production📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async doPayment() { | |
| // If there is a payment block, process the payment | |
| const { state: stripeState, processPayment } = useStripeElements() | |
| const paymentBlock = this.currentFields.find(field => field.type === 'payment') | |
| if (paymentBlock && !stripeState.value.intentId && (paymentBlock.required || !stripeState.value.card._empty)) { | |
| try { | |
| // Process the payment | |
| const result = await processPayment(this.form.slug, paymentBlock.required) | |
| console.log('result', result) | |
| if (result && result?.error) { | |
| this.dataForm.errors.set(paymentBlock.id, result.error.message) | |
| useAlert().error(result.error.message) | |
| return false | |
| } | |
| if (result?.paymentIntent?.status === 'succeeded') { | |
| stripeState.value.intentId = result.paymentIntent.id | |
| useAlert().success('Thank you! Your payment is successful.') | |
| return true | |
| } | |
| useAlert().error('Something went wrong. Please try again.') | |
| } catch (error) { | |
| console.error(error) | |
| useAlert().error(error?.message || 'Payment failed') | |
| } | |
| return false | |
| } | |
| return true | |
| }, | |
| async doPayment() { | |
| // If there is a payment block, process the payment | |
| const { state: stripeState, processPayment } = useStripeElements() | |
| const paymentBlock = this.currentFields.find(field => field.type === 'payment') | |
| if (paymentBlock && !stripeState.value.intentId && (paymentBlock.required || !stripeState.value.card._empty)) { | |
| try { | |
| // Process the payment | |
| const result = await processPayment(this.form.slug, paymentBlock.required) | |
| // console.log('result', result) // Remove or mask sensitive data in production | |
| if (result && result?.error) { | |
| this.dataForm.errors.set(paymentBlock.id, result.error.message) | |
| useAlert().error(result.error.message) | |
| return false | |
| } | |
| if (result?.paymentIntent?.status === 'succeeded') { | |
| stripeState.value.intentId = result.paymentIntent.id | |
| useAlert().success('Thank you! Your payment is successful.') | |
| return true | |
| } | |
| useAlert().error('Something went wrong. Please try again.') | |
| } catch (error) { | |
| console.error(error) | |
| useAlert().error(error?.message || 'Payment failed') | |
| } | |
| return false | |
| } | |
| return true | |
| }, |
| <script> | ||
| export default { | ||
| components: { }, | ||
| props: { | ||
| value: { | ||
| type: Object, | ||
| }, | ||
| }, | ||
|
|
||
| data() { | ||
| return {} | ||
| }, | ||
|
|
||
| computed: { | ||
| paymentUrl() { | ||
| if (!this.value) return null | ||
| const isLocal = useRuntimeConfig().public.env === 'local' | ||
| return `https://dashboard.stripe.com${isLocal ? '/test' : ''}/payments/${this.value}` | ||
| }, | ||
| }, | ||
| } | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Safeguard paymentUrl construction.
Currently, the string interpolation uses this.value directly in the URL. If this.value is not a string or lacks an ID, it might produce an invalid URL. Validate or convert this.value to a string or use this.value.id.
- return `https://dashboard.stripe.com${isLocal ? '/test' : ''}/payments/${this.value}`
+ const paymentId = typeof this.value === 'object' && this.value.id ? this.value.id : this.value
+ return `https://dashboard.stripe.com${isLocal ? '/test' : ''}/payments/${paymentId}`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <script> | |
| export default { | |
| components: { }, | |
| props: { | |
| value: { | |
| type: Object, | |
| }, | |
| }, | |
| data() { | |
| return {} | |
| }, | |
| computed: { | |
| paymentUrl() { | |
| if (!this.value) return null | |
| const isLocal = useRuntimeConfig().public.env === 'local' | |
| return `https://dashboard.stripe.com${isLocal ? '/test' : ''}/payments/${this.value}` | |
| }, | |
| }, | |
| } | |
| </script> | |
| <script> | |
| export default { | |
| components: { }, | |
| props: { | |
| value: { | |
| type: Object, | |
| }, | |
| }, | |
| data() { | |
| return {} | |
| }, | |
| computed: { | |
| paymentUrl() { | |
| if (!this.value) return null | |
| const isLocal = useRuntimeConfig().public.env === 'local' | |
| const paymentId = typeof this.value === 'object' && this.value.id ? this.value.id : this.value | |
| return `https://dashboard.stripe.com${isLocal ? '/test' : ''}/payments/${paymentId}` | |
| }, | |
| }, | |
| } | |
| </script> |
|
|
||
| Event::listen(function (\SocialiteProviders\Manager\SocialiteWasCalled $event) { | ||
| $event->extendSocialite('stripe', \SocialiteProviders\Stripe\Provider::class); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Missing Stripe OAuth Environment Variables Documentation
The Stripe OAuth configuration is properly structured in services.php, but the required environment variables are not documented in any .env.example file. This could cause deployment issues.
Please add these required variables to api/.env.example:
STRIPE_CLIENT_IDSTRIPE_CLIENT_SECRETSTRIPE_REDIRECT_URI
🔗 Analysis chain
LGTM! Verify Stripe OAuth configuration.
The Stripe OAuth provider is correctly integrated using Laravel Socialite's event listener pattern.
Run this script to verify the OAuth configuration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Stripe OAuth configuration
# Test: Check if required OAuth config exists in services.php
echo "Checking services.php for Stripe OAuth configuration..."
rg "stripe.*=>.*\[" config/services.php -A 5
# Test: Check if required environment variables are documented
echo "Checking .env.example for Stripe OAuth variables..."
rg "STRIPE.*(CLIENT_ID|CLIENT_SECRET|REDIRECT)" .env.example
Length of output: 456
Script:
#!/bin/bash
# Search for config files and env examples
echo "Searching for config files..."
fd -t f "services" -E "node_modules"
echo -e "\nSearching for env files..."
fd -t f ".env" -E "node_modules"
echo -e "\nSearching for Stripe-related configuration in any file..."
rg -l "STRIPE" -t php -t env
Length of output: 475
Script:
#!/bin/bash
echo "Checking services.php content..."
cat api/config/services.php
echo -e "\nChecking environment files for Stripe configuration..."
echo "=== client/.env.example ==="
cat client/.env.example
echo -e "\n=== client/.env.docker ==="
cat client/.env.docker
echo -e "\n=== scripts/setup-env.sh ==="
cat scripts/setup-env.sh
Length of output: 7910
| return { | ||
| state, | ||
| processPayment | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Export cleanup method.
Make the cleanup method available to consumers of this composable.
return {
state,
- processPayment
+ processPayment,
+ cleanup
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| state, | |
| processPayment | |
| } | |
| } | |
| return { | |
| state, | |
| processPayment, | |
| cleanup | |
| } | |
| } |
| "amount_to_pay": "پہلو کرنے کی رقم", | ||
| "success": "پہلو کرنا موفق", |
There was a problem hiding this comment.
Translation corrections needed for payment terms in Urdu.
The translations for "amount_to_pay" and "success" appear to use incorrect terms:
- "پہلو" means "aspect/side" - should be "ادائیگی" (payment) instead
- Suggested corrections:
- "amount_to_pay": "ادائیگی کی رقم"
- "success": "ادائیگی کامیاب"
| }, | ||
| "payment": { | ||
| "amount_to_pay": "भुगतान करण्यासाठी रक्कम", | ||
| "success": "भुगतान सफल", | ||
| "name_on_card": "कार्डचे नाव", | ||
| "billing_email": "बिलिंग ईमेल पत्ता" |
There was a problem hiding this comment.
💡 Codebase verification
Translation corrections needed in multiple language files
Found specific translation issues that need attention:
ur.json: Incorrect translations for "payment" (پہلو) and "billing" (بلندی)bn.json: Redundant wording in success messagete.json: Incorrect term used for payment (పరివర్తనం)
🔗 Analysis chain
Verify translations in other language files.
While reviewing the payment translations across multiple languages, I noticed some improvements needed:
- Urdu translations need correction for payment-related terms
- Bengali translations could be clearer
Consider reviewing other language files (not provided in this PR) to ensure consistent translation quality across all supported languages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# List all language files to ensure complete coverage
fd -e json . client/i18n/lang/
# Check if all language files have the payment section
rg -l '"payment":\s*{' client/i18n/lang/
Length of output: 667
Script:
#!/bin/bash
# Extract payment sections from all language files
for file in client/i18n/lang/*.json; do
echo "=== $file ==="
# Extract content between "payment": { and the next closing brace
rg -U '"payment":\s*\{[^}]+\}' "$file"
echo
done
Length of output: 7975
| "payment": { | ||
| "amount_to_pay": "Jumlah yang harus dibayar", | ||
| "success": "Pembayaran berhasil", | ||
| "name_on_card": "Nama di kartu", | ||
| "billing_email": "Alamat email penagihan" |
There was a problem hiding this comment.
Critical: Translations are in Indonesian instead of Javanese
The payment section translations are currently in Indonesian (Bahasa Indonesia) instead of Javanese language:
- "Jumlah yang harus dibayar" is Indonesian
- "Pembayaran berhasil" is Indonesian
- "Nama di kartu" is Indonesian
- "Alamat email penagihan" is Indonesian
These should be translated to Javanese for consistency with the rest of the file.
| @@ -39,6 +39,12 @@ | |||
| }, | |||
| "barcodeInput": { | |||
| "clickToOpenCamera": "క్లిక్ చేయండి కెమెరా తెరవండి" | |||
| }, | |||
| "payment": { | |||
| "amount_to_pay": "పరివర్తనం చేయడానికి రోలు", | |||
There was a problem hiding this comment.
Critical: Incorrect Telugu translation for "amount_to_pay"
The current translation "పరివర్తనం చేయడానికి రోలు" means "role for transformation" which is incorrect. It should be "చెల్లించవలసిన మొత్తం" which means "amount to pay" in Telugu.
| @@ -16,6 +16,7 @@ | |||
| "ext-json": "*", | |||
| "aws/aws-sdk-php": "*", | |||
| "doctrine/dbal": "*", | |||
| "fakerphp/faker": "^1.23", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up duplicate package entries and standardize version constraints.
-
Remove duplicate entries:
fakerphp/fakerappears twicespatie/laravel-rayappears twice
-
Consider using consistent version constraints:
- Some packages use
*while others use specific versions - This mix could lead to unexpected behavior during updates
- Some packages use
Apply this diff to clean up the dependencies:
- "fakerphp/faker": "^1.23",
"giggsey/libphonenumber-for-php": "*",
"google/apiclient": "^2.16",
"guzzlehttp/guzzle": "*",
"laravel/cashier": "*",
"laravel/framework": "^11.9",
"laravel/horizon": "*",
"laravel/sanctum": "^4.0",
"laravel/socialite": "*",
"laravel/tinker": "^2.9",
"laravel/ui": "*",
"laravel/vapor-cli": "*",
"laravel/vapor-core": "*",
"league/flysystem-aws-s3-v3": "*",
"maatwebsite/excel": "^3.1",
"openai-php/client": "*",
"propaganistas/laravel-disposable-email": "*",
"sentry/sentry-laravel": "*",
"socialiteproviders/stripe": "^4.1",
"spatie/laravel-data": "^4.6",
- "spatie/laravel-ray": "*",Also applies to: 37-37, 39-39
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
client/composables/useStripeElements.js (3)
21-23:⚠️ Potential issueAvoid accessing private Stripe Element properties.
The
_emptyproperty is internal to Stripe Elements and may change in future versions.
37-57:⚠️ Potential issueAdd error handling and loading state.
The payment processing lacks proper error handling for network failures and loading state management.
59-63: 🛠️ Refactor suggestionExport cleanup method.
Make the cleanup method available to consumers of this composable.
api/app/Http/Controllers/Forms/FormPaymentController.php (1)
79-79:⚠️ Potential issueUse round or int to handle fractional amounts.
Casting using
(int)floors the amount. If a user enters$10.99, it will become1099which is correct, but$10.999becomes1099instead of1100.
🧹 Nitpick comments (4)
client/composables/useStripeElements.js (2)
1-2: Consider using Symbol for singleton implementation.The current singleton implementation using
nullcould potentially be reset. Using Symbol would make it truly private.-let state = null // singleton pattern +const STATE_SYMBOL = Symbol('StripeElementsState'); +let state = { + [STATE_SYMBOL]: null +};
25-35: Improve validation logic structure.The nested validation checks could be simplified and made more maintainable.
- if(isRequired || !state.value.card._empty){ - if(!state.value.cardHolderName) { - return { error: { message: 'Card holder name is required' } } - } - if(!state.value.cardHolderEmail) { - return { error: { message: 'Billing email address is required' } } - } - if(!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(state.value.cardHolderEmail)) { - return { error: { message: 'Invalid billing email address' } } - } - } + const shouldValidate = isRequired || !state.value.card._empty; + if (shouldValidate) { + const validations = [ + { condition: !state.value.cardHolderName, message: 'Card holder name is required' }, + { condition: !state.value.cardHolderEmail, message: 'Billing email address is required' }, + { condition: !/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(state.value.cardHolderEmail), + message: 'Invalid billing email address' } + ]; + + const failedValidation = validations.find(v => v.condition); + if (failedValidation) { + return { error: { message: failedValidation.message } }; + } + }api/app/Http/Controllers/Forms/FormPaymentController.php (1)
82-86: Enhance payment intent metadata.Consider adding more metadata to help with payment reconciliation and tracking.
'metadata' => [ 'form_id' => $form->id, 'workspace_id' => $form->workspace_id, 'form_name' => $form->title, + 'created_at' => now()->toIso8601String(), + 'environment' => config('app.env'), ],client/components/forms/PaymentInput.client.vue (1)
108-110: Remove unused parameter.The
elementparameter is defined but never used.-const onCardReady = (element) => { +const onCardReady = () => { stripeState.value.card = card.value?.stripeElement }🧰 Tools
🪛 GitHub Check: Run client linters
[warning] 108-108:
'element' is defined but never used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/app/Http/Controllers/Forms/FormPaymentController.php(1 hunks)api/routes/api.php(2 hunks)client/components/forms/PaymentInput.client.vue(1 hunks)client/composables/useStripeElements.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/routes/api.php
🧰 Additional context used
🪛 GitHub Check: Run client linters
client/components/forms/PaymentInput.client.vue
[warning] 108-108:
'element' is defined but never used
[warning] 128-128:
'newValue' is defined but never used
[warning] 131-131:
'newValue' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (1)
api/app/Http/Controllers/Forms/FormPaymentController.php (1)
76-76: Use environment-specific Stripe key.Using
cashier.secretmight not be the intended configuration for this integration.
| } catch (\Stripe\Exception\CardException $e) { | ||
| Log::warning('Failed to create payment intent', [ | ||
| 'form_id' => $form->id, | ||
| 'message' => $e->getMessage() | ||
| ]); | ||
| return $this->error(['message' => $e->getMessage()]); | ||
| } catch (\Exception $e) { | ||
| Log::error('Failed to create payment intent', [ | ||
| 'form_id' => $form->id, | ||
| 'error' => $e->getMessage() | ||
| ]); | ||
| return $this->error(['message' => 'Failed to initialize payment.']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling granularity.
The catch blocks could be more specific to handle different Stripe exceptions differently.
- } catch (\Stripe\Exception\CardException $e) {
+ } catch (\Stripe\Exception\CardException $e) {
Log::warning('Failed to create payment intent', [
'form_id' => $form->id,
'message' => $e->getMessage()
]);
return $this->error(['message' => $e->getMessage()]);
+ } catch (\Stripe\Exception\InvalidRequestException $e) {
+ Log::error('Invalid Stripe request', [
+ 'form_id' => $form->id,
+ 'error' => $e->getMessage()
+ ]);
+ return $this->error(['message' => 'Invalid payment configuration.']);
+ } catch (\Stripe\Exception\AuthenticationException $e) {
+ Log::critical('Stripe authentication failed', [
+ 'form_id' => $form->id,
+ 'error' => $e->getMessage()
+ ]);
+ return $this->error(['message' => 'Payment service configuration error.']);
} catch (\Exception $e) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (\Stripe\Exception\CardException $e) { | |
| Log::warning('Failed to create payment intent', [ | |
| 'form_id' => $form->id, | |
| 'message' => $e->getMessage() | |
| ]); | |
| return $this->error(['message' => $e->getMessage()]); | |
| } catch (\Exception $e) { | |
| Log::error('Failed to create payment intent', [ | |
| 'form_id' => $form->id, | |
| 'error' => $e->getMessage() | |
| ]); | |
| return $this->error(['message' => 'Failed to initialize payment.']); | |
| } | |
| } catch (\Stripe\Exception\CardException $e) { | |
| Log::warning('Failed to create payment intent', [ | |
| 'form_id' => $form->id, | |
| 'message' => $e->getMessage() | |
| ]); | |
| return $this->error(['message' => $e->getMessage()]); | |
| } catch (\Stripe\Exception\InvalidRequestException $e) { | |
| Log::error('Invalid Stripe request', [ | |
| 'form_id' => $form->id, | |
| 'error' => $e->getMessage() | |
| ]); | |
| return $this->error(['message' => 'Invalid payment configuration.']); | |
| } catch (\Stripe\Exception\AuthenticationException $e) { | |
| Log::critical('Stripe authentication failed', [ | |
| 'form_id' => $form->id, | |
| 'error' => $e->getMessage() | |
| ]); | |
| return $this->error(['message' => 'Payment service configuration error.']); | |
| } catch (\Exception $e) { | |
| Log::error('Failed to create payment intent', [ | |
| 'form_id' => $form->id, | |
| 'error' => $e->getMessage() | |
| ]); | |
| return $this->error(['message' => 'Failed to initialize payment.']); | |
| } |
| const onCardChanged = (event) => { | ||
| console.log('card changed', event) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance card change event handling.
The console.log statement should be removed or replaced with proper error handling.
const onCardChanged = (event) => {
- console.log('card changed', event)
+ if (event.error) {
+ useAlert().error(event.error.message)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onCardChanged = (event) => { | |
| console.log('card changed', event) | |
| } | |
| const onCardChanged = (event) => { | |
| if (event.error) { | |
| useAlert().error(event.error.message) | |
| } | |
| } |
| } catch (error) { | ||
| console.error('Stripe initialization error:', error) | ||
| stripeState.value.isLoaded = false | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in initStripe.
The catch block should provide more specific error messages and handle different error types appropriately.
} catch (error) {
- console.error('Stripe initialization error:', error)
+ const errorMessage = error.response?.data?.message || 'Failed to initialize payment system'
+ useAlert().error(errorMessage)
+ console.error('Stripe initialization error:', error)
stripeState.value.isLoaded = false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.error('Stripe initialization error:', error) | |
| stripeState.value.isLoaded = false | |
| } | |
| } | |
| } catch (error) { | |
| const errorMessage = error.response?.data?.message || 'Failed to initialize payment system' | |
| useAlert().error(errorMessage) | |
| console.error('Stripe initialization error:', error) | |
| stripeState.value.isLoaded = false | |
| } | |
| } |
| const resetCard = async () => { | ||
| if (card.value?.stripeElement) { | ||
| card.value.stripeElement.unmount() | ||
| await nextTick() | ||
| card.value.stripeElement.mount(card.value.$el) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to resetCard method.
The card reset operation should handle potential errors during unmount/mount operations.
const resetCard = async () => {
if (card.value?.stripeElement) {
- card.value.stripeElement.unmount()
- await nextTick()
- card.value.stripeElement.mount(card.value.$el)
+ try {
+ card.value.stripeElement.unmount()
+ await nextTick()
+ await card.value.stripeElement.mount(card.value.$el)
+ } catch (error) {
+ console.error('Failed to reset card element:', error)
+ useAlert().error('Failed to reset payment form')
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resetCard = async () => { | |
| if (card.value?.stripeElement) { | |
| card.value.stripeElement.unmount() | |
| await nextTick() | |
| card.value.stripeElement.mount(card.value.$el) | |
| } | |
| } | |
| const resetCard = async () => { | |
| if (card.value?.stripeElement) { | |
| try { | |
| card.value.stripeElement.unmount() | |
| await nextTick() | |
| await card.value.stripeElement.mount(card.value.$el) | |
| } catch (error) { | |
| console.error('Failed to reset card element:', error) | |
| useAlert().error('Failed to reset payment form') | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
api/app/Policies/OAuthProviderPolicy.php (2)
68-74: Consider optimizing the database query for better performance.The current implementation loads all forms and then filters them in memory. For better performance, especially with large datasets, consider moving the filtering logic to the database query.
Here's an optimized version using a database query:
- $formsUsingStripe = $user->forms() - ->get() - ->filter(function ($form) use ($provider) { - return collect($form->properties) - ->some(fn ($prop) => ($prop['stripe_account_id'] ?? null) === $provider->id); - }) - ->isNotEmpty(); + $formsUsingStripe = $user->forms() + ->whereRaw("JSON_SEARCH(properties, 'one', ?, null, '$.*.stripe_account_id') IS NOT NULL", [$provider->id]) + ->exists();
67-78: Improve code readability by extracting the Stripe validation logic.The nested conditional and collection operations make the code harder to follow. Consider extracting this logic into a dedicated method for better readability and maintainability.
Here's a suggested refactor:
+ private function isStripeAccountInUse(User $user, OAuthProvider $provider): bool + { + return $user->forms() + ->whereRaw("JSON_SEARCH(properties, 'one', ?, null, '$.*.stripe_account_id') IS NOT NULL", [$provider->id]) + ->exists(); + } + public function delete(User $user, OAuthProvider $provider) { $integrations = FormIntegration::where('oauth_id', $provider->id)->get(); if ($integrations->count() > 0) { return $this->denyWithStatus(400, 'This connection cannot be removed because there is already an integration using it.'); } if ($provider->provider->value === OAuthProviderService::Stripe->value) { - $formsUsingStripe = $user->forms() - ->get() - ->filter(function ($form) use ($provider) { - return collect($form->properties) - ->some(fn ($prop) => ($prop['stripe_account_id'] ?? null) === $provider->id); - }) - ->isNotEmpty(); - if ($formsUsingStripe) { + if ($this->isStripeAccountInUse($user, $provider)) { return $this->denyWithStatus(400, 'This Stripe connection cannot be removed because it is being used in a form payment field.'); } }api/app/Http/Controllers/Forms/FormPaymentController.php (1)
44-49: Consider moving form visibility check to middleware.The form visibility check could be moved to a middleware to maintain separation of concerns and reusability.
Create a new middleware class:
class CheckFormVisibility { public function handle($request, Closure $next) { $form = $request->form; if ($form->workspace === null || $form->visibility !== 'public') { Log::warning('Attempt to access invalid form', [ 'form_id' => $form->id ]); return response()->json(['message' => 'Form not found.'], 404); } return $next($request); } }client/components/forms/PaymentInput.client.vue (1)
112-114: Remove unused parameter in onCardReady.The
elementparameter is defined but never used.-const onCardReady = (element) => { +const onCardReady = () => { stripeState.value.card = card.value?.stripeElement }🧰 Tools
🪛 GitHub Check: Run client linters
[warning] 112-112:
'element' is defined but never usedclient/components/open/forms/fields/components/PaymentFieldOptions.vue (4)
40-49: Enhance button accessibility.Add an aria-label to the button to improve screen reader support.
<UButton class="mt-4" icon="i-heroicons-arrow-right" block trailing :loading="stripeLoading" + aria-label="Connect your Stripe account" @click.prevent="connectStripe" >
55-58: Add accessibility attributes to the information icon.The icon needs proper accessibility attributes to be more inclusive.
<Icon name="heroicons:information-circle-16-solid" class="h-4 w-4 mt-1" + role="img" + aria-label="Information" />
67-72: Enhance field prop validation.Consider adding more specific validation for the field prop to ensure it has the required properties.
const props = defineProps({ field: { type: Object, - required: true + required: true, + validator: (value) => { + return ['type', 'currency', 'amount'].every(prop => + prop in value || value[prop] === undefined + ) + } } })
78-87: Separate concerns in onMounted hook.Consider extracting the default value initialization into a separate function for better maintainability.
+const initializeDefaultValues = () => { + if(props.field?.currency === undefined || props.field?.currency === null) { + props.field.currency = 'USD' + } + if(props.field?.amount === undefined || props.field?.amount === null) { + props.field.amount = 10 + } +} + onMounted(() => { providersStore.fetchOAuthProviders() - - if(props.field?.currency === undefined || props.field?.currency === null) { - props.field.currency = 'USD' - } - if(props.field?.amount === undefined || props.field?.amount === null) { - props.field.amount = 10 - } + initializeDefaultValues() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/app/Http/Controllers/Forms/FormPaymentController.php(1 hunks)api/app/Policies/OAuthProviderPolicy.php(2 hunks)client/components/forms/PaymentInput.client.vue(1 hunks)client/components/open/forms/OpenFormField.vue(3 hunks)client/components/open/forms/fields/components/PaymentFieldOptions.vue(1 hunks)client/stores/working_form.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/stores/working_form.js
🧰 Additional context used
🪛 GitHub Check: Run client linters
client/components/forms/PaymentInput.client.vue
[warning] 112-112:
'element' is defined but never used
[warning] 132-132:
'newValue' is defined but never used
[warning] 135-135:
'newValue' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (9)
api/app/Policies/OAuthProviderPolicy.php (1)
5-5: LGTM! Import statement is correctly placed.The added import for
OAuthProviderServiceis necessary for the new Stripe provider checks and follows Laravel conventions.api/app/Http/Controllers/Forms/FormPaymentController.php (2)
80-80: Round amount to handle fractional values correctly.When converting to cents, use
round()to handle fractional amounts properly. For example, $10.999 should become 1100 cents, not 1099.- 'amount' => (int) ($paymentBlock['amount'] * 100), // Stripe requires amount in cents + 'amount' => (int) round($paymentBlock['amount'] * 100), // Stripe requires amount in cents
104-116: Enhance error handling for different Stripe exceptions.The catch blocks should handle different Stripe exceptions with appropriate error messages and logging levels.
} catch (\Stripe\Exception\CardException $e) { Log::warning('Failed to create payment intent', [ 'form_id' => $form->id, 'message' => $e->getMessage() ]); return $this->error(['message' => $e->getMessage()]); + } catch (\Stripe\Exception\InvalidRequestException $e) { + Log::error('Invalid Stripe request', [ + 'form_id' => $form->id, + 'error' => $e->getMessage() + ]); + return $this->error(['message' => 'Invalid payment configuration.']); + } catch (\Stripe\Exception\AuthenticationException $e) { + Log::critical('Stripe authentication failed', [ + 'form_id' => $form->id, + 'error' => $e->getMessage() + ]); + return $this->error(['message' => 'Payment service configuration error.']); } catch (\Exception $e) {client/components/forms/PaymentInput.client.vue (3)
108-110: Remove console.log and enhance error handling.The console.log statement should be replaced with proper error handling.
const onCardChanged = (event) => { - console.log('card changed', event) + if (event.error) { + useAlert().error(event.error.message) + } }
178-182: Improve error handling in Stripe initialization.The catch block should provide more specific error messages and handle different error types.
} catch (error) { - console.error('Stripe initialization error:', error) + const errorMessage = error.response?.data?.message || 'Failed to initialize payment system' + useAlert().error(errorMessage) + console.error('Stripe initialization error:', error) stripeState.value.isLoaded = false }
184-190: Add error handling to card reset method.The card reset operation should handle potential errors during unmount/mount operations.
const resetCard = async () => { if (card.value?.stripeElement) { - card.value.stripeElement.unmount() - await nextTick() - card.value.stripeElement.mount(card.value.$el) + try { + card.value.stripeElement.unmount() + await nextTick() + await card.value.stripeElement.mount(card.value.$el) + } catch (error) { + console.error('Failed to reset card element:', error) + useAlert().error('Failed to reset payment form') + } } }client/components/open/forms/OpenFormField.vue (1)
403-407: Add validation for payment field properties.The payment field properties lack validation for currency codes and amounts.
#!/bin/bash # Description: Check for currency and amount validation in the codebase # Search for payment field type handling echo "=== Payment Field Type Handling ===" rg "type === ['|\"]payment['|\"]" -A 5 # Search for currency validation echo -e "\n=== Currency Validation ===" rg "currency.*valid|validateCurrency" -A 3 # Search for amount validation echo -e "\n=== Amount Validation ===" rg "amount.*valid|validateAmount" -A 3client/components/open/forms/fields/components/PaymentFieldOptions.vue (2)
20-27: Add proper amount validation.The amount input needs additional validation to prevent negative or zero values.
84-86: Reconsider default amount value.The default amount of 10 might not be appropriate for all currencies due to different decimal places and minimum transaction amounts.
Run this script to check Stripe's minimum amounts per currency:
| <a | ||
| target="#" | ||
| class="text-gray-500 cursor-pointer" | ||
| @click.prevent="crisp.openHelpdeskArticle('how-to-collect-payment-svig30')" | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix invalid anchor target attribute.
The anchor tag has an invalid target="#" attribute. Since this is a click handler and not a navigation link, remove the target attribute.
<a
- target="#"
class="text-gray-500 cursor-pointer"
@click.prevent="crisp.openHelpdeskArticle('how-to-collect-payment-svig30')"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <a | |
| target="#" | |
| class="text-gray-500 cursor-pointer" | |
| @click.prevent="crisp.openHelpdeskArticle('how-to-collect-payment-svig30')" | |
| > | |
| <a | |
| class="text-gray-500 cursor-pointer" | |
| @click.prevent="crisp.openHelpdeskArticle('how-to-collect-payment-svig30')" | |
| > |
| const currencyList = computed(() => { | ||
| const currencies = useFeatureFlag('services.stripe.currencies') || {} | ||
| return Object.keys(currencies).map((item) => ({ | ||
| name: currencies[item], | ||
| value: item | ||
| })) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve currency list error handling.
The computed property should handle the case when the feature flag is undefined and provide a default currency list.
const currencyList = computed(() => {
- const currencies = useFeatureFlag('services.stripe.currencies') || {}
+ const currencies = useFeatureFlag('services.stripe.currencies') || {
+ 'USD': 'US Dollar',
+ 'EUR': 'Euro',
+ 'GBP': 'British Pound'
+ }
return Object.keys(currencies).map((item) => ({
name: currencies[item],
value: item
}))
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const currencyList = computed(() => { | |
| const currencies = useFeatureFlag('services.stripe.currencies') || {} | |
| return Object.keys(currencies).map((item) => ({ | |
| name: currencies[item], | |
| value: item | |
| })) | |
| }) | |
| const currencyList = computed(() => { | |
| const currencies = useFeatureFlag('services.stripe.currencies') || { | |
| 'USD': 'US Dollar', | |
| 'EUR': 'Euro', | |
| 'GBP': 'British Pound' | |
| } | |
| return Object.keys(currencies).map((item) => ({ | |
| name: currencies[item], | |
| value: item | |
| })) | |
| }) |
|
|
||
| const connectStripe = () => { | ||
| stripeLoading.value = true | ||
| providersStore.connect('stripe', true, true) | ||
| } | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to connectStripe method.
The method should handle potential errors from the connect operation and reset the loading state.
const connectStripe = () => {
stripeLoading.value = true
- providersStore.connect('stripe', true, true)
+ providersStore.connect('stripe', true, true)
+ .catch((error) => {
+ console.error('Failed to connect to Stripe:', error)
+ // Handle the error appropriately
+ })
+ .finally(() => {
+ stripeLoading.value = false
+ })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const connectStripe = () => { | |
| stripeLoading.value = true | |
| providersStore.connect('stripe', true, true) | |
| } | |
| </script> | |
| const connectStripe = () => { | |
| stripeLoading.value = true | |
| providersStore.connect('stripe', true, true) | |
| .catch((error) => { | |
| console.error('Failed to connect to Stripe:', error) | |
| // Handle the error appropriately | |
| }) | |
| .finally(() => { | |
| stripeLoading.value = false | |
| }) | |
| } | |
| </script> |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
client/components/open/forms/OpenForm.vue (1)
568-568:⚠️ Potential issueRemove sensitive payment data logging.
The console statement may leak payment details. This was previously flagged in past reviews.
- console.log('result', result)
🧹 Nitpick comments (2)
client/components/open/forms/OpenForm.vue (2)
122-132: Enhance payment message with amount information.The payment message should include the actual amount that will be charged to improve transparency and user trust.
<template v-if="isLastPage"> - Your credit card will be charged upon clicking the "Submit" button. Payments are securely processed through Stripe. + Your credit card will be charged {{ paymentBlock.amount }} upon clicking the "Submit" button. Payments are securely processed through Stripe. </template> <template v-else> - Your credit card will be charged upon clicking the "Next" button. Payments are securely processed through Stripe. + Your credit card will be charged {{ paymentBlock.amount }} upon clicking the "Next" button. Payments are securely processed through Stripe. </template>
317-318: Add null check for payment block access.The computed property might return undefined. Consider adding a null check to prevent runtime errors when accessing payment block properties.
paymentBlock() { - return this.currentFields.find(field => field.type === 'payment') + return this.currentFields?.find(field => field.type === 'payment') || null }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/components/open/forms/OpenForm.vue(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
| async submitForm() { | ||
| if (!this.isAutoSubmit && this.formPageIndex !== this.fieldGroups.length - 1) return | ||
|
|
||
| this.dataForm.busy = true | ||
| if (!await this.doPayment()) { | ||
| this.dataForm.busy = false | ||
| return | ||
| } | ||
| this.dataForm.busy = false | ||
|
|
There was a problem hiding this comment.
Improve error handling in form submission flow.
The submitForm and nextPage methods don't properly handle errors from doPayment. If an error occurs, the form could remain in a busy state.
async submitForm() {
if (!this.isAutoSubmit && this.formPageIndex !== this.fieldGroups.length - 1) return
this.dataForm.busy = true
- if (!await this.doPayment()) {
- this.dataForm.busy = false
- return
+ try {
+ if (!await this.doPayment()) {
+ this.dataForm.busy = false
+ return
+ }
+ } catch (error) {
+ this.dataForm.busy = false
+ useAlert().error('Payment processing failed')
+ return
}
this.dataForm.busy = falseSimilar changes should be applied to the nextPage method.
Also applies to: 539-551
| async doPayment() { | ||
| // If there is a payment block, process the payment | ||
| const { state: stripeState, processPayment } = useStripeElements() | ||
| if (this.paymentBlock && !stripeState.value.intentId && (this.paymentBlock.required || !stripeState.value.card._empty)) { | ||
| try { | ||
| // Process the payment | ||
| const result = await processPayment(this.form.slug, this.paymentBlock.required) | ||
| console.log('result', result) | ||
| if (result && result?.error) { | ||
| this.dataForm.errors.set(this.paymentBlock.id, result.error.message) | ||
| useAlert().error(result.error.message) | ||
| return false | ||
| } | ||
|
|
||
| if (result?.paymentIntent?.status === 'succeeded') { | ||
| stripeState.value.intentId = result.paymentIntent.id | ||
| useAlert().success('Thank you! Your payment is successful.') | ||
| return true | ||
| } | ||
| useAlert().error('Something went wrong. Please try again.') | ||
| } catch (error) { | ||
| console.error(error) | ||
| useAlert().error(error?.message || 'Payment failed') | ||
| } | ||
| return false | ||
| } | ||
| return true | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance payment processing error handling and success messages.
The payment processing implementation has several areas for improvement:
- Error messages should be more specific to help users understand and resolve issues.
- Success message should include the payment amount.
- Additional error cases should be handled (e.g., card declined, insufficient funds).
async doPayment() {
const { state: stripeState, processPayment } = useStripeElements()
if (this.paymentBlock && !stripeState.value.intentId && (this.paymentBlock.required || !stripeState.value.card._empty)) {
try {
const result = await processPayment(this.form.slug, this.paymentBlock.required)
- console.log('result', result)
if (result && result?.error) {
- this.dataForm.errors.set(this.paymentBlock.id, result.error.message)
- useAlert().error(result.error.message)
+ const errorMessage = this.getPaymentErrorMessage(result.error)
+ this.dataForm.errors.set(this.paymentBlock.id, errorMessage)
+ useAlert().error(errorMessage)
return false
}
if (result?.paymentIntent?.status === 'succeeded') {
stripeState.value.intentId = result.paymentIntent.id
- useAlert().success('Thank you! Your payment is successful.')
+ useAlert().success(`Thank you! Your payment of ${this.paymentBlock.amount} was successful.`)
return true
}
- useAlert().error('Something went wrong. Please try again.')
+ useAlert().error('Payment processing failed. Please try again or contact support.')
} catch (error) {
console.error(error)
- useAlert().error(error?.message || 'Payment failed')
+ useAlert().error(this.getPaymentErrorMessage(error))
}
return false
}
return true
}
+getPaymentErrorMessage(error) {
+ // Map common Stripe error codes to user-friendly messages
+ const errorMessages = {
+ 'card_declined': 'Your card was declined. Please try another card.',
+ 'insufficient_funds': 'Insufficient funds. Please try another card.',
+ 'expired_card': 'Your card has expired. Please try another card.',
+ 'incorrect_cvc': 'The security code is incorrect. Please check and try again.',
+ 'processing_error': 'An error occurred while processing your card. Please try again.',
+ 'invalid_number': 'The card number is invalid. Please check and try again.'
+ }
+ return errorMessages[error?.code] || error?.message || 'Payment failed. Please try again.'
+}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/components/open/forms/OpenForm.vue (2)
122-132: Enhance payment messaging with dynamic amount display.Consider displaying the actual payment amount in the message to improve transparency.
- Your credit card will be charged upon clicking the "Submit" button. Payments are securely processed through Stripe. + Your credit card will be charged {{ paymentBlock.amount }} upon clicking the "Submit" button. Payments are securely processed through Stripe. - Your credit card will be charged upon clicking the "Next" button. Payments are securely processed through Stripe. + Your credit card will be charged {{ paymentBlock.amount }} upon clicking the "Next" button. Payments are securely processed through Stripe.
317-318: Optimize payment block lookup.The current implementation searches the array on every access. Consider caching the result or using a more efficient lookup method.
paymentBlock() { - return this.currentFields.find(field => field.type === 'payment') + return this._paymentBlock ??= this.currentFields.find(field => field.type === 'payment') }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/components/open/forms/OpenCompleteForm.vue(2 hunks)client/components/open/forms/OpenForm.vue(5 hunks)client/components/open/forms/OpenFormField.vue(3 hunks)client/runtimeConfig.js(1 hunks)client/stores/working_form.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- client/stores/working_form.js
- client/components/open/forms/OpenCompleteForm.vue
- client/runtimeConfig.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (8)
client/components/open/forms/OpenForm.vue (4)
376-385: LGTM! Payment handling in form submission is well implemented.The implementation correctly handles the payment flow and error states.
572-572: Remove console.log statement to prevent leaking sensitive payment data.The console statement may leak payment details or error responses.
- console.log('result', result)
565-592: Enhance payment processing error handling.The payment processing implementation needs improvement in error handling and user feedback.
#!/bin/bash # Check for other instances of payment processing to ensure consistent error handling ast-grep --pattern 'processPayment($_, $_)'
543-556: 🛠️ Refactor suggestionImprove error handling in page navigation.
Consider adding a try-catch block around the payment processing to ensure proper error handling and state management.
async nextPage() { if (this.adminPreview || this.urlPrefillPreview) { this.formPageIndex++ this.scrollToTop() return false } this.dataForm.busy = true - if (!await this.doPayment()) { + try { + if (!await this.doPayment()) { + this.dataForm.busy = false + return false + } + } catch (error) { this.dataForm.busy = false - return false + useAlert().error('Payment processing failed') + return false }Likely invalid or redundant comment.
client/components/open/forms/OpenFormField.vue (4)
228-229: LGTM! Field type mapping follows conventions.The addition of payment field type mapping is consistent with the existing pattern.
403-407: Add validation for payment field properties.The codebase lacks validation for payment field properties:
- No validation for currency codes (ISO 4217)
- No validation for payment amounts
404-404: LGTM! RTL support is properly implemented.The direction property correctly respects the form's RTL layout setting.
407-407: Consider security implications of exposing Stripe account ID.Exposing the
stripe_account_iddirectly in the client-side code might pose security risks. Consider:
- Using a reference ID instead of the actual Stripe account ID
- Implementing server-side validation of the account ID
Run this script to check for other instances of exposed Stripe account IDs:
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
api/tests/Feature/Forms/FormPaymentTest.php (7)
1-5: Add PHPDoc block to describe the test class purpose.Consider adding a class-level docblock to describe the purpose and scope of these payment integration tests.
<?php + +/** + * @covers \App\Http\Controllers\Forms\FormPaymentController + * @group payments + * @group stripe + */ use Illuminate\Testing\Fluent\AssertableJson; use App\Models\OAuthProvider;
6-14: Avoid hardcoding Stripe account ID in tests.The Stripe account ID
acct_1LhEwZCragdZygxEis hardcoded. Consider using a constant or a more generic test value to prevent test brittleness.- 'provider_user_id' => 'acct_1LhEwZCragdZygxE' + 'provider_user_id' => 'acct_' . str_repeat('1', 16) // Generic test account ID
18-26: Extract payment block configuration to a helper method.Consider extracting the payment block configuration to a helper method to improve reusability and maintainability. Also, consider using constants for magic values.
+ private const TEST_AMOUNT = 99.99; + private const TEST_CURRENCY = 'USD'; + + private function getPaymentBlockConfig($stripeAccountId): array + { + return [ + 'type' => 'payment', + 'stripe_account_id' => $stripeAccountId, + 'amount' => self::TEST_AMOUNT, + 'currency' => self::TEST_CURRENCY + ]; + } + $this->form->properties = array_merge($this->form->properties, [ - [ - 'type' => 'payment', - 'stripe_account_id' => $this->stripeAccount->id, - 'amount' => 99.99, - 'currency' => 'USD' - ] + $this->getPaymentBlockConfig($this->stripeAccount->id) ]);
29-37: Enhance test assertions for better coverage.While the test verifies the basic response structure, consider adding more specific assertions:
- Verify the HTTP status code explicitly
- Assert the exact shape of the response
- Add test cases for error scenarios (e.g., invalid form slug)
it('can get stripe account for form', function () { $this->getJson(route('forms.stripe-connect.get-account', $this->form->slug)) - ->assertSuccessful() + ->assertStatus(200) ->assertJson(function (AssertableJson $json) { return $json->has('stripeAccount') ->where('stripeAccount', fn ($id) => str_starts_with($id, 'acct_')) + ->missing('error') ->etc(); }); }); + +it('returns 404 when getting stripe account for invalid form', function () { + $this->getJson(route('forms.stripe-connect.get-account', 'invalid-slug')) + ->assertStatus(404) + ->assertJson(['message' => 'Form not found.']); +});
39-48: Use enum or constant for form visibility.Consider using an enum or constant for the form visibility value to prevent magic strings and improve maintainability.
+ private const FORM_VISIBILITY_PRIVATE = 'private'; + // Update form visibility to private - $this->form->update(['visibility' => 'private']); + $this->form->update(['visibility' => self::FORM_VISIBILITY_PRIVATE]);
50-64: Simplify payment block removal and extract constants.The payment block removal logic could be simplified, and the block type should be a constant to avoid repetition.
+ private const BLOCK_TYPE_PAYMENT = 'payment'; + // Remove payment block entirely - $properties = collect($this->form->properties) - ->reject(fn ($block) => $block['type'] === 'payment') - ->values() - ->all(); + $properties = array_values(array_filter( + $this->form->properties, + fn ($block) => $block['type'] !== self::BLOCK_TYPE_PAYMENT + ));
66-82: Simplify payment block update and use meaningful constants.The payment block update logic could be simplified, and the invalid account ID should be a meaningful constant.
+ private const INVALID_STRIPE_ACCOUNT_ID = -1; + // Update payment block with non-existent stripe account - $properties = collect($this->form->properties)->map(function ($block) { - if ($block['type'] === 'payment') { - $block['stripe_account_id'] = 999999; - } - return $block; - })->all(); + $properties = array_map(function ($block) { + if ($block['type'] === self::BLOCK_TYPE_PAYMENT) { + $block['stripe_account_id'] = self::INVALID_STRIPE_ACCOUNT_ID; + } + return $block; + }, $this->form->properties);client/components/forms/PaymentInput.client.vue (3)
67-69: Add loading state message for better UX.While the loader is shown, adding a loading message would improve user experience.
<div v-else> + <p class="text-sm text-gray-500 mb-2">{{ $t('forms.payment.loading') }}</p> <Loader class="mx-auto h-6 w-6" /> </div>
31-34: Format amount with proper decimal places.The amount display should follow currency formatting conventions.
<div class="mb-4 flex items-center justify-between bg-gray-50 px-4 py-3 rounded-lg"> <span class="text-sm font-medium text-gray-700">{{ $t('forms.payment.amount_to_pay') }}</span> - <span class="text-sm font-medium text-gray-900">{{ currencySymbol }}{{ amount }}</span> + <span class="text-sm font-medium text-gray-900">{{ currencySymbol }}{{ amount.toFixed(2) }}</span> </div>
113-115: Remove unused parameter and improve type safety.The
elementparameter is unused and the optional chaining could be improved.-const onCardReady = (element) => { +const onCardReady = () => { - stripeState.value.card = card.value?.stripeElement + if (card.value && card.value.stripeElement) { + stripeState.value.card = card.value.stripeElement + } }🧰 Tools
🪛 GitHub Check: Run client linters
[warning] 113-113:
'element' is defined but never usedclient/components/open/forms/fields/components/PaymentFieldOptions.vue (2)
68-73: Enhance props validation.The field prop validation could be more specific about required properties for payment fields.
Consider adding detailed prop validation:
const props = defineProps({ field: { type: Object, - required: true + required: true, + validator: (field) => { + const requiredProps = ['type', 'currency', 'amount'] + return requiredProps.every(prop => prop in field) + } } })
90-100: Add error handling for computed properties.The computed properties need protection against malformed data.
Consider these improvements:
const stripeAccounts = computed(() => providersStore.getAll.filter((item) => item.provider === 'stripe').map((item) => ({ - name: item.name + (item.email ? ' (' + item.email + ')' : ''), + name: [item.name, item.email && `(${item.email})`].filter(Boolean).join(' '), value: item.id }))) const currencyList = computed(() => { + if (!Array.isArray(stripeCurrencies)) { + console.error('Invalid currency data') + return [] + } return stripeCurrencies.map((item) => ({ name: item.name, value: item.code })) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/app/Http/Controllers/Content/FeatureFlagsController.php(1 hunks)api/app/Rules/PaymentBlockConfigurationRule.php(1 hunks)api/config/services.php(1 hunks)api/resources/data/stripe_currencies.json(1 hunks)api/tests/Feature/Forms/FormPaymentTest.php(1 hunks)client/components/forms/PaymentInput.client.vue(1 hunks)client/components/open/forms/fields/components/PaymentFieldOptions.vue(1 hunks)client/data/stripe_currencies.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- api/resources/data/stripe_currencies.json
- client/data/stripe_currencies.json
- api/app/Http/Controllers/Content/FeatureFlagsController.php
🚧 Files skipped from review as they are similar to previous changes (2)
- api/config/services.php
- api/app/Rules/PaymentBlockConfigurationRule.php
🧰 Additional context used
🪛 GitHub Check: Run client linters
client/components/forms/PaymentInput.client.vue
[warning] 113-113:
'element' is defined but never used
[warning] 133-133:
'newValue' is defined but never used
[warning] 136-136:
'newValue' is defined but never used
🔇 Additional comments (4)
client/components/forms/PaymentInput.client.vue (1)
109-111: Remove console.log and implement proper error handling.The console.log statement should be replaced with proper error handling.
client/components/open/forms/fields/components/PaymentFieldOptions.vue (3)
20-27: Add proper amount validation.The amount input needs additional validation to prevent negative or zero values.
50-54: Fix invalid anchor target attribute.The anchor tag has an invalid
target="#"attribute. Since this is a click handler and not a navigation link, remove the target attribute.
102-107: Add error handling to connectStripe method.The method should handle potential errors from the connect operation and reset the loading state.
| const initStripe = async () => { | ||
| if (!formSlug.value) return | ||
| try { | ||
| const response = await opnFetch('/forms/' + formSlug.value + '/stripe-connect/get-account') |
There was a problem hiding this comment.
Add CSRF protection to API calls.
The API call to fetch Stripe account details should include CSRF protection.
- const response = await opnFetch('/forms/' + formSlug.value + '/stripe-connect/get-account')
+ const response = await opnFetch('/forms/' + formSlug.value + '/stripe-connect/get-account', {
+ headers: {
+ 'X-CSRF-TOKEN': document.querySelector('meta[name="csrf-token"]').content
+ }
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await opnFetch('/forms/' + formSlug.value + '/stripe-connect/get-account') | |
| const response = await opnFetch('/forms/' + formSlug.value + '/stripe-connect/get-account', { | |
| headers: { | |
| 'X-CSRF-TOKEN': document.querySelector('meta[name="csrf-token"]').content | |
| } | |
| }) |
| const props = defineProps({ | ||
| ...inputProps, | ||
| direction: { type: String, default: 'ltr' }, | ||
| currency: { type: String, default: 'USD' }, | ||
| amount: { type: Number, default: 0 }, | ||
| oauthProviderId: { type: String, default: null } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance props validation for better error handling.
Add validation for currency and amount props to ensure valid values.
const props = defineProps({
...inputProps,
direction: { type: String, default: 'ltr' },
- currency: { type: String, default: 'USD' },
- amount: { type: Number, default: 0 },
+ currency: {
+ type: String,
+ default: 'USD',
+ validator: (value) => stripeCurrencies.some(currency => currency.code === value)
+ },
+ amount: {
+ type: Number,
+ default: 0,
+ validator: (value) => value >= 0
+ },
oauthProviderId: { type: String, default: null }
})Committable suggestion skipped: line range outside the PR's diff.
| onMounted(async () => { | ||
| initStripe() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup logic for Stripe elements.
Add onUnmounted hook to clean up Stripe elements and prevent memory leaks.
onMounted(async () => {
initStripe()
})
+
+onUnmounted(() => {
+ if (card.value?.stripeElement) {
+ card.value.stripeElement.unmount()
+ }
+ stripeState.value.isLoaded = false
+ stripeState.value.stripe = null
+ stripeState.value.elements = null
+})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onMounted(async () => { | |
| initStripe() | |
| }) | |
| onMounted(async () => { | |
| initStripe() | |
| }) | |
| onUnmounted(() => { | |
| if (card.value?.stripeElement) { | |
| card.value.stripeElement.unmount() | |
| } | |
| stripeState.value.isLoaded = false | |
| stripeState.value.stripe = null | |
| stripeState.value.elements = null | |
| }) |
| <select-input | ||
| name="currency" | ||
| label="Currency" | ||
| :options="currencyList" | ||
| :form="field" | ||
| :required="true" | ||
| :searchable="true" | ||
| :disabled="stripeAccounts.length === 0" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and validation feedback.
The form elements lack proper accessibility attributes and validation feedback states.
Apply these changes:
<select-input
name="currency"
label="Currency"
+ aria-label="Select payment currency"
:options="currencyList"
:form="field"
:required="true"
:searchable="true"
:disabled="stripeAccounts.length === 0"
+ :error="field.errors?.currency"
/>
<div v-if="stripeAccounts.length > 0">
<select-input
name="stripe_account_id"
label="Stripe Account"
+ aria-label="Select Stripe account"
:options="stripeAccounts"
:form="field"
:required="true"
+ :error="field.errors?.stripe_account_id"
/>Also applies to: 28-39
| const crisp = useCrisp() | ||
| const providersStore = useOAuthProvidersStore() | ||
| const stripeLoading = ref(false) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error state management.
The component lacks error state management for failed operations.
Add error state handling:
const crisp = useCrisp()
const providersStore = useOAuthProvidersStore()
const stripeLoading = ref(false)
+const error = ref(null)
+
+const clearError = () => {
+ error.value = null
+}
+
+const setError = (message) => {
+ error.value = message
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const crisp = useCrisp() | |
| const providersStore = useOAuthProvidersStore() | |
| const stripeLoading = ref(false) | |
| const crisp = useCrisp() | |
| const providersStore = useOAuthProvidersStore() | |
| const stripeLoading = ref(false) | |
| const error = ref(null) | |
| const clearError = () => { | |
| error.value = null | |
| } | |
| const setError = (message) => { | |
| error.value = message | |
| } |
| onMounted(() => { | ||
| providersStore.fetchOAuthProviders() | ||
|
|
||
| if(props.field?.currency === undefined || props.field?.currency === null) { | ||
| props.field.currency = 'USD' | ||
| } | ||
| if(props.field?.amount === undefined || props.field?.amount === null) { | ||
| props.field.amount = 10 | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve lifecycle hook implementation.
The component needs better error handling and cleanup.
Consider these improvements:
onMounted(() => {
- providersStore.fetchOAuthProviders()
+ providersStore.fetchOAuthProviders().catch(err => {
+ setError('Failed to fetch Stripe accounts: ' + err.message)
+ })
if(props.field?.currency === undefined || props.field?.currency === null) {
props.field.currency = 'USD'
}
if(props.field?.amount === undefined || props.field?.amount === null) {
- props.field.amount = 10
+ props.field.amount = getCurrencyDefaultAmount(props.field.currency)
}
})
+
+onUnmounted(() => {
+ clearError()
+})Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
client/components/open/forms/components/form-components/FormSubmissionSettings.vue (2)
30-35: Improve alert message clarity and fix grammatical error.The alert message has a grammatical error and could be more informative.
<UAlert v-if="hasPaymentBlock" color="primary" variant="subtle" - title="You have a payment block in your form. so can't disable auto save" + title="Auto-save is required when using payment blocks in your form" class="max-w-md" />
244-246: Consider memoizing the payment block check for large forms.The computed property works correctly but could benefit from memoization when dealing with large forms containing many properties.
const hasPaymentBlock = computed(() => { - return form.value.properties.some(property => property.type === 'payment') + // Memoize the result to avoid unnecessary recalculations + if (!form.value?.properties) return false + if (!hasPaymentBlock.cache) { + hasPaymentBlock.cache = form.value.properties.some(property => property.type === 'payment') + } + return hasPaymentBlock.cache }) + +// Reset cache when properties change +watch(() => form.value.properties, () => { + hasPaymentBlock.cache = undefined +})client/components/forms/PaymentInput.client.vue (4)
31-34: Enhance amount display formatting.The amount display should handle decimal places and proper currency formatting.
- <span class="text-sm font-medium text-gray-900">{{ currencySymbol }}{{ amount }}</span> + <span class="text-sm font-medium text-gray-900">{{ currencySymbol }}{{ amount.toLocaleString(locale, { minimumFractionDigits: 2, maximumFractionDigits: 2 }) }}</span>
42-51: Add accessibility attributes to payment form.The card input section should have proper ARIA labels for better accessibility.
<StripeElement ref="card" :dir="props.direction" :elements="elements" :options="cardOptions" + aria-label="Credit or debit card" @ready="onCardReady" @change="onCardChanged" />
113-115: Remove unused parameter.The
elementparameter inonCardReadyis not being used.-const onCardReady = (element) => { +const onCardReady = () => { stripeState.value.card = card.value?.stripeElement }🧰 Tools
🪛 GitHub Check: Run client linters
[warning] 113-113:
'element' is defined but never used
138-143: Add debouncing to card reset operations.The card reset operations triggered by direction and locale changes should be debounced to prevent excessive remounting.
+const debouncedResetCard = useDebounceFn(async () => { + await resetCard() +}, 300) + watch(() => props.direction, async () => { - await resetCard() + await debouncedResetCard() }) watch(() => props.locale, async () => { - await resetCard() + await debouncedResetCard() })🧰 Tools
🪛 GitHub Check: Run client linters
[warning] 138-138:
'newValue' is defined but never used
[warning] 141-141:
'newValue' is defined but never used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/app/Http/Resources/FormResource.php(2 hunks)client/components/forms/PaymentInput.client.vue(1 hunks)client/components/open/forms/OpenCompleteForm.vue(1 hunks)client/components/open/forms/components/form-components/FormSubmissionSettings.vue(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/components/open/forms/OpenCompleteForm.vue
🧰 Additional context used
🪛 GitHub Check: Run client linters
client/components/forms/PaymentInput.client.vue
[warning] 113-113:
'element' is defined but never used
[warning] 138-138:
'newValue' is defined but never used
[warning] 141-141:
'newValue' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build the Nuxt app
- GitHub Check: Run code linters
🔇 Additional comments (4)
api/app/Http/Resources/FormResource.php (2)
52-52: LGTM! Well-structured addition of the auto_save field.The change follows Laravel's resource transformation patterns and maintains good separation of concerns.
117-127: Verify handling of multiple payment blocks.Since the PR enforces a single payment block per form (via
PaymentBlockConfigurationRule), let's verify that this implementation handles that constraint correctly.Run this script to check the validation and resource handling:
✅ Verification successful
Implementation correctly handles payment block constraints
The
PaymentBlockConfigurationRuleexplicitly validates that only one payment block exists per form, making the current implementation inFormResourcesafe and consistent with this constraint.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify payment block validation and resource handling consistency # Find the validation rule implementation echo "Checking validation rule implementation:" rg -A 5 "class PaymentBlockConfigurationRule" # Find any other payment block type checks echo -e "\nChecking other payment block type checks:" rg "type.*payment" --type phpLength of output: 1625
Script:
#!/bin/bash # Get the complete implementation of PaymentBlockConfigurationRule echo "PaymentBlockConfigurationRule implementation:" cat api/app/Rules/PaymentBlockConfigurationRule.phpLength of output: 2568
client/components/open/forms/components/form-components/FormSubmissionSettings.vue (2)
41-41: LGTM! Consistent spacing improvements.The margin-top additions improve the vertical rhythm of the form elements.
Also applies to: 145-145
27-29: Verify UX decision for enforcing auto-save with payment blocks.The auto-save toggle is disabled when a payment block is present. While this makes sense for payment processing, we should ensure this limitation is clearly communicated to users.
Consider adding a tooltip to explain why the toggle is disabled when hovering over it in the disabled state.
<ToggleSwitchInput name="auto_save" :form="form" label="Auto save form response" help="Saves form progress, allowing respondents to resume later." class="mt-4" :disabled="hasPaymentBlock" + :title="hasPaymentBlock ? 'Auto-save is required for forms with payment blocks' : ''" />✅ Verification successful
Auto-save requirement with payment blocks is verified and correct
The payment integration requires reliable state management and data persistence for payment processing and recovery. The auto-save requirement is justified. However, let's make the UX clearer:
<ToggleSwitchInput name="auto_save" :form="form" label="Auto save form response" help="Saves form progress, allowing respondents to resume later." class="mt-4" :disabled="hasPaymentBlock" + :help="hasPaymentBlock ? 'Auto-save is enabled by default for forms with payments to ensure transaction reliability.' : 'Saves form progress, allowing respondents to resume later.'" />🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for payment block related code and auto-save logic echo "=== Payment Block Implementation ===" rg -l "hasPaymentBlock" --type vue echo -e "\n=== Auto-save Logic ===" rg "auto.?save.*payment" --type vue -A 2 -B 2 echo -e "\n=== Disabled State Tooltips ===" rg "disabled.*title" --type vue -A 1 -B 1 echo -e "\n=== Payment Components ===" fd ".*Payment.*\.vue$"Length of output: 752
Script:
#!/bin/bash # Inspect payment components and related logic echo "=== Payment Field Options Content ===" cat "client/components/open/forms/fields/components/PaymentFieldOptions.vue" echo -e "\n=== Payment Input Content ===" cat "client/components/forms/PaymentInput.client.vue" echo -e "\n=== Form Settings AST Search ===" ast-grep --pattern 'computed: { $$$ hasPaymentBlock() { $$$ } $$$ }'Length of output: 8729
| private function hasPaymentBlock() | ||
| { | ||
| return array_filter($this->properties, function ($property) { | ||
| return $property['type'] === 'payment'; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve the hasPaymentBlock() method implementation.
The current implementation has several issues:
- The method name implies a boolean return, but it returns an array
- Missing null check for
$this->properties - Could be more efficient
Consider this improved implementation:
private function hasPaymentBlock()
{
- return array_filter($this->properties, function ($property) {
- return $property['type'] === 'payment';
- });
+ if (!isset($this->properties)) {
+ return false;
+ }
+ return (bool) array_search('payment', array_column($this->properties, 'type'));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private function hasPaymentBlock() | |
| { | |
| return array_filter($this->properties, function ($property) { | |
| return $property['type'] === 'payment'; | |
| }); | |
| } | |
| private function hasPaymentBlock() | |
| { | |
| if (!isset($this->properties)) { | |
| return false; | |
| } | |
| return (bool) array_search('payment', array_column($this->properties, 'type')); | |
| } |
| private function getAutoSave() | ||
| { | ||
| return $this->hasPaymentBlock() ? true : $this->auto_save; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Fix hasPaymentBlock to return boolean instead of array
The current implementation of hasPaymentBlock() returns a filtered array instead of a boolean, which could lead to unexpected behavior. While it works in the current context due to PHP's type coercion, it's not semantically correct.
Please update the implementation:
private function hasPaymentBlock()
{
- return array_filter($this->properties, function ($property) {
+ return collect($this->properties)->contains(function ($property) {
return $property['type'] === 'payment';
});
}🔗 Analysis chain
Document the auto-save requirement for payment forms.
The implementation enforces auto-save for forms with payment blocks, but this important business rule isn't documented. Additionally, the current implementation might not work as intended due to hasPaymentBlock() returning an array.
Please:
- Add a PHPDoc comment explaining why payment forms require auto-save
- Consider this improved implementation:
/**
+ * Get the auto_save value for the form.
+ * Forms with payment blocks must have auto_save enabled to ensure payment data persistence.
+ *
+ * @return bool
+ */
private function getAutoSave()
{
- return $this->hasPaymentBlock() ? true : $this->auto_save;
+ return $this->hasPaymentBlock() || $this->auto_save;
}🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find hasPaymentBlock implementation
echo "=== hasPaymentBlock implementation ==="
ast-grep --pattern 'function hasPaymentBlock() {
$$$
}'
echo -e "\n=== FormResource test files ==="
fd -e php test FormResource
echo -e "\n=== Other usages of hasPaymentBlock ==="
rg "hasPaymentBlock" -A 2
Length of output: 1910
Script:
#!/bin/bash
# Get complete implementation of hasPaymentBlock method
echo "=== Complete hasPaymentBlock implementation ==="
rg -U "private function hasPaymentBlock\(\).*?\}" --multiline api/app/Http/Resources/FormResource.php
Length of output: 208
Script:
#!/bin/bash
echo "=== FormResource.php content ==="
cat api/app/Http/Resources/FormResource.php
Length of output: 4245
| <StripeElements | ||
| ref="stripeElements" | ||
| v-slot="{ elements }" | ||
| :stripe-key="publishableKey" | ||
| :instance-options="stripeOptions" | ||
| > |
There was a problem hiding this comment.
💡 Codebase verification
CSP headers for Stripe Elements are missing and need to be implemented
The codebase lacks Content Security Policy configuration required for secure Stripe Elements integration. You need to:
- Add CSP configuration in your Nuxt.js application by creating a server middleware that sets the following headers:
connect-src: ['self', 'api.stripe.com']
frame-src: ['self', 'js.stripe.com']
script-src: ['self', 'js.stripe.com']- Create a new file
server/middleware/security.tswith the CSP configuration.
This is crucial for:
- Secure operation of Stripe Elements
- Protection against XSS attacks
- Compliance with security best practices
🔗 Analysis chain
Add Content Security Policy headers for Stripe.
Ensure that your application's Content Security Policy (CSP) headers include the necessary domains for Stripe Elements:
- connect-src: 'api.stripe.com'
- frame-src: 'js.stripe.com'
- script-src: 'js.stripe.com'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if CSP headers are properly configured
curl -I https://your-domain.com | grep -i 'content-security-policy'
Length of output: 384
Script:
#!/bin/bash
# Search for files that might contain CSP configuration
echo "=== Searching for Nuxt config files ==="
fd -e js -e ts "^nuxt\.config\."
echo -e "\n=== Searching for files containing CSP related code ==="
rg -i "content.?security.?policy|CSP" --type js --type ts --type vue -l
echo -e "\n=== Searching for security middleware files ==="
fd -e js -e ts "security" -t f
Length of output: 516
Script:
#!/bin/bash
echo "=== Checking Nuxt config for CSP configuration ==="
cat client/nuxt.config.ts
Length of output: 4114
Script:
#!/bin/bash
echo "=== Checking runtime config for CSP configuration ==="
cat client/runtimeConfig.ts
echo -e "\n=== Checking for server middleware files ==="
fd -e js -e ts "middleware" -t d
Length of output: 340
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
api/app/Rules/PaymentBlockConfigurationRule.php (2)
36-44: Optimize payment block count validation.The current implementation processes all properties even after finding more than one payment block. Consider using
contains()for better performance.- $paymentBlocks = collect($this->properties) - ->filter(fn ($prop) => $prop['type'] === 'payment') - ->count(); - - if ($paymentBlocks > 1) { + $hasExistingPaymentBlock = false; + foreach ($this->properties as $prop) { + if ($prop['type'] === 'payment') { + if ($hasExistingPaymentBlock) { + $fail('Only one payment block allowed'); + return; + } + $hasExistingPaymentBlock = true; + } + } - $fail('Only one payment block allowed'); - return; - }
60-79: Add type validation for stripe_account_id and consider caching provider.The validation includes a database query and lacks type checking for the stripe_account_id.
// Stripe account validation - if (!isset($this->field['stripe_account_id']) || empty($this->field['stripe_account_id'])) { + if (!isset($this->field['stripe_account_id']) + || !is_string($this->field['stripe_account_id']) + || empty($this->field['stripe_account_id'])) { $fail('Stripe account is required'); return; }Also, consider caching the OAuthProvider lookup results if this validation is called frequently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/app/Rules/PaymentBlockConfigurationRule.php(1 hunks)client/data/blocks_types.json(1 hunks)client/stores/working_form.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/stores/working_form.js
🔇 Additional comments (3)
api/app/Rules/PaymentBlockConfigurationRule.php (2)
1-19: LGTM! Class structure follows Laravel conventions.The class is well-structured with proper property visibility and a focused constructor.
30-34: LGTM! Clear self-hosted environment check.Good validation to prevent payment blocks in self-hosted environments with a clear error message.
client/data/blocks_types.json (1)
155-164: LGTM! Payment block configuration is consistent with other blocks.The payment block definition follows the established pattern and includes the necessary self_hosted flag that aligns with the validation rules.
| // Amount validation | ||
| if (!isset($this->field['amount']) || !is_numeric($this->field['amount']) || $this->field['amount'] < 0.5) { | ||
| $fail('Amount must be a number greater than 0.5'); | ||
| return; | ||
| } | ||
|
|
||
| // Currency validation | ||
| $stripeCurrencies = json_decode(file_get_contents(resource_path('data/stripe_currencies.json')), true); | ||
| if (!isset($this->field['currency']) || !in_array(strtoupper($this->field['currency']), array_column($stripeCurrencies, 'code'))) { | ||
| $fail('Currency must be a valid currency'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cache stripe currencies to improve performance.
Reading the JSON file on every validation could impact performance. Consider caching the currencies list.
+ protected static array $stripeCurrencies;
+
+ protected function getStripeCurrencies(): array
+ {
+ if (!isset(self::$stripeCurrencies)) {
+ self::$stripeCurrencies = json_decode(file_get_contents(
+ resource_path('data/stripe_currencies.json')
+ ), true);
+ }
+ return self::$stripeCurrencies;
+ }
// In validate method:
- $stripeCurrencies = json_decode(file_get_contents(resource_path('data/stripe_currencies.json')), true);
+ $stripeCurrencies = $this->getStripeCurrencies();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Amount validation | |
| if (!isset($this->field['amount']) || !is_numeric($this->field['amount']) || $this->field['amount'] < 0.5) { | |
| $fail('Amount must be a number greater than 0.5'); | |
| return; | |
| } | |
| // Currency validation | |
| $stripeCurrencies = json_decode(file_get_contents(resource_path('data/stripe_currencies.json')), true); | |
| if (!isset($this->field['currency']) || !in_array(strtoupper($this->field['currency']), array_column($stripeCurrencies, 'code'))) { | |
| $fail('Currency must be a valid currency'); | |
| return; | |
| } | |
| // Amount validation | |
| if (!isset($this->field['amount']) || !is_numeric($this->field['amount']) || $this->field['amount'] < 0.5) { | |
| $fail('Amount must be a number greater than 0.5'); | |
| return; | |
| } | |
| protected static array $stripeCurrencies; | |
| protected function getStripeCurrencies(): array | |
| { | |
| if (!isset(self::$stripeCurrencies)) { | |
| self::$stripeCurrencies = json_decode(file_get_contents( | |
| resource_path('data/stripe_currencies.json') | |
| ), true); | |
| } | |
| return self::$stripeCurrencies; | |
| } | |
| // Currency validation | |
| $stripeCurrencies = $this->getStripeCurrencies(); | |
| if (!isset($this->field['currency']) || !in_array(strtoupper($this->field['currency']), array_column($stripeCurrencies, 'code'))) { | |
| $fail('Currency must be a valid currency'); | |
| return; | |
| } |
| public function validate(string $attribute, mixed $value, Closure $fail): void | ||
| { | ||
| // Set the field | ||
| $fieldIndex = explode('.', $attribute)[1]; | ||
| $this->field = $this->properties[$fieldIndex]; | ||
|
|
||
| if ($this->field['type'] !== 'payment') { | ||
| return; // If not a payment block, validation passes | ||
| } |
There was a problem hiding this comment.
Add array access validation to prevent undefined index errors.
The field index extraction assumes the attribute string contains a dot and the extracted index exists in the properties array. This could lead to undefined index errors.
public function validate(string $attribute, mixed $value, Closure $fail): void
{
// Set the field
- $fieldIndex = explode('.', $attribute)[1];
- $this->field = $this->properties[$fieldIndex];
+ $parts = explode('.', $attribute);
+ if (count($parts) < 2 || !isset($this->properties[$parts[1]])) {
+ $fail('Invalid field configuration');
+ return;
+ }
+ $this->field = $this->properties[$parts[1]];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function validate(string $attribute, mixed $value, Closure $fail): void | |
| { | |
| // Set the field | |
| $fieldIndex = explode('.', $attribute)[1]; | |
| $this->field = $this->properties[$fieldIndex]; | |
| if ($this->field['type'] !== 'payment') { | |
| return; // If not a payment block, validation passes | |
| } | |
| public function validate(string $attribute, mixed $value, Closure $fail): void | |
| { | |
| // Set the field | |
| $parts = explode('.', $attribute); | |
| if (count($parts) < 2 || !isset($this->properties[$parts[1]])) { | |
| $fail('Invalid field configuration'); | |
| return; | |
| } | |
| $this->field = $this->properties[$parts[1]]; | |
| if ($this->field['type'] !== 'payment') { | |
| return; // If not a payment block, validation passes | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/components/forms/PaymentInput.client.vue (3)
108-110: Remove unused parameter.The
elementparameter is not used in the function.-const onCardReady = (element) => { +const onCardReady = () => { stripeState.value.card = card.value?.stripeElement }🧰 Tools
🪛 GitHub Check: Run client linters
[warning] 108-108:
'element' is defined but never used
133-138: Remove unused parameters.The
newValueparameters are not used in both watchers.-watch(() => props.direction, async (newValue) => { +watch(() => props.direction, async () => { await resetCard() }) -watch(() => props.locale, async (newValue) => { +watch(() => props.locale, async () => { await resetCard() })🧰 Tools
🪛 GitHub Check: Run client linters
[warning] 133-133:
'newValue' is defined but never used
[warning] 136-136:
'newValue' is defined but never used
154-159: Consider using a constant for the route name.Using string literals for route names can be error-prone. Consider extracting the route name to a constant.
+const FORM_ROUTE_PREFIX = 'forms-slug' + const formSlug = computed(() => { - if (route.name && route.name.startsWith("forms-slug")) { + if (route.name && route.name.startsWith(FORM_ROUTE_PREFIX)) { return route.params.slug } return null })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/components/forms/PaymentInput.client.vue(1 hunks)client/components/open/forms/OpenForm.vue(4 hunks)
🧰 Additional context used
🪛 GitHub Check: Run client linters
client/components/forms/PaymentInput.client.vue
[warning] 108-108:
'element' is defined but never used
[warning] 133-133:
'newValue' is defined but never used
[warning] 136-136:
'newValue' is defined but never used
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build the Nuxt app
🔇 Additional comments (9)
client/components/forms/PaymentInput.client.vue (5)
112-122: LGTM!The watchers correctly synchronize the form state with the Stripe state.
124-131: LGTM!The watchers correctly handle bi-directional synchronization of the payment intent ID between the stripe state and form input.
140-152: LGTM!The computed properties correctly provide the required values for currency symbol, stripe options, and card options.
165-187: Improve error handling in initStripe.The error handling could be more specific and informative.
} catch (error) { - console.error('Stripe initialization error:', error) + const errorMessage = error.response?.data?.message || 'Failed to initialize payment system' + useAlert().error(errorMessage) + console.error('Stripe initialization error:', error) stripeState.value.isLoaded = false }
189-195: Add error handling to resetCard method.The card reset operation should handle potential errors during unmount/mount operations.
const resetCard = async () => { if (card.value?.stripeElement) { - card.value.stripeElement.unmount() - await nextTick() - card.value.stripeElement.mount(card.value.$el) + try { + card.value.stripeElement.unmount() + await nextTick() + await card.value.stripeElement.mount(card.value.$el) + } catch (error) { + console.error('Failed to reset card element:', error) + useAlert().error('Failed to reset payment form') + } } }client/components/open/forms/OpenForm.vue (4)
317-318: LGTM!The computed property correctly finds and returns the payment block from the current fields.
376-380: LGTM!The function correctly handles form submission and error cases.
541-574: LGTM!The function correctly handles page navigation, error handling, and payment processing integration.
575-603: Enhance payment processing error handling.The error handling could be more specific and informative.
} catch (error) { this.dataForm.busy = false - console.error(error) - useAlert().error(error?.message || 'Payment failed') + const errorMessage = this.getPaymentErrorMessage(error) + useAlert().error(errorMessage) } +getPaymentErrorMessage(error) { + // Map common Stripe error codes to user-friendly messages + const errorMessages = { + 'card_declined': 'Your card was declined. Please try another card.', + 'insufficient_funds': 'Insufficient funds. Please try another card.', + 'expired_card': 'Your card has expired. Please try another card.', + 'incorrect_cvc': 'The security code is incorrect. Please check and try again.', + 'processing_error': 'An error occurred while processing your card. Please try again.', + 'invalid_number': 'The card number is invalid. Please check and try again.' + } + return errorMessages[error?.code] || error?.message || 'Payment failed. Please try again.' +}
|
@JhumanJ |
Summary by CodeRabbit
Release Notes: Stripe Payment Integration
New Features
Improvements
Configuration
Localization