Skip to content

Set Organization Name for collection camp#494

Closed
belwalshubham wants to merge 10 commits intodevelopfrom
feat-update-organization-for-collection-camp
Closed

Set Organization Name for collection camp#494
belwalshubham wants to merge 10 commits intodevelopfrom
feat-update-organization-for-collection-camp

Conversation

@belwalshubham
Copy link
Copy Markdown
Collaborator

@belwalshubham belwalshubham commented Nov 25, 2024

PR Description:

Once the organization is mapped to the collection camp during the review of the intent details, and after it's authorized, users should be able to see the organization directly on the collection camp list. This will allow them to click on the organization name and be redirected straight to its details.

image

Summary by CodeRabbit

  • New Features
    • Introduced a method to update the organization name for collection camps based on specific conditions.
    • Enhanced event subscription to trigger the new functionality during the CiviCRM post-processing phase.
    • Added a mechanism to validate and store organization IDs for improved data integrity.
    • Implemented error handling for missing organization IDs during updates.

@belwalshubham belwalshubham self-assigned this Nov 25, 2024
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Nov 25, 2024

Walkthrough

The changes introduce a new private static variable $organizationId in the InstitutionCollectionCampService class to hold the organization's ID. A public static method setOrganizationNameForCollectionCamp is added to validate the afform_name in the objectRef and update the Institution_Name field in the Collection_Camp entity. Additionally, the getSubscribedEvents method is modified to link the new method to the &hook_civicrm_post event, while retaining the previous &hook_civicrm_pre event.

Changes

File Path Change Summary
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php - Added method setOrganizationNameForCollectionCamp for validating and updating organization names.
- Updated getSubscribedEvents to link setOrganizationNameForCollectionCamp with &hook_civicrm_post and retain &hook_civicrm_pre for another method.

Possibly related PRs

Suggested reviewers

  • tarunnjoshi

🎉 In the code's dance, a new step we see,
Setting names for camps, as easy as can be!
With hooks that trigger, in post-processing flow,
The organization shines, in the data's bright glow.
So here’s to the change, with a cheer and a clap,
For InstitutionCollectionCampService, a well-deserved tap! 🎊


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between f45efed and edd1fe5.

📒 Files selected for processing (1)
  • wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)

38-41: Add comprehensive PHPDoc block

The method lacks proper documentation. Add a PHPDoc block describing parameters, return type, and purpose.

 /**
-  *
+  * Updates the organization name for a collection camp after intent verification
+  * 
+  * @param string $op Operation being performed
+  * @param string $objectName Name of the object
+  * @param int $objectId ID of the object
+  * @param mixed $objectRef Reference to the object
+  * @return void
   */

43-43: Extract form name to class constant

The form name is hardcoded as a magic string. Consider extracting it to a class constant for better maintainability.

