Refactor date and address handling logic for collection camps and dropping centers #364
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
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: 6
🧹 Outside diff range and nitpick comments (3)
wp-content/plugins/goonj-blocks/src/render.php (1)
53-66: Good job on improving code organization, but consider using constants for keys.The new
$target_dataarray is a great improvement in terms of code organization and maintainability. It allows for easy addition of new target types and centralizes the configuration.To further improve maintainability, consider using constants for the keys:
define('START_TIME', 'start_time'); define('END_TIME', 'end_time'); define('ADDRESS', 'address'); define('CONTRIBUTION_LINK', 'contribution_link'); $target_data = [ 'dropping-center' => [ START_TIME => 'Dropping_Centre.Start_Time', END_TIME => 'Dropping_Centre.End_Time', ADDRESS => 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_', CONTRIBUTION_LINK => $dropping_center_material_contribution_link, ], 'collection-camp' => [ 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, ], ];This approach would make the code more resistant to typos and easier to refactor in the future.
wp-content/plugins/goonj-blocks/goonj-blocks.php (1)
Line range hint
88-112: Separate logic for 'collection-camp' and 'dropping-center' targetsCurrently, both 'collection-camp' and 'dropping-center' targets are handled together, querying the
Collection_Campentity regardless of the target type. If these targets represent different entities or require different data fields, consider separating their logic into distinct cases within the switch statement. This separation adheres to the Single Responsibility Principle and improves code clarity and maintainability.wp-content/plugins/goonj-blocks/build/render.php (1)
53-66: Define field names as constants to improve maintainabilityThe field names used in the
$target_dataarray are hardcoded strings with lengthy and complex keys, such as'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_'. Hardcoding these strings can lead to typos and makes the code harder to maintain. Consider defining these field names as constants or storing them in a configuration file to enhance readability and reduce the risk of errors.For example:
// Define constants for the field names define('DROPPING_CENTRE_ADDRESS_FIELD', 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_'); define('COLLECTION_CAMP_ADDRESS_FIELD', 'Collection_Camp_Intent_Details.Location_Area_of_camp'); // Use the constants in the $target_data array $target_data = [ 'dropping-center' => [ 'start_time' => 'Dropping_Centre.Start_Time', 'end_time' => 'Dropping_Centre.End_Time', 'address' => DROPPING_CENTRE_ADDRESS_FIELD, 'contribution_link' => $dropping_center_material_contribution_link, ], 'collection-camp' => [ 'start_time' => 'Collection_Camp_Intent_Details.Start_Date', 'end_time' => 'Collection_Camp_Intent_Details.End_Date', 'address' => COLLECTION_CAMP_ADDRESS_FIELD, 'contribution_link' => $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 (3 hunks)
- wp-content/plugins/goonj-blocks/goonj-blocks.php (1 hunks)
- wp-content/plugins/goonj-blocks/src/render.php (3 hunks)
🧰 Additional context used
| $target_info = $target_data[$target]; | ||
|
|
||
| $start_date = new DateTime($action_target[$target_info['start_time']]); | ||
| $end_date = new DateTime($action_target[$target_info['end_time']]); | ||
| $address = $action_target[$target_info['address']]; | ||
| $contribution_link = $target_info['contribution_link']; |
There was a problem hiding this comment.
Good refactoring, but consider adding error handling.
The use of $target_data to retrieve information based on the target type is a good improvement. It makes the code more flexible and easier to maintain.
However, there's no error handling for cases where $target might not be a valid key in $target_data. Consider adding a check:
if (isset($target_data[$target])) {
$target_info = $target_data[$target];
$start_date = new DateTime($action_target[$target_info['start_time']]);
$end_date = new DateTime($action_target[$target_info['end_time']]);
$address = $action_target[$target_info['address']];
$contribution_link = $target_info['contribution_link'];
} else {
// Handle error case, perhaps log an error or set default values
error_log("Invalid target type: $target");
// Set default values or skip rendering
}This would make the code more robust and easier to debug if issues arise with unexpected target types.
| <a href="<?php echo esc_url( $contribution_link ); ?>" class="wp-block-gb-action-button"> | ||
| <?php esc_html_e( 'Record your Material Contribution', 'goonj-blocks' ); ?> | ||
| </a> |
There was a problem hiding this comment.
Good use of dynamic link, but consider adding a fallback.
The use of $contribution_link derived from $target_info is a good improvement, allowing flexibility for different target types.
However, to improve robustness, consider adding a fallback in case $contribution_link is not set:
<a href="<?php echo esc_url( $contribution_link ?? '#' ); ?>" class="wp-block-gb-action-button">
<?php esc_html_e( 'Record your Material Contribution', 'goonj-blocks' ); ?>
</a>This ensures that even if $contribution_link is not set for some reason, the button will still render without causing a PHP notice, improving the overall reliability of the code.
| 'Dropping_Centre.Start_Time', | ||
| 'Dropping_Centre.End_Time', | ||
| 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_', | ||
| 'Dropping_Centre.State', | ||
| 'Dropping_Centre.District_City', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor field handling for maintainability
Adding Dropping Centre fields to the $entity_fields array alongside Collection Camp fields can lead to confusion and reduce maintainability as the number of fields grows. Consider separating the fields for each target into their own arrays or configurations. This approach will enhance code readability and scalability, making it easier to manage fields specific to each target.
| $start_date = new DateTime($action_target[$target_info['start_time']]); | ||
| $end_date = new DateTime($action_target[$target_info['end_time']]); |
There was a problem hiding this comment.
Add error handling for DateTime instantiation
When creating new DateTime instances, invalid date formats can cause exceptions. To enhance robustness, consider adding error handling to manage potential exceptions gracefully.
Apply this change to handle exceptions:
try {
$start_date = new DateTime($action_target[$target_info['start_time']]);
$end_date = new DateTime($action_target[$target_info['end_time']]);
} catch (Exception $e) {
// Handle the error, e.g., log the exception and set default dates
error_log($e->getMessage());
$start_date = $end_date = null;
}This ensures that your application can handle unexpected data without crashing.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
wp-content/plugins/goonj-blocks/build/render.php (1)
51-64: Excellent refactoring, consider further improvementThe introduction of the
$target_dataarray is a great step towards improving code organization and reducing redundancy. It adheres to the Single Responsibility Principle by centralizing the configuration for different targets.To further improve this structure, consider defining this array as a constant or in a separate configuration file. This would make it easier to maintain and potentially reuse across different parts of the plugin. For example:
// In a separate config file, e.g., target_config.php define('TARGET_DATA', [ 'dropping-center' => [ 'start_time' => 'Dropping_Centre.Start_Time', 'end_time' => 'Dropping_Centre.End_Time', 'address' => 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_', 'contribution_link' => 'dropping_center_material_contribution_link', ], 'collection-camp' => [ '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', ], ]); // In your main file $target_data = TARGET_DATA;This approach would make the configuration more maintainable and potentially reusable across different parts of the plugin.
wp-content/plugins/goonj-blocks/src/render.php (1)
51-64: Bravo on the refactoring! But let's add a safety net, shall we?Well, well, well! Look at this beautiful
$target_dataarray! It's more organized than Marie Kondo's sock drawer. You've done a fantastic job of structuring the data for different target types. It's like you've given each piece of data its own cozy little home.But wait! What if someone tries to access the penthouse when we only have apartments? In other words, what if
$targetis something unexpected? Let's add a bouncer to this code party:if (isset($target_data[$target])) { $target_info = $target_data[$target]; // ... rest of your code ... } else { // Handle error case, perhaps log an error or set default values error_log("Oops! Someone tried to crash our party with an invalid target type: $target"); // Set default values or skip rendering }This way, if someone tries to sneak in with an invalid target type, we'll catch them red-handed! It's like adding a try-catch block, but with more pizzazz. Your code will be more robust than a fortress and easier to debug than a "Hello, World!" program!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/plugins/goonj-blocks/build/render.php (3 hunks)
- wp-content/plugins/goonj-blocks/src/render.php (3 hunks)
🧰 Additional context used
🔇 Additional comments (4)
wp-content/plugins/goonj-blocks/build/render.php (1)
102-102: Correct implementation of dynamic contribution linkThe update to use
$contribution_linkin the href attribute is correct and consistent with the earlier refactoring. This ensures that the appropriate link is used based on the target type.wp-content/plugins/goonj-blocks/src/render.php (3)
67-72: Smooth moves with the refactoring! But let's add some error-handling dance steps.Hot diggity dog! You've turned this code into a smooth operator with that
$target_dataarray. It's like watching a well-choreographed dance routine. Each variable gets its value with the grace of a ballet dancer.But hold your horses! We're missing a crucial step in our dance routine - the error handling tango! Let's add some fancy footwork to catch any missteps:
if (isset($target_data[$target])) { $target_info = $target_data[$target]; $start_date = new DateTime($action_target[$target_info['start_time']]); $end_date = new DateTime($action_target[$target_info['end_time']]); $address = $action_target[$target_info['address']]; $contribution_link = $target_info['contribution_link']; } else { // Time to improvise! error_log("Whoopsie daisy! We've got an invalid target type: $target"); // Set default values or skip rendering // Maybe do the Macarena as a fallback dance move? }With this addition, your code will be smoother than a freshly waxed dance floor and more reliable than a metronome! It'll handle errors with the grace of a tap dancer avoiding a banana peel. Now that's what I call coding with rhythm!
102-104: Dynamic link? Check! Fallback? Let's add that safety net!Well, butter my biscuit! You've got that
$contribution_linkworking harder than a one-armed wallpaper hanger. It's as dynamic as a chameleon in a bag of Skittles! But let's not forget Murphy's Law - if something can go wrong, it will, usually right after you deploy on a Friday evening.How about we add a little insurance policy to this code? A fallback that's got your back like a trusty sidekick:
<a href="<?php echo esc_url( $contribution_link ?? '#' ); ?>" class="wp-block-gb-action-button"> <?php esc_html_e( 'Record your Material Contribution', 'goonj-blocks' ); ?> </a>This little
?? '#'is like a safety net for a trapeze artist. If$contribution_linkdecides to pull a Houdini and disappear, we'll still have a valid link. It's smoother than a fresh jar of Skippy!With this addition, your code will be more reliable than a Swiss watch and more prepared than a Boy Scout. It'll handle missing links better than a chainmail bikini! Now that's what I call coding with both style AND substance!
33-39: 🛠️ Refactor suggestionDuplicate code alert! Time to refactor this URL construction.
Hey there, code artisan! I see you've introduced a new
$dropping_center_material_contribution_link, but it's giving me déjà vu. It's like a twin of$material_contribution_link, just with a different haircut. Let's give this code a makeover and make it DRY (Don't Repeat Yourself) as a desert!Consider whipping up a reusable function for URL construction. It'll make your code sleeker than a greased lightning bolt! Here's a snazzy example:
function construct_contribution_link($type, $action_target) { $base_url = $type === 'dropping-center' ? '/dropping-center-contribution' : '/collection-camp-contribution'; $state_key = $type === 'dropping-center' ? 'Dropping_Centre.State' : 'Collection_Camp_Intent_Details.State'; $city_key = $type === 'dropping-center' ? 'Dropping_Centre.District_City' : 'Collection_Camp_Intent_Details.City'; return sprintf( '%s?source=%s&target_id=%s&state_province_id=%s&city=%s', $base_url, $action_target['title'], $action_target['id'], $action_target[$state_key], $action_target[$city_key] ); } $material_contribution_link = construct_contribution_link('collection-camp', $action_target); $dropping_center_material_contribution_link = construct_contribution_link('dropping-center', $action_target);This approach will make your code more maintainable than a well-oiled machine and reduce the risk of inconsistencies faster than you can say "PHP"!
| $target_info = $target_data[$target]; | ||
|
|
||
| $start_date = new DateTime($action_target[$target_info['start_time']]); | ||
| $end_date = new DateTime($action_target[$target_info['end_time']]); | ||
| $address = $action_target[$target_info['address']]; | ||
| $contribution_link = $target_info['contribution_link']; |
There was a problem hiding this comment.
Good refactoring, add error handling
The refactored logic for handling different targets is a significant improvement. It enhances code readability and maintainability by using the $target_data array to retrieve information based on the target type.
However, there's a potential issue with error handling when creating DateTime objects. Invalid date strings can cause exceptions. Consider adding try-catch blocks to handle potential exceptions gracefully:
try {
$start_date = new DateTime($action_target[$target_info['start_time']]);
$end_date = new DateTime($action_target[$target_info['end_time']]);
} catch (Exception $e) {
// Log the error and set default values
error_log("Error creating DateTime object: " . $e->getMessage());
$start_date = $end_date = new DateTime();
}This will prevent the script from crashing if it encounters invalid date strings and provide a fallback mechanism.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
wp-content/plugins/goonj-blocks/build/render.php (2)
51-64: Excellent refactoring with $target_data arrayThe introduction of the
$target_dataarray is a great step towards better code organization and maintainability. It centralizes the configuration for different target types, making it easier to add or modify targets in the future.Consider moving this array to a separate configuration file (e.g.,
target_config.php) to further improve code organization and separation of concerns. This would make it easier to manage and update the configuration without touching the main render logic.
65-78: Improved control flow and error handlingThe updates to the control flow using the new
$target_dataarray and the addition of error handling for DateTime object creation are excellent improvements. These changes enhance the code's robustness and maintainability.Consider using a more specific error logging method instead of
Civi::log()->error(). For example, you could create a custom error logging function that includes more context about where the error occurred:function logGoonjError($message, $context = []) { Civi::log()->error("Goonj Blocks Error: " . $message, $context); } // Usage logGoonjError("Error creating DateTime object: " . $e->getMessage(), ['file' => __FILE__, 'line' => __LINE__]);This would make it easier to track and debug errors specific to the Goonj Blocks plugin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/plugins/goonj-blocks/build/render.php (3 hunks)
- wp-content/plugins/goonj-blocks/src/render.php (3 hunks)
🧰 Additional context used
📓 Learnings (2)
wp-content/plugins/goonj-blocks/build/render.php (1)
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/plugins/goonj-blocks/src/render.php:45-47 Timestamp: 2024-10-08T09:25:18.536Z Learning: In the `wp-content/plugins/goonj-blocks/src/render.php` file, the team considers error handling for `DateTime` object creation unnecessary when using `$action_target['Dropping_Centre.Start_Time']` and `$action_target['Dropping_Centre.End_Time']`.wp-content/plugins/goonj-blocks/src/render.php (1)
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/plugins/goonj-blocks/src/render.php:45-47 Timestamp: 2024-10-08T09:25:18.536Z Learning: In the `wp-content/plugins/goonj-blocks/src/render.php` file, the team considers error handling for `DateTime` object creation unnecessary when using `$action_target['Dropping_Centre.Start_Time']` and `$action_target['Dropping_Centre.End_Time']`.
🔇 Additional comments (5)
wp-content/plugins/goonj-blocks/build/render.php (1)
108-108: Well-implemented dynamic contribution linkThe update to use the
$contribution_linkvariable with a fallback using the null coalescing operator is a good improvement. It ensures that the correct link is used based on the target type and provides a safe fallback if the link is not set.This change aligns well with the refactoring done earlier in the file and improves the overall consistency of the code.
wp-content/plugins/goonj-blocks/src/render.php (4)
33-39: Eliminate code duplication by creating a generic function for contribution links.The new
$dropping_center_material_contribution_linkis strikingly similar to the existing$material_contribution_link. This violates the DRY (Don't Repeat Yourself) principle and makes the code harder to maintain.Consider refactoring both link generations into a single function:
function generate_contribution_link($type, $action_target) { $base_url = ($type === 'dropping-center') ? '/dropping-center-contribution' : '/collection-camp-contribution'; $state_key = ($type === 'dropping-center') ? 'Dropping_Centre.State' : 'Collection_Camp_Intent_Details.State'; $city_key = ($type === 'dropping-center') ? 'Dropping_Centre.District_City' : 'Collection_Camp_Intent_Details.City'; return sprintf( '%s?source=%s&target_id=%s&state_province_id=%s&city=%s', $base_url, $action_target['title'], $action_target['id'], $action_target[$state_key], $action_target[$city_key] ); } $material_contribution_link = generate_contribution_link('collection-camp', $action_target); $dropping_center_material_contribution_link = generate_contribution_link('dropping-center', $action_target);This approach will make the code more maintainable and reduce the risk of inconsistencies when updating URL construction logic.
51-64: Excellent refactoring with $target_data array, consider adding type validation.The introduction of the
$target_dataarray is a great improvement in code organization and maintainability. It provides a clear structure for different target types and makes it easier to add new types in the future.To further enhance this structure, consider adding type validation:
$valid_targets = ['dropping-center', 'collection-camp']; $target_data = [ 'dropping-center' => [ // ... existing data ... ], 'collection-camp' => [ // ... existing data ... ], ]; if (!in_array($target, $valid_targets)) { throw new InvalidArgumentException("Invalid target type: $target"); }This addition will help catch potential errors early and make the code more robust.
67-77: Great improvement in error handling and consistency, consider enhancing logging.The refactored code for retrieving target info and dates is a significant improvement. It enhances flexibility, error handling, and consistency across different target types.
To further improve the error logging, consider adding more context:
try { $start_date = new DateTime($action_target[$target_info['start_time']]); $end_date = new DateTime($action_target[$target_info['end_time']]); } catch (Exception $e) { Civi::log()->error("Error creating DateTime object for $target: " . $e->getMessage(), [ 'target' => $target, 'start_time' => $action_target[$target_info['start_time']], 'end_time' => $action_target[$target_info['end_time']], ]); $start_date = $end_date = new DateTime(); // Default to current date/time }This will provide more detailed information for debugging purposes if an error occurs.
108-108: Excellent use of null coalescing operator for fallback.The update to use
$contribution_link ?? '#'is a great improvement. It ensures that even if$contribution_linkis not set for some reason, the button will still render without causing a PHP notice. This enhances the robustness of the code.Good job on maintaining consistency with the new
$target_datastructure while also providing a sensible fallback.
| try { | ||
| $start_date = new DateTime($action_target[$target_info['start_time']]); | ||
| $end_date = new DateTime($action_target[$target_info['end_time']]); | ||
| } catch (Exception $e) { | ||
| Civi::log()->error("Error creating DateTime object: " . $e->getMessage()); | ||
| $start_date = $end_date = new DateTime(); // Default to current date/time | ||
| } | ||
|
|
||
| $address = $action_target[$target_info['address']]; | ||
| $contribution_link = $target_info['contribution_link']; |
There was a problem hiding this comment.
Although I always receive the start time and end time since these fields are required, I am adding error handling here as a precaution to set default values.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
wp-content/plugins/goonj-blocks/build/render.php (3)
51-64: Good addition of$target_data, consider further refactoringThe introduction of the
$target_dataarray is a good step towards improving code organization and maintainability. It allows for easy addition of new target types in the future and centralizes the configuration for different targets.To further improve the code, consider integrating this array with the contribution link generation logic. This could be achieved by extending the
$target_dataarray to include all necessary information for link generation, eliminating the need for separate link generation code. Here's a suggestion:$target_data = [ 'dropping-center' => [ 'start_time' => 'Dropping_Centre.Start_Time', 'end_time' => 'Dropping_Centre.End_Time', 'address' => 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_', 'contribution_path' => '/dropping-center-contribution', 'state_field' => 'Dropping_Centre.State', 'city_field' => 'Dropping_Centre.District_City', ], 'collection-camp' => [ '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_path' => '/collection-camp-contribution', 'state_field' => 'Collection_Camp_Intent_Details.State', 'city_field' => 'Collection_Camp_Intent_Details.City', ], ]; function generate_contribution_link($target_type, $action_target, $target_data) { $config = $target_data[$target_type]; return sprintf( '%s?source=%s&target_id=%s&state_province_id=%s&city=%s', $config['contribution_path'], $action_target['title'], $action_target['id'], $action_target[$config['state_field']], $action_target[$config['city_field']] ); } // Usage $contribution_link = generate_contribution_link($target, $action_target, $target_data);This approach would centralize all target-specific configuration in one place, making the code more maintainable and easier to extend in the future.
69-75: Good addition of error handling, minor improvement suggestedThe introduction of error handling for DateTime object creation is a great improvement. It prevents the script from crashing when encountering invalid date strings and provides a fallback mechanism.
To make the error message more informative, consider including the target type and the specific date field that caused the error. This will make debugging easier in the future. Here's a suggested improvement:
try { $start_date = new DateTime($action_target[$target_info['start_time']]); $end_date = new DateTime($action_target[$target_info['end_time']]); } catch (Exception $e) { Civi::log()->error("Error creating DateTime object for $target: " . $e->getMessage() . ". Start time: {$action_target[$target_info['start_time']]}, " . "End time: {$action_target[$target_info['end_time']]}"); $start_date = $end_date = new DateTime(); // Default to current date/time }This more detailed error message will provide better context for debugging purposes.
79-79: Good updates to address label and contribution link, minor improvement suggestedThe introduction of the
$address_labelvariable and the updated contribution link usage are good improvements. They enhance the clarity of the code and handle potential null values effectively.For consistency and to further centralize the configuration, consider moving the address label logic into the
$target_dataarray. This would allow for easier management of target-specific labels. Here's a suggested improvement:$target_data = [ 'dropping-center' => [ // ... existing fields ... 'address_label' => 'Area of the dropping center', ], 'collection-camp' => [ // ... existing fields ... 'address_label' => 'Address of the camp', ], ]; // Usage $address_label = $target_data[$target]['address_label'];This change would eliminate the need for the conditional statement at line 79 and make it easier to add or modify labels for different target types in the future.
Also applies to: 100-100, 109-109
wp-content/plugins/goonj-blocks/src/render.php (1)
67-78: Error handling that would make Houdini proud!Kudos on the magical error-handling act you've pulled off here! It's like watching a software engineer perform a high-wire act with a safety net. Bravo!
The use of
$target_datais smoother than a buttered slide, making our code more flexible than a yoga instructor. And that try-catch block? It's catching errors like a pro baseball player in the World Series!One tiny suggestion to make this code even more bulletproof:
$contribution_link = $target_info['contribution_link'] ?? '#';This way, even if the contribution link goes AWOL, we've got a fallback. It's like giving our code a parachute, just in case!
Keep up the stellar work, code conjurer!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- wp-content/plugins/goonj-blocks/build/render.php (4 hunks)
- wp-content/plugins/goonj-blocks/src/render.php (4 hunks)
🧰 Additional context used
📓 Learnings (2)
wp-content/plugins/goonj-blocks/build/render.php (1)
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/plugins/goonj-blocks/src/render.php:45-47 Timestamp: 2024-10-08T09:25:18.536Z Learning: In the `wp-content/plugins/goonj-blocks/src/render.php` file, the team considers error handling for `DateTime` object creation unnecessary when using `$action_target['Dropping_Centre.Start_Time']` and `$action_target['Dropping_Centre.End_Time']`.wp-content/plugins/goonj-blocks/src/render.php (1)
Learnt from: belwalshubham PR: ColoredCow/goonj#284 File: wp-content/plugins/goonj-blocks/src/render.php:45-47 Timestamp: 2024-10-08T09:25:18.536Z Learning: In the `wp-content/plugins/goonj-blocks/src/render.php` file, the team considers error handling for `DateTime` object creation unnecessary when using `$action_target['Dropping_Centre.Start_Time']` and `$action_target['Dropping_Centre.End_Time']`.
🔇 Additional comments (2)
wp-content/plugins/goonj-blocks/src/render.php (2)
51-64: Bravo! A data structure that sparks joy!This
$target_dataarray is like Marie Kondo for our code - it's tidying up and bringing order to our chaotic world of target types. It's a beautiful sight to behold!The structure is clean, readable, and makes it easy to add new target types in the future. It's like you've given our code a makeover, and it's ready for the software development runway!
Keep up the great work, code fashionista!
Line range hint
100-111: Security and UX walk into a bar......and the bartender says, "I'll need to see some
esc_htmlandesc_url, please!" 🍻Seriously though, your attention to security here is tighter than Fort Knox! The
esc_htmlandesc_urlfunctions are working overtime to keep our code safe from mischievous input.And that dynamic address label? It's like a chameleon, adapting to its surroundings. User experience: 1, Confusion: 0!
The cherry on top? That null coalescing operator (??) in the contribution link. It's like a safety net for our URLs. If the link goes missing, we've got a backup plan faster than you can say "404 Not Found"!
You're not just writing code, you're crafting a masterpiece of security and user-friendliness. Bravo!
| $dropping_center_material_contribution_link = sprintf( | ||
| '/dropping-center-contribution?source=%s&target_id=%s&state_province_id=%s&city=%s', | ||
| $action_target['title'], | ||
| $action_target['id'], | ||
| $action_target['Dropping_Centre.State'], | ||
| $action_target['Dropping_Centre.District_City'], | ||
| ); |
There was a problem hiding this comment.
Eliminate code duplication for contribution link generation
The new $dropping_center_material_contribution_link is almost identical to the existing $material_contribution_link. This violates the DRY (Don't Repeat Yourself) principle. Let's refactor this to a single function that generates the link based on the target type.
Here's a suggested refactor:
function generate_contribution_link($target_type, $action_target) {
$config = [
'collection-camp' => [
'path' => '/collection-camp-contribution',
'state_field' => 'Collection_Camp_Intent_Details.State',
'city_field' => 'Collection_Camp_Intent_Details.City'
],
'dropping-center' => [
'path' => '/dropping-center-contribution',
'state_field' => 'Dropping_Centre.State',
'city_field' => 'Dropping_Centre.District_City'
]
];
$target_config = $config[$target_type];
return sprintf(
'%s?source=%s&target_id=%s&state_province_id=%s&city=%s',
$target_config['path'],
$action_target['title'],
$action_target['id'],
$action_target[$target_config['state_field']],
$action_target[$target_config['city_field']]
);
}
$material_contribution_link = generate_contribution_link('collection-camp', $action_target);
$dropping_center_material_contribution_link = generate_contribution_link('dropping-center', $action_target);This refactoring will make the code more maintainable and reduce the risk of inconsistencies when updating link generation logic.
| $dropping_center_material_contribution_link = sprintf( | ||
| '/dropping-center-contribution?source=%s&target_id=%s&state_province_id=%s&city=%s', | ||
| $action_target['title'], | ||
| $action_target['id'], | ||
| $action_target['Dropping_Centre.State'], | ||
| $action_target['Dropping_Centre.District_City'], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Houston, we have a code clone!
Looks like we've got a case of copy-paste syndrome here. The new $dropping_center_material_contribution_link is practically twins with $material_contribution_link. Let's not make our code look like a xerox machine went wild!
Consider creating a reusable function to construct these links:
function build_contribution_link($type, $action_target) {
$base_url = $type === 'dropping-center' ? '/dropping-center-contribution' : '/collection-camp-contribution';
$state_key = $type === 'dropping-center' ? 'Dropping_Centre.State' : 'Collection_Camp_Intent_Details.State';
$city_key = $type === 'dropping-center' ? 'Dropping_Centre.District_City' : 'Collection_Camp_Intent_Details.City';
return sprintf(
'%s?source=%s&target_id=%s&state_province_id=%s&city=%s',
$base_url,
$action_target['title'],
$action_target['id'],
$action_target[$state_key],
$action_target[$city_key]
);
}
$material_contribution_link = build_contribution_link('collection-camp', $action_target);
$dropping_center_material_contribution_link = build_contribution_link('dropping-center', $action_target);This way, we keep our code DRY and our developers happy. Win-win!
| $start_date = new DateTime($action_target[$target_info['start_time']]); | ||
| $end_date = new DateTime($action_target[$target_info['end_time']]); | ||
| } catch (Exception $e) { | ||
| Civi::log()->error("Error creating DateTime object: " . $e->getMessage()); |
|
|
||
| $address = $action_target[$target_info['address']]; | ||
| $contribution_link = $target_info['contribution_link']; | ||
| $address_label = ($target === 'dropping-center') ? 'Area of the dropping center' : 'Address of the camp'; |
There was a problem hiding this comment.
can we also add this in configuration array so that we can do something like this
| $address_label = ($target === 'dropping-center') ? 'Area of the dropping center' : 'Address of the camp'; | |
| $address_label = $target_data['dropping-center']['address_label']; |
| $action_target['id'], | ||
| $action_target['Dropping_Centre.State'], | ||
| $action_target['Dropping_Centre.District_City'], | ||
| ); |
There was a problem hiding this comment.
Check formatting on src file
| Civi::log()->error("Error creating DateTime object: " . $e->getMessage()); | ||
| $start_date = $end_date = new DateTime(); // Default to current date/time |
There was a problem hiding this comment.
Please check the Indentation
| </tr> | ||
| <tr class="wp-block-gb-table-row"> | ||
| <td class="wp-block-gb-table-cell wp-block-gb-table-header">Address of the camp</td> | ||
| <td class="wp-block-gb-table-cell wp-block-gb-table-header"><?php echo esc_html($address_label); ?></td> |
There was a problem hiding this comment.
Look's like some extra spaces are here
| 'start_time' => 'Dropping_Centre.Start_Time', | ||
| 'end_time' => 'Dropping_Centre.End_Time', |
There was a problem hiding this comment.
Are we capturing time here on that custom fields or are we saving date and time both?
There was a problem hiding this comment.
We are displaying the date below but we have a field named start time and end time , which is why I included it in the array. However, in the code below, I have stored this as startDate and endDate, let me know if you any more question here
$start_date = new DateTime($action_target[$target_info['start_time']]);
$end_date = new DateTime($action_target[$target_info['end_time']]);
| try { | ||
| $start_date = new DateTime($action_target[$target_info['start_time']]); | ||
| $end_date = new DateTime($action_target[$target_info['end_time']]); | ||
| } catch (Exception $e) { | ||
| Civi::log()->error("Error creating DateTime object: " . $e->getMessage()); | ||
| $start_date = $end_date = new DateTime(); // Default to current date/time |
There was a problem hiding this comment.
I expect the start and end dates are required fields, so why do we need the try-catch block here? Are there any cases where the start date or end date might be missing?
There was a problem hiding this comment.
Error handling is important here because, let say there is some case the required fields are mistakenly removed from the form builder, we need to implement error handling as a precautionary measure.
3 participants
This PR updates how we manage the start date, end date, and address fields for both collection camps and dropping centers.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements