Conversation
tarunnjoshi
force-pushed
the
change-mmt-to-front-facing-form
branch
2 times, most recently
from
237cf7c to
8a95178
Compare
tarunnjoshi
force-pushed
the
change-mmt-to-front-facing-form
branch
from
8a95178 to
51820b1
Compare
WalkthroughThe changes encompass modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JS as main.js
participant PHP as CollectionCampService
participant Email as Email System
User->>JS: Load page with URL hash
JS->>JS: Retrieve message from URL
JS->>User: Display message in custom-message div
User->>PHP: Trigger email generation
PHP->>Email: Send email with vehicleDispatchId
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: 2
🧹 Outside diff range and nitpick comments (5)
wp-content/themes/goonj-crm/main.js (1)
Line range hint
1-59: Potential XSS vulnerability and error handling improvements neededThe code effectively handles different message scenarios, but there are a few areas for improvement:
Security: The
messageparameter from the URL is directly used to determine which HTML content to insert. This could potentially lead to XSS attacks if an attacker can manipulate the URL.Error Handling: The code doesn't handle the case where the 'custom-message' div doesn't exist in the DOM.
To address these issues, consider the following changes:
- Implement a whitelist for allowed message values:
const allowedMessages = ['not-inducted-volunteer', 'individual-user', 'past-collection-data', 'collection-camp-page', 'not-inducted-for-dropping-center']; if (message && allowedMessages.includes(message)) { // Proceed with existing logic }
- Add a check for the existence of the 'custom-message' div:
var messageDiv = document.getElementById('custom-message'); if (messageDiv) { // Existing logic } else { console.error("Element with id 'custom-message' not found"); }
- Consider using
textContentinstead ofinnerHTMLwhere possible to further mitigate XSS risks.Additionally, to improve code maintainability, consider extracting the HTML templates into separate functions or a configuration object. This would make the code more readable and easier to update in the future.
wp-content/themes/goonj-crm/style.css (4)
Line range hint
1-14: LGTM! Consider documenting the purpose of utility classes.The utility classes for width, height, and min/max dimensions are well-structured and cover a good range of sizes. This provides flexibility in layout design.
Consider adding a brief comment at the beginning of this section to explain the purpose and usage of these utility classes. This can help other developers understand and use them effectively.
Line range hint
1-14: LGTM! Consider optimizing responsive styles.The use of media queries and responsive utility classes is a good approach for creating a flexible, responsive design. However, there are a few areas that could be improved:
The repetition of utility classes for each breakpoint (md-, lg-, xl-) might lead to a large CSS file. Consider using a CSS preprocessor like Sass to generate these classes more efficiently.
Some specific component styles are mixed with the utility classes in the media queries. It might be clearer to separate these into their own sections.
Consider using a CSS preprocessor like Sass to generate the responsive utility classes more efficiently. This could significantly reduce the file size and make the code more maintainable.
Also, consider separating the specific component styles from the utility classes within the media queries. This can improve code organization and make it easier to maintain in the future.
Also applies to: 520-1046
Line range hint
15-519: LGTM! Component styles are comprehensive but consider some refinements.The component styles are well-organized and cover various aspects of the theme, including forms, buttons, and layout elements. The integration with CiviCRM components is handled well. However, there are a few areas that could be improved:
The use of !important is frequent. While sometimes necessary for overriding plugin styles, it should be used cautiously as it can lead to specificity issues.
Some styles are very specific (e.g.,
.crm-container .select2-container .select2-choice abbr). Consider if these could be simplified or made more generic.Error and validation styles are present, which is good for user experience. Consider expanding on these for other form states (e.g., success, warning).
Review the use of !important and see if any can be removed by restructuring selectors.
Consider simplifying overly specific selectors where possible.
Expand form state styles to include success and warning states for better user feedback.
Line range hint
1047-1060: Critical: Move theme information to the beginning of the file.The theme information comment block is currently placed at the end of the file. This is contrary to WordPress standards and may cause issues with theme detection.
Move the entire theme information comment block (lines 1047-1060) to the very beginning of the file. This ensures proper theme detection and follows WordPress best practices.
+/*! +Theme Name: Goonj CRM +Theme URI: https://coloredcow.com +Author: coloredcow +Author URI: https://coloredcow.com +Description: Theme for Goonj CRM +Requires at least: 6.0 +Tested up to: 6.5 +Requires PHP: 5.7 +Version: 1.0 +License: GNU General Public License v2 or later +License URI: http://www.gnu.org/licenses/gpl-2.0.html +Text Domain: goonj-crm +Tags: non-profit, crm, coloredcow +*/ + .w-16{width:16px !important}.h-16{height:16px !important}.min-h-16{min-height:16px}.max-h-16{max-height:16px}.min-w-16{min-width:16px}.max-w-16{max-width:16px}.w-25{width:25px !important}.h-25{height:25px !important}.min-h-25{min-height:25px}.max-h-25{max-height:25px}.min-w-25{min-width:25px}.max-w-25{max-width:25px}.w-50{width:50px !important}.h-50{height:50px !important}.min-h-50{min-height:50px}.max-h-50{max-height:50px}.min-w-50{min-width:50px}.max-w-50{max-width:50px}.w-60{width:60px !important}.h-60{height:60px !important}.min-h-60{min-height:60px}.max-h-60{max-height:60px}.min-w-60{min-width:60px}.max-w-60{max-width:60px}.w-75{width:75px !important}.h-75{height:75px !important}.min-h-75{min-height:75px}.max-h-75{max-height:75px}.min-w-75{min-width:75px}.max-w-75{max-width:75px}.w-95{width:95px !important}.h-95{height:95px !important}.min-h-95{min-height:95px}.max-h-95{max-height:95px}.min-w-95{min-width:95px}.max-w-95{max-width:95px}.w-100{width:100px !important}.h-100{height:100px !important}.min-h-100{min-height:100px}.max-h-100{max-height:100px}.min-w-100{min-width:100px}.max-w-100{max-width:100px}.w-140{width:140px !important}.h-140{height:140px !important}.min-h-140{min-height:140px}.max-h-140{max-height:140px}.min-w-140{min-width:140px}.max-w-140{max-width:140px}.w-150{width:150px !important}.h-150{height:150px !important}.min-h-150{min-height:150px}.max-h-150{max-height:150px}.min-w-150{min-width:150px}.max-w-150{max-width:150px}.w-200{width:200px !important}.h-200{height:200px !important}.min-h-200{min-height:200px}.max-h-200{max-height:200px}.min-w-200{min-width:200px}.max-w-200{max-width:200px}.w-520{width:520px !important}.h-520{height:520px !important}.min-h-520{min-height:520px}.max-h-520{max-height:520px}.min-w-520{min-width:520px}.max-w-520{max-width:520px}.w-550{width:550px !important}.h-550{height:550px !important}.min-h-550{min-height:550px}.max-h-550{max-height:550px}.min-w-550{min-width:550px}.max-w-550{max-width:550px}.w-25p{width:25% !important}.h-25p{height:25% !important}.min-h-25p{min-height:25%}.max-h-25p{max-height:25%}.min-w-25p{min-width:25%}.max-w-25p{max-width:25%}.w-50p{width:50% !important}.h-50p{height:50% !important}.min-h-50p{min-height:50%}.max-h-50p{max-height:50%}.min-w-50p{min-width:50%}.max-w-50p{max-width:50%}.w-75p{width:75% !important}.h-75p{height:75% !important}.min-h-75p{min-height:75%}.max-h-75p{max-height:75%}.min-w-75p{min-width:75%}.max-w-75p{max-width:75%}.w-100p{width:100% !important}.h-100p{height:100% !important}.min-h-100p{min-height:100%}.max-h-100p{max-height:100%}.min-w-100p{min-width:100%}.max-w-100p{max-width:100%}.fz-14{font-size:14px !important}.fz-16{font-size:16px !important}.fz-20{font-size:20px !important}.fw-400{font-weight:400 !important}.fw-600{font-weight:600 !important}.mr-0{margin-right:0px !important}.ml-0{margin-left:0px !important}.mt-0{margin-top:0px !important}.mb-0{margin-bottom:0px !important}.pr-0{padding-right:0px !important}.pl-0{padding-left:0px !important}.pt-0{padding-top:0px !important}.pb-0{padding-bottom:0px !important}.mr-2{margin-right:2px !important}.ml-2{margin-left:2px !important}.mt-2{margin-top:2px !important}.mb-2{margin-bottom:2px !important}.pr-2{padding-right:2px !important}.pl-2{padding-left:2px !important}.pt-2{padding-top:2px !important}.pb-2{padding-bottom:2px !important}.mr-6{margin-right:6px !important}.ml-6{margin-left:6px !important}.mt-6{margin-top:6px !important}.mb-6{margin-bottom:6px !important}.pr-6{padding-right:6px !important}.pl-6{padding-left:6px !important}.pt-6{padding-top:6px !important}.pb-6{padding-bottom:6px !important}.mr-11{margin-right:11px !important}.ml-11{margin-left:11px !important}.mt-11{margin-top:11px !important}.mb-11{margin-bottom:11px !important}.pr-11{padding-right:11px !important}.pl-11{padding-left:11px !important}.pt-11{padding-top:11px !important}.pb-11{padding-bottom:11px !important}.mr-12{margin-right:12px !important}.ml-12{margin-left:12px !important}.mt-12{margin-top:12px !important}.mb-12{margin-bottom:12px !important}.pr-12{padding-right:12px !important}.pl-12{padding-left:12px !important}.pt-12{padding-top:12px !important}.pb-12{padding-bottom:12px !important}.mr-24{margin-right:24px !important}.ml-24{margin-left:24px !important}.mt-24{margin-top:24px !important}.mb-24{margin-bottom:24px !important}.pr-24{padding-right:24px !important}.pl-24{padding-left:24px !important}.pt-24{padding-top:24px !important}.pb-24{padding-bottom:24px !important}.mr-25{margin-right:25px !important}.ml-25{margin-left:25px !important}.mt-25{margin-top:25px !important}.mb-25{margin-bottom:25px !important}.pr-25{padding-right:25px !important}.pl-25{padding-left:25px !important}.pt-25{padding-top:25px !important}.pb-25{padding-bottom:25px !important}.mr-27{margin-right:27px !important}.ml-27{margin-left:27px !important}.mt-27{margin-top:27px !important}.mb-27{margin-bottom:27px !important}.pr-27{padding-right:27px !important}.pl-27{padding-left:27px !important}.pt-27{padding-top:27px !important}.pb-27{padding-bottom:27px !important}.mr-30{margin-right:30px !important}.ml-30{margin-left:30px !important}.mt-30{margin-top:30px !important}.mb-30{margin-bottom:30px !important}.pr-30{padding-right:30px !important}.pl-30{padding-left:30px !important}.pt-30{padding-top:30px !important}.pb-30{padding-bottom:30px !important}.mr-36{margin-right:36px !important}.ml-36{margin-left:36px !important}.mt-36{margin-top:36px !important}.mb-36{margin-bottom:36px !important}.pr-36{padding-right:36px !important}.pl-36{padding-left:36px !important}.pt-36{padding-top:36px !important}.pb-36{padding-bottom:36px !important} -/*! -Theme Name: Goonj CRM -Theme URI: https://coloredcow.com -Author: coloredcow -Author URI: https://coloredcow.com -Description: Theme for Goonj CRM -Requires at least: 6.0 -Tested up to: 6.5 -Requires PHP: 5.7 -Version: 1.0 -License: GNU General Public License v2 or later -License URI: http://www.gnu.org/licenses/gpl-2.0.html -Text Domain: goonj-crm -Tags: non-profit, crm, coloredcow -*/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2 hunks)
- wp-content/themes/goonj-crm/main.js (1 hunks)
- wp-content/themes/goonj-crm/style.css (2 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
994-994: Verify that all calls togoonjcustom_material_management_email_htmlinclude the new parameter.The method
goonjcustom_material_management_email_htmlnow includes an additional parameter$vehicleDispatchId. Please ensure that all calls to this method throughout the codebase are updated to include this new parameter to prevent potential errors or unexpected behavior.Run the following script to find all usages of the method and check their parameter counts:
✅ Verification successful
All calls to
goonjcustom_material_management_email_htmlinclude the new parameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all calls to 'goonjcustom_material_management_email_html' and verify they have six parameters. # Test: Search for function calls to 'goonjcustom_material_management_email_html'. # Expect: All calls should include six parameters. rg --type php -Pn 'goonjcustom_material_management_email_html\s*\(' -A 0 | while read -r line; do params=$(echo "$line" | grep -o ',' | wc -l) if [ "$params" -ne 5 ]; then echo "Potential mismatch in parameter count at: $line" fi doneLength of output: 1255
Script:
#!/bin/bash # Description: Find all calls to 'goonjcustom_material_management_email_html' and verify they have six parameters using ast-grep. # Search for function calls to 'goonjcustom_material_management_email_html' with exactly six arguments. ast-grep --lang php --pattern 'callee:identifier[name="goonjcustom_material_management_email_html"]' --write '$ARGS[]' | while read -r args; do param_count=$(echo "$args" | grep -o ',' | wc -l) if [ "$param_count" -ne 5 ]; then echo "Potential mismatch in parameter count: $args" fi doneLength of output: 340
Script:
#!/bin/bash # Description: Find all calls to 'goonjcustom_material_management_email_html' and verify they have six parameters. # Search for function calls to 'goonjcustom_material_management_email_html' excluding its definition. rg --type php -Pn 'goonjcustom_material_management_email_html\s*\(' --context-context=0 | grep -v 'function goonjcustom_material_management_email_html' | while read -r line; do # Extract the parameters within the parentheses params=$(echo "$line" | sed -n 's/.*goonjcustom_material_management_email_html\s*(\(.*\)).*/\1/p') # Count the number of commas to determine the number of parameters param_count=$(echo "$params" | grep -o ',' | wc -l) # Since the number of commas is one less than the number of parameters actual_params=$((param_count + 1)) if [ "$actual_params" -ne 6 ]; then echo "Potential mismatch in parameter count: $line" fi doneLength of output: 613
Script:
#!/bin/bash # Description: Find all calls to 'goonjcustom_material_management_email_html' and verify they have six parameters. # Search for function calls to 'goonjcustom_material_management_email_html' excluding its definition. rg --type php -Pn 'goonjcustom_material_management_email_html\s*\(' --context 0 | grep -v 'function goonjcustom_material_management_email_html' | while read -r line; do # Extract the part within the parentheses params=$(echo "$line" | sed -n 's/.*goonjcustom_material_management_email_html\s*(\(.*\)).*/\1/p') # Remove any potential trailing commas or spaces params=$(echo "$params" | sed 's/,$//;s/\s//g') # Count the number of commas to determine the number of parameters param_count=$(echo "$params" | grep -o ',' | wc -l) # Since the number of commas is one less than the number of parameters actual_params=$((param_count + 1)) if [ "$actual_params" -ne 6 ]; then echo "Potential mismatch in parameter count: $line" fi doneLength of output: 964
| setTimeout(function() { | ||
| const labels = document.querySelectorAll('label.crm-af-field-label'); | ||
|
|
||
| labels.forEach((label) => { | ||
| if (label.textContent.trim() === 'Existing Collection Source Vehicle Dispatch') { | ||
| label.closest('af-field').style.display = 'none'; | ||
| } | ||
| }); | ||
| }, 1000); // Longer delay to allow for dynamic rendering |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve reliability of dynamic element hiding
The current implementation for hiding specific labels has a few areas that could be improved:
- Using
setTimeoutwith a fixed delay might not be reliable for all scenarios, especially if the page load time varies. - The code doesn't handle cases where the label or its parent might not exist.
- The selector used to find the parent element (
closest('af-field')) assumes a specific HTML structure.
Consider the following improvements:
- Use a MutationObserver to react to DOM changes instead of relying on a fixed timeout:
function hideExistingCollectionSourceLabel() {
const labels = document.querySelectorAll('label.crm-af-field-label');
labels.forEach((label) => {
if (label.textContent.trim() === 'Existing Collection Source Vehicle Dispatch') {
const parent = label.closest('af-field');
if (parent) {
parent.style.display = 'none';
}
}
});
}
const observer = new MutationObserver((mutations) => {
for (let mutation of mutations) {
if (mutation.type === 'childList') {
hideExistingCollectionSourceLabel();
}
}
});
observer.observe(document.body, { childList: true, subtree: true });
// Initial check
hideExistingCollectionSourceLabel();- Add error handling to log issues if elements are not found:
if (!parent) {
console.warn(`Parent 'af-field' not found for label: ${label.textContent}`);
}- Consider using data attributes instead of relying on text content for more reliable selection:
<label class="crm-af-field-label" data-field-type="existing-collection-source">Existing Collection Source Vehicle Dispatch</label>if (label.dataset.fieldType === 'existing-collection-source') {
// Hide the label
}For a more maintainable solution, consider implementing a configuration-driven approach to hide or show form fields. This could involve maintaining a list of fields to hide in a configuration object or file, making it easier to update without changing the code.
| public static function goonjcustom_material_management_email_html($mmtId, $contactName, $collectionCampId, $campCode, $campAddress, $vehicleDispatchId) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
| $materialdispatchUrl = $homeUrl . 'wp-admin/admin.php?page=CiviCRM&q=civicrm%2Feck%2Fentity&reset=1&type=Collection_Camp&id=' . $collectionCampId . '&selectedChild=materialAuthorization#?intent_id=' . $collectionCampId . '&Camp_Vehicle_Dispatch.Filled_by=' . $mmtId; | ||
|
|
||
| $materialdispatchUrl = $homeUrl . 'acknowledgement-form-for-logistics/#?Acknowledgement_For_Logistics.Filled_by=' . $mmtId . '&Camp_Vehicle_Dispatch.Collection_Camp_Intent_Id=' . $collectionCampId . '&id=' . $vehicleDispatchId; |
There was a problem hiding this comment.
Properly encode URL parameters when constructing URLs.
In the goonjcustom_material_management_email_html function, the URL $materialdispatchUrl is constructed by directly concatenating variables, which may lead to security vulnerabilities or incorrect URL formatting if the variables contain special characters. To ensure the URL is correctly formatted and secure, consider using http_build_query to encode the query parameters.
Apply this refactor to safely construct the URL:
public static function goonjcustom_material_management_email_html($mmtId, $contactName, $collectionCampId, $campCode, $campAddress, $vehicleDispatchId) {
$homeUrl = \CRM_Utils_System::baseCMSURL();
- $materialdispatchUrl = $homeUrl . 'acknowledgement-form-for-logistics/#?Acknowledgement_For_Logistics.Filled_by=' . $mmtId . '&Camp_Vehicle_Dispatch.Collection_Camp_Intent_Id=' . $collectionCampId . '&id=' . $vehicleDispatchId;
+ $queryParams = http_build_query([
+ 'Acknowledgement_For_Logistics.Filled_by' => $mmtId,
+ 'Camp_Vehicle_Dispatch.Collection_Camp_Intent_Id' => $collectionCampId,
+ 'id' => $vehicleDispatchId,
+ ]);
+ $materialdispatchUrl = $homeUrl . 'acknowledgement-form-for-logistics/#?' . $queryParams;
$html = "
<p>Dear MMT team,</p>
<p>This is to inform you that a vehicle has been sent from camp <strong>$campCode</strong> at <strong>$campAddress</strong>.</p>
<p>Kindly acknowledge the details by clicking on this form <a href=\"$materialdispatchUrl\"> Link </a> when it is received at the center.</p>
<p>Warm regards,<br>Urban Relations Team</p>";
return $html;
}📝 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.
| public static function goonjcustom_material_management_email_html($mmtId, $contactName, $collectionCampId, $campCode, $campAddress, $vehicleDispatchId) { | |
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | |
| $materialdispatchUrl = $homeUrl . 'wp-admin/admin.php?page=CiviCRM&q=civicrm%2Feck%2Fentity&reset=1&type=Collection_Camp&id=' . $collectionCampId . '&selectedChild=materialAuthorization#?intent_id=' . $collectionCampId . '&Camp_Vehicle_Dispatch.Filled_by=' . $mmtId; | |
| $materialdispatchUrl = $homeUrl . 'acknowledgement-form-for-logistics/#?Acknowledgement_For_Logistics.Filled_by=' . $mmtId . '&Camp_Vehicle_Dispatch.Collection_Camp_Intent_Id=' . $collectionCampId . '&id=' . $vehicleDispatchId; | |
| public static function goonjcustom_material_management_email_html($mmtId, $contactName, $collectionCampId, $campCode, $campAddress, $vehicleDispatchId) { | |
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | |
| $queryParams = http_build_query([ | |
| 'Acknowledgement_For_Logistics.Filled_by' => $mmtId, | |
| 'Camp_Vehicle_Dispatch.Collection_Camp_Intent_Id' => $collectionCampId, | |
| 'id' => $vehicleDispatchId, | |
| ]); | |
| $materialdispatchUrl = $homeUrl . 'acknowledgement-form-for-logistics/#?' . $queryParams; |
2 participants
Change mmt to frontend
Mobile View:
Phone View:
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores