-
Notifications
You must be signed in to change notification settings - Fork 0
Replace URL placeholder even if it is encoded #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Replace URL placeholder even if it is encoded #12
Conversation
…ethod across controllers and services
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request introduces a URL placeholder replacement utility method and refactors the codebase to standardize URL parameter handling across controllers. The change aims to handle both plain and encoded URL placeholders consistently while reducing code duplication.
Key changes:
- Added
sqReplaceUrlPlaceholdermethod toString.prototypefor replacing URL placeholders with encoded values - Refactored URL construction throughout
PaymentController.js,SettingsController.js, andStateController.jsto use the new utility method - Added a pull request template file for standardizing PR submissions
Reviewed changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/UtilityService.js | Implements the new sqReplaceUrlPlaceholder method as a String prototype extension |
| dist/resources/js/UtilityService.js | Distribution version of the utility service with the same implementation |
| src/controllers/StateController.js | Refactors 10 URL construction instances to use the new utility method |
| dist/resources/js/StateController.js | Distribution version with matching refactoring changes |
| src/controllers/SettingsController.js | Updates 2 URL placeholder replacements to use the new utility method |
| dist/resources/js/SettingsController.js | Distribution version with matching updates |
| src/controllers/PaymentController.js | Converts 2 URL construction calls to use the new utility method |
| dist/resources/js/PaymentController.js | Distribution version with matching conversions |
| .github/PULL_REQUEST_TEMPLATE.md | Adds standardized PR template for team consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!String.prototype.sqReplaceUrlPlaceholder) { | ||
| Object.assign(String.prototype, { | ||
| sqReplaceUrlPlaceholder(placeholder, value) { | ||
| const placeholderStr = String(placeholder); | ||
| const valueStr = encodeURIComponent(String(value)); | ||
| const encodedPlaceholder = encodeURIComponent(placeholderStr); | ||
| return this.replaceAll(placeholderStr, valueStr).replaceAll(encodedPlaceholder, valueStr); | ||
| } | ||
| }); | ||
| } |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): Consider alternative to extending String.prototype
Extending built-in prototypes like String.prototype is generally discouraged in JavaScript as it can lead to conflicts with other libraries, future JavaScript features, or cause unexpected behavior when iterating over object properties. Consider implementing this as a standalone utility function instead, such as:
SequraFE.utilities.replaceUrlPlaceholder = function(url, placeholder, value) {
const placeholderStr = String(placeholder);
const valueStr = encodeURIComponent(String(value));
const encodedPlaceholder = encodeURIComponent(placeholderStr);
return url.replaceAll(placeholderStr, valueStr).replaceAll(encodedPlaceholder, valueStr);
};This approach is safer and more maintainable.
| let config = utilities.cloneObject(configuration.pageConfiguration[controllerName] || {}); | ||
| Object.keys(config).forEach((key) => { | ||
| config[key] = config[key].replace('{storeId}', encodeURIComponent(this.getStoreId())); | ||
| config[key] = config[key].sqReplaceUrlPlaceholder('{storeId}', this.getStoreId()); |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (blocking): Potential runtime error when config[key] is not a string
This code assumes all values in the config object have the sqReplaceUrlPlaceholder method, but config may contain non-string values (like the page property added on line 372). This will cause a runtime error if config[key] is not a string.
Add a type check before calling the method to ensure the value is a string.
| config[key] = config[key].sqReplaceUrlPlaceholder('{storeId}', this.getStoreId()); | |
| if (typeof config[key] === 'string' && typeof config[key].sqReplaceUrlPlaceholder === 'function') { | |
| config[key] = config[key].sqReplaceUrlPlaceholder('{storeId}', this.getStoreId()); | |
| } |
| sqReplaceUrlPlaceholder(placeholder, value) { | ||
| const placeholderStr = String(placeholder); | ||
| const valueStr = encodeURIComponent(String(value)); | ||
| const encodedPlaceholder = encodeURIComponent(placeholderStr); | ||
| return this.replaceAll(placeholderStr, valueStr).replaceAll(encodedPlaceholder, valueStr); | ||
| } |
Copilot
AI
Dec 31, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (blocking): Missing JSDoc documentation for new prototype method
The sqReplaceUrlPlaceholder method should include JSDoc documentation to describe its parameters, return value, and usage. This is especially important for methods added to built-in prototypes, as they become part of the global API.
Add documentation explaining:
- The placeholder parameter (the placeholder string to replace)
- The value parameter (the value to replace it with)
- The return value (the string with replacements made)
- That it handles both plain and encoded placeholders
What is the goal?
Provide a URL placeholder replacement mechanism to use across controllers and services supporting encoded and not encoded values.
How is it being implemented?
This pull request introduces a utility method to safely replace URL placeholders and refactors the codebase to use this new method throughout various controllers. The main goal is to standardize and improve the way URL parameters (like
{storeId}or{merchantId}) are replaced, ensuring proper encoding and reducing code duplication.Key changes:
Utility method introduction:
sqReplaceUrlPlaceholderto theString.prototypeinUtilityService.js, which replaces placeholders in URLs with encoded values, handling both plain and encoded placeholders.Refactoring controllers to use the new utility:
.replace('{storeId}', value)or similar patterns inPaymentController.js,SettingsController.js, andStateController.jsto use.sqReplaceUrlPlaceholder(...)instead, ensuring safer and more consistent URL handling. [1] [2] [3] [4] [5] [6] [7] [8] [9]Opportunistic refactorings
Add PR template file.
How is it tested?
Manual tests
How is it going to be deployed?
Standard deployment