Skip to content

Add button for webhook re-registration on the Connection page#11

Merged
tamaralogeecom merged 14 commits into
masterfrom
feature/LIS-90
Mar 17, 2026
Merged

Add button for webhook re-registration on the Connection page#11
tamaralogeecom merged 14 commits into
masterfrom
feature/LIS-90

Conversation

@tamaralogeecom

Copy link
Copy Markdown
Collaborator

What is the goal?

  • Add a button for webhook re-registration on the Connection page.

References

How is it being implemented?

  • Added an action bar in ElementGenerator to allow multiple buttons to be displayed in the same row
  • Added buttons for webhook re-registration and disconnection using the new ElementGenerator method
  • Added a success response handler in ResponseService to process successful responses
  • Implemented the logic to handle webhook re-registration

How is it tested?

  • Manual tests
    • Follow the README instructions for building the resources
    • Follow the README instructions for copying resources into the Shopify integration
    • Install the SeQura Marketing app in Shopify
    • Check the look and feel on the Connection settings page

@m1k3lm m1k3lm requested a review from Copilot March 8, 2026 09:50
@m1k3lm m1k3lm marked this pull request as ready for review March 8, 2026 09:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds UI support on the Connection settings page to trigger webhook re-registration, including new layout helpers and localized messaging, and commits the corresponding built dist/ artifacts.

Changes:

  • Add a new “actions bar” layout helper to place multiple buttons on the same row (and style it).
  • Add a webhook re-registration button and request/payload handling on the Connection settings form, with success/error flash messaging.
  • Add new i18n strings for webhook re-registration across supported languages (and mirrored dist/ updates).

Reviewed changes

Copilot reviewed 13 out of 26 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/services/ResponseService.js Adds a new successHandler for rendering success flash messages.
src/services/ElementGenerator.js Adds createActionsBar helper and exports it for UI composition.
src/forms/ConnectionSettingsForm.js Renders the new re-register + disconnect buttons row and implements re-registration call/payload.
src/design/components/field-wrapper.scss Styles the new .sqm--actions-bar container (flex + gap).
src/lang/en.json Adds webhook re-registration title/success/error strings.
src/lang/es.json Adds webhook re-registration title/success/error strings.
src/lang/fr.json Adds webhook re-registration title/success/error strings.
src/lang/it.json Adds webhook re-registration title/success/error strings.
src/lang/pt.json Adds webhook re-registration title/success/error strings.
src/forms/GeneralSettingsForm.js Minor formatting/semicolon consistency adjustments.
src/controllers/StateController.js Minor formatting/semicolon consistency adjustment.
src/controllers/OnboardingController.js Removes an unused parameter/doc entry for widget settings render function.
src/controllers/AdvancedController.js Minor formatting/semicolon consistency adjustment.
dist/resources/js/ResponseService.js Built artifact mirroring src/services/ResponseService.js changes.
dist/resources/js/ElementGenerator.js Built artifact mirroring src/services/ElementGenerator.js changes.
dist/resources/js/ConnectionSettingsForm.js Built artifact mirroring src/forms/ConnectionSettingsForm.js changes.
dist/resources/js/GeneralSettingsForm.js Built artifact mirroring src/forms/GeneralSettingsForm.js changes.
dist/resources/js/StateController.js Built artifact mirroring src/controllers/StateController.js changes.
dist/resources/js/OnboardingController.js Built artifact mirroring src/controllers/OnboardingController.js changes.
dist/resources/js/AdvancedController.js Built artifact mirroring src/controllers/AdvancedController.js changes.
dist/resources/lang/en.json Built artifact mirroring src/lang/en.json changes.
dist/resources/lang/es.json Built artifact mirroring src/lang/es.json changes.
dist/resources/lang/fr.json Built artifact mirroring src/lang/fr.json changes.
dist/resources/lang/it.json Built artifact mirroring src/lang/it.json changes.
dist/resources/lang/pt.json Built artifact mirroring src/lang/pt.json changes.
dist/resources/css/sequra-core.css Built artifact mirroring the SCSS actions bar styling changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +55 to +60
if (response.successMessage) {
container.prepend(utilities.createFlashMessage(response.successMessage, 'success'));
}

return Promise.reject(response);
};

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

issue (blocking): successHandler rejects the promise even on success
Returning Promise.reject(...) here makes every call site treat a successful response as an error (and can create unhandled rejection warnings if callers don’t .catch). Return a resolved promise (or no promise) after rendering the flash message, and update the JSDoc accordingly.

