Add Activity In Institution related to institution Campaign #633
Conversation
|
Warning Rate limit exceeded@belwalshubham has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 6 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. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant CiviCRM
participant InstitutionCampaignService
participant ActivityLog
CiviCRM->>InstitutionCampaignService: Campaign Post Event
InstitutionCampaignService->>InstitutionCampaignService: Validate Campaign
InstitutionCampaignService->>ActivityLog: Create Campaign Organization Activity
Note over InstitutionCampaignService: Log activity with campaign details
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionCampaignService.php (10)
9-11: Provide Class-Level Documentation forInstitutionCampaignServiceThe class
InstitutionCampaignServicelacks a descriptive docblock. Adding a meaningful class-level comment will improve code readability and maintainability by explaining the purpose and responsibilities of this class.
14-16: Add Docblock togetSubscribedEventsMethodThe method
getSubscribedEventsis missing a docblock. Including a brief description of the method's functionality will enhance understanding for other developers.
25-27: DocumentlinkCollectionCampToContactMethodThe
linkCollectionCampToContactmethod lacks a docblock detailing its purpose and parameters. Providing this information will aid in code comprehension and maintenance.
28-28: Ensure Consistent Parameter Type HintingThe parameters of
linkCollectionCampToContactare type-hinted, which is good. However, for consistency, consider adding type hints to all methods in the class.
45-47: Remove Redundant Check for$currentInstitutionIdAfter verifying that
$currentInstitutionIdis not empty and returning early if it is, the subsequent check before callingcreateCollectionCampOrganizeActivityis redundant.Apply this diff to remove the redundant condition:
if (!$currentInstitutionId) { return; } $campaignTitle = $currentInstitutionCampaign['title']; $campaignId = $currentInstitutionCampaign['id']; -// Check for status change. -if ($currentInstitutionId) { self::createCollectionCampOrganizeActivity($currentInstitutionId, $campaignTitle, $campaignId); -}Also applies to: 52-54
57-59: Provide Documentation forcreateCollectionCampOrganizeActivityMethodThe method
createCollectionCampOrganizeActivitylacks a docblock. Adding a description of its purpose, along with parameter explanations, will improve code clarity.
60-60: Add Type Hints tocreateCollectionCampOrganizeActivityParametersThe method
createCollectionCampOrganizeActivitydoes not have type hints for its parameters. Consistently using type hints enhances code reliability and readability.Apply this diff to add type hints:
-private static function createCollectionCampOrganizeActivity($currentInstitutionId, $campaignTitle, $campaignId) { +private static function createCollectionCampOrganizeActivity(int $currentInstitutionId, string $campaignTitle, int $campaignId) {
72-73: Use Appropriate Logging Level for ExceptionsCurrently, exceptions are logged using
debug, which may not be sufficient for error tracking. Usingerrorlevel logging will ensure that these exceptions are more visible and can be addressed promptly.Apply this diff to change the logging level:
-\Civi::log()->debug("Exception while creating Organize Collection Camp activity: " . $ex->getMessage()); +\Civi::log()->error("Exception while creating Organize Collection Camp activity: " . $ex->getMessage());
1-77: Ensure Single Responsibility Principle Is FollowedThe
InstitutionCampaignServiceclass is handling event subscription, campaign data retrieval, and activity creation. This could be a violation of the Single Responsibility Principle. Consider splitting the code into separate classes or services for handling these distinct responsibilities.
34-41: Avoid Duplicate Code by Creating a Reusable Method for API CallsThe code for making API calls using
Campaign::get()andActivity::create()could be refactored into reusable methods or a helper class to reduce duplication and improve maintainability.Also applies to: 62-69
| $institutionCampaign = Campaign::get(TRUE) | ||
| ->addSelect( | ||
| 'id', | ||
| 'Additional_Details.Institution', | ||
| 'title' | ||
| ) | ||
| ->addWhere('id', '=', $objectId) | ||
| ->execute(); |
There was a problem hiding this comment.
Handle Potential Empty Result from Campaign::get()
There is no check to ensure that $institutionCampaign contains results before calling $institutionCampaign->first(). If the campaign with the given $objectId does not exist, calling first() may result in an error.
Apply this diff to add a check for empty results:
$institutionCampaign = Campaign::get(TRUE)
->addSelect(
'id',
'Additional_Details.Institution',
'title'
)
->addWhere('id', '=', $objectId)
->execute();
+$currentInstitutionCampaign = $institutionCampaign->first();
+if (!$currentInstitutionCampaign) {
+ // Handle the case where the campaign is not found.
+ return;
+}Committable suggestion skipped: line range outside the PR's diff.
| try { | ||
| $results = Activity::create(FALSE) | ||
| ->addValue('subject', $campaignTitle) | ||
| ->addValue('activity_type_id:name', 'Institution Campaign') | ||
| ->addValue('status_id:name', 'Completed') | ||
| ->addValue('activity_date_time', date('Y-m-d H:i:s')) | ||
| ->addValue('source_contact_id', $currentInstitutionId) | ||
| ->addValue('target_contact_id', $currentInstitutionId) | ||
| ->execute(); | ||
|
|
||
| } | ||
| catch (\CiviCRM_API4_Exception $ex) { | ||
| \Civi::log()->debug("Exception while creating Organize Collection Camp activity: " . $ex->getMessage()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Catch General Exceptions in Addition to API Exceptions
Only catching \CiviCRM_API4_Exception may cause other exceptions to go unhandled. It's advisable to catch general exceptions to prevent the application from crashing due to unexpected errors.
Apply this diff to catch general exceptions:
try {
$results = Activity::create(FALSE)
->addValue('subject', $campaignTitle)
->addValue('activity_type_id:name', 'Institution Campaign')
->addValue('status_id:name', 'Completed')
->addValue('activity_date_time', date('Y-m-d H:i:s'))
->addValue('source_contact_id', $currentInstitutionId)
->addValue('target_contact_id', $currentInstitutionId)
->execute();
-}
-catch (\CiviCRM_API4_Exception $ex) {
- \Civi::log()->error("Exception while creating Organize Collection Camp activity: " . $ex->getMessage());
+} catch (\Exception $ex) {
+ \Civi::log()->error("An error occurred while creating the activity: " . $ex->getMessage());
}📝 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.
| try { | |
| $results = Activity::create(FALSE) | |
| ->addValue('subject', $campaignTitle) | |
| ->addValue('activity_type_id:name', 'Institution Campaign') | |
| ->addValue('status_id:name', 'Completed') | |
| ->addValue('activity_date_time', date('Y-m-d H:i:s')) | |
| ->addValue('source_contact_id', $currentInstitutionId) | |
| ->addValue('target_contact_id', $currentInstitutionId) | |
| ->execute(); | |
| } | |
| catch (\CiviCRM_API4_Exception $ex) { | |
| \Civi::log()->debug("Exception while creating Organize Collection Camp activity: " . $ex->getMessage()); | |
| } | |
| try { | |
| $results = Activity::create(FALSE) | |
| ->addValue('subject', $campaignTitle) | |
| ->addValue('activity_type_id:name', 'Institution Campaign') | |
| ->addValue('status_id:name', 'Completed') | |
| ->addValue('activity_date_time', date('Y-m-d H:i:s')) | |
| ->addValue('source_contact_id', $currentInstitutionId) | |
| ->addValue('target_contact_id', $currentInstitutionId) | |
| ->execute(); | |
| } catch (\Exception $ex) { | |
| \Civi::log()->error("An error occurred while creating the activity: " . $ex->getMessage()); | |
| } |
| $currentInstitutionId = $currentInstitutionCampaign['Additional_Details.Institution']; | ||
| if (!$currentInstitutionId) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Check for Undefined Index in Data Retrieval
When accessing $currentInstitutionCampaign['Additional_Details.Institution'], there's a risk of an undefined index if the 'Additional_Details.Institution' key is missing. This could lead to a PHP notice or error.
Apply this diff to ensure the index exists before accessing it:
$currentInstitutionId = $currentInstitutionCampaign['Additional_Details.Institution'];
+if (!isset($currentInstitutionCampaign['Additional_Details.Institution'])) {
+ return;
+}Committable suggestion skipped: line range outside the PR's diff.
Add functionality to log an "Institution Campaign" activity in CiviCRM when a campaign is linked to an institution.
Summary by CodeRabbit
New Features
Bug Fixes