Initial Draft: Added Institution Collection Camp Contribution#490
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Outside diff range and nitpick comments (6)
wp-content/plugins/goonj-blocks/goonj-blocks.php (2)
34-34: Consider grouping related actions for better maintainabilityThe
$actionsarray is mixing different types of entities (camps, centers, schedules). Consider grouping related actions into categorized arrays for better maintainability.-$actions = array('collection-camp', 'dropping-center', 'processing-center', 'induction-schedule', 'institution-collection-camp'); +$camp_actions = array('collection-camp', 'institution-collection-camp'); +$center_actions = array('dropping-center', 'processing-center'); +$schedule_actions = array('induction-schedule'); +$actions = array_merge($camp_actions, $center_actions, $schedule_actions);
Line range hint
108-120: Refactor duplicate API call logic and enhance error handlingThe switch case for 'institution-collection-camp' shares identical logic with 'collection-camp' and 'dropping-center', leading to code duplication. Additionally, there's no validation for required fields in the API response.
Consider extracting the common API call logic:
function get_collection_entity($id, $entity_fields) { $result = \Civi\Api4\EckEntity::get('Collection_Camp', false) ->selectRowCount() ->addSelect(...$entity_fields) ->addWhere('id', '=', $id) ->setLimit(1) ->execute(); if ($result->count() === 0) { return null; } $entity = $result->first(); // Validate required fields $required_fields = ['title', 'Collections_will_start_on_Date']; foreach ($required_fields as $field) { if (empty($entity[$field])) { throw new Exception("Missing required field: $field"); } } return $entity; }Then simplify the switch case:
case 'collection-camp': case 'institution-collection-camp': case 'dropping-center': - $result = \Civi\Api4\EckEntity::get('Collection_Camp', false) - ->selectRowCount() - ->addSelect(...$entity_fields) - ->addWhere('id', '=', $id) - ->setLimit(1) - ->execute(); - - if ($result->count() === 0) { + try { + $entity = get_collection_entity($id, $entity_fields); + if (!$entity) { + $is_404 = true; + } else { + $wp_query->set('action_target', $entity); + } + } catch (Exception $e) { $is_404 = true; - } else { - $wp_query->set('action_target', $result->first()); + error_log("Error processing collection entity: " . $e->getMessage()); } break;wp-content/plugins/goonj-blocks/build/render.php (2)
20-20: Consider using a more distinctive heading for institution collection campThe heading "Collection Camp" is used for both 'collection-camp' and 'institution-collection-camp', which could cause confusion for users. Consider using a more specific heading like "Institution Collection Camp" to clearly differentiate between the two types.
- 'institution-collection-camp' => 'Collection Camp', + 'institution-collection-camp' => 'Institution Collection Camp',
130-130: Simplify target type handling with configuration-driven approachThe code is becoming harder to maintain with multiple if conditions for different target types. Consider using a configuration-driven approach to define target type behaviors.
+$target_type_config = [ + 'collection-camp' => [ + 'show_dates' => true, + 'show_volunteer' => false, + ], + 'institution-collection-camp' => [ + 'show_dates' => true, + 'show_volunteer' => false, + ], + 'dropping-center' => [ + 'show_dates' => false, + 'show_volunteer' => true, + ] +]; -if (in_array($target, ['collection-camp','institution-collection-camp', 'dropping-center'])) : +if (isset($target_type_config[$target])) : $target_info = $target_data[$target]; // ... existing code ... - <?php if ($target === 'collection-camp' || $target === 'institution-collection-camp') : ?> + <?php if ($target_type_config[$target]['show_dates']) : ?>Also applies to: 162-174
wp-content/plugins/goonj-blocks/src/render.php (2)
20-20: Consider differentiating the display text for institution collection campsThe heading text 'Collection Camp' is used for both regular and institution collection camps, which might cause confusion for users. Consider using a more descriptive heading like 'Institution Collection Camp' to clearly distinguish between the two types.
- 'institution-collection-camp' => 'Collection Camp', + 'institution-collection-camp' => 'Institution Collection Camp',
120-127: Consider refactoring duplicate camp configurationsThe configuration for
institution-collection-camplargely mirrorscollection-campbut with different field names. Consider extracting common structure into a shared configuration to reduce duplication and make maintenance easier.+ $camp_base_config = [ + 'address_label' => 'Address of the camp', + 'donation_link' => $donation_link, + ]; + $target_data = [ 'collection-camp' => array_merge($camp_base_config, [ 'start_time' => 'Collection_Camp_Intent_Details.Start_Date', 'end_time' => 'Collection_Camp_Intent_Details.End_Date', 'address' => 'Collection_Camp_Intent_Details.Location_Area_of_camp', 'contribution_link' => $material_contribution_link, ]), 'institution-collection-camp' => array_merge($camp_base_config, [ 'start_time' => 'Institution_Collection_Camp_Intent.Collections_will_start_on_Date', 'end_time' => 'Institution_Collection_Camp_Intent.Collections_will_end_on_Date_', 'address' => 'Institution_Collection_Camp_Intent.Collection_Camp_Address', 'contribution_link' => $institution_collection_camp_material_contribution_link, ]),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
wp-content/plugins/goonj-blocks/build/render.php(4 hunks)wp-content/plugins/goonj-blocks/goonj-blocks.php(3 hunks)wp-content/plugins/goonj-blocks/src/render.php(4 hunks)
🔇 Additional comments (2)
wp-content/plugins/goonj-blocks/goonj-blocks.php (1)
Line range hint 34-120: Verify impact on existing routes and API endpoints
The addition of the new institution-collection-camp functionality modifies routing and API interactions. Let's verify that existing routes are not affected.
✅ Verification successful
No conflicts found in route handling for institution-collection-camp
The changes to routing and API interactions for institution-collection-camp are well-integrated with the existing codebase:
- The rewrite rules are properly scoped within the existing actions array and follow the same pattern as other action types
- Query vars are cleanly registered through the WordPress filter system
- The template redirect handler properly checks for page existence and required parameters
- The API endpoint usage follows the same pattern as other collection camp types with proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential conflicts in route handling
echo "Checking for route handling patterns..."
rg -A 2 "add_rewrite_rule|template_redirect"
# Check for other uses of Collection_Camp entity
echo "Checking Collection_Camp entity usage..."
rg "Collection_Camp" --type php
# Check for potential API endpoint conflicts
echo "Checking API endpoint patterns..."
rg -A 3 "Civi\\Api4\\EckEntity::get|'Collection_Camp'" --type php
Length of output: 55845
Script:
#!/bin/bash
# Check for potential conflicts in route handling with more context
echo "Checking for route handling patterns in the plugin..."
rg -A 5 "add_rewrite_rule|template_redirect" wp-content/plugins/goonj-blocks/
# Check for other route handlers in the plugin
echo "Checking for other route handlers..."
rg -A 5 "add_action.*template_redirect|add_filter.*query_vars" wp-content/plugins/goonj-blocks/
# Check for potential API endpoint conflicts with proper escaping
echo "Checking API endpoint patterns..."
rg -A 3 "Civi\\\\Api4\\\\EckEntity::get.*Collection_Camp" --type php
Length of output: 3977
wp-content/plugins/goonj-blocks/src/render.php (1)
130-130: LGTM! Consistent handling of institution collection camps
The conditional logic correctly includes the new camp type and reuses the existing date/time display logic, maintaining consistency across different camp types.
Also applies to: 162-162
| 'Institution_Collection_Camp_Intent.Collections_will_start_on_Date', | ||
| 'Institution_Collection_Camp_Intent.Collections_will_end_on_Date_', | ||
| 'Institution_Collection_Camp_Intent.Collection_Camp_Address' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document and organize entity fields for better clarity
The new Institution Collection Camp fields are added without documentation or logical grouping. This makes it harder to understand their purpose and relationships.
+// Institution Collection Camp date fields
'Institution_Collection_Camp_Intent.Collections_will_start_on_Date',
'Institution_Collection_Camp_Intent.Collections_will_end_on_Date_',
+// Institution Collection Camp location fields
'Institution_Collection_Camp_Intent.Collection_Camp_Address'Additionally, consider organizing fields into associative arrays by entity type for better maintainability:
$entity_fields = [
'common' => ['id', 'title'],
'collection_camp' => [
'Collection_Camp_Intent_Details.Start_Date',
// ... other collection camp fields
],
'institution_collection_camp' => [
'Institution_Collection_Camp_Intent.Collections_will_start_on_Date',
// ... other institution fields
],
// ... other entities
];| 'institution-collection-camp' => [ | ||
| 'start_time' => 'Institution_Collection_Camp_Intent.Collections_will_start_on_Date', | ||
| 'end_time' => 'Institution_Collection_Camp_Intent.Collections_will_end_on_Date_', | ||
| 'address' => 'Institution_Collection_Camp_Intent.Collection_Camp_Address', | ||
| 'address_label' => 'Address of the camp', | ||
| 'contribution_link' => $institution_collection_camp_material_contribution_link, | ||
| 'donation_link' => $donation_link, | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required fields in target data
The target data structure assumes all fields will be present in $action_target. Consider adding validation to handle cases where required fields might be missing.
+function validate_target_fields($action_target, $required_fields) {
+ foreach ($required_fields as $field) {
+ if (empty($action_target[$field])) {
+ throw new InvalidArgumentException("Required field missing: $field");
+ }
+ }
+}
'institution-collection-camp' => [
+ 'required_fields' => [
+ 'Institution_Collection_Camp_Intent.Collections_will_start_on_Date',
+ 'Institution_Collection_Camp_Intent.Collections_will_end_on_Date_',
+ 'Institution_Collection_Camp_Intent.Collection_Camp_Address',
+ ],
'start_time' => 'Institution_Collection_Camp_Intent.Collections_will_start_on_Date',Committable suggestion skipped: line range outside the PR's diff.
| $institution_collection_camp_material_contribution_link = sprintf( | ||
| '/institution-collection-camp-contribution?source=%s&target_id=%s&state_province_id=%s&city=%s', | ||
| $action_target['title'], | ||
| $action_target['id'], | ||
| $action_target['Institution_Collection_Camp_Intent.State'], | ||
| $action_target['Institution_Collection_Camp_Intent.District_City'], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor link generation to reduce code duplication
The link generation logic is duplicated across multiple similar blocks (material_contribution_link, dropping_center_material_contribution_link, institution_collection_camp_material_contribution_link). Consider extracting this into a helper function.
+function generate_contribution_link($type, $action_target) {
+ $link_templates = [
+ 'collection-camp' => '/collection-camp-contribution',
+ 'institution-collection-camp' => '/institution-collection-camp-contribution',
+ 'dropping-center' => '/dropping-center-contribution'
+ ];
+
+ $field_mappings = [
+ 'collection-camp' => [
+ 'state' => 'Collection_Camp_Intent_Details.State',
+ 'city' => 'Collection_Camp_Intent_Details.City'
+ ],
+ 'institution-collection-camp' => [
+ 'state' => 'Institution_Collection_Camp_Intent.State',
+ 'city' => 'Institution_Collection_Camp_Intent.District_City'
+ ],
+ 'dropping-center' => [
+ 'state' => 'Dropping_Centre.State',
+ 'city' => 'Dropping_Centre.District_City'
+ ]
+ ];
+
+ return sprintf(
+ '%s?source=%s&target_id=%s&state_province_id=%s&city=%s',
+ $link_templates[$type],
+ $action_target['title'],
+ $action_target['id'],
+ $action_target[$field_mappings[$type]['state']],
+ $action_target[$field_mappings[$type]['city']]
+ );
+}Committable suggestion skipped: line range outside the PR's diff.
| $institution_collection_camp_material_contribution_link = sprintf( | ||
| '/institution-collection-camp-contribution?source=%s&target_id=%s&state_province_id=%s&city=%s', | ||
| $action_target['title'], | ||
| $action_target['id'], | ||
| $action_target['Institution_Collection_Camp_Intent.State'], | ||
| $action_target['Institution_Collection_Camp_Intent.District_City'], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add field validation for institution collection camp parameters
The link generation assumes all required fields exist in $action_target. Consider adding validation to handle cases where required fields might be missing.
$institution_collection_camp_material_contribution_link = sprintf(
+ // Validate required fields
+ !empty($action_target['Institution_Collection_Camp_Intent.State']) &&
+ !empty($action_target['Institution_Collection_Camp_Intent.District_City']) ?
'/institution-collection-camp-contribution?source=%s&target_id=%s&state_province_id=%s&city=%s',
$action_target['title'],
$action_target['id'],
$action_target['Institution_Collection_Camp_Intent.State'],
$action_target['Institution_Collection_Camp_Intent.District_City'],
+ : '#'
);Committable suggestion skipped: line range outside the PR's diff.
…at/institution-collection-camp-contribution
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
wp-content/plugins/goonj-blocks/build/render.php (2)
20-20: Consider using a more distinctive heading for institution collection campHaving both 'collection-camp' and 'institution-collection-camp' map to the same heading 'Collection Camp' might confuse users. Consider a more specific heading like 'Institution Collection Camp' to differentiate between the two types.
- 'institution-collection-camp' => 'Collection Camp', + 'institution-collection-camp' => 'Institution Collection Camp',
130-130: Extract repeated condition into a constantThe condition
in_array($target, ['collection-camp','institution-collection-camp', 'dropping-center'])is repeated. This could be extracted into a constant to improve maintainability and make the shared behavior more explicit.+const CAMP_TYPES = ['collection-camp', 'institution-collection-camp', 'dropping-center']; -if (in_array($target, ['collection-camp','institution-collection-camp', 'dropping-center'])) : +if (in_array($target, CAMP_TYPES)) : -<?php if (in_array($target, ['collection-camp', 'institution-collection-camp'])) : ?> +<?php if (in_array($target, array_slice(CAMP_TYPES, 0, 2))) : ?>Also applies to: 162-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/plugins/goonj-blocks/build/render.php(4 hunks)
🔇 Additional comments (2)
wp-content/plugins/goonj-blocks/build/render.php (2)
42-48: Code duplication in link generation persists
This is the third instance of nearly identical link generation code, following the same pattern as material_contribution_link and dropping_center_material_contribution_link. A previous review already suggested extracting this into a helper function.
120-127: 🛠️ Refactor suggestion
Standardize field names and add validation
-
Field names differ from other camp types:
- 'Collections_will_start_on_Date' vs 'Start_Date'
- 'Collections_will_end_on_Date_' vs 'End_Date'
- 'Collection_Camp_Address' vs 'Location_Area_of_camp'
-
As noted in a previous review, field validation is still missing.
Consider standardizing field names across all camp types and implementing the previously suggested validation:
$required_fields = [
'start_time',
'end_time',
'address',
];
foreach ($required_fields as $field) {
if (empty($target_info[$field]) || empty($action_target[$target_info[$field]])) {
throw new InvalidArgumentException("Required field missing: $field");
}
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
wp-content/plugins/goonj-blocks/build/render.php (3)
20-20: Consider distinguishing institution collection camp headingThe heading 'Collection Camp' is used for both 'collection-camp' and 'institution-collection-camp', which might cause confusion. Consider using a more specific heading for institution collection camps to differentiate them.
- 'institution-collection-camp' => 'Collection Camp', + 'institution-collection-camp' => 'Institution Collection Camp',
120-127: Standardize field naming across target typesThe field names for dates are inconsistent between target types:
- collection-camp:
Start_DateandEnd_Date- institution-collection-camp:
Collections_will_start_on_DateandCollections_will_end_on_Date_This inconsistency makes the code harder to maintain and understand.
Consider standardizing the field names in the CiviCRM custom fields or creating mapping constants:
const DATE_FIELD_MAPPINGS = [ 'collection-camp' => [ 'start' => 'Start_Date', 'end' => 'End_Date' ], 'institution-collection-camp' => [ 'start' => 'Collections_will_start_on_Date', 'end' => 'Collections_will_end_on_Date_' ] ];
130-130: Consider using constants for target typesTarget types are hardcoded in multiple places. This could lead to maintenance issues and typos.
Consider defining constants for target types:
const TARGET_TYPES = [ 'COLLECTION_CAMP' => 'collection-camp', 'INSTITUTION_COLLECTION_CAMP' => 'institution-collection-camp', 'DROPPING_CENTER' => 'dropping-center' ];Then use them in conditions:
-if (in_array($target, ['collection-camp','institution-collection-camp', 'dropping-center'])) : +if (in_array($target, [TARGET_TYPES['COLLECTION_CAMP'], TARGET_TYPES['INSTITUTION_COLLECTION_CAMP'], TARGET_TYPES['DROPPING_CENTER']])) :Also applies to: 162-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/plugins/goonj-blocks/build/render.php(4 hunks)
🔇 Additional comments (1)
wp-content/plugins/goonj-blocks/build/render.php (1)
42-48: Previous refactoring suggestion still applies
A previous review already suggested refactoring the link generation logic to reduce code duplication. This new addition further reinforces that need.
| $headings = [ | ||
| 'collection-camp' => 'Collection Camp', | ||
| 'dropping-center' => 'Dropping Center', | ||
| 'institution-collection-camp' => 'Collection Camp', |
There was a problem hiding this comment.
Is the heading name Collection Camp is correct for the institution Collection Camp. Please verify and let me know
There was a problem hiding this comment.
For now, let's proceed with using "collection camp," but I'll confirm with the Goonj team on Monday. I'll make a note of it, and we can change it later if needed.
There was a problem hiding this comment.
Sure please do. Let's merge this for now
| 'Institution_Collection_Camp_Intent.Collection_Camp_Address' | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Why these extra spaces here?
belwalshubham
deleted the
feat/institution-collection-camp-contribution
branch
2 participants
Summary by CodeRabbit
New Features
Bug Fixes