fix(*) send physical induction slot for mulitiple city#516
fix(*) send physical induction slot for mulitiple city#516nishant22029 merged 4 commits intodevelopfrom
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request primarily focus on enhancing the logic for volunteer induction processes within the Goonj system. Key updates include modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@nishant22029 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 19 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
wp-content/plugins/goonj-blocks/src/functions.php (3)
102-102: Add validation for city format in database queryThe query selects city data but doesn't ensure consistent formatting. Consider adding a SQL-level formatting function.
- ->addSelect('id', 'display_name', 'Goonj_Office_Details.Other_Induction_Cities', 'address_primary.city') + ->addSelect('id', 'display_name', 'Goonj_Office_Details.Other_Induction_Cities', + 'LOWER(TRIM(address_primary.city)) as address_primary_city')
119-123: Optimize city array operationsThe current implementation performs multiple array operations that could be combined for better efficiency.
- $otherCities = array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities'])); - $officeContactInductionCities = array_merge($officeContactInductionCities, $otherCities); + $officeContactInductionCities = array_merge( + $officeContactInductionCities, + array_filter(array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities']))) + );
107-129: Add documentation for city handling logicThe city handling logic is complex and lacks documentation explaining the business rules and edge cases.
Add PHPDoc comments explaining:
- The purpose of the
$officeContactInductionCitiesarray- The city formatting rules
- How duplicates are handled
- The significance of primary vs other induction cities
wp-content/plugins/goonj-blocks/build/functions.php (4)
108-110: Remove duplicate initialization of$officeContactInductionCitiesThe variable
$officeContactInductionCitiesis initialized twice without any operations in between. This redundancy can be removed to streamline the code.Apply this diff to remove the redundant line:
108 $officeContactInductionCities = []; 110 - $officeContactInductionCities = [];
105-105: Eliminate commented-out code for cleaner codebaseThe commented-out line at line 105 is no longer in use. Removing unused code helps in keeping the codebase clean and maintainable.
Apply this diff to remove the commented-out line:
105 - // ->addWhere('address_primary.city', 'LIKE', $contactCityFormatted . '%')
113-133: Refactor induction cities extraction into a separate functionThe block of code from lines 113 to 133 is responsible for extracting and processing induction cities from
$officeContactData. To adhere to the single responsibility principle and enhance readability, consider moving this logic into a dedicated function.This refactoring will make the main function cleaner and the code easier to test and maintain.
150-150: Remove obsolete commented-out codeThe commented-out return statement at line 150 appears to be outdated and unnecessary. Eliminating such code reduces clutter and potential confusion.
Apply this diff to remove the unused code:
150 - // return generate_slots($assignedOfficeId, $defaultMaxSlot, $physicalInductionType, $inductionSlotStartDate);wp-content/civi-extensions/goonjcustom/Civi/InductionService.php (4)
835-856: Extract Logic into a Separate Method for ClarityThe block of code responsible for collecting induction cities from office contacts is sizable and could be extracted into its own method. This adheres to the Single Responsibility Principle and improves code readability.
Consider extracting the code into a method like
getOfficeContactInductionCities($officeContact)and then call this method withinfetchTypeOfInduction.
864-869: Simplify Induction Type Determination LogicThe current
elseblock redundantly assigns$inductionType = 'Online'even though it's not needed. Since the variable is already set to 'Online' if the condition isn't met, you can streamline this part of the code for better readability.Apply this diff to simplify the logic:
if (in_array(strtolower($contactCityFormatted), $officeContactInductionCities)) { return $inductionType; } else { - $inductionType = 'Online'; return 'Online'; }
828-869: Avoid Deep Nesting and Improve ReadabilityThe method
fetchTypeOfInductioncontains deep nesting, making it harder to read and maintain. Consider refactoring the code to reduce nesting levels, possibly by using early returns or breaking down the code into smaller methods.For example:
if ($officeContact->count() === 0) { return 'Online'; } // Proceed with processing since offices existThis approach enhances code clarity by handling exceptional cases early.
828-869: Add Comments to Explain Complex LogicThe logic within
fetchTypeOfInductionis non-trivial. Adding explanatory comments or docstrings will help other developers understand the purpose and functionality of the code, improving maintainability.Consider adding comments that describe the steps involved in determining the induction type based on state and city.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
wp-content/civi-extensions/goonjcustom/Civi/InductionService.php(1 hunks)wp-content/plugins/goonj-blocks/build/functions.php(1 hunks)wp-content/plugins/goonj-blocks/build/render.php(0 hunks)wp-content/plugins/goonj-blocks/src/functions.php(1 hunks)
💤 Files with no reviewable changes (1)
- wp-content/plugins/goonj-blocks/build/render.php
🔇 Additional comments (1)
wp-content/plugins/goonj-blocks/src/functions.php (1)
102-140: Verify consistent city handling across codebase
Let's ensure that city comparisons and formatting are consistent across the codebase.
✅ Verification successful
City handling looks consistent with best practices
The code follows consistent patterns for city handling:
- Uses array_merge() to combine primary and other induction cities
- Applies strtolower() for case-insensitive city comparisons
- Properly handles array operations with array_unique() and array_values()
- Validates city existence before access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for city comparisons and formatting in PHP files
echo "Searching for city comparisons..."
rg -t php "(?i)(city.*=|=.*city|\bcity\b.*comparison)" -A 3 -B 3
echo "Searching for array operations on cities..."
rg -t php "(?i)(explode.*city|array_map.*city|array_merge.*city)" -A 3 -B 3
echo "Checking for potential case sensitivity issues..."
rg -t php "(?i)(strtolower.*city|strtoupper.*city)" -A 3 -B 3
Length of output: 154401
| if (in_array(strtolower($contactCityFormatted), $officeContactInductionCities)) { | ||
| return generate_slots($assignedOfficeId, $defaultMaxSlot, $physicalInductionType, $inductionSlotStartDate); | ||
| } else { | ||
| return generate_slots($assignedOfficeId, $defaultMaxSlot, $onlineInductionType, $inductionSlotStartDate); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve induction type determination logic
The current implementation has several potential issues:
- Case sensitivity in city comparison could cause bugs
- Missing logging for debugging induction type decisions
- Complex nested conditions
- if (in_array(strtolower($contactCityFormatted), $officeContactInductionCities)) {
- return generate_slots($assignedOfficeId, $defaultMaxSlot, $physicalInductionType, $inductionSlotStartDate);
- } else {
- return generate_slots($assignedOfficeId, $defaultMaxSlot, $onlineInductionType, $inductionSlotStartDate);
- }
+ $inductionType = in_array(strtolower($contactCityFormatted), $officeContactInductionCities)
+ ? $physicalInductionType
+ : $onlineInductionType;
+
+ \Civi::log()->info('Determined induction type', [
+ 'city' => $contactCityFormatted,
+ 'type' => $inductionType,
+ 'availableCities' => $officeContactInductionCities
+ ]);
+
+ return generate_slots($assignedOfficeId, $defaultMaxSlot, $inductionType, $inductionSlotStartDate);📝 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 (in_array(strtolower($contactCityFormatted), $officeContactInductionCities)) { | |
| return generate_slots($assignedOfficeId, $defaultMaxSlot, $physicalInductionType, $inductionSlotStartDate); | |
| } else { | |
| return generate_slots($assignedOfficeId, $defaultMaxSlot, $onlineInductionType, $inductionSlotStartDate); | |
| } | |
| $inductionType = in_array(strtolower($contactCityFormatted), $officeContactInductionCities) | |
| ? $physicalInductionType | |
| : $onlineInductionType; | |
| \Civi::log()->info('Determined induction type', [ | |
| 'city' => $contactCityFormatted, | |
| 'type' => $inductionType, | |
| 'availableCities' => $officeContactInductionCities | |
| ]); | |
| return generate_slots($assignedOfficeId, $defaultMaxSlot, $inductionType, $inductionSlotStartDate); |
| if ($officeContact->count() > 0) { | ||
| // Extract the first row (assuming one result, based on rowCount => 1) | ||
| $officeContactData = $officeContact[0]; | ||
|
|
||
| // Add the primary city to the array | ||
| if (!empty($officeContactData['address_primary.city'])) { | ||
| $officeContactInductionCities[] = $officeContactData['address_primary.city']; | ||
| } | ||
|
|
||
| // Add the other induction cities to the array | ||
| if (!empty($officeContactData['Goonj_Office_Details.Other_Induction_Cities'])) { | ||
| // Split the string into an array and merge it | ||
| $otherCities = array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities'])); | ||
| $officeContactInductionCities = array_merge($officeContactInductionCities, $otherCities); | ||
| } | ||
|
|
||
| // Convert all cities to lowercase, remove duplicates, and re-index the array | ||
| $officeContactInductionCities = array_map('strtolower', $officeContactInductionCities); | ||
| $officeContactInductionCities = array_unique($officeContactInductionCities); | ||
| $officeContactInductionCities = array_values($officeContactInductionCities); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to Handle Multiple Offices and Avoid Assumptions
The current implementation assumes only one Goonj office per state by directly accessing $officeContact[0]. This assumption could lead to issues if multiple offices exist in the same state. To enhance scalability and maintainability, consider iterating over all matching offices to gather induction cities.
Apply this diff to handle multiple offices:
-// Extract the first row (assuming one result, based on rowCount => 1)
-$officeContactData = $officeContact[0];
-
-// Add the primary city to the array
-if (!empty($officeContactData['address_primary.city'])) {
- $officeContactInductionCities[] = $officeContactData['address_primary.city'];
-}
-
-// Add the other induction cities to the array
-if (!empty($officeContactData['Goonj_Office_Details.Other_Induction_Cities'])) {
- // Split the string into an array and merge it
- $otherCities = array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities']));
- $officeContactInductionCities = array_merge($officeContactInductionCities, $otherCities);
-}
-
-// Convert all cities to lowercase, remove duplicates, and re-index the array
-$officeContactInductionCities = array_map('strtolower', $officeContactInductionCities);
-$officeContactInductionCities = array_unique($officeContactInductionCities);
-$officeContactInductionCities = array_values($officeContactInductionCities);
+// Iterate over each office to collect induction cities
+foreach ($officeContact as $officeContactData) {
+ // Add the primary city to the array
+ if (!empty($officeContactData['address_primary.city'])) {
+ $officeContactInductionCities[] = $officeContactData['address_primary.city'];
+ }
+
+ // Add the other induction cities to the array
+ if (!empty($officeContactData['Goonj_Office_Details.Other_Induction_Cities'])) {
+ // Split the string into an array and merge it
+ $otherCities = array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities']));
+ $officeContactInductionCities = array_merge($officeContactInductionCities, $otherCities);
+ }
+}
+
+// Normalize the induction cities array
+$officeContactInductionCities = array_map('strtolower', $officeContactInductionCities);
+$officeContactInductionCities = array_unique($officeContactInductionCities);
+$officeContactInductionCities = array_values($officeContactInductionCities);📝 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 ($officeContact->count() > 0) { | |
| // Extract the first row (assuming one result, based on rowCount => 1) | |
| $officeContactData = $officeContact[0]; | |
| // Add the primary city to the array | |
| if (!empty($officeContactData['address_primary.city'])) { | |
| $officeContactInductionCities[] = $officeContactData['address_primary.city']; | |
| } | |
| // Add the other induction cities to the array | |
| if (!empty($officeContactData['Goonj_Office_Details.Other_Induction_Cities'])) { | |
| // Split the string into an array and merge it | |
| $otherCities = array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities'])); | |
| $officeContactInductionCities = array_merge($officeContactInductionCities, $otherCities); | |
| } | |
| // Convert all cities to lowercase, remove duplicates, and re-index the array | |
| $officeContactInductionCities = array_map('strtolower', $officeContactInductionCities); | |
| $officeContactInductionCities = array_unique($officeContactInductionCities); | |
| $officeContactInductionCities = array_values($officeContactInductionCities); | |
| } | |
| if ($officeContact->count() > 0) { | |
| // Iterate over each office to collect induction cities | |
| foreach ($officeContact as $officeContactData) { | |
| // Add the primary city to the array | |
| if (!empty($officeContactData['address_primary.city'])) { | |
| $officeContactInductionCities[] = $officeContactData['address_primary.city']; | |
| } | |
| // Add the other induction cities to the array | |
| if (!empty($officeContactData['Goonj_Office_Details.Other_Induction_Cities'])) { | |
| // Split the string into an array and merge it | |
| $otherCities = array_map('trim', explode(',', $officeContactData['Goonj_Office_Details.Other_Induction_Cities'])); | |
| $officeContactInductionCities = array_merge($officeContactInductionCities, $otherCities); | |
| } | |
| } | |
| // Normalize the induction cities array | |
| $officeContactInductionCities = array_map('strtolower', $officeContactInductionCities); | |
| $officeContactInductionCities = array_unique($officeContactInductionCities); | |
| $officeContactInductionCities = array_values($officeContactInductionCities); | |
| } |
| ->addWhere('contact_sub_type', 'CONTAINS', 'Goonj_Office') | ||
| ->addWhere('address_primary.state_province_id', '=', $contactStateId) | ||
| ->addWhere('address_primary.city', 'LIKE', $contactCityFormatted . '%') | ||
| // ->addWhere('address_primary.city', 'LIKE', $contactCityFormatted . '%') |
There was a problem hiding this comment.
If we don't need it can we remove it
| ->addWhere('address_primary.city', 'LIKE', $contactCityFormatted . '%') | ||
| // ->addWhere('address_primary.city', 'LIKE', $contactCityFormatted . '%') | ||
| ->execute(); | ||
| \Civi::log()->info('officeContact', ['officeContact'=>$officeContact]); |
| $officeContactInductionCities = []; | ||
|
|
||
| if ($officeContact->count() === 0) { | ||
| $officeContactInductionCities = []; |
There was a problem hiding this comment.
Why we need these two with same variable name?
| // Check if the result has any rows | ||
| if ($officeContact->count() > 0) { | ||
| // Extract the first row (assuming one result, based on rowCount => 1) | ||
| $officeContactData = $officeContact[0]; |
There was a problem hiding this comment.
What if we have more multiple offices exist in the same state
| // Log the result for debugging | ||
| \Civi::log()->info('officeContactInductionCities', ['officeContactInductionCities' => $officeContactInductionCities, 'contactCityFormatted'=>$contactCityFormatted]); | ||
|
|
There was a problem hiding this comment.
Why do we need debugging now? Do we know that we were getting any error?
|
|
||
|
|
| } | ||
| // generate physical induction slots having office in their states | ||
| return generate_slots($assignedOfficeId, $defaultMaxSlot, $physicalInductionType, $inductionSlotStartDate); | ||
| // return generate_slots($assignedOfficeId, $defaultMaxSlot, $physicalInductionType, $inductionSlotStartDate); |
There was a problem hiding this comment.
If we dont need it, please remove it
nishant22029
deleted the
fix/check-multiple-cities-for-physical-induction
branch
2 participants
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes