Added service file and redirections for diffrent button in events#678
Added service file and redirections for diffrent button in events#678nishant22029 wants to merge 27 commits intodevelopfrom
Conversation
WalkthroughThis pull request introduces event-related functionality to the Goonj Blocks WordPress plugin. The changes span across multiple files in the plugin, primarily focusing on adding support for 'events' as a new target type. The modifications include updating rendering logic, expanding API interactions, and introducing new data structures to handle event-specific information such as start time, end time, address, and contribution links. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Nitpick comments (8)
wp-content/plugins/goonj-blocks/goonj-blocks.php (1)
159-159: Remove or reconsider excessive logging
While logging is beneficial for debugging, logging entire data objects in production can clutter logs and potentially reveal sensitive info. Please make sure redaction or care is in place if personal data is included.wp-content/plugins/goonj-blocks/build/render.php (4)
84-89: Potential cross-file duplication for $event_material_contribution_link
You introduced an $event_material_contribution_link that mirrors existing pattern setups, which is good for consistency. However, there is some duplication in both build/render.php and src/render.php. Consider extracting these repeated link-building statements to a helper function or method to adhere to the DRY (Don’t Repeat Yourself) principle.- $event_material_contribution_link = sprintf( - '/events-contribution-verification/?source=%s&target_id=%s', - $action_target['title'], - $action_target['id'], - ); ... + // Example approach: + $event_material_contribution_link = gb_generate_link('events-contribution-verification', $action_target);
215-223: New 'events' target data
Including 'events' in $target_data with start/end times and addresses is aligned with how other entities are handled. However, the sample label for 'events' is "Address of the camp," which may be misleading if these are not strictly "camps." Consider a more neutral label, like "Address of the Event."- 'address_label' => 'Address of the camp', + 'address_label' => 'Address of the event',
226-226: Expanded if-condition for 'events'
Including 'events' in the condition ensures the code below processes event data. Keep an eye on the complexity of this condition. If you keep adding more targets, you might want to consider a more modular approach or a switch statement.
278-286: Address rendering for 'events'
You're correctly switching to CRM_Utils_Address::format for 'events'. Make sure you handle the scenario when $address is null or missing fields—particularly if the event does not have a defined primary address.if ($target === 'events' && !empty($address)) { echo CRM_Utils_Address::format($address); } else { echo esc_html($address); }wp-content/plugins/goonj-blocks/src/render.php (3)
84-89: Duplication of link-building logic
As noted in build/render.php, this link-building code is also repeated. Consider creating a shared helper function for consistency and easier maintenance.
215-223: Embedding 'events' in $target_data
Equivalent to the changes in build/render.php. Same suggestion applies regarding “Address of the event” label.
278-286: Address rendering for 'events'
Fine approach. As recommended for build/render.php, consider safe-checking that the $address isn’t empty.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wp-content/plugins/goonj-blocks/build/render.php(5 hunks)wp-content/plugins/goonj-blocks/goonj-blocks.php(2 hunks)wp-content/plugins/goonj-blocks/src/render.php(5 hunks)
🔇 Additional comments (7)
wp-content/plugins/goonj-blocks/goonj-blocks.php (2)
34-34: Add 'events' action to rewrite rules
The inclusion of 'events' here is good for ensuring that the custom rewrite rule will handle the /actions/events/ route properly. However, consider clarifying the naming conventions or ensure you have consistent coverage of other event-related routes if you want granular route handling, such as indexing or other detail pages.
163-187: New 'events' case
This block correctly retrieves the event data and associated primary address, then sets it on the global query object. The approach is consistent with other action types. Consider adding more robust error handling (e.g., if the address is missing).
wp-content/plugins/goonj-blocks/build/render.php (2)
28-29: Introduction of 'events' heading
Adding 'events' to the $headings array ensures the UI properly labels event pages. This is consistent with your approach for other types.
262-262: Inclusion of 'events' in date/time row
Expanding the same date/time UI sections for 'events' maintains consistency. Ensure correct date/time formatting if event date/time fields differ from the other targets.
✅ Verification successful
Let me gather more information about the date/time UI sections and their implementation across different targets.
Date/time formatting for events is properly implemented
Based on the code analysis, the 'events' target is correctly integrated into the date/time UI sections with proper field mappings:
- Uses consistent date/time fields: 'start_date' and 'end_date'
- Shares the same date/time formatting functions:
gb_format_date()andgb_format_time_range() - Follows the same UI structure as other targets in the conditional block
The only difference is in address formatting where events use CRM_Utils_Address::format() instead of esc_html(), which is an intentional distinction.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for date/time related code sections in render.php
rg -B 5 -A 10 "date|time" wp-content/plugins/goonj-blocks/build/render.php
# Look for event-specific date/time field implementations
rg -B 3 -A 3 "events.*date|events.*time" wp-content/plugins/goonj-blocks/build/render.php
# Search for the complete conditional block structure
rg -B 2 -A 10 "\$target === '(collection-camp|institution-collection-camp|goonj-activities|institution-goonj-activities|events)'" wp-content/plugins/goonj-blocks/build/render.php
Length of output: 8463
wp-content/plugins/goonj-blocks/src/render.php (3)
28-29: 'events' heading
Same feedback as build/render.php: good introduction of the label "Goonj Events."
226-226: Conditional inclusion of 'events'
Same feedback: it’s correct to expand the if-condition to handle 'events'; watch out for future expansions.
262-262: Add 'events' to the date/time logic
Same as build/render.php. Confirm date/time are accurately retrieved from the needed event fields.
✅ Verification successful
Let me search for the date/time logic implementation to verify if it properly handles the 'events' target.
Let me try to find the date/time logic for events in the codebase.
Date/time fields for events are properly handled
Based on the code inspection, the date/time logic for 'events' target is correctly implemented:
- The 'events' target is included in the conditional block that handles date/time display
- The date/time fields are properly mapped in the target_data array:
- 'start_time' => 'start_date'
- 'end_time' => 'end_date'
- The values are formatted consistently using the same functions as other targets:
- gb_format_date() for start and end dates
- gb_format_time_range() for time display
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for date/time related code in render.php files
rg -A 10 "date|time" --type php "render.php$"
Length of output: 99
Script:
#!/bin/bash
# Search for event-related date/time handling
rg -A 10 "events.*date|events.*time" --type php
Length of output: 43651
Script:
#!/bin/bash
# Look for the complete conditional block containing date/time logic
rg -B 5 -A 15 "\$target === '(collection-camp|institution-collection-camp|goonj-activities|institution-goonj-activities|events)'" --type php
Length of output: 147
Script:
#!/bin/bash
# Search for the file content around the target condition
rg -B 10 -A 20 "target === 'collection-camp'" wp-content/plugins/goonj-blocks/src/render.php
Length of output: 1942
Script:
#!/bin/bash
# Look for date/time related code in the render.php file
rg -B 5 -A 10 "date|time" wp-content/plugins/goonj-blocks/src/render.php
Length of output: 8183
Script:
#!/bin/bash
# Search for event-related fields and their handling
rg -B 5 -A 10 "events.*field|event.*date|event.*time" wp-content/plugins/goonj-blocks/src/
Length of output: 92
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (1)
91-95: Grammatical error in email body.
The text “Their is one forms that require your attention…” is grammatically incorrect. Rework to ensure clarity.Suggested refactor:
- <p>Thank you for attending the goonj activity <strong>$eventId</strong> at <strong>$eventAddress</strong>. Their is one forms that require your attention... + <p>Thank you for attending the goonj activity <strong>$eventId</strong> at <strong>$eventAddress</strong>. There is one form requiring your attention...wp-content/themes/goonj-crm/templates/form-check-user.php (1)
12-12: Consider centralizing the “purposes not requiring email” array.
The addition of 'event-material-contribution' here is valid, but the same array is repeated in multiple places in the codebase. Centralizing this list in a single configuration or constant would help maintain consistency and avoid mistakes when adding or removing entries in the future.wp-content/themes/goonj-crm/functions.php (2)
150-150: Avoid repeating “email not required” logic in multiple locations.
This array is defined here and in “form-check-user.php” (line 12). Consider storing it in a single location (e.g., a config file, global constants, or a helper function) to eliminate duplication.
326-335: Validate newly introduced case for ‘event-material-contribution’.
This new switch case for ‘event-material-contribution’ follows the existing pattern. However, consider extracting common logic (e.g., building URLs with shared parameters) into a reusable function to reduce potential duplication across different cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjInitiatedEventsCron.php(1 hunks)wp-content/themes/goonj-crm/functions.php(4 hunks)wp-content/themes/goonj-crm/templates/form-check-user.php(1 hunks)
🔇 Additional comments (1)
wp-content/themes/goonj-crm/functions.php (1)
428-438: Ensure consistent handling of event-based flows for existing contacts.
These lines effectively redirect existing contacts to the new events material contribution form. The logic is consistent with other contribution flows. However, stay vigilant for potential differences or additional parameters unique to events that might require special handling.
| * Goonjcustom.GoonjActivitiesCampCron API specification (optional) | ||
| * This is used for documentation and validation. | ||
| * | ||
| * @param array $spec |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DocBlock naming mismatch.
The docblock references "GoonjActivitiesCampCron" but the actual function is named "goonj_initiated_events_cron". This might cause confusion for future maintainers and potential API consumers.
| \Civi::log()->info('Error Goonj Activities Cron', [ | ||
| 'id' => $camp['id'], | ||
| 'error' => $e->getMessage(), | ||
| ]); | ||
| } |
There was a problem hiding this comment.
Undefined variable $camp.
Inside the catch block, the code references $camp['id'] even though $camp is not defined. It’s likely intended to reference $event['id'].
Suggested fix:
- 'id' => $camp['id'],
+ 'id' => $event['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.
| \Civi::log()->info('Error Goonj Activities Cron', [ | |
| 'id' => $camp['id'], | |
| 'error' => $e->getMessage(), | |
| ]); | |
| } | |
| \Civi::log()->info('Error Goonj Activities Cron', [ | |
| 'id' => $event['id'], | |
| 'error' => $e->getMessage(), | |
| ]); | |
| } |
| \Civi::log()->error("Error in sendLogisticsEmail for $campId " . $e->getMessage()); | ||
| } |
There was a problem hiding this comment.
Undefined variable $campId.
In the exception message, '$campId' is referenced, but there's no variable named $campId in this scope. Consider replacing it with $eventId or removing it if it's not needed.
Suggested fix:
- \Civi::log()->error("Error in sendLogisticsEmail for $campId " . $e->getMessage());
+ \Civi::log()->error("Error in sendLogisticsEmail for $eventId " . $e->getMessage());📝 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.
| \Civi::log()->error("Error in sendLogisticsEmail for $campId " . $e->getMessage()); | |
| } | |
| \Civi::log()->error("Error in sendLogisticsEmail for $eventId " . $e->getMessage()); | |
| } |
| $campOutcomeFormUrl = $homeUrl . 'goonj-initiated-events-outcome/#?Event1=' . $eventId . '&Goonj_Events_Outcome.Filled_By=' . $campAttendedById; | ||
|
|
||
| $html = " | ||
| <p>Dear $attendeeName,</p> | ||
| <p>Thank you for attending the goonj activity <strong>$eventId</strong> at <strong>$eventAddress</strong>. Their is one forms that require your attention during and after the goonj activity:</p> | ||
| <ol> | ||
| Please complete this form from the goonj activity location once the goonj activity ends.</li> | ||
| <li><a href=\"$campOutcomeFormUrl\">Goonj Activity Outcome Form</a><br> |
There was a problem hiding this comment.
Inconsistent variable name used in URL.
The query parameter is "…&Goonj_Events_Outcome.Filled_By=' . $campAttendedById", while the variable is named $eventAttendedById. This mismatch can cause confusion or incorrect parameter usage.
Suggested fix:
- $campOutcomeFormUrl = $homeUrl . 'goonj-initiated-events-outcome/#?Event1=' . $eventId . '&Goonj_Events_Outcome.Filled_By=' . $campAttendedById;
+ $campOutcomeFormUrl = $homeUrl . 'goonj-initiated-events-outcome/#?Event1=' . $eventId . '&Goonj_Events_Outcome.Filled_By=' . $eventAttendedById;Committable suggestion skipped: line range outside the PR's diff.
| case 'event-material-contribution': | ||
| $sourceProcessingCenter = $individual['Individual_fields.Source_Processing_Center']; | ||
| $events = \Civi\Api4\Event::get(TRUE) | ||
| ->addWhere('title', '=', $source) | ||
| ->execute()->first(); | ||
| $redirectPath = sprintf( | ||
| '/events-material-contribution/#?Material_Contribution.Event=%s&source_contact_id=%s', | ||
| $events['id'], | ||
| $individual['id'] | ||
| ); | ||
| break; |
There was a problem hiding this comment.
💡 Codebase verification
Add null check before accessing event ID in the redirect path
The codebase shows a pattern where first() results are consistently checked with !empty() before accessing their properties, but this pattern is missing in the event-material-contribution case. This could lead to a PHP error if no matching event is found.
- In
wp-content/themes/goonj-crm/functions.php, add a null check before using$events['id']in the redirect path, similar to other cases in the same file:
if (!empty($events['id'])) {
$redirectPath = sprintf(
'/events-material-contribution/#?Material_Contribution.Event=%s&source_contact_id=%s',
$events['id'],
$individual['id']
);
} else {
// Handle the case where no matching event is found
$redirectPath = '/events-material-contribution/';
}🔗 Analysis chain
Check for possible multiple or missing Event records when redirecting.
The code assumes a single match for the event title. If multiple events share the same title or if none is found, this block could fail. To mitigate this risk, consider verifying the query result count or implementing a fallback if the event is not found.
Let me verify this by checking the codebase for similar event title handling patterns and potential error handling mechanisms.
This will help us understand:
- How event queries are typically handled in the codebase
- Whether there's existing error handling for similar scenarios
- Common patterns for dealing with potential multiple or missing results
Let me analyze the results to provide a concrete recommendation.
Let me try a different search strategy to find Event query patterns.
This should help us:
- Locate files that use Event queries
- Understand how first() results are typically handled in the codebase
- Identify if there are any error handling patterns we should follow
Let me try one final search to find the error handling patterns around Event queries.
This should help us understand:
- How Event queries are structured in the relevant files
- Whether there's any error handling around execute()->first() calls
- Common patterns for handling potential null results
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for Event::get usage patterns to understand how events are typically queried
ast-grep --pattern 'Event::get($$$)->addWhere($$$)->execute()'
# Search for error handling patterns around Event queries
rg -B 2 -A 2 "Event::get.*execute"
Length of output: 9336
Script:
#!/bin/bash
# Search for Event query patterns using ripgrep
rg -l "\\\\Civi\\\\Api4\\\\Event::get"
# Then search for first() usage patterns
rg -B 3 -A 3 "->first\(\)"
Length of output: 966
Script:
#!/bin/bash
# Search for Event query patterns in the identified files
rg -A 5 "Event::get" wp-content/themes/goonj-crm/functions.php wp-content/plugins/goonj-blocks/goonj-blocks.php
# Search for error handling around execute()
rg -B 2 -A 2 "execute\(\)->first" wp-content/themes/goonj-crm/functions.php wp-content/plugins/goonj-blocks/goonj-blocks.php
Length of output: 3252
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
wp-content/themes/goonj-crm/assets/styles/goonj-main.scss (1)
Line range hint
1-400: Improve maintainability with SCSS best practicesThe file has several maintainability concerns that should be addressed:
- Excessive nesting (up to 8 levels deep) makes the code harder to read and maintain
- Heavy use of
!importantindicates specificity issues- Repeated declarations for font-family, padding, margins, etc.
- Long, specific selectors that are brittle and hard to maintain
Consider these improvements:
- Limit nesting to 3-4 levels max
- Extract common styles into reusable classes or mixins
- Use BEM naming to reduce specificity and eliminate need for
!important- Create variables for common values:
// Variables $font-family-base: "Open Sans", sans-serif; $border-radius: 4px; $spacing-unit: 8px; // Mixins for common styles @mixin form-input { border: 1px solid rgba(0, 0, 0, 0.3); border-radius: $border-radius; padding: $spacing-unit; background-image: none; } @mixin base-text { font-family: $font-family-base; line-height: 16px; } // Example BEM structure .crm-form { &__fieldset { border-top: none; } &__input { @include form-input; } &__label { @include base-text; text-align: justify; width: 50%; } }This would make the code more maintainable and easier to update.
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (2)
31-82: Consider Splitting Method LogicThe sendEventOutcomeEmail method handles both retrieval of contact info and sending mail, as well as updating the event. Splitting it into smaller dedicated methods (e.g., retrieving contact info, constructing mail parameters, sending mail, and updating the event) will make it more readable and maintainable.
256-319: Tabset Configuration for Material ContributionsThe new method goonjActivitiesTabset handles adding a custom tab for “Material Contributions.” The approach of checking permissions and injecting AngularJS modules is clear. Please confirm the intended user roles have the correct permission sets to see the new tab.
wp-content/themes/goonj-crm/style.css (1)
15-15: Avoid Overriding Background Shorthand After background-imageStatic analysis indicates an unexpected shorthand property "background" after "background-image:none". For clarity and maintainability, consider removing or merging duplicative properties to avoid overrides that could cause confusion or undesired styling.
🧰 Tools
🪛 Biome (1.9.4)
[error] 15-15: Unexpected shorthand property background after background-image
(lint/suspicious/noShorthandPropertyOverrides)
[error] 15-15: Unexpected shorthand property text-decoration after text-decoration-thickness
(lint/suspicious/noShorthandPropertyOverrides)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php(1 hunks)wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/Events.tpl(1 hunks)wp-content/themes/goonj-crm/assets/styles/goonj-main.scss(2 hunks)wp-content/themes/goonj-crm/style.css(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
wp-content/themes/goonj-crm/style.css
[error] 15-15: Unexpected shorthand property background after background-image
(lint/suspicious/noShorthandPropertyOverrides)
[error] 15-15: Unexpected shorthand property text-decoration after text-decoration-thickness
(lint/suspicious/noShorthandPropertyOverrides)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/Events.tpl (1)
1-5: Looks Good: AngularJS Integration for Events
This template snippet effectively integrates AngularJS components for event data. It dynamically passes the event ID to the directive, keeping the structure concise and readable.
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (2)
21-26: Modular and Extensible Event Hook Subscriptions
The subscription to CiviCRM hooks for event QR generation and tab configuration is well-structured. This approach centralizes event-specific logic in one class, promoting single responsibility and maintainability.
78-79: Undefined variable $campId — Previously Flagged
This was noted in a prior review. The variable $campId does not exist in the current scope, but is referenced in the exception log string. Please replace it with $eventId or remove it if not needed.
| fieldset > div:nth-last-child(4) { | ||
| display: none !important; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate nth-last-child rules
The same rule for hiding the 4th-to-last child is duplicated in two places. This violates the DRY principle and makes maintenance harder.
Consider consolidating these rules using a shared selector or SCSS mixin:
// Define a mixin for hiding elements
@mixin hide-last-n-children($count) {
> div:nth-last-child(-n+#{$count}) {
display: none !important;
}
}
// Use it in both places
.crm-public-form-item fieldset,
#crm-main-content-wrapper form .crm-block fieldset fieldset {
@include hide-last-n-children(4);
}Also applies to: 277-279
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (1)
13-14: Consider splitting the service class based on responsibilities.The class currently handles multiple unrelated responsibilities (email sending, QR code generation, and tab management), which violates the Single Responsibility Principle.
Consider splitting into separate services:
EventEmailService- Handle email-related operationsEventQRCodeService- Handle QR code generationEventTabService- Handle tab managementwp-content/plugins/goonj-blocks/src/render.php (1)
235-235: Simplify target type checking.The repeated use of
in_arraywith a long list of targets could be simplified using a constant or configuration array.+const VALID_TARGETS = [ + 'collection-camp', + 'institution-collection-camp', + 'dropping-center', + 'goonj-activities', + 'institution-dropping-center', + 'institution-goonj-activities', + 'events' +]; -if (in_array($target, ['collection-camp','institution-collection-camp', 'dropping-center', 'goonj-activities', 'institution-dropping-center', 'institution-goonj-activities', 'events'])) : +if (in_array($target, VALID_TARGETS)) :Also applies to: 271-271
wp-content/plugins/goonj-blocks/build/render.php (2)
289-294: Consider consistent address formattingThe code uses different address formatting methods based on the target type. This inconsistency could lead to maintenance issues.
Consider using
CRM_Utils_Address::format()consistently across all target types if it provides better formatting:- if ($target === 'events') { - echo CRM_Utils_Address::format($address); - } else { - echo esc_html($address); - } + echo CRM_Utils_Address::format($address);Alternatively, document why events require different address formatting.
235-235: Simplify target validationThe target validation uses a long
in_arraycheck with multiple values. This could be simplified using array operations.Consider using a constant or configuration array:
+ const VALID_TARGETS = [ + 'collection-camp', + 'institution-collection-camp', + 'dropping-center', + 'goonj-activities', + 'institution-dropping-center', + 'institution-goonj-activities', + 'events' + ]; - if (in_array($target, ['collection-camp','institution-collection-camp', 'dropping-center', 'goonj-activities', 'institution-dropping-center', 'institution-goonj-activities', 'events'])) : + if (in_array($target, VALID_TARGETS)) :
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php(1 hunks)wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/Events.tpl(1 hunks)wp-content/plugins/goonj-blocks/build/render.php(6 hunks)wp-content/plugins/goonj-blocks/src/render.php(6 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (1)
78-79: Fix variable naming inconsistency.
The error message uses $campId but the variable is named $eventId.
wp-content/plugins/goonj-blocks/build/render.php (1)
28-29: LGTM: Heading entry follows existing pattern
The addition of events-related headings maintains consistency with the existing code structure.
| <crm-angular-js modules="{$tabValue.module}"> | ||
| <form id="bootstrap-theme"> | ||
| <{$tabValue.directive} options="{ldelim}eventID: {$tabValue.entity.id}{rdelim}"></{$tabValue.directive}> | ||
| </form> |
There was a problem hiding this comment.
Add CSRF protection to the form.
The form lacks CSRF protection which could make it vulnerable to cross-site request forgery attacks.
Add a CSRF token to the form:
<form id="bootstrap-theme">
+ {crmKey name="crm-token"}
<{$tabValue.directive} options="{ldelim}eventID: {$tabValue.entity.id}{rdelim}"></{$tabValue.directive}>
</form>📝 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.
| <crm-angular-js modules="{$tabValue.module}"> | |
| <form id="bootstrap-theme"> | |
| <{$tabValue.directive} options="{ldelim}eventID: {$tabValue.entity.id}{rdelim}"></{$tabValue.directive}> | |
| </form> | |
| <crm-angular-js modules="{$tabValue.module}"> | |
| <form id="bootstrap-theme"> | |
| {crmKey name="crm-token"} | |
| <{$tabValue.directive} options="{ldelim}eventID: {$tabValue.entity.id}{rdelim}"></{$tabValue.directive}> | |
| </form> |
| <script> | ||
| console.log("Tab Value:", {$tabValue|json_encode|escape:'js'}); | ||
| console.log("Event ID:", {$tabValue.entity.id|default:'undefined'}); | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug console.log statements.
Debug logging should not be present in production code as it could expose sensitive information.
- <script>
- console.log("Tab Value:", {$tabValue|json_encode|escape:'js'});
- console.log("Event ID:", {$tabValue.entity.id|default:'undefined'});
-</script>| // function goonjActivitiesTabset($tabsetName, &$tabs, $context) { | ||
| // // Check if the tabset is for Event Management | ||
| // if ($tabsetName == 'civicrm/event/manage') { | ||
| // // Ensure the event ID is available in the context | ||
| // if (!empty($context['event_id'])) { | ||
| // $eventID = $context['event_id']; | ||
| // \Civi::log()->info('eventID', ['eventID'=>$eventID]); | ||
|
|
||
| // // Construct the URL to the Search Kit view with the event ID filter in hash fragment | ||
| // $url = \CRM_Utils_System::url( | ||
| // 'civicrm/events-material-contributions' | ||
| // ) . "#?Material_Contribution.Event=$eventID"; | ||
|
|
||
| // // Add the "Material Contributions" tab | ||
| // $newTab = [ | ||
| // 'id' => 'material_contributions', // Unique identifier for the tab | ||
| // 'title' => ts('Material Contributions'), | ||
| // 'link' => $url, // URL to the Search Kit view | ||
| // 'valid' => true, // Ensures the tab is valid | ||
| // 'active' => true, // Activates the tab | ||
| // 'current' => false, // Indicates this tab is not the currently active tab | ||
| // ]; | ||
|
|
||
| // // Insert the new tab into the tabs array (e.g., at position 4) | ||
| // $tabs = array_merge( | ||
| // array_slice($tabs, 0, 4), // Tabs before the new tab | ||
| // [$newTab], // The new tab | ||
| // array_slice($tabs, 4) // Tabs after the new tab | ||
| // ); | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code.
Large blocks of commented-out code make the file harder to maintain and understand. If this code is no longer needed, remove it. If it might be needed later, track it in version control.
Also applies to: 202-254
| $eventAttendedBy = Contact::get(FALSE) | ||
| ->addSelect('email.email', 'display_name') | ||
| ->addJoin('Email AS email', 'LEFT') | ||
| ->addWhere('id', '=', $eventAttendedById) | ||
| ->execute()->single(); |
There was a problem hiding this comment.
Add error handling for API calls.
The Contact API call lacks proper error handling. If the contact is not found, it will throw an exception that's not properly caught.
- $eventAttendedBy = Contact::get(FALSE)
- ->addSelect('email.email', 'display_name')
- ->addJoin('Email AS email', 'LEFT')
- ->addWhere('id', '=', $eventAttendedById)
- ->execute()->single();
+ try {
+ $eventAttendedBy = Contact::get(FALSE)
+ ->addSelect('email.email', 'display_name')
+ ->addJoin('Email AS email', 'LEFT')
+ ->addWhere('id', '=', $eventAttendedById)
+ ->execute()->single();
+ } catch (\Exception $e) {
+ \Civi::log()->error('Failed to fetch contact details: ' . $e->getMessage());
+ throw new \Exception('Failed to fetch contact details');
+ }📝 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.
| $eventAttendedBy = Contact::get(FALSE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $eventAttendedById) | |
| ->execute()->single(); | |
| try { | |
| $eventAttendedBy = Contact::get(FALSE) | |
| ->addSelect('email.email', 'display_name') | |
| ->addJoin('Email AS email', 'LEFT') | |
| ->addWhere('id', '=', $eventAttendedById) | |
| ->execute()->single(); | |
| } catch (\Exception $e) { | |
| \Civi::log()->error('Failed to fetch contact details: ' . $e->getMessage()); | |
| throw new \Exception('Failed to fetch contact details'); | |
| } |
| $event_material_contribution_link = sprintf( | ||
| '/events-contribution-verification/?source=%s&target_id=%s', | ||
| $action_target['title'], | ||
| $action_target['id'], | ||
| ); |
There was a problem hiding this comment.
Sanitize URL parameters for event contribution link.
The URL parameters are not properly sanitized before being used in the link generation.
$event_material_contribution_link = sprintf(
'/events-contribution-verification/?source=%s&target_id=%s',
- $action_target['title'],
- $action_target['id'],
+ urlencode($action_target['title']),
+ urlencode($action_target['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.
| $event_material_contribution_link = sprintf( | |
| '/events-contribution-verification/?source=%s&target_id=%s', | |
| $action_target['title'], | |
| $action_target['id'], | |
| ); | |
| $event_material_contribution_link = sprintf( | |
| '/events-contribution-verification/?source=%s&target_id=%s', | |
| urlencode($action_target['title']), | |
| urlencode($action_target['id']), | |
| ); |
| 'events' => [ | ||
| 'start_time' => 'start_date', | ||
| 'end_time' => 'end_date', | ||
| 'address' => 'address', | ||
| 'address_label' => 'Address of the camp', | ||
| 'contribution_link' => $event_material_contribution_link, | ||
| 'donation_link' => $event_donation_link, | ||
| 'register_link' => $institution_goonj_activities_register_link | ||
| ] |
There was a problem hiding this comment.
Fix incorrect register link in events configuration.
The events configuration is using $institution_goonj_activities_register_link instead of a dedicated events register link.
'events' => [
'start_time' => 'start_date',
'end_time' => 'end_date',
'address' => 'address',
'address_label' => 'Address of the camp',
'contribution_link' => $event_material_contribution_link,
'donation_link' => $event_donation_link,
- 'register_link' => $institution_goonj_activities_register_link
+ 'register_link' => sprintf(
+ '/volunteer-registration/form/#?source=%s&state_province_id=%s&city=%s',
+ urlencode($action_target['title']),
+ urlencode($action_target['state']),
+ urlencode($action_target['city'])
+ )
]Committable suggestion skipped: line range outside the PR's diff.
| $eventSourceField = CustomField::get(FALSE) | ||
| ->addSelect('id') | ||
| ->addWhere('custom_group_id:name', '=', 'Contribution_Details') | ||
| ->addWhere('name', '=', 'Events') | ||
| ->execute()->single(); | ||
| $eventSourceFieldId = 'custom_' . $eventSourceField['id']; | ||
| $base_event_donation_link = home_url('/contribute/donate'); | ||
| $event_donation_link = $base_event_donation_link . '?' . $eventSourceFieldId . '=' . $source_contact_id; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract custom field retrieval logic
The custom field retrieval pattern is repeated three times in the code (for source, PU source, and event source fields). This violates the DRY principle.
Consider extracting this into a reusable function:
+ function get_custom_field_id($group_name, $field_name) {
+ $field = CustomField::get(FALSE)
+ ->addSelect('id')
+ ->addWhere('custom_group_id:name', '=', $group_name)
+ ->addWhere('name', '=', $field_name)
+ ->execute()
+ ->single();
+ return 'custom_' . $field['id'];
+ }
- $eventSourceField = CustomField::get(FALSE)
- ->addSelect('id')
- ->addWhere('custom_group_id:name', '=', 'Contribution_Details')
- ->addWhere('name', '=', 'Events')
- ->execute()->single();
- $eventSourceFieldId = 'custom_' . $eventSourceField['id'];
+ $eventSourceFieldId = get_custom_field_id('Contribution_Details', 'Events');📝 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.
| $eventSourceField = CustomField::get(FALSE) | |
| ->addSelect('id') | |
| ->addWhere('custom_group_id:name', '=', 'Contribution_Details') | |
| ->addWhere('name', '=', 'Events') | |
| ->execute()->single(); | |
| $eventSourceFieldId = 'custom_' . $eventSourceField['id']; | |
| $base_event_donation_link = home_url('/contribute/donate'); | |
| $event_donation_link = $base_event_donation_link . '?' . $eventSourceFieldId . '=' . $source_contact_id; | |
| function get_custom_field_id($group_name, $field_name) { | |
| $field = CustomField::get(FALSE) | |
| ->addSelect('id') | |
| ->addWhere('custom_group_id:name', '=', $group_name) | |
| ->addWhere('name', '=', $field_name) | |
| ->execute() | |
| ->single(); | |
| return 'custom_' . $field['id']; | |
| } | |
| $eventSourceFieldId = get_custom_field_id('Contribution_Details', 'Events'); | |
| $base_event_donation_link = home_url('/contribute/donate'); | |
| $event_donation_link = $base_event_donation_link . '?' . $eventSourceFieldId . '=' . $source_contact_id; |
| 'events' => [ | ||
| 'start_time' => 'start_date', | ||
| 'end_time' => 'end_date', | ||
| 'address' => 'address', | ||
| 'address_label' => 'Address of the camp', | ||
| 'contribution_link' => $event_material_contribution_link, | ||
| 'donation_link' => $event_donation_link, | ||
| 'register_link' => $institution_goonj_activities_register_link | ||
| ] |
There was a problem hiding this comment.
Verify the register_link assignment
The events target is using $institution_goonj_activities_register_link for its register_link. This seems incorrect as it's using an institution-specific link for a general event.
Consider creating a dedicated registration link for events:
'events' => [
'start_time' => 'start_date',
'end_time' => 'end_date',
'address' => 'address',
'address_label' => 'Address of the camp',
'contribution_link' => $event_material_contribution_link,
'donation_link' => $event_donation_link,
- 'register_link' => $institution_goonj_activities_register_link
+ 'register_link' => sprintf(
+ '/volunteer-registration/form/#?source=%s&event_id=%s',
+ $action_target['title'],
+ $action_target['id']
+ )
]Committable suggestion skipped: line range outside the PR's diff.
| $event_material_contribution_link = sprintf( | ||
| '/events-contribution-verification/?source=%s&target_id=%s', | ||
| $action_target['title'], | ||
| $action_target['id'], | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating URL generation logic
The URL generation pattern is duplicated across multiple similar links (collection-camp, dropping-center, etc.). This violates the DRY principle.
Consider extracting the URL generation logic into a helper function:
+ function generate_contribution_link($type, $source, $target_id, $params = []) {
+ $base_url = sprintf('/%s-contribution/?source=%s&target_id=%s',
+ $type,
+ urlencode($source),
+ urlencode($target_id)
+ );
+ return add_query_args($base_url, $params);
+ }
- $event_material_contribution_link = sprintf(
- '/events-contribution-verification/?source=%s&target_id=%s',
- $action_target['title'],
- $action_target['id'],
- );
+ $event_material_contribution_link = generate_contribution_link(
+ 'events',
+ $action_target['title'],
+ $action_target['id']
+ );Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (4)
1249-1249: Reassess the clarity of this log message.
While loggingisset($_GET[$sourceFieldId])might help debugging, consider adding more descriptive text, so future maintainers better understand the context.
1253-1253: Avoid logging session key-values unless necessary.
Ensure thatcamp_sourceis not sensitive. Logging it can be helpful for debugging, but be sure it's safe to appear in logs.
1261-1261: Same caution: Consider logging policy.
“pu_source” is unlikely to be sensitive, but double-check any compliance requirements on logs.
1266-1272: Validate multiple GET parameter presence.
When more than one source parameter is passed, we always choose the first. Consider clarifying or handling conflicting sources more robustly if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(3 hunks)wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php
🔇 Additional comments (7)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (7)
1228-1228: Ensure sensitive or personal data is not logged.
Publicly logging user input might inadvertently expose sensitive information. Here it's likely benign, but carefully verify that only safe, non-sensitive data is being logged.
1256-1256: Session cleanup looks good.
Clearing other source keys maintains a single active source in the session. This chunk is conceptually correct.
1264-1265: Well-structured approach to ensuring a single source.
The code correctly unsets unneeded session keys, maintaining clarity of the active source.
1276-1276: Streamlined session clearing.
Unset all source-related session data if no parameter is found, preventing stale data from persisting.
1283-1283: Retrieving $eventSource from session looks fine.
This follows the same pattern as the other sources. Make sure it’s tested.
1295-1297: Extend existing logic to handle event source.
Good job integrating a new 'Events' source in the autoFill flow.
1302-1302: Initialize or unset all custom fields in a single block.
This code sets $autoFillData[$eventSourceFieldId] = NULL; to ensure a clean state; keep it consistent with other fields so no stale data remains.
| $eventSourceField = CustomField::get(FALSE) | ||
| ->addSelect('id') | ||
| ->addWhere('custom_group_id:name', '=', 'Contribution_Details') | ||
| ->addWhere('name', '=', 'Events') | ||
| ->execute()->single(); | ||
| $eventSourceFieldId = 'custom_' . $eventSourceField['id']; | ||
|
|
There was a problem hiding this comment.
Add null checks for $eventSourceField before using its properties.
If the 'Events' field doesn’t exist, $eventSourceField might be null, leading to a potential error when accessing $eventSourceField['id'].
+ if (!$eventSourceField) {
+ \Civi::log()->error("Unable to retrieve 'Events' custom field.");
+ return;
+ }
$eventSourceFieldId = 'custom_' . $eventSourceField['id'];Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (1)
78-79:⚠️ Potential issueUse correct variable instead of
$campId
The error message references$campId, which isn’t defined in this scope. This is similar to a previously flagged issue. Replace$campIdwith$eventIdor remove it if not required.- \Civi::log()->error("Error in sendLogisticsEmail for $campId " . $e->getMessage()); + \Civi::log()->error("Error in sendLogisticsEmail for $eventId " . $e->getMessage());Also applies to: 402-403
🧹 Nitpick comments (10)
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjInitiatedEventsFeedbackCron.php (1)
53-64: Add exception handling around email sending
Your code sends feedback emails and logs errors, which is good. Consider adding more targeted recovery steps if email sending fails (e.g., retries or a fallback).wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php (2)
91-98: Fix grammatical errors
The phrase “Their is one forms that require your attention” is grammatically incorrect. Use “There is one form that requires your attention” or “There are forms that require…” for clarity.- <p>Thank you for attending the goonj activity <strong>$eventId</strong>... Their is one forms that require ... + <p>Thank you for attending the goonj activity <strong>$eventId</strong>... There is one form that requires ...Also applies to: 417-421
361-361: Unused$feedbackEmailSentvariable
The variable$feedbackEmailSentis fetched but never used. If you intend to skip sending further emails based on this value, please add the necessary condition. Otherwise, remove it to reduce confusion.wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (1)
295-302: Consider clarifying method return type in the docblock.Currently, the method sometimes returns
FALSE, but the docblock doesn't indicate the possible return values. Updating it will help clarify what the callers might expect from this method.wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (4)
1229-1229: Remove debug logging statements.These debug logging statements should be removed before merging to production as they pollute the logs with unnecessary information.
- \Civi::log()->info('sourceFieldId', ['sourceFieldId'=>$sourceFieldId]); - \Civi::log()->info('isset($_GET[$sourceFieldId]',['isset($_GET[$sourceFieldId]'=>isset($_GET[$sourceFieldId])]); - \Civi::log()->info('camp_source',['source'=>$_SESSION['camp_source']]); - \Civi::log()->info('pu_source',['pu_source'=>$_SESSION['pu_source']]); - \Civi::log()->info('eventSource',['eventSource'=>$_SESSION['eventSource']]);Also applies to: 1250-1250, 1254-1254, 1262-1262, 1270-1270
1240-1246: Add null check for event source field.Similar to the existing pattern for other source fields, add a null check before using the event source field to prevent potential errors.
$eventSourceField = CustomField::get(FALSE) ->addSelect('id') ->addWhere('custom_group_id:name', '=', 'Contribution_Details') ->addWhere('name', '=', 'Events') ->execute()->single(); +if (!$eventSourceField) { + \Civi::log()->error("Unable to retrieve 'Events' custom field."); + return; +} $eventSourceFieldId = 'custom_' . $eventSourceField['id'];
1535-1535: Replace magic number with a constant.The payment instrument ID (4) should be defined as a class constant for better maintainability and readability.
+ const PAYMENT_INSTRUMENT_CHEQUE = 4; + public function validateCheckNumber($formName, &$fields, &$files, &$form, &$errors) { if ($formName == 'CRM_Contribute_Form_Contribution') { - if (isset($fields['payment_instrument_id']) && $fields['payment_instrument_id'] == 4) { + if (isset($fields['payment_instrument_id']) && $fields['payment_instrument_id'] == self::PAYMENT_INSTRUMENT_CHEQUE) {
1542-1558: Move JavaScript code to a separate file.The JavaScript code should be moved to a separate .js file and loaded using the proper asset management system. This improves maintainability and follows separation of concerns.
- Create a new file
js/check-number-validation.jswith the JavaScript code- Load it using the CiviCRM resource management system:
- \CRM_Core_Resources::singleton()->addScript(" - (function($) { - function ensureErrorVisible() { - var errorField = $('#check_number-error'); - var inputField = $('#check_number'); - $('.crm-error').hide(); - if (!errorField.length) { - inputField.after('<div id=\"check_number-error\" class=\"crm-error\">' + " . json_encode($message) . " + '</div>'); - } else { - errorField.show(); - } - } - - $(document).ajaxComplete(ensureErrorVisible); - $(document).ready(ensureErrorVisible); - })(CRM.$); - "); + \CRM_Core_Resources::singleton()->addScriptFile('goonjcustom', 'js/check-number-validation.js'); + \CRM_Core_Resources::singleton()->addVars('checkNumberValidation', [ + 'errorMessage' => $message + ]);wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (2)
47-49: DocBlock placeholders
These docblock lines do not provide new information. Consider adding relevant documentation or removing them to keep the code clean.
380-380: Simple null check
if ($contactId) { ... }is okay. Consider verifying the correct data type to avoid possible type juggling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
wp-content/civi-extensions/civirazorpay/CRM/Core/Civirazorpay/Payment/Razorpay.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(5 hunks)wp-content/civi-extensions/goonjcustom/Civi/GoonjInitiatedEventsService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionGoonjActivitiesService.php(1 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(5 hunks)wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php(4 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjInitiatedEventsCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjInitiatedEventsFeedbackCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UrbanPlannedVisitOutcomeCron.php(1 hunks)wp-content/civi-extensions/goonjcustom/templates/CRM/Contribute/Form/Contribution/ThankYou.tpl(0 hunks)
💤 Files with no reviewable changes (1)
- wp-content/civi-extensions/goonjcustom/templates/CRM/Contribute/Form/Contribution/ThankYou.tpl
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjInitiatedEventsCron.php
🔇 Additional comments (23)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionDroppingCenterService.php (1)
102-102: Ensure usage of logical operators for boolean conditions
It appears the change from & to && resolves the logical requirement of checking that both $contactId and $groupId are truthy, rather than performing a bitwise operation. This is an appropriate fix that prevents potential misinterpretations in condition evaluation.
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/UrbanPlannedVisitOutcomeCron.php (1)
46-47: Check the logic around Outcome_Email_Sent conditions
These updated conditions ensure that rows are pulled if Outcome_Email_Sent is NULL or 0. Confirm that there's no overlap in how these states are used in your workflow (e.g., a default of NULL vs. 0).
wp-content/civi-extensions/goonjcustom/api/v3/Goonjcustom/GoonjInitiatedEventsFeedbackCron.php (1)
36-48: Validate participant status logic
The logic uses ->addWhere('participant.status_id', '=', 2) and an OR clause for Last_Reminder_Sent. Ensure that these status code references align with your participant statuses. Also confirm you want to limit the result to 25 records (->setLimit(25)), as this could miss remaining events.
wp-content/civi-extensions/civirazorpay/CRM/Core/Civirazorpay/Payment/Razorpay.php (2)
299-302: Subscription Cancelled event recognized
Excellent addition to handle subscription.cancelled. Verify that any upstream or downstream processes reliant on subscription status remain consistent.
449-498: Confirm user experience on forced cancellations
The processSubscriptionCancelled method sets the contribution status to “Cancelled” and updates the cancel_date. Confirm that end users (or staff) receive appropriate messaging about canceled subscriptions.
wp-content/civi-extensions/goonjcustom/Civi/UrbanPlannedVisitService.php (3)
6-7: Imports look good.
These new imports from Civi\Api4\Group and Civi\Api4\GroupContact are appropriate for the added group assignment feature.
304-314: Validate the existence of index keys in $individualData and $visitData.
In these lines, the code directly references ['id'], ['fields'], and ['Urban_Planned_Visit.State']. If these keys are missing or not expected, a PHP notice or error might be thrown. Consider adding checks or default values to handle these scenarios gracefully.
330-353: Check for a completely missing fallback group.
If no group is found during fallback, $stateContactGroup remains null, and this block attempts to log a non-existent 'title'. This will cause an error. Consider guarding against a null return, e.g., by logging a clearer warning and skipping group assignment if no fallback group can be found.
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (13)
5-5: No issues with newly added import statement
The addition of use Civi\Afform\Event\AfformSubmitEvent; looks appropriate and consistent with the file's requirements.
6-6: Imports look consistent
The new imports from Civi\Api4\* modules appear valid. Ensure that everything added is used within this file to keep the imports minimal and maintain clarity.
13-13: CoreUtil import is valid
Including use Civi\Api4\Utils\CoreUtil; is helpful for static method calls used later in the file.
24-24: Constant naming suggestion
Institution_INTENT_FB_NAME is readable and sufficiently descriptive as a constant. This change is acceptable.
26-26: Static field for address
Defining $instituteAddress as a static property is fine. Just ensure you reset it whenever needed if multiple submissions happen in a single request cycle.
33-36: Event subscriptions added with priority
Defining priorities [9, 8] is correct if the order is intentional. No further issues.
41-43: Renaming the hook pre function
Replacing 'assignChapterGroupToIndividual' with 'assignChapterGroupToContacts' clarifies that it can handle multiple contact types. Good approach.
50-82: Validating address capture logic
Overall, setInstituteAddress retrieves and stores state_province_id properly. However, be aware of edge cases (e.g., no address joined).
119-145: New approach for group assignment
assignChapterGroupToContacts unifies the logic for adding both Individual and Organization to the chapter group. This is a good improvement in design.
285-287: Chaining builder for Goonj_Office
The builder lines for updating Review.Goonj_Office are consistent. No additional concerns here.
322-322: Proper logical check
if ($coordinatorId && self::$organizationId) { is correct. Good usage of logical AND to ensure both IDs exist.
324-326: Chaining builder for Coordinating_POC
Similar to the Goonj_Office lines, these lines update a related field in an organized manner. No issues found.
382-384: Querying Type_of_Institution
Fetching Institute_Registration.Type_of_Institution:label is correct. Make sure you verify the expected structure if the label is missing.
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)
146-146: Assign chapter group condition
if ($objectName !== 'Eck_Collection_Camp' || !self::isCurrentSubtype($objectRef)) { return; } properly short-circuits. Ensure you handle all subtypes as needed or log the skip reason.
171-171: Logical AND check
if($contactId && $groupId){ ... } is correct to avoid attempts at creating group contact with invalid IDs. Good usage of try-catch for error handling.
| */ | ||
| private static function addContactToGroup($contactId, $groupId) { | ||
| if($contactId & $groupID){ | ||
| if($contactId & $groupId){ |
There was a problem hiding this comment.
Use logical AND (&&) instead of bitwise operator (&).
Here, the bitwise & operator can cause unexpected behavior if either value exceeds 1 in binary representation. The proper operator for checking the truthiness of both $contactId and $groupId is &&.
Apply this diff to fix the conditional check:
- if($contactId & $groupId){
+ if ($contactId && $groupId) {📝 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.
| if($contactId & $groupId){ | |
| if ($contactId && $groupId) { |
| /** | ||
| * | ||
| */ | ||
| private static function getLogisticsEmailHtml($attendeeName, $eventId, $eventAttendedById, $eventCode, $eventAddress) { | ||
| $homeUrl = \CRM_Utils_System::baseCMSURL(); | ||
| // Construct the full URLs for the forms. | ||
| $campOutcomeFormUrl = $homeUrl . 'goonj-initiated-events-outcome/#?Event1=' . $eventId . '&Goonj_Events_Outcome.Filled_By=' . $campAttendedById; | ||
|
|
||
| $html = " | ||
| <p>Dear $attendeeName,</p> | ||
| <p>Thank you for attending the goonj activity <strong>$eventId</strong> at <strong>$eventAddress</strong>. Their is one forms that require your attention during and after the goonj activity:</p> | ||
| <ol> | ||
| Please complete this form from the goonj activity location once the goonj activity ends.</li> | ||
| <li><a href=\"$campOutcomeFormUrl\">Goonj Activity Outcome Form</a><br> | ||
| This feedback form should be filled out after the goonj activity/session ends, once you have an overview of the event's outcomes.</li> | ||
| </ol> | ||
| <p>We appreciate your cooperation.</p> | ||
| <p>Warm Regards,<br>Urban Relations Team</p>"; | ||
|
|
||
| return $html; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded text could be externalized or templated
Your HTML strings are embedded directly in the method, making them harder to maintain. Consider external templates or translation functions for cleaner code and to support multi-language expansions.
Also applies to: 410-427
| if ($groupId & $contactId) { | ||
| GroupContact::create(FALSE) | ||
| ->addValue('contact_id', $contactId) | ||
| ->addValue('group_id', $groupId) | ||
| ->addValue('status', 'Added') | ||
| ->execute(); | ||
| } |
There was a problem hiding this comment.
Replace bitwise AND with a logical AND.
if ($groupId & $contactId) {
// ...
}This uses the bitwise AND operator (&) instead of the logical AND (&&). For boolean checks, you should use &&. Using & can lead to unintended behavior if $groupId or $contactId are numeric or not strictly boolean.
-if ($groupId & $contactId) {
+if ($groupId && $contactId) {
GroupContact::create(FALSE)
->addValue('contact_id', $contactId)
->addValue('group_id', $groupId)
->addValue('status', 'Added')
->execute();
}| $stateOfficeId = $stateOffice['id']; | ||
|
|
||
| if($stateOfficeId & self::$organizationId){ | ||
| if ($stateOfficeId & self::$organizationId) { |
There was a problem hiding this comment.
Replace bitwise operator with logical operator
Line 283 uses bitwise & instead of logical &&. This may lead to unexpected results.
-if ($stateOfficeId & self::$organizationId) {
+if ($stateOfficeId && self::$organizationId) {📝 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.
| if ($stateOfficeId & self::$organizationId) { | |
| if ($stateOfficeId && self::$organizationId) { |
| /** | ||
| * | ||
| */ | ||
| public static function setInstitutionPocAddress(AfformSubmitEvent $event) { | ||
| $afform = $event->getAfform(); | ||
| $formName = $afform['name']; | ||
|
|
||
| if ($formName !== self::Institution_INTENT_FB_NAME) { | ||
| return; | ||
| } | ||
|
|
||
| $entityType = $event->getEntityType(); | ||
|
|
||
| if (!CoreUtil::isContact($entityType)) { | ||
| return; | ||
| } | ||
|
|
||
| foreach ($event->records as $index => $contact) { | ||
| if (empty($contact['fields'])) { | ||
| continue; | ||
| } | ||
|
|
||
| $contactId = $contact['fields']['id']; | ||
|
|
||
| $stateProvinceId = self::$instituteAddress['state_province_id']; | ||
|
|
||
| $updateResults = Address::update(FALSE) | ||
| ->addValue('state_province_id', $stateProvinceId) | ||
| ->addWhere('contact_id', '=', $test) | ||
| ->execute(); | ||
| } | ||
|
|
||
| \Civi::log()->info('Address update results', $updateResults); | ||
|
|
||
| } |
There was a problem hiding this comment.
Potential bug with undeclared $test variable
In setInstitutionPocAddress(), line 111 uses ->addWhere('contact_id', '=', $test), but $test is never defined. This appears to be an accidental leftover that will cause runtime errors.
Suggested fix:
- ->addWhere('contact_id', '=', $test)
+ ->addWhere('contact_id', '=', $contactId)📝 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 setInstitutionPocAddress(AfformSubmitEvent $event) { | |
| $afform = $event->getAfform(); | |
| $formName = $afform['name']; | |
| if ($formName !== self::Institution_INTENT_FB_NAME) { | |
| return; | |
| } | |
| $entityType = $event->getEntityType(); | |
| if (!CoreUtil::isContact($entityType)) { | |
| return; | |
| } | |
| foreach ($event->records as $index => $contact) { | |
| if (empty($contact['fields'])) { | |
| continue; | |
| } | |
| $contactId = $contact['fields']['id']; | |
| $stateProvinceId = self::$instituteAddress['state_province_id']; | |
| $updateResults = Address::update(FALSE) | |
| ->addValue('state_province_id', $stateProvinceId) | |
| ->addWhere('contact_id', '=', $test) | |
| ->execute(); | |
| } | |
| \Civi::log()->info('Address update results', $updateResults); | |
| } | |
| /** | |
| * | |
| */ | |
| public static function setInstitutionPocAddress(AfformSubmitEvent $event) { | |
| $afform = $event->getAfform(); | |
| $formName = $afform['name']; | |
| if ($formName !== self::Institution_INTENT_FB_NAME) { | |
| return; | |
| } | |
| $entityType = $event->getEntityType(); | |
| if (!CoreUtil::isContact($entityType)) { | |
| return; | |
| } | |
| foreach ($event->records as $index => $contact) { | |
| if (empty($contact['fields'])) { | |
| continue; | |
| } | |
| $contactId = $contact['fields']['id']; | |
| $stateProvinceId = self::$instituteAddress['state_province_id']; | |
| $updateResults = Address::update(FALSE) | |
| ->addValue('state_province_id', $stateProvinceId) | |
| ->addWhere('contact_id', '=', $contactId) | |
| ->execute(); | |
| } | |
| \Civi::log()->info('Address update results', $updateResults); | |
| } |
4 participants
Summary by CodeRabbit
New Features
Bug Fixes
Chores