Fetch contacts details based on contact id#472
Fetch contacts details based on contact id#472belwalshubham wants to merge 2 commits intodevelopfrom
Conversation
belwalshubham
requested review from
nishant22029 and
tarunnjoshi
and removed request for
tarunnjoshi
WalkthroughThe pull request introduces significant changes to the 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 (
|
| $contacts = Contact::get(FALSE) | ||
| ->addSelect('contact_sub_type') | ||
| ->addWhere('id', '=', $contactId) | ||
| ->execute()->single(); |
There was a problem hiding this comment.
$contactDetails = Contact::get(FALSE)
->addSelect('contact_sub_type')
->addWhere('id', '=', $contactId)
->execute();
$contactDetail = $contactDetails->first();
if( empty($contactDetail)){
return;
}
| $organization = Organization::get(FALSE) | ||
| ->addSelect('Institute_Registration.Type_of_Institution:label') | ||
| ->addWhere('id', '=', $contactId) | ||
| ->execute()->single(); |
There was a problem hiding this comment.
If contact not exist then this query will give error do you have tested that case?
There was a problem hiding this comment.
It never breaks because I've added a check before this condition to return if no contact ID is found:
if (!$contactId)
{
\CRM_Core_Error::debug_log_message('Contact ID not found');
return; }
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (3)
Line range hint
59-66: Eliminate duplicate checks for$stateIdThe condition
if (!$stateId)is checked twice consecutively, which is redundant. Consolidate these checks to improve code clarity and maintainability.Apply this diff to remove the duplicate condition:
- if (!$stateId) { - return; - } - if (!$stateId) { - \CRM_Core_Error::debug_log_message('Cannot assign Goonj Office to institution id: ' . $contactId); - return; - } + if (!$stateId) { + \CRM_Core_Error::debug_log_message('Cannot assign Goonj Office to institution id: ' . $contactId); + return; + }
Line range hint
40-100: RefactorsetOfficeDetailsfor single responsibilityThe
setOfficeDetailsmethod handles multiple concerns, including:
- Validating operation type and object name.
- Retrieving contact and state information.
- Assigning offices and coordinators.
To adhere to the Single Responsibility Principle and enhance readability, consider splitting this method into smaller, focused methods.
Line range hint
177-190: Improve robustness ofgetRelationshipTypeNamemappingThe
getRelationshipTypeNamemethod relies on extracting the first word of$typeOfInstitutionto determine the relationship type. This may not account for institution types with multiple words or unexpected formats. To ensure accurate mappings:
- Use the full
$typeOfInstitutionas a key in the$typeToRelationshipMap.- Validate that all possible institution types are covered in the mapping.
| * The parameters that were sent into the calling function. | ||
| */ | ||
| public static function setOfficeDetails(string $op, string $objectName, int $objectId, &$objectRef) { | ||
| public static function setOfficeDetails(string $op, string $objectName, int $contactId, &$objectRef) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid overwriting the $contactId parameter
The $contactId parameter is being overwritten inside the setOfficeDetails method with $objectRef->contact_id. This can lead to confusion and potential bugs. Consider either:
- Removing
$contactIdfrom the parameter list and relying solely on$objectRef->contact_id. - Using the passed
$contactIdwithout overwriting it.
Apply this diff to avoid overwriting the parameter:
- $contactId = $objectRef->contact_id;Also applies to: 46-46
| public static function setOfficeDetails(string $op, string $objectName, int $contactId, &$objectRef) { | ||
|
|
||
| if ($objectName !== 'AfformSubmission') { | ||
| if ($op !== 'create' || $objectName !== 'Address') { |
There was a problem hiding this comment.
can we add one more check here to check the form or something unique because I expect the other things won't work if we merge it like this?
Use case: Please check the monitory contribution case here, and check if the new individual came there then it goes to their correct group.
NOTE: Please don't merge this PR before resolving these things and please test all the cases on staging after taking latest staging pull
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (3)
48-49: Enhance error logging with more contextThe error message could be more informative by including the state ID that was missing.
- \CRM_Core_Error::debug_log_message('Cannot assign Goonj Office to institution id: ' . $contactId); + \CRM_Core_Error::debug_log_message(sprintf( + 'Cannot assign Goonj Office to institution (ID: %d) - missing state_province_id', + $contactId + ));
55-56: Improve error message with operation contextThe error message should include more context about the operation being performed.
- \CRM_Core_Error::debug_log_message('Contact id not found'); + \CRM_Core_Error::debug_log_message(sprintf( + 'Contact ID not found while setting office details for address creation (Reference ID: %s)', + $objectRef->id ?? 'unknown' + ));
Line range hint
40-195: Consider extracting common patterns into helper methodsThere are several opportunities to improve code organization:
- Create a centralized error logging helper
- Extract coordinator selection logic into a separate method
Here's a suggested approach:
private static function logError($message, $context = []) { $contextStr = !empty($context) ? ' Context: ' . json_encode($context) : ''; \CRM_Core_Error::debug_log_message($message . $contextStr); } private static function selectRandomCoordinator($coordinators) { if ($coordinators->count() === 0) { return null; } $randomIndex = rand(0, $coordinators->count() - 1); return $coordinators->itemAt($randomIndex); }This would allow for consistent error logging and remove duplicate coordinator selection logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(2 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (1)
40-42: Verify test coverage for all use cases
As mentioned in previous reviews, please ensure test coverage for:
- Monitory contribution cases
- New individual scenarios
- Edge cases like missing contacts, invalid state IDs, etc.
| $organization = Organization::get(FALSE) | ||
| ->addSelect('Institute_Registration.Type_of_Institution:label') | ||
| ->addWhere('id', '=', $contactId) | ||
| ->execute()->single(); |
There was a problem hiding this comment.
Add error handling for organization retrieval
Similar to the contact retrieval, the single() method needs error handling to prevent exceptions.
- $organization = Organization::get(FALSE)
- ->addSelect('Institute_Registration.Type_of_Institution:label')
- ->addWhere('id', '=', $contactId)
- ->execute()->single();
+ try {
+ $organization = Organization::get(FALSE)
+ ->addSelect('Institute_Registration.Type_of_Institution:label')
+ ->addWhere('id', '=', $contactId)
+ ->execute()->first();
+
+ if (!$organization) {
+ \CRM_Core_Error::debug_log_message(sprintf(
+ 'Organization not found for contact ID: %d while getting relationship type',
+ $contactId
+ ));
+ return 'Default Coordinator of';
+ }
+ } catch (\Exception $e) {
+ \CRM_Core_Error::debug_log_message(sprintf(
+ 'Error retrieving organization (Contact ID: %d): %s',
+ $contactId,
+ $e->getMessage()
+ ));
+ return 'Default Coordinator of';
+ }📝 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.
| $organization = Organization::get(FALSE) | |
| ->addSelect('Institute_Registration.Type_of_Institution:label') | |
| ->addWhere('id', '=', $contactId) | |
| ->execute()->single(); | |
| try { | |
| $organization = Organization::get(FALSE) | |
| ->addSelect('Institute_Registration.Type_of_Institution:label') | |
| ->addWhere('id', '=', $contactId) | |
| ->execute()->first(); | |
| if (!$organization) { | |
| \CRM_Core_Error::debug_log_message(sprintf( | |
| 'Organization not found for contact ID: %d while getting relationship type', | |
| $contactId | |
| )); | |
| return 'Default Coordinator of'; | |
| } | |
| } catch (\Exception $e) { | |
| \CRM_Core_Error::debug_log_message(sprintf( | |
| 'Error retrieving organization (Contact ID: %d): %s', | |
| $contactId, | |
| $e->getMessage() | |
| )); | |
| return 'Default Coordinator of'; | |
| } |
| $contacts = Contact::get(FALSE) | ||
| ->addSelect('contact_sub_type') | ||
| ->addWhere('id', '=', $contactId) | ||
| ->execute()->single(); |
There was a problem hiding this comment.
Add error handling for contact retrieval
The single() method will throw an exception if no contact is found. Consider using first() with proper error handling.
- $contacts = Contact::get(FALSE)
- ->addSelect('contact_sub_type')
- ->addWhere('id', '=', $contactId)
- ->execute()->single();
+ try {
+ $contact = Contact::get(FALSE)
+ ->addSelect('contact_sub_type')
+ ->addWhere('id', '=', $contactId)
+ ->execute()->first();
+
+ if (!$contact) {
+ \CRM_Core_Error::debug_log_message(sprintf(
+ 'Contact not found for ID: %d while checking contact subtype',
+ $contactId
+ ));
+ return;
+ }
+ } catch (\Exception $e) {
+ \CRM_Core_Error::debug_log_message(sprintf(
+ 'Error retrieving contact (ID: %d): %s',
+ $contactId,
+ $e->getMessage()
+ ));
+ return;
+ }📝 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.
| $contacts = Contact::get(FALSE) | |
| ->addSelect('contact_sub_type') | |
| ->addWhere('id', '=', $contactId) | |
| ->execute()->single(); | |
| try { | |
| $contact = Contact::get(FALSE) | |
| ->addSelect('contact_sub_type') | |
| ->addWhere('id', '=', $contactId) | |
| ->execute()->first(); | |
| if (!$contact) { | |
| \CRM_Core_Error::debug_log_message(sprintf( | |
| 'Contact not found for ID: %d while checking contact subtype', | |
| $contactId | |
| )); | |
| return; | |
| } | |
| } catch (\Exception $e) { | |
| \CRM_Core_Error::debug_log_message(sprintf( | |
| 'Error retrieving contact (ID: %d): %s', | |
| $contactId, | |
| $e->getMessage() | |
| )); | |
| return; | |
| } |
|
Closing, This approach no longer needed. |
3 participants
Summary by CodeRabbit