Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Material contibution #354

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Material contibution #354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b3e8d3d
fix(*): refactor qr code generate function
belwalshubham Oct 9, 2024
4e51228
fix(*): code cleanups
belwalshubham Oct 9, 2024
9eb62a1
resolve feedbacks
belwalshubham Oct 9, 2024
42eab60
fix(*): cleanups
belwalshubham Oct 9, 2024
a962288
Refactor date and address handling logic for collection camps and dro…
belwalshubham Oct 10, 2024
34398b4
resolve merge conflicts
belwalshubham Oct 10, 2024
c5d9e45
fix(*): code cleanups
belwalshubham Oct 10, 2024
550660f
update contribution link
belwalshubham Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 18 additions & 86 deletions wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Civi\Api4\StateProvince;
use Civi\Api4\Utils\CoreUtil;
use Civi\Core\Service\AutoSubscriber;
use Civi\QrCodeService;

/**
*
Expand Down Expand Up @@ -439,6 +440,7 @@ public static function linkCollectionCampToContact(string $op, string $objectNam
$contactId = $currentCollectionCamp['Collection_Camp_Core_Details.Contact_Id'];
$collectionCampTitle = $currentCollectionCamp['title'];
$collectionCampId = $currentCollectionCamp['id'];
$collectionCampSubtype = $currentCollectionCamp['subtype:name'];

// Check for status change.
if ($currentStatus !== $newStatus) {
Expand Down Expand Up @@ -493,104 +495,28 @@ public static function generateCollectionCampQr(string $op, string $objectName,
}

$collectionCamps = EckEntity::get('Collection_Camp', TRUE)
->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id')
->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_Core_Details.Contact_Id', 'subtype:name')
->addWhere('id', '=', $objectId)
->execute();

$currentCollectionCamp = $collectionCamps->first();
$currentStatus = $currentCollectionCamp['Collection_Camp_Core_Details.Status'];
$collectionCampId = $currentCollectionCamp['id'];
$collectionCampSubtype = $currentCollectionCamp['subtype:name'];

if (empty($collectionCampSubtype)) {
\Civi::log()->warning('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId);
return;
}
Comment on lines +505 to +510
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor duplicated code for handling $collectionCampSubtype

The logic in lines 505-510 for retrieving and validating $collectionCampSubtype is duplicated in the reGenerateCollectionCampQr method (lines 547-552). To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider extracting this code into a reusable private method.

Apply this diff to refactor the duplicated code:

+ private static function getCollectionCampSubtype(int $collectionCampId) {
+     $collectionCamp = EckEntity::get('Collection_Camp', TRUE)
+         ->addSelect('subtype:name')
+         ->addWhere('id', '=', $collectionCampId)
+         ->execute()->single();
+     
+     $collectionCampSubtype = $collectionCamp['subtype:name'];
+     
+     if (empty($collectionCampSubtype)) {
+         \Civi::log()->warning('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId);
+         return null;
+     }
+     
+     return $collectionCampSubtype;
+ }

Then update the generateCollectionCampQr method:

-     $collectionCampSubtype = $currentCollectionCamp['subtype:name'];
-
-     if (empty($collectionCampSubtype)) {
-         \Civi::log()->warning('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId);
-         return;
-     }
+     $collectionCampSubtype = self::getCollectionCampSubtype($collectionCampId);
+     if ($collectionCampSubtype === null) {
+         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
$collectionCampSubtype = $currentCollectionCamp['subtype:name'];
if (empty($collectionCampSubtype)) {
\Civi::log()->warning('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId);
return;
}
private static function getCollectionCampSubtype(int $collectionCampId) {
$collectionCamp = EckEntity::get('Collection_Camp', TRUE)
->addSelect('subtype:name')
->addWhere('id', '=', $collectionCampId)
->execute()->single();
$collectionCampSubtype = $collectionCamp['subtype:name'];
if (empty($collectionCampSubtype)) {
\Civi::log()->warning('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId);
return null;
}
return $collectionCampSubtype;
}
$collectionCampSubtype = self::getCollectionCampSubtype($collectionCampId);
if ($collectionCampSubtype === null) {
return;
}


// Check for status change.
if ($currentStatus !== $newStatus) {
if ($newStatus === 'authorized') {
self::generateQrCode($collectionCampId);
QrCodeService::generateQrCode($collectionCampId, $collectionCampSubtype);
}
}
}

/**
*
*/
public static function generateQrCode($collectionCampId) {

try {
$baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL;
$url = "{$baseUrl}actions/collection-camp/{$collectionCampId}";

$options = new QROptions([
'version' => 5,
'outputType' => QRCode::OUTPUT_IMAGE_PNG,
'eccLevel' => QRCode::ECC_L,
'scale' => 10,
]);

$qrcode = (new QRCode($options))->render($url);

// Remove the base64 header and decode the image data.
$qrcode = str_replace('data:image/png;base64,', '', $qrcode);

$qrcode = base64_decode($qrcode);

$baseFileName = "qr_code_{$collectionCampId}.png";

$fileName = \CRM_Utils_File::makeFileName($baseFileName);

$tempFilePath = \CRM_Utils_File::tempnam($baseFileName);

$numBytes = file_put_contents($tempFilePath, $qrcode);

if (!$numBytes) {
\CRM_Core_Error::debug_log_message('Failed to write QR code to temporary file for collection camp ID ' . $collectionCampId);
return FALSE;
}

$customFields = CustomField::get(FALSE)
->addSelect('id')
->addWhere('custom_group_id:name', '=', 'Collection_Camp_QR_Code')
->addWhere('name', '=', 'QR_Code')
->setLimit(1)
->execute();

$qrField = $customFields->first();

if (!$qrField) {
\CRM_Core_Error::debug_log_message('No field to save QR Code for collection camp ID ' . $collectionCampId);
return FALSE;
}

$qrFieldId = 'custom_' . $qrField['id'];

// Save the QR code as an attachment linked to the collection camp.
$params = [
'entity_id' => $collectionCampId,
'name' => $fileName,
'mime_type' => 'image/png',
'field_name' => $qrFieldId,
'options' => [
'move-file' => $tempFilePath,
],
];

$result = civicrm_api3('Attachment', 'create', $params);

if (empty($result['id'])) {
\CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId);
return FALSE;
}

$attachment = $result['values'][$result['id']];

$attachmentUrl = $attachment['url'];
}
catch (\CiviCRM_API3_Exception $e) {
\CRM_Core_Error::debug_log_message('Error generating QR code: ' . $e->getMessage());
return FALSE;
}

