Code refactoring

CHANGELOG.md

@@ -1,4 +1,9 @@
 # Changelog
+## 3.0.5 - 2023-07-16
+ - Fix: SQL error on donors list when SQL modes is set to `ONLY_FULL_GROUP_BY` 
+ - Change: Remove JS cdata and type (thanks to cabot)
+ - Code refactoring
+
 ## 3.0.4 - 2021-04-20
  - Fix: Merchant ID does not match
  - Fix: Undefined index on transactions module
@@ -50,7 +55,7 @@
 
 ## 2.1.2 - 2019-11-19
  - Change: Temporarily disable TLS check because PayPal TLS website is down 
- - Change: Add `payer_donated_amount` on `donors_group_user_add()` event (thanks Dark❶)
+ - Change: Add `payer_donated_amount` on `donors_group_user_add()` event (thanks to Dark❶)
  - Fix: Auto group feature does not work (#75)
  - Fix: SQL Error when accessing on list of donors (#78)
  - Fix: English wording 
@@ -62,21 +67,21 @@
 
 ## 2.1.0 - 2019-05-09
  - Add: Donation on error can be manually approved
- - Add: Allow changing transactions donor (thanks kasimi)
- - Change: Make donor name a link to the profile when viewing a transaction (#61) (thanks kasimi)
- - Change: Use built-in phpbb_email_hash() function (#59) (thanks kasimi)
+ - Add: Allow changing transactions donor (thanks to kasimi)
+ - Change: Make donor name a link to the profile when viewing a transaction (#61) (thanks to kasimi)
+ - Change: Use built-in phpbb_email_hash() function (#59) (thanks to kasimi)
  - Fix: All transactions log are cleared when "Delete marked" is used
  - Fix: Allow modification of PayPal remote hosts
  - Fix: Template system returns error when multiple styles are enabled
- - Fix: Use square brackets for array access (#62) (thanks kasimi)
- - Fix: Use singular form of Donor (#60) (thanks kasimi)
+ - Fix: Use square brackets for array access (#62) (thanks to kasimi)
+ - Fix: Use singular form of Donor (#60) (thanks to kasimi)
  - Major code improvement/cleanup
 
 ## 2.0.1 - 2018-10-22
  - Add: PayPal Postdata check and error tracking
  - Add: Memo field in transactions log view
  - Add: Multiple checks on PayPal returned variables
- - Change: Enhance CSS compatibility with other styles (thanks Mazeltof)
+ - Change: Enhance CSS compatibility with other styles (thanks to Mazeltof)
  - Change: Improve log error
  - Change: Refactor IPN Listener
  - Change: Use the same name for the extension display name and the contribution in the phpBB CDB
@@ -111,15 +116,15 @@
  - Fix: "ppde_first_start" not set properly after first start of PPDE
  - Fix: Fails to get extension metadata after upgrading to phpBB 3.2.1 (#47)
  - Fix: Some SQL result wasn't freed
- - Fix: English wording (thanks kasimi)
+ - Fix: English wording (thanks to kasimi)
  - Fix: Unable to use "delete all" in Transactions Log module
  - Remove: Extension version check removed from the Overview module
  - Remove: fsockopen related code
  - Translation: Update Transifex config and Readme files
  - Code improvement
 
 ## 1.0.3 - 2017-01-22
- - Add: Add extension events. More information in `/docs/events.md` (Thanks kasimi)
+ - Add: Add extension events. More information in `/docs/events.md` (thanks to kasimi)
  - Add: The default donation value becomes the default value in the dropdown list. If it's not present, the value is added to the dropdown list
  - Change: PPDE links moved before the link "FAQ" in the header navbar
  - Change: Remove unnecessary input and label attributes
@@ -145,8 +150,8 @@
  - Fix: "Sandbox only for founder" always displayed as enabled even if it was disabled
  - Fix: Use `is_set()` method in `$request` instead of use `!isset()` on Super Global
  - Fix: Wrong value on xHTML `<input>` `disabled` attribute
- - Change: Add the type of parameter into the method declaration (Thanks ErnadoO)
- - Change: Hide/Show ACP IPN Features using jQuery (Thanks cabot)
+ - Change: Add the type of parameter into the method declaration (thanks to ErnadoO)
+ - Change: Hide/Show ACP IPN Features using jQuery (thanks to cabot)
 
 ## 1.0.1 - 2016-05-09
  - Fix: IN_PHPBB is not defined in `/skouat/ppde/controller/ipn_listener.php`

actions/post_data.php

@@ -110,14 +110,13 @@ public function check_post_data($data_ary = []): array
 
  /**
  * Check requirements for data value.
- * If a check fails, error message are stored in $this->error_message
+ * If a check fails, error messages are stored in $this->error_message
  *
  * @param array $data_ary
  *
  * @return array
  * @access public
  */
-
  public function call_func($data_ary): array
  {
  $check = [];

controller/admin/admin_main.php

@@ -14,11 +14,13 @@
 
 abstract class admin_main
 {
+ protected const SECONDS_IN_A_DAY = 86400;
+
  /** @var array */
  protected $args = [];
- /** @var object \phpbb\config\config */
+ /** @var \phpbb\config\config */
  protected $config;
- /** @var object Symfony\Component\DependencyInjection\ContainerInterface */
+ /** @var Symfony\Component\DependencyInjection\ContainerInterface */
  protected $container;
  /** @var string */
  protected $id_prefix_name;

controller/admin/overview_controller.php

@@ -303,6 +303,6 @@ private function per_day_stats($config_name): string
  */
  private function get_install_days()
  {
- return (float) (time() - $this->config['ppde_install_date']) / 86400;
+ return (float) (time() - $this->config['ppde_install_date']) / self::SECONDS_IN_A_DAY;
  }
 }

controller/admin/transactions_controller.php

@@ -168,7 +168,7 @@ public function display(): void
  gen_sort_selects($limit_days, $sort_by_text, $this->args['hidden_fields']['st'], $this->args['hidden_fields']['sk'], $this->args['hidden_fields']['sd'], $s_limit_days, $s_sort_key, $s_sort_dir, $u_sort_param);
 
  // Define where and sort sql for use in displaying transactions
- $sql_where = ($this->args['hidden_fields']['st']) ? (time() - ($this->args['hidden_fields']['st'] * 86400)) : 0;
+ $sql_where = ($this->args['hidden_fields']['st']) ? (time() - ($this->args['hidden_fields']['st'] * self::SECONDS_IN_A_DAY)) : 0;
  $sql_sort = $sort_by_sql[$this->args['hidden_fields']['sk']] . ' ' . (($this->args['hidden_fields']['sd'] === 'd') ? 'DESC' : 'ASC');
 
  $keywords = $this->request->variable('keywords', '', true);
@@ -390,13 +390,13 @@ public function change(): void
  * @param string $username
  * @param int $donor_id
  *
- * @return int
- * @throws transaction_exception
+ * @return int returns user_id
+ * @throws transaction_exception if the user_id is less than or equal to the default value for ANONYMOUS.
  * @access private
  */
  private function validate_user_id($username, $donor_id = 0): int
  {
- if (($username === '') && ($donor_id === ANONYMOUS || $this->request->is_set('u')))
+ if ($this->should_return_anonymous($username, $donor_id))
  {
  return ANONYMOUS;
  }
@@ -411,6 +411,20 @@ private function validate_user_id($username, $donor_id = 0): int
  return $user_id;
  }
 
+ /**
+ * Determines if the given username and donor ID should result in an anonymous response.
+ *
+ * @param string $username The username to check.
+ * @param int $donor_id The donor ID to check.
+ * @return bool Returns true if the username is empty and either the donor ID is ANONYMOUS or the 'u' parameter is
+ * set in the URL. Otherwise, returns false.
+ */
+ private function should_return_anonymous(string $username, int $donor_id): bool
+ {
+ // if the username is empty and (donor_id is ANONYMOUS or 'u' parameter is set in URL),
+ return $username === '' && ($donor_id === ANONYMOUS || $this->request->is_set('u'));
+ }
+
  public function approve(): void
  {
  $transaction_id = (int) $this->args['hidden_fields']['id'];
@@ -467,23 +481,7 @@ public function add(): void
 
  if ($this->request->is_set_post('submit'))
  {
- try
- {
- $this->ppde_actions->log_to_db($this->build_data_ary($transaction_data));
-
- // Prepare transaction settings before doing actions
- $this->ppde_actions->set_transaction_data($transaction_data);
- $this->ppde_actions->is_donor_is_member();
-
- $this->do_transactions_actions($this->ppde_actions->get_donor_is_member() && !$transaction_data['MT_ANONYMOUS']);
-
- $this->log->add('admin', $this->user->data['user_id'], $this->user->ip, 'LOG_PPDE_MT_ADDED', time(), [$transaction_data['MT_USERNAME']]);
- trigger_error($this->language->lang('PPDE_MT_ADDED') . adm_back_link($this->u_action));
- }
- catch (transaction_exception $e)
- {
- $errors = $e->get_errors();
- }
+ $errors = $this->process_transaction($transaction_data, $errors);
  }
 
  $this->ppde_actions_currency->build_currency_select_menu((int) $this->config['ppde_default_currency']);
@@ -502,6 +500,50 @@ public function add(): void
  ]);
  }
 
+ /**
+ * Process a transaction with the given transaction data and handle any errors that occur.
+ *
+ * @param array $transaction_data The data for the transaction.
+ * @param array $errors The array to store any errors that occur during processing.
+ *
+ * @return array The updated array of errors after processing the transaction.
+ */
+ private function process_transaction(array $transaction_data, array $errors): array
+ {
+ try
+ {
+ $this->ppde_actions->log_to_db($this->build_data_ary($transaction_data));
+
+ // Prepare transaction settings before doing actions
+ $this->ppde_actions->set_transaction_data($transaction_data);
+ $this->ppde_actions->is_donor_is_member();
+
+ $this->do_transactions_actions($this->ppde_actions->get_donor_is_member() && !$transaction_data['MT_ANONYMOUS']);
+
+ $this->log_transaction($transaction_data);
+ }
+ catch (transaction_exception $e)
+ {
+ $errors = $e->get_errors();
+ }
+
+ return $errors;
+ }
+
+ /**
+ * Logs an entry in the phpBB admin log.
+ *
+ * @param array $transaction_data The data of the transaction to be logged.
+ *
+ * @return void
+ * @access private
+ */
+ private function log_transaction(array $transaction_data): void
+ {
+ $this->log->add('admin', $this->user->data['user_id'], $this->user->ip, 'LOG_PPDE_MT_ADDED', time(), [$transaction_data['MT_USERNAME']]);
+ trigger_error($this->language->lang('PPDE_MT_ADDED') . adm_back_link($this->u_action));
+ }
+
  /**
  * Returns requested data from manual transaction form
  *
@@ -517,9 +559,9 @@ private function request_transaction_vars(): array
  'MT_LAST_NAME' => $this->request->variable('last_name', '', true),
  'MT_PAYER_EMAIL' => $this->request->variable('payer_email', '', true),
  'MT_RESIDENCE_COUNTRY' => $this->request->variable('residence_country', ''),
- 'MT_MC_GROSS' => $this->request->variable('mc_gross', (float) 0),
+ 'MT_MC_GROSS' => $this->request->variable('mc_gross', 0.0),
  'MT_MC_CURRENCY' => $this->request->variable('mc_currency', ''),
- 'MT_MC_FEE' => $this->request->variable('mc_fee', (float) 0),
+ 'MT_MC_FEE' => $this->request->variable('mc_fee', 0.0),
  'MT_PAYMENT_DATE_YEAR' => $this->request->variable('payment_date_year', (int) $this->user->format_date(time(), 'Y')),
  'MT_PAYMENT_DATE_MONTH' => $this->request->variable('payment_date_month', (int) $this->user->format_date(time(), 'n')),
  'MT_PAYMENT_DATE_DAY' => $this->request->variable('payment_date_day', (int) $this->user->format_date(time(), 'j')),
@@ -801,7 +843,7 @@ public function delete(): void
  * @return void
  * @access protected
  */
- protected function display_log_assign_template_vars($row): void
+ protected function display_log_assign_template_vars(array $row): void
  {
  $this->template->assign_block_vars('log', [
  'CONFIRMED' => ($row['confirmed']) ? $this->language->lang('PPDE_DT_VERIFIED') : $this->language->lang('PPDE_DT_UNVERIFIED'),
@@ -818,14 +860,14 @@ protected function display_log_assign_template_vars($row): void
  }
 
  /**
- * Set output vars for display in the template
+ * Assigns action template variables
  *
- * @param array $data
+ * @param array $data The data array containing the necessary information for assigning template variables.
  *
  * @return void
  * @access protected
  */
- protected function action_assign_template_vars($data)
+ protected function action_assign_template_vars(array $data): void
  {
  $s_hidden_fields = build_hidden_fields([
  'id' => $data['transaction_id'],
@@ -858,7 +900,7 @@ protected function action_assign_template_vars($data)
 
  'L_PPDE_DT_SETTLE_AMOUNT' => $this->language->lang('PPDE_DT_SETTLE_AMOUNT', $data['settle_currency']),
  'L_PPDE_DT_EXCHANGE_RATE_EXPLAIN' => $this->language->lang('PPDE_DT_EXCHANGE_RATE_EXPLAIN', $this->user->format_date($data['payment_date'])),
- 'S_CONVERT' => !($data['settle_amount'] == 0 && empty($data['exchange_rate'])),
+ 'S_CONVERT' => !((int) $data['settle_amount'] === 0 && empty($data['exchange_rate'])),
  'S_ERROR' => !empty($data['txn_errors']),
  'S_ERROR_APPROVED' => !empty($data['txn_errors_approved']),
  'S_HIDDEN_FIELDS' => $s_hidden_fields,

controller/ipn_listener.php

@@ -330,13 +330,40 @@ private function handle_post_data(): void
  */
  private function check_account_id(): void
  {
- $account_value = !empty($this->transaction_data['test_ipn']) ? $this->config['ppde_sandbox_address'] : $this->config['ppde_account_id'];
- if (strtoupper($account_value) !== strtoupper($this->transaction_data['receiver_id']) && strtolower($account_value) !== strtolower($this->transaction_data['receiver_email']))
+ if ($this->is_invalid_txn_account_id())
  {
  $this->transaction_data['txn_errors'] .= '<br>' . $this->language->lang('INVALID_TXN_ACCOUNT_ID');
  }
  }
 
+ /**
+ * Checks if the transaction account ID is invalid.
+ *
+ * @return bool Returns true if the transaction account ID is invalid, false otherwise.
+ */
+ private function is_invalid_txn_account_id(): bool
+ {
+ $account_value_lower = strtolower($this->get_account_value());
+
+ return !in_array($account_value_lower, [
+ strtolower($this->transaction_data['receiver_id']),
+ strtolower($this->transaction_data['receiver_email']),
+ strtolower($this->transaction_data['business']),
+ ], true);
+ }
+
+ /**
+ * Get the account value depending on whether the transaction is an IPN test or not..
+ *
+ * @return string The account value.
+ */
+ private function get_account_value(): string
+ {
+ return !empty($this->transaction_data['test_ipn'])
+ ? $this->config['ppde_sandbox_address']
+ : $this->config['ppde_account_id'];
+ }
+
  /**
  * Check response returned by PayPal et log errors if there is no valid response
  * Set true if response is 'VERIFIED'. In other case set to false and log errors
@@ -459,7 +486,7 @@ private function do_actions(): void
  * Event that is triggered when a transaction has been successfully completed
  *
  * @event skouat.ppde.do_actions_completed_before
- * @var array  transaction_data Array containing transaction data
+ * @var array $vars ['transaction_data'] Array containing transaction data
  * @since 1.0.3
  */
  $vars = [