Skip to content

Add Logistics tab on institution collection camp#477

Merged
belwalshubham merged 6 commits intodevelopfrom
feat/add-institution-collection-camp-logistics
Nov 21, 2024
Merged

Add Logistics tab on institution collection camp#477
belwalshubham merged 6 commits intodevelopfrom
feat/add-institution-collection-camp-logistics

Conversation

@belwalshubham
Copy link
Copy Markdown
Collaborator

@belwalshubham belwalshubham commented Nov 21, 2024

PR description

  • Add logistics tab

Summary by CodeRabbit

  • New Features

    • Introduced a new constant for the Collection_Camp entity.
    • Added functionality for managing tab configurations related to the Collection_Camp entity, enhancing user navigation.
  • Improvements

    • Updated event subscription mechanism to include new event handlers for better integration with the tabset functionality.

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

coderabbitai bot commented Nov 21, 2024

Walkthrough

The pull request introduces enhancements to the InstitutionCollectionCampService class within the Civi namespace. A new constant ENTITY_NAME is added, and the getSubscribedEvents method is updated to handle a new event related to tab configurations. Additionally, two private static methods are introduced: isViewingInstituteCollectionCamp, which checks the current context against a specific entity type, and institutionCollectionCampTabset, which manages tab configurations. The setOfficeDetails method remains unchanged but is now complemented by the new tabset functionality.

Changes

File Path Change Summary
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCollectionCampService.php - Added constant ENTITY_NAME set to 'Collection_Camp'.
- Updated getSubscribedEvents to include a handler for &hook_civicrm_tabset.
- Introduced methods isViewingInstituteCollectionCamp and institutionCollectionCampTabset.
- setOfficeDetails method remains unchanged.

Possibly related PRs

Suggested reviewers

  • tarunnjoshi

🎉 In the land of code where changes flow,
A constant and methods begin to grow.
With tabs for collections, the service shines bright,
Enhancing the logic, making it right.
So here's to the updates, both clever and neat,
In the realm of Civi, this code is a treat! 🎊


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: 0

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

158-174: Consider using early returns to improve readability

While the validation logic is sound, the method could be more readable with early returns.

Here's a suggested refactoring:

 private static function isViewingCollectionCamp($tabsetName, $context) {
-    if ($tabsetName !== 'civicrm/eck/entity' || empty($context) || $context['entity_type']['name'] !== self::ENTITY_NAME) {
-      return FALSE;
-    }
+    if ($tabsetName !== 'civicrm/eck/entity') {
+        return FALSE;
+    }
+    
+    if (empty($context) || $context['entity_type']['name'] !== self::ENTITY_NAME) {
+        return FALSE;
+    }

     $entityId = $context['entity_id'];

     $entity = EckEntity::get(self::ENTITY_NAME, TRUE)
       ->addWhere('id', '=', $entityId)
       ->execute()->single();

     $entitySubtypeValue = $entity['subtype'];
     $subtypeId = self::getSubtypeId();

     return (int) $entitySubtypeValue === $subtypeId;
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3a2a1fd and e47f231.

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

19-19: LGTM: Constant follows naming conventions

The new constant ENTITY_NAME is well-placed and follows the existing pattern of constant declarations in the class.


28-28: LGTM: Event subscription follows CiviCRM hook patterns

The new hook subscription for tabset is properly integrated with the existing event subscriptions.


177-203: Consider extracting tab configuration to a constant or configuration file

The tab configuration array could be moved to a class constant or separate configuration file to improve maintainability and reusability.

Here's a suggested approach:

+ private const TAB_CONFIGS = [
+     'logistics' => [
+         'title' => 'Logistics',
+         'module' => 'afsearchInstitutionCollectionCampLogistics',
+         'directive' => 'afsearch-institution-collection-camp-logistics',
+         'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl',
+     ],
+ ];

  public static function InstitutionCollectionCampTabset($tabsetName, &$tabs, $context) {
      if (!self::isViewingCollectionCamp($tabsetName, $context)) {
          return;
      }

-     $tabConfigs = [
-         'logistics' => [
-             'title' => ts('Logistics'),
-             'module' => 'afsearchInstitutionCollectionCampLogistics',
-             'directive' => 'afsearch-institution-collection-camp-logistics',
-             'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl',
-         ],
-     ];

-     foreach ($tabConfigs as $key => $config) {
+     foreach (self::TAB_CONFIGS as $key => $config) {
          $tabs[$key] = [
              'id' => $key,
-             'title' => $config['title'],
+             'title' => ts($config['title']),
              'is_active' => 1,
              'template' => $config['template'],
              'module' => $config['module'],
              'directive' => $config['directive'],
          ];

          \Civi::service('angularjs.loader')->addModules($config['module']);
      }
  }

Also, verify that the template file exists:

✅ Verification successful

Template file exists, configuration extraction is a valid suggestion

The template file CRM/Goonjcustom/Tabs/CollectionCamp.tpl exists at the correct location relative to the extension root. The suggestion to extract the tab configuration to a constant is valid and would improve code maintainability without breaking existing functionality.

  • The template path in the configuration matches the actual file location
  • Moving configuration to a constant would make it easier to manage and reuse
  • The ts() function placement in the suggested diff is correct for translation handling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the template file exists
fd -t f "CollectionCamp.tpl" -x echo "Template file found: {}"

Length of output: 174

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)

158-177: Add PHPDoc block and type hints.

While the method is well-structured, it could benefit from better documentation and type safety:

  /**
+  * Determines if the current context is viewing a collection camp.
+  *
+  * @param string $tabsetName The name of the tabset
+  * @param array $context The context array containing entity information
+  * @return bool Returns true if viewing a collection camp, false otherwise
   */
- private static function isViewingCollectionCamp($tabsetName, $context) {
+ private static function isViewingCollectionCamp(string $tabsetName, array $context): bool {

179-181: Add comprehensive PHPDoc block.

The method lacks proper documentation of its parameters and purpose.

  /**
+  * Adds the logistics tab to the collection camp view.
+  *
+  * @param string $tabsetName The name of the tabset
+  * @param array &$tabs Reference to the tabs array
+  * @param array $context The context array containing entity information
+  * @return void
   */

187-194: Consider moving tab configurations to a separate configuration file.

The tab configuration array would be better placed in a separate configuration file or class constant, especially if more tabs will be added in the future.

Consider creating a dedicated configuration file like config/tabs.php:

<?php
return [
    'collection_camp_tabs' => [
        'logistics' => [
            'title' => ts('Logistics'),
            'module' => 'afsearchInstitutionCollectionCampLogistics',
            'directive' => 'afsearch-institution-collection-camp-logistics',
            'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl',
        ],
    ],
];
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between e47f231 and 394fbd7.

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

19-19: LGTM! Good use of constants.

The addition of ENTITY_NAME constant helps eliminate magic strings and improves maintainability.


28-28: LGTM! Event subscription properly configured.

The hook subscription follows the established pattern and correctly maps to the new tabset method.


196-207: 🛠️ Refactor suggestion

Add template existence validation.

The code should verify that the template file exists before attempting to use it.

Add template validation before registering the tab:

  foreach ($tabConfigs as $key => $config) {
+     $templatePath = $config['template'];
+     if (!file_exists(\Civi::paths()->getPath("[civicrm.root]/{$templatePath}"))) {
+         \CRM_Core_Error::debug_log_message("Template not found: {$templatePath}");
+         continue;
+     }
      $tabs[$key] = [

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)

158-177: Add PHPDoc documentation for the method.

The method lacks proper documentation. Add PHPDoc to describe parameters, return type, and purpose.

Add documentation:

   /**
+   * Checks if the current context is viewing an institution collection camp.
+   *
+   * @param string $tabsetName The name of the tabset
+   * @param array $context The context array containing entity information
+   * @return bool Returns true if viewing an institution collection camp
    */
    private static function isViewingInstituteCollectionCamp($tabsetName, $context) {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 394fbd7 and ba74f33.

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

19-19: LGTM! Good use of constants.

The constant follows the class's naming convention and promotes maintainability by centralizing the entity name.

return [
'&hook_civicrm_fieldOptions' => 'setIndianStateOptions',
'&hook_civicrm_custom' => 'setOfficeDetails',
'&hook_civicrm_tabset' => 'InstitutionCollectionCampTabset',
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

Method name should follow camelCase convention.

The method name InstitutionCollectionCampTabset violates PSR-1 naming conventions for methods. In PHP, method names should be in camelCase.

Rename the method:

-      '&hook_civicrm_tabset' => 'InstitutionCollectionCampTabset',
+      '&hook_civicrm_tabset' => 'institutionCollectionCampTabset',
📝 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
'&hook_civicrm_tabset' => 'InstitutionCollectionCampTabset',
'&hook_civicrm_tabset' => 'institutionCollectionCampTabset',

Comment on lines +187 to +193
$tabConfigs = [
'logistics' => [
'title' => ts('Logistics'),
'module' => 'afsearchInstitutionCollectionCampLogistics',
'directive' => 'afsearch-institution-collection-camp-logistics',
'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl',
],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codebase verification

Missing Angular.js module registration and potential template path mismatch

The template file exists but at a different path than specified in the configuration:

  • Found at: wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCamp.tpl
  • Config path: CRM/Goonjcustom/Tabs/CollectionCamp.tpl

Additionally:

  • No registration found for the Angular.js module afsearchInstitutionCollectionCampLogistics
  • No related logistics templates or JavaScript implementations were found

Please ensure:

  1. The Angular.js module is properly registered and implemented
  2. The template path in the configuration matches the actual file location
🔗 Analysis chain

Verify template and Angular.js module existence.

Let's ensure the required files exist and are properly registered.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the template file exists
if [ -f "templates/CRM/Goonjcustom/Tabs/CollectionCamp.tpl" ]; then
    echo "Template file exists"
else
    echo "Template file missing"
fi

# Search for Angular.js module registration
rg -l "afsearchInstitutionCollectionCampLogistics" --type js

Length of output: 176


Script:

#!/bin/bash
# Let's check if the template exists in any other location
fd "CollectionCamp.tpl"

# Let's check for any Angular.js module definitions in JavaScript files
rg "angular\.module.*afsearchInstitutionCollectionCampLogistics" --type js

# Let's also check for any partial matches to find potential typos or different naming
rg "CollectionCamp" --type js
rg "afsearch.*Logistics" --type js

# Check for any other template files that might be related
fd -e tpl -e html "logistics" -i

Length of output: 442

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)

206-222: Align method name with constant naming convention

The method name isViewingInstituteCollectionCamp uses "Institute" while the constant uses "Institution". This inconsistency could lead to confusion.

Consider renaming for consistency:

-  private static function isViewingInstituteCollectionCamp($tabsetName, $context) {
+  private static function isViewingInstitutionCollectionCamp($tabsetName, $context) {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ba74f33 and cae3137.

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

Line range hint 20-20: LGTM: Constant definition follows naming conventions

The new constant ENTITY_NAME is well-defined and follows proper naming conventions.

'directive' => $config['directive'],
];

\Civi::service('angularjs.loader')->addModules($config['module']);
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 error handling for Angular.js module loading

Direct service access without error handling could cause runtime errors if the module fails to load.

Add try-catch block:

-      \Civi::service('angularjs.loader')->addModules($config['module']);
+      try {
+        \Civi::service('angularjs.loader')->addModules($config['module']);
+      } catch (\Exception $e) {
+        \CRM_Core_Error::debug_log_message("Failed to load Angular module {$config['module']}: " . $e->getMessage());
+        $tabs[$key]['is_active'] = 0;  // Disable tab if module fails to load
+      }

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

Comment on lines +227 to +253
public static function InstitutionCollectionCampTabset($tabsetName, &$tabs, $context) {
if (!self::isViewingInstituteCollectionCamp($tabsetName, $context)) {
return;
}

$tabConfigs = [
'logistics' => [
'title' => ts('Logistics'),
'module' => 'afsearchInstitutionCollectionCampLogistics',
'directive' => 'afsearch-institution-collection-camp-logistics',
'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl',
],
];

foreach ($tabConfigs as $key => $config) {
$tabs[$key] = [
'id' => $key,
'title' => $config['title'],
'is_active' => 1,
'template' => $config['template'],
'module' => $config['module'],
'directive' => $config['directive'],
];

\Civi::service('angularjs.loader')->addModules($config['module']);
}
}
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 input validation for context parameter

The method assumes $context is an array but doesn't validate it.

Add validation at the start of the method:

   public static function InstitutionCollectionCampTabset($tabsetName, &$tabs, $context) {
+    if (!is_array($context)) {
+      \CRM_Core_Error::debug_log_message("Invalid context parameter provided to InstitutionCollectionCampTabset");
+      return;
+    }
+
     if (!self::isViewingInstituteCollectionCamp($tabsetName, $context)) {
       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
public static function InstitutionCollectionCampTabset($tabsetName, &$tabs, $context) {
if (!self::isViewingInstituteCollectionCamp($tabsetName, $context)) {
return;
}
$tabConfigs = [
'logistics' => [
'title' => ts('Logistics'),
'module' => 'afsearchInstitutionCollectionCampLogistics',
'directive' => 'afsearch-institution-collection-camp-logistics',
'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl',
],
];
foreach ($tabConfigs as $key => $config) {
$tabs[$key] = [
'id' => $key,
'title' => $config['title'],
'is_active' => 1,
'template' => $config['template'],
'module' => $config['module'],
'directive' => $config['directive'],
];
\Civi::service('angularjs.loader')->addModules($config['module']);
}
}
public static function InstitutionCollectionCampTabset($tabsetName, &$tabs, $context) {
if (!is_array($context)) {
\CRM_Core_Error::debug_log_message("Invalid context parameter provided to InstitutionCollectionCampTabset");
return;
}
if (!self::isViewingInstituteCollectionCamp($tabsetName, $context)) {
return;
}
$tabConfigs = [
'logistics' => [
'title' => ts('Logistics'),
'module' => 'afsearchInstitutionCollectionCampLogistics',
'directive' => 'afsearch-institution-collection-camp-logistics',
'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl',
],
];
foreach ($tabConfigs as $key => $config) {
$tabs[$key] = [
'id' => $key,
'title' => $config['title'],
'is_active' => 1,
'template' => $config['template'],
'module' => $config['module'],
'directive' => $config['directive'],
];
\Civi::service('angularjs.loader')->addModules($config['module']);
}
}

Comment on lines +234 to +238
'title' => ts('Logistics'),
'module' => 'afsearchInstitutionCollectionCampLogistics',
'directive' => 'afsearch-institution-collection-camp-logistics',
'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl',
],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codebase verification

Missing Angular.js module and directive implementations

The code references an Angular.js module 'afsearchInstitutionCollectionCampLogistics' and directive 'afsearch-institution-collection-camp-logistics', but their implementations are missing from the codebase. This violates the completeness principle and will lead to runtime errors.

  • The template file exists at wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCamp.tpl but expects Angular.js components that are not defined
  • No Angular.js module definition found for 'afsearchInstitutionCollectionCampLogistics'
  • No directive implementation found for 'afsearch-institution-collection-camp-logistics'
  • The only JavaScript file in the extension (puppeteer.js) is unrelated to these Angular.js components
🔗 Analysis chain

Verify Angular.js module and template dependencies

Let's verify the existence and proper setup of the Angular.js module and its template.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for Angular.js module definition
echo "Checking for Angular.js module definition..."
rg "angular\.module.*afsearchInstitutionCollectionCampLogistics" --type js

# Check for template file
echo "Checking for template file..."
fd "CollectionCamp.tpl"

# Check for directive implementation
echo "Checking for directive implementation..."
rg "directive.*afsearch-institution-collection-camp-logistics" --type js

Length of output: 523


Script:

#!/bin/bash
# Let's check for any Angular.js related files in the extension
echo "Checking for Angular.js files..."
fd -e js . wp-content/civi-extensions/goonjcustom

# Check content of any js files for module or directive definitions
echo "Checking js file contents..."
fd -e js . wp-content/civi-extensions/goonjcustom --exec cat {}

# Check if the template file exists and its content
echo "Checking template content..."
cat wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCamp.tpl

# Check for any references to the module or directive
echo "Checking for any references..."
rg "afsearchInstitutionCollectionCampLogistics|afsearch-institution-collection-camp-logistics" -t js -t php

Length of output: 1790

/**
*
*/
public static function InstitutionCollectionCampTabset($tabsetName, &$tabs, $context) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It violates PSR-1 naming conventions

Suggested change
public static function InstitutionCollectionCampTabset($tabsetName, &$tabs, $context) {
public static function institutionCollectionCampTabset($tabsetName, &$tabs, $context) {

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

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between cae3137 and 4064568.

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

31-31: Method name in hook subscription should follow camelCase convention

The subscribed method name should follow PSR-1 naming conventions.

Comment on lines +206 to +222
private static function isViewingInstituteCollectionCamp($tabsetName, $context) {
if ($tabsetName !== 'civicrm/eck/entity' || empty($context) || $context['entity_type']['name'] !== self::ENTITY_NAME) {
return FALSE;
}

$entityId = $context['entity_id'];

$entity = EckEntity::get(self::ENTITY_NAME, TRUE)
->addWhere('id', '=', $entityId)
->execute()->single();

$entitySubtypeValue = $entity['subtype'];

$subtypeId = self::getSubtypeId();

return (int) $entitySubtypeValue === $subtypeId;
}
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 input validation and error handling

The method could benefit from improved validation and error handling:

  1. Missing validation for input parameters
  2. No error handling for entity retrieval

Apply this diff:

 private static function isViewingInstituteCollectionCamp($tabsetName, $context) {
+    if (!is_string($tabsetName) || !is_array($context)) {
+      \CRM_Core_Error::debug_log_message('Invalid parameters provided to isViewingInstituteCollectionCamp');
+      return FALSE;
+    }
+
     if ($tabsetName !== 'civicrm/eck/entity' || empty($context) || $context['entity_type']['name'] !== self::ENTITY_NAME) {
       return FALSE;
     }

     $entityId = $context['entity_id'];
 
-    $entity = EckEntity::get(self::ENTITY_NAME, TRUE)
-      ->addWhere('id', '=', $entityId)
-      ->execute()->single();
+    try {
+      $entity = EckEntity::get(self::ENTITY_NAME, TRUE)
+        ->addWhere('id', '=', $entityId)
+        ->execute()->single();
+    } catch (\Exception $e) {
+      \CRM_Core_Error::debug_log_message("Failed to retrieve entity: " . $e->getMessage());
+      return FALSE;
+    }

+    if (empty($entity)) {
+      return FALSE;
+    }

     $entitySubtypeValue = $entity['subtype'];
     $subtypeId = self::getSubtypeId();
 
     return (int) $entitySubtypeValue === $subtypeId;
   }
📝 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
private static function isViewingInstituteCollectionCamp($tabsetName, $context) {
if ($tabsetName !== 'civicrm/eck/entity' || empty($context) || $context['entity_type']['name'] !== self::ENTITY_NAME) {
return FALSE;
}
$entityId = $context['entity_id'];
$entity = EckEntity::get(self::ENTITY_NAME, TRUE)
->addWhere('id', '=', $entityId)
->execute()->single();
$entitySubtypeValue = $entity['subtype'];
$subtypeId = self::getSubtypeId();
return (int) $entitySubtypeValue === $subtypeId;
}
private static function isViewingInstituteCollectionCamp($tabsetName, $context) {
if (!is_string($tabsetName) || !is_array($context)) {
\CRM_Core_Error::debug_log_message('Invalid parameters provided to isViewingInstituteCollectionCamp');
return FALSE;
}
if ($tabsetName !== 'civicrm/eck/entity' || empty($context) || $context['entity_type']['name'] !== self::ENTITY_NAME) {
return FALSE;
}
$entityId = $context['entity_id'];
try {
$entity = EckEntity::get(self::ENTITY_NAME, TRUE)
->addWhere('id', '=', $entityId)
->execute()->single();
} catch (\Exception $e) {
\CRM_Core_Error::debug_log_message("Failed to retrieve entity: " . $e->getMessage());
return FALSE;
}
if (empty($entity)) {
return FALSE;
}
$entitySubtypeValue = $entity['subtype'];
$subtypeId = self::getSubtypeId();
return (int) $entitySubtypeValue === $subtypeId;
}

Comment on lines +232 to +252
$tabConfigs = [
'logistics' => [
'title' => ts('Logistics'),
'module' => 'afsearchInstitutionCollectionCampLogistics',
'directive' => 'afsearch-institution-collection-camp-logistics',
'template' => 'CRM/Goonjcustom/Tabs/CollectionCamp.tpl',
],
];

foreach ($tabConfigs as $key => $config) {
$tabs[$key] = [
'id' => $key,
'title' => $config['title'],
'is_active' => 1,
'template' => $config['template'],
'module' => $config['module'],
'directive' => $config['directive'],
];

\Civi::service('angularjs.loader')->addModules($config['module']);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Missing required Angular.js components for tab functionality

The verification confirms that the required Angular.js components are indeed missing:

  • No Angular.js module 'afsearchInstitutionCollectionCampLogistics' is defined
  • No directive 'afsearch-institution-collection-camp-logistics' exists
  • The template file exists but depends on these missing components

Required implementations:

  • Create wp-content/civi-extensions/goonjcustom/js/afsearchInstitutionCollectionCampLogistics.js with:
    angular.module('afsearchInstitutionCollectionCampLogistics', [])
      .directive('afsearchInstitutionCollectionCampLogistics', [...]);
  • Add error handling for module loading as suggested in the diff
🔗 Analysis chain

Missing Angular.js components and error handling

Several critical issues need to be addressed:

  1. The Angular.js module 'afsearchInstitutionCollectionCampLogistics' is not implemented
  2. The directive 'afsearch-institution-collection-camp-logistics' is missing
  3. No error handling for Angular.js module loading

Let's verify the existence of required components:

Required actions:

  1. Implement the Angular.js module
  2. Implement the directive
  3. Add error handling for module loading:
-      \Civi::service('angularjs.loader')->addModules($config['module']);
+      try {
+        \Civi::service('angularjs.loader')->addModules($config['module']);
+      } catch (\Exception $e) {
+        \CRM_Core_Error::debug_log_message("Failed to load Angular module {$config['module']}: " . $e->getMessage());
+        $tabs[$key]['is_active'] = 0;  // Disable tab if module fails to load
+      }

Let's gather more information to verify if these Angular.js components might exist in other locations or formats.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for Angular.js module definition
echo "Checking for Angular.js module..."
rg "angular\.module.*afsearchInstitutionCollectionCampLogistics" --type js

# Check for directive implementation
echo "Checking for directive..."
rg "directive.*afsearch-institution-collection-camp-logistics" --type js

# Check template location
echo "Checking template location..."
fd "CollectionCamp.tpl"

Length of output: 471


Script:

#!/bin/bash
# Check for any Angular.js module definitions in the extension
echo "Checking all Angular module definitions..."
rg "angular\.module" --type js wp-content/civi-extensions/goonjcustom/

# Check for any directive definitions in the extension
echo "Checking all directive definitions..."
rg "\.directive\(" --type js wp-content/civi-extensions/goonjcustom/

# Check for any JavaScript files that might contain these components
echo "Checking for relevant JS files..."
fd -e js . wp-content/civi-extensions/goonjcustom/

# Verify template content
echo "Checking template content..."
cat "wp-content/civi-extensions/goonjcustom/templates/CRM/Goonjcustom/Tabs/CollectionCamp.tpl"

Length of output: 873

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.

2 participants