Copilot uses AI. Check for mistakes.
size: 'medium',
className: '',
onClick: handleReRegister,
label: 'Re-register webhooks'

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

issue (blocking): Button label bypasses i18n keys
createButton expects a translation key (it runs the label through translationService.translate). Using the hard-coded string 'Re-register webhooks' means the new connection.webhookReRegistration.title translations won’t be used and non-English locales will still show English. Use the translation key instead (and keep button labels consistently key-based).

Suggested change
label: 'Re-register webhooks'
label: 'connection.webhookReRegistration.title'

Copilot uses AI. Check for mistakes.
Comment thread src/forms/ConnectionSettingsForm.js Outdated
Comment on lines +603 to +604
SequraFE.responseService.errorHandler(
{errorMessage: error}

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

issue (blocking): Passing a non-string error object to errorHandler can throw
In the .catch, error is likely an object (e.g., the JSON error body or a thrown Error). Wrapping it as { errorMessage: error } causes createFlashMessage to eventually call .split on a non-string, which will throw and prevent any feedback from being shown. Pass through the original error shape if it already has errorCode/errorMessage, or map to a safe string key like general.errors.unknown (and optionally console.error(error) for debugging).

Suggested change
SequraFE.responseService.errorHandler(
{errorMessage: error}
// Log raw error for debugging while keeping user-facing message safe.
if (typeof console !== 'undefined' && typeof console.error === 'function') {
console.error(error);
}
const normalizedError =
error &&
typeof error === 'object' &&
(typeof error.errorMessage === 'string' || typeof error.errorCode === 'string')
? error
: { errorMessage: 'general.errors.unknown' };
SequraFE.responseService.errorHandler(
normalizedError

Copilot uses AI. Check for mistakes.
Comment on lines +629 to +631
const username = getSettingsForActiveDeployment(changedSettings).username ?? '';
const password = getSettingsForActiveDeployment(changedSettings).password ?? '';
const deployment = getSettingsForActiveDeployment(changedSettings).deployment ?? '';

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

suggestion (non-blocking): Avoid repeated getSettingsForActiveDeployment calls
createReRegisterPayload calls getSettingsForActiveDeployment(changedSettings) three times. Store the result once (e.g., const active = ...) to reduce duplication and keep the payload construction easier to maintain.

Suggested change
const username = getSettingsForActiveDeployment(changedSettings).username ?? '';
const password = getSettingsForActiveDeployment(changedSettings).password ?? '';
const deployment = getSettingsForActiveDeployment(changedSettings).deployment ?? '';
const activeSettings = getSettingsForActiveDeployment(changedSettings);
const username = activeSettings.username ?? '';
const password = activeSettings.password ?? '';
const deployment = activeSettings.deployment ?? '';

Copilot uses AI. Check for mistakes.
Comment on lines +445 to +448
* @returns HTMLElement
*/
const createActionsBar = (className, children) => {
const fieldWrapper = createElement('div', 'sq-field-wrapper ' + className)

Copilot AI Mar 8, 2026

Copy link

Choose a reason for hiding this comment

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

suggestion (non-blocking): JSDoc and semicolon consistency for createActionsBar
createActionsBar’s JSDoc omits parameter descriptions/types (className, children), and the fieldWrapper assignment is missing a trailing semicolon, which is inconsistent with the repo’s Prettier config (semi: true). Adding the params to the doc and formatting this block will keep ElementGenerator consistent with the rest of the helpers.

Suggested change
* @returns HTMLElement
*/
const createActionsBar = (className, children) => {
const fieldWrapper = createElement('div', 'sq-field-wrapper ' + className)
* @param {string} className Additional CSS classes for the wrapper element.
* @param {HTMLElement[]} children Child elements (typically action buttons) to include in the bar.
* @returns {HTMLElement}
*/
const createActionsBar = (className, children) => {
const fieldWrapper = createElement('div', 'sq-field-wrapper ' + className);

Copilot uses AI. Check for mistakes.
@m1k3lm

m1k3lm commented Mar 13, 2026

Copy link
Copy Markdown
Member

@tamaralogeecom @AleksandarBoljanovic could you review copilot's comments?

@tamaralogeecom tamaralogeecom merged commit 7b22234 into master Mar 17, 2026
4 checks passed
@tamaralogeecom tamaralogeecom deleted the feature/LIS-90 branch March 17, 2026 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants