Register the integration webhook in Sequra#104
Register the integration webhook in Sequra#104tamaralogeecom wants to merge 17 commits intomasterfrom
Conversation
ISSUE: LIS-90
ISSUE: LIS-90
ISSUE: LIS-90
ISSUE: LIS-90
ISSUE: LIS-90
ISSUE: LIS-90
ISSUE: LIS-90
ISSUE: LIS-90
ISSUE: LIS-90
# Conflicts: # composer.json # package-lock.json
ISSUE: LIS-90
ISSUE: LIS-90
ISSUE: LIS-90
ISSUE: LIS-90
|
We have noticed that an “Advanced” section has been added in the latest CORE UI version. Should these changes be included in the current onboarding implementation for the Magento 2 integration? They would require implementing a new endpoint and backend business logic. If this is something that should be included at this point, then the exact requirements for what the endpoint should return need to be clearly defined. |
|
We also observed that |
|
Please provide us with the translations for the button on the Connection Settings page, which allows merchants to re-register their store integration for the corresponding deployment target: Button label:
Success message:
Error message:
|
Thanks @tamaralogeecom for pointing this out. I've submitted a solution via this PR. Please have a look at it |
This new section is not part of the Onboarding flow. It is accesible once the onboarding is completed. I would implement it in a new PR due you will need to do several changes to support it. There's a working implementation in the woocommerce repository you can use as a guide. |
Button label:
Success message:
Error message:
|
There was a problem hiding this comment.
Pull request overview
This pull request implements webhook registration functionality in Sequra for a Magento 2 integration. The changes include adding a webhook re-registration button on the Connection Settings page, implementing a RegexProvider for validation, updating the CORE and CORE UI library versions, and adding an Advanced section with debug logging capabilities.
Key Changes:
- Added webhook re-registration functionality with UI button and backend endpoint
- Implemented regex validation system with ValidationService updates
- Added Advanced/Debug section for log management
- Updated CORE UI library to enable new features
- Fixed URL parameter encoding (changed from
encodeURIComponentwrapping to inline usage) - Added comprehensive translation updates for 5 languages (EN, ES, FR, IT, PT)
Reviewed changes
Copilot reviewed 36 out of 41 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| StateController.js (override) | Adds Advanced state/pages, fixes URL encoding, adds data store clearing |
| SettingsController.js (override) | Updates to use new encoding pattern and adds SequraFE.customHeader |
| PaymentController.js (override) | Adds support link, updates encoding, handles payment method icons |
| ConnectionSettingsForm.js | Implements webhook re-registration button and handler |
| AdvancedController.js | New controller for debug logs management page |
| ValidationService.js | Adds regex provider requirement check and new validation methods |
| UtilityService.js | Adds getMenuItems helper for navigation |
| ResponseService.js | Adds successHandler for success messages |
| ElementGenerator.js | Adds createActionsBar and createSupportLink helpers |
| GeneralSettingsForm.js | Adds service-related fields and validation |
| Translation files | Comprehensive updates for webhook, debug, and service features |
| sequra-core.css | Major styling updates with proper scoping and new components |
| index.phtml | Adds regex configuration and ValidationService loading |
| sequra_configuration_index.xml | Updates script loading order |
| package.json | Updates CORE UI version to feature branch |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ISSUE: LIS-90
ISSUE: LIS-90
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 41 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (!dataStore.deploymentsSettings?.some(deployment => deployment.active === true)) { | ||
| page = SequraFE.appPages.DEPLOYMENTS; | ||
| } |
There was a problem hiding this comment.
issue (blocking): Invalid appPages reference breaks onboarding navigation
SequraFE.appPages.DEPLOYMENTS doesn’t exist (DEPLOYMENTS is nested under appPages.ONBOARDING). This will set page to undefined and can route users to the wrong onboarding step. Use SequraFE.appPages.ONBOARDING.DEPLOYMENTS here (and in the other occurrences in this switch).
| currentVersion: version.current, | ||
| newVersion: { | ||
| versionLabel: version.new, | ||
| versionUrl: version.downloadNewVersionUrl | ||
| }, |
There was a problem hiding this comment.
issue (blocking): newVersion is always rendered, even when no update is available
PageHeader shows a download link whenever newVersion is truthy and versionLabel !== currentVersion. Passing { versionLabel: null, versionUrl: null } will still be truthy and can render a broken link. Pass null unless both version.new and version.downloadNewVersionUrl are present.
| Promise.all([ | ||
| sellingCountries ? [] : api.get(configuration.getSellingCountriesUrl, null, SequraFE.customHeader), | ||
| paymentMethods ? [] : api.get(configuration.getPaymentMethodsUrl.replace(encodeURIComponent('{merchantId}'), countryConfiguration[0].merchantId)), | ||
| ]).then(([sellingCountriesRes, paymentMethodsRes]) => { |
There was a problem hiding this comment.
issue (blocking): Missing custom header when loading payment methods
This get() call omits SequraFE.customHeader (3rd param), so it may fail in environments where the backend requires the custom header. Use api.get(url, null, SequraFE.customHeader) (and keep the encoded {merchantId} replacement).
| const valid = validator.validateDateOrDuration(changedGeneralSettings.defaultServicesEndDate); | ||
| validator.validateField( | ||
| document.querySelector('.sq-default-services-end-date'), | ||
| !valid, | ||
| 'validation.invalidTimeDuration' | ||
| ); | ||
|
|
There was a problem hiding this comment.
issue (blocking): isValidTimeDuration() can crash when the field isn’t rendered
document.querySelector('.sq-default-services-end-date') can be null when service-selling fields aren’t added, but validateField() assumes a non-null element and will throw. Guard on SequraFE.flags.isServiceSellingAllowed and/or the element existing before validating.
| const valid = validator.validateDateOrDuration(changedGeneralSettings.defaultServicesEndDate); | |
| validator.validateField( | |
| document.querySelector('.sq-default-services-end-date'), | |
| !valid, | |
| 'validation.invalidTimeDuration' | |
| ); | |
| // If service selling is not allowed, skip validating the related field. | |
| if (SequraFE.flags && SequraFE.flags.isServiceSellingAllowed === false) { | |
| return true; | |
| } | |
| const fieldElement = document.querySelector('.sq-default-services-end-date'); | |
| const valid = validator.validateDateOrDuration(changedGeneralSettings.defaultServicesEndDate); | |
| // Only attempt to validate the field if the element exists in the DOM. | |
| if (fieldElement) { | |
| validator.validateField( | |
| fieldElement, | |
| !valid, | |
| 'validation.invalidTimeDuration' | |
| ); | |
| } |
| $jsonEncoded = json_encode($regex); | ||
| return $jsonEncoded === false ? '' : $jsonEncoded; |
There was a problem hiding this comment.
issue (blocking): Returning an empty string can generate invalid JS
If json_encode() fails, this returns '', and the template outputs SequraFE.regex = ; which is a syntax error. Return '{}' (or throw) on failure; consider JSON_THROW_ON_ERROR to surface encoding problems.
| $jsonEncoded = json_encode($regex); | |
| return $jsonEncoded === false ? '' : $jsonEncoded; | |
| try { | |
| $jsonEncoded = json_encode($regex, JSON_THROW_ON_ERROR); | |
| } catch (\JsonException $e) { | |
| // Fallback to an empty JSON object to avoid generating invalid JavaScript | |
| return '{}'; | |
| } | |
| return $jsonEncoded; |
| promises = Promise.all([ | ||
| SequraFE.state.getData('paymentMethods') ?? api.get(configuration.getPaymentMethodsUrl.replace(encodeURIComponent('{merchantId}'), countrySettings[0].merchantId)), | ||
| SequraFE.state.getData('allAvailablePaymentMethods') ?? api.get(configuration.getAllAvailablePaymentMethodsUrl, null, SequraFE.customHeader), | ||
| ]) |
There was a problem hiding this comment.
issue (blocking): Missing auth header + wrong get() signature when fetching payment methods
ajaxService.get(url, errorCallback, customHeader) treats the 2nd argument as the error callback. This call passes no customHeader, so requests can fail if the backend requires it. Pass null (or a callback) as the 2nd arg and SequraFE.customHeader as the 3rd.
| const handleCountryChange = (value) => { | ||
| utilities.showLoader(); | ||
| api.get(configuration.getPaymentMethodsUrl.replace(encodeURIComponent('{merchantId}'), value)) | ||
| .then((methods) => { | ||
| paymentMethods = [...methods]; | ||
| document.querySelector('.sq-table-container').remove(); | ||
| document.querySelector('.sq-content-inner')?.append( | ||
| components.DataTable.create(getTableHeaders(), getTableRows()) | ||
| ) | ||
| }) | ||
| .finally(utilities.hideLoader); |
There was a problem hiding this comment.
issue (blocking): Missing custom header when changing country
This request omits SequraFE.customHeader (and passes the URL as the only argument, so the 2nd arg is treated as errorCallback). Use api.get(url, null, SequraFE.customHeader) to keep the request consistent with other authenticated calls.
| size: 'medium', | ||
| className: '', | ||
| onClick: handleReRegister, | ||
| label: 'Re-register webhooks' |
There was a problem hiding this comment.
issue: Button label isn’t translatable
createButton() treats label as a translation key. Using a literal string here prevents localization (and bypasses the existing connection.webhookReRegistration.title key). Use a translation key for the button label.
| label: 'Re-register webhooks' | |
| label: 'connection.webhookReRegistration.title' |
| SequraFE.responseService.errorHandler( | ||
| {errorMessage: error} |
There was a problem hiding this comment.
issue: Error handling passes a non-string into errorHandler
In the catch block, { errorMessage: error } passes an Error/object, which will typically render as [object Object] and won’t translate. Pass a translation key (or error.message) or just forward the backend error shape returned by ajaxService (often contains errorCode/errorMessage).
| SequraFE.responseService.errorHandler( | |
| {errorMessage: error} | |
| const errorMessage = | |
| error && typeof error === 'object' | |
| ? (error.errorMessage || error.message || 'connection.webhookReRegistration.errorMessage') | |
| : (error || 'connection.webhookReRegistration.errorMessage'); | |
| SequraFE.responseService.errorHandler( | |
| {errorMessage: errorMessage} |
| "prefer-stable": true, | ||
| "require": { | ||
| "php": ">=7.4 <8.5", | ||
| "sequra/integration-core": "dev-feature/LIS-90", |
There was a problem hiding this comment.
issue: Composer dependency points to a dev branch
Depending on sequra/integration-core: dev-feature/LIS-90 makes builds non-reproducible and can unexpectedly change as the branch moves. Prefer a tagged release (or a fixed commit reference) before merging to reduce deployment risk.
| "sequra/integration-core": "dev-feature/LIS-90", | |
| "sequra/integration-core": "dev-feature/LIS-90#abcdef1234567890abcdef1234567890abcdef12", |
What is the goal?
References
How is it being implemented?
How is it tested?