redirection to correct flow after new individual creation#299
redirection to correct flow after new individual creation#299
Conversation
|
Warning Rate limit exceeded@pokhiii has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 9 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. WalkthroughThe changes introduce a new action hook, Changes
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
🧹 Outside diff range and nitpick comments (2)
wp-content/themes/goonj-crm/functions.php (2)
463-463: Typo in comment 'Colllection Camp'There's a typo in the comment:
// First, we check if the source of Individual is Colllection Camp (or Dropping Center).The word "Colllection" should be "Collection".
Correct it as follows:
-// First, we check if the source of Individual is Colllection Camp (or Dropping Center). +// First, we check if the source of Individual is Collection Camp (or Dropping Center).
485-485: Use absolute URL withwp_safe_redirectWhen using
wp_safe_redirect(), it's recommended to use an absolute URL to ensure the redirection works correctly in all contexts.Modify the redirect to use
home_url():-wp_safe_redirect( $redirectPath ); +wp_safe_redirect( home_url( $redirectPath ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- wp-content/themes/goonj-crm/functions.php (1 hunks)
🔇 Additional comments (1)
wp-content/themes/goonj-crm/functions.php (1)
445-449: Possible issues with primary email and phone joinsThe query assumes that the individual has both a primary email and a primary phone. If either is missing, the joins might not function as expected, potentially causing the query to fail or return incomplete data.
Consider modifying the query to handle cases where the primary email or phone may not exist, or ensure that these fields are mandatory for an individual.
Automation to verify individuals without primary email or phone:
| add_action( 'template_redirect', 'goonj_redirect_after_individual_cration' ); | ||
| function goonj_redirect_after_individual_cration() { |
There was a problem hiding this comment.
Typo in function name 'goonj_redirect_after_individual_cration'
The function name goonj_redirect_after_individual_cration contains a typo in the word "cration". It should be "creation" to reflect correct spelling and improve code readability.
Apply this diff to correct the typo:
-add_action( 'template_redirect', 'goonj_redirect_after_individual_cration' );
+add_action( 'template_redirect', 'goonj_redirect_after_individual_creation' );
-function goonj_redirect_after_individual_cration() {
+function goonj_redirect_after_individual_creation() {📝 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.
| add_action( 'template_redirect', 'goonj_redirect_after_individual_cration' ); | |
| function goonj_redirect_after_individual_cration() { | |
| add_action( 'template_redirect', 'goonj_redirect_after_individual_creation' ); | |
| function goonj_redirect_after_individual_creation() { |
| ! isset( $_REQUEST['goonjAction'] ) || | ||
| $_REQUEST['goonjAction'] !== 'individualCreated' || | ||
| ! isset( $_REQUEST['individualId'] ) | ||
| ) { | ||
| return; |
There was a problem hiding this comment.
Security Concern: Use of $_REQUEST Superglobal
Using $_REQUEST can be insecure because it includes data from $_GET, $_POST, and $_COOKIE. This might allow unexpected data sources to affect the logic.
It's safer to specify the expected superglobal. For example, if you expect the data to come from a GET request, use $_GET:
- if (
- ! isset( $_REQUEST['goonjAction'] ) ||
- $_REQUEST['goonjAction'] !== 'individualCreated' ||
- ! isset( $_REQUEST['individualId'] )
- ) {
+ if (
+ ! isset( $_GET['goonjAction'] ) ||
+ $_GET['goonjAction'] !== 'individualCreated' ||
+ ! isset( $_GET['individualId'] )
+ ) {Ensure that the rest of the code is updated accordingly.
📝 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.
| ! isset( $_REQUEST['goonjAction'] ) || | |
| $_REQUEST['goonjAction'] !== 'individualCreated' || | |
| ! isset( $_REQUEST['individualId'] ) | |
| ) { | |
| return; | |
| ! isset( $_GET['goonjAction'] ) || | |
| $_GET['goonjAction'] !== 'individualCreated' || | |
| ! isset( $_GET['individualId'] ) | |
| ) { | |
| return; |
| if ( | ||
| ! isset( $_REQUEST['goonjAction'] ) || | ||
| $_REQUEST['goonjAction'] !== 'individualCreated' || | ||
| ! isset( $_REQUEST['individualId'] ) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| $individual = \Civi\Api4\Contact::get( false ) | ||
| ->addSelect( 'source', 'Individual_fields.Creation_Flow', 'email.email', 'phone.phone' ) | ||
| ->addJoin( 'Email AS email', 'LEFT' ) | ||
| ->addJoin( 'Phone AS phone', 'LEFT' ) | ||
| ->addWhere( 'email.is_primary', '=', true ) | ||
| ->addWhere( 'phone.is_primary', '=', true ) | ||
| ->addWhere( 'id', '=', absint( $_REQUEST['individualId'] ) ) | ||
| ->setLimit( 1 ) | ||
| ->execute()->single(); | ||
|
|
||
| $creationFlow = $individual['Individual_fields.Creation_Flow']; | ||
| $source = $individual['source']; | ||
|
|
||
| if ( ! $source ) { | ||
| return; | ||
| } | ||
|
|
||
| switch ( $creationFlow ) { | ||
| case 'material-contribution': | ||
| // If the individual was created while in the process of material contribution, | ||
| // then we need to find out from WHERE was she trying to contribute. | ||
|
|
||
| // First, we check if the source of Individual is Colllection Camp (or Dropping Center). | ||
| $collectionCamp = \Civi\Api4\EckEntity::get( 'Collection_Camp', false ) | ||
| ->addWhere( 'title', '=', $source ) | ||
| ->setLimit( 1 ) | ||
| ->execute()->first(); | ||
|
|
||
| if ( ! empty( $collectionCamp['id'] ) ) { | ||
| $redirectPath = sprintf( | ||
| '/material-contribution/#?email=%s&phone=%s&Material_Contribution.Collection_Camp=%s&source_contact_id=%s', | ||
| $individual['email.email'], | ||
| $individual['phone.phone'], | ||
| $collectionCamp['id'], | ||
| $individual['id'] | ||
| ); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if ( empty( $redirectPath ) ) { | ||
| return; | ||
| } | ||
|
|
||
| wp_safe_redirect( $redirectPath ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring for improved modularity
The function goonj_redirect_after_individual_cration is quite long and handles multiple responsibilities. This can make it harder to maintain and test.
Consider breaking down the function into smaller, more focused functions:
- A function to validate the request parameters
- A function to retrieve individual data
- Separate functions for each creation flow
Example refactoring:
function goonj_redirect_after_individual_creation() {
if (!goonj_validate_individual_creation_request()) {
return;
}
$individual = goonj_get_individual_data($_REQUEST['individualId']);
if (empty($individual)) {
return;
}
$redirectPath = goonj_get_redirect_path($individual);
if (!empty($redirectPath)) {
wp_safe_redirect($redirectPath);
exit;
}
}
function goonj_validate_individual_creation_request() {
return isset($_REQUEST['goonjAction'])
&& $_REQUEST['goonjAction'] === 'individualCreated'
&& isset($_REQUEST['individualId']);
}
function goonj_get_individual_data($individualId) {
// API call to retrieve individual data
}
function goonj_get_redirect_path($individual) {
$creationFlow = $individual['Individual_fields.Creation_Flow'];
$source = $individual['source'];
if (!$source) {
return '';
}
switch ($creationFlow) {
case 'material-contribution':
return goonj_get_material_contribution_redirect_path($individual, $source);
// Add other cases here
default:
error_log("Unhandled creation flow: " . $creationFlow);
return '';
}
}
function goonj_get_material_contribution_redirect_path($individual, $source) {
// Existing logic for material contribution redirect
}This refactoring improves modularity, making the code easier to understand, maintain, and test.
| $individual = \Civi\Api4\Contact::get( false ) | ||
| ->addSelect( 'source', 'Individual_fields.Creation_Flow', 'email.email', 'phone.phone' ) | ||
| ->addJoin( 'Email AS email', 'LEFT' ) | ||
| ->addJoin( 'Phone AS phone', 'LEFT' ) | ||
| ->addWhere( 'email.is_primary', '=', true ) | ||
| ->addWhere( 'phone.is_primary', '=', true ) | ||
| ->addWhere( 'id', '=', absint( $_REQUEST['individualId'] ) ) | ||
| ->setLimit( 1 ) | ||
| ->execute()->single(); |
There was a problem hiding this comment.
Add error handling for CiviCRM API call
The CiviCRM API call lacks error handling. If the API call fails, it may lead to unexpected behavior or errors.
Consider wrapping the API call in a try-catch block and handle potential exceptions:
- $individual = \Civi\Api4\Contact::get( false )
- ->addSelect( 'source', 'Individual_fields.Creation_Flow', 'email.email', 'phone.phone' )
- ->addJoin( 'Email AS email', 'LEFT' )
- ->addJoin( 'Phone AS phone', 'LEFT' )
- ->addWhere( 'email.is_primary', '=', true )
- ->addWhere( 'phone.is_primary', '=', true )
- ->addWhere( 'id', '=', absint( $_REQUEST['individualId'] ) )
- ->setLimit( 1 )
- ->execute()->single();
+ try {
+ $individual = \Civi\Api4\Contact::get( false )
+ ->addSelect( 'source', 'Individual_fields.Creation_Flow', 'email.email', 'phone.phone' )
+ ->addJoin( 'Email AS email', 'LEFT' )
+ ->addJoin( 'Phone AS phone', 'LEFT' )
+ ->addWhere( 'email.is_primary', '=', true )
+ ->addWhere( 'phone.is_primary', '=', true )
+ ->addWhere( 'id', '=', absint( $_REQUEST['individualId'] ) )
+ ->setLimit( 1 )
+ ->execute()->single();
+ } catch (\Exception $e) {
+ error_log('Error retrieving individual data: ' . $e->getMessage());
+ return;
+ }
+
+ if (empty($individual)) {
+ 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.
| $individual = \Civi\Api4\Contact::get( false ) | |
| ->addSelect( 'source', 'Individual_fields.Creation_Flow', 'email.email', 'phone.phone' ) | |
| ->addJoin( 'Email AS email', 'LEFT' ) | |
| ->addJoin( 'Phone AS phone', 'LEFT' ) | |
| ->addWhere( 'email.is_primary', '=', true ) | |
| ->addWhere( 'phone.is_primary', '=', true ) | |
| ->addWhere( 'id', '=', absint( $_REQUEST['individualId'] ) ) | |
| ->setLimit( 1 ) | |
| ->execute()->single(); | |
| try { | |
| $individual = \Civi\Api4\Contact::get( false ) | |
| ->addSelect( 'source', 'Individual_fields.Creation_Flow', 'email.email', 'phone.phone' ) | |
| ->addJoin( 'Email AS email', 'LEFT' ) | |
| ->addJoin( 'Phone AS phone', 'LEFT' ) | |
| ->addWhere( 'email.is_primary', '=', true ) | |
| ->addWhere( 'phone.is_primary', '=', true ) | |
| ->addWhere( 'id', '=', absint( $_REQUEST['individualId'] ) ) | |
| ->setLimit( 1 ) | |
| ->execute()->single(); | |
| } catch (\Exception $e) { | |
| error_log('Error retrieving individual data: ' . $e->getMessage()); | |
| return; | |
| } | |
| if (empty($individual)) { | |
| return; | |
| } |
1 participant
Creation_Flowfor the individual and thesourceto decide what to do: keep it on the same page redirect to something new.Summary by CodeRabbit
New Features
Bug Fixes