Webhook event tracking subscription email #355
Conversation
WalkthroughThis pull request introduces comprehensive changes to integrate SendGrid email services into the project. The modifications span multiple files, including Changes
Sequence DiagramsequenceDiagram
participant User
participant NewsletterForm
participant WordPressAPI
participant SendGrid
User->>NewsletterForm: Submits email and zipcode
NewsletterForm->>WordPressAPI: POST /newsletter/signUp
WordPressAPI->>SendGrid: Add contact to list
SendGrid-->>WordPressAPI: Confirm contact added
WordPressAPI-->>NewsletterForm: Return success
NewsletterForm->>User: Display success message
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: 10
🔭 Outside diff range comments (1)
wp-content/themes/workingnyc/views/newsletter.twig (1)
Line range hint
28-90: Add CSRF protection to the formThe form lacks CSRF protection, which is essential for security.
Add a CSRF token to protect against cross-site request forgery attacks:
<form action="https://api.sendgrid.com/v3/marketing/contacts" id="mc-embedded-subscribe-form" method="post" target="_blank"> + <input type="hidden" name="_csrf_token" value="{{ csrf_token() }}"> <div class="flex flex-col" data-js="form-fields">
🧹 Nitpick comments (4)
wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js (3)
56-56: Remove redundantevent.preventDefault()call in_submitmethodThe
event.preventDefault()is called both in the form'ssubmithandler and inside the_submitmethod. Since it's already called in thesubmithandler, the call inside_submitis redundant and can be removed to avoid confusion.// Inside the _submit method, remove the redundant event.preventDefault() _submit(event) { - event.preventDefault(); // Serialize the data this._data = serialize(event.target, {hash: true}); // ... }Also applies to: 80-80
80-80: Simplify URL construction by removing unuseddomainvariableThe
domainvariable is set to an empty string and is not utilized meaningfully in the URL construction. Concatenating an empty string has no effect. Simplify the code by removing thedomainvariable and directly constructing the action URL.- let domain= ''; // ... // Adjust the action URL - action = domain + action.replace("addUser&","addUser?"); + action = action.replace("addUser&", "addUser?"); // Fetch the URL - fetch(`${domain}${action}`) + fetch(`${action}`)Also applies to: 93-93, 100-100
93-93: UseURLSearchParamsfor robust query parameter handlingThe current method of replacing
"addUser&"with"addUser?"is error-prone and may not handle query parameters correctly in all cases. UtilizeURLSearchParamsto construct the query string safely and reliably.// Instead of manually manipulating the query string, use URLSearchParams + let params = new URLSearchParams(new FormData(event.target)); + let action = `/wp-json/api/v1/addUser?${params.toString()}`; // Fetch the URL - fetch(`${action}`) + fetch(action)wp-content/mu-plugins/register-rest-routes.php (1)
313-325: Refactor configuration loading to reduce code duplicationThe code for loading SendGrid configuration values from the YAML file is duplicated in both the
/confirmation-events/and/addUser/endpoints. Refactor this into a separate function to improve maintainability.+ function load_sendgrid_config() { + $path = ABSPATH . 'wp-content/mu-plugins/config/config.yml'; + if (file_exists($path)) { + $config_val = Spyc::YAMLLoad($path); + return $config_val['sendgrid']; + } + return null; + } // In your endpoints: - // Existing code to load config + $sendgrid_values = load_sendgrid_config(); + if ($sendgrid_values) { + // Use $sendgrid_values['SENDGRID_API_KEY'], etc. + } else { + return new WP_REST_Response('SendGrid configuration not found', 500); + }Also applies to: 417-429
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
composer.json(1 hunks)vendor/composer/autoload_classmap.php(3 hunks)vendor/composer/autoload_files.php(1 hunks)vendor/composer/autoload_psr4.php(1 hunks)vendor/composer/autoload_static.php(6 hunks)vendor/composer/installed.json(2 hunks)vendor/composer/installed.php(4 hunks)wp-content/mu-plugins/register-rest-routes.php(2 hunks)wp-content/themes/workingnyc/assets/js/newsletter-24761d3.js(4 hunks)wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js(4 hunks)wp-content/themes/workingnyc/template-newsletter-confirmation-page.php(1 hunks)wp-content/themes/workingnyc/views/newsletter-confirmation-page.twig(1 hunks)wp-content/themes/workingnyc/views/newsletter.twig(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- wp-content/themes/workingnyc/views/newsletter-confirmation-page.twig
🔇 Additional comments (13)
wp-content/themes/workingnyc/assets/js/newsletter-24761d3.js (1)
Line range hint 1-1: Ensure that the minified JS file is up-to-date with source changes
The file newsletter-24761d3.js appears to be a build artifact. Verify that it reflects the latest changes made in newsletter-helper.js. Keeping build artifacts updated is crucial for consistent application behavior.
You can run the build process to regenerate this file and ensure it's up-to-date.
wp-content/themes/workingnyc/template-newsletter-confirmation-page.php (1)
15-31: LGTM!
The template file template-newsletter-confirmation-page.php correctly sets up the context and renders the newsletter-confirmation-page.twig template. Ensure that the corresponding Twig template exists and is correctly implemented.
vendor/composer/autoload_psr4.php (1)
19-24: LGTM! PSR-4 autoloading configuration is correct
The SendGrid namespace mappings are properly configured with appropriate directory paths for each component.
vendor/composer/autoload_files.php (1)
17-17: LGTM! ECDSA library autoloading for webhook security
The ECDSA library is correctly configured for autoloading, which is essential for verifying SendGrid webhook signatures.
✅ Verification successful
LGTM! ECDSA library autoloading is correctly configured
The ECDSA library from starkbank is properly configured for autoloading as part of the SendGrid dependency (v8.1), which is required for webhook signature verification. The library files are correctly registered in the autoloader configuration files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if webhook signature verification is implemented
rg -A 5 'EventWebhook\\(Verifier|SignatureVerifier)'
Length of output: 54
Script:
#!/bin/bash
# Search for SendGrid webhook verification related code with broader patterns
rg -i "webhook.*signature|signature.*verif|sendgrid.*webhook" -A 5
# Also check for ECDSA usage
rg -i "ecdsa|ellipticcurve" -A 5
# Look for files that might contain webhook handling
fd -t f -e php webhook
Length of output: 66004
Script:
#!/bin/bash
# Look for SendGrid webhook verification related files
fd -t f -e php sendgrid
# Search for ECDSA library usage
rg -l "starkbank.ecdsa"
# Check composer.json for SendGrid dependencies
cat composer.json
Length of output: 2796
vendor/composer/installed.php (2)
208-231: Verify SendGrid package version compatibility
The SendGrid package (v8.1.2) and its HTTP client (v4.1.1) require PHP >=7.3. Ensure your production environment meets these requirements.
Run this script to check PHP version compatibility:
✅ Verification successful
SendGrid package version requirements are compatible with project dependencies
The verification shows that:
- SendGrid (v8.1.2) requires PHP >= 7.3
- SendGrid's HTTP client (v4.1.1) requires PHP >= 7.3
- The project's dependencies include packages requiring PHP 8.0.2 or higher
Since the project already requires PHP 8.0.2 for several dependencies, it meets SendGrid's minimum PHP version requirement of 7.3. The package versions are compatible with the project's environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check PHP version and SendGrid compatibility
php -v | grep -Po '(?<=PHP )\d+\.\d+\.\d+'
Length of output: 138
Script:
#!/bin/bash
# Let's check composer.json for PHP version requirements
cat composer.json | grep -A 5 "require"
# Also check if there are any PHP version constraints in composer.lock
cat composer.lock | grep -A 2 '"php":'
Length of output: 3281
Line range hint 1373-1381: Review SendGrid package security requirements
The SendGrid package requires several PHP extensions. Ensure these are properly configured in production:
- ext-curl
- ext-json
- ext-mbstring
- ext-openssl
✅ Verification successful
Required PHP extensions are properly configured through Composer dependencies
Based on the analysis of composer.json and related files, the SendGrid package requirements are properly handled through Composer's dependency management:
- The SendGrid package is installed via Composer:
"sendgrid/sendgrid": "^8.1" - All required PHP extensions are declared in the package's requirements:
"require": { "ext-curl": "*", "ext-json": "*", "ext-mbstring": "*", "ext-openssl": "*" }
- The package is actively used in the codebase for email functionality, as evidenced by the SendGrid implementation in
wp-content/mu-plugins/register-rest-routes.php
Since Composer handles dependency resolution and will fail installation if required extensions are missing, no additional configuration is needed beyond what Composer already manages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify required PHP extensions
php -m | grep -E 'curl|json|mbstring|openssl'
Length of output: 141
Script:
#!/bin/bash
# Check composer.json for required PHP extensions
cat composer.json | grep -A 10 "require"
# Check if there are any PHP configuration files
fd -e php -e json . | grep -i "config"
# Look for SendGrid related files and configurations
rg -i "sendgrid" -A 5 -B 5
Length of output: 68253
vendor/composer/installed.json (1)
1293-1425: LGTM: Package configuration is consistent
The SendGrid package configuration in installed.json matches the changes in installed.php.
vendor/composer/autoload_classmap.php (2)
11-11: SendGrid integration looks properly configured.
The autoload mappings for SendGrid classes are correctly structured and include all necessary components for email functionality:
- Core SendGrid classes
- Contact management
- Event webhook handling
- Mail composition and delivery
- Twilio email integration
The class mappings follow Composer's autoloading conventions and provide a complete set of SendGrid functionality.
Also applies to: 454-504, 2797-2797
454-504: Verify SendGrid version compatibility.
The extensive set of SendGrid mail-related classes being added suggests this is a major integration. Please ensure the SendGrid library version (^8.1) specified in composer.json is compatible with your PHP version and other dependencies.
vendor/composer/autoload_static.php (4)
40-45: SendGrid namespace registration looks correct
The SendGrid namespaces are properly registered in the PSR-4 prefix lengths array with appropriate namespace lengths.
110-133: SendGrid PSR-4 directory mappings are properly configured
The directory mappings for SendGrid components are correctly set up to point to their respective locations under the vendor directory.
638-688: SendGrid class mappings are complete and consistent
All required SendGrid classes are properly mapped in the classMap array, including:
- Core SendGrid classes
- Mail related classes
- Contact management classes
- Event webhook classes
- Helper classes
18-18: Dependencies are properly configured
The additional dependencies are correctly registered:
- starkbank/ecdsa library file is mapped
- TwilioEmail class is mapped
The changes maintain consistency with the existing autoloader structure.
Also applies to: 2981-2981
| if (data == 'success') { | ||
| this._success(data); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for unsuccessful responses in _callback method
Currently, the _callback method only handles the success case. To enhance user experience, implement error handling to provide feedback when the response indicates failure.
_callback(data) {
if (data == 'success') {
this._success(data);
+ } else {
+ this._error('Subscription failed. Please verify your information and try again.');
}
return this;
}Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| fetch(`${domain}${action}`) | ||
| .then(response => response.json()) | ||
| .then(d => { | ||
| window[this.callback](d); | ||
| }) | ||
| .catch(error => { | ||
| console.error('Error:', error); | ||
| throw error; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle errors gracefully in the fetch call
Throwing an error inside the catch block without proper handling can lead to unhandled promise rejections. Instead, invoke the _error method to provide user feedback and maintain application stability.
fetch(`${action}`)
.then(response => response.json())
.then(d => {
window[this.callback](d);
})
.catch(error => {
console.error('Error:', error);
- throw error;
+ this._error('An error occurred while processing your request. Please try again later.');
});| return new WP_REST_Response($e->getMessage(), 500); | ||
| } |
There was a problem hiding this comment.
Fix incorrect exception variable in error handling
The catch blocks capture the exception as $ex, but the error message references $e, which is undefined. This will result in an undefined variable error.
} catch (Exception $ex) {
- return new WP_REST_Response($e->getMessage(), 500);
+ return new WP_REST_Response($ex->getMessage(), 500);
}Also applies to: 471-472
| $email_address = $request->get_param('EMAIL'); | ||
| $zipcode = $request->get_param('ZIPCODE'); |
There was a problem hiding this comment.
Validate and sanitize user input from request parameters
The EMAIL and ZIPCODE parameters are used directly without validation. This can lead to security vulnerabilities like SQL injection or email header injection. Ensure that all user inputs are properly sanitized and validated.
$email_address = $request->get_param('EMAIL');
$zipcode = $request->get_param('ZIPCODE');
+
+ // Sanitize and validate inputs
+ $email_address = sanitize_email($email_address);
+ if (!is_email($email_address)) {
+ return new WP_REST_Response('Invalid email address', 400);
+ }
+ $zipcode = sanitize_text_field($zipcode);📝 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.
| $email_address = $request->get_param('EMAIL'); | |
| $zipcode = $request->get_param('ZIPCODE'); | |
| $email_address = $request->get_param('EMAIL'); | |
| $zipcode = $request->get_param('ZIPCODE'); | |
| // Sanitize and validate inputs | |
| $email_address = sanitize_email($email_address); | |
| if (!is_email($email_address)) { | |
| return new WP_REST_Response('Invalid email address', 400); | |
| } | |
| $zipcode = sanitize_text_field($zipcode); |
There was a problem hiding this comment.
- @mithunpandeti do this
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| 'methods' => 'GET', | ||
| 'permission_callback' => '__return_true', |
There was a problem hiding this comment.
Change HTTP method to POST for /addUser/ endpoint
The /addUser/ endpoint currently uses the GET method to handle user data submission, including sensitive information like email addresses. It's more secure and appropriate to use the POST method for such operations.
register_rest_route('api/v1', '/addUser/', array(
- 'methods' => 'GET',
+ 'methods' => 'POST',Ensure that the frontend code is updated accordingly to send a POST request.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
- @mithunpandeti Convert to POST
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/mu-plugins/register-rest-routes.php(2 hunks)wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js
🔇 Additional comments (4)
wp-content/mu-plugins/register-rest-routes.php (4)
300-302: Implement webhook authentication
The endpoint lacks proper authentication for incoming webhook requests.
405-406: Change HTTP method to POST
Using GET method for user data submission is inappropriate.
431-432: Validate and sanitize user input
The email and zipcode parameters are used without validation.
470-472: Fix incorrect exception variable
The catch block uses $e instead of $ex.
| $sendgrid_contact_list_id; | ||
|
|
||
| //Fetch the Sendgrid Configuration values from config file | ||
| if (file_exists($path)) { |
There was a problem hiding this comment.
- You should be able to remove this and use the environment variables directly. See the Readme file here: https://github.com/CityOfNewYork/working-nyc/tree/main/wp-content/mu-plugins/wp-config
- Add all new config variables to config-sample.yml
| $exp = WEEK_IN_SECONDS; // expiration of the transient caches | ||
|
|
||
|
|
||
| register_rest_route('api/v1', '/confirmation-events/', array( |
There was a problem hiding this comment.
- Rename endpoint to
newsletter/confirm(so that it's RESTful)
| $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; | ||
| $log_file = $log_dir . '/events.log'; | ||
| $logMessage = print_r($data, true); | ||
| foreach ($data as $event) { |
There was a problem hiding this comment.
- Create a helper method for logging events, and don't log the user's email address
| $timestamp = isset($event['timestamp']) ? $event['timestamp'] : 'unknown'; | ||
| $url = isset($event['url']) ? $event['url'] : 'unknown'; | ||
| $log_data = "Event: $event_type | Email: $email | URL: $url | Timestamp: $timestamp\n"; | ||
| if($event_type=='click' && $templateName=='Test Email'){ |
There was a problem hiding this comment.
- Add SendGrid template name to config.yml
| ] | ||
| }'); | ||
| try { | ||
| //file_put_contents($log_file, 'test', FILE_APPEND); |
There was a problem hiding this comment.
- Remove commented lines
|
|
||
| if($response->statusCode() == 202){ | ||
| $sendEmail = new \SendGrid\Mail\Mail(); | ||
| $sendEmail->setFrom("[email protected]", "No Reply"); |
There was a problem hiding this comment.
- Add the from email address and subject to config.yml
| } | ||
| } | ||
| } | ||
| //$response->set_status(200); |
There was a problem hiding this comment.
- Remove commented line
| $exp = WEEK_IN_SECONDS; // expiration of the transient caches | ||
|
|
||
|
|
||
| register_rest_route('api/v1', '/addUser/', array( |
There was a problem hiding this comment.
- Rename endpoint to
newsletter/signUp
| @@ -25,7 +25,7 @@ | |||
| <section class="site-max-width"> | |||
| <div class="py-5 tablet:py-6"> | |||
| <div data-js="newsletter-form"> | |||
| <form action="https://nyc.us18.list-manage.com/subscribe/post?u={{ constant('MAILCHIMP_ACCOUNT') }}&id={{ constant('MAILCHIMP_AUDIENCE_ID') }}" id="mc-embedded-subscribe-form" method="post" target="_blank"> | |||
| <form action="https://api.sendgrid.com/v3/marketing/contacts" id="mc-embedded-subscribe-form" method="post" target="_blank"> | |||
There was a problem hiding this comment.
- Can you remove this SendGrid URL? The user will be added through the API endpoint you created right?
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
wp-content/themes/workingnyc/assets/js/newsletter-0adfb7d.js (4)
349-349: Clarify usage of the 'domain' variable.
Currently set to an empty string. If you intend to allow for multi-environment usage or dynamic domain assignment in the future, consider documenting or implementing it in this PR.
362-362: Ensure the query string replacement is resilient.
'.replace("signUp&","signUp?")' might fail if the string "signUp&" isn’t present – or if the parameters shift. Consider a more robust approach for forming querystrings (e.g., URLSearchParams).
370-379: Catch fetch response status codes.
You are parsing JSON without checking for HTTP errors (e.g., 4xx, 5xx). Consider verifying “response.ok” before parsing JSON to handle errors from the server distinctly.
415-419: Add more detailed error feedback.
The current error message is generic. If the API can return more nuanced error details (e.g., “email already exists”), consider surfacing it for better user experience.wp-content/mu-plugins/config/config-sample.yml (2)
41-48: Remove trailing spaces in commented lines.
Avoid trailing whitespace to maintain clear version history and consistent formatting.Apply this diff:
-# SENDGRID_API_KEY: -# SENDGRID_CONFIRMATION_TEMPATE_ID: -# SENDGRID_CONFIRMED_TEMPLATE_ID: -# SENDGRID_LIST_ID: -# SENDGRID_SENDGER_EMAIL_ADDRESS: -# SENGRID_SENDER_NAME: -# SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT: -# SENDGRID_SUBSCRIPTION_CONFIRM_SUBJECT: +# SENDGRID_API_KEY: +# SENDGRID_CONFIRMATION_TEMPATE_ID: +# SENDGRID_CONFIRMED_TEMPLATE_ID: +# SENDGRID_LIST_ID: +# SENDGRID_SENDGER_EMAIL_ADDRESS: +# SENGRID_SENDER_NAME: +# SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT: +# SENDGRID_SUBSCRIPTION_CONFIRM_SUBJECT:🧰 Tools
🪛 yamllint (1.35.1)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
49-49: Add a newline at the end of the file.
As flagged by static analysis, ensure each file concludes with a newline character to comply with typical POSIX standards and avoid warnings.🧰 Tools
🪛 yamllint (1.35.1)
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
[error] 49-49: trailing spaces
(trailing-spaces)
wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js (3)
56-56: Manage form submission flow with meaningful naming.
Renaming “_submit” to “submitForm” or similar can improve readability, matching common naming patterns.
80-80: Document “domain” property usage.
Like in “newsletter-0adfb7d.js,” domain is empty below. Document if it’s a placeholder for future or environment-based usage.
152-152: Consider localized messages for subscription errors.
Currently displaying a generic string. If the project supports multiple languages, use a localized message or a string reference fromNewsletter.strings.wp-content/mu-plugins/register-rest-routes.php (1)
323-323: Protect subscription-moving logic with additional authentication.
Marking a user as “confirmed” in SendGrid is a sensitive step. Ensure these events come exclusively from SendGrid by validating signatures, IP addresses, or keys.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wp-content/mu-plugins/config/config-sample.yml(1 hunks)wp-content/mu-plugins/register-rest-routes.php(2 hunks)wp-content/themes/workingnyc/assets/js/newsletter-0adfb7d.js(4 hunks)wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js(4 hunks)wp-content/themes/workingnyc/views/newsletter.twig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/themes/workingnyc/views/newsletter.twig
🧰 Additional context used
🪛 yamllint (1.35.1)
wp-content/mu-plugins/config/config-sample.yml
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
[error] 49-49: trailing spaces
(trailing-spaces)
🔇 Additional comments (5)
wp-content/themes/workingnyc/assets/js/newsletter-0adfb7d.js (2)
353-353: Check for trailing slash or missing slash in the endpoint.
To avoid potential concatenation errors, confirm that your endpoints always contain the needed slash ("/wp-json/...").
325-325: Use optional chaining or null checks to avoid potential null reference errors.
If “event” is unexpectedly null, accessing “event.target” could throw an error.
wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js (2)
148-149: Duplicate error handling comment
Similar to a past review comment: ensure _callback handles secondary errors or partial failures as well.
84-84: Ensure endpoint consistency.
“/wp-json/api/v1/newsletter/signUp” is used here and in other files. Validate that both references remain in sync if the endpoint changes.
✅ Verification successful
Endpoint reference is consistent across the codebase
The endpoint "/wp-json/api/v1/newsletter/signUp" is consistently referenced in the newsletter form template (newsletter.twig), the source JavaScript module (newsletter-helper.js), and its compiled version (newsletter-0adfb7d.js). No inconsistencies were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Confirm that the endpoint matches references across the codebase:
rg "/wp-json/api/v1/newsletter/signUp"
Length of output: 447
wp-content/mu-plugins/register-rest-routes.php (1)
425-425:
Use $ex instead of $e for the exception variable.
You are referencing $e->getMessage() but declared $ex in the catch. This will cause an undefined variable error.
} catch (Exception $ex) {
- return new WP_REST_Response($e->getMessage(), 500);
+ return new WP_REST_Response($ex->getMessage(), 500);
}Likely invalid or redundant comment.
| $sendEmail->setFrom(SENDGRID_SENDGER_EMAIL_ADDRESS, SENGRID_SENDER_NAME); | ||
| $sendEmail->setSubject(SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT); |
There was a problem hiding this comment.
Rectify variable name mismatch.
The key is “SENDGRID_SENDGER_EMAIL_ADDRESS”, while the code references “SENGRID_SENDER_NAME” (typo). Standardize these for clarity.
$sendEmail->setFrom(SENDGRID_SENDGER_EMAIL_ADDRESS, SENGRID_SENDER_NAME);
- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^
+ SENDGRID_SENDER_EMAIL_ADDRESS, SENDGRID_SENDER_NAME📝 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.
| $sendEmail->setFrom(SENDGRID_SENDGER_EMAIL_ADDRESS, SENGRID_SENDER_NAME); | |
| $sendEmail->setSubject(SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT); | |
| $sendEmail->setFrom(SENDGRID_SENDER_EMAIL_ADDRESS, SENDGRID_SENDER_NAME); | |
| $sendEmail->setSubject(SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT); |
There was a problem hiding this comment.
@SourabhChakraborty, here's the updated code snippet resolving the variable name typos:
$sendEmail->setFrom(SENDGRID_SENDER_EMAIL_ADDRESS, SENDGRID_SENDER_NAME);
$sendEmail->setSubject(SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT);✏️ Learnings added
Learnt from: SourabhChakraborty
PR: CityOfNewYork/working-nyc#355
File: wp-content/mu-plugins/register-rest-routes.php:345-346
Timestamp: 2024-12-23T20:29:27.731Z
Learning: In file `wp-content/mu-plugins/register-rest-routes.php`, the variable name "SENDGRID_SENDGER_EMAIL_ADDRESS" should be "SENDGRID_SENDER_EMAIL_ADDRESS" and "SENGRID_SENDER_NAME" should be "SENDGRID_SENDER_NAME".
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| function LogEventData($eventData){ | ||
| $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; | ||
| $log_file = $log_dir . '/events.log'; | ||
| $event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown'; | ||
| $templateName = isset($eventData['sg_template_name']) ? $eventData['sg_template_name'] : 'unknown'; | ||
| $email = isset($eventData['email']) ? $eventData['email'] : 'unknown'; | ||
| $timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown'; | ||
| $url = isset($eventData['url']) ? $eventData['url'] : 'unknown'; | ||
| $log_data = "Event: $event_type | URL: $url | Timestamp: $timestamp\n"; | ||
| file_put_contents($log_file, $log_data, FILE_APPEND); | ||
| return array('event'=>$event_type, 'email'=>$email, 'templateName'=>$templateName); | ||
| } |
There was a problem hiding this comment.
Check for directory existence before logging.
Your code writes to “/sendgrid-events/events.log” but does not ensure the directory is created. Consider adding error handling or directory creation to avoid file-write failures.
function LogEventData($eventData){
$log_dir = WP_CONTENT_DIR . '/sendgrid-events';
+ if (!file_exists($log_dir)) {
+ mkdir($log_dir, 0755, true);
+ }
$log_file = $log_dir . '/events.log';
...
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
- do this
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| function LogEventData($eventData){ | ||
| $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; | ||
| $log_file = $log_dir . '/events.log'; | ||
| $event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown'; | ||
| $templateName = isset($eventData['sg_template_name']) ? $eventData['sg_template_name'] : 'unknown'; | ||
| $email = isset($eventData['email']) ? $eventData['email'] : 'unknown'; | ||
| $timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown'; | ||
| $url = isset($eventData['url']) ? $eventData['url'] : 'unknown'; | ||
| $log_data = "Event: $event_type | URL: $url | Timestamp: $timestamp\n"; | ||
| file_put_contents($log_file, $log_data, FILE_APPEND); | ||
| return array('event'=>$event_type, 'email'=>$email, 'templateName'=>$templateName); | ||
| } |
There was a problem hiding this comment.
- do this
| $log_file = $log_dir . '/events.log'; | ||
| $event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown'; | ||
| $templateName = isset($eventData['sg_template_name']) ? $eventData['sg_template_name'] : 'unknown'; | ||
| $email = isset($eventData['email']) ? $eventData['email'] : 'unknown'; |
There was a problem hiding this comment.
- Don't log the user's email address because it is sensitive data
| if($event_data['event']=='click' && $event_data['templateName']==SENDGRID_CONFIRMED_TEMPLATE_NAME){ | ||
| $apiKey = SENDGRID_API_KEY; | ||
| $sg = new \SendGrid($apiKey); | ||
| $request_body = json_decode('{ |
There was a problem hiding this comment.
- Create a PHP array directly instead of writing a JSON and converting it to a PHP array
There was a problem hiding this comment.
Sendgrid API is not accepting the Array.
| # SENDGRID_CONFIRMATION_TEMPATE_ID: | ||
| # SENDGRID_CONFIRMED_TEMPLATE_ID: | ||
| # SENDGRID_LIST_ID: | ||
| # SENDGRID_SENDGER_EMAIL_ADDRESS: |
| $sendEmail->setFrom(SENDGRID_SENDGER_EMAIL_ADDRESS, SENGRID_SENDER_NAME); | ||
| $sendEmail->setSubject(SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT); |
| 'methods' => 'GET', | ||
| 'permission_callback' => '__return_true', |
There was a problem hiding this comment.
- @mithunpandeti Convert to POST
| add_action('rest_api_init', function() { | ||
| $v = 'api/v1'; // namespace for the current version of the API | ||
|
|
||
| $exp = WEEK_IN_SECONDS; // expiration of the transient caches |
There was a problem hiding this comment.
- Remove this line here and in the other rest route you created
| $email_address = $request->get_param('EMAIL'); | ||
| $zipcode = $request->get_param('ZIPCODE'); |
There was a problem hiding this comment.
- @mithunpandeti do this
|
|
||
| $exp = WEEK_IN_SECONDS; // expiration of the transient caches | ||
|
|
||
| function LogEventData($eventData){ |
There was a problem hiding this comment.
- Use separate functions for logging event data and transforming event data. This function should not return anything, and you should use a separate function that has
return array('event'=>$event_type, 'email'=>$email, 'templateName'=>$templateName). - (minor) use snake case instead of camel case:
log_event_data($event_data)instead ofLogEventData($eventData)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
wp-content/mu-plugins/register-rest-routes.php (2)
346-347:⚠️ Potential issueFix variable name typos.
The variable names contain typos as identified in the learnings.
-$sendEmail->setFrom(SENDGRID_SENDGER_EMAIL_ADDRESS, SENGRID_SENDER_NAME); +$sendEmail->setFrom(SENDGRID_SENDER_EMAIL_ADDRESS, SENDGRID_SENDER_NAME);
377-380:⚠️ Potential issueChange HTTP method to POST for data submission.
Using GET for submitting sensitive data like email addresses is not secure.
register_rest_route('api/v1', '/newsletter/signUp/', array( - 'methods' => 'GET', + 'methods' => 'POST', 'permission_callback' => '__return_true',
🧹 Nitpick comments (1)
wp-content/mu-plugins/register-rest-routes.php (1)
9-11: Consider adding a namespace declaration.Adding a namespace would help prevent naming conflicts and follow PHP best practices.
<?php +namespace WorkingNYC\REST; use Spyc; use RestPreparePosts\RestPreparePosts as RestPreparePosts;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/mu-plugins/register-rest-routes.php(2 hunks)
🧰 Additional context used
📓 Learnings (1)
wp-content/mu-plugins/register-rest-routes.php (1)
Learnt from: SourabhChakraborty
PR: CityOfNewYork/working-nyc#355
File: wp-content/mu-plugins/register-rest-routes.php:345-346
Timestamp: 2024-12-23T20:29:27.958Z
Learning: In file `wp-content/mu-plugins/register-rest-routes.php`, the variable name "SENDGRID_SENDGER_EMAIL_ADDRESS" should be "SENDGRID_SENDER_EMAIL_ADDRESS" and "SENGRID_SENDER_NAME" should be "SENDGRID_SENDER_NAME".
🔇 Additional comments (2)
wp-content/mu-plugins/register-rest-routes.php (2)
422-422:
Fix variable reference in catch block.
The catch block references undefined variable $e instead of $ex.
-return new WP_REST_Response($e->getMessage(), 500);
+return new WP_REST_Response($ex->getMessage(), 500);Likely invalid or redundant comment.
381-384:
Add input validation for email and zipcode.
The current implementation doesn't validate the input parameters.
'callback' => function(WP_REST_Request $request) {
$email_address = $request->get_param('EMAIL');
$zipcode = $request->get_param('ZIPCODE');
+
+ if (empty($email_address) || !is_email($email_address)) {
+ return new WP_REST_Response('Invalid email address', 400);
+ }
+
+ if (empty($zipcode) || !preg_match('/^\d{5}(-\d{4})?$/', $zipcode)) {
+ return new WP_REST_Response('Invalid zipcode', 400);
+ }Likely invalid or redundant comment.
| function log_event_data($eventType, $URL, $time_stamp){ | ||
| $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; | ||
| $log_file = $log_dir . '/events.log'; | ||
| $log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n"; | ||
| file_put_contents($log_file, $log_data, FILE_APPEND); | ||
| } |
There was a problem hiding this comment.
Add directory check and error handling for logging.
The logging function should ensure the directory exists and handle potential write failures.
function log_event_data($eventType, $URL, $time_stamp) {
$log_dir = WP_CONTENT_DIR . '/sendgrid-events';
+ if (!file_exists($log_dir)) {
+ if (!mkdir($log_dir, 0755, true)) {
+ error_log('Failed to create SendGrid events log directory');
+ return false;
+ }
+ }
$log_file = $log_dir . '/events.log';
$log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n";
- file_put_contents($log_file, $log_data, FILE_APPEND);
+ if (file_put_contents($log_file, $log_data, FILE_APPEND | LOCK_EX) === false) {
+ error_log('Failed to write to SendGrid events log');
+ return false;
+ }
+ return true;
}📝 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.
| function log_event_data($eventType, $URL, $time_stamp){ | |
| $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; | |
| $log_file = $log_dir . '/events.log'; | |
| $log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n"; | |
| file_put_contents($log_file, $log_data, FILE_APPEND); | |
| } | |
| function log_event_data($eventType, $URL, $time_stamp) { | |
| $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; | |
| if (!file_exists($log_dir)) { | |
| if (!mkdir($log_dir, 0755, true)) { | |
| error_log('Failed to create SendGrid events log directory'); | |
| return false; | |
| } | |
| } | |
| $log_file = $log_dir . '/events.log'; | |
| $log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n"; | |
| if (file_put_contents($log_file, $log_data, FILE_APPEND | LOCK_EX) === false) { | |
| error_log('Failed to write to SendGrid events log'); | |
| return false; | |
| } | |
| return true; | |
| } |
| $request_body = json_decode('{ | ||
| "list_ids": ["'. SENDGRID_LIST_ID .'"], | ||
| "contacts": [ | ||
| { | ||
| "email": "' . $event_data['email'] . '", | ||
| "custom_fields": { | ||
| "Subscription_Status": "confirmed" | ||
| } | ||
| } | ||
| ] | ||
| }'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use PHP array instead of JSON string.
Creating a JSON string and then decoding it is inefficient and error-prone.
-$request_body = json_decode('{
- "list_ids": ["'. SENDGRID_LIST_ID .'"],
- "contacts": [
- {
- "email": "' . $event_data['email'] . '",
- "custom_fields": {
- "Subscription_Status": "confirmed"
- }
- }
- ]
-}');
+$request_body = [
+ 'list_ids' => [SENDGRID_LIST_ID],
+ 'contacts' => [
+ [
+ 'email' => $event_data['email'],
+ 'custom_fields' => [
+ 'Subscription_Status' => 'confirmed'
+ ]
+ ]
+ ]
+];📝 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.
| $request_body = json_decode('{ | |
| "list_ids": ["'. SENDGRID_LIST_ID .'"], | |
| "contacts": [ | |
| { | |
| "email": "' . $event_data['email'] . '", | |
| "custom_fields": { | |
| "Subscription_Status": "confirmed" | |
| } | |
| } | |
| ] | |
| }'); | |
| $request_body = [ | |
| 'list_ids' => [SENDGRID_LIST_ID], | |
| 'contacts' => [ | |
| [ | |
| 'email' => $event_data['email'], | |
| 'custom_fields' => [ | |
| 'Subscription_Status' => 'confirmed' | |
| ] | |
| ] | |
| ] | |
| ]; |
| 'callback' => function(WP_REST_Request $request) { | ||
| $input = $request->get_body(); | ||
| $data = json_decode($input, true); | ||
|
|
||
| if (is_array($data)) { | ||
| foreach ($data as $event) { | ||
| $event_data = transform_event_data($event); |
There was a problem hiding this comment.
Add JSON validation and improve error handling.
The current implementation doesn't properly validate the JSON input.
'callback' => function(WP_REST_Request $request) {
$input = $request->get_body();
+ if (empty($input)) {
+ return new WP_REST_Response('Empty request body', 400);
+ }
$data = json_decode($input, true);
+ if (json_last_error() !== JSON_ERROR_NONE) {
+ return new WP_REST_Response('Invalid JSON: ' . json_last_error_msg(), 400);
+ }
if (is_array($data)) {
foreach ($data as $event) {
+ if (!isset($event['event']) || !isset($event['email'])) {
+ continue;
+ }
$event_data = transform_event_data($event);📝 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.
| 'callback' => function(WP_REST_Request $request) { | |
| $input = $request->get_body(); | |
| $data = json_decode($input, true); | |
| if (is_array($data)) { | |
| foreach ($data as $event) { | |
| $event_data = transform_event_data($event); | |
| 'callback' => function(WP_REST_Request $request) { | |
| $input = $request->get_body(); | |
| if (empty($input)) { | |
| return new WP_REST_Response('Empty request body', 400); | |
| } | |
| $data = json_decode($input, true); | |
| if (json_last_error() !== JSON_ERROR_NONE) { | |
| return new WP_REST_Response('Invalid JSON: ' . json_last_error_msg(), 400); | |
| } | |
| if (is_array($data)) { | |
| foreach ($data as $event) { | |
| if (!isset($event['event']) || !isset($event['email'])) { | |
| continue; | |
| } | |
| $event_data = transform_event_data($event); |
| function transform_event_data($eventData){ | ||
| $event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown'; | ||
| $templateName = isset($eventData['sg_template_name']) ? $eventData['sg_template_name'] : 'unknown'; | ||
| $email = isset($eventData['email']) ? $eventData['email'] : 'unknown'; | ||
| $timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown'; | ||
| $url = isset($eventData['url']) ? $eventData['url'] : 'unknown'; | ||
| log_event_data($event_type, $url, $timestamp); | ||
| return array('event'=>$event_type, 'email'=>$email, 'templateName'=>$templateName); | ||
| } |
There was a problem hiding this comment.
Refactor event data transformation to avoid logging PII.
The function currently logs email addresses which is sensitive data. Also, consider separating the logging from the transformation.
-function transform_event_data($eventData) {
+function transform_event_data($eventData) {
$event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown';
$templateName = isset($eventData['sg_template_name']) ? $eventData['sg_template_name'] : 'unknown';
$email = isset($eventData['email']) ? $eventData['email'] : 'unknown';
$timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown';
$url = isset($eventData['url']) ? $eventData['url'] : 'unknown';
- log_event_data($event_type, $url, $timestamp);
return array('event'=>$event_type, 'email'=>$email, 'templateName'=>$templateName);
}
+function log_sendgrid_event($eventData) {
+ $event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown';
+ $url = isset($eventData['url']) ? $eventData['url'] : 'unknown';
+ $timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown';
+ return log_event_data($event_type, $url, $timestamp);
+}Committable suggestion skipped: line range outside the PR's diff.
…dress and zip code.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
wp-content/themes/workingnyc/assets/js/newsletter-31ac4cb.js (4)
347-355: Consider validating empty fields before packaging form data
Current logic simply iterates over all form fields and adds them todataObject. If you need to ensure critical fields are present (e.g., email address), a quick validation step could prevent unintentional blank data being sent.
358-358: Remove unused commented-out code
The commented line// let domain= '';might create confusion. Consider removing it if no longer needed.- // let domain= '';
362-362: Externalize endpoint URL
Hardcoding"/wp-json/api/v1/newsletter/signUp"in the frontend can cause maintenance issues if the endpoint changes in the future. Consider moving it to a config variable or environment variable to ease updates.
366-366: Remove obsolete Mailchimp callback reference
This commented-out line references an old Mailchimp-style callback approach. If not reused, remove it to reduce confusion.- //action = `${action}&c=window.${this.callback}`;wp-content/mu-plugins/config/config-sample.yml (1)
40-49: Remove trailing spaces and add a newline at EOF
Trailing spaces and missing newline can trigger lint errors and version-control churn. Clean up these lines for consistency.-# SENDGRID_API_KEY: -# SENDGRID_CONFIRMATION_TEMPATE_ID: -# SENDGRID_CONFIRMED_TEMPLATE_ID: -# SENDGRID_LIST_ID: -# SENDGRID_SENDER_EMAIL_ADDRESS: -# SENDGRID_SENDER_NAME: -# SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT: -# SENDGRID_SUBSCRIPTION_CONFIRM_SUBJECT: -# SENDGRID_CONFIRMED_TEMPLATE_NAME: \ No newline at end of file +# SENDGRID_API_KEY: +# SENDGRID_CONFIRMATION_TEMPATE_ID: +# SENDGRID_CONFIRMED_TEMPLATE_ID: +# SENDGRID_LIST_ID: +# SENDGRID_SENDER_EMAIL_ADDRESS: +# SENDGRID_SENDER_NAME: +# SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT: +# SENDGRID_SUBSCRIPTION_CONFIRM_SUBJECT: +# SENDGRID_CONFIRMED_TEMPLATE_NAME: +🧰 Tools
🪛 yamllint (1.35.1)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
[error] 49-49: trailing spaces
(trailing-spaces)
wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js (2)
78-86: Consider additional safeguards for empty or invalid data
ConstructingdataObjectfromformDatais straightforward, but you might consider a quick validation step for critical fields before sending the request.
93-93: Externalize endpoint URL
Hardcoding"/wp-json/api/v1/newsletter/signUp"can pose risks if the endpoint changes. Consider moving it to a configuration file or environment variable.wp-content/mu-plugins/register-rest-routes.php (1)
312-372: Add inbound request validation for the/newsletter/confirm/endpoint
This endpoint processes requests without verifying they originate from SendGrid or a trusted source. Consider implementing a signature validation or a shared secret.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wp-content/mu-plugins/config/config-sample.yml(1 hunks)wp-content/mu-plugins/register-rest-routes.php(2 hunks)wp-content/themes/workingnyc/assets/js/newsletter-31ac4cb.js(3 hunks)wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js(3 hunks)
🧰 Additional context used
📓 Learnings (1)
wp-content/mu-plugins/register-rest-routes.php (1)
Learnt from: SourabhChakraborty
PR: CityOfNewYork/working-nyc#355
File: wp-content/mu-plugins/register-rest-routes.php:345-346
Timestamp: 2024-12-23T20:29:27.958Z
Learning: In file `wp-content/mu-plugins/register-rest-routes.php`, the variable name "SENDGRID_SENDGER_EMAIL_ADDRESS" should be "SENDGRID_SENDER_EMAIL_ADDRESS" and "SENGRID_SENDER_NAME" should be "SENDGRID_SENDER_NAME".
🪛 yamllint (1.35.1)
wp-content/mu-plugins/config/config-sample.yml
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
[error] 49-49: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
wp-content/themes/workingnyc/assets/js/newsletter-31ac4cb.js (2)
325-325: No issues found in _submit invocation
The call to this._submit(event) here appropriately delegates the form submission logic to the _submit method, keeping code tidy.
422-426: Verify constant 'success' usage
The _callback method relies on data == 'success' to determine a successful subscription. Confirm that the server consistently returns 'success'. Otherwise, consider using a more robust response format (e.g., {"status": "success"}) to avoid confusion.
✅ Verification successful
Let me search for more specific references to the success response.
Let me try a different search approach to find the newsletter subscription handling.
Let me check the source file for the newsletter subscription handling.
Let me check the newsletter helper module which contains the callback implementation.
The string comparison data == 'success' is correct and consistent with the codebase
Based on the source code review, the string comparison data == 'success' in the _callback method is correct. The newsletter module makes a POST request to /wp-json/api/v1/newsletter/signUp endpoint and expects a simple 'success' string response for successful subscriptions. When successful, it triggers the _success() method which handles showing the appropriate confirmation message to the user. For any other response, it shows an error message.
The implementation follows a simple API contract where the backend endpoint returns just 'success' for successful subscriptions, making the string comparison valid and intentional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search all references for the string "success" to see if code consistently returns 'success'.
rg "success"
Length of output: 130398
Script:
#!/bin/bash
# Search for success response handling in newsletter related files
rg -A 5 "data.*success" wp-content/themes/workingnyc/assets/js/newsletter
Length of output: 165
Script:
#!/bin/bash
# Search for newsletter form submission handling
ast-grep --pattern 'function $_($_, $_) {
$$$
if ($_ == "success") {
$$$
}
$$$
}'
Length of output: 9069
Script:
#!/bin/bash
# Look for any AJAX response handling in the codebase
rg -A 5 "success.*response" wp-content/themes/workingnyc/
Length of output: 59
Script:
#!/bin/bash
# Search for the newsletter file with any extension
fd "newsletter" wp-content/themes/workingnyc/
# Also search for subscription related code
rg -A 10 "subscription|subscribe" wp-content/themes/workingnyc/
Length of output: 15270
Script:
#!/bin/bash
# Check the newsletter source file
cat wp-content/themes/workingnyc/src/js/newsletter.js
Length of output: 1973
Script:
#!/bin/bash
# Check the newsletter helper module
cat wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js
Length of output: 10789
wp-content/themes/workingnyc/src/js/modules/newsletter-helper.js (3)
56-56: Form submit delegation
The call to this._submit(event) properly delegates responsibilities to _submit, centralizing the submission logic.
89-89: Leverage server-side checks despite serialization
this._data = serialize(...) is useful for client-side usage. Ensure the server also sanitizes and validates data to protect against tampering.
155-159: Confirm 'success' is consistently returned
The _callback method interprets 'success' literally. Ensure the backend consistently returns 'success' or adopt a more structured response format.
wp-content/mu-plugins/register-rest-routes.php (1)
293-311:
Avoid logging sensitive PII
log_event_data and transform_event_data currently log user email addresses. Logging this data can violate privacy or compliance requirements (e.g., GDPR). Mask or remove PII from logs.
- $log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n";
+ $log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp | [Email Removed]\n";Likely invalid or redundant comment.
|
|
||
| fetch(action, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify(dataObject) | ||
| }) | ||
| .then(response => response.json()) | ||
| .then(d => { | ||
| console.log(d); | ||
| window[this.callback](d); | ||
| }) | ||
| .catch(error => { | ||
| console.error('Error:', error); | ||
| this._error('An error occurred while processing your request. Please try again later.'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add robust error handling for non-2xx responses
Currently, the .then(response => response.json()) block assumes the response is JSON. If the server returns a 4xx or 5xx status with invalid JSON, the app can fail silently. Check response.ok before parsing JSON to handle unexpected responses gracefully.
fetch(action, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify(dataObject)
})
.then(response => {
+ if (!response.ok) {
+ throw new Error(`HTTP error! status: ${response.status}`);
+ }
return response.json();
})
.then(d => { ... })
.catch(...);📝 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.
| fetch(action, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify(dataObject) | |
| }) | |
| .then(response => response.json()) | |
| .then(d => { | |
| console.log(d); | |
| window[this.callback](d); | |
| }) | |
| .catch(error => { | |
| console.error('Error:', error); | |
| this._error('An error occurred while processing your request. Please try again later.'); | |
| }); | |
| fetch(action, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| body: JSON.stringify(dataObject) | |
| }) | |
| .then(response => { | |
| if (!response.ok) { | |
| throw new Error(`HTTP error! status: ${response.status}`); | |
| } | |
| return response.json(); | |
| }) | |
| .then(d => { | |
| console.log(d); | |
| window[this.callback](d); | |
| }) | |
| .catch(error => { | |
| console.error('Error:', error); | |
| this._error('An error occurred while processing your request. Please try again later.'); | |
| }); |
|
|
||
| fetch(action, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| body: JSON.stringify(dataObject) | ||
| }) | ||
| .then(response => response.json()) | ||
| .then(d => { | ||
| console.log(d) | ||
| window[this.callback](d); | ||
| }) | ||
| .catch(error => { | ||
| console.error('Error:', error); | ||
| this._error('An error occurred while processing your request. Please try again later.'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for non-2xx responses before parsing
In .then(response => response.json()), add checks for response.ok. Unexpected HTTP codes or non-JSON bodies can break the flow.
| use Spyc; | ||
|
|
||
| use RestPreparePosts\RestPreparePosts as RestPreparePosts; |
There was a problem hiding this comment.
💡 Codebase verification
Remove use Spyc; from register-rest-routes.php as it's unused
The Spyc class is imported but not used in the register-rest-routes.php file. While Spyc is actively used in other files for YAML parsing (wp-config/Encrypt.php, wp-config/Decrypt.php, wp-config/Config.php, and wp-assets/WpAssets.php), there are no references to it in register-rest-routes.php.
🔗 Analysis chain
Ensure Spyc usage is necessary
use Spyc; is included, presumably for YAML parsing. If not heavily used, remove to reduce dependencies. Otherwise, confirm it's essential for configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching all references of Spyc usage to confirm necessity
rg "Spyc"
Length of output: 11958
| add_action('rest_api_init', function() { | ||
|
|
||
| // Remove all non-numeric characters | ||
| function sanitize_zip_code($zipcode) { | ||
| return preg_replace('/\D/', '', $zipcode); | ||
| } | ||
|
|
||
| register_rest_route('api/v1', '/newsletter/signUp/', array( | ||
| 'methods' => 'POST', | ||
| 'permission_callback' => '__return_true', | ||
|
|
||
| 'callback' => function(WP_REST_Request $request) { | ||
|
|
||
| $email_address = filter_var($request->get_param('EMAIL'), FILTER_SANITIZE_EMAIL); | ||
| if (!is_email($email_address)) { | ||
| return new WP_REST_Response('Invalid email address', 400); | ||
| } | ||
|
|
||
| $zipcode = sanitize_zip_code($request->get_param('ZIPCODE')); | ||
|
|
||
| $apiKey = SENDGRID_API_KEY; | ||
| $sg = new \SendGrid($apiKey); | ||
| $request_body = json_decode('{ | ||
| "list_ids": ["'. SENDGRID_LIST_ID .'"], | ||
| "contacts": [ | ||
| { | ||
| "email": "' . $email_address . '", | ||
| "postal_code": "' . $zipcode . '", | ||
| "custom_fields": { | ||
| "Subscription_Status": "pending" | ||
| } | ||
| } | ||
| ] | ||
| }'); | ||
| try { | ||
| $response = $sg->client | ||
| ->marketing() | ||
| ->contacts() | ||
| ->put($request_body); | ||
|
|
||
| if($response->statusCode() == 202){ | ||
| $email = new \SendGrid\Mail\Mail(); | ||
| $email->setFrom(SENDGRID_SENDER_EMAIL_ADDRESS, SENDGRID_SENDER_NAME); | ||
| $email->setSubject(SENDGRID_SUBSCRIPTION_CONFIRM_SUBJECT); | ||
| $email->addTo($email_address,"User"); | ||
| $templateId = $sendgrid_confirmation_template_id; | ||
| $email->setTemplateId(SENDGRID_CONFIRMATION_TEMPATE_ID); | ||
| $sendgrid = new \SendGrid($apiKey); | ||
| try { | ||
| $response_email = $sendgrid->send($email); | ||
| if($response_email->statusCode() == 202){ | ||
| return new WP_REST_Response("success", 200); | ||
| } | ||
| } catch (Exception $e) { | ||
| return new WP_REST_Response($e->getMessage(), 500); | ||
| } | ||
| } | ||
| } catch (Exception $ex) { | ||
| return new WP_REST_Response($e->getMessage(), 500); | ||
| } | ||
|
|
||
| } | ||
| )); | ||
| }); |
There was a problem hiding this comment.
Fix variable mismatch in the catch block & template usage
Line 433 references $e->getMessage(), but the caught variable is $ex. Also, confirm that $sendgrid_confirmation_template_id is not needed if you use SENDGRID_CONFIRMATION_TEMPATE_ID. Mismatches can cause runtime errors.
} catch (Exception $ex) {
- return new WP_REST_Response($e->getMessage(), 500);
+ return new WP_REST_Response($ex->getMessage(), 500);
}📝 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.
| add_action('rest_api_init', function() { | |
| // Remove all non-numeric characters | |
| function sanitize_zip_code($zipcode) { | |
| return preg_replace('/\D/', '', $zipcode); | |
| } | |
| register_rest_route('api/v1', '/newsletter/signUp/', array( | |
| 'methods' => 'POST', | |
| 'permission_callback' => '__return_true', | |
| 'callback' => function(WP_REST_Request $request) { | |
| $email_address = filter_var($request->get_param('EMAIL'), FILTER_SANITIZE_EMAIL); | |
| if (!is_email($email_address)) { | |
| return new WP_REST_Response('Invalid email address', 400); | |
| } | |
| $zipcode = sanitize_zip_code($request->get_param('ZIPCODE')); | |
| $apiKey = SENDGRID_API_KEY; | |
| $sg = new \SendGrid($apiKey); | |
| $request_body = json_decode('{ | |
| "list_ids": ["'. SENDGRID_LIST_ID .'"], | |
| "contacts": [ | |
| { | |
| "email": "' . $email_address . '", | |
| "postal_code": "' . $zipcode . '", | |
| "custom_fields": { | |
| "Subscription_Status": "pending" | |
| } | |
| } | |
| ] | |
| }'); | |
| try { | |
| $response = $sg->client | |
| ->marketing() | |
| ->contacts() | |
| ->put($request_body); | |
| if($response->statusCode() == 202){ | |
| $email = new \SendGrid\Mail\Mail(); | |
| $email->setFrom(SENDGRID_SENDER_EMAIL_ADDRESS, SENDGRID_SENDER_NAME); | |
| $email->setSubject(SENDGRID_SUBSCRIPTION_CONFIRM_SUBJECT); | |
| $email->addTo($email_address,"User"); | |
| $templateId = $sendgrid_confirmation_template_id; | |
| $email->setTemplateId(SENDGRID_CONFIRMATION_TEMPATE_ID); | |
| $sendgrid = new \SendGrid($apiKey); | |
| try { | |
| $response_email = $sendgrid->send($email); | |
| if($response_email->statusCode() == 202){ | |
| return new WP_REST_Response("success", 200); | |
| } | |
| } catch (Exception $e) { | |
| return new WP_REST_Response($e->getMessage(), 500); | |
| } | |
| } | |
| } catch (Exception $ex) { | |
| return new WP_REST_Response($e->getMessage(), 500); | |
| } | |
| } | |
| )); | |
| }); | |
| add_action('rest_api_init', function() { | |
| // Remove all non-numeric characters | |
| function sanitize_zip_code($zipcode) { | |
| return preg_replace('/\D/', '', $zipcode); | |
| } | |
| register_rest_route('api/v1', '/newsletter/signUp/', array( | |
| 'methods' => 'POST', | |
| 'permission_callback' => '__return_true', | |
| 'callback' => function(WP_REST_Request $request) { | |
| $email_address = filter_var($request->get_param('EMAIL'), FILTER_SANITIZE_EMAIL); | |
| if (!is_email($email_address)) { | |
| return new WP_REST_Response('Invalid email address', 400); | |
| } | |
| $zipcode = sanitize_zip_code($request->get_param('ZIPCODE')); | |
| $apiKey = SENDGRID_API_KEY; | |
| $sg = new \SendGrid($apiKey); | |
| $request_body = json_decode('{ | |
| "list_ids": ["'. SENDGRID_LIST_ID .'"], | |
| "contacts": [ | |
| { | |
| "email": "' . $email_address . '", | |
| "postal_code": "' . $zipcode . '", | |
| "custom_fields": { | |
| "Subscription_Status": "pending" | |
| } | |
| } | |
| ] | |
| }'); | |
| try { | |
| $response = $sg->client | |
| ->marketing() | |
| ->contacts() | |
| ->put($request_body); | |
| if($response->statusCode() == 202){ | |
| $email = new \SendGrid\Mail\Mail(); | |
| $email->setFrom(SENDGRID_SENDER_EMAIL_ADDRESS, SENDGRID_SENDER_NAME); | |
| $email->setSubject(SENDGRID_SUBSCRIPTION_CONFIRM_SUBJECT); | |
| $email->addTo($email_address,"User"); | |
| $templateId = $sendgrid_confirmation_template_id; | |
| $email->setTemplateId(SENDGRID_CONFIRMATION_TEMPATE_ID); | |
| $sendgrid = new \SendGrid($apiKey); | |
| try { | |
| $response_email = $sendgrid->send($email); | |
| if($response_email->statusCode() == 202){ | |
| return new WP_REST_Response("success", 200); | |
| } | |
| } catch (Exception $e) { | |
| return new WP_REST_Response($e->getMessage(), 500); | |
| } | |
| } | |
| } catch (Exception $ex) { | |
| return new WP_REST_Response($ex->getMessage(), 500); | |
| } | |
| } | |
| )); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
wp-content/mu-plugins/register-rest-routes.php (4)
296-304:⚠️ Potential issueAdd error handling for file operations
The logging function should handle potential file operation failures.
function log_event_data($eventType, $URL, $time_stamp){ $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; if (!file_exists($log_dir)) { - mkdir($log_dir, 0755, true); + if (!mkdir($log_dir, 0755, true)) { + error_log('Failed to create SendGrid events log directory'); + return false; + } } $log_file = $log_dir . '/events.log'; $log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n"; - file_put_contents($log_file, $log_data, FILE_APPEND); + if (file_put_contents($log_file, $log_data, FILE_APPEND | LOCK_EX) === false) { + error_log('Failed to write to SendGrid events log'); + return false; + } + return true; }
306-314: 🛠️ Refactor suggestionSeparate logging from data transformation
The function violates the Single Responsibility Principle by mixing logging with data transformation. Also, avoid logging PII (email addresses).
function transform_event_data($eventData){ $event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown'; $templateName = isset($eventData['sg_template_name']) ? $eventData['sg_template_name'] : 'unknown'; $email = isset($eventData['email']) ? $eventData['email'] : 'unknown'; - $timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown'; - $url = isset($eventData['url']) ? $eventData['url'] : 'unknown'; - log_event_data($event_type, $url, $timestamp); return array('event'=>$event_type, 'email'=>$email, 'templateName'=>$templateName); } +function log_sendgrid_event($eventData) { + $event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown'; + $url = isset($eventData['url']) ? $eventData['url'] : 'unknown'; + $timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown'; + return log_event_data($event_type, $url, $timestamp); +}
320-326:⚠️ Potential issueAdd JSON validation
The current implementation doesn't properly validate the JSON input.
'callback' => function(WP_REST_Request $request) { $input = $request->get_body(); + if (empty($input)) { + return new WP_REST_Response('Empty request body', 400); + } $data = json_decode($input, true); + if (json_last_error() !== JSON_ERROR_NONE) { + return new WP_REST_Response('Invalid JSON: ' . json_last_error_msg(), 400); + } if (is_array($data)) {
330-340: 🛠️ Refactor suggestionUse PHP array instead of JSON string
Creating a JSON string and then decoding it is inefficient and error-prone.
-$request_body = json_decode('{ - "list_ids": ["'. SENDGRID_LIST_ID .'"], - "contacts": [ - { - "email": "' . $event_data['email'] . '", - "custom_fields": { - "Subscription_Status": "confirmed" - } - } - ] -}'); +$request_body = [ + 'list_ids' => [SENDGRID_LIST_ID], + 'contacts' => [ + [ + 'email' => $event_data['email'], + 'custom_fields' => [ + 'Subscription_Status' => 'confirmed' + ] + ] + ] +];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/mu-plugins/register-rest-routes.php(2 hunks)
🧰 Additional context used
📓 Learnings (1)
wp-content/mu-plugins/register-rest-routes.php (1)
Learnt from: SourabhChakraborty
PR: CityOfNewYork/working-nyc#355
File: wp-content/mu-plugins/register-rest-routes.php:345-346
Timestamp: 2024-12-23T20:29:27.958Z
Learning: In file `wp-content/mu-plugins/register-rest-routes.php`, the variable name "SENDGRID_SENDGER_EMAIL_ADDRESS" should be "SENDGRID_SENDER_EMAIL_ADDRESS" and "SENGRID_SENDER_NAME" should be "SENDGRID_SENDER_NAME".
🔇 Additional comments (2)
wp-content/mu-plugins/register-rest-routes.php (2)
9-11: Remove unused import
The use Spyc; statement is not used in this file. Remove it to keep the imports clean and relevant.
435-437:
Fix variable mismatch in catch block
The catch block uses $e but catches the exception as $ex.
} catch (Exception $ex) {
- return new WP_REST_Response($e->getMessage(), 500);
+ return new WP_REST_Response($ex->getMessage(), 500);
}Likely invalid or redundant comment.
| $templateId = $sendgrid_confirmation_template_id; | ||
| $email->setTemplateId(SENDGRID_CONFIRMATION_TEMPATE_ID); |
There was a problem hiding this comment.
Remove unused variable and fix template ID usage
The variable $sendgrid_confirmation_template_id is declared but never used, and the next line uses SENDGRID_CONFIRMATION_TEMPATE_ID directly.
- $templateId = $sendgrid_confirmation_template_id;
- $email->setTemplateId(SENDGRID_CONFIRMATION_TEMPATE_ID);
+ $email->setTemplateId(SENDGRID_CONFIRMATION_TEMPATE_ID);📝 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.
| $templateId = $sendgrid_confirmation_template_id; | |
| $email->setTemplateId(SENDGRID_CONFIRMATION_TEMPATE_ID); | |
| $email->setTemplateId(SENDGRID_CONFIRMATION_TEMPATE_ID); |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/mu-plugins/register-rest-routes.php(2 hunks)
🧰 Additional context used
📓 Learnings (1)
wp-content/mu-plugins/register-rest-routes.php (1)
Learnt from: SourabhChakraborty
PR: CityOfNewYork/working-nyc#355
File: wp-content/mu-plugins/register-rest-routes.php:345-346
Timestamp: 2024-12-23T20:29:27.958Z
Learning: In file `wp-content/mu-plugins/register-rest-routes.php`, the variable name "SENDGRID_SENDGER_EMAIL_ADDRESS" should be "SENDGRID_SENDER_EMAIL_ADDRESS" and "SENGRID_SENDER_NAME" should be "SENDGRID_SENDER_NAME".
🔇 Additional comments (4)
wp-content/mu-plugins/register-rest-routes.php (4)
9-11: Remove unused import use Spyc;
The Spyc class is imported but never used in this file. Remove this unused import to keep the code clean.
330-340: 🛠️ Refactor suggestion
Use PHP array instead of JSON string construction
Creating a JSON string and then decoding it is inefficient and error-prone:
-$request_body = json_decode('{
- "list_ids": ["'. SENDGRID_LIST_ID .'"],
- "contacts": [
- {
- "email": "' . $event_data['email'] . '",
- "custom_fields": {
- "Subscription_Status": "confirmed"
- }
- }
- ]
-}');
+$request_body = [
+ 'list_ids' => [SENDGRID_LIST_ID],
+ 'contacts' => [
+ [
+ 'email' => $event_data['email'],
+ 'custom_fields' => [
+ 'Subscription_Status' => 'confirmed'
+ ]
+ ]
+ ]
+];Likely invalid or redundant comment.
320-326:
Add input validation for webhook requests
The current implementation lacks proper input validation:
'callback' => function(WP_REST_Request $request) {
$input = $request->get_body();
+ if (empty($input)) {
+ return new WP_REST_Response('Empty request body', 400);
+ }
$data = json_decode($input, true);
+ if (json_last_error() !== JSON_ERROR_NONE) {
+ return new WP_REST_Response('Invalid JSON: ' . json_last_error_msg(), 400);
+ }
if (is_array($data)) {
foreach ($data as $event) {
+ if (!isset($event['event']) || !isset($event['email'])) {
+ continue;
+ }
$event_data = transform_event_data($event);Likely invalid or redundant comment.
306-314: 🛠️ Refactor suggestion
Separate concerns and avoid logging PII
The function mixes transformation and logging concerns, and logs sensitive email data:
- Separate the logging from transformation
- Avoid logging PII (email addresses)
function transform_event_data($eventData) {
$event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown';
$templateName = isset($eventData['sg_template_name']) ? $eventData['sg_template_name'] : 'unknown';
$email = isset($eventData['email']) ? $eventData['email'] : 'unknown';
- $timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown';
- $url = isset($eventData['url']) ? $eventData['url'] : 'unknown';
- log_event_data($event_type, $url, $timestamp);
return array('event'=>$event_type, 'email'=>$email, 'templateName'=>$templateName);
}
+function log_sendgrid_event($eventData) {
+ $event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown';
+ $url = isset($eventData['url']) ? $eventData['url'] : 'unknown';
+ $timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown';
+ return log_event_data($event_type, $url, $timestamp);
+}Likely invalid or redundant comment.
| $templateId = SENDGRID_CONFIRMATION_TEMPATE_ID; | ||
| $email->setTemplateId($templateId); |
There was a problem hiding this comment.
Fix template ID constant typo
There's a typo in the constant name SENDGRID_CONFIRMATION_TEMPATE_ID (missing 'L' in TEMPLATE):
-$templateId = SENDGRID_CONFIRMATION_TEMPATE_ID;
-$email->setTemplateId($templateId);
+$email->setTemplateId(SENDGRID_CONFIRMATION_TEMPLATE_ID);📝 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.
| $templateId = SENDGRID_CONFIRMATION_TEMPATE_ID; | |
| $email->setTemplateId($templateId); | |
| $email->setTemplateId(SENDGRID_CONFIRMATION_TEMPLATE_ID); |
| $request_body = json_decode('{ | ||
| "list_ids": ["'. SENDGRID_LIST_ID .'"], | ||
| "contacts": [ | ||
| { | ||
| "email": "' . $email_address . '", | ||
| "postal_code": "' . $zipcode . '", | ||
| "custom_fields": { | ||
| "Subscription_Status": "pending" | ||
| } | ||
| } | ||
| ] | ||
| }'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use PHP array for request body construction
Similar to the confirmation route, use PHP array instead of JSON string:
-$request_body = json_decode('{
- "list_ids": ["'. SENDGRID_LIST_ID .'"],
- "contacts": [
- {
- "email": "' . $email_address . '",
- "postal_code": "' . $zipcode . '",
- "custom_fields": {
- "Subscription_Status": "pending"
- }
- }
- ]
- }');
+$request_body = [
+ 'list_ids' => [SENDGRID_LIST_ID],
+ 'contacts' => [
+ [
+ 'email' => $email_address,
+ 'postal_code' => $zipcode,
+ 'custom_fields' => [
+ 'Subscription_Status' => 'pending'
+ ]
+ ]
+ ]
+];📝 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.
| $request_body = json_decode('{ | |
| "list_ids": ["'. SENDGRID_LIST_ID .'"], | |
| "contacts": [ | |
| { | |
| "email": "' . $email_address . '", | |
| "postal_code": "' . $zipcode . '", | |
| "custom_fields": { | |
| "Subscription_Status": "pending" | |
| } | |
| } | |
| ] | |
| }'); | |
| $request_body = [ | |
| 'list_ids' => [SENDGRID_LIST_ID], | |
| 'contacts' => [ | |
| [ | |
| 'email' => $email_address, | |
| 'postal_code' => $zipcode, | |
| 'custom_fields' => [ | |
| 'Subscription_Status' => 'pending' | |
| ] | |
| ] | |
| ] | |
| ]; |
| function log_event_data($eventType, $URL, $time_stamp){ | ||
| $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; | ||
| if (!file_exists($log_dir)) { | ||
| mkdir($log_dir, 0755, true); | ||
| } | ||
| $log_file = $log_dir . '/events.log'; | ||
| $log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n"; | ||
| file_put_contents($log_file, $log_data, FILE_APPEND); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations
The logging function should handle potential file operation failures:
function log_event_data($eventType, $URL, $time_stamp){
$log_dir = WP_CONTENT_DIR . '/sendgrid-events';
if (!file_exists($log_dir)) {
- mkdir($log_dir, 0755, true);
+ if (!mkdir($log_dir, 0755, true)) {
+ error_log('Failed to create SendGrid events log directory');
+ return false;
+ }
}
$log_file = $log_dir . '/events.log';
$log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n";
- file_put_contents($log_file, $log_data, FILE_APPEND);
+ if (file_put_contents($log_file, $log_data, FILE_APPEND | LOCK_EX) === false) {
+ error_log('Failed to write to SendGrid events log');
+ return false;
+ }
+ return true;
}📝 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.
| function log_event_data($eventType, $URL, $time_stamp){ | |
| $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; | |
| if (!file_exists($log_dir)) { | |
| mkdir($log_dir, 0755, true); | |
| } | |
| $log_file = $log_dir . '/events.log'; | |
| $log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n"; | |
| file_put_contents($log_file, $log_data, FILE_APPEND); | |
| } | |
| function log_event_data($eventType, $URL, $time_stamp){ | |
| $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; | |
| if (!file_exists($log_dir)) { | |
| if (!mkdir($log_dir, 0755, true)) { | |
| error_log('Failed to create SendGrid events log directory'); | |
| return false; | |
| } | |
| } | |
| $log_file = $log_dir . '/events.log'; | |
| $log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n"; | |
| if (file_put_contents($log_file, $log_data, FILE_APPEND | LOCK_EX) === false) { | |
| error_log('Failed to write to SendGrid events log'); | |
| return false; | |
| } | |
| return true; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wp-content/mu-plugins/config/config-sample.yml (1)
41-49: LGTM! Configuration options are well-structured.The SendGrid configuration options are comprehensive and align with the implementation in register-rest-routes.php.
Fix the formatting issues:
-# SENDGRID_API_KEY: -# SENDGRID_CONFIRMATION_TEMPLATE_ID: -# SENDGRID_CONFIRMED_TEMPLATE_ID: -# SENDGRID_LIST_ID: -# SENDGRID_SENDER_EMAIL_ADDRESS: -# SENDGRID_SENDER_NAME: -# SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT: -# SENDGRID_SUBSCRIPTION_CONFIRM_SUBJECT: -# SENDGRID_CONFIRMED_TEMPLATE_NAME: +# SENDGRID_API_KEY: +# SENDGRID_CONFIRMATION_TEMPLATE_ID: +# SENDGRID_CONFIRMED_TEMPLATE_ID: +# SENDGRID_LIST_ID: +# SENDGRID_SENDER_EMAIL_ADDRESS: +# SENDGRID_SENDER_NAME: +# SENDGRID_CONFIRMED_SUBSCRIPTION_SUBJECT: +# SENDGRID_SUBSCRIPTION_CONFIRM_SUBJECT: +# SENDGRID_CONFIRMED_TEMPLATE_NAME: +🧰 Tools
🪛 yamllint (1.35.1)
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
[error] 49-49: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/mu-plugins/config/config-sample.yml(1 hunks)wp-content/mu-plugins/register-rest-routes.php(2 hunks)
🧰 Additional context used
📓 Learnings (1)
wp-content/mu-plugins/register-rest-routes.php (1)
Learnt from: SourabhChakraborty
PR: CityOfNewYork/working-nyc#355
File: wp-content/mu-plugins/register-rest-routes.php:345-346
Timestamp: 2024-12-23T20:29:27.958Z
Learning: In file `wp-content/mu-plugins/register-rest-routes.php`, the variable name "SENDGRID_SENDGER_EMAIL_ADDRESS" should be "SENDGRID_SENDER_EMAIL_ADDRESS" and "SENGRID_SENDER_NAME" should be "SENDGRID_SENDER_NAME".
🪛 yamllint (1.35.1)
wp-content/mu-plugins/config/config-sample.yml
[error] 41-41: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 43-43: trailing spaces
(trailing-spaces)
[error] 44-44: trailing spaces
(trailing-spaces)
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: no new line character at the end of file
(new-line-at-end-of-file)
[error] 49-49: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
wp-content/mu-plugins/register-rest-routes.php (6)
400-411:⚠️ Potential issueUse PHP array for request body construction.
Similar to the confirmation route, use PHP array instead of JSON string:
-$request_body = json_decode('{ - "list_ids": ["'. SENDGRID_LIST_ID .'"], - "contacts": [ - { - "email": "' . $email_address . '", - "postal_code": "' . $zipcode . '", - "custom_fields": { - "Subscription_Status": "pending" - } - } - ] - }'); +$request_body = [ + 'list_ids' => [SENDGRID_LIST_ID], + 'contacts' => [ + [ + 'email' => $email_address, + 'postal_code' => $zipcode, + 'custom_fields' => [ + 'Subscription_Status' => 'pending' + ] + ] + ] +];Likely invalid or redundant comment.
330-340:⚠️ Potential issueUse PHP array instead of JSON string concatenation.
Creating a JSON string and then decoding it is inefficient and potentially unsafe:
-$request_body = json_decode('{ - "list_ids": ["'. SENDGRID_LIST_ID .'"], - "contacts": [ - { - "email": "' . $event_data['email'] . '", - "custom_fields": { - "Subscription_Status": "confirmed" - } - } - ] -}'); +$request_body = [ + 'list_ids' => [SENDGRID_LIST_ID], + 'contacts' => [ + [ + 'email' => $event_data['email'], + 'custom_fields' => [ + 'Subscription_Status' => 'confirmed' + ] + ] + ] +];Likely invalid or redundant comment.
296-304:⚠️ Potential issueAdd error handling for file operations.
The logging function needs proper error handling and file locking:
function log_event_data($eventType, $URL, $time_stamp){ $log_dir = WP_CONTENT_DIR . '/sendgrid-events'; if (!file_exists($log_dir)) { - mkdir($log_dir, 0755, true); + if (!mkdir($log_dir, 0755, true)) { + error_log('Failed to create SendGrid events log directory'); + return false; + } } $log_file = $log_dir . '/events.log'; $log_data = "Event: $eventType | URL: $URL | Timestamp: $time_stamp\n"; - file_put_contents($log_file, $log_data, FILE_APPEND); + if (file_put_contents($log_file, $log_data, FILE_APPEND | LOCK_EX) === false) { + error_log('Failed to write to SendGrid events log'); + return false; + } + return true; }Likely invalid or redundant comment.
431-433:⚠️ Potential issueFix variable mismatch in catch block.
The caught exception variable is
$ebut should be consistent with other catch blocks:- } catch (Exception $e) { - return new WP_REST_Response($e->getMessage(), 500); + } catch (Exception $ex) { + return new WP_REST_Response($ex->getMessage(), 500); }Likely invalid or redundant comment.
306-314:⚠️ Potential issueSeparate logging from data transformation and avoid logging PII.
The function currently logs email addresses (PII) and mixes concerns:
function transform_event_data($eventData){ $event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown'; $templateName = isset($eventData['sg_template_name']) ? $eventData['sg_template_name'] : 'unknown'; $email = isset($eventData['email']) ? $eventData['email'] : 'unknown'; $timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown'; $url = isset($eventData['url']) ? $eventData['url'] : 'unknown'; - log_event_data($event_type, $url, $timestamp); return array('event'=>$event_type, 'email'=>$email, 'templateName'=>$templateName); } +function log_sendgrid_event($eventData) { + $event_type = isset($eventData['event']) ? $eventData['event'] : 'unknown'; + $url = isset($eventData['url']) ? $eventData['url'] : 'unknown'; + $timestamp = isset($eventData['timestamp']) ? $eventData['timestamp'] : 'unknown'; + return log_event_data($event_type, $url, $timestamp); +}Likely invalid or redundant comment.
320-326:⚠️ Potential issueAdd JSON validation and improve error handling.
The current implementation doesn't properly validate the JSON input:
'callback' => function(WP_REST_Request $request) { $input = $request->get_body(); + if (empty($input)) { + return new WP_REST_Response('Empty request body', 400); + } $data = json_decode($input, true); + if (json_last_error() !== JSON_ERROR_NONE) { + return new WP_REST_Response('Invalid JSON: ' . json_last_error_msg(), 400); + } if (is_array($data)) { foreach ($data as $event) { + if (!isset($event['event']) || !isset($event['email'])) { + continue; + } $event_data = transform_event_data($event);Likely invalid or redundant comment.
| use Spyc; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused Spyc import.
The Spyc library is imported but not used in this file.
-use Spyc;
-
Summary by CodeRabbit
New Features
Dependencies
Configuration
User Experience