+    const INTENT_VERIFICATION_FORM = 'afformInstitutionCollectionCampIntentVerification';
+
-    if (empty($objectRef->afform_name) || $objectRef->afform_name !== 'afformInstitutionCollectionCampIntentVerification') {
+    if (empty($objectRef->afform_name) || $objectRef->afform_name !== self::INTENT_VERIFICATION_FORM) {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8248a5e and 087a552.

📒 Files selected for processing (2)
  • wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1 hunks)
  • wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (1 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (1)

71-86: Verify alignment with PR objectives

The PR objectives mention setting organization name for collection camps, but these changes only add debug logging to institution office details processing. Please clarify how these changes contribute to the stated PR objective.

wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)

32-34: LGTM: Hook registration follows CiviCRM conventions

The hook registration is properly structured and consistent with CiviCRM's event subscription patterns.


57-61: Verify organization name display in UI

The implementation correctly updates the organization name, but we should verify that the UI components properly display this information.

✅ Verification successful

UI display of Institution_Name is properly handled

Based on the codebase analysis, the Institution_Name field is correctly integrated into the Collection_Camp entity and its display is handled through:

  1. The CRM/Goonjcustom/Tabs/CollectionCampService.tpl template which renders the field using Angular directives
  2. The field is properly updated through EckEntity in InstitutionCollectionCampService.php
  3. The value is accessible through standard CiviCRM entity APIs and templates

The implementation aligns with CiviCRM's entity display patterns and no additional UI changes are needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for templates or forms that should display the Institution_Name field
rg -l "Institution_Name|organization.*name" --type php --type html wp-content/

Length of output: 5588


Script:

#!/bin/bash
# Let's check the actual implementation of templates/forms in goonjcustom extension
# that might be displaying the Institution_Name field

# First, let's check for any templates or forms related to Collection_Camp
rg -g "*.{php,html,tpl}" "Collection_Camp.*Institution_Name" wp-content/civi-extensions/goonjcustom/ -A 5

# Also check for any UI components or templates that render institution data
rg -g "*.{php,html,tpl}" "Institution.*display|render.*Institution" wp-content/civi-extensions/goonjcustom/ -A 5

Length of output: 222


Script:

#!/bin/bash
# Let's check how the Collection_Camp entity is used in the UI layer
rg -g "*.{php,html,tpl}" "Collection_Camp" wp-content/civi-extensions/goonjcustom/ -A 5

# Also check for any EckEntity usage patterns that might show how fields are displayed
rg -g "*.php" "EckEntity::.*display|EckEntity::.*view" wp-content/civi-extensions/goonjcustom/ -A 5

# Check for any custom templates or forms related to institutions
fd -e tpl -e html . wp-content/civi-extensions/goonjcustom/templates/

Length of output: 120583

Comment on lines +84 to +86
error_log("stateId: " . print_r($stateId, TRUE));
error_log("contactId: " . print_r($contactId, TRUE));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect log placement and undefined variables

The logging statements are placed before the variables are assigned, which will result in logging undefined values.

Move the logging after the variable assignments:

-error_log("stateId: " . print_r($stateId, TRUE));
-error_log("contactId: " . print_r($contactId, TRUE));

$stateId = $stateField['value'];
$contactId = $stateField['entity_id'];

+\Civi::log()->info('State and contact details', [
+  'state_id' => $stateId,
+  'contact_id' => $contactId
+]);

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +76 to +78
error_log("entityID: " . print_r($entityID, TRUE));
error_log("groupID: " . print_r($groupID, TRUE));
error_log("params: " . print_r($params, TRUE));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consolidate logs and handle sensitive data

Multiple separate log calls can lead to interleaved logs in high-concurrency scenarios. Also, raw parameter logging might expose sensitive information.

-error_log("entityID: " . print_r($entityID, TRUE));
-error_log("groupID: " . print_r($groupID, TRUE));
-error_log("params: " . print_r($params, TRUE));
+\Civi::log()->info('Processing office details update', [
+  'entity_id' => $entityID,
+  'group_id' => $groupID,
+  'params' => self::sanitizeParams($params)  // Create a method to sanitize sensitive data
+]);

Committable suggestion skipped: line range outside the PR's diff.

* The parameters that were sent into the calling function.
*/
public static function setOfficeDetails($op, $groupID, $entityID, &$params) {
error_log("updateOrganization: " . print_r(self::getOrgSubtypeName($entityID), TRUE));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider using proper logging levels and context

Using error_log for debugging purposes is not recommended. Consider using CiviCRM's logging system with appropriate log levels and contextual information.

-error_log("updateOrganization: " . print_r(self::getOrgSubtypeName($entityID), TRUE));
+\Civi::log()->debug('Processing organization update', [
+  'organization_subtype' => self::getOrgSubtypeName($entityID),
+  'method' => 'setOfficeDetails'
+]);
📝 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.

Suggested change
error_log("updateOrganization: " . print_r(self::getOrgSubtypeName($entityID), TRUE));
\Civi::log()->debug('Processing organization update', [
'organization_subtype' => self::getOrgSubtypeName($entityID),
'method' => 'setOfficeDetails'
]);

Comment on lines +47 to +48
$data = json_decode($objectRef->data, TRUE);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add JSON decode error handling

The JSON decode operation lacks error handling, which could lead to silent failures.

-    $data = json_decode($objectRef->data, TRUE);
+    $data = json_decode($objectRef->data, TRUE);
+    if (json_last_error() !== JSON_ERROR_NONE) {
+      \CRM_Core_Error::debug_log_message('Failed to decode JSON data for collection camp: ' . json_last_error_msg());
+      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.

Suggested change
$data = json_decode($objectRef->data, TRUE);
$data = json_decode($objectRef->data, TRUE);
if (json_last_error() !== JSON_ERROR_NONE) {
\CRM_Core_Error::debug_log_message('Failed to decode JSON data for collection camp: ' . json_last_error_msg());
return;
}

Comment on lines +49 to +55
// Retrieve organizationId and entityId, return if not set.
$organizationId = $data['Organization1'][0]['fields']['id'] ?? NULL;
$entityId = $data['Eck_Collection_Camp1'][0]['fields']['id'] ?? NULL;

if (!$organizationId || !$entityId) {
return;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance data validation and error handling

The code could benefit from more robust validation and logging:

  1. Validate organization existence
  2. Add type checking for array access
  3. Log validation failures
-    $organizationId = $data['Organization1'][0]['fields']['id'] ?? NULL;
-    $entityId = $data['Eck_Collection_Camp1'][0]['fields']['id'] ?? NULL;
-
-    if (!$organizationId || !$entityId) {
-      return;
-    }
+    // Validate data structure
+    if (!isset($data['Organization1'][0]['fields']['id']) || 
+        !isset($data['Eck_Collection_Camp1'][0]['fields']['id'])) {
+      \CRM_Core_Error::debug_log_message('Missing required fields in collection camp data');
+      return;
+    }
+
+    $organizationId = (int) $data['Organization1'][0]['fields']['id'];
+    $entityId = (int) $data['Eck_Collection_Camp1'][0]['fields']['id'];
+
+    // Verify organization exists
+    $orgExists = \Civi\Api4\Contact::get(FALSE)
+      ->selectRowCount()
+      ->addWhere('id', '=', $organizationId)
+      ->addWhere('contact_type', '=', 'Organization')
+      ->execute()
+      ->count();
+
+    if (!$orgExists) {
+      \CRM_Core_Error::debug_log_message("Organization not found: $organizationId");
+      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.

Suggested change
// Retrieve organizationId and entityId, return if not set.
$organizationId = $data['Organization1'][0]['fields']['id'] ?? NULL;
$entityId = $data['Eck_Collection_Camp1'][0]['fields']['id'] ?? NULL;
if (!$organizationId || !$entityId) {
return;
}
// Validate data structure
if (!isset($data['Organization1'][0]['fields']['id']) ||
!isset($data['Eck_Collection_Camp1'][0]['fields']['id'])) {
\CRM_Core_Error::debug_log_message('Missing required fields in collection camp data');
return;
}
$organizationId = (int) $data['Organization1'][0]['fields']['id'];
$entityId = (int) $data['Eck_Collection_Camp1'][0]['fields']['id'];
// Verify organization exists
$orgExists = \Civi\Api4\Contact::get(FALSE)
->selectRowCount()
->addWhere('id', '=', $organizationId)
->addWhere('contact_type', '=', 'Organization')
->execute()
->count();
if (!$orgExists) {
\CRM_Core_Error::debug_log_message("Organization not found: $organizationId");
return;
}

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)

36-39: Add PHPDoc documentation for the method

The method lacks PHPDoc documentation. Please add documentation describing:

  • Purpose of the method
  • Parameters and their types
  • Return type
  • Any exceptions that might be thrown

Example:

/**
 * Updates the organization name for a collection camp during intent verification.
 *
 * @param string $op The operation being performed
 * @param string $objectName The name of the object
 * @param int $objectId The ID of the object
 * @param mixed $objectRef Reference to the object being modified
 * @return void
 */

55-58: Add logging for important operations

Consider adding logging for successful updates to help with debugging and monitoring.

     EckEntity::update('Collection_Camp', TRUE)
       ->addValue('Institution_collection_camp_Review.Institution_Name', $organizationId)
       ->addWhere('id', '=', $entityId)
-      ->execute();
+      ->execute();
+    \CRM_Core_Error::debug_log_message(
+      "Updated organization name (ID: $organizationId) for collection camp (ID: $entityId)"
+    );
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a8abefa and b6a53b0.

📒 Files selected for processing (1)
  • wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1 hunks)
🔇 Additional comments (4)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (4)

32-32: LGTM! Hook registration is appropriate

The post-hook registration is well-placed for updating the organization name after the main operation completes.


45-46: Add JSON decode error handling

The previous review comment about JSON decode error handling is still valid. Please implement the suggested error handling.


47-53: Enhance data validation and error handling

The previous review comment about data validation and error handling is still valid. Please implement the suggested improvements for robust validation.


39-59: Verify UI implementation for displaying organization name

The code successfully updates the organization reference, but we should verify if the UI components exist to display this information in the collection camp list.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1)

69-69: Remove or update incorrect comment

The comment // Use self::$test (Organization ID) appears to be incorrect or outdated as it references a non-existent variable.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b6a53b0 and 7edfbba.

📒 Files selected for processing (1)
  • wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)

31-34: Verify event execution order

The order of event handlers might be important here. Ensure that setOrganizationId is executed before setOrganizationNameForCollectionCamp as the latter depends on the organization ID being set.


23-23: Consider thread-safety implications of static state

The static $organizationId variable could lead to race conditions in concurrent scenarios. Consider using instance-based state management or a more thread-safe approach.

Comment on lines +60 to +66
// Retrieve the collection camp entity ID.
$entityId = $objectRef['data']['Eck_Collection_Camp1'][0]['fields']['id'] ?? NULL;

if (!$entityId) {
\Civi::log()->error('Collection Camp ID is not set', ['entityId' => $entityId]);
return;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve data structure validation

The deep array access needs more robust validation to prevent potential errors.

-    $entityId = $objectRef['data']['Eck_Collection_Camp1'][0]['fields']['id'] ?? NULL;
-
-    if (!$entityId) {
-      \Civi::log()->error('Collection Camp ID is not set', ['entityId' => $entityId]);
-      return;
-    }
+    // Validate data structure
+    if (!isset($objectRef['data']) || 
+        !isset($objectRef['data']['Eck_Collection_Camp1']) ||
+        !is_array($objectRef['data']['Eck_Collection_Camp1']) ||
+        empty($objectRef['data']['Eck_Collection_Camp1'][0]['fields']['id'])) {
+      \Civi::log()->error('Invalid data structure for Collection Camp', [
+        'data' => $objectRef['data'] ?? NULL
+      ]);
+      return;
+    }
+    
+    $entityId = (int) $objectRef['data']['Eck_Collection_Camp1'][0]['fields']['id'];
📝 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.

Suggested change
// Retrieve the collection camp entity ID.
$entityId = $objectRef['data']['Eck_Collection_Camp1'][0]['fields']['id'] ?? NULL;
if (!$entityId) {
\Civi::log()->error('Collection Camp ID is not set', ['entityId' => $entityId]);
return;
}
// Validate data structure
if (!isset($objectRef['data']) ||
!isset($objectRef['data']['Eck_Collection_Camp1']) ||
!is_array($objectRef['data']['Eck_Collection_Camp1']) ||
empty($objectRef['data']['Eck_Collection_Camp1'][0]['fields']['id'])) {
\Civi::log()->error('Invalid data structure for Collection Camp', [
'data' => $objectRef['data'] ?? NULL
]);
return;
}
$entityId = (int) $objectRef['data']['Eck_Collection_Camp1'][0]['fields']['id'];

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (3)

23-23: Consider alternative state management approaches

Using a static variable for $organizationId could lead to state management issues in concurrent environments and make testing more difficult. Consider using dependency injection or a service class to manage this state.


72-73: Fix incorrect code comment

The comment refers to self::$test but should refer to self::$organizationId.

-    // Use self::$test (Organization ID)
+    // Use organization ID from static variable

56-76: Consider transaction management for database operations

The database update operation should be wrapped in a transaction to ensure data consistency, especially since this involves updating entity relationships.

 public static function setOrganizationNameForCollectionCamp(string $op, string $objectName, $objectId, &$objectRef) {
   if (!self::$organizationId) {
     \Civi::log()->error('Organization ID is not set', ['organizationId' => self::$organizationId]);
     return;
   }

   $entityId = $objectRef['data']['Eck_Collection_Camp1'][0]['fields']['id'] ?? NULL;

   if (!$entityId) {
     \Civi::log()->error('Collection Camp ID is not set', ['entityId' => $entityId]);
     return;
   }

+  // Start transaction
+  $transaction = new \CRM_Core_Transaction();
+  try {
     EckEntity::update('Collection_Camp', TRUE)
       ->addValue('Institution_collection_camp_Review.Institution_Name', self::$organizationId)
       ->addWhere('id', '=', $entityId)
       ->execute();
+    $transaction->commit();
+  }
+  catch (\Exception $e) {
+    $transaction->rollback();
+    \Civi::log()->error('Failed to update Collection Camp organization name', [
+      'error' => $e->getMessage(),
+      'entityId' => $entityId,
+      'organizationId' => self::$organizationId
+    ]);
+  }
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 7edfbba and 79a7991.

📒 Files selected for processing (1)
  • wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)

31-37: LGTM! Hook registration is properly structured

The hook registration follows the expected pattern and maintains consistency with the existing code structure.


44-51: 🛠️ Refactor suggestion

Validate organization existence before setting ID

The method should verify that the organization exists and is active before setting its ID.

 public static function setOrganizationId(string $op, string $objectName, int $objectId, &$objectRef) {
   if ($objectName !== 'Organization' || !$objectRef->id) {
     return;
   }
+  
+  // Verify organization exists and is active
+  $org = \Civi\Api4\Contact::get(FALSE)
+    ->addSelect('is_active')
+    ->addWhere('id', '=', $objectRef->id)
+    ->addWhere('contact_type', '=', 'Organization')
+    ->execute()
+    ->first();
+  
+  if (!$org || !$org['is_active']) {
+    return;
+  }

   self::$organizationId = $objectRef->id;
 }

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (2)

23-23: Consider alternative state management approaches

Using a static variable for $organizationId could lead to state management issues in concurrent environments and make testing more difficult. Consider using dependency injection or a service class to manage this state.


72-73: Fix incorrect code comment

The comment mentions self::$test but the code uses self::$organizationId. This inconsistency should be fixed.

-    // Use self::$test (Organization ID)
+    // Use organization ID to set the institution name
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 79a7991 and de3910f.

📒 Files selected for processing (1)
  • wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1 hunks)
🔇 Additional comments (1)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php (1)

31-37: Verify hook execution order

The order of hook execution between hook_civicrm_post and hook_civicrm_pre is critical here. Ensure that setOrganizationId is called before setOrganizationNameForCollectionCamp to maintain proper state.

Comment on lines +44 to +51
public static function setOrganizationId(string $op, string $objectName, int $objectId, &$objectRef) {
if ($objectName !== 'Organization' || !$objectRef->id) {
return;
}

self::$organizationId = $objectRef->id;

}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add type validation for objectRef

The method should validate that $objectRef is an object before accessing its properties to prevent potential PHP errors.

 public static function setOrganizationId(string $op, string $objectName, int $objectId, &$objectRef) {
+    if (!is_object($objectRef)) {
+      return;
+    }
     if ($objectName !== 'Organization' || !$objectRef->id) {
       return;
     }
     self::$organizationId = $objectRef->id;
 }
📝 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.

Suggested change
public static function setOrganizationId(string $op, string $objectName, int $objectId, &$objectRef) {
if ($objectName !== 'Organization' || !$objectRef->id) {
return;
}
self::$organizationId = $objectRef->id;
}
public static function setOrganizationId(string $op, string $objectName, int $objectId, &$objectRef) {
if (!is_object($objectRef)) {
return;
}
if ($objectName !== 'Organization' || !$objectRef->id) {
return;
}
self::$organizationId = $objectRef->id;
}

Comment on lines +71 to +76
EckEntity::update('Collection_Camp', TRUE)
// Use self::$test (Organization ID)
->addValue('Institution_collection_camp_Review.Institution_Name', self::$organizationId)
->addWhere('id', '=', $entityId)
->execute();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add transaction handling

The database update operation should be wrapped in a transaction to ensure data consistency.

+    \Civi::transaction()->start();
+    try {
       EckEntity::update('Collection_Camp', TRUE)
         ->addValue('Institution_collection_camp_Review.Institution_Name', self::$organizationId)
         ->addWhere('id', '=', $entityId)
         ->execute();
+      \Civi::transaction()->commit();
+    }
+    catch (\Exception $e) {
+      \Civi::transaction()->rollback();
+      \Civi::log()->error('Failed to update Collection Camp organization name', [
+        'error' => $e->getMessage(),
+        'entityId' => $entityId
+      ]);
+    }
📝 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.

Suggested change
EckEntity::update('Collection_Camp', TRUE)
// Use self::$test (Organization ID)
->addValue('Institution_collection_camp_Review.Institution_Name', self::$organizationId)
->addWhere('id', '=', $entityId)
->execute();
}
\Civi::transaction()->start();
try {
EckEntity::update('Collection_Camp', TRUE)
->addValue('Institution_collection_camp_Review.Institution_Name', self::$organizationId)
->addWhere('id', '=', $entityId)
->execute();
\Civi::transaction()->commit();
}
catch (\Exception $e) {
\Civi::transaction()->rollback();
\Civi::log()->error('Failed to update Collection Camp organization name', [
'error' => $e->getMessage(),
'entityId' => $entityId
]);
}
}

@belwalshubham
Copy link
Copy Markdown
Collaborator Author

This PR is no longer needed.

@pokhiii pokhiii deleted the feat-update-organization-for-collection-camp branch April 19, 2025 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant