-
Notifications
You must be signed in to change notification settings - Fork 0
Add suport for sms #21
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
Conversation
Introduced a new /admin/area/setPhone endpoint to update admin phone numbers, including input validation and sanitization. Improved phone validation logic in sendSms and utils, clarified error messages, and updated routes to register the new endpoint. Also fixed a comment in sms.ts for clarity.
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 PR adds support for sending and managing SMS messages within the admin area.
- Introduces a singleton
Sms
class for gateway integration, token management, and sending messages. - Extends utility functions for phone normalization and validation.
- Adds three new endpoints (
smsStatus
,sendSms
,setPhone
), updates routes, tests, model, and docs.
Reviewed Changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
tools/utils.ts | Updated phone validation logic |
tools/sms.ts | New Sms class handling SMS gateway interactions |
router/admin/area/smsStatus.ts | Implement /admin/area/smsStatus endpoint |
router/admin/area/sendSms.ts | Implement /admin/area/sendSms endpoint |
router/admin/area/setPhone.ts | Implement /admin/area/setPhone endpoint |
routes.ts | Registered new SMS-related routes |
Models/Area.ts | Added adminPhone field to Area schema |
README.md | Added versioning section |
Comments suppressed due to low confidence (1)
tools/sms.ts:10
- The property name 'gatway' appears to be a typo. Rename it to 'gateway' for clarity and consistency.
gatway: string;
tools/utils.ts
Outdated
@@ -57,6 +57,8 @@ function phoneNumberCheck(phone: string): boolean { | |||
if (typeof phone != 'string') return false; | |||
if (!phone.startsWith('+')) return false; | |||
|
|||
phone = phone.replace(' ', ''); |
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.
Consider using replaceAll or a global regex (e.g., phone = phone.replace(/\s/g, '')) to remove all whitespace instead of only the first occurrence.
phone = phone.replace(' ', ''); | |
phone = phone.replaceAll(' ', ''); |
Copilot uses AI. Check for mistakes.
//unactivate self-signed certificate by default if slef-signed is true | ||
this.selfSigned = process.env.SMS_SELFSIGNED === 'true' ? false : true; |
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.
[nitpick] There's a typo in the comment ('slef-signed'). Also, the logic and comment are confusing; consider clarifying the intended behavior for self-signed certificate handling.
//unactivate self-signed certificate by default if slef-signed is true | |
this.selfSigned = process.env.SMS_SELFSIGNED === 'true' ? false : true; | |
// Disable self-signed certificate validation by default. If SMS_SELFSIGNED is 'true', allow self-signed certificates. | |
this.selfSigned = process.env.SMS_SELFSIGNED === 'true' ? true : false; |
Copilot uses AI. Check for mistakes.
this.enabled = true; | ||
return true; | ||
} else { | ||
this.enabled = true; |
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.
In the else branch, the gateway responded but didn't return the expected text; enabling SMS here may be incorrect. Consider setting enabled to false when the response text is unexpected.
this.enabled = true; | |
this.enabled = false; |
Copilot uses AI. Check for mistakes.
router/admin/area/sendSms.ts
Outdated
} | ||
|
||
let errored = false; | ||
req.body.phone.map((phone: [string, string | undefined]) => { |
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.
[nitpick] Using map purely for side effects can be confusing. Consider using forEach or a simple for loop for validation instead.
req.body.phone.map((phone: [string, string | undefined]) => { | |
req.body.phone.forEach((phone: [string, string | undefined]) => { |
Copilot uses AI. Check for mistakes.
return; | ||
} | ||
|
||
const service = sms.enabled ? (sms.isSmsTools ? 'sms-tools' : 'sms-gateway') : 'inactif'; |
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.
[nitpick] The fallback service label 'inactif' seems to mix French and English. Consider using 'inactive' for consistency.
const service = sms.enabled ? (sms.isSmsTools ? 'sms-tools' : 'sms-gateway') : 'inactif'; | |
const service = sms.enabled ? (sms.isSmsTools ? 'sms-tools' : 'sms-gateway') : 'inactive'; |
Copilot uses AI. Check for mistakes.
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.
no the language of application is French for all client data
README.md
Outdated
@@ -39,3 +39,10 @@ or | |||
```bash | |||
npm run test file | |||
``` | |||
|
|||
## versionning |
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.
There's a typo in the heading. 'versionning' should be 'versioning'.
## versionning | |
## versioning |
Copilot uses AI. Check for mistakes.
20b9615
to
55c824d
Compare
## Pull Request Overview This PR adds support for sending and managing SMS messages within the admin area. - Introduces a singleton `Sms` class for gateway integration, token management, and sending messages. - Extends utility functions for phone normalization and validation. - Adds three new endpoints (`smsStatus`, `sendSms`, `setPhone`), updates routes, tests, model, and docs. ### Reviewed Changes Copilot reviewed 20 out of 22 changed files in this pull request and generated 6 comments. <details> <summary>Show a summary per file</summary> | File | Description | |------------------------------------|-------------------------------------------------------| | tools/utils.ts | Updated phone validation logic | | tools/sms.ts | New `Sms` class handling SMS gateway interactions | | router/admin/area/smsStatus.ts | Implement `/admin/area/smsStatus` endpoint | | router/admin/area/sendSms.ts | Implement `/admin/area/sendSms` endpoint | | router/admin/area/setPhone.ts | Implement `/admin/area/setPhone` endpoint | | routes.ts | Registered new SMS-related routes | | Models/Area.ts | Added `adminPhone` field to `Area` schema | | README.md | Added versioning section | </details> <details> <summary>Comments suppressed due to low confidence (1)</summary> **tools/sms.ts:10** * The property name 'gatway' appears to be a typo. Rename it to 'gateway' for clarity and consistency. ``` gatway: string; ``` </details>
Pull Request Overview
This PR adds support for sending and managing SMS messages within the admin area.
Sms
class for gateway integration, token management, and sending messages.smsStatus
,sendSms
,setPhone
), updates routes, tests, model, and docs.Reviewed Changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
Sms
class handling SMS gateway interactions/admin/area/smsStatus
endpoint/admin/area/sendSms
endpoint/admin/area/setPhone
endpointadminPhone
field toArea
schemaComments suppressed due to low confidence (1)
tools/sms.ts:10