return TRUE;
}

/**
* This hook is called after a db write on entities.
*
Expand All @@ -612,18 +538,24 @@ public static function reGenerateCollectionCampQr(string $op, string $objectName
try {
$collectionCampId = $objectRef->id;
$collectionCamp = EckEntity::get('Collection_Camp', TRUE)
->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_QR_Code.QR_Code')
->addSelect('Collection_Camp_Core_Details.Status', 'Collection_Camp_QR_Code.QR_Code', 'subtype:name')
->addWhere('id', '=', $collectionCampId)
->execute()->single();

$status = $collectionCamp['Collection_Camp_Core_Details.Status'];
$collectionCampQr = $collectionCamp['Collection_Camp_QR_Code.QR_Code'];
$collectionCampSubtype = $collectionCamp['subtype:name'];

if (empty($collectionCampSubtype)) {
\Civi::log()->error('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId);
return;
}
Comment on lines +547 to +552
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Eliminate code duplication by using a helper method

The code in lines 547-552 duplicates the logic for retrieving and checking $collectionCampSubtype. Refactoring this into a shared private method will enhance code readability and simplify maintenance.

Update the reGenerateCollectionCampQr method:

-     $collectionCampSubtype = $collectionCamp['subtype:name'];
-
-     if (empty($collectionCampSubtype)) {
-         \Civi::log()->error('Collection camp subtype is not set or is empty for Collection Camp ID: ' . $collectionCampId);
-         return;
-     }
+     $collectionCampSubtype = self::getCollectionCampSubtype($collectionCampId);
+     if ($collectionCampSubtype === null) {
+         return;
+     }

Ensure that the new helper method handles logging appropriately based on the context (e.g., use different log levels if necessary).

Committable suggestion was skipped due to low confidence.


if ($status !== 'authorized' || $collectionCampQr !== NULL) {
return;
}

self::generateQrCode($collectionCampId);
QrCodeService::generateQrCode($collectionCampId, $collectionCampSubtype);

}
catch (\Exception $e) {
Expand Down
109 changes: 109 additions & 0 deletions wp-content/civi-extensions/goonjcustom/Civi/QrCodeService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
<?php

namespace Civi;

use Civi\Api4\CustomField;
use Civi\Core\Service\AutoSubscriber;
use chillerlan\QRCode\QRCode;
use chillerlan\QRCode\QROptions;

/**
*
*/
class QrCodeService extends AutoSubscriber {
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Unnecessary Extension of AutoSubscriber

The QrCodeService class extends AutoSubscriber, but the getSubscribedEvents() method returns an empty array. Since the class isn't subscribing to any events, consider removing the extension to adhere to the Single Responsibility Principle and eliminate unnecessary inheritance.


const DROPPING_CENTER_URL_PATTERN = "%sactions/dropping-center/%s";
const COLLECTION_CAMP_URL_PATTERN = "%sactions/collection-camp/%s";

/**
*
*/
public static function getSubscribedEvents() {
return [];
}

/**
*
*/
public static function generateQrCode($collectionCampId, $collectionCampSubtype) {
try {
$baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL;

if ($collectionCampSubtype === 'Dropping_Center') {
$url = sprintf(self::DROPPING_CENTER_URL_PATTERN, $baseUrl, $collectionCampId);
}
else {
$url = sprintf(self::COLLECTION_CAMP_URL_PATTERN, $baseUrl, $collectionCampId);
}

$options = new QROptions([
'version' => 5,
'outputType' => QRCode::OUTPUT_IMAGE_PNG,
'eccLevel' => QRCode::ECC_L,
'scale' => 10,
]);

$qrcode = (new QRCode($options))->render($url);

// Remove the base64 header and decode the image data.
$qrcode = str_replace('data:image/png;base64,', '', $qrcode);
$qrcode = base64_decode($qrcode);

$baseFileName = "qr_code_{$collectionCampId}.png";
$fileName = \CRM_Utils_File::makeFileName($baseFileName);
$tempFilePath = \CRM_Utils_File::tempnam($baseFileName);

$numBytes = file_put_contents($tempFilePath, $qrcode);

if (!$numBytes) {
\CRM_Core_Error::debug_log_message('Failed to write QR code to temporary file for collection camp ID ' . $collectionCampId);
return FALSE;
}

$customFields = CustomField::get(FALSE)
->addSelect('id')
->addWhere('custom_group_id:name', '=', 'Collection_Camp_QR_Code')
->addWhere('name', '=', 'QR_Code')
->setLimit(1)
->execute();

$qrField = $customFields->first();

if (!$qrField) {
\CRM_Core_Error::debug_log_message('No field to save QR Code for collection camp ID ' . $collectionCampId);
return FALSE;
}

$qrFieldId = 'custom_' . $qrField['id'];

// Save the QR code as an attachment linked to the collection camp.
$params = [
'entity_id' => $collectionCampId,
'name' => $fileName,
'mime_type' => 'image/png',
'field_name' => $qrFieldId,
'options' => [
'move-file' => $tempFilePath,
],
];

$result = civicrm_api3('Attachment', 'create', $params);

if (!empty($result['is_error'])) {
\CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId);
return FALSE;
}
Comment on lines +92 to +95
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve Error Logging for API Failures

When the API call to create the attachment fails, the error message lacks specific details.

Modify the error logging to include the API error message:

if (!empty($result['is_error'])) {
- \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId);
+ \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId . '. Error: ' . $result['error_message']);
  return FALSE;
}

This provides clearer insight into the cause of the failure, aiding in faster debugging.

📝 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
if (!empty($result['is_error'])) {
\CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId);
return FALSE;
}
if (!empty($result['is_error'])) {
\CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId . '. Error: ' . $result['error_message']);
return FALSE;
}


$attachment = $result['values'][$result['id']];
$attachmentUrl = $attachment['url'];

}
catch (\Exception $e) {
\CRM_Core_Error::debug_log_message('Error generating QR code: ' . $e->getMessage());
return FALSE;
}

return TRUE;
}
Comment on lines +28 to +107
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Method generateQrCode Violates Single Responsibility Principle

The generateQrCode method is handling multiple responsibilities:

  • Generating the QR code.
  • Handling file operations (writing to a temp file).
  • Interacting with the database to retrieve custom fields.
  • Creating an attachment via the CiviCRM API.

Consider refactoring this method into smaller, dedicated methods for each responsibility. This will enhance readability, maintainability, and testability of the code.

Refactoring suggestions:

  • Extract QR Code Generation:

    private static function createQrCode($url) {
  $options = new QROptions([
    'version'    => 5,
    'outputType' => QRCode::OUTPUT_IMAGE_PNG,
    'eccLevel'   => QRCode::ECC_L,
    'scale'      => 10,
  ]);

  return (new QRCode($options))->render($url);
}

  • Extract File Handling:

    private static function saveQrCodeToFile($qrcodeData, $collectionCampId) {
  $baseFileName = "qr_code_{$collectionCampId}.png";
  $fileName = \CRM_Utils_File::makeFileName($baseFileName);
  $tempFilePath = \CRM_Utils_File::tempnam($baseFileName);

  $numBytes = file_put_contents($tempFilePath, $qrcodeData);

  if (!$numBytes) {
    \CRM_Core_Error::debug_log_message('Failed to write QR code to temporary file for collection camp ID ' . $collectionCampId);
    return FALSE;
  }

  return ['tempFilePath' => $tempFilePath, 'fileName' => $fileName];
}

  • Extract Custom Field Retrieval:

    private static function getQrCodeCustomFieldId() {
  $customFields = CustomField::get(FALSE)
    ->addSelect('id')
    ->addWhere('custom_group_id:name', '=', 'Collection_Camp_QR_Code')
    ->addWhere('name', '=', 'QR_Code')
    ->setLimit(1)
    ->execute();

  $qrField = $customFields->first();

  if (!$qrField) {
    return FALSE;
  }

  return 'custom_' . $qrField['id'];
}

  • Extract Attachment Creation:

    private static function attachQrCodeToCollectionCamp($collectionCampId, $fileName, $tempFilePath, $qrFieldId) {
  $params = [
    'entity_id' => $collectionCampId,
    'name' => $fileName,
    'mime_type' => 'image/png',
    'field_name' => $qrFieldId,
    'options' => [
      'move-file' => $tempFilePath,
    ],
  ];

  $result = civicrm_api3('Attachment', 'create', $params);

  if (!empty($result['is_error'])) {
    \CRM_Core_Error::debug_log_message('Failed to create attachment for collection camp ID ' . $collectionCampId . '. Error: ' . $result['error_message']);
    return FALSE;
  }

  return TRUE;
}

By breaking down the method, each part becomes more modular and easier to maintain.


}
40 changes: 34 additions & 6 deletions wp-content/plugins/goonj-blocks/build/render.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,40 @@
$action_target['id']
);

if ( in_array( $target, array( 'collection-camp', 'dropping-center' ) ) ) :
$start_date = new DateTime( $action_target['Collection_Camp_Intent_Details.Start_Date'] );
$end_date = new DateTime( $action_target['Collection_Camp_Intent_Details.End_Date'] );
$address = $action_target['Collection_Camp_Intent_Details.Location_Area_of_camp'];
$dropping_center_material_contribution_link = sprintf(
'/dropping-center-contribution?source=%s&target_id=%s',
$action_target['title'],
$action_target['id'],
);
Comment on lines +42 to +46
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor link generation to eliminate duplicate code

The link generation for 'dropping-center' and 'collection-camp' shares similar structures. To adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability, consider abstracting this logic into a reusable function.

You can create a function like this:

function generate_contribution_link($type, $action_target) {
    if ($type === 'collection-camp') {
        return sprintf(
            '/collection-camp-contribution?source=%s&target_id=%s&state_province_id=%s&city=%s',
            $action_target['title'],
            $action_target['id'],
            $action_target['Collection_Camp_Intent_Details.State'],
            $action_target['Collection_Camp_Intent_Details.City'],
        );
    } elseif ($type === 'dropping-center') {
        return sprintf(
            '/dropping-center-contribution?source=%s&target_id=%s',
            $action_target['title'],
            $action_target['id'],
        );
    }
    return '';
}

Then, update the assignments:

-$material_contribution_link = sprintf(
-    '/collection-camp-contribution?source=%s&target_id=%s&state_province_id=%s&city=%s',
-    $action_target['title'],
-    $action_target['id'],
-    $action_target['Collection_Camp_Intent_Details.State'],
-    $action_target['Collection_Camp_Intent_Details.City'],
-);
-
-$dropping_center_material_contribution_link = sprintf(
-    '/dropping-center-contribution?source=%s&target_id=%s',
-    $action_target['title'],
-    $action_target['id'],
-);
+ $contribution_link = generate_contribution_link($target, $action_target);

And adjust the $target_config accordingly.


?>
$target_config = [
'dropping-center' => [
'start_time' => 'Dropping_Centre.Start_Time',
'end_time' => 'Dropping_Centre.End_Time',
'address' => 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_',
'contribution_link' => $dropping_center_material_contribution_link,
],
'collection-camp' => [
'start_time' => 'Collection_Camp_Intent_Details.Start_Date',
'end_time' => 'Collection_Camp_Intent_Details.End_Date',
'address' => 'Collection_Camp_Intent_Details.Location_Area_of_camp',
'contribution_link' => $material_contribution_link,
],
];
if (isset($target_config[$target])) :
try {
$config = $target_config[$target];
$start_date = new DateTime($action_target[$config['start_time']]);
$end_date = new DateTime($action_target[$config['end_time']]);
$address = $action_target[$config['address']];
$contribution_link = $config['contribution_link'];
}
catch (Exception $e) {
\Civi::log()->error('Invalid date format for start or end time', ['error' => $e->getMessage(), 'target' => $target]);
echo '<div class="error">An error occurred. Please try again later.</div>';
return;
}
Comment on lines +62 to +74
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve exception handling to protect sensitive information

While the try-catch block effectively handles exceptions, logging the error message may expose sensitive information. Consider logging only essential details and providing a user-friendly message.

Apply this change to safeguard sensitive data:

catch (Exception $e) {
-  \Civi::log()->error('Invalid date format for start or end time', ['error' => $e->getMessage(), 'target' => $target]);
+  \Civi::log()->error('Invalid date format for start or end time', ['target' => $target]);
   echo '<div class="error">An error occurred. Please try again later.</div>';
   return;
}

This modification ensures that detailed exception messages aren't exposed, aligning with best security practices.

📝 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
if (isset($target_config[$target])) :
try {
$config = $target_config[$target];
$start_date = new DateTime($action_target[$config['start_time']]);
$end_date = new DateTime($action_target[$config['end_time']]);
$address = $action_target[$config['address']];
$contribution_link = $config['contribution_link'];
}
catch (Exception $e) {
\Civi::log()->error('Invalid date format for start or end time', ['error' => $e->getMessage(), 'target' => $target]);
echo '<div class="error">An error occurred. Please try again later.</div>';
return;
}
if (isset($target_config[$target])) :
try {
$config = $target_config[$target];
$start_date = new DateTime($action_target[$config['start_time']]);
$end_date = new DateTime($action_target[$config['end_time']]);
$address = $action_target[$config['address']];
$contribution_link = $config['contribution_link'];
}
catch (Exception $e) {
\Civi::log()->error('Invalid date format for start or end time', ['target' => $target]);
echo '<div class="error">An error occurred. Please try again later.</div>';
return;
}

?>
<div class="wp-block-gb-heading-wrapper">
<h2 class="wp-block-gb-heading"><?php echo esc_html($heading_text); ?></h2>
</div>
Expand All @@ -68,7 +96,7 @@
<a href="<?php echo esc_url( $register_link ); ?>" class="wp-block-gb-action-button">
<?php esc_html_e( 'Volunteer with Goonj', 'goonj-blocks' ); ?>
</a>
<a href="<?php echo esc_url( $material_contribution_link ); ?>" class="wp-block-gb-action-button">
<a href="<?php echo esc_url( $contribution_link ); ?>" class="wp-block-gb-action-button">
<?php esc_html_e( 'Record your Material Contribution', 'goonj-blocks' ); ?>
</a>
</div>
Expand Down
3 changes: 3 additions & 0 deletions wp-content/plugins/goonj-blocks/goonj-blocks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ function gb_goonj_blocks_check_action_target_exists() {
'Collection_Camp_Intent_Details.Location_Area_of_camp',
'Collection_Camp_Intent_Details.City',
'Collection_Camp_Intent_Details.State',
'Dropping_Centre.Start_Time',
'Dropping_Centre.End_Time',
'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_',
);

switch ( $target ) {
Expand Down
40 changes: 34 additions & 6 deletions wp-content/plugins/goonj-blocks/src/render.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,40 @@
$action_target['id']
);

if ( in_array( $target, array( 'collection-camp', 'dropping-center' ) ) ) :
$start_date = new DateTime( $action_target['Collection_Camp_Intent_Details.Start_Date'] );
$end_date = new DateTime( $action_target['Collection_Camp_Intent_Details.End_Date'] );
$address = $action_target['Collection_Camp_Intent_Details.Location_Area_of_camp'];
$dropping_center_material_contribution_link = sprintf(
'/dropping-center-contribution?source=%s&target_id=%s',
$action_target['title'],
$action_target['id'],
);

?>
$target_config = [
'dropping-center' => [
'start_time' => 'Dropping_Centre.Start_Time',
'end_time' => 'Dropping_Centre.End_Time',
'address' => 'Dropping_Centre.Where_do_you_wish_to_open_dropping_center_Address_',
'contribution_link' => $dropping_center_material_contribution_link,
],
'collection-camp' => [
'start_time' => 'Collection_Camp_Intent_Details.Start_Date',
'end_time' => 'Collection_Camp_Intent_Details.End_Date',
'address' => 'Collection_Camp_Intent_Details.Location_Area_of_camp',
'contribution_link' => $material_contribution_link,
],
];
Comment on lines +48 to +61
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider including 'processing-center' in $target_config to reduce code duplication

The current implementation handles 'dropping-center' and 'collection-camp' using the $target_config array, but 'processing-center' is handled separately. To adhere to the DRY (Don't Repeat Yourself) principle and enhance maintainability, consider adding 'processing-center' to $target_config. This unifies the logic and simplifies future extensions.

Here's a suggested change:

     ],
     'collection-camp' => [
       'start_time' => 'Collection_Camp_Intent_Details.Start_Date',
       'end_time' => 'Collection_Camp_Intent_Details.End_Date',
       'address' => 'Collection_Camp_Intent_Details.Location_Area_of_camp',
       'contribution_link' => $material_contribution_link,
     ],
+    'processing-center' => [
+      'address' => $action_target['address'],
+      'visit_link' => $pu_visit_check_link,
+      'contribution_link' => $pu_material_contribution_check_link,
+    ],
   ];

Adjust the subsequent code to utilize the $config array for 'processing-center' as well. This might include handling the absence of start and end dates for 'processing-center' or adding default values where necessary.

Committable suggestion was skipped due to low confidence.

if (isset($target_config[$target])) :
try {
$config = $target_config[$target];
$start_date = new DateTime($action_target[$config['start_time']]);
$end_date = new DateTime($action_target[$config['end_time']]);
$address = $action_target[$config['address']];
$contribution_link = $config['contribution_link'];
}
catch (Exception $e) {
\Civi::log()->error('Invalid date format for start or end time', ['error' => $e->getMessage(), 'target' => $target]);
echo '<div class="error">An error occurred. Please try again later.</div>';
return;
}
Comment on lines +63 to +74
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove unnecessary error handling around DateTime creation

Previous team discussions concluded that error handling for DateTime object creation is unnecessary when using $action_target data. The try-catch block may be redundant, as the dates are expected to be valid. Removing it simplifies the code and aligns with team conventions.

Apply this diff to remove the try-catch block:

-if (isset($target_config[$target])) :
-  try {
     $config = $target_config[$target];
     $start_date = new DateTime($action_target[$config['start_time']]);
     $end_date = new DateTime($action_target[$config['end_time']]);
     $address = $action_target[$config['address']];
     $contribution_link = $config['contribution_link'];
-  }
-  catch (Exception $e) {
-    \Civi::log()->error('Invalid date format for start or end time', ['error' => $e->getMessage(), 'target' => $target]);
-    echo '<div class="error">An error occurred. Please try again later.</div>';
-    return;
-  }	
+if (isset($target_config[$target])) :
+  $config = $target_config[$target];
+  $start_date = new DateTime($action_target[$config['start_time']]);
+  $end_date = new DateTime($action_target[$config['end_time']]);
+  $address = $action_target[$config['address']];
+  $contribution_link = $config['contribution_link'];

This change removes the unnecessary error handling, streamlining the code.

Committable suggestion was skipped due to low confidence.

?>
<div class="wp-block-gb-heading-wrapper">
<h2 class="wp-block-gb-heading"><?php echo esc_html($heading_text); ?></h2>
</div>
Expand All @@ -68,7 +96,7 @@
<a href="<?php echo esc_url( $register_link ); ?>" class="wp-block-gb-action-button">
<?php esc_html_e( 'Volunteer with Goonj', 'goonj-blocks' ); ?>
</a>
<a href="<?php echo esc_url( $material_contribution_link ); ?>" class="wp-block-gb-action-button">
<a href="<?php echo esc_url( $contribution_link ); ?>" class="wp-block-gb-action-button">
<?php esc_html_e( 'Record your Material Contribution', 'goonj-blocks' ); ?>
</a>
</div>
Expand Down