Institution : Set Office Details#481
Conversation
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@belwalshubham has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 33 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. 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 (7)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (3)
48-50: Add PHPDoc documentation for the new method.Please add comprehensive PHPDoc documentation including:
- Method description
- @param annotation with type and description
- @return annotation with type and description
/** - * + * Retrieves the contact subtype name for a given entity ID. + * + * @param int $entityID The ID of the organization entity + * @return string|null The contact subtype name or null if not found */
48-65: Consider abstracting common API call patterns.The trait has multiple methods making similar API calls to fetch subtype information. Consider creating a private helper method to handle the common API call pattern.
Example abstraction:
/** * Makes a standardized API call to fetch entity information. * * @param string $entity The API entity name * @param string $field The field to select * @param int $id The entity ID * @return array|null The API result */ private static function fetchEntityField($entity, $field, $id) { $result = civicrm_api4($entity, 'get', [ 'select' => [$field], 'where' => [['id', '=', $id]], 'checkPermissions' => FALSE, ]); return $result[0][$field] ?? NULL; }This would allow simplifying both
getContactSubtypeNameandgetEntitySubtypeNamemethods.
Line range hint
1-66: Consider reviewing the trait's design for testability and responsibility separation.The current implementation using static methods and properties makes unit testing more challenging and creates global state. Consider:
- Moving towards instance methods for better testability
- Separating contact-specific and entity-specific subtype handling into different traits
This would allow for better dependency injection and easier testing. For example:
trait ContactSubtypeSource { protected $api; public function setApi($api) { $this->api = $api; } public function getContactSubtypeName($entityID) { // Implementation using instance methods } }wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (4)
34-56: Improve parameter naming and code clarity infindStateFieldmethodThe parameter
$arrayin thefindStateFieldmethod is not descriptive. To enhance code readability and maintain coding standards, consider renaming it to something more indicative of its purpose, such as$paramsor$customFieldsArray.
141-147: Consolidate multipleOrganization::updatecalls to reduce code duplicationThere are two consecutive
Organization::updatecalls that update the same organization with different values:Organization::update('Organization', FALSE) ->addValue('Review.Status', 1) ->addValue('Review.Initiated_by', 1) ->addWhere('id', '=', $contactId) ->execute(); Organization::update('Organization', FALSE) ->addValue('Review.Coordinating_POC', $coordinatorId) ->addWhere('id', '=', $contactId) ->execute();To improve efficiency and adhere to the DRY (Don't Repeat Yourself) principle, consider combining these into a single update call.
Apply this diff to refactor the code:
Organization::update('Organization', FALSE) ->addValue('Review.Status', 1) ->addValue('Review.Initiated_by', 1) + ->addValue('Review.Coordinating_POC', $coordinatorId) ->addWhere('id', '=', $contactId) ->execute(); - -Organization::update('Organization', FALSE) - ->addValue('Review.Coordinating_POC', $coordinatorId) - ->addWhere('id', '=', $contactId) - ->execute();
204-211: Ensure consistent return values ingetRelationshipTypeNamemethodIn the
getRelationshipTypeNamemethod, if$organizationis not found, the function returns without a value:if (!$organization) { return; }For clarity and to maintain consistent return types, consider explicitly returning
NULLto indicate that no relationship type name is available.Apply this diff:
if (!$organization) { - return; + return NULL; }
Line range hint
70-146: RefactorsetOfficeDetailsmethod to adhere to Single Responsibility PrincipleThe
setOfficeDetailsmethod handles multiple responsibilities, including validating parameters, fetching data, and updating records. This can make the code harder to read and maintain. To improve code quality, consider refactoring this method into smaller, focused methods that each handle a single responsibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(4 hunks)wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php(1 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (1)
51-65: Verify usage and impact of the new method.
Let's verify where this new method is being used and ensure it's properly integrated.
✅ Verification successful
The new method is properly integrated and used correctly
The getContactSubtypeName method is:
- Correctly implemented in the
CollectionSourcetrait - Properly imported and used in
InstitutionServiceclass - Used appropriately to check for institute subtype (
ENTITY_SUBTYPE_NAME = 'Institute') in a guard clause - Has no breaking changes as it's the only usage of the Organization API endpoint in this context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of getContactSubtypeName
rg "getContactSubtypeName" --type php
# Look for potential breaking changes in Organization API usage
rg "civicrm_api4\('Organization'" --type php
Length of output: 521
Script:
#!/bin/bash
# Let's check the context of usage in InstitutionService.php
rg -B 5 -A 5 "getContactSubtypeName" wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php
# Check if ENTITY_SUBTYPE_NAME is defined in the same class
rg "ENTITY_SUBTYPE_NAME" wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php
# Check if InstitutionService uses CollectionSource trait
ast-grep --pattern 'class InstitutionService {
$$$
}'
Length of output: 10042
Script:
#!/bin/bash
# Let's check if InstitutionService uses the CollectionSource trait with a different pattern
rg -B 2 -A 2 "class InstitutionService" wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php
# Check for use statements of CollectionSource trait
rg "use.*CollectionSource" wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php
Length of output: 383
| public static function getContactSubtypeName($entityID) { | ||
| $getSubtypeName = civicrm_api4('Organization', 'get', [ | ||
| 'select' => [ | ||
| 'contact_sub_type', | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $entityID], | ||
| ], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
|
|
||
| $entityData = $getSubtypeName[0]['contact_sub_type'] ?? []; | ||
|
|
||
| return $entityData[0] ?? NULL; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and reduce code duplication.
The method has several areas for improvement:
- No input validation for $entityID
- Unsafe array access could trigger notices
- Similar code pattern to getEntitySubtypeName suggests potential for abstraction
Consider applying these improvements:
public static function getContactSubtypeName($entityID) {
+ if (!is_numeric($entityID)) {
+ throw new \InvalidArgumentException('Entity ID must be numeric');
+ }
+
$getSubtypeName = civicrm_api4('Organization', 'get', [
'select' => [
'contact_sub_type',
],
'where' => [
['id', '=', $entityID],
],
'checkPermissions' => FALSE,
]);
- $entityData = $getSubtypeName[0]['contact_sub_type'] ?? [];
-
- return $entityData[0] ?? NULL;
+ return !empty($getSubtypeName[0]['contact_sub_type'][0])
+ ? $getSubtypeName[0]['contact_sub_type'][0]
+ : NULL;
}📝 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 getContactSubtypeName($entityID) { | |
| $getSubtypeName = civicrm_api4('Organization', 'get', [ | |
| 'select' => [ | |
| 'contact_sub_type', | |
| ], | |
| 'where' => [ | |
| ['id', '=', $entityID], | |
| ], | |
| 'checkPermissions' => FALSE, | |
| ]); | |
| $entityData = $getSubtypeName[0]['contact_sub_type'] ?? []; | |
| return $entityData[0] ?? NULL; | |
| } | |
| public static function getContactSubtypeName($entityID) { | |
| if (!is_numeric($entityID)) { | |
| throw new \InvalidArgumentException('Entity ID must be numeric'); | |
| } | |
| $getSubtypeName = civicrm_api4('Organization', 'get', [ | |
| 'select' => [ | |
| 'contact_sub_type', | |
| ], | |
| 'where' => [ | |
| ['id', '=', $entityID], | |
| ], | |
| 'checkPermissions' => FALSE, | |
| ]); | |
| return !empty($getSubtypeName[0]['contact_sub_type'][0]) | |
| ? $getSubtypeName[0]['contact_sub_type'][0] | |
| : NULL; | |
| } |
| $stateId = $stateField['value']; | ||
| $contactId = $stateField['entity_id']; | ||
|
|
||
| if (!$stateId) { |
There was a problem hiding this comment.
Remove redundant check for $stateId to eliminate unreachable code
There is a redundant check for $stateId:
if (!$stateId) {
return;
}
if (!$stateId) {
\CRM_Core_Error::debug_log_message('Cannot assign Goonj Office to institution id: ' . $contactId);
return;
}The first if (!$stateId) block returns early, making the subsequent check unreachable. To adhere to best practices and avoid confusion, consider combining these checks and logging the error before returning.
Apply this diff to fix the issue:
- if (!$stateId) {
- return;
- }
-
- if (!$stateId) {
+ if (!$stateId) {
\CRM_Core_Error::debug_log_message('Cannot assign Goonj Office to institution id: ' . $contactId);
return;
}
| Organization::update('Organization', FALSE) | ||
| ->addValue('Review.Coordinating_POC', $coordinatorId) | ||
| ->addWhere('id', '=', $contactId) | ||
| ->execute(); |
There was a problem hiding this comment.
why using two diffrent query to update values of single contact of type organization ?
There was a problem hiding this comment.
I’ve added another query because there’s an issue with the form where hidden default values are not being saved. To work around this, I’m updating it through the query.
There was a problem hiding this comment.
but why you are not updating all together
Organization::update('Organization', FALSE)
->addValue('Review.Status', 1)
->addValue('Review.Initiated_by', 1)
->addWhere('id', '=', $contactId)
->execute();
| ->addSelect('Institute_Registration.Type_of_Institution:label') | ||
| ->addWhere('id', '=', $contactId) |
| $getSubtypeName = civicrm_api4('Organization', 'get', [ | ||
| 'select' => [ | ||
| 'contact_sub_type', | ||
| ], | ||
| 'where' => [ | ||
| ['id', '=', $entityID], | ||
| ], | ||
| 'checkPermissions' => FALSE, | ||
| ]); | ||
|
|
||
| $entityData = $getSubtypeName[0]['contact_sub_type'] ?? []; | ||
|
|
||
| return $entityData[0] ?? NULL; | ||
| } |
There was a problem hiding this comment.
dont use tradional query
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (2)
31-56: Add PHPDoc and type hints to improve code clarityThe new
findStateFieldmethod would benefit from proper documentation and type hints.- private static function findStateField(array $array) { + /** + * Finds the state field in the given array of custom fields. + * + * @param array $array Array of custom fields + * @return array|false Returns field data if found, FALSE otherwise + */ + private static function findStateField(array $array): array|false {
204-212: Enhance error handling in getRelationshipTypeNameWhile the null check is good, consider:
- Adding type hints
- Using early return pattern
- Adding logging for debugging
- private static function getRelationshipTypeName($contactId) { + private static function getRelationshipTypeName(int $contactId): ?string { $organization = Organization::get(FALSE) ->addSelect('Institute_Registration.Type_of_Institution:label') ->addWhere('id', '=', $contactId) ->execute()->single(); if (!$organization) { + \CRM_Core_Error::debug_log_message("Organization not found for contact ID: $contactId"); return NULL; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(4 hunks)wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php
🔇 Additional comments (4)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (4)
6-6: LGTM: Clean trait integration and imports
The addition of the CustomField API import and CollectionSource trait integration follows PSR standards and improves code organization.
Also applies to: 10-10, 16-16
18-18: LGTM: Well-defined constant
Good practice to define 'Institute' as a constant, making it easier to maintain and reuse.
Line range hint 89-93: Redundant state ID checks still present
This is a duplicate of a previous review comment. The redundant checks should be combined.
25-28: Verify the hook timing change impact
The switch from hook_civicrm_post to hook_civicrm_custom changes when the office details are set. Please ensure this doesn't affect the order of operations or cause any race conditions.
✅ Verification successful
Let me analyze the hook timing change impact based on the search results.
Hook timing change is appropriate and safe
The switch from hook_civicrm_post to hook_civicrm_custom for setting office details is actually more appropriate because:
hook_civicrm_customis specifically designed to handle custom field changes, which aligns better with setting office details- The codebase shows other services like
DroppingCenterServiceandCollectionCampServicealso usinghook_civicrm_customforsetOfficeDetailsmethod, indicating this is the established pattern - There are no race conditions evident since:
hook_civicrm_customfires when custom fields are saved- The office details are derived from custom fields
- No other critical dependencies rely on this hook timing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of these hooks to understand the execution order
rg -A 3 "hook_civicrm_(custom|post)" --type php
Length of output: 55411
| Organization::update('Organization', FALSE) | ||
| ->addValue('Review.Status', 1) | ||
| ->addValue('Review.Initiated_by', 1) | ||
| ->addWhere('id', '=', $contactId) | ||
| ->execute(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap multiple database operations in a transaction
Multiple update operations should be wrapped in a transaction to ensure data consistency.
+ try {
+ \CRM_Core_Transaction::create()->run(function() use ($contactId, $coordinatorId) {
Organization::update('Organization', FALSE)
->addValue('Review.Status', 1)
->addValue('Review.Initiated_by', 1)
->addWhere('id', '=', $contactId)
->execute();
Organization::update('Organization', FALSE)
->addValue('Review.Coordinating_POC', $coordinatorId)
->addWhere('id', '=', $contactId)
->execute();
+ });
+ } catch (\Exception $e) {
+ \CRM_Core_Error::debug_log_message('Failed to update organization: ' . $e->getMessage());
+ return FALSE;
+ }Also applies to: 147-150
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (2)
13-15: Add comprehensive class documentationThe class lacks proper PHPDoc documentation. Consider adding a detailed description of the class's purpose, responsibilities, and usage.
Add documentation like this:
/** - * + * Service class for handling institution-related operations. + * + * This class manages the assignment of office details and coordinates + * relationships between institutions and Goonj offices. */
31-34: Add method documentationThe
findStateFieldmethod lacks PHPDoc documentation. Add parameter and return type information.Add documentation like this:
/** - * - */ + * Finds the state field in the given array of custom fields. + * + * @param array $array The array of custom fields to search + * @return array|false Returns the field data or FALSE if not found + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(4 hunks)wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (2)
88-92:
Combine organization updates and wrap in transaction
There are two separate organization updates that should be:
- Combined into a single update operation
- Wrapped in a transaction for data consistency
Apply this diff:
+ try {
+ \CRM_Core_Transaction::create()->run(function() use ($contactId, $coordinatorId) {
Organization::update('Organization', FALSE)
->addValue('Review.Status', 1)
->addValue('Review.Initiated_by', 1)
+ ->addValue('Review.Coordinating_POC', $coordinatorId)
->addWhere('id', '=', $contactId)
->execute();
-
- Organization::update('Organization', FALSE)
- ->addValue('Review.Coordinating_POC', $coordinatorId)
- ->addWhere('id', '=', $contactId)
- ->execute();
+ });
+ } catch (\Exception $e) {
+ \CRM_Core_Error::debug_log_message('Failed to update organization: ' . $e->getMessage());
+ return FALSE;
+ }Also applies to: 148-151
25-28: Verify the hook change impact
The switch from hook_civicrm_post to hook_civicrm_custom is a significant change that could affect when the office details are set.
✅ Verification successful
Let me gather more information about the setOfficeDetails method to understand the hook change impact better.
Hook change from post to custom is appropriate for this use case
The change from hook_civicrm_post to hook_civicrm_custom for the setOfficeDetails method is actually correct because:
- The
setOfficeDetailsmethod's implementation shows it specifically checks for$op === 'create'and handles custom field updates related to state/office details - Other services (DroppingCenterService, CollectionCampService) use the same pattern with
hook_civicrm_customfor similar office details functionality hook_civicrm_customis more appropriate as it's specifically designed for handling custom field updates, which is what this method does
The change aligns the hook usage with its actual purpose and matches the pattern used across similar services in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of these hooks to ensure no functionality overlap
rg "hook_civicrm_post|hook_civicrm_custom" --type php
Length of output: 16871
Script:
#!/bin/bash
# Search for setOfficeDetails method implementation
ast-grep --pattern 'function setOfficeDetails($$$)'
# Also search for any references to this method
rg "setOfficeDetails" -A 5
Length of output: 14349
| if (!$organization) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return default relationship type when organization not found
Instead of returning undefined, return the default coordinator relationship type for consistency.
Apply this diff:
if (!$organization) {
- return;
+ 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.
| if (!$organization) { | |
| return; | |
| } | |
| if (!$organization) { | |
| return 'Default Coordinator of'; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (1)
49-51: Add proper PHPDoc documentationThe method is missing proper documentation. Please add PHPDoc with:
- Method description
- @param annotation with type and description
- @return annotation with type and description
/** - * + * Retrieves the contact subtype name for a given organization entity. + * + * @param int $entityID The ID of the organization entity + * @return string|null The contact subtype name or null if not found */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php(2 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php (1)
6-6: LGTM: Proper namespace import
The Organization class import follows PSR-4 standards and is appropriately used.
| public static function getContactSubtypeName($entityID) { | ||
| $getSubtypeName = Organization::get(FALSE) | ||
| ->addSelect('contact_sub_type') | ||
| ->addWhere('id', '=', $entityID) | ||
| ->execute()->single(); | ||
|
|
||
| $entityData = $getSubtypeName['contact_sub_type'] ?? []; | ||
| return $entityData[0] ?? NULL; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and reduce code duplication
The method has several areas for improvement:
- Missing input validation
- Potential null pointer issues
- Similar pattern to getEntitySubtypeName suggests potential for abstraction
Consider applying these improvements:
public static function getContactSubtypeName($entityID) {
+ if (!is_numeric($entityID)) {
+ throw new \InvalidArgumentException('Entity ID must be numeric');
+ }
+
$getSubtypeName = Organization::get(FALSE)
->addSelect('contact_sub_type')
->addWhere('id', '=', $entityID)
->execute()->single();
- $entityData = $getSubtypeName['contact_sub_type'] ?? [];
- return $entityData[0] ?? NULL;
+ return !empty($getSubtypeName['contact_sub_type'][0])
+ ? $getSubtypeName['contact_sub_type'][0]
+ : NULL;
}Additionally, consider creating a base method to handle the common pattern between this and getEntitySubtypeName:
private static function getSubtypeNameFromEntity($entityID, $entityType, $field) {
if (!is_numeric($entityID)) {
throw new \InvalidArgumentException('Entity ID must be numeric');
}
$result = $entityType::get(FALSE)
->addSelect($field)
->addWhere('id', '=', $entityID)
->execute()->single();
return !empty($result[$field][0]) ? $result[$field][0] : NULL;
}
| Organization::update('Organization', FALSE) | ||
| ->addValue('Review.Coordinating_POC', $coordinatorId) | ||
| ->addWhere('id', '=', $contactId) | ||
| ->execute(); |
There was a problem hiding this comment.
but why you are not updating all together
Organization::update('Organization', FALSE)
->addValue('Review.Status', 1)
->addValue('Review.Initiated_by', 1)
->addWhere('id', '=', $contactId)
->execute();
|
@nishant22029 just replying here regarding the previous comment. We aren't updating together because there might be a situation where there's no coordinating POC assigned to that institute. In such cases, the review status and the 'initiated by' fields are not updated. These should be updated immediately after fetching the contact ID. Let me know if you have any questions." |
2 participants
Summary by CodeRabbit
New Features
InstitutionServiceclass with improved event handling and data processing logic.Bug Fixes
Documentation