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 25 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 (2)
WalkthroughThis pull request introduces logging enhancements to the Changes
Sequence DiagramsequenceDiagram
participant Form as Afform Submit
participant Service as InstitutionService
participant Address as Address Utility
Form->>Service: Submit Event
Service->>Service: Check Form Name
Service->>Service: Validate Entity Type
Service->>Address: Update Address
Address-->>Service: Return Update Result
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (2)
24-26: Consider adjusting naming and static usage
Definingconst Institution_INTENT_FB_NAMEwith a capitalized prefix is good, but for consistency across constants, you might rename itINSTITUTION_INTENT_FB_NAME. Also, relying on a static property ($goonjActivitiesAddress) can lead to concurrency conflicts when multiple requests run concurrently; consider storing the address in a request-scoped variable or passing it explicitly.
47-81: Check for multiple address entries and handle potential missing data
ThesetInstituteAddressfunction currently assigns only the first address record[0], which might be limiting if there are multiple addresses. Consider supporting all addresses or clearly documenting that only the first address is processed.wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (1)
230-247: Be cautious about sensitive data in logs
These newly addederror_logstatements may inadvertently expose personally identifiable information (PII) or sensitive data if$fieldscontain user information. Ensure that logging is sanitized or strictly limited to non-sensitive data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(2 hunks)wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php(3 hunks)
🔇 Additional comments (2)
wp-content/civi-extensions/goonjcustom/Civi/InstitutionService.php (2)
5-13: All required imports appear consistent and complete
These added use statements correctly reference the classes used in the new methods.
33-36: Validate event priority ordering
The subscription to civi.afform.submit is prioritizing setInstituteAddress (9) above setInstitutionPocAddress (8). Ensure this priority ordering is intended. If not, switch them so that POC address logic executes first if needed.
| public static function setInstitutionPocAddress(AfformSubmitEvent $event) { | ||
| $afform = $event->getAfform(); | ||
| $formName = $afform['name']; | ||
|
|
||
| if ($formName !== self::Institution_INTENT_FB_NAME) { | ||
| return; | ||
| } | ||
|
|
||
| $entityType = $event->getEntityType(); | ||
|
|
||
| if (!CoreUtil::isContact($entityType)) { | ||
| return; | ||
| } | ||
|
|
||
| foreach ($event->records as $index => $contact) { | ||
| if (empty($contact['fields'])) { | ||
| continue; | ||
| } | ||
|
|
||
| $contactId = $contact['fields']['id']; | ||
|
|
||
| $stateProvinceId = self::$goonjActivitiesAddress['state_province_id']; | ||
|
|
||
| $updateResults = Address::update(FALSE) | ||
| ->addValue('state_province_id', $stateProvinceId) | ||
| ->addWhere('contact_id', '=', $test) | ||
| ->execute(); | ||
| } | ||
|
|
||
| \Civi::log()->info('Address update results', $updateResults); | ||
|
|
||
| } |
There was a problem hiding this comment.
Critical bug: Missing variable $test reference
Line 111 uses ->addWhere('contact_id', '=', $test), but $test is not defined anywhere. This is likely a typo and should be replaced with $contactId or the appropriate variable.
You can fix this issue by changing $test to $contactId:
- ->addWhere('contact_id', '=', $test)
+ ->addWhere('contact_id', '=', $contactId)📝 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 setInstitutionPocAddress(AfformSubmitEvent $event) { | |
| $afform = $event->getAfform(); | |
| $formName = $afform['name']; | |
| if ($formName !== self::Institution_INTENT_FB_NAME) { | |
| return; | |
| } | |
| $entityType = $event->getEntityType(); | |
| if (!CoreUtil::isContact($entityType)) { | |
| return; | |
| } | |
| foreach ($event->records as $index => $contact) { | |
| if (empty($contact['fields'])) { | |
| continue; | |
| } | |
| $contactId = $contact['fields']['id']; | |
| $stateProvinceId = self::$goonjActivitiesAddress['state_province_id']; | |
| $updateResults = Address::update(FALSE) | |
| ->addValue('state_province_id', $stateProvinceId) | |
| ->addWhere('contact_id', '=', $test) | |
| ->execute(); | |
| } | |
| \Civi::log()->info('Address update results', $updateResults); | |
| } | |
| public static function setInstitutionPocAddress(AfformSubmitEvent $event) { | |
| $afform = $event->getAfform(); | |
| $formName = $afform['name']; | |
| if ($formName !== self::Institution_INTENT_FB_NAME) { | |
| return; | |
| } | |
| $entityType = $event->getEntityType(); | |
| if (!CoreUtil::isContact($entityType)) { | |
| return; | |
| } | |
| foreach ($event->records as $index => $contact) { | |
| if (empty($contact['fields'])) { | |
| continue; | |
| } | |
| $contactId = $contact['fields']['id']; | |
| $stateProvinceId = self::$goonjActivitiesAddress['state_province_id']; | |
| $updateResults = Address::update(FALSE) | |
| ->addValue('state_province_id', $stateProvinceId) | |
| ->addWhere('contact_id', '=', $contactId) | |
| ->execute(); | |
| } | |
| \Civi::log()->info('Address update results', $updateResults); | |
| } |
| error_log("entityType.." . print_r($entityType, TRUE)); | ||
|
|
||
|
|
||
| if (!CoreUtil::isContact($entityType)) { | ||
| return; | ||
| } | ||
|
|
||
| foreach ($event->records as $index => $contact) { | ||
| error_log("contact.." . print_r($contact['fields'], TRUE)); | ||
| if (empty($contact['fields'])) { | ||
| continue; | ||
| } | ||
|
|
||
| error_log("nitu.." . print_r($event->records, TRUE)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent logging entire contact objects
Using print_r($contact['fields'], TRUE) and printing $event->records in logs might include private data. Consider logging only necessary fields or masking sensitive details.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes