Add tests & migrate to valid HTML5#544
Conversation
WalkthroughThis pull request introduces extensive changes across multiple components of the application. The CI workflow has been streamlined by removing the dedicated MySQL setup step and adding system dependency installations, Java availability checks, and setup for the v.Nu HTML validator. Configuration files have been updated, including disabling model auto-loading. Several controllers and views have been refactored, primarily replacing deprecated ID attributes with class selectors for pagination and accessibility improvements. A new library, Install_info, centralizes installation environment checks previously implemented as private controller methods. A patch for PHP 8.4 compatibility has been added, and numerous new test suites and test utilities have been introduced to enhance coverage and reliability. Changes
Sequence Diagram(s)Install Process with Install_info LibrarysequenceDiagram
participant Client
participant InstallController
participant InstallInfoLibrary
Client->>InstallController: Request installation process
InstallController->>InstallInfoLibrary: is_windows()
InstallController->>InstallInfoLibrary: get_daemon_path("daemon")
InstallController->>InstallInfoLibrary: get_daemon_var_path(...), daemon_var_path_exists(...), get_daemon_url(...)
InstallController->>InstallInfoLibrary: get_htaccess_CI_ENV_path()
InstallController->>InstallInfoLibrary: get_CI_ENV()
InstallInfoLibrary-->>InstallController: Return installation info
InstallController-->>Client: Respond with updated configuration
CI Workflow TriggersequenceDiagram
participant Repo
participant GitHubActions
participant System
participant Composer
participant PHPUnit
Repo->>GitHubActions: Push commit
GitHubActions->>System: Install system dependencies (libdbd-sqlite3, gammu-smsd)
GitHubActions->>System: Check and install Java if needed
GitHubActions->>Composer: Install composer dependencies (with renamed step)
GitHubActions->>System: Download and setup v.Nu tool
GitHubActions->>PHPUnit: Run tests with HTML5 output
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🔭 Outside diff range comments (1)
application/views/main/password_reset.php (1)
1-72: 💡 Verification agent🧩 Analysis chain
Consider adding CSRF protection to form.
While transitioning to modern HTML, it would be beneficial to ensure all forms have CSRF protection.
🌐 Web query:
How to implement CSRF protection in CodeIgniter forms?💡 Result:
To implement CSRF protection in CodeIgniter forms, follow these steps:
Enable CSRF Protection
Activate CSRF globally by modifying
app/Config/App.php(CodeIgniter 4) orapplication/config/config.php(CodeIgniter 3):// CodeIgniter 4 public $CSRFProtection = 'session'; public $CSRFTokenName = 'csrf_token'; public $CSRFRegenerate = true; // CodeIgniter 3 $config['csrf_protection'] = TRUE; $config['csrf_token_name'] = 'csrf_token'; $config['csrf_regenerate'] = TRUE; [2][3][8]Form Implementation
Using Form Helper (Recommended):
<?= form_open('/submit', ['csrf' => true]) ?> <!-- Form fields --> <?= form_close() ?>This automatically injects a hidden CSRF token field[2][3][8].
Manual Implementation:
<input type="hidden" name="<?= esc($this->security->get_csrf_token_name()) ?>" value="<?= esc($this->security->get_csrf_hash()) ?>"> [8][9]Use
esc()to prevent XSS vulnerabilities[1].AJAX Handling
Include CSRF tokens in request headers:
// Get token from cookie const csrfToken = document.cookie.match(/csrf_cookie=([^;]+)/)[1]; // Attach to headers fetch('/submit', { method: 'POST', headers: { 'X-CSRF-TOKEN': csrfToken, 'Content-Type': 'application/json' } }); [2][10]Key Configuration Options
CSRFRegenerate: Renew token after each submission (prevents replay attacks)CSRFExcludeUris: Bypass protection for specific routes like APIsCSRFExpire: Token lifetime (default 7200 seconds)[1][5][7]Validation Workflow
CodeIgniter automatically:
- Generates unique token per session
- Validates token on non-GET requests
- Rejects mismatched tokens with security exception[5][6][7]
For API endpoints, consider disabling automatic redirects:
public $CSRFRedirect = false; [2]Security Enhancements
- Double Cookie Submit: Verify token in both form data and cookie
- Per-Form Tokens: Generate unique tokens for critical forms
- SameSite Cookies: Set
Laxpolicy for cross-origin protection[1][6]Example error scenario: Submitting a form with an altered token triggers a security exception and logs the attempt[2][7].
Citations:
- 1: https://app.studyraid.com/en/read/12370/399366/csrf-protection-implementation
- 2: https://www.webniraj.com/2014/01/12/codeigniter-using-csrf-tokens-to-secure-your-application/
- 3: https://forum.codeigniter.com/showthread.php?tid=83584
- 4: https://www.youtube.com/watch?v=P1oBRPOXxbE
- 5: https://clouddevs.com/codeigniter/csrf-token/
- 6: https://clouddevs.com/codeigniter/csrf-protection/
- 7: https://www.codeigniter.com/userguide3/libraries/security.html?highlight=csrf
- 8: https://jenssegers.com/codeigniter-2-csrf-protection
- 9: https://forum.codeigniter.com/thread-65236.html
- 10: https://www.youtube.com/watch?v=otUyA5rK2Og
Action Required: Add CSRF Protection to the Password Reset Form
It's important to ensure that all forms, including the password reset form in
application/views/main/password_reset.php, are protected against CSRF attacks. CodeIgniter offers built‑in support for this. For example, you can either:
- Enable automatic CSRF token injection by updating the form helper call (if your CodeIgniter version supports it):
- <?php echo form_open('login/password_reset'); ?> + <?php echo form_open('login/password_reset', ['csrf' => true]); ?>- Or manually include the CSRF token by adding a hidden input just after opening the form:
<input type="hidden" name="<?= esc($this->security->get_csrf_token_name()) ?>" value="<?= esc($this->security->get_csrf_hash()) ?>">Also, double-check that CSRF protection is enabled in your configuration (typically
application/config/config.phpfor CodeIgniter 3 orapp/Config/App.phpfor CodeIgniter 4).
🧹 Nitpick comments (94)
application/views/js_init/phonebook/js_group.php (1)
8-8: Good migration to HTML5 data attributes but variable naming needs attention.The change correctly updates the attribute from
publictodata-public, which aligns with HTML5 standards for custom data attributes. However,publicis a reserved word in JavaScript that shouldn't be used as a variable name.Consider renaming the variable to avoid using a JavaScript reserved keyword:
-var public = $(this).parents("tr:first").attr("data-public"); +var isPublic = $(this).parents("tr:first").attr("data-public");And update the corresponding usage:
-if (public == "true") $("input#is_public").prop('checked', true); +if (isPublic == "true") $("input#is_public").prop('checked', true);tests/plugins/Stop_manager_test.php (2)
19-26: Remove or explain commented-out code.The setUp() method contains a commented-out line that isn't providing any value. Consider either removing it entirely or adding a comment explaining why it's being kept for future reference.
public function setUp() : void { if (file_exists(FCPATH . 'install')) { unlink(FCPATH . 'install'); } - //DBSetup::setup_db_kalkun_testing2($this); }
38-67: Add PHPDoc to test method.Adding a detailed PHPDoc comment to the test method would provide better context about what functionality of the stop_manager plugin is being tested.
/** * @dataProvider database_Provider + * @param string $db_engine The database engine to test against + * @return void + * @brief Tests the basic functionality of the stop_manager plugin index page */ public function test_index($db_engine)tests/plugins/Sms_to_email_test.php (5)
25-25: Remove commented-out code.The commented-out line calling
setup_db_kalkun_testing2appears to be development code that's no longer needed. Commented-out code can confuse maintainers about its relevance or purpose.- //DBSetup::setup_db_kalkun_testing2($this);
28-31: Remove unused commented-out method.The entire
reloadCIwithEnginemethod is commented out and appears to be unused. Removing it would improve code cleanliness.- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/
33-36: Consider enhancing the database provider with descriptive test names.The current implementation returns database engines without descriptive test names, which could make test failures harder to diagnose.
public static function database_Provider() { - return DBSetup::$db_engines_to_test; + $result = []; + foreach (DBSetup::$db_engines_to_test as $engine) { + $result['With ' . $engine[0] . ' database'] = $engine; + } + return $result; }
47-47: Consider making the configuration setup more explicit.The string 'gammu_no_pbk_kalkun_fresh_install_manual_sql_injection' is cryptic without context and may be difficult for future maintainers to understand.
Consider either:
- Adding a comment explaining what this configuration represents
- Using a more descriptive constant or named parameter
- $dbsetup->setup_config('gammu_no_pbk_kalkun_fresh_install_manual_sql_injection'); + // Set up a fresh Gammu installation without phonebook and with manual SQL setup + $dbsetup->setup_config('gammu_no_pbk_kalkun_fresh_install_manual_sql_injection');
50-53: Consider refactoring the session check.The condition
session_status() === PHP_SESSION_NONE && is_cli() === FALSEcould be simplified, and the explicit comparison withFALSEis not necessary in PHP.- if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) + if (session_status() === PHP_SESSION_NONE && !is_cli())tests/plugins/Whitelist_number_test.php (4)
19-26: Remove or implement the commented-out code.There's commented-out code on line 25 that appears to be from development. Either implement it if needed or remove it to keep the code clean.
- //DBSetup::setup_db_kalkun_testing2($this);
28-31: Remove commented-out method for cleaner code.This entire commented-out method
reloadCIwithEngineisn't being used. It's better to remove unused code to maintain cleanliness and reduce confusion.- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/
65-65: Add a descriptive message to the HTML validation assertion.Adding a descriptive message to the assertion would make test failures more informative and easier to debug.
- $this->assertValidHtml($output); + $this->assertValidHtml($output, 'The whitelist_number plugin should return valid HTML');
48-59: Consider extracting the session setup to a reusable helper method.The session setup code is likely to be reused across multiple test classes. Consider extracting it to a helper method in the parent test class or a utility class for better reusability and maintainability.
You could create a method like
setupAdminSession()in the parent class or a utility class that handles this common setup task.tests/plugins/Sms_to_xmpp_test.php (2)
25-25: Remove or implement the commented-out code.There's a commented-out line for database setup. Consider either removing it entirely or implementing it if it's needed.
- //DBSetup::setup_db_kalkun_testing2($this);
28-31: Remove or implement the commented-out method.The
reloadCIwithEnginemethod is commented out but still present in the code. For better code maintenance and readability, either implement this method if it's needed or remove it completely.- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/tests/plugins/Sms_to_twitter_test.php (6)
15-15: Fix typo in the imported file name.There appears to be a typo in the file name. The correct file name should be
Plugins_test.phpwith a single 's', notPluginss_test.phpwith a double 's'.-require_once __DIR__.'/../controllers/Pluginss_test.php'; +require_once __DIR__.'/../controllers/Plugins_test.php';
25-26: Remove commented-out code.The commented-out code should be removed to improve code maintainability and readability.
- //DBSetup::setup_db_kalkun_testing2($this);
28-31: Remove unused commented-out method.This commented-out method is not used and should be removed to keep the codebase clean.
- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/
66-66: Complete the implementation of this test.The test is currently marked as incomplete with
$this->markTestIncomplete(). This indicates that the test is a placeholder and needs further implementation. Consider adding specific assertions to verify the plugin's functionality beyond just checking valid HTML.Would you like me to help expand this test with specific assertions to verify the SMS to Twitter plugin's functionality? I can suggest some assertions based on what behaviors should be tested.
48-59: Consider moving session setup to a reusable utility method.The session setup logic might be needed in other test cases. Consider extracting it to a utility method in the parent class or a helper class to promote code reuse and maintainability.
Consider creating a helper method in the parent class:
// In KalkunTestCase.php protected function setupAdminSession($username = 'kalkun', $user_id = '1') { $this->request->setCallablePreConstructor( function () use ($username, $user_id) { if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) { session_start(); } $_SESSION['loggedin'] = 'TRUE'; $_SESSION['id_user'] = $user_id; $_SESSION['level'] = 'admin'; $_SESSION['username'] = $username; } ); }Then simplify your test:
-$this->request->setCallablePreConstructor( - function () { - if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) - { - session_start(); - } - $_SESSION['loggedin'] = 'TRUE'; - $_SESSION['id_user'] = '1'; - $_SESSION['level'] = 'admin'; - $_SESSION['username'] = 'kalkun'; - } -); +$this->setupAdminSession();
1-68: Add explanatory comments about test purpose and approach.The test class would benefit from additional documentation explaining:
- What the SMS to Twitter plugin does
- What specific functionality this test is verifying
- The approach used in testing (database setup, session simulation, etc.)
This would make it easier for other developers to understand the test's purpose and maintain it.
Add documentation below the class declaration:
class Sms_to_twitter_test extends KalkunTestCase { + /** + * This test class verifies the SMS to Twitter plugin functionality. + * The plugin allows sending SMS messages to Twitter as tweets. + * + * Testing approach: + * 1. Set up a fresh database installation + * 2. Simulate an admin user session + * 3. Install the SMS to Twitter plugin + * 4. Verify the plugin's interface renders correctly + * 5. TODO: Add tests for actual tweet functionality + */tests/plugins/Sms_member_test.php (4)
19-26: Consider adding more setup or removing commented codeThe
setUpmethod appropriately removes any installation file to ensure a clean test environment. However, there's commented-out code that should either be implemented or removed.public function setUp() : void { if (file_exists(FCPATH . 'install')) { unlink(FCPATH . 'install'); } - //DBSetup::setup_db_kalkun_testing2($this); }
28-31: Remove commented-out codeThe commented-out
reloadCIwithEnginemethod is not being used. It's best practice to remove commented-out code blocks rather than commit them.-/*public function reloadCIwithEngine($db_engine) -{ - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); -}*/
48-59: Consider extracting session setup to a helper methodThe session setup is quite verbose and might be reused across multiple test classes. Consider extracting this to a helper method in your base test class or a separate utility.
-$this->request->setCallablePreConstructor( - function () { - if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) - { - session_start(); - } - $_SESSION['loggedin'] = 'TRUE'; - $_SESSION['id_user'] = '1'; - $_SESSION['level'] = 'admin'; - $_SESSION['username'] = 'kalkun'; - } -); +$this->setupAdminSession(); // Then add this method to your base test class: // public function setupAdminSession() // { // $this->request->setCallablePreConstructor( // function () { // if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) // { // session_start(); // } // $_SESSION['loggedin'] = 'TRUE'; // $_SESSION['id_user'] = '1'; // $_SESSION['level'] = 'admin'; // $_SESSION['username'] = 'kalkun'; // } // ); // }
17-67: Add test cases for other plugin actionsCurrently, the test only covers the basic index action of the plugin. Consider adding test cases for other actions/functionality that the SMS member plugin provides.
For example, you might want to add tests for:
- Member registration
- SMS sending to members
- Member list display
- Error handling
Example addition:
/** * @dataProvider database_Provider */ public function test_register_member($db_engine) { // Setup similar to test_index // Test member registration $output = $this->request('POST', 'plugin/sms_member/register', [ 'phone' => '123456789', 'name' => 'Test Member' ]); $this->assertValidHtml($output); // Additional assertions }tests/models/Gammu_model_get_conversation_test.php (2)
1-704: Consider refactoring common test setup patterns to reduce duplication.Many of the test methods contain similar setup code for initializing the session, database, and model. Consider extracting this common setup into helper methods or expanding the setUp method to reduce code duplication.
Example refactoring for common setup:
public function setUp() : void { $this->resetInstance(); $this->CI->load->model('gateway/Gammu_model'); $this->obj = $this->CI->Gammu_model; + + // Set up session data + $this->CI->load->library('session'); + $_SESSION['loggedin'] = 'TRUE'; + $_SESSION['id_user'] = '1'; + $_SESSION['level'] = 'admin'; + $_SESSION['username'] = 'kalkun'; + + // Load database + $this->CI->load->database(); } // Then in test methods, you would only need: // $dbsetup->setup_config('...'); // $dbsetup->execute($this->CI);
1-704: Consider using constants for magic numbers.The code uses several magic numbers (e.g., folder IDs: 1, 3, 5, 11) throughout the tests. Consider defining these as class constants for better readability and maintainability.
Example implementation:
class Gammu_model_get_conversation_test extends KalkunTestCase { + // Define folder constants + const FOLDER_INBOX = 1; + const FOLDER_OUTBOX = 2; + const FOLDER_SENT = 3; + const FOLDER_TRASH = 5; + const FOLDER_CUSTOM = 11; public function setUp() : void { $this->resetInstance(); $this->CI->load->model('gateway/Gammu_model'); $this->obj = $this->CI->Gammu_model; } // Then use constants in the test methods // e.g., replace $id_folder = 1; with $id_folder = self::FOLDER_INBOX;tests/models/Gammu_model_get_messages_test.php (4)
103-123: Consider using more meaningful variable names for clarity.The 'processed' option is set to FALSE without a clear explanation of what this means in context.
- 'processed' => FALSE, // only on type=inbox / used in Daemon::message_routine() + 'processed' => FALSE, // Messages that haven't been processed by the daemon yet
124-151: Complex offset calculations could be improved with constants or helper methods.The test contains hardcoded offset values (like 250, 875) without clear explanation of their origin, making the code harder to maintain.
Consider extracting these magic numbers into named constants or calculating them based on well-defined parameters:
- $id_offset = 875; // 250 for user1, 125 for user2 500 for user3 + // Define constants at class level + // const USER1_OFFSET = 250; + // const USER2_OFFSET = 125; + // const USER3_OFFSET = 500; + // const USER4_OFFSET = 875; + $id_offset = self::USER4_OFFSET; // More readable and maintainable
451-496: Unnecessary database reloading in outbox test.The test reloads the database connection and model even though it was already done in the reloadCIwithEngine method.
- $this->CI->load->database(); - $this->CI->load->model('gateway/Gammu_model'); - $this->obj = $this->CI->Gammu_model;
498-543: Redundant database loading in sent items test.Similar to the outbox test, there's redundant loading of the database and model.
- $this->CI->load->database(); - $this->CI->load->model('gateway/Gammu_model'); - $this->obj = $this->CI->Gammu_model;tests/models/Gammu_model_test.php (1)
355-359: Consider uncommenting database validation check.There's a commented out section that checks if the database query result is NULL. This might be important for detecting configuration issues during testing.
- //if ($result->row() === NULL) - //{ - // $this->markTestIncomplete('gammu-smsd may not be properly configured to connect to a database.'); - //} - //else - //{ + if ($result->row() === NULL) + { + $this->markTestIncomplete('gammu-smsd may not be properly configured to connect to a database.'); + } + else + {application/helpers/kalkun_helper.php (3)
19-22: Improve function naming convention for consistencyThe function name
DNDcheckdoesn't follow common PHP naming conventions which tend to use snake_case (e.g.dnd_check) or camelCase (e.g.dndCheck).Consider renaming the function to follow consistent naming conventions used throughout the codebase:
-if ( ! function_exists('DNDcheck')) -{ - function DNDcheck($mobileno) +if ( ! function_exists('dnd_check')) +{ + function dnd_check($mobileno)To preserve backward compatibility, you could create both functions with the new one calling the old one if necessary.
42-42: Improve response parsing robustnessThe current implementation relies on a specific string pattern in the HTML response, which could be brittle if the API response format changes.
Consider implementing a more robust response parsing method:
- return (is_int(strpos(strtolower(strip_tags($response)), 'number is not')) ? FALSE : TRUE); + $cleanResponse = strtolower(strip_tags($response)); + + // More robust pattern matching for various potential responses + $isRegistered = !( + strpos($cleanResponse, 'number is not') !== false || + strpos($cleanResponse, 'not registered') !== false + ); + + return $isRegistered;
19-43: Add response caching for performance optimizationThe current implementation makes a new HTTP request each time the function is called, even for the same phone number. This could lead to unnecessary network traffic and slower response times.
Consider implementing a simple caching mechanism to avoid repeated requests for the same number:
if ( ! function_exists('DNDcheck')) { function DNDcheck($mobileno) { + static $cache = array(); + + // Extract the last 10 digits for consistency $mobileno = substr($mobileno, -10, 10); + + // Check if result is cached + if (isset($cache[$mobileno])) { + return $cache[$mobileno]; + } $url = 'http://www.nccptrai.gov.in/nccpregistry/saveSearchSub.misc'; // Rest of implementation... - return (is_int(strpos(strtolower(strip_tags($response)), 'number is not')) ? FALSE : TRUE); + $result = (is_int(strpos(strtolower(strip_tags($response)), 'number is not')) ? FALSE : TRUE); + + // Cache the result + $cache[$mobileno] = $result; + + return $result; } }tests/testutils/DBVars.php (2)
1-17: Class structure looks good, consider a namespaceThe
DBVarsclass is well-structured with clear constants for database testing. However, since this is a new file, consider adding a namespace to avoid potential conflicts with other classes.<?php /** * Kalkun * An open source web based SMS Management * * @package Kalkun * @author Kalkun Dev Team * @license https://spdx.org/licenses/GPL-2.0-or-later.html * @link https://kalkun.sourceforge.io/ */ +namespace Kalkun\Tests\Utils; class DBVars { const DATABASE = 'kalkun_testing'; const USERNAME = 'kalkun_test_user'; const PASSWORD = 'kalkun_test_user'; }
19-36: Add error handling for CLI usageThe CLI implementation is functional but lacks error handling for invalid arguments. Consider adding a default case to show usage instructions when an invalid or missing argument is provided.
if (php_sapi_name() === 'cli' && isset($argv)) { if (isset($argv[1])) { switch ($argv[1]) { case 'database': echo DBVars::DATABASE; break; case 'username': echo DBVars::USERNAME; break; case 'password': echo DBVars::PASSWORD; break; + default: + echo "Usage: php DBVars.php [database|username|password]\n"; + break; } } + else + { + echo "Usage: php DBVars.php [database|username|password]\n"; + } }application/views/main/users/add_user.php (1)
35-36: Consider separating class and id attributesWhile adding the
id="level"is good for accessibility, combining both class and id in the same string variable can be confusing for maintenance. Consider separating them for clarity.-$option = 'class="text ui-widget-content ui-corner-all" id="level"'; +$class_option = 'class="text ui-widget-content ui-corner-all"'; +$id_option = 'id="level"'; +echo form_dropdown('level', $level, $level_act, $class_option.' '.$id_option);Or alternatively, use the array syntax for attributes in CodeIgniter's form_dropdown:
-$option = 'class="text ui-widget-content ui-corner-all" id="level"'; -echo form_dropdown('level', $level, $level_act, $option); +$options = array( + 'class' => 'text ui-widget-content ui-corner-all', + 'id' => 'level' +); +echo form_dropdown('level', $level, $level_act, $options);application/third_party/MX/Loader.php (1)
48-53: Appropriate fix for test isolationThis change is correctly implementing a solution for CI test isolation issues. The code resets the
CI::$APPstatic variable to the current controller instance when in testing environment, ensuring each test runs with a clean application state.This pattern helps prevent state bleed between test cases, which is especially important in HMVC architectures where shared static references can cause unexpected behavior.
tests/plugins/Sms_credit_test.php (2)
25-25: Consider removing or uncommenting the commented lineThis commented code is likely leftover from development and should either be removed or uncommented if needed.
- //DBSetup::setup_db_kalkun_testing2($this);
28-31: Remove commented method if not neededThis commented method
reloadCIwithEngineis not being used in the tests and should be removed if it's not needed.- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/media/css/blue.css (2)
39-40: Improved HTML5 compatibility by switching from ID to class selectorsGreat change migrating from ID selectors to class selectors. This improves HTML5 compatibility and allows for more reusable styling across multiple elements.
Using class selectors instead of ID selectors is especially important for components that can appear multiple times on a page (like pagination elements, dashboard boxes, etc.). This change helps ensure valid HTML5 where IDs must be unique across the document.
Also applies to: 43-44, 47-58
51-54: Remove extra semicolon in CSS declarationThere's an unnecessary semicolon on line 53 that should be removed.
- background-color: #F5F5F5; - ; + background-color: #F5F5F5;🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Expected a declaration, or an at rule but instead found ';'.
Expected a declaration, or an at rule here.
(parse)
application/views/main/forgot_password.php (1)
61-61: Consider adding focus and hover states for the submit button.While the styling improvements are good, consider enhancing the user experience with focus and hover states for the submit button.
Add CSS for interactive states to improve user experience:
#submit:hover, #submit:focus { background-color: #f0f0f0; outline: 2px solid #86C0D2; }media/css/install.css (1)
1-180: Consider adding media queries for responsive design.While the CSS changes improve the layout, consider adding media queries to ensure the application looks good on mobile devices and smaller screens.
Add media queries for responsive design:
@media (max-width: 800px) { #container, #left-container, #right-container, #footer { width: 100%; float: none; border-radius: 0; } body { width: auto; margin: 0; padding: 0 10px 50px 10px; } }application/plugins/whitelist_number/views/index.php (1)
34-41: Valid HTML5 hack - consider adding aria-hidden for screen readersAdding a hidden row to pass validation is a reasonable approach, though I would suggest making it explicitly hidden from screen readers as well.
- <tr style="display: none"> + <tr style="display: none" aria-hidden="true">tests/plugins/Sms_to_wordpress_test.php (2)
25-25: Commented code should be removed or explainedThere's a commented line that references a setup method that is not being used. Consider either removing it or adding a comment explaining why it's kept for future reference.
- //DBSetup::setup_db_kalkun_testing2($this);
28-32: Remove or document commented methodThis commented method
reloadCIwithEngineis not being used. Consider removing it if it's not needed or add a comment explaining its intended future use.- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/tests/plugins/Server_alert_test.php (3)
25-26: Remove or uncomment the commented-out code.This commented-out call to
DBSetup::setup_db_kalkun_testing2($this)should either be uncommented if needed or removed entirely to keep the codebase clean.- //DBSetup::setup_db_kalkun_testing2($this);
28-31: Remove unused commented-out method.This entire commented-out method
reloadCIwithEngineis not being used. If it's not needed, it should be removed to maintain a clean codebase. If it will be used in the future, consider adding a TODO comment explaining its purpose.- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/
1-68: Consider adding test for plugin actions beyond just viewing the index page.The current test only verifies the index page of the plugin. Consider adding tests for other actions like adding, editing, and deleting alerts to ensure the plugin functionality works correctly.
Example test method for adding an alert:
/** * @dataProvider database_Provider */ public function test_add_alert($db_engine) { // Setup similar to test_index // ... // Prepare POST data for creating an alert $postData = [ 'alert_name' => 'Test Alert', 'ip_address' => '127.0.0.1', 'port_number' => '80', 'timeout' => '30', 'phone_number' => '1234567890', 'respond_message' => 'Test response' ]; // Send POST request to create alert $output = $this->request('POST', 'plugin/server_alert', $postData); // Verify alert was created $this->request->setCallablePreConstructor(/* same as in test_index */); $indexOutput = $this->request('GET', 'plugin/server_alert'); $this->assertStringContainsString('Test Alert', $indexOutput); $this->assertStringContainsString('127.0.0.1', $indexOutput); }application/plugins/blacklist_number/views/index.php (1)
37-45: Hidden row hack for HTML validation.The hidden row with empty table headers is a valid workaround for HTML5 validation requirements. Consider adding a more detailed comment explaining why this is needed (e.g., "HTML5 requires th elements in table header rows").
<tr style="display: none"> - <!-- this is a hack to pass validation https://stackoverflow.com/a/52175495 --> + <!-- This is a hack to pass HTML5 validation which requires th elements in table header rows. + Without this, tables with colspan would fail validation. See: https://stackoverflow.com/a/52175495 --> <th></th> <th></th> <th></th> <th></th> <th></th> </tr>tests/plugins/Blacklist_number_test.php (3)
25-26: Remove or uncomment the commented-out code.This commented-out call to
DBSetup::setup_db_kalkun_testing2($this)should either be uncommented if needed or removed entirely to keep the codebase clean.- //DBSetup::setup_db_kalkun_testing2($this);
28-31: Remove unused commented-out method.This entire commented-out method
reloadCIwithEngineis not being used. If it's not needed, it should be removed to maintain a clean codebase. If it will be used in the future, consider adding a TODO comment explaining its purpose.- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/
12-68: Consider refactoring common test code to reduce duplication.There's significant code duplication between this test class and the
Server_alert_testclass. Consider refactoring common setup and test functionality into a base test class or trait to improve maintainability.Example approach:
// In a new file like tests/plugins/AbstractPluginTest.php: abstract class AbstractPluginTest extends KalkunTestCase { protected $pluginName; public function setUp() : void { if (file_exists(FCPATH . 'install')) { unlink(FCPATH . 'install'); } } public static function database_Provider() { return DBSetup::$db_engines_to_test; } /** * @dataProvider database_Provider */ public function test_index($db_engine) { if (empty($this->pluginName)) { throw new \Exception('Plugin name must be set in child class'); } $dbsetup = new DBSetup([ 'engine' => $db_engine, ]); $dbsetup->setup_config('gammu_no_pbk_kalkun_fresh_install_manual_sql_injection'); $this->setupSession(); $dbsetup->install_plugin($this, $this->pluginName); Pluginss_test::reset_plugins_lib_static_members(); $output = $this->request('GET', 'plugin/' . $this->pluginName); $this->assertValidHtml($output); $this->performAdditionalAssertions($output); $this->markTestIncomplete(); } protected function setupSession() { $this->request->setCallablePreConstructor( function () { if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) { session_start(); } $_SESSION['loggedin'] = 'TRUE'; $_SESSION['id_user'] = '1'; $_SESSION['level'] = 'admin'; $_SESSION['username'] = 'kalkun'; } ); } protected function performAdditionalAssertions($output) { // To be overridden by child classes } } // Then in Blacklist_number_test.php: class Blacklist_number_test extends AbstractPluginTest { protected $pluginName = 'blacklist_number'; protected function performAdditionalAssertions($output) { $this->assertStringContainsString('Blacklist Number', $output); $this->assertStringContainsString('Add Blacklist Number', $output); } }application/plugins/soap/views/index.php (3)
54-65: Consider replacing the validation hack
The empty header cells are a hack to pass HTML validation, which might confuse future contributors. Consider restructuring or adding more explanatory comments for maintainability.
91-92: Add accessibility attributes when using icon spans
Switching from<img>to<span>icons is fine, but consider including ARIA labels or hidden text for screen readers.
146-147: Maintain accessibility with new icon spans
Similar to lines 91-92, these icons could benefit from improved accessibility attributes likearia-hidden="true"or ARIA labels.tests/plugins/Jsonrpc_test.php (2)
28-32: Commented-out functionreloadCIwithEngine
If it’s no longer necessary, consider removing the entire function to avoid confusion.
38-104: Robust error handling intest_send_sms_client
Good approach handling different error outcomes (connection refused vs. 404). Consider clarifying or centralizing the error handling logic for easier maintenance.tests/controllers/Install_test.php (1)
96-121:test_requirement_check_error: reflection usage
Injecting an invalid driver via reflection is a clever way to test rigorous failure handling. Retain caution, as reflection can be brittle.tests/controllers/Phonebook_test.php (2)
40-60: Incomplete test should be completed.The test method is marked as incomplete. While this is acceptable during development, these tests should be completed to ensure proper test coverage.
Would you like me to help complete some of these test implementations with proper assertions?
44-55: Consider extracting session setup to reduce duplication.The session setup logic is duplicated across all test methods. Consider extracting this to a helper method to reduce code duplication.
- $this->request->setCallablePreConstructor( - function () { - if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) - { - session_start(); - } - $_SESSION['loggedin'] = 'TRUE'; - $_SESSION['id_user'] = '2'; - $_SESSION['level'] = 'user'; - $_SESSION['username'] = 'user1'; - } - ); + $this->setupUserSession();Then add this method to the class:
private function setupUserSession() { $this->request->setCallablePreConstructor( function () { if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) { session_start(); } $_SESSION['loggedin'] = 'TRUE'; $_SESSION['id_user'] = '2'; $_SESSION['level'] = 'user'; $_SESSION['username'] = 'user1'; } ); }tests/plugins/Soap_test.php (3)
61-67: Complete the incomplete test.
This test is marked with$this->markTestIncomplete();and lacks a final assertion for the tested functionality. Consider adding the necessary assertions or finalizing the test logic to ensure full coverage.
210-232: Consider verifying HTTP response codes.
You currently check the presence of a string in the response body but do not confirm the HTTP status code or any potential errors. For more comprehensive coverage, ensure that the tests also verify the expected HTTP status code.
432-439: Post-logout session cleanup.
While expecting a RuntimeException due to session destruction, ensure that subsequent tests do not rely on session state. Consider explicitly clearing or resetting the session to avoid unpredictable interactions in later tests.patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.4.patch (1)
11-64: Retain partial type safety with explicit nullable type.
By removing thearraytype hint entirely (e.g.public static function init($autoload_dirs = null)), you lose type safety. Consider using a nullable type declaration (e.g.?array) to maintain clarity about expected parameter types while also avoiding deprecation warnings in earlier PHP versions.- public static function init(array $autoload_dirs = null) + public static function init(?array $autoload_dirs = null)tests/controllers/Login_test.php (3)
48-57: Expand verification of successful login.
You currently verify only that the response is a redirect (302) tokalkun. For more thorough coverage, consider checking session states, login-related flash data, or the subsequent page content to ensure the login process truly succeeded.
62-72: Enhance coverage for login failure.
The test checks the flash message but does not verify the resulting HTTP status code or final rendered page. Consider adding these validations to ensure the correct error handling flow is displayed to the user.
74-110: Session management caution.
Destroying an uninitialized session is expected here, but ensure that future tests do not preserve any partial session state. Consider explicitly resetting session variables afterward to avoid side effects.tests/controllers/Daemon_test.php (3)
43-51: Consider verifying a successful processing outcome.
Although$this->assertEmpty($data)indicates the endpoint returned no content, consider also asserting that the database state matches your expectations (e.g., verifying updated fields or logs) to ensure the routine ran successfully.
259-294: Handle invalid gateway calls more robustly.
You deliberately provoke a file_get_contents failure for invalid gateway parameters. Consider adding a more explicit check or a custom exception for clarity. This helps maintain a clear error boundary and provides more descriptive feedback than a generic stream-related error.
316-321: Validate plugin state after alert triggers.
You confirm the plugin status is'false'in the database, but also consider verifying if any side effects (such as logs or alert notifications) are triggered as expected. This will ensure a more complete test coverage of the server alert process.tests/controllers/Pluginss_test.php (2)
31-34: Remove duplicated assignment to$instance.The line
Plugins_lib_kalkun::$instance = NULL;is repeated at lines 31 and 34. Consider removing one to avoid confusion and keep the code clean.
51-54: Remove or clarify commented-out reflection block.This code block is commented out, which suggests it may no longer be needed. If it’s truly obsolete, removing it will keep the codebase tidy. Otherwise, clarifying why it's commented out would help maintainers.
tests/controllers/Users_test.php (2)
118-120: Extend coverage for removing users.Here, the test deletes the user with
id_user = 1, effectively clearing the database of the primary admin. Consider adding a scenario to remove additional users to ensure coverage of various edge cases (e.g., different user levels).
575-575: Address the TODO for user deletion with messages in related tables.A TODO is noted here about covering scenarios where messages already exist in inbox, outbox, and other tables. Implementing this would strengthen test coverage for real-world usage.
Shall I open a new issue or generate an additional test method covering message-presence scenarios?
tests/controllers/Messages_folder_test.php (3)
263-264: Confirm usage of"unreaded"class name.The class name “unreaded” is used in these lines and might be a deliberate style choice or a legacy name. If it’s a simple typo, consider renaming to “unread” for consistency and clarity.
Also applies to: 312-313
335-341: Remove or clarify commented-out test methods.The methods
test_folder_ajaxGET_by_conversation_sort_ascare commented out. If they are no longer necessary, removing them would help maintain clarity. Otherwise, clarifying their future purpose would be beneficial.Also applies to: 814-820
16-36: Factor out repeated engine reloading.Many test methods repeat the same pattern:
•reloadCIwithEngine($db_engine)
•setCallablePreConstructor(...)
• Updating config files
• Resetting the test instanceConsider extracting these steps into a shared helper or a dedicated setup method to reduce duplication and make tests more concise.
tests/controllers/Messages_my_folder_test.php (5)
20-27: Consider adding error handling forunlinkinsetUp.
Ifunlinkfails due to permissions or file locking, you may miss the error. Logging or a try/catch block could help identify setup issues.
47-58: Refactor repeated session initialization logic.
This pattern appears in many tests. Consider extracting a helper method to streamline session setup and reduce duplication.
53-55: Favor using booleans over string literals.
Use$_SESSION['loggedin'] = true;to maintain consistent data types.
347-352: Remove or clarify commented-out test code.
Keeping placeholder tests commented out may cause confusion. If not needed, remove it; otherwise, consider finalizing the test.
833-839: Remove or finalize this commented-out test.
As with the other placeholder test, keeping it commented may become stale.tests/testutils/DBSetup.php (4)
336-338: Avoid usingdie;in non-interactive automation.A simple
die;can obscure debugging in automated test environments. Instead, consider throwing an exception or returning an error code to allow the calling test to gracefully handle or report errors.-case 'gammu_no_pbk_kalkun_upgrade_from_0.8.3': ... +default: - die; + throw new \RuntimeException('Config not found');
349-435: Reduce duplication inwrite_config_file_for_database().This method duplicates many lines for each database engine. Extracting the config content into a parameterized template or using placeholders could simplify maintenance and readability.
873-919: Ensure rollback or cleanup logic is covered.
closure()accumulates record insertions for replay, but there’s no built-in rollback or cleanup if partial inserts fail. Consider implementing a safety mechanism or adding structured error-handling in destructive operations, especially if other tests rely on a consistent state.
921-963: Unify logic betweenexecute()andclosure().Both methods implement near-identical insert/update logic. Minimizing repetition by referencing a shared function or object could reduce maintenance overhead and improve clarity, particularly around partial validation or error tracking during batch insertions.
tests/controllers/Messages_test.php (4)
18-24: Add a matchingtearDown()for cleanup.While
setUp()removes any leftoverinstallfile, there is no corresponding teardown method to revert or clear states (e.g., restore config files, or drop dedicated DB usage). Absent teardown logic may risk side effects between tests if new artifacts are created.
534-544: Complete or remove placeholder tests.The
test_move_message(),test_delete_messages(), andtest_delete_all()methods are marked as incomplete. Leaving them indefinitely may mislead coverage metrics or cause confusion. Finalize these tests or remove them if no longer necessary.Do you want help writing these tests or opening a ticket to track their completion?
826-849: Ensure thorough coverage of reply logic.The
test_compose_reply()method checks hidden fields and signature presence, but does not verify any subsequent server-side logic (such as how the system processes or modifies the “reply_value”). Expanding coverage can help detect regressions in final message composition logic.
1128-1137: Consider including a “resend_inbox” scenario or removing it entirely.The commented-out
test_compose_resend_inbox()hints at an intentionally unsupported GUI action. If this feature isn’t planned, remove the commented code. If it might be added, keep a TODO or placeholder test code with references to upcoming changes.tests/controllers/Kalkun_test.php (2)
20-26: Consider handling potential unlink errors
Whenunlink()fails, it remains silent and can mask permission or file-lock anomalies in your CI environment. You may consider adding a conditional check or logging an error to aid troubleshooting.
36-210: Streamline repeated code and verify blank redirect
- The session setup and database configuration (
DBSetup+ reproducible call tosetup_config('gammu_no_pbk_kalkun_fresh_install_manual_sql_injection')) appear in nearly every test. You could refactor them into a shared utility or a specializedsetUp()method to reduce duplication and improve readability.- The config name
'gammu_no_pbk_kalkun_fresh_install_manual_sql_injection'can be renamed to something more descriptive (e.g.,'kalkun_fresh_install_config') to clarify its purpose.- At lines 56–58,
$this->assertRedirect('', 302)tests a redirect to an empty path. If this behavior is truly intended, consider explicitly using'/'or clarifying the default route to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
.github/workflows/codeigniter.yml(1 hunks).gitignore(1 hunks)application/config/autoload.php(1 hunks)application/controllers/Install.php(3 hunks)application/controllers/Messages.php(5 hunks)application/controllers/Phonebook.php(1 hunks)application/controllers/Users.php(1 hunks)application/helpers/kalkun_helper.php(1 hunks)application/libraries/Install_info.php(1 hunks)application/plugins/blacklist_number/controllers/Blacklist_number.php(1 hunks)application/plugins/blacklist_number/views/index.php(5 hunks)application/plugins/server_alert/controllers/Server_alert.php(1 hunks)application/plugins/server_alert/views/index.php(6 hunks)application/plugins/sms_credit/controllers/Sms_credit.php(2 hunks)application/plugins/sms_credit/views/index.php(6 hunks)application/plugins/sms_credit/views/navigation.php(1 hunks)application/plugins/sms_credit/views/packages.php(2 hunks)application/plugins/sms_member/views/index.php(1 hunks)application/plugins/sms_to_email/views/index.php(3 hunks)application/plugins/sms_to_wordpress/views/index.php(1 hunks)application/plugins/sms_to_xmpp/views/index.php(1 hunks)application/plugins/soap/controllers/Soap.php(1 hunks)application/plugins/soap/views/index.php(7 hunks)application/plugins/stop_manager/controllers/Stop_manager.php(1 hunks)application/plugins/stop_manager/views/index.php(4 hunks)application/plugins/whitelist_number/controllers/Whitelist_number.php(1 hunks)application/plugins/whitelist_number/views/index.php(5 hunks)application/third_party/MX/Loader.php(1 hunks)application/third_party/MX/Modules.php(1 hunks)application/views/js_init/js_keyboard.php(1 hunks)application/views/js_init/js_layout.php(1 hunks)application/views/js_init/message/js_conversation.php(1 hunks)application/views/js_init/message/js_function.php(1 hunks)application/views/js_init/phonebook/js_group.php(1 hunks)application/views/js_init/phonebook/js_phonebook.php(1 hunks)application/views/main/base.php(10 hunks)application/views/main/dashboard/home.php(2 hunks)application/views/main/dashboard/statistic.php(1 hunks)application/views/main/footer.php(1 hunks)application/views/main/forgot_password.php(4 hunks)application/views/main/install/config_setup.php(21 hunks)application/views/main/install/database_setup.php(7 hunks)application/views/main/install/layout.php(2 hunks)application/views/main/install/requirement_check.php(3 hunks)application/views/main/install/welcome.php(0 hunks)application/views/main/layout.php(2 hunks)application/views/main/login.php(4 hunks)application/views/main/messages/compose.php(13 hunks)application/views/main/messages/conversation.php(3 hunks)application/views/main/messages/index.php(2 hunks)application/views/main/messages/message_list.php(1 hunks)application/views/main/messages/navigation.php(2 hunks)application/views/main/password_reset.php(5 hunks)application/views/main/phonebook/contact/add_contact.php(1 hunks)application/views/main/phonebook/contact/index.php(2 hunks)application/views/main/phonebook/contact/navigation.php(1 hunks)application/views/main/phonebook/contact/pbk_list.php(2 hunks)application/views/main/phonebook/group/index.php(1 hunks)application/views/main/phonebook/group/navigation.php(1 hunks)application/views/main/phonebook/group/pbk_list.php(2 hunks)application/views/main/plugin/index.php(1 hunks)application/views/main/search.php(2 hunks)application/views/main/settings/appearance.php(2 hunks)application/views/main/settings/filters.php(3 hunks)application/views/main/settings/general.php(6 hunks)application/views/main/settings/password.php(1 hunks)application/views/main/settings/personal.php(1 hunks)application/views/main/settings/setting.php(1 hunks)application/views/main/users/add_user.php(2 hunks)application/views/main/users/index.php(0 hunks)application/views/main/users/navigation.php(1 hunks)composer.json(1 hunks)media/css/base.css(8 hunks)media/css/blue.css(1 hunks)media/css/compose.css(1 hunks)media/css/install.css(4 hunks)patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.4.patch(1 hunks)tests/.gitignore(1 hunks)tests/controllers/Daemon_test.php(1 hunks)tests/controllers/Install_test.php(1 hunks)tests/controllers/Kalkun_test.php(1 hunks)tests/controllers/Login_test.php(1 hunks)tests/controllers/Messages_folder_test.php(1 hunks)tests/controllers/Messages_my_folder_test.php(1 hunks)tests/controllers/Messages_test.php(1 hunks)tests/controllers/Phonebook_test.php(1 hunks)tests/controllers/Pluginss_test.php(1 hunks)tests/controllers/Users_test.php(1 hunks)tests/libraries/Install_info_test.php(1 hunks)tests/models/Gammu_model_get_conversation_test.php(1 hunks)tests/models/Gammu_model_get_messages_test.php(1 hunks)tests/models/Gammu_model_test.php(1 hunks)tests/plugins/Blacklist_number_test.php(1 hunks)tests/plugins/Jsonrpc_test.php(1 hunks)tests/plugins/Server_alert_test.php(1 hunks)tests/plugins/Sms_credit_test.php(1 hunks)tests/plugins/Sms_member_test.php(1 hunks)tests/plugins/Sms_to_email_test.php(1 hunks)tests/plugins/Sms_to_twitter_test.php(1 hunks)tests/plugins/Sms_to_wordpress_test.php(1 hunks)tests/plugins/Sms_to_xmpp_test.php(1 hunks)tests/plugins/Soap_test.php(1 hunks)tests/plugins/Stop_manager_test.php(1 hunks)tests/plugins/Whitelist_number_test.php(1 hunks)tests/testutils/ConfigFile.php(1 hunks)tests/testutils/DBSetup.php(1 hunks)tests/testutils/DBVars.php(1 hunks)
⛔ Files not processed due to max files limit (11)
- tests/testutils/GammuSmsdConfigFile.php
- tests/testutils/Html4Validator.php
- tests/testutils/Html5Validator.php
- tests/testutils/HtmlValidator.php
- tests/testutils/KalkunTestCase.php
- tests/testutils/TestSuiteFile.php
- tests/testutils/sql-gammu-1.37.4/mysql.sql
- tests/testutils/sql-gammu-1.37.4/pgsql.sql
- tests/testutils/sql-gammu-1.37.4/sqlite.sql
- tests/testutils/sql-gammu-1.42.0/mysql-legacy.sql
- tests/testutils/sql-gammu-1.42.0/mysql.sql
💤 Files with no reviewable changes (2)
- application/views/main/users/index.php
- application/views/main/install/welcome.php
✅ Files skipped from review due to trivial changes (36)
- tests/testutils/ConfigFile.php
- .gitignore
- application/plugins/sms_credit/views/navigation.php
- application/plugins/blacklist_number/controllers/Blacklist_number.php
- application/plugins/soap/controllers/Soap.php
- application/plugins/stop_manager/controllers/Stop_manager.php
- application/plugins/sms_credit/controllers/Sms_credit.php
- application/views/main/plugin/index.php
- application/views/main/footer.php
- application/views/main/settings/setting.php
- application/views/main/phonebook/contact/navigation.php
- application/views/main/users/navigation.php
- application/views/main/settings/password.php
- application/controllers/Phonebook.php
- application/plugins/whitelist_number/controllers/Whitelist_number.php
- application/plugins/sms_to_wordpress/views/index.php
- application/views/main/phonebook/group/navigation.php
- application/views/js_init/js_layout.php
- application/views/main/phonebook/contact/add_contact.php
- application/third_party/MX/Modules.php
- application/views/main/settings/general.php
- application/plugins/server_alert/controllers/Server_alert.php
- application/plugins/sms_member/views/index.php
- application/views/main/settings/personal.php
- media/css/compose.css
- application/views/main/dashboard/statistic.php
- application/views/main/messages/message_list.php
- application/views/main/search.php
- application/views/main/install/database_setup.php
- application/views/main/install/config_setup.php
- application/controllers/Messages.php
- application/views/main/messages/compose.php
- application/views/main/dashboard/home.php
- application/controllers/Users.php
- application/plugins/sms_to_email/views/index.php
- application/views/main/messages/conversation.php
🧰 Additional context used
🪛 Biome (1.9.4)
media/css/blue.css
[error] 53-53: Expected a declaration, or an at rule but instead found ';'.
Expected a declaration, or an at rule here.
(parse)
🔇 Additional comments (276)
tests/plugins/Stop_manager_test.php (4)
1-16: Good job on the test file setup and necessary includes.The header and includes look well-structured. The copyright, license information, and authorship details are clearly documented.
33-36: LGTM: Clean data provider implementation.The data provider is well-implemented, leveraging the existing database engine configurations.
38-61: Good test setup and session handling.The test setup is thorough with proper database configuration and session initialization for authentication. The use of callables for pre-construction setup is a good approach.
62-67: Complete the incomplete test.The test is currently marked as incomplete and only validates if the output is valid HTML without checking any specific functionality of the stop_manager plugin.
Consider expanding this test to verify specific functionality of the stop_manager plugin, such as:
- Check for expected elements or content in the response
- Verify database interactions if applicable
- Test different plugin operations/routes
$output = $this->request('GET', 'plugin/stop_manager'); $this->assertValidHtml($output); - $this->markTestIncomplete(); + // Assert presence of expected UI elements or content + $this->assertStringContainsString('Stop Manager', $output); + // Add more specific assertions heretests/plugins/Sms_to_email_test.php (2)
38-67: Incomplete test needs to be completed or documented.The test is marked as incomplete (
markTestIncomplete), but there's no comment explaining what remaining work is needed. This test only verifies valid HTML output without asserting specific plugin functionality.Consider either:
- Complete the test with additional assertions for plugin-specific behavior
- Add a comment explaining what work remains to be done
$output = $this->request('GET', 'plugin/sms_to_email'); $this->assertValidHtml($output); - $this->markTestIncomplete(); + $this->markTestIncomplete('Need to add assertions for specific sms_to_email plugin functionality');
1-69:Details
✅ Verification successful
Verify the test is registered in the test suite.
This is a new test file, and it's important to verify that it will be discovered and run by the test runner.
🏁 Script executed:
#!/bin/bash # Check if the test class is properly discoverable by PHPUnit echo "Checking if the test class is discoverable..." # Look for PHPUnit configuration if [ -f "phpunit.xml" ]; then echo "Found phpunit.xml" grep -A 10 "testsuites\|testsuite" phpunit.xml elif [ -f "phpunit.xml.dist" ]; then echo "Found phpunit.xml.dist" grep -A 10 "testsuites\|testsuite" phpunit.xml.dist else echo "No PHPUnit configuration found. Looking for patterns in directory structure..." # Check if the test follows directory patterns that PHPUnit might discover automatically find . -path "*test*.php" | grep -i "sms_to_email" fi # Look for any test registration or bootstrap files echo "Looking for test bootstrap files..." find . -name "*bootstrap*.php"Length of output: 525
ACTION REQUIRED: Confirm Test Discovery
The output confirms that the test file (
tests/plugins/Sms_to_email_test.php) is auto-discoverable by the test runner based on its naming convention. Although no PHPUnit configuration was found, the file is located using the directory search pattern, ensuring it will be executed. Please ensure any future changes to the test configuration (such as adding a phpunit.xml file) continue to support this auto-discovery.tests/plugins/Whitelist_number_test.php (2)
41-67: Complete the test implementation.The test is marked as incomplete with
$this->markTestIncomplete(). This indicates that the test needs to be finalized with proper assertions for the whitelist functionality beyond just validating HTML.Consider adding specific assertions to verify that:
- The whitelist interface loads correctly
- The whitelist functionality works as expected (adding/removing numbers)
- The whitelist rules are properly enforced
Once implementation is complete, remove the
markTestIncomplete()line.
61-62:Details
✅ Verification successful
Verify the necessity of resetting plugins library static members.
The code calls
Pluginss_test::reset_plugins_lib_static_members()after installing the plugin. Verify that this is necessary and that the static members don't need to be initialized before the test runs.
🏁 Script executed:
#!/bin/bash # Look for other tests that use Pluginss_test::reset_plugins_lib_static_members() grep -r "Pluginss_test::reset_plugins_lib_static_members" --include="*.php" ./tests/Length of output: 2896
Resetting static members is necessary for test isolation.
The grep results confirm that the call toPluginss_test::reset_plugins_lib_static_members()is consistently used across various tests (e.g., intests/testutils/DBSetup.php,tests/plugins/Stop_manager_test.php,tests/plugins/Soap_test.php, etc.). This widespread usage indicates that static members are not automatically initialized before tests run and must be manually reset to prevent state leakage between tests. No changes are needed in this test file.tests/plugins/Sms_to_xmpp_test.php (3)
1-10: Well-structured file header with appropriate license information.The file header includes all necessary information: copyright notice, author, license, and project link.
62-62:Details
❓ Verification inconclusive
Verify the reset method's impact.
The call to
Pluginss_test::reset_plugins_lib_static_members()might have side effects or dependencies that aren't clear from this file alone. Ensure that this method is properly documented and tested.
🏁 Script executed:
#!/bin/bash # Check the implementation of the reset method echo "Searching for the implementation of reset_plugins_lib_static_members..." rg -A 10 "reset_plugins_lib_static_members" --type php echo "Checking how this method is used in other tests..." rg "reset_plugins_lib_static_members" --type php | grep -v "Sms_to_xmpp_test"Length of output: 28074
Ensure Comprehensive Documentation and Testing of State Reset
The call to
Pluginss_test::reset_plugins_lib_static_members()in this test is consistent with its widespread usage across the test suite—as confirmed by its appearance in many other tests. Its implementation (intests/controllers/Pluginss_test.php) resets static members ofPlugins_lib_kalkun, thereby mitigating unwanted state persistence.
- Please verify that its side effects and dependencies are clearly documented in the codebase.
- Confirm that existing tests fully cover its behavior under different scenarios to avoid unexpected interactions.
46-46:Details
✅ Verification successful
Verify the naming of configuration setup.
The configuration name 'gammu_no_pbk_kalkun_fresh_install_manual_sql_injection' contains the term 'manual_sql_injection' which could be concerning. Ensure this is a safe testing mechanism and not actually bypassing SQL parameterization or security measures.
🏁 Script executed:
#!/bin/bash # Check where this configuration is defined and how it's used echo "Searching for the definition of this configuration setup..." rg -A 10 -B 5 "gammu_no_pbk_kalkun_fresh_install_manual_sql_injection" --type php echo "Checking if actual SQL injection is being performed..." rg "manual_sql_injection" --type php | grep -i "exec\|query"Length of output: 65827
Action: Confirm Test-Only Usage; Consider Clarifying Naming
The verification shows that the configuration
gammu_no_pbk_kalkun_fresh_install_manual_sql_injection
is defined within the test utilities (intests/testutils/DBSetup.php) and is used exclusively across various test cases. It loads predetermined SQL scripts for simulating fresh installations without actually triggering dynamic SQL injection or bypassing any parameterization.Although no unsafe SQL practices occur here, the inclusion of “manual_sql_injection” in the name may be misleading. It might be beneficial to rename this configuration to something less alarming (for example, appending a term like “_test_config”) so it clearly indicates that it is only for controlled testing scenarios.
tests/plugins/Sms_to_twitter_test.php (1)
62-62:Details
✅ Verification successful
Verify the static method call matches the corrected class name.
If you fix the typo in the import file name as suggested above, make sure this static method call matches the corrected class name. Double-check the class name in the imported file.
🏁 Script executed:
#!/bin/bash # Check if the Plugins_test class exists (with a single 's') and contains the reset_plugins_lib_static_members method rg -A 1 "class Plugins_test" tests/controllers/ rg "reset_plugins_lib_static_members" tests/controllers/Length of output: 1592
Static Call Naming Verified – No Further Action Required
The static method call in tests/plugins/Sms_to_twitter_test.php now correctly references the class name as defined in tests/controllers/Pluginss_test.php. All calls toPluginss_test::reset_plugins_lib_static_members()are consistent with the class definition, so no additional changes are needed.tests/plugins/Sms_member_test.php (2)
1-10: Documentation follows project standardsThe file header correctly includes the copyright information, author details, license, and project link according to the project standards.
12-15: Test dependencies look appropriateThe test properly requires all necessary test utilities and controllers needed for testing the SMS member plugin.
tests/models/Gammu_model_get_conversation_test.php (13)
1-14: Well-structured file header with proper licensing information.The file header provides comprehensive information about the Kalkun SMS Manager including copyright, licensing, and project links.
15-22: Clean class setup with proper inheritance.The class properly extends KalkunTestCase and implements the setUp method correctly, resetting the instance and loading the necessary model.
24-27: Good use of data providers for parameterized testing.Using a database provider to test across different database engines improves test coverage and ensures compatibility.
90-148: Good test for access control.This test verifies that users cannot access conversations that don't belong to them, which is an important security check.
150-155: Effective data provider with meaningful test case names.The provider uses descriptive keys ('not to inbox trash' and 'to inbox trash') which clearly indicate the test scenarios.
160-239: Comprehensive test for basic conversation retrieval.The test thoroughly validates conversation retrieval functionality with appropriate assertions to verify both the number of conversations and their content.
244-325: Good test for multipart message handling.This test properly verifies that the get_conversation method correctly handles multipart messages, which is crucial for SMS applications where message splitting is common.
330-396: Well-structured pagination test.The test effectively verifies that limit and offset parameters work correctly for paginating through conversation results.
401-470: Thorough test for user-defined folders.The test correctly verifies that conversations can be retrieved from user-defined folders (id_folder = 11).
475-535: Complete test for outbox messages.The test properly verifies outbox message retrieval functionality with appropriate setup and assertions.
540-619: Comprehensive test for sent items.This test thoroughly verifies sent item retrieval with appropriate handling of trash status.
624-703: Well-structured test for multipart sent items.The test effectively verifies sent items that consist of multiple parts, ensuring comprehensive coverage of the functionality.
32-88:Details
✅ Verification successful
Well-structured test for invalid type handling.
This test properly verifies exception handling when an invalid conversation type is requested. The test setup is comprehensive and test assertions are correctly defined.
I notice the method
_expectExceptionMessageMatcheson line 85 which may be a custom method in the parent class:
🏁 Script executed:
#!/bin/bash # Check if the _expectExceptionMessageMatches method is defined in the parent class # Look for the method definition in KalkunTestCase rg -A 3 "_expectExceptionMessageMatches" tests/testutils/KalkunTestCase.phpLength of output: 225
Verified: Custom Exception Handling is Correctly Implemented
Your test for handling an invalid conversation type is well-structured and comprehensive. The custom method
_expectExceptionMessageMatchesused on line 85 is confirmed to be defined in the parent class (tests/testutils/KalkunTestCase.php), ensuring that exception message verification functions as intended. No further modifications are necessary.tests/models/Gammu_model_get_messages_test.php (12)
1-14: Great use of proper file header documentation.The file header adequately provides the required copyright, licensing, and attribution information, which is essential for open-source projects.
15-34: Good test class structure with proper setup and utility methods.The class correctly extends KalkunTestCase and includes a proper setUp method for database initialization. The reloadCIwithEngine method provides a clean way to switch between database engines during testing.
36-55: Well-implemented negative test case for invalid message type.The test correctly verifies that the application throws the expected exception when an invalid message type is provided. This is essential for ensuring proper error handling in the application.
60-88: LGTM: Effective user permission testing.This test effectively verifies that users cannot access messages that don't belong to them, which is a critical security aspect.
154-202: Good use of expected values to verify message content.The test effectively verifies both the number of rows and the specific content of each message, ensuring that the correct messages are retrieved for the inbox with no limit or offset.
204-252: LGTM: Thorough test for messages in trash folder.This test properly verifies the behavior of retrieving messages from the trash folder, which is an important use case.
303-349: Good test for multipart message handling.This test effectively verifies the retrieval of multipart messages, which can be a complex area in SMS handling.
351-400: Well-structured test for limit and offset functionality.The test properly verifies that the limit and offset parameters work as expected when retrieving messages.
402-449: LGTM: Thorough test for custom folder retrieval.This test effectively verifies that messages can be retrieved from custom folders based on the folder ID.
545-587: Good verification of multipart message handling for sent items.The test properly verifies that multipart messages are correctly retrieved from the sent items folder, including checking the first part's content.
589-603:Details
❓ Verification inconclusive
Implement the incomplete test cases.
Two test methods are marked as incomplete. These should be implemented to ensure comprehensive test coverage.
These tests are marked as incomplete but appear to have functional counterparts already implemented. Verify if they are still needed:
🏁 Script executed:
#!/bin/bash # Description: Check if there are other test methods that might already cover the functionality # of the incomplete test methods # Check for existing tests related to outbox and sentitems rg -B 3 -A 1 "test_get_messages_(outbox|sentitems)" ./tests/models/Length of output: 1897
Clarify Redundant Incomplete Test Cases
It appears that the methods
test_get_messages_outboxandtest_get_messages_sentitemsare marked as incomplete withmarkTestIncomplete(), yet similar tests already exist (test_get_messages_outbox_no_limit_no_offset,test_get_messages_sentitems_no_limit_no_offset, andtest_get_messages_sentitems_multipart_no_limit_no_offset). Please verify whether these incomplete tests are intended to cover additional scenarios not already tested by their counterparts. If they are truly redundant, consider removing them; otherwise, implement them to cover any missing test cases.
254-301:Details
❓ Verification inconclusive
Verify logic in ascending order test.
The test appears to be testing the ordering functionality correctly, but the expected ID calculation and message retrieval pattern could be clarified.
🏁 Script executed:
#!/bin/bash # Description: Check if there's proper documentation for the ordering functionality in Gammu_model # Search for documentation on ordering in the model rg -A 10 -B 5 "order_by.*ReceivingDateTime" --glob "*.php" ./application/models/Length of output: 1526
Clarify expected ordering logic in the inbox messages test.
The test in
tests/models/Gammu_model_get_messages_test.php(lines 254–301) uses'order_by_type' => 'asc'in the options, yet it expects the messages to be retrieved in an order where the highest ID appears first—consistent with descending order of IDs. Our investigation ofGammu_model.phpconfirms that the ordering is set with:$this->db->order_by('ReceivingDateTime', 'DESC');This indicates that even when the test supplies
'asc', the underlying implementation enforces a descending order (likely through business logic or default behavior). Although the ordering functionality is appropriately tested, the expected ID calculation and message retrieval pattern would benefit from an explanatory comment clarifying this inversion of order parameters versus actual behavior.
- Action items:
- Add or update inline comments in the test to explain why
'order_by_type' => 'asc'results in messages being retrieved with descending IDs.- Consider updating related documentation in the model if the behavior is intentional.
tests/models/Gammu_model_test.php (18)
1-15: Great comprehensive header and namespace setup!The copyright notice, license information, and file imports are well-structured and follow best practices.
16-23: Well-structured test class with appropriate setUp method.The setUp method correctly initializes the test environment by resetting the instance and loading the Gammu_model.
25-28: Good use of a database provider.Using a provider for database engines ensures that tests run across multiple database configurations, which is excellent for compatibility testing.
30-50: Comprehensive test data for Unicode message splitting.The data provider contains a variety of test cases including emojis, special characters, and multi-byte sequences, which thoroughly tests the Unicode handling capabilities.
52-63: Well-structured test for multipart Unicode messages.This test appropriately uses the data provider and thoroughly validates that the split messages match the expected output.
65-88: Comprehensive UTF-16 code unit counting test.This test covers a wide range of character types including emojis, Latin and non-Latin scripts, which is essential for proper SMS character counting.
90-134: Good test coverage for string splitting functionality.The test includes commented-out cases that aren't supported by mb_* functions, showing awareness of the limitations of PHP's multibyte string handling.
136-171: Well-designed test for special character detection.The test accurately verifies that GSM 7-bit special characters are correctly identified, which is crucial for SMS message length calculation.
195-246: Thorough testing of message multipart functionality.The test cases cover various scenarios including Unicode messages, GSM 7-bit messages, and special cases with characters at specific positions.
248-346: Comprehensive WAP link sending tests with database verification.The tests include multiple scenarios and verify both successful execution and appropriate error handling. The database validation ensures that messages are correctly inserted.
369-441: Well-structured tests for error handling.These tests appropriately check both log messages and exceptions when WAP link sending fails, ensuring robust error handling.
449-501: Comprehensive test for WAP link message sending.The test properly configures the environment, executes the function, and verifies the expected output and database state.
503-538: Good validation of parameter checking.The tests ensure that the function correctly rejects invalid inputs, which is essential for preventing erroneous message sending.
540-663: Thorough testing of Unicode and GSM 7-bit single messages.The tests cover different character encodings and verify all aspects of the message sending process, including the database state.
665-958: Comprehensive tests for multipart messages and custom parameters.These tests verify various scenarios including multipart Unicode and GSM 7-bit messages, flash messages, and custom parameter overrides.
960-1053: Well-structured tests for message routing.These tests verify that messages are correctly routed and inserted into the database with the proper attributes.
1055-1117: Thorough testing of multipart message handling.The tests verify that multipart messages are correctly stored in the database with the appropriate UDH, position, and coding values.
173-193:Details
❓ Verification inconclusive
Verify parameter usage in _get_message_length test.
In the function call to
_get_message_length, you're passing$expectedas the third parameter, which seems unusual. Typically, you wouldn't pass the expected result to the method being tested.
🏁 Script executed:
#!/bin/bash # Check the signature of the _get_message_length method ast-grep --pattern $'function _get_message_length($text, $coding, $length) { $$$ }'Length of output: 86
Below is a revised review comment asking for a closer manual check of the method signature. Since our initial AST search did not return the expected signature information, please re-run a manual verification (or a more refined script search) to conclusively confirm whether the
_get_message_lengthmethod is designed to accept three parameters. This will help determine if passing$expectedas the third argument is intentional or a mistake.Review Focus: Verify Parameter Usage in _get_message_length
- The test currently calls
_get_message_length($input, $coding, $expected), passing the expected value as the third argument.- This usage appears unusual since, typically, the expected result is not supplied to the method being tested but is instead compared against its return value.
- Action: Manually verify that the
_get_message_lengthmethod (likely defined in the model file, e.g.,models/Gammu_model.php) indeed accepts three parameters and that the third parameter is intended as an input (such as an initial length or accumulator) rather than its expected output. Update the test or the method accordingly if this is not the case.application/plugins/sms_to_xmpp/views/index.php (1)
5-5: Good migration from ID to class selector.This change from
id="validateTips"toclass="validateTips"follows best practices for HTML5. IDs must be unique in a document, while classes can be reused, making this a more maintainable approach for validation messages that may appear in multiple places.tests/.gitignore (1)
2-3: Improved gitignore settings for test artifacts.These additions are appropriate:
.phpunit.result.cacheexcludes PHPUnit's cache file which shouldn't be committed- Ignoring the entire
build/directory is cleaner than ignoring specific subdirectories, assuming all content in this directory is generated during testingThis change ensures that temporary test artifacts don't pollute the repository.
application/views/main/phonebook/contact/pbk_list.php (2)
18-18: Good transition from ID to class selectors.Changing from
id="pbkname"andid="pbknumber"to class attributes improves HTML validity and allows for reuse. Since these elements might appear multiple times in a contact list, classes are more appropriate than IDs which must be unique.Also applies to: 20-20
31-31: Enhanced accessibility with image alt attributes.Adding
alt="dot"to image tags improves accessibility for users with screen readers or when images fail to load. This change helps meet WCAG accessibility guidelines.Also applies to: 36-36, 39-39
application/views/main/settings/filters.php (3)
2-2: Replaced deprecated HTML attribute with CSS styling.Replacing
align="center"withstyle="text-align: center"is a good update. Thealignattribute is deprecated in HTML5, so using CSS for presentation is the proper approach.
26-26: Added alt attribute to improve accessibility.Adding the
alt="dot"attribute to the image tag improves accessibility for screen readers and is required for valid HTML5.
49-49: Fixed form label association.Changing the label's
forattribute frommove_totoid_foldercorrectly associates the label with its corresponding form field (which hasid="id_folder"). This improves accessibility and usability.application/plugins/sms_credit/views/packages.php (2)
38-38: Good accessibility improvement!Adding the
alt="dot"attribute to the image tag enhances accessibility by providing a text description for screen readers and other assistive technologies.
53-53: Good HTML5 modernizationChanging from
id="validateTips"toclass="validateTips"is a good practice, especially if this element style is reused across multiple pages. IDs must be unique on a page while classes can appear multiple times.tests/testutils/DBVars.php (1)
14-16:Details
❓ Verification inconclusive
Verify security of hardcoded database credentials
While these appear to be test credentials, ensure they are not used in production and that this file is properly managed in version control. Consider using environment variables for these values if they need to be different across environments.
🏁 Script executed:
#!/bin/bash # Check if the credentials are only used in test contexts # Look for usage of these constants in production code echo "Checking for usage of test credentials in non-test code..." rg -l "DBVars::DATABASE|DBVars::USERNAME|DBVars::PASSWORD" --glob "!tests/**"Length of output: 206
Review Update: Ensure Test Credentials Are Isolated & Verify Their Usage
Your changes in tests/testutils/DBVars.php (lines 14–16) now contain test-specific credentials. A command was executed to check for usage of these constants in non-test code and returned no results—indicating they likely aren’t referenced in production. However, the absence of output makes it hard to fully confirm that these values aren’t used elsewhere.
- Action Required: Please manually verify that the
DBVarsconstants (DATABASE, USERNAME, PASSWORD) are not referenced outside test contexts.- Consideration: If these values are needed in environments other than tests, consider using environment variables to allow configuration changes across environments.
application/views/main/phonebook/group/pbk_list.php (3)
7-7: Good use of data attributeAdding the
data-publicattribute is a modern HTML5 approach for storing custom data that can be easily accessed via JavaScript's dataset API.
11-11: Good HTML5 modernizationChanging from
id="pbkname"toclass="pbkname"follows best practices since this element likely appears multiple times in the phonebook list. Classes are meant for reusable elements while IDs must be unique on a page.
20-20: Good accessibility improvementAdding the
alt="dot"attribute to the image enhances accessibility by providing a text alternative for screen readers and other assistive technologies.application/views/main/users/add_user.php (2)
5-5: Good HTML5 modernizationChanging from
id="validateTips"toclass="validateTips"is appropriate if this validation message style is used in multiple places across the application.
8-8: Improved form accessibilityCorrecting the
forattributes in labels to match their corresponding inputidattributes (realname,phone_number,username) significantly improves accessibility. This ensures that clicking on a label properly focuses the associated form field and helps screen readers correctly associate labels with inputs.Also applies to: 13-13, 18-18
application/views/main/messages/navigation.php (8)
36-36: Improved HTML semantics by replacing button with anchor linkThe change from a button element with an ID to a div containing an anchor with appropriate classes follows HTML5 best practices. This improves semantics while maintaining functionality.
41-42: Improved markup for action buttonsReplacing button elements with semantic anchor links wrapped in div containers improves accessibility and follows modern HTML5 standards.
48-51: Enhanced semantic structure for spam control buttonsThe conversion from buttons to anchor links maintains functionality while improving HTML structure. Using semantic HTML elements like this helps with accessibility.
62-65: Proper semantic markup for action buttonsConverting these buttons to anchor links wrapped in divs improves the semantic structure while preserving functionality.
66-73: Improved structure for delete buttonThe conversion to an anchor link maintains functionality while improving HTML semantics. The button closing tag is now properly aligned with the opening tag, improving code readability.
78-81: Enhanced semantic structure for refresh buttonsConverting these buttons to anchor links provides better semantics and accessibility while maintaining the same functionality.
88-88: Improved structure for resend buttonReplacing the button with a semantic anchor link improves HTML structure and follows best practices.
94-94: Changed ID to class for pagination containerConverting from an ID to a class for the pagination container follows best practices for CSS selectors, especially since pagination may appear multiple times in a document.
application/views/main/phonebook/contact/index.php (4)
13-14: Replaced deprecated<big>tag with CSS stylingGood replacement of the deprecated HTML
<big>tag with a modern CSS approach using<span style="font-size: larger">. This improves HTML5 compliance.
18-18: Replaced deprecated<big>tag with styled spanAnother good replacement of the deprecated HTML
<big>tag with semantic markup and CSS styling.
26-26: Changed ID to class for validation tipsConverting from an ID to a class for validation tips follows best practices, as these messages might appear multiple times in different contexts.
42-42: Fixed label association with form controlThe
forattribute now correctly references the ID of the associated select element (importgroupvalue), improving accessibility by properly associating the label with its form control.application/views/main/phonebook/group/index.php (1)
18-18: Improved label association with input fieldThe
forattribute now correctly references the ID of the associated input element (group_name), improving accessibility and following HTML best practices.application/plugins/sms_credit/views/index.php (9)
38-38: Added alt attribute to image for accessibilityAdding the
alt="dot"attribute to the image improves accessibility for screen readers and follows HTML5 standards.
59-59: Changed ID to class for validation tipsConverting from ID to class for validation tips follows best practices for CSS selectors and allows for multiple validation messages in different contexts.
62-67: Fixed label associations with input fieldsThe
forattributes of these labels now correctly reference the IDs of their associated input fields (realname,phone_number,username), enhancing accessibility and following HTML best practices.
75-76: Added ID to dropdown for proper form control identificationAdding an ID attribute to the dropdown select element improves accessibility and allows for proper label association.
87-89: Added ID to package dropdown for proper form control identificationAdding an ID attribute to the package dropdown select element improves accessibility and allows for proper label association.
104-104: Changed ID to class for validation tips in edit dialogConverting from ID to class for validation tips in the edit dialog follows best practices for CSS selectors and maintains consistency with other similar elements.
107-115: Fixed label association with package dropdownThe
forattribute now correctly references the ID of the associated select element (edit_id_package), improving accessibility.
118-119: Fixed label association with date inputThe
forattribute now correctly references the ID of the associated date input (edit_package_start), improving accessibility.
121-122: Fixed label association with end date inputThe
forattribute now correctly references the ID of the associated date input (edit_package_end), improving accessibility.application/views/main/install/layout.php (3)
1-3: Good update to HTML5 doctypeUpdating to HTML5 doctype is appropriate and follows modern web standards. This change is in line with the PR objective of migrating to valid HTML5.
42-42: Great accessibility improvementAdding an alt attribute to the image improves accessibility for screen readers and complies with WCAG guidelines.
43-56: Clean HTML structure with proper indentationThe HTML structure is clean and well-indented, which contributes to better maintainability of the code.
application/plugins/stop_manager/views/index.php (4)
19-19: Good removal of deprecated attributesRemoving the deprecated cellpadding and cellspacing attributes from the table element is appropriate for HTML5 compliance.
46-46: Improved semantic markupReplacing the img tag with a span for the icon is better for semantic HTML. Using CSS classes for icons rather than image elements is a modern approach.
59-59: Better practice using class instead of IDConverting from ID to class for pagination elements is a good practice, especially if the element might appear multiple times on a page or across different views.
77-77: Appropriate class namingChanging from ID to class for validateTips improves code maintainability and follows best practices for CSS selectors.
.github/workflows/codeigniter.yml (7)
79-80: Clear step namingGood practice to use descriptive names for CI/CD steps to improve readability and maintainability.
83-86: Proper system dependencies setupInstalling necessary system dependencies and setting up database users properly is important for the CI environment. This appears to be replacing the previous MySQL setup step with a more comprehensive approach.
87-94: Thorough Java availability checkThe Java check is comprehensive, trying multiple paths before installing if necessary. This ensures the HTML validation tools will work correctly.
95-104: Good configuration for validator downloadUsing a specific release of the validator tool ensures consistent test results, which is important for CI.
105-109: HTML validation tooling properly setupInstallation of tidy and v.Nu (HTML5 validator) supports the PR objective of migrating to valid HTML5 by enabling HTML validation in the CI pipeline.
111-111: Simplified test preparationUsing a dedicated script to prepare tests improves maintainability and keeps the workflow file cleaner.
113-113: HTML5 output specificationSetting the HTML environment variable to 5 aligns with the PR objective of migrating to HTML5.
application/views/main/base.php (9)
4-4: Improved image accessibilityAdding descriptive alt text to the mascot image improves accessibility for screen readers.
8-9: Modernized HTML markupReplacing deprecated centering methods with CSS text-align is a good HTML5 practice.
40-42: Properly commented out donation sectionThe commented section is neatly formatted, making it clear that this content is intentionally disabled.
80-80: Corrected label associationUpdating the 'for' attribute to match the corresponding input ID improves accessibility and follows HTML standards.
96-97: Removed deprecated valign attributeGood practice to remove the deprecated valign attribute in favor of CSS styling.
102-102: Consistent styling with classesConverting from inline alignment attributes to semantic class names is a good practice that separates content from presentation.
Also applies to: 122-122, 141-141, 163-163, 192-192
229-229: Clean table stylingUsing CSS for table styling rather than deprecated HTML attributes follows modern web standards.
231-231: Improved form accessibilityEnsuring label 'for' attributes match their corresponding input IDs improves accessibility for screen readers and keyboard navigation.
Also applies to: 235-235, 239-239, 257-257, 263-263, 273-273
241-241: Consistent select stylingUsing inline styles consistently for select elements ensures uniform appearance across the form.
Also applies to: 265-265, 275-275
application/views/js_init/js_keyboard.php (1)
133-133:Details
✅ Verification successful
Good migration from ID to class selector.
This change replaces the ID selector
#back_threadlistwith a class selector.back_threadlist.first(), which aligns with HTML5 best practices. Classes are more appropriate for elements that might appear multiple times in the DOM, and this change improves code maintainability.
🏁 Script executed:
#!/bin/bash # Check if there are other files referencing back_threadlist to ensure all instances have been updated properly echo "Checking for back_threadlist in HTML and PHP files..." rg -t html -t php "back_threadlist" --context=2Length of output: 18570
Migration to class selector verified and approved
The change in
application/views/js_init/js_keyboard.php(line 133) correctly updates the selector from#back_threadlistto use the class-based approach:var dest = $('.back_threadlist').first().attr('href');Our verification confirms:
- The element is now correctly selected using a class, which is more appropriate for potential multiple instances in the DOM.
- Consistent usage of
.back_threadlistacross HTML/PHP views and test files (e.g., innavigation.phpand various test cases) aligns with HTML5 best practices and improves maintainability.application/views/js_init/phonebook/js_phonebook.php (1)
172-172:Details
✅ Verification successful
Good migration from ID to class selectors.
This change properly replaces ID selectors (
#pbknameand#pbknumber) with class selectors (.pbknameand.pbknumber), which is more appropriate for the DOM structure. This aligns with the pull request's objective to migrate to valid HTML5.
🏁 Script executed:
#!/bin/bash # Check if the corresponding HTML elements have also been updated from ID to class echo "Checking for pbkname and pbknumber in HTML files..." rg -t html "pbkname|pbknumber" --context=2Length of output: 157
Attention: Verify HTML/PHP Template Changes for Selector Migration
The migration inapplication/views/js_init/phonebook/js_phonebook.phpfrom ID selectors to class selectors appears to be correct. However, the initial search forpbknameandpbknumberin HTML files did not return any results. This might be due to differences in file types or template locations. Please run the above script (or verify manually) to ensure that the corresponding HTML/PHP template files have been updated to use the class selectors appropriately.
🏁 Script executed:
#!/bin/bash echo "Searching for updated selectors 'pbkname' and 'pbknumber' in PHP and HTML files..." # Search in PHP files (common for templates) rg -t php "class\s*=\s*[\"'][^\"']*\b(pbkname|pbknumber)\b" --context=2 # Search in HTML files rg -t html "class\s*=\s*[\"'][^\"']*\b(pbkname|pbknumber)\b" --context=2Length of output: 1826
Good migration confirmed – Class selectors for pbkname/pbknumber are consistently in use
The changes in
application/views/js_init/phonebook/js_phonebook.phphave been verified against the PHP templates (e.g.,application/views/main/phonebook/contact/pbk_list.phpandapplication/views/main/phonebook/group/pbk_list.php), which correctly use the class selectors (.pbknameand.pbknumber). No discrepancies were found.application/views/js_init/message/js_conversation.php (1)
99-99:Details
❓ Verification inconclusive
Good use of data attribute instead of ID.
This change from using the
idattribute to using the HTML5data-id_folderattribute improves semantic structure and separates styling/selection concerns from data storage. This is a recommended practice in modern HTML5 development.
🏁 Script executed:
#!/bin/bash # Check if the corresponding HTML elements have been updated to use data-id_folder echo "Checking for move_to elements with data-id_folder attribute..." rg -t html "move_to.*data-id_folder" --context=2Length of output: 185
Attention: Verify Consistent HTML Template Updates
The change in
application/views/js_init/message/js_conversation.php—using$(this).attr('data-id_folder')instead of the oldidattribute—is a good move towards a more semantic, HTML5-friendly approach. However, our automated search for HTML elements (using thergcommand on patterns related tomove_toanddata-id_folder) did not return any results. This means we couldn't confirm if the corresponding HTML templates have been updated to employ thedata-id_folderattribute consistently.Please manually verify that the relevant HTML files or other view components include the updated attribute usage to avoid any discrepancies across the codebase.
tests/plugins/Sms_credit_test.php (5)
12-16: LGTM! Required dependencies properly importedThe test file correctly imports all necessary utility classes for the tests to function.
19-26: Good setup for test isolationThe
setUpmethod correctly ensures that any installation file is removed before running tests, which helps with having a clean test environment.
41-67: Test remains incomplete - consider implementing assertionsWhile the test structure is correct, this test is marked as incomplete and needs implementation. Consider adding assertions to verify that the SMS credit plugin's index page works correctly.
The test currently checks if the HTML is valid, but it should also verify that the expected content is present on the page. For example, it could check if the SMS credit dashboard elements are rendered correctly.
72-98: Test remains incomplete - needs implementation for packagesThis test for the packages functionality is marked as incomplete and should be implemented with proper assertions to verify the packages page's functionality.
In addition to validating HTML structure, consider adding assertions to verify that:
- The SMS packages are correctly displayed
- The package-specific elements (prices, quantities, etc.) are rendered properly
- The form for adding new packages works as expected (if applicable)
33-36: Good practice using a data provider for database enginesUsing a static data provider method to test across multiple database engines is a good practice for ensuring compatibility.
application/views/js_init/message/js_function.php (1)
148-148:Details
❓ Verification inconclusive
Improved attribute usage with data- attributes*
Great improvement by changing from using the element's ID attribute to a data attribute. This follows modern HTML5 best practices by separating structural identification from data storage.
Make sure all corresponding HTML templates have been updated to include this
data-id_folderattribute on the elements with class.move_toinstead of relying on ID attributes.
🏁 Script executed:
#!/bin/bash # Find all templates that use the move_to class to ensure they have the data-id_folder attribute rg --type html "class=\"move_to\"" application/views/Length of output: 53
Ensure Corresponding HTML Templates Include
data-id_folderGreat job updating the JS code to retrieve
data-id_folderrather than using the element’s ID. However, our automated search (usingrg) for HTML templates withclass="move_to"did not yield conclusive evidence that they now include thedata-id_folderattribute. Please verify manually that all relevant templates inapplication/views/have been updated accordingly.composer.json (1)
63-65: Properly extended PHP version support.Adding support for PHP 8.4 is a good forward-looking change that aligns with the existing pattern of PHP version compatibility patches in this file. This complements the various other PHP 8.4 patches already in place for other dependencies.
application/views/main/layout.php (3)
3-3: Successfully migrated to HTML5 doctype.The change from the previous doctype to HTML5 is appropriate and aligns with modern web standards. This is a good step toward the PR objective of migrating to valid HTML5.
17-17: Improved accessibility with alt attribute.Adding the alt attribute to the logo image improves accessibility for screen readers and is compliant with HTML5 standards and WCAG guidelines.
23-23: Replaced deprecated HTML attribute with CSS styling.Good modernization by replacing the deprecated
alignattribute with proper CSS styling. This change adheres to HTML5 standards where presentation should be separated from content.application/views/main/messages/index.php (2)
19-19: Enhanced data attribute usage for semantic HTML.Excellent improvement by replacing the generic
idattribute with the more semanticdata-id_folderattribute. This approach better separates the identification of elements from their styling and behavior, aligning with modern HTML5 practices.
36-36: Improved form accessibility.Good update to the
forattribute that now correctly references the matching input fieldedit_folder_name. This ensures proper label-input association, which is important for accessibility and usability.application/views/main/settings/appearance.php (2)
1-1: Replaced inline attributes with CSS class.Good migration from inline HTML attributes (
width="100%"andcellpadding="5") to a class-based approach using the "settings" class. This separation of content and presentation follows HTML5 best practices.
2-2: Removed deprecated vertical alignment attributes.Proper cleanup by removing the deprecated
valign="top"attributes from table rows. In HTML5, this styling should be handled through CSS instead of HTML attributes.Also applies to: 13-13
application/views/main/forgot_password.php (6)
3-3: Good transition to HTML5!Changing the doctype declaration to HTML5 using CodeIgniter's helper is a positive step towards modernizing the application.
28-28: Improved semantic structure with div container.Replacing the deprecated
<center>tag with a semantic<div>container is good practice for HTML5 compliance. This change improves the document structure while maintaining the same visual presentation.Also applies to: 69-69
35-35: Accessibility improvement with alt attribute.Adding the alt attribute to the logo image enhances accessibility for screen readers and complies with WCAG guidelines.
36-36: CSS-based alignment is better than deprecated HTML attributes.Using
text-align: centervia CSS instead of the HTMLalignattribute follows modern web standards.
46-46: Table attributes moved to CSS.Removing inline HTML table attributes (cellpadding, cellspacing, border) in favor of CSS-based styling is the right approach for HTML5.
48-48: Modern text styling approach.Replacing the deprecated
<big>tag with a<span>using font-size styling is better for maintainability and follows HTML5 best practices.application/views/main/login.php (7)
3-3: Good transition to HTML5!Changing the doctype declaration to HTML5 is consistent with the changes in other views and aligns with modern web standards.
28-28: Improved semantic structure with div container.Replacing the deprecated
<center>tag with a semantic<div>container follows HTML5 best practices.Also applies to: 74-74
35-35: Enhanced accessibility with alt attribute.Adding the alt attribute to the logo image is essential for accessibility and complies with WCAG guidelines.
36-36: CSS-based alignment is the right approach.Using
text-align: centervia CSS instead of the HTMLalignattribute follows modern web standards.
48-48: Modern table styling approach.Removing inline HTML table attributes in favor of CSS-based styling is appropriate for HTML5.
63-63: Clean button styling.The updated styling for the submit button is clean and follows modern web practices.
67-67: Link styling improvement.Using CSS for text alignment and link styling rather than HTML attributes is a good modernization step.
media/css/install.css (5)
4-5: Improved body layout with auto margins and explicit width.The changes to the body margin and width provide better centering and layout control. Using
automargins horizontally centers the content, which is good for responsive design.Note that
width: fit-contentwill make the body shrink to the width of its content rather than filling the viewport. Ensure this is the intended behavior, especially for smaller screens.
114-115: Improved table rendering with proper border collapse.Adding
border-collapse: collapseandborder-spacing: 0to.simpletableensures consistent table rendering across browsers and prevents double borders.
147-147: Full-width tables for better layout.Setting
.formtabletowidth: 100%ensures it takes up the full available space, improving form layout consistency.
152-152: Better form field alignment.Adding
vertical-align: topto.formtable tdimproves the alignment of form fields, especially when labels and inputs have different heights.
176-180: Clean section header styling.The new
.h4.sectionclass provides consistent styling for section headers with centered text, padding, and a subtle border. This improves visual hierarchy and readability.application/views/main/password_reset.php (7)
3-3: Good transition to HTML5!Changing the doctype declaration to HTML5 is consistent with the changes in other views and aligns with modern web standards.
28-28: Improved semantic structure with div container.Replacing the deprecated
<center>tag with a semantic<div>container follows HTML5 best practices.Also applies to: 68-68
35-35: Enhanced accessibility with alt attribute.Adding the alt attribute to the logo image improves accessibility for screen readers.
36-36: CSS-based alignment is the right approach.Using
text-align: centervia CSS instead of the HTMLalignattribute follows modern web standards.
47-47: Modern table styling approach.Removing inline HTML table attributes in favor of CSS-based styling is appropriate for HTML5.
52-56: Good use of HTML5 form validation attributes.The use of the
minlengthattribute on password fields enhances client-side validation, which is a good security practice.
59-59: Clean button styling.The updated styling for the submit button is clean and follows modern web practices.
application/plugins/whitelist_number/views/index.php (5)
6-6: Good semantic HTML update from ID to classChanging from
id="validateTips"toclass="validateTips"improves the HTML semantics since the same validation message style can now be used multiple times in the document.
18-18: Good semantic HTML update from ID to classSimilar to the previous change, using
class="validateTips"rather than an ID allows for consistent styling across multiple validation messages.
50-50: Corrected colspan valueGood adjustment of the colspan value from 5 to 4, which properly matches the table structure.
58-59: Improved accessibility by replacing images with semantic elementsReplacing image tags with span elements that use CSS icons improves accessibility and reduces page load time.
69-69: Good semantic HTML update from ID to classConverting
id="simplepaging"toclass="simplepaging"allows for better reusability if there are multiple pagination sections in the application.application/views/main/install/requirement_check.php (3)
5-5: Improved HTML by removing deprecated attributesRemoving the
border,cellspacing, andcellpaddingattributes from the table and relying solely on CSS through theclassattribute follows modern HTML best practices.
166-166: Fixed HTML structure by removing unnecessary closing tagThe improvement simplifies the HTML structure by removing an unnecessary
</td>tag that was previously after the<?php endif; ?>.
196-207: Cleaner code structure for form renderingThe restructuring of the PHP code that renders the forms improves readability and maintains a cleaner separation between PHP and HTML.
tests/plugins/Sms_to_wordpress_test.php (3)
17-26: Well-structured test class setupThe test class follows good practices with a clear setup method that properly handles test initialization by checking for and removing any existing installation file.
33-36: Good use of data providerUsing a data provider to test with different database engines is a good practice that allows thorough testing across different configurations.
41-67: Test is marked as incomplete but appears to have functional assertionsThe test makes assertions about valid HTML but then marks itself as incomplete. Consider either:
- Completing the test by removing the
markTestIncomplete()call if the current assertions are sufficient- Adding a comment explaining what additional assertions are needed to complete the test
Is this test intentionally incomplete, or are the current assertions sufficient for testing the plugin? Consider adding a comment explaining what's missing.
tests/libraries/Install_info_test.php (5)
16-24: Well-structured test classThe test class follows PHPUnit best practices with a clear setup method that calls the parent method.
41-52: Good handling of PHPUnit version compatibilityThe code correctly handles different PHPUnit versions by checking the version and using the appropriate method names (
onlyMethodsvssetMethods).
59-60: Effective unit test with clear assertionThe test for
get_daemon_pathcorrectly validates that the method returns the expected path with the appropriate extension based on the operating system.
107-126: Comprehensive data provider and test implementationThe test for
get_daemon_var_pathuses a comprehensive data provider to test various scenarios, including edge cases and invalid inputs.
128-169: Thorough testing of CI_ENV detectionThe test for
contains_CI_ENVincludes a variety of edge cases, including different formatting, commented lines, and empty values, which is excellent for ensuring robust behavior.application/plugins/blacklist_number/views/index.php (4)
5-5: Good conversion from ID to class for validation message.Converting from ID to class is appropriate since this element appears in multiple dialogs and IDs must be unique in valid HTML5.
19-19: Good conversion from ID to class for validation message.Consistent with the earlier change, converting from ID to class for validation messages improves HTML5 validity.
66-67: Good replacement of img tags with span elements.Replacing img tags with span elements using CSS classes for icons is a modern approach that improves maintainability and follows better HTML5 practices.
77-77: Good conversion from ID to class for pagination.Converting from ID to class for the pagination container is appropriate, especially if this pattern is used in multiple places throughout the application.
application/plugins/server_alert/views/index.php (7)
6-6: Good conversion from ID to class for validation message.Converting from ID to class is appropriate since this element appears in multiple dialogs and IDs must be unique in valid HTML5.
32-32: Good conversion from ID to class for validation message.Consistent with the earlier change, converting from ID to class for validation messages improves HTML5 validity.
58-71: Hidden row hack for HTML validation.The hidden row with empty table headers is a valid workaround for HTML5 validation. The number of elements (10) correctly matches the total number of columns in the table.
80-80: Good conversion from HTML attribute to CSS styling.Converting from HTML
align="center"attribute to CSSstyle="text-align: center"is appropriate for HTML5 compliance as the align attribute is deprecated in HTML5.
86-86: Correct colspan adjustment.The colspan value has been properly updated from 8 to 10 to match the total number of columns in the table after the changes.
101-101: Good replacement of img tags with span elements.Replacing img tags with span elements using CSS classes for icons is a modern approach that improves maintainability and follows better HTML5 practices.
Also applies to: 105-106
115-116: Correct colspan and class updates.The colspan value has been properly updated to match the total number of columns, and the pagination div now uses a class instead of an ID, which is appropriate for HTML5 compliance.
application/plugins/soap/views/index.php (7)
13-13: Good shift to class-based styling
Switching from anidattribute to a class name aligns with HTML5 best practices and enables more flexible styling.
30-30: Consistent change to class selector
Maintaining a consistent naming pattern helps ensure maintainability and clarity in styling and scripts.
72-72: Use of style attribute for alignment
Replacing the deprecatedalignattribute withstyle="text-align: center"is recommended for modern HTML markup.
102-102: Pagination block seems straightforward
No apparent issues with the pagination markup; it appears semantically correct.
115-115: Consistent class naming
Mirrors the earlier update for field validation tips. Good for uniform code clarity.
150-150: Simple conditional closure
Thisendif;appears to be a minor indentation or spacing change. Nothing critical.
152-153: Verify table column alignment
Colspan was reduced from 8 to 4. Double-check that this still matches the visible columns to maintain a proper layout.tests/plugins/Jsonrpc_test.php (4)
1-16: Header and imports
The file-level comments and included test utilities are well-organized. No issues identified.
17-26: setUp method ensures environment readiness
The default setup logic is clear, although the DB setup call is commented out. Confirm whether that is intentional or if it needs reactivation.
33-36: database_Provider method is straightforward
Providing multiple database engines for testing is helpful for coverage; implementation looks correct.
127-164:test_send_sms_empty_datavalidation
This test verifies a parse-error response, which helps ensure robust JSON-RPC error handling. Implementation looks solid.tests/controllers/Install_test.php (17)
3-10: Updated docblock
The year range and license details are correct, aligning with project conventions.
12-15: Requiring new test utility files
The added files (ConfigFile,DBSetup,KalkunTestCase) support an extended test framework. This inclusion is consistent.
16-16: Class now extendsKalkunTestCase
Switching fromTestCaseto a specialized base class is appropriate for uniform test features.
20-26: Ensuringinstallfile presence
ThesetUpmethod logic clarifies test scenarios for the installation assistant. Implementation is clear.
29-31: Revisedtest_index
Uses_assertStringContainsStringfor checking output. This fosters more precise test validations.
33-47:test_index_disabled
Properly verifies the scenario where theinstallfile is removed. The 403 response code check is valid.
49-55:test_config_setup_GET
Confirms that the final config step page loads correctly. Straightforward assertion.
57-63:test_config_setup_POST_remove_install_file
Validates that the script can remove or signal manual removal of theinstallfile. Looks good.
65-81:test_config_setup_POST_install_not_writable
Temporarily changing directory permissions to test error handling is effective. Good to see permissions restored.
83-94:test_requirement_check
Ensures the requirement-check page loads, verifying the presence of expected HTML tags.
123-126: Provider for multiple DB engines
This approach fosters broad test coverage with minimal duplication.
128-144:test_database_setup_GET
Uses config scenarios from the data provider to verify different database setups. Implementation is consistent.
146-187:test_database_setup_GET_with_db_exception
Covers edge cases like missing DB or read-only file system for SQLite. Agree with skipping certain checks for known quirks.
189-201:database_setup_run_db_setupProvider
Listing multiple upgrade paths is beneficial for testing all legacy upgrade routines.
203-223:test_database_setup_POST_run_db_setup
Ensures the next step button is displayed after running the DB setup. Logical flow.
225-233:uses_default_encryption_keyProvider
Explicit coverage for different encryption keys. Straightforward approach.
235-247:test_uses_default_encryption_key
Verifies custom vs. default encryption key usage, ensuring secure configuration.application/controllers/Install.php (3)
220-220: Good refactoring to use a dedicated library for installation info.The introduction of the Install_info library follows good software design principles by extracting installation-related functionality into a dedicated class.
239-255: Clear improvement using library methods instead of private controller methods.Moving from private controller methods to dedicated library methods improves code organization and maintainability. The code is now more modular and follows better separation of concerns.
268-269: Consistent use of the Install_info library methods.The migration from private methods to library methods is consistently applied across the controller, improving the overall design.
media/css/base.css (7)
468-486: Good migration from ID to class selectors for improved modularity.Converting from ID-based selectors (
#paging) to class-based selectors (.paging) improves CSS reusability and modularity, aligning with HTML5 best practices.
486-528: Well-structured CSS for pagination components.The CSS for simplepaging maintains proper styling while transitioning from ID to class selectors. The CSS is well-organized and properly formatted.
532-555: Good refactoring of dashboard box styling from IDs to classes.This change aligns with the overall strategy of moving from ID-based to class-based selectors, making the dashboard components more reusable.
707-717: Good addition of table styling for search forms.The new CSS for
sms_search_formtables ensures proper table rendering with collapsed borders and appropriate spacing.
1202-1218: Well-structured button group styling.The updated styling for button groups using div elements instead of button elements provides better structural support and consistent appearance.
1220-1252: Good addition of table layouts for structural components.The new CSS for shortcuts and system info components ensures proper vertical alignment and column widths.
1253-1279: Good standardized table styling for settings components.The new CSS for settings and SMS-to-email tables provides consistent padding and width values, improving the overall UI consistency.
application/libraries/Install_info.php (9)
1-14: Well-structured class header with proper documentation.The file header includes all necessary license and copyright information, following project standards for code documentation.
16-20: Simple and effective environment retrieval method.The
get_CI_ENVmethod is concise and properly handles the case when the environment variable is not set by returning an empty string.
21-52: Good implementation of htaccess path detection.The method checks multiple possible locations for the .htaccess file, properly handles readability checks, and uses the helper method
contains_CI_ENVto validate the file contents.
54-73: Well-designed pattern matching for CI_ENV detection.The method creates different regular expression patterns based on whether CI_ENV is set or not, and properly validates matches.
75-101: Good platform-aware daemon path resolution.The method determines the correct file extension based on the OS and checks multiple possible locations, enhancing compatibility across different environments.
103-133: Thorough variable extraction with proper error handling.The
get_daemon_var_pathmethod includes multiple validation steps before attempting to extract variables from files, ensuring robust behavior.
136-149: Simple and effective path existence validation.The
daemon_var_path_existsmethod provides a clean abstraction for checking path validity and readability.
151-179: Robust URL extraction with proper validation.The
get_daemon_urlmethod includes thorough input validation and uses a well-crafted regular expression to extract URL information from daemon scripts.
181-184: Platform detection method is concise and effective.The
is_windowsmethod uses a case-insensitive comparison of the first three characters of PHP_OS, which is the standard approach for detecting Windows environments.tests/controllers/Phonebook_test.php (3)
1-15: Well-structured test file with proper includes.The test file properly includes all necessary utility files and follows the project's header documentation standard.
16-26: Good test setup with environment preparation.The
setUpmethod ensures the test environment is clean by removing the installation file and setting up the database correctly.
27-35: Good test data provider implementation.The class includes helper methods for reloading CodeIgniter with different database engines and a data provider to support testing across multiple database types.
tests/plugins/Soap_test.php (1)
19-26: Check concurrency safety when removing the install file.
This code forcibly removes theinstallfile, which may lead to concurrency issues if multiple tests or processes attempt to remove or recreate it simultaneously. Consider adding concurrency controls or verifying that the environment is locked before removing the file.tests/controllers/Login_test.php (1)
19-23: Ensure concurrency safety when removing the install file.
Similar to other tests, forcibly deletinginstallmay cause race conditions if multiple tests run in parallel. Verify or lock the environment to avoid conflicts.tests/controllers/Daemon_test.php (1)
18-24: Remove install file concurrency check.
As in other test files, removing theinstallfile unconditionally can cause race conditions in parallel test runs. Confirm that only one test uses or manipulates this file at a time.tests/controllers/Pluginss_test.php (1)
174-201: Validate directory rename operations.The logic that renames the
pluginsdirectory toplugins.bakis wrapped in a try-catch-finally block, which is good for reverting changes if an exception occurs. Ensure that permission errors or disk-full scenarios are properly handled or at least logged to simplify debugging in CI environments.tests/controllers/Messages_my_folder_test.php (31)
29-32: No issues found inreloadCIwithEngine.
The method accurately configures the test database engine.
34-37: Data provider usage is clear and correct.
39-64: Test methodtest_my_folder_ajaxGET_invalidlooks good.
Coverage of invalid folder scenario is appropriate and response code is verified.
65-93:test_my_folder_ajaxGET_inbox_no_message_in_folderis well-structured.
This test properly checks for an empty folder case.
94-122:test_my_folder_ajaxGET_inbox_trash_no_message_in_folderis consistent.
No concerns; logic follows established patterns.
127-181:test_my_folder_ajaxGET_inbox_byConversation_single_unread_contactInPbkcovers critical UI checks.
Validates unread statuses and conversation toggles comprehensively.
186-231:test_my_folder_ajaxGET_inbox_byConversation_single_read_contactNotInPbkis detailed and works fine.
No issues noted.
236-290:test_my_folder_ajaxGET_inbox_notByConversation_single_unread_contactInPbkappears correct.
Good usage of configuration toggling.
296-341:test_my_folder_ajaxGET_inbox_notByConversation_single_read_contactNotInPbkis thoroughly tested.
No issues found.
358-409:test_my_folder_ajaxGET_inbox_byConversation_sortDescfully covers sorting logic.
Implementation shows no obvious flaws.
414-467:test_my_folder_ajaxGET_inbox_byConversation_sortDesc_withOffsetthoroughly validates pagination.
472-524:test_my_folder_ajaxGET_inbox_notByConversation_sortDescis consistent with the conversation grouping toggle.
530-583:test_my_folder_ajaxGET_inbox_notByConversation_sortDesc_withOffsetlooks good.
No issues identified.
589-641:test_my_folder_ajaxGET_inbox_notByConversation_sortAsclogic is correct.
646-699:test_my_folder_ajaxGET_inbox_notByConversation_sortAsc_withOffsetfully handles paging checks.
704-727:test_my_folder_ajaxGET_sentitems_no_message_in_folderproperly verifies an empty sent folder.
733-777:test_my_folder_ajaxGET_sentitems_byConversation_single_contactInPbkis well-designed.
No issues to report.
783-827:test_my_folder_ajaxGET_sentitems_byConversation_single_contactNotInPbkis consistent with the previous pattern.
844-895:test_my_folder_ajaxGET_sentitems_byConversation_sortDescthoroughly validates sorting.
901-953:test_my_folder_ajaxGET_sentitems_byConversation_sortDesc_withOffsetchecks pagination thoroughly.
All looks good.
959-1011:test_my_folder_ajaxGET_sentitems_notByConversation_sortDescis tested in depth.
1017-1069:test_my_folder_ajaxGET_sentitems_notByConversation_sortDesc_withOffsethandles offset logic well.
1076-1127:test_my_folder_ajaxGET_sentitems_notByConversation_sortAsccoverage is valid.
1133-1185:test_my_folder_ajaxGET_sentitems_notByConversation_sortAsc_withOffsetvalidates sorted pagination.
No issues.
1191-1272:test_my_folder_GET_inbox_byConversation_withOffseteffectively covers multi-page scenarios.
1278-1362:test_my_folder_GET_inbox_notByConversation_withOffsetis also well-handled.
1368-1453:test_my_folder_GET_sentitems_byConversation_withOffsetconfirms offset behavior properly.
1459-1544:test_my_folder_GET_sentitems_notByConversation_withOffsetlogic appears sound.
1550-1632:test_my_folder_GET_inbox_trash_byConversation_withOffsetchecks trash folder pagination thoroughly.
1638-1720:test_my_folder_GET_sentitems_trash_byConversation_withOffsetensures coverage of trash in sent items.
1726-1805:test_my_folder_GET_inbox_spam_byConversation_withOffsetcompletes the set of folder/pagination tests.
Everything is consistent here as well.tests/testutils/DBSetup.php (1)
60-71: Enhance error handling for MySQL config file.While creating the MySQL config file and writing the password, consider handling write or file permission failures more explicitly. Relying solely on the constructor to perform file writes may risk silent errors if file permissions are insufficient or the directory is not writable.
tests/controllers/Messages_test.php (2)
731-733: Verify merging of config settings with$configfile.You are directly writing PHP config code to
kalkun_settings.php. Ensure subsequent test runs or cleanup steps do not silently fail because of leftover config or unexpected merges with other test scenarios. Isolating test-specific config within a dedicated approach can mitigate cross-test interference.
958-991: Confirm membership logic beyond UI checks.
test_compose_member()asserts the presence of the “Member” option, but does not confirm how membership data is validated or integrated into the message composition pipeline. Strengthening these tests increases robustness, ensuring membership-based sends work as intended.tests/controllers/Kalkun_test.php (2)
545-578: Comprehensive phone number validation coverage
These tests effectively verify valid and invalid formats, handle single and multiple inputs, and confirm the correct JSON response. Great job adding these scenarios!
1103-1135: Good negative scenario for password updates
Validating the outcome when an incorrect current password is provided ensures robust authentication checks.
tenzap
force-pushed
the
feature-223-tests-and-html5
branch
from
a054b3e to
9f8c546
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (46)
tests/testutils/sql-gammu-1.37.4/mysql.sql (6)
12-20: Daemons Table Structure is Clear
Thedaemonstable is defined appropriately with the required text fields. Consider verifying that the use of the MyISAM engine is intentional—if transactional support or foreign key constraints become important, InnoDB might be a better choice.
32-35: Gammu Table Definition – Numeric Default Formatting
Thegammutable is simple and functional. Note that the default value forVersionis specified as'0'(a string). It is generally preferable to use an unquoted numeric value (i.e., 0) to avoid potential issues in strict SQL modes.
48-63: Inbox Table Structure – Numeric Defaults and Primary Key Naming
Theinboxtable is comprehensively defined. Consider revising numeric defaults (e.g., forClass) to remove quotes (using -1 instead of '-1') for compatibility with strict SQL modes. Additionally, the primary key is declared asPRIMARY KEY \ID` (`ID`); unless there is a specific need to name the key explicitly, you might simplify it toPRIMARY KEY (`ID`)`.
170-186: Phones Table – Verify Default Placeholder Values
Thephonestable uses default error values ('ERROR') for theNetCodeandNetNamecolumns and assigns -1 as the default forBatteryandSignal. Please verify that these placeholder defaults are intentional and appropriate for your testing and production contexts, as they may obscure missing or uninitialized data.
199-225: Sentitems Table – Comprehensive Definition with Indexing
Thesentitemstable is well-designed with a composite primary key and several indexes to optimize performance. Similar to earlier tables, consider using unquoted numeric defaults (e.g., forStatusError,TPMR, andRelativeValidity) to adhere to best practices in strict SQL modes.
227-229: Dumping Data Section for Sentitems
The file concludes with comments indicating the dumping of data for thesentitemstable; however, no actual data insertion follows. If this is intended as a placeholder, consider adding sample data for testing or removing these comments to reduce confusion.tests/plugins/Sms_to_xmpp_test.php (1)
28-31: Remove commented-out code that's not being used.This commented-out method isn't being used in the test. Either implement it if it's needed or remove it to keep the code clean.
- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/tests/plugins/Blacklist_number_test.php (1)
1-68: Add a test for creating and deleting blacklist entries.The current test only checks if the page loads properly, but doesn't test the actual functionality of creating, listing, or deleting blacklist entries. Consider adding more comprehensive tests.
Add a new test method like this:
/** * @dataProvider database_Provider */ public function test_create_and_delete_blacklist($db_engine) { // Setup similar to test_index $dbsetup = new DBSetup([ 'engine' => $db_engine, ]); $dbsetup->setup_config('gammu_no_pbk_kalkun_fresh_install_manual_sql_injection'); // Setup session $this->request->setCallablePreConstructor( function () { if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) { session_start(); } $_SESSION['loggedin'] = 'TRUE'; $_SESSION['id_user'] = '1'; $_SESSION['level'] = 'admin'; $_SESSION['username'] = 'kalkun'; } ); $dbsetup->install_plugin($this, 'blacklist_number'); Pluginss_test::reset_plugins_lib_static_members(); // Test creating a blacklist entry $number = '+1234567890'; $output = $this->request('POST', 'plugin/blacklist_number/add', [ 'phone_number' => $number ]); // Verify the entry was created $output = $this->request('GET', 'plugin/blacklist_number'); $this->assertStringContainsString($number, $output, 'Blacklist entry should be visible in the list'); // Test deleting the entry (assuming there's a delete endpoint) // You may need to extract the ID of the entry from the response // $this->request('POST', 'plugin/blacklist_number/delete/ID_HERE'); // Verify the entry was deleted // $output = $this->request('GET', 'plugin/blacklist_number'); // $this->assertStringNotContainsString($number, $output, 'Blacklist entry should be removed from the list'); }tests/testutils/GammuSmsdConfigFile.php (1)
62-79: Ensure device directory cleanup logic is robust.
The destructor attempts to remove the.remove_dir_when_finishedfile and conditionally removes the folder if empty. Consider handling race conditions or partial cleanup failures with additional checks or logging.tests/controllers/Phonebook_test.php (6)
37-60: test_index is incomplete.
You have a session-based test structure, but the test ends withmarkTestIncomplete. Consider adding relevant assertions (e.g., verifying returned data, checking for expected HTML elements) to finalize the test.
65-85: test_index_search_name lacks specific validations.
You’re sending a POST request withsearch_name, but no further checks exist to confirm expected results (like searching for a contact name). Consider asserting the presence of the searched contact in the response.
90-110: test_index_public is structurally consistent.
Same pattern astest_index, but for a "public" endpoint. It’s good to keep them separate. Be sure to add final assertions verifying that "public" data is displayed as expected.
115-135: Encourage more coverage for test_group.
The test ensures HTML validity but needs domain-specific assertions to confirm correct group data.
165-185: test_group_contacts lacks contact-related validations.
After retrieving the group contacts, consider checking for contact count, contact details, or any group association.
190-210: test_add_contact is partially complete.
The test ensures the page loads, but no checks on submitted form data or database changes. Would you like help adding a post request with form data to actually create a contact in the test environment?media/css/base.css (3)
468-485: Replacing #paging with .paging is a logical class-based approach.
Check all references to ensure no ID-based references remain in templates or JavaScript. Minor note:.paging asets an underline style; confirm it aligns with the desired UI approach.
707-717: Ensure table reset rules for .sms_search_form are correct.
.sms_search_formsetsborder-collapseand removes borders. If other forms rely on table styling, consider scope to avoid unintended collisions.
1202-1228: Button group improvements.
Applying consistent padding, background gradient, and text styles is beneficial. However, ensure the.bttn-group divapproach does not conflict with existing structure if there were nested elements or inline styles.tests/plugins/Soap_test.php (7)
28-31: Commented-out method reloadCIwithEngine.
If it’s not used, removing it might reduce confusion. Alternatively, re-enable if needed for environment reloading.
38-67: test_index tries to install the SOAP plugin with session-based context.
Marking incomplete is understandable if you plan additional checks (e.g., verifying plugin routes or SOAP interface functionality).
69-104: test_api covers a base SOAP endpoint check.
You confirm the<title>NuSOAP: KalkunRemoteAccess</title>but rely onmarkTestIncomplete. Consider finalizing by verifying more specific HTML or SOAP elements.
173-210: test_api_index_login injects a custom SOAP request.
It’s great that you’re verifying token matching. Final checks could confirm that the plugin sets up the correct session state in the DB or the local environment.
240-281: test_api_index_version checks the returned version.
The approach is consistent with earlier tests. Finalizing would involve verifying the plugin returns the correct version from an actual config or environment variable if needed.
283-334: test_api_index_sendMessage checks sending success.
Verifying the returned integer is good. However, consider confirming a database record or log entry for deeper coverage.
389-446: test_api_index_logout triggers session destruction exception.
Checks the error path, which is helpful. If tests later run in an environment with an active session, ensure verifying the successful logout path as well.tests/controllers/Login_test.php (3)
48-57: Check session state after redirect.
After asserting the redirect, consider verifying that$_SESSION['loggedin']is indeed set, confirming a successful login flow on the server side.
62-72: Externalize user-facing error messages.
Hard-coded text like "Username or password are incorrect." can complicate localization efforts. A language file or constant is recommended.
131-160: Refine SMS grammar.
The phrase "If you are a registered user, a SMS has been sent to you." might be grammatically improved with "an SMS."tests/controllers/Daemon_test.php (1)
123-179: Expand on plugin 'break' coverage.
Currently, the test expects a'break'return. Consider verifying or logging additional plugin state for a more robust check.tests/controllers/Users_test.php (4)
16-24: Consider adding a tearDown method for resource cleanup.You currently have a
setUp()method to remove theinstallfile before tests. However, there is notearDown()method to restore or clean up any remaining artifacts or database state after tests. Including one would help ensure that each test starts with the same baseline environment and that any leftover test data is removed.
26-29: Validate or extend the database providers if more engines are introduced.The
database_Provider()method returns a list of database engines to test. If new engines or environment configurations are ever added to the system, ensure this method is kept up to date to maintain comprehensive coverage.
367-406: Consider verifying secure password handling.Your test updates a user’s password in
test_add_user_process_edit_userwithout verifying any hashing or encryption. Adding an assertion that checks the stored password is hashed (not plain text) can further strengthen security.
575-579: Expand coverage for user deletion with existing messages.A TODO comment hints at needing more checks/processes for scenarios where the user has messages in inbox/outbox/sentitems. Expanding this test will help ensure safe data handling.
Do you want assistance in creating these expanded tests or opening a new issue to track them?
tests/controllers/Messages_folder_test.php (4)
16-26: Evaluate potential tearDown or fixture cleanup.The
setUp()method callsDBSetup::setup_db_kalkun_testing2($this), but there is no explicit teardown reflecting the same convention. Consider adding cleanup steps to ensure a pristine final state.
29-32: Unify CI reloading logic with setUp if feasible.
reloadCIwithEngineis helpful for database switching. If the test suite grows, centralizing this logic in or alongsidesetUp()could reduce repetition and simplify maintenance.
140-145: Extract repeated config-writing steps.This block writes
$config['conversation_grouping'] = TRUE;to the test config. The same pattern appears multiple times for toggling conversation grouping on or off. A small helper method or fixture approach for toggling conversation grouping can help keep tests DRY.
196-222: Consider adding a test for manual message marking as read.
test_folder_ajaxGET_inbox_byConversation_single_read_contactNotInPbkchecks a read message’s display. While this covers display logic, you might also test explicit read toggling (if supported) to confirm the system updates message flags as intended.tests/controllers/Messages_my_folder_test.php (4)
47-58: Consider refactoring repeated session initialization code.These lines (and similar blocks in subsequent tests) manually set session variables for each test, leading to repetition. A helper or shared setup method could reduce duplication and improve maintainability.
172-172: Include alt attributes for GIF images to enhance HTML5 accessibility.The tests check for arrow images in the message header (
arrow_left.gif,arrow_right.gif), but there's no assertion ofalttext. HTML5 encourages providing alt attributes to improve accessibility and validity.Also applies to: 223-223, 769-769, 819-819, 889-889, 939-939
273-290: Rename the “unreaded” class for clarity.In lines such as
div.messagelist.unreaded, consider using a more conventional naming likeunread. While it doesn't break functionality, it’s more readable and grammatically consistent.
118-118: Enhance clarity of “no message” text.Currently, the test checks for
<p class="no_content"> <i>There is no message in this folder.</i> </p>. Though valid HTML5, consider using<em>or a more descriptive element to improve semantics.Also applies to: 724-724
tests/controllers/Messages_test.php (2)
81-92: Eliminate repetition in session setup code.Multiple tests do the same session-related pre-constructor steps. Factor them into a shared method or trait for clarity and maintainability.
Also applies to: 111-122, 185-196
534-535: Implement the placeholder tests.These tests are marked incomplete. Provide coverage or remove them if they’re unnecessary.
Would you like help drafting these test implementations or opening a new issue to track completion?
Also applies to: 543-544, 552-553
tests/controllers/Kalkun_test.php (3)
20-26: Consider restoring the deleted installation file in tearDown().
Currently, thesetUp()method removesFCPATH . 'install'if it exists. If other tests or deployments rely on that file, you might want to restore or regenerate it in atearDown()method to avoid side effects.
991-1025: Consider internationalization for the “Username already taken” message.
While the logic of rejecting duplicate usernames is correct, displaying a locally hardcoded English string may limit usability in multilingual environments.
1300-1327: Enhance assertion on unread message counts.
Currently, the test only verifies JSON structure but does not confirm actual unread counts. If practical, add a fixture or insertion to confirm correct increments/decrements in unread stats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (99)
.github/workflows/codeigniter.yml(1 hunks)application/config/autoload.php(1 hunks)application/controllers/Messages.php(5 hunks)application/controllers/Phonebook.php(1 hunks)application/controllers/Users.php(1 hunks)application/helpers/kalkun_helper.php(1 hunks)application/plugins/blacklist_number/controllers/Blacklist_number.php(1 hunks)application/plugins/blacklist_number/views/index.php(5 hunks)application/plugins/server_alert/controllers/Server_alert.php(1 hunks)application/plugins/server_alert/views/index.php(6 hunks)application/plugins/sms_credit/controllers/Sms_credit.php(2 hunks)application/plugins/sms_credit/views/index.php(6 hunks)application/plugins/sms_credit/views/navigation.php(1 hunks)application/plugins/sms_credit/views/packages.php(2 hunks)application/plugins/sms_member/views/index.php(1 hunks)application/plugins/sms_to_email/views/index.php(3 hunks)application/plugins/sms_to_wordpress/views/index.php(1 hunks)application/plugins/sms_to_xmpp/views/index.php(1 hunks)application/plugins/soap/controllers/Soap.php(1 hunks)application/plugins/soap/views/index.php(7 hunks)application/plugins/stop_manager/controllers/Stop_manager.php(1 hunks)application/plugins/stop_manager/views/index.php(4 hunks)application/plugins/whitelist_number/controllers/Whitelist_number.php(1 hunks)application/plugins/whitelist_number/views/index.php(5 hunks)application/views/js_init/js_keyboard.php(1 hunks)application/views/js_init/js_layout.php(1 hunks)application/views/js_init/message/js_conversation.php(1 hunks)application/views/js_init/message/js_function.php(1 hunks)application/views/js_init/phonebook/js_group.php(1 hunks)application/views/js_init/phonebook/js_phonebook.php(1 hunks)application/views/main/base.php(10 hunks)application/views/main/dashboard/home.php(2 hunks)application/views/main/dashboard/statistic.php(1 hunks)application/views/main/footer.php(1 hunks)application/views/main/forgot_password.php(4 hunks)application/views/main/install/config_setup.php(21 hunks)application/views/main/install/database_setup.php(7 hunks)application/views/main/install/layout.php(2 hunks)application/views/main/install/requirement_check.php(3 hunks)application/views/main/install/welcome.php(0 hunks)application/views/main/layout.php(2 hunks)application/views/main/login.php(4 hunks)application/views/main/messages/compose.php(13 hunks)application/views/main/messages/conversation.php(3 hunks)application/views/main/messages/index.php(2 hunks)application/views/main/messages/message_list.php(1 hunks)application/views/main/messages/navigation.php(2 hunks)application/views/main/password_reset.php(5 hunks)application/views/main/phonebook/contact/add_contact.php(1 hunks)application/views/main/phonebook/contact/index.php(2 hunks)application/views/main/phonebook/contact/navigation.php(1 hunks)application/views/main/phonebook/contact/pbk_list.php(2 hunks)application/views/main/phonebook/group/index.php(1 hunks)application/views/main/phonebook/group/navigation.php(1 hunks)application/views/main/phonebook/group/pbk_list.php(2 hunks)application/views/main/plugin/index.php(1 hunks)application/views/main/search.php(2 hunks)application/views/main/settings/appearance.php(2 hunks)application/views/main/settings/filters.php(3 hunks)application/views/main/settings/general.php(6 hunks)application/views/main/settings/password.php(1 hunks)application/views/main/settings/personal.php(1 hunks)application/views/main/settings/setting.php(1 hunks)application/views/main/users/add_user.php(2 hunks)application/views/main/users/index.php(0 hunks)application/views/main/users/navigation.php(1 hunks)composer.json(1 hunks)media/css/base.css(8 hunks)media/css/blue.css(1 hunks)media/css/compose.css(1 hunks)media/css/install.css(4 hunks)patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.4.patch(1 hunks)tests/controllers/Daemon_test.php(1 hunks)tests/controllers/Kalkun_test.php(1 hunks)tests/controllers/Login_test.php(1 hunks)tests/controllers/Messages_folder_test.php(1 hunks)tests/controllers/Messages_my_folder_test.php(1 hunks)tests/controllers/Messages_test.php(1 hunks)tests/controllers/Phonebook_test.php(1 hunks)tests/controllers/Pluginss_test.php(1 hunks)tests/controllers/Users_test.php(1 hunks)tests/models/Gammu_model_get_conversation_test.php(1 hunks)tests/models/Gammu_model_get_messages_test.php(1 hunks)tests/models/Gammu_model_test.php(1 hunks)tests/plugins/Blacklist_number_test.php(1 hunks)tests/plugins/Jsonrpc_test.php(1 hunks)tests/plugins/Server_alert_test.php(1 hunks)tests/plugins/Sms_credit_test.php(1 hunks)tests/plugins/Sms_member_test.php(1 hunks)tests/plugins/Sms_to_email_test.php(1 hunks)tests/plugins/Sms_to_twitter_test.php(1 hunks)tests/plugins/Sms_to_wordpress_test.php(1 hunks)tests/plugins/Sms_to_xmpp_test.php(1 hunks)tests/plugins/Soap_test.php(1 hunks)tests/plugins/Stop_manager_test.php(1 hunks)tests/plugins/Whitelist_number_test.php(1 hunks)tests/testutils/DBSetup.php(1 hunks)tests/testutils/GammuSmsdConfigFile.php(1 hunks)tests/testutils/sql-gammu-1.37.4/mysql.sql(1 hunks)
💤 Files with no reviewable changes (2)
- application/views/main/install/welcome.php
- application/views/main/users/index.php
🚧 Files skipped from review as they are similar to previous changes (72)
- application/plugins/sms_credit/views/navigation.php
- application/views/main/settings/setting.php
- application/views/main/messages/conversation.php
- application/plugins/sms_to_xmpp/views/index.php
- application/views/main/messages/message_list.php
- application/views/main/users/navigation.php
- application/plugins/stop_manager/controllers/Stop_manager.php
- application/views/js_init/phonebook/js_group.php
- application/plugins/sms_member/views/index.php
- application/views/main/settings/filters.php
- application/views/main/phonebook/group/index.php
- application/plugins/sms_credit/controllers/Sms_credit.php
- application/plugins/sms_to_wordpress/views/index.php
- application/plugins/blacklist_number/controllers/Blacklist_number.php
- application/views/js_init/phonebook/js_phonebook.php
- application/controllers/Phonebook.php
- application/views/main/footer.php
- media/css/compose.css
- application/views/js_init/message/js_conversation.php
- application/controllers/Users.php
- application/views/main/install/database_setup.php
- application/views/main/layout.php
- application/views/js_init/js_layout.php
- application/views/main/settings/general.php
- application/views/main/phonebook/contact/pbk_list.php
- composer.json
- application/views/main/settings/appearance.php
- application/views/main/dashboard/home.php
- application/views/main/messages/navigation.php
- application/plugins/server_alert/controllers/Server_alert.php
- application/views/main/phonebook/group/pbk_list.php
- application/plugins/sms_credit/views/packages.php
- application/views/main/plugin/index.php
- application/views/main/install/layout.php
- application/plugins/sms_credit/views/index.php
- application/views/main/dashboard/statistic.php
- application/plugins/whitelist_number/controllers/Whitelist_number.php
- application/controllers/Messages.php
- application/plugins/server_alert/views/index.php
- application/views/main/phonebook/contact/navigation.php
- application/views/main/install/requirement_check.php
- application/views/main/messages/compose.php
- application/plugins/stop_manager/views/index.php
- application/plugins/blacklist_number/views/index.php
- application/views/main/phonebook/contact/index.php
- application/views/js_init/js_keyboard.php
- application/views/main/phonebook/group/navigation.php
- application/views/main/settings/password.php
- application/views/main/login.php
- application/views/main/password_reset.php
- tests/plugins/Sms_to_wordpress_test.php
- tests/plugins/Sms_member_test.php
- tests/plugins/Sms_credit_test.php
- application/views/main/search.php
- media/css/install.css
- application/views/main/settings/personal.php
- application/plugins/soap/controllers/Soap.php
- application/views/main/install/config_setup.php
- application/plugins/whitelist_number/views/index.php
- tests/plugins/Jsonrpc_test.php
- tests/plugins/Server_alert_test.php
- tests/plugins/Sms_to_twitter_test.php
- application/views/main/phonebook/contact/add_contact.php
- application/views/main/users/add_user.php
- application/views/js_init/message/js_function.php
- application/helpers/kalkun_helper.php
- application/plugins/sms_to_email/views/index.php
- application/views/main/forgot_password.php
- application/views/main/messages/index.php
- application/views/main/base.php
- application/plugins/soap/views/index.php
- patches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.4.patch
🧰 Additional context used
🪛 Gitleaks (8.21.2)
tests/testutils/DBSetup.php
1497-1497: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Biome (1.9.4)
media/css/blue.css
[error] 53-53: Expected a declaration, or an at rule but instead found ';'.
Expected a declaration, or an at rule here.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: test (ubuntu-latest, 5.6)
- GitHub Check: test (ubuntu-latest, 7.4)
- GitHub Check: test (ubuntu-latest, 7.3)
- GitHub Check: test (ubuntu-latest, 7.2)
- GitHub Check: test (ubuntu-latest, 7.1)
- GitHub Check: test (ubuntu-latest, 7.0)
- GitHub Check: test (ubuntu-latest, 8.4)
- GitHub Check: test (ubuntu-latest, 8.3)
- GitHub Check: test (ubuntu-latest, 8.2)
- GitHub Check: test (ubuntu-latest, 8.1)
- GitHub Check: test (ubuntu-latest, 8.0)
🔇 Additional comments (107)
tests/testutils/sql-gammu-1.37.4/mysql.sql (5)
1-9: Header and Charset Instructions are Clear
The file begins with proper licensing declarations and includes helpful comments regarding charset compatibility, guiding users on potential issues withutf8mb4.
75-100: Outbox Table Design and Indexing
Theoutboxtable is well-structured with sensible defaults and relevant indexes. One point to review: theSendingTimeOutcolumn is defined astimestamp NULL DEFAULT CURRENT_TIMESTAMP. Verify that assigning a default timestamp to a nullable field is the intended behavior in your context.
112-121: Outbox Multipart Table Structure
Theoutbox_multiparttable is defined correctly with a composite primary key and clear default values. The structure is straightforward and meets its intended purpose.
134-140: PBK Table Definition
Thepbktable is straightforward and correctly sets up the necessary fields and primary key. No issues are present.
153-157: PBK Groups Table Structure
Thepbk_groupstable is defined correctly. The explicit naming of the primary key is consistent with other table definitions; if this is a deliberate style choice, it is acceptable.application/config/autoload.php (1)
136-136: Properly configured model autoloading.The change to use an empty array for model autoloading is correct and follows CodeIgniter best practices. This ensures no models are automatically loaded with each request, which can improve performance.
.github/workflows/codeigniter.yml (7)
79-79: Step name clarified for better understanding.Good improvement to the step name, making it more specific about installing composer dependencies.
83-86: Well-structured system dependencies installation.Adding clear steps to install the required system dependencies (libdbd-sqlite3, gammu-smsd) and creating database users through a script improves the workflow's clarity and maintainability.
87-95: Robust Java availability check with fallback.The Java check implementation is thorough by attempting multiple paths before installing. This ensures that Java is available for the HTML validation tools while avoiding unnecessary installations.
95-104: Clean implementation of v.Nu downloader.Using the release-downloader action to fetch a specific version of the HTML validator is a good approach, ensuring consistent validation results across workflow runs.
105-110: Proper setup of HTML validation tools.Installing tidy and setting up the v.Nu HTTP server enhances the testing capabilities by allowing HTML5 validation, which aligns with the PR objective of migrating to valid HTML5.
111-111: Simplified ci-phpunit-test installation.Replacing multiple commands with a single script call improves maintainability and readability of the workflow.
113-113: PHPUnit configured for HTML5 testing.Setting the HTML environment variable to 5 properly configures PHPUnit to test with HTML5 standards, which supports the PR objective.
tests/models/Gammu_model_get_messages_test.php (16)
1-14: Well-structured file header with proper licensing and documentation.The file header contains appropriate copyright information, authorship details, and licensing, following the project's conventions.
15-30: Good test class setup with proper database initialization.The test class structure follows good practices:
- Extends KalkunTestCase for common test functionality
- Sets up the database environment in setUp()
- Implements reloadCIwithEngine() for testing across different database engines
31-34: Effective use of data providers for cross-database testing.Using a data provider to run tests against multiple database engines is an excellent approach for ensuring compatibility across different database backends.
36-55: Thorough testing of invalid input handling.This test properly verifies that the model throws an appropriate exception when given invalid input, which is crucial for robust error handling.
57-88: Proper user permission validation testing.The test correctly verifies that a user cannot access messages that don't belong to them, which is an important security check.
90-152: Comprehensive test for unprocessed messages retrieval.This test thoroughly verifies the retrieval of unprocessed messages, including handling of multipart messages. The assertions check both the count and the specific attributes of retrieved messages.
154-202: Well-constructed test for inbox message retrieval.This test covers inbox message retrieval with appropriate data setup and verification, checking both message counts and message content details.
204-252: Thorough testing of trash folder functionality.This test properly verifies that messages in the trash folder are retrieved correctly, with appropriate message ID and content verification.
254-301: Proper testing of message ordering.The test verifies that messages are correctly ordered by ReceivingDateTime in ascending order, which is important for user-facing message displays.
303-349: Comprehensive multipart message retrieval testing.This test thoroughly verifies the retrieval of multipart messages, ensuring that each part is correctly linked to its message and that the content is properly assembled.
351-400: Well-structured test for pagination functionality.The test properly verifies that limit and offset parameters work correctly for pagination, checking both the count and specific message attributes.
402-449: Effective testing of custom folder functionality.This test properly validates retrieval of messages from custom folders by setting the id_folder parameter and verifying the message contents.
451-496: Thorough testing of outbox message retrieval.This test covers the retrieval of messages from the outbox folder, with appropriate verification of message attributes.
498-543: Proper testing of sent items retrieval.This test correctly verifies the retrieval of sent messages, checking both count and specific message attributes.
545-587: Comprehensive multipart sent items retrieval testing.This test thoroughly verifies the retrieval of multipart sent messages, ensuring proper handling of the message parts.
589-604: Incomplete tests properly marked for future implementation.Using markTestIncomplete() to indicate tests that need to be implemented later is a good practice that helps track testing coverage.
tests/models/Gammu_model_get_conversation_test.php (11)
1-14: Well-structured file header with proper licensing and documentation.The file header follows the project's conventions with appropriate copyright information, authorship, and licensing details.
15-29: Clean test class setup with efficient model loading.The test class is well-structured:
- Properly extends KalkunTestCase
- Sets up the CI instance and loads the required model in setUp()
- Uses a data provider for testing across database engines
30-88: Thorough invalid type error handling test.This test effectively verifies that the get_conversation method throws the expected exception when provided with an invalid type. The database setup and error expectation are well-structured.
90-148: Proper user permission validation.The test correctly verifies that a user cannot access conversations that don't belong to them, which is an important security check.
150-239: Well-implemented data provider and inbox conversation test.The test uses a custom data provider to test both regular and trash folders. The test thoroughly verifies conversation retrieval with appropriate assertions on the number of conversations and their attributes.
241-325: Comprehensive multipart inbox conversation test.This test provides thorough verification of multipart message handling in conversations, with proper setup and assertions.
327-396: Effective pagination testing for conversations.The test properly verifies that limit and offset parameters work correctly for pagination of conversations, with appropriate assertions.
398-470: Thorough custom folder conversation testing.This test effectively verifies conversation retrieval from custom folders with appropriate database setup and assertions.
472-535: Well-structured outbox conversation test.The test thoroughly verifies outbox conversation retrieval with appropriate database setup and assertions on conversation attributes.
537-619: Comprehensive sent items conversation test.This test provides thorough verification of sent item conversations with proper handling of both regular and trash folders.
621-703: Thorough multipart sent items conversation test.The test effectively verifies multipart sent item conversations with appropriate database setup and assertions.
tests/models/Gammu_model_test.php (3)
28-31: Remove the commented-out method.This method is fully commented out and not currently used. Please remove it or add a clear TODO comment if it is needed in the future.
668-747: Great test coverage for multipart Unicode messages.The tests for splitting, encoding, and verifying multipart Unicode messages are comprehensive and well-structured. This helps ensure robust handling of complex message scenarios.
1121-1257: Implement or remove incomplete tests.Several test methods rely on
markTestIncomplete(). Completing these tests will improve coverage for critical code paths (e.g., search, retrieval, or message management) and help maintain quality.media/css/blue.css (1)
39-58: Switching from ID to class selectors looks good.Replacing IDs with class selectors often improves reusability and consistency of style rules.
🧰 Tools
🪛 Biome (1.9.4)
[error] 53-53: Expected a declaration, or an at rule but instead found ';'.
Expected a declaration, or an at rule here.
(parse)
tests/plugins/Stop_manager_test.php (2)
28-31: Remove commented-out method.This method is fully commented out. If it's not needed, please delete it. Otherwise, add a clear comment describing its intended purpose.
61-67: Consider finalizing the test.The test is marked incomplete. If the feature is stable, implementing the missing assertions will deepen coverage and prevent regressions.
tests/plugins/Sms_to_xmpp_test.php (1)
64-66: Complete the test implementation by adding specific assertions.The test only verifies that the plugin's page returns valid HTML and is then marked as incomplete. This doesn't effectively test the plugin's functionality.
Apply this diff to improve the test:
$output = $this->request('GET', 'plugin/sms_to_xmpp'); $this->assertValidHtml($output); -$this->markTestIncomplete(); +// Add specific assertions here +$this->assertStringContainsString('XMPP Settings', $output, 'Output should contain settings heading'); +$this->assertStringContainsString('form', $output, 'Output should contain a form'); +$this->assertStringContainsString('JID', $output, 'Output should contain JID field'); +$this->assertStringContainsString('Password', $output, 'Output should contain password field');tests/plugins/Whitelist_number_test.php (2)
15-15: Verify thePluginss_testimport with double 's'.The import statement on line 15 references a file with an unusual name containing double 's':
Pluginss_test.php. It's used on line 62, but this naming convention seems non-standard and might be a typo.#!/bin/bash # Check if the file with double 's' exists and check for similar files echo "Checking for controllers/Pluginss_test.php:" if [ -f "controllers/Pluginss_test.php" ]; then echo "File exists with double 's'" else echo "File doesn't exist with double 's'. Searching for similar files:" find controllers -name "Plugin*_test.php" 2>/dev/null || echo "No matching files found" fi
28-31: Remove commented-out code that's not being used.This commented-out method isn't being used in the test. Either implement it if it's needed or remove it to keep the code clean.
- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/tests/plugins/Sms_to_email_test.php (2)
15-15: Verify thePluginss_testimport with double 's'.The import statement on line 15 references a file with an unusual name containing double 's':
Pluginss_test.php. It's used on line 62, but this naming convention seems non-standard and might be a typo.#!/bin/bash # Check if the file with double 's' exists and check for similar files echo "Checking for controllers/Pluginss_test.php:" if [ -f "controllers/Pluginss_test.php" ]; then echo "File exists with double 's'" else echo "File doesn't exist with double 's'. Searching for similar files:" find controllers -name "Plugin*_test.php" 2>/dev/null || echo "No matching files found" fi
28-31: Remove commented-out code that's not being used.This commented-out method isn't being used in the test. Either implement it if it's needed or remove it to keep the code clean.
- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/tests/plugins/Blacklist_number_test.php (3)
15-15: Verify thePluginss_testimport with double 's'.The import statement on line 15 references a file with an unusual name containing double 's':
Pluginss_test.php. It's used on line 62, but this naming convention seems non-standard and might be a typo.#!/bin/bash # Check if the file with double 's' exists and check for similar files echo "Checking for controllers/Pluginss_test.php:" if [ -f "controllers/Pluginss_test.php" ]; then echo "File exists with double 's'" else echo "File doesn't exist with double 's'. Searching for similar files:" find controllers -name "Plugin*_test.php" 2>/dev/null || echo "No matching files found" fi
28-31: Remove commented-out code that's not being used.This commented-out method isn't being used in the test. Either implement it if it's needed or remove it to keep the code clean.
- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/
64-66: Add meaningful assertions to complete the test.The test currently only validates that the output is valid HTML but doesn't verify any specific functionality of the blacklist_number plugin.
Apply this diff to improve the test:
$output = $this->request('GET', 'plugin/blacklist_number'); $this->assertValidHtml($output); -$this->markTestIncomplete(); +$this->assertStringContainsString('Blacklist Number', $output); +$this->assertStringContainsString('Add Blacklist Number', $output); +$this->assertStringContainsString('class="nice-table"', $output);tests/testutils/GammuSmsdConfigFile.php (4)
14-17: Use of private property for internal configuration storage looks good.
Keeping the$configproperty private helps avoid external interference.
18-60: Consider error handling for directory creation failures.
The constructor creates a dummy directory if it doesn’t exist, but any errors frommkdirortouchcould cause a silent failure if the function call fails (e.g., insufficient permissions). Adding a try/catch or error check can improve robustness.
81-84: Method is straightforward and minimal.
UsingaddItemto store configuration pairs is clean and ensures the config array stays organized.
86-99: Ensure no special characters break the config output.
When constructing content strings, special characters in$valuecould potentially break formatting if they are not escaped or sanitized.tests/controllers/Phonebook_test.php (3)
18-26: setUp method ensures a clean environment.
Removing the install file and callingsetup_db_kalkun_testing2is good. Be sure that removing the file does not inadvertently cause issues if the file doesn't exist or is locked.
27-31: reloadCIwithEngine might be underutilized.
Reloading the CI configuration per database engine is a valid approach, but consider verifying that the configuration is fully applied (e.g., if additional environment variables or caches require resetting).
140-160: test_group_public parallels test_group.
Add checks to ensure the "public" group usage or data is displayed or handled differently from private groups, if applicable.media/css/base.css (3)
486-528: .simplepaging updates unify pagination styling.
Be sure to confirm the new classes are actually applied in the HTML. If server-side pagination code still references old IDs or classes, it could cause styling mismatches.
532-555: Transitioning dash box styles from IDs to class selectors.
Maintains a consistent pattern. Confirm the layout logic (e.g., JavaScript toggles or the presence of multiple dash boxes on a page) is intact under the new class-based naming.
913-918: Usingwidth: fit-content;in #login_logo.
fit-contentcan have limited support in older browsers. If cross-browser compatibility is critical, consider a fallback or alternative layout approach.tests/plugins/Soap_test.php (5)
17-26: setUp method comment suggests a DB reset, but it's commented out.
Consider clarifying whyDBSetup::setup_db_kalkun_testing2($this);is disabled. If partial environment resets are intended, ensure the partial approach doesn’t cause test leakage across methods.
33-36: database_Provider is straightforward.
Returning$db_engines_to_testensures coverage across multiple database backends. Good approach for portability and test thoroughness.
106-138: test_api_wsdl ensures the WSDL is returned.
You check fortargetNamespacepresence, which is good. Consider adding more robust validations on structure or required definitions in the WSDL.
140-171: test_api_getApiVersion checks an empty body response.
This might be intended, but add clarifying documentation or confirm the plugin returns an empty response for version queries.
336-387: test_api_index_sendFlashMessage is similar to test_api_index_sendMessage.
If “flash message” logic requires distinct handling, add more thorough checks to differentiate them from standard messages.tests/controllers/Login_test.php (5)
17-23: Consider verifying the 'install' file location.
The logic for removing the 'install' file might fail if 'FCPATH' changes or if the deployment environment differs. Confirm that the path is correct for all test environments.
32-43: Form rendering test coverage is solid.
The test checks the localized login title and ensures valid HTML, which is a good practice.
77-110: Validate session expectations across PHP versions and session drivers.
The test relies on aRuntimeExceptionwith a specific message. Changes in session handling or underlying PHP versions might alter this behavior.
165-181: Solid password reset test.
Verifies valid token flow correctly. The coverage is thorough.
243-254: Comprehensive invalid token scenario.
All negative paths appear covered for token validation.tests/controllers/Daemon_test.php (7)
18-24: SetUp block looks consistent.
No issues found with removing the installation file prior to tests.
86-118: Beware concurrency side effects in spam rating.
Setting$CI->Spam_model->ratingcutoff = 0might interfere if tests run in parallel and rely on its default value.
265-294: Outbox routine concurrency caution.
Network calls in the outbox routine may fail under parallel testing or incorrect timezones. Verify each environment’s constraints.
328-348: Effective user filter coverage.
Tests confirm correct folder assignment when filters match. Implementation seems robust.
409-445: Ownership assignment logic is clear.
The mention-based approach to route messages works well and is suitably tested.
495-541: Phonebook-based routing solidly implemented.
Correctly assigns messages to the matching user from phonebook data.
546-595: Fallback owner scenario well-tested.
Ensures a predefined fallback if no user mention or phone match is found.tests/controllers/Pluginss_test.php (7)
17-25: Resetting static plugin members ensures test independence.
reset_plugins_lib_static_members()is a solid approach to avoid cross-test contamination.
174-201: Temporary renaming of 'plugins' directory can break under parallel tests.
Consider using a per-test subdirectory or ephemeral FS if concurrency is anticipated.
203-253: Same concurrency concern for 'available' plugins.
Renaming directories might cause collisions or leftover files if a test fails abruptly.
356-380: Extensive plugin coverage.
The multi-plugin testing ensures a wide range of plugin functionalities are validated.
387-417: Well-structured plugin installation tests.
Confirms correct installation by checking the plugin presence in the installed list.
449-490: Effective library state reset before uninstall.
Resetting static data ensures the uninstall logic sees the plugin as installed.
498-560: Robust plugin directory removal test.
Properly handles plugin removal, verifying it no longer appears in installed or available lists.tests/controllers/Users_test.php (4)
34-60: Solid test coverage for non-admin access.The
test_index_non_adminmethod thoroughly checks that non-admin users are redirected with an access denied message, which ensures proper authorization logic. This coverage is appropriate and seems correct.
62-91: Accurate verification of admin access.The
test_index_GETmethod correctly asserts that an admin user can retrieve the Users index page and verifies essential HTML markers. This adds clarity and confidence to your access control flow.
236-260: Verify routing consistency between “users/add_user” and “phonebook/add_user_process”.The test checks for
"phonebook/add_user_process"in the returned HTML after calling"users/add_user". If this routing is intentional, no issue. Otherwise, ensure the correct route is being tested or displayed to avoid confusion.
411-454: Comprehensive demo mode restriction test.The demo mode scenario blocking username changes is well covered here. This ensures minimal risk of regressions and clarifies expected behavior in restricted environments.
tests/controllers/Messages_folder_test.php (4)
140-176: Excellent test of unread messages with conversation grouping enabled.The
test_folder_ajaxGET_inbox_byConversation_single_unread_contactInPbkmethod provides thorough assertions on message previews, CSS classes, and onclick events. This is commendably detailed.
260-279: Solid coverage for non-conversational unread messages.
test_folder_ajaxGET_inbox_notByConversation_single_unread_contactInPbkthoroughly checks the message rendering, IDs, and preview text. This breadth of verification ensures clarity in user experience.
351-393: Good usage of loops for verifying multiple message elements.In
test_folder_ajaxGET_inbox_byConversation_sortDesc, the loop systematically checks ID/value attributes for each message. This approach ensures consistent coverage of each item’s correctness.
858-874: Consistent approach for sort checks.
test_folder_ajaxGET_sentitems_byConversation_sortDescmirrors your inbox test patterns. This consistency aids maintainability and ensures symmetrical coverage across folders.tests/controllers/Messages_my_folder_test.php (3)
60-63: Good use of HTTP 400 for invalid folder handling.Verifying the 400 status code for invalid folder requests aligns with RESTful practices. The test logic appears correct and self-explanatory.
395-406: Commendable thoroughness in checking message IDs.Looping from 1 to 20 with a sequential check of
input.select_conversation.nicecheckboxensures the test accurately verifies distinct message entries. This systematic approach increases confidence in coverage.
1-10: Documentation header and license check.The file header, license notice, and references are comprehensive and comply with open-source guidelines. No changes needed.
tests/testutils/DBSetup.php (2)
162-190: Reuse exit code validation for create_db.Past reviewers noted that MySQL and SQLite creation do not capture return statuses from the shell commands. PostgreSQL does perform validation, so consider extending that same mechanism for MySQL and SQLite to ensure consistent error handling and debugging.
193-249: Likewise, unify exit-code checking in drop_db.The code currently validates return codes only under PostgreSQL. MySQL and SQLite calls are left unchecked. Per the past review comment, adopt a uniform approach for all engines to make failures visible and easier to troubleshoot.
tests/controllers/Messages_test.php (1)
16-1514: Overall test suite looks solid.Excellent coverage of various scenarios. The setup pattern, request flows, and assertion patterns are consistent, making the tests easy to follow.
tests/controllers/Kalkun_test.php (5)
412-437: Good test coverage for the application dashboard.
Verifying that the HTML title and overall structure are correct is a solid approach, particularly withassertValidHtml(). This test method is well-defined.
533-541: Data provider structure looks clean and extensible.
Thephone_number_validation_Provider()method succinctly configures test cases by prepending each case with the database engine. This is a neat approach.
546-578: Verify the string value for boolean JSON output.
The test checksassertEquals('true', $data_decoded). JSON booleans are typicallytruewithout quotes, so ensure the endpoint actually returns a string"true"instead of a boolean. Otherwise, the test may incorrectly pass.
661-683: Testing 404 response on invalid settings route looks good.
This is a valid negative test scenario. Ensuring that non-existent routes return a 404 is crucial.
1271-1295: Clear test for filter settings page.
Confirming that the page includes a link to “Create a new filter” is a straightforward, effective verification. The usage of_assertStringContainsString()to check specific text is correct.
tenzap
force-pushed
the
feature-223-tests-and-html5
branch
from
9f8c546 to
daf71b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (35)
media/css/install.css (1)
175-179: Review: New Section Heading Styling withh4.section
The newly introducedh4.sectionrule provides centered text, bottom padding, and a bottom border. Its purpose is clear for section header styling.Note: The use of
!importantontext-align: centermay be stronger than necessary; consider removing it if specificity allows..github/workflows/codeigniter.yml (1)
95-104: HTML validation tools setup with pinned version.Using a specific version ('20.6.30') of the validator ensures reproducible tests. Consider updating the comment to match the actual actions being performed.
- - name: Install tidy, v.Nu (Nu Html Checker) + - name: Download v.Nu (Nu Html Checker)tests/plugins/Stop_manager_test.php (2)
25-25: Remove or uncomment the setup_db_kalkun_testing2 call.This commented-out line should either be removed if not needed or uncommented if required for the test. Leaving commented code without explanation reduces clarity.
- //DBSetup::setup_db_kalkun_testing2($this);
66-66: Complete the test implementation.The test is currently marked as incomplete, which means it's not fully testing the functionality. Consider implementing proper assertions to validate the specific behavior of the Stop_manager plugin.
- $this->markTestIncomplete(); + // Verify specific elements expected in the Stop_manager plugin page + $this->assertStringContainsString('Stop Manager', $output, 'Output should contain plugin title'); + // Add more specific assertions to validate plugin functionalitytests/plugins/Sms_to_wordpress_test.php (2)
25-25: Remove or uncomment the setup_db_kalkun_testing2 call.This commented-out line should either be removed if not needed or uncommented if required for the test. Leaving commented code without explanation reduces clarity.
- //DBSetup::setup_db_kalkun_testing2($this);
66-66: Complete the test implementation for WordPress integration.The test is currently marked as incomplete, which means it's not fully testing the functionality. Consider implementing proper assertions to validate the specific behavior of the Sms_to_wordpress plugin, especially the WordPress integration aspects.
- $this->markTestIncomplete(); + // Verify specific elements expected in the Sms_to_wordpress plugin page + $this->assertStringContainsString('SMS to WordPress', $output, 'Output should contain plugin title'); + $this->assertStringContainsString('WordPress URL', $output, 'Output should contain WordPress URL field'); + // Add more specific assertions to validate plugin functionalitytests/plugins/Whitelist_number_test.php (1)
25-25: Remove or uncomment the setup_db_kalkun_testing2 call.This commented-out line should either be removed if not needed or uncommented if required for the test. Leaving commented code without explanation reduces clarity.
- //DBSetup::setup_db_kalkun_testing2($this);tests/plugins/Soap_test.php (2)
28-31: Remove or refactor commented-out code if not used.
It appears you have a helper methodreloadCIwithEnginecommented out. If it's no longer needed, consider removing it. Otherwise, you may want to keep it active if it can be reused in future tests.
48-59: Consolidate repeated session code to improve maintainability.
Your session initialization code is almost identical across multiple tests. Consider factoring it into a reusable helper or incorporating it into thesetUp()method to streamline this file and reduce duplication.Also applies to: 82-93, 119-130, 153-164, 187-197, 247-258, 297-307, 349-360, 403-413
tests/models/Gammu_model_get_conversation_test.php (1)
655-680: Consider adding edge-case validations.
While multipart messages are handled, you may want to add tests for scenarios such as:
- Zero messages for a given sender.
- Messages spanning unusual date ranges.
- Inconsistent or missing fields.
Such edge cases can strengthen the reliability of these tests.
tests/testutils/GammuSmsdConfigFile.php (3)
25-29: Consider adding error handling for directory creation.
Ifmkdir()fails or the permissions are insufficient, the code won't detect it. Adding exception handling or assertions can improve reliability.+if (!@mkdir(dirname($path) . '/gammu-dummy', 0777) && !is_dir(dirname($path) . '/gammu-dummy')) { + throw new \RuntimeException("Failed to create gammu-dummy directory."); +} touch(dirname($path) . '/gammu-dummy' . '/.remove_dir_when_finished');
31-55: Handle unknown database engines explicitly.
When$dbsetup->get_engine()returns an unexpected value, the default case is empty. Consider throwing an exception or logging a warning to handle invalid engines more explicitly.default: - break; + throw new \InvalidArgumentException("Unknown database engine: " . $dbsetup->get_engine());
62-79: Ensure cleanup logic covers more scenarios.
The destructor removes the dummy directory only if it’s empty. If other files remain, the directory is kept. This is reasonable but might introduce test pollution if those extra files were left by incomplete test runs. Consider stricter cleanup or verifying that extra files truly need to persist.tests/controllers/Daemon_test.php (3)
280-291: Validate output buffering usage.
The calls toob_end_flush()andob_get_clean()may cause errors if buffering is not actively started, or they may interfere with subsequent output. Review whether these calls are strictly necessary to avoid side effects in tests.
449-490: Consider edge cases in phone number matching.
inbox_routing_user_phonenumberonly checks exact matches. Some phone numbers could be stored in different formats (e.g., missing country code). Consider testing canonicalization or partial matching to ensure robust coverage.
553-595: Test multiple inbox owners scenario.
Currently,inbox_owner_idis set to an array with a single value. If you anticipate multiple owners, ensure coverage exists to handle multiple IDs or default fallback logic.tests/controllers/Users_test.php (3)
54-60: Check final redirect location.
Intest_index_non_admin, you verify a 302 redirect and flashdata “Access denied.”. As an enhancement, consider explicitly asserting the exact redirect target to confirm full correctness.
85-91: Enhance admin page content checks.
Intest_index_GET, you confirm certain HTML strings but could also verify whether expected admin-specific UI elements (e.g., user management controls) are present to guarantee correct access for admin users.
577-579: Expand deletion coverage.
When testingdelete_user, consider also verifying the cleanup of user-related records (inbox, outbox, phonebooks, etc.) to confirm no orphan data remains.tests/controllers/Messages_folder_test.php (3)
39-63: Evaluate coverage for phonebook folder scenario.
test_folder_ajaxGET_phonebookonly verifies a redirect tophonebook/. Consider adding negative tests (e.g., invalid folder IDs) or verifying deeper aspects of phonebook-based message retrieval if applicable.
429-454: Check offset boundaries.
test_folder_ajaxGET_inbox_byConversation_sortDesc_withOffsetsets$offset = 20. Consider validating out-of-range offsets or offsets beyond the total message count to confirm correct pagination boundaries.
1603-1669: Test outbox folder actions thoroughly.
test_folder_GET_outbox_notByConversation_withOffsetprimarily checks pagination. Consider adding validations for typical outbox actions (e.g., re-sending messages or clearing canceled transmissions) to improve coverage.tests/controllers/Messages_my_folder_test.php (6)
20-27: Consider extracting the installation file checks and DB setup into a shared helper method.Currently, lines 22-27 repeat the pattern of removing the install file and setting up the database. Factoring these steps into a shared helper method in the base test class or a dedicated utility can reduce duplication and simplify maintenance.
98-121: Differentiate test coverage for special-case folders.Lines 98-121 test trash behavior (“There is no message in Trash.”). You might consider additional assertions to confirm that the correct trash folder ID is being used versus a regular inbox folder. This clarifies boundary coverage for special folders.
347-352: Remove or clarify the commented-out test method.Lines 347-352 are commented out with a note about conversation sorting. If no longer applicable, remove or replace with a TODO/FIXME to indicate future intent. Leaving commented test code can lead to confusion.
386-389: Check for consistent naming of the 'read' status.Throughout the file (e.g., line 389) we update rows with
'readed' => 'false'. If the underlying database column truly requires “readed,” that’s fine. Otherwise, standardizing to “read” or “is_read” might improve clarity.
395-408: Enhance large-for-loop validations with failure context.Lines 398-405 loop over 20 items, asserting correctness. If any iteration fails, it can be tricky to identify which iteration caused the error. Consider more descriptive assertion messages or additional debug output to help pinpoint the failing index quickly.
347-929: Use HTML5-friendly assets if possible.Several tests reference GIF images (e.g., "arrow_left.gif", "arrow_right.gif"). While this might not be urgent, consider exploring SVG or other modern formats for better compatibility with HTML5 and potential resolution/scaling benefits.
tests/testutils/DBSetup.php (2)
16-21: Consider splitting the large test utility class.
DBSetupcentralizes multiple database setup operations in a single file, making it quite large. To improve maintainability and clarity, consider splitting it into smaller, focused utilities or classes (e.g., a DB creation class, a data insertion class, a migration class) so each aspect is easier to maintain and reuse.
448-449: Remove or guard debug output.Line 448 echoes out “Filling DB..." which can clutter logs. Consider removing it or gating behind a verbosity flag to keep test output clean.
tests/controllers/Messages_test.php (4)
41-52: Refactor repeated session initialization.Session setup blocks appear frequently (e.g., lines 41-52). To reduce duplication, consider consolidating this logic into a shared setup method or a small helper utility so each test can reuse it.
534-535: Implement or remove the incomplete test.The
test_move_message()method is marked incomplete. If you don’t plan to implement it soon, consider removing or commenting it out to keep the suite consistent.Do you want me to propose an implementation or open a new issue to track it?
543-544: Implement or remove the incomplete test.
test_delete_messages()is also left incomplete. Finalize it or remove it to prevent confusion and keep the test suite tidy.
552-553: Implement or remove the incomplete test.
test_delete_all()is not implemented yet. Decide whether you need it in the short term and either implement or remove it.tests/models/Gammu_model_test.php (1)
324-330: Reduce session initialization duplication.
Session initialization is repeated across multiple tests (e.g., lines 324–330, 471–476, 674–681, 760–765). You can refactor this code into a helper method or incorporate it directly into setUp() to avoid repetition and improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (92)
.github/workflows/codeigniter.yml(1 hunks)application/controllers/Messages.php(5 hunks)application/controllers/Phonebook.php(1 hunks)application/controllers/Users.php(1 hunks)application/helpers/kalkun_helper.php(1 hunks)application/plugins/blacklist_number/controllers/Blacklist_number.php(1 hunks)application/plugins/blacklist_number/views/index.php(5 hunks)application/plugins/server_alert/controllers/Server_alert.php(1 hunks)application/plugins/server_alert/views/index.php(6 hunks)application/plugins/sms_credit/controllers/Sms_credit.php(2 hunks)application/plugins/sms_credit/views/index.php(6 hunks)application/plugins/sms_credit/views/navigation.php(1 hunks)application/plugins/sms_credit/views/packages.php(2 hunks)application/plugins/sms_member/views/index.php(1 hunks)application/plugins/sms_to_email/views/index.php(3 hunks)application/plugins/sms_to_wordpress/views/index.php(1 hunks)application/plugins/sms_to_xmpp/views/index.php(1 hunks)application/plugins/soap/controllers/Soap.php(1 hunks)application/plugins/soap/views/index.php(7 hunks)application/plugins/stop_manager/controllers/Stop_manager.php(1 hunks)application/plugins/stop_manager/views/index.php(4 hunks)application/plugins/whitelist_number/controllers/Whitelist_number.php(1 hunks)application/plugins/whitelist_number/views/index.php(5 hunks)application/views/js_init/js_keyboard.php(1 hunks)application/views/js_init/js_layout.php(1 hunks)application/views/js_init/message/js_conversation.php(1 hunks)application/views/js_init/message/js_function.php(1 hunks)application/views/js_init/phonebook/js_group.php(1 hunks)application/views/js_init/phonebook/js_phonebook.php(1 hunks)application/views/main/base.php(10 hunks)application/views/main/dashboard/home.php(2 hunks)application/views/main/dashboard/statistic.php(1 hunks)application/views/main/footer.php(1 hunks)application/views/main/forgot_password.php(4 hunks)application/views/main/install/config_setup.php(21 hunks)application/views/main/install/database_setup.php(7 hunks)application/views/main/install/layout.php(2 hunks)application/views/main/install/requirement_check.php(3 hunks)application/views/main/install/welcome.php(0 hunks)application/views/main/layout.php(2 hunks)application/views/main/login.php(4 hunks)application/views/main/messages/compose.php(13 hunks)application/views/main/messages/conversation.php(3 hunks)application/views/main/messages/index.php(2 hunks)application/views/main/messages/message_list.php(1 hunks)application/views/main/messages/navigation.php(2 hunks)application/views/main/password_reset.php(5 hunks)application/views/main/phonebook/contact/add_contact.php(1 hunks)application/views/main/phonebook/contact/index.php(2 hunks)application/views/main/phonebook/contact/navigation.php(1 hunks)application/views/main/phonebook/contact/pbk_list.php(2 hunks)application/views/main/phonebook/group/index.php(1 hunks)application/views/main/phonebook/group/navigation.php(1 hunks)application/views/main/phonebook/group/pbk_list.php(2 hunks)application/views/main/plugin/index.php(1 hunks)application/views/main/search.php(2 hunks)application/views/main/settings/appearance.php(2 hunks)application/views/main/settings/filters.php(3 hunks)application/views/main/settings/general.php(6 hunks)application/views/main/settings/password.php(1 hunks)application/views/main/settings/personal.php(1 hunks)application/views/main/settings/setting.php(1 hunks)application/views/main/users/add_user.php(2 hunks)application/views/main/users/index.php(0 hunks)application/views/main/users/navigation.php(1 hunks)media/css/base.css(8 hunks)media/css/blue.css(1 hunks)media/css/compose.css(1 hunks)media/css/install.css(4 hunks)tests/controllers/Daemon_test.php(1 hunks)tests/controllers/Messages_folder_test.php(1 hunks)tests/controllers/Messages_my_folder_test.php(1 hunks)tests/controllers/Messages_test.php(1 hunks)tests/controllers/Phonebook_test.php(1 hunks)tests/controllers/Users_test.php(1 hunks)tests/models/Gammu_model_get_conversation_test.php(1 hunks)tests/models/Gammu_model_get_messages_test.php(1 hunks)tests/models/Gammu_model_test.php(1 hunks)tests/plugins/Blacklist_number_test.php(1 hunks)tests/plugins/Jsonrpc_test.php(1 hunks)tests/plugins/Server_alert_test.php(1 hunks)tests/plugins/Sms_credit_test.php(1 hunks)tests/plugins/Sms_member_test.php(1 hunks)tests/plugins/Sms_to_email_test.php(1 hunks)tests/plugins/Sms_to_twitter_test.php(1 hunks)tests/plugins/Sms_to_wordpress_test.php(1 hunks)tests/plugins/Sms_to_xmpp_test.php(1 hunks)tests/plugins/Soap_test.php(1 hunks)tests/plugins/Stop_manager_test.php(1 hunks)tests/plugins/Whitelist_number_test.php(1 hunks)tests/testutils/DBSetup.php(1 hunks)tests/testutils/GammuSmsdConfigFile.php(1 hunks)
💤 Files with no reviewable changes (2)
- application/views/main/install/welcome.php
- application/views/main/users/index.php
🚧 Files skipped from review as they are similar to previous changes (71)
- application/views/js_init/js_keyboard.php
- application/views/main/settings/setting.php
- application/plugins/blacklist_number/controllers/Blacklist_number.php
- application/plugins/sms_to_wordpress/views/index.php
- application/plugins/sms_to_xmpp/views/index.php
- application/plugins/sms_credit/views/navigation.php
- application/views/main/settings/appearance.php
- application/views/main/settings/general.php
- application/views/main/messages/message_list.php
- application/views/main/users/navigation.php
- application/plugins/sms_member/views/index.php
- application/controllers/Users.php
- application/plugins/whitelist_number/controllers/Whitelist_number.php
- application/views/main/install/layout.php
- application/views/main/messages/conversation.php
- application/views/main/dashboard/statistic.php
- application/views/js_init/phonebook/js_phonebook.php
- application/controllers/Phonebook.php
- application/views/main/settings/personal.php
- application/views/main/settings/filters.php
- application/views/js_init/phonebook/js_group.php
- application/views/main/phonebook/contact/add_contact.php
- application/views/js_init/message/js_function.php
- application/plugins/stop_manager/controllers/Stop_manager.php
- application/views/main/layout.php
- application/views/main/plugin/index.php
- application/helpers/kalkun_helper.php
- application/views/main/forgot_password.php
- application/plugins/soap/controllers/Soap.php
- media/css/blue.css
- application/views/main/settings/password.php
- application/plugins/sms_credit/controllers/Sms_credit.php
- application/plugins/server_alert/controllers/Server_alert.php
- application/plugins/sms_credit/views/index.php
- application/views/main/phonebook/contact/navigation.php
- application/views/main/phonebook/group/index.php
- application/views/main/users/add_user.php
- application/views/main/messages/compose.php
- application/views/main/install/database_setup.php
- application/plugins/sms_to_email/views/index.php
- application/views/main/phonebook/contact/pbk_list.php
- application/views/main/messages/index.php
- application/views/main/dashboard/home.php
- application/views/main/install/requirement_check.php
- application/views/main/password_reset.php
- application/plugins/stop_manager/views/index.php
- application/views/main/login.php
- application/plugins/whitelist_number/views/index.php
- application/views/main/search.php
- tests/plugins/Server_alert_test.php
- application/views/main/phonebook/group/pbk_list.php
- media/css/compose.css
- application/plugins/blacklist_number/views/index.php
- application/plugins/server_alert/views/index.php
- tests/plugins/Sms_to_twitter_test.php
- tests/models/Gammu_model_get_messages_test.php
- tests/plugins/Jsonrpc_test.php
- tests/plugins/Sms_member_test.php
- application/views/main/install/config_setup.php
- application/views/main/messages/navigation.php
- application/views/main/phonebook/contact/index.php
- application/views/main/phonebook/group/navigation.php
- application/plugins/soap/views/index.php
- application/views/js_init/message/js_conversation.php
- tests/plugins/Sms_to_xmpp_test.php
- application/controllers/Messages.php
- application/views/main/base.php
- tests/plugins/Sms_credit_test.php
- application/views/main/footer.php
- application/views/js_init/js_layout.php
- application/plugins/sms_credit/views/packages.php
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: test (ubuntu-latest, 5.6)
- GitHub Check: test (ubuntu-latest, 7.4)
- GitHub Check: test (ubuntu-latest, 7.3)
- GitHub Check: test (ubuntu-latest, 7.2)
- GitHub Check: test (ubuntu-latest, 7.1)
- GitHub Check: test (ubuntu-latest, 7.0)
- GitHub Check: test (ubuntu-latest, 8.4)
- GitHub Check: test (ubuntu-latest, 8.3)
- GitHub Check: test (ubuntu-latest, 8.2)
- GitHub Check: test (ubuntu-latest, 8.1)
- GitHub Check: test (ubuntu-latest, 8.0)
🔇 Additional comments (62)
media/css/install.css (3)
4-5: Review: Body Margin and Width Update
The body’s margin has been updated to0px auto 50px autoand a newwidth: fit-content;property was added. This centers the content horizontally and helps the page adapt to content width.Suggestion: Verify that
fit-contentis supported in all target browsers or consider adding vendor prefixes (e.g.,-webkit-fit-content) if legacy support is required.
114-115: Review: Enhanced Table Styling for.simpletable
Addingborder-collapse: collapse;andborder-spacing: 0;improves table border rendering and spacing. This refinement ensures table borders are rendered cleanly.
147-152: Review: Form Table Layout Improvements
The updates for the.formtableclass (width: 100%;) and for.formtable td(vertical-align: top;) will ensure the form spans the full container width and that cell content is aligned at the top, promoting consistent layout in forms.media/css/base.css (12)
468-471: Review: Paging and Alignment Utilities Update
The addition of the.pagingand.align_rightselectors standardizes right alignment for pagination-related elements. This helps in maintaining consistency across layout components.
473-477: Review: Pagination Link Styling
The updated.paging arule with adjusted padding, font size, and underlining improves the visibility and usability of pagination links.
479-484: Review: Current Page Indicator Styling
The modifications to.paging .current_page—including margin and padding adjustments—provide a clearer visual cue for the current page.
486-523: Review: Simple Paging Component Enhancement
A comprehensive block of new rules for.simplepaging(including styles for its UL, LI, and current page elements) redefines the pagination controls using class selectors. The changes standardize aspects like padding, border, and font size, contributing to a consistent design.Suggestion: Test the paging component across various viewport sizes to ensure visual consistency.
532-554: Review: Dashboard Styling Refinement
The conversion of dashboard selectors from ID-based to class-based (e.g..dash_box_titlebar,.dash_box, and their child elements) improves modularity. The updated margins, padding, and border styles enhance the overall appearance and maintain consistency with the new design pattern.
707-717: Review: SMS Search Form Table Reset
The new styles for.sms_search_form table, th, tdremove default borders and spacing (viaborder-collapse: collapse;andborder-spacing: 0;), ensuring a clean and tight layout for search forms. Additionally, reducing padding in table cells (line 715-717) supports a more compact design.
1202-1218: Review: Button Group Style Update
The rules for.bttn-group divand its related selectors redefine the button group area using div elements instead of button elements. This update improves layout control (e.g., padding and spacing).Suggestion: Ensure that the HTML has been updated accordingly to reflect these class name changes, as the switch from buttons to divs is significant.
1220-1226: Review: Shortcuts Table Styling Improvements
The updated rules for.shortcuts tr(vertical alignment) and.shortcuts td.section(centered text) help standardize the appearance of shortcut tables. These changes improve visual alignment and clarity.
1228-1236: Review: System Information Table Adjustments
New width settings for.sysinfo td:first-childand.sysinfo td:nth-child(2), along with setting vertical alignment for.sysinfo tr, ensure that system info tables render in a well-structured and predictable layout.
1238-1247: Review: Detail Area Table Standardization
The updated.detail_area > tablerules remove default spacing and padding, establishing a clean table layout within detail areas. Assigning fixed widths to the first two cells ensures consistent formatting.
1253-1265: Review: Settings Table Styling Enhancement
The new styles fortable.settingspromote full-width usage and consistent padding, with explicit widths for the first column and vertical alignment for rows. This results in a more uniform display for settings-related tables.
1267-1279: Review: SMS-to-Email Table Styling Update
Adjustments totable.sms_to_email(including full-width, padding, vertical alignment, and fixed width for the first column) ensure that tables related to SMS-to-email functionality are clearly structured and easy to read..github/workflows/codeigniter.yml (7)
79-82: Clear step naming improves workflow readability.Renaming the step to specifically indicate it's for composer dependencies improves clarity in the workflow.
83-84: Good addition of system dependencies.Adding explicit installation of required system dependencies like
libdbd-sqlite3andgammu-smsdhelps ensure consistent test environments.
85-86: Well-designed database setup step.Moving the database setup to a dedicated script (
./utils/db_create_user.sh) improves maintainability by extracting complex logic from the workflow file.
87-94: Robust Java verification with fallback installation.The step properly checks for Java availability in multiple locations before attempting installation, which is a good defensive practice.
105-109: Good separation of validator installation.Installing tidy and setting up the vnu HTTP server separately makes the workflow more maintainable.
110-111: Improved script reusability.Refactoring the ci-phpunit-test installation into a dedicated script improves workflow maintainability and readability.
113-113: HTML5 output specification for tests.Setting the HTML environment variable to 5 aligns with the PR objective of migrating to valid HTML5.
tests/plugins/Stop_manager_test.php (1)
28-31: Remove commented-out method.The
reloadCIwithEngine()method is entirely commented out and not being used in the current implementation. If it's not needed, it should be removed to keep the codebase clean. If it will be needed in the future, consider adding a TODO comment explaining its intended purpose.- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/tests/plugins/Sms_to_wordpress_test.php (1)
28-31: Remove commented-out method.The
reloadCIwithEngine()method is entirely commented out and not being used in the current implementation. If it's not needed, it should be removed to keep the codebase clean. If it will be needed in the future, consider adding a TODO comment explaining its intended purpose.- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/tests/plugins/Whitelist_number_test.php (3)
15-15: Correct the filename in the import statement.The import on line 15 (
require_once __DIR__.'/../controllers/Pluginss_test.php';) attempts to include a file with an unusual name containing double 's' in "Pluginss". Verify if this is the correct filename or a typo.#!/bin/bash # Verify the Pluginss_test.php filename echo "Checking if file exists with double 's'..." find . -path "*controllers/Pluginss_test.php" -type f echo "Checking for similar filenames..." find . -path "*controllers/Plugin*_test.php" -type f
28-31: Remove commented-out method.The
reloadCIwithEngine()method is entirely commented out and not being used in the current implementation. If it's not needed, it should be removed to keep the codebase clean. If it will be needed in the future, consider adding a TODO comment explaining its intended purpose.- /*public function reloadCIwithEngine($db_engine) - { - DBSetup::$current_setup[$db_engine]->write_config_file_for_database(); - }*/
64-66: Complete the test implementation with whitelist-specific assertions.The test only verifies that the plugin's page returns valid HTML and is then marked as incomplete. This doesn't effectively test the plugin's functionality.
$output = $this->request('GET', 'plugin/whitelist_number'); $this->assertValidHtml($output); -$this->markTestIncomplete(); +// Add specific assertions for whitelist plugin +$this->assertStringContainsString('Whitelist Number', $output, 'Output should contain plugin title'); +$this->assertStringContainsString('Add Whitelist Number', $output, 'Output should contain add functionality'); +$this->assertStringContainsString('class="nice-table"', $output, 'Output should contain the table with whitelist entries');tests/plugins/Blacklist_number_test.php (2)
19-26: ThesetUp()logic looks good.
This approach to removing the installer file ensures that the test environment stays clean by preventing inadvertent re-installations. Good job checking for the file’s existence before unlinking.
64-66: Add meaningful assertions to verify the plugin’s behavior.
This comment mirrors your previous review feedback, where more specific checks on the rendered output and plugin functionality were suggested before concluding the test. Consider verifying key UI text or DOM structure to ensure plugin correctness as recommended previously.tests/plugins/Soap_test.php (2)
64-67: Complete the test logic.
Calls to$this->markTestIncomplete()suggest further validation is planned but not yet implemented. Adding more specific assertions (e.g., verifying expected HTML markup or SOAP response details) will make these tests more robust.Also applies to: 103-103
434-435: Validate output buffer handling.
Flushing output buffers withob_end_flush()could have side effects, especially if multiple buffers are stacked. Consider confirming that this is strictly needed here and that it won’t interfere with subsequent assertions.tests/models/Gammu_model_get_conversation_test.php (2)
139-139: Review theinsert('user')call without arguments.
Inserting into theusertable with no parameters might be intentional if default values are handled elsewhere, but it could also be a placeholder that needs concrete data. Double-check to ensure this insert accomplishes its intended purpose.
379-395: Solid test for inbox pagination.
Your logic for checking partial retrieval with limit and offset calculations appears correct. This helps ensure theget_conversation()method respects pagination parameters. Nice work.tests/plugins/Sms_to_email_test.php (2)
19-26: Consider verifying the removal of the 'install' file.
ThesetUpmethod removesFCPATH . 'install'if present, but no assertion or verification is made to confirm this step's success.Do you want to confirm that no leftover installation files exist before running the test suite? If so, I can generate a shell script that scans for any remaining installation files.
64-66: Add plugin-specific test assertions.
This test is marked incomplete, and it only checks for valid HTML. You likely need to verify that the plugin handles email-related workflows—e.g., form fields, message content, or SMTP settings.tests/controllers/Phonebook_test.php (7)
57-60: Complete the test with phonebook-specific assertions.
Currently, the test only verifies valid HTML and stops. Integrate assertions verifying the rendered phonebook list.-$this->markTestIncomplete(); +$this->assertStringContainsString('Your Contacts', $output, 'Expected phonebook title not found.');
82-85: Complete the test with phonebook-specific assertions.
The test checks valid HTML but doesn’t confirm whether the correct search results appear for "Contact for number 001."-$this->markTestIncomplete(); +$this->assertStringContainsString('Contact for number 001', $output, 'Search results do not match.');
107-110: Complete the test with phonebook-specific assertions.
Similar to the previous tests, you only check the HTML and mark it incomplete. Verify that public contacts are displayed as expected.-$this->markTestIncomplete(); +$this->assertStringContainsString('Public Contacts', $output, 'Public contacts heading is missing.');
132-135: Complete the test with group-related assertions.
This test doesn’t confirm that group data is displayed correctly. Add checks for the presence of group titles or contacts.-$this->markTestIncomplete(); +$this->assertStringContainsString('Group List', $output, 'Group list heading is missing.');
157-160: Complete the test with group-related assertions for public groups.
The test verifies HTML but not the expected group data. Add checks for public group membership or metadata.-$this->markTestIncomplete(); +$this->assertStringContainsString('Public Group', $output, 'Missing public group heading.');
182-185: Complete the test with phonebook-specific assertions for group contacts.
Likewise, this test only checks valid HTML. Ensure group contacts are correctly returned (e.g., presence of a contact name within that group).-$this->markTestIncomplete(); +$this->assertStringContainsString('Contact for group ID 1', $output, 'Group contact not found.');
208-210: Add assertions for new contact creation.
Use additional checks to verify that the "Add Contact" form is displayed properly and that the necessary fields (e.g., name, phone number) are present.-$this->markTestIncomplete(); +$this->assertStringContainsString('Add New Contact', $output, 'Expected add contact heading not found.');tests/controllers/Daemon_test.php (3)
20-24: Verify safe removal of install file.
Consider adding a check to ensureFCPATH . 'install'is not a directory or handling potential concurrency issues where another process might modify or recreate this file simultaneously.
330-346: Confirm filter data initialization.
When setting up filters with$dbsetup->insert('filter'), consider verifying that the filter conditions (e.g., matching text, phone number) are accurately tested later to capture both positive and negative filter scenarios.
495-541: Verify phonebook matching logic.
When checkingpbk.Number, confirm that various corner cases (like duplicates, different formats, or leading zeros) won’t impact ownership assignment.tests/controllers/Users_test.php (2)
20-24: Validate concurrency handling when removing install file.
As with similar logic in other tests, verify thatFCPATH . 'install'is guaranteed to be a file and that another process won’t concurrently recreate or remove it, causing potential race conditions.
349-362: Ensure secure password handling.
When adding a new user, you passpassword_for_new_userin plain text. Confirm that the underlying process hashes or securely stores the password before committing it to the database. Tests might also validate that the stored password differs from the plain text value.tests/controllers/Messages_folder_test.php (3)
26-27: Confirm DB setup resilience.
DBSetup::setup_db_kalkun_testing2($this)might fail under concurrency or partial state. Ensure rollback logic or robust checks exist to avoid partially initialized states for subsequent tests.
245-279: Examine concurrency handling & partial writes.
Here, you update user settings (conversation_sort) and theinboxtable for test conditions. Ensure that a test failure won’t leave data in an inconsistent state for subsequent runs.
1079-1098: Examine sorting logic corner cases.
test_folder_ajaxGET_sentitems_notByConversation_sortAscverifies ascending sorting. You may also want to test a scenario where no messages are present or where message IDs are non-sequential to confirm stable sorting.tests/controllers/Messages_my_folder_test.php (4)
47-58: Validate session control in CLI environments.The session checks within the callable (e.g., checking
is_cli() === FALSEbeforesession_start()) are repeated in many tests. If you run tests exclusively through CLI, ensure session handling logic is correct or abstracted out.
127-180: Check for more robust string matching or locale variations.In lines 157 and beyond, the code modifies
TextDecodedwith'%User '.$id_user.'%', then searches for'User 9'in the resulting text. If the codebase ever changes how names or placeholders are stored, these tests may break. Consider matching more flexibly or introducing constants for placeholders.
1000-1008: Confirm ID offset values don't break under scale.The ID offset logic (lines 1005-1007, for example) uses computed offsets. Verify that these offsets remain correct if the number of senders/messages grows beyond current assumptions (DBSetup constants).
1278-1363: Great thoroughness in multi-page navigation tests.The multi-page offset tests (lines 1278-1363) show robust coverage of paging logic, ensuring correct item counts and navigating to the last page. Nicely done!
tests/testutils/DBSetup.php (4)
162-190: Check the command return status for MySQL/SQLite creation.
197-249: Check the command return status for MySQL/SQLite drop.
958-960: Fix the sequence pointer for sentitems.
1497-1497: Avoid storing tokens in plaintext.tests/models/Gammu_model_test.php (4)
18-23: Test setup implementation is well-structured.
This setup method cleanly initializes the test environment by resetting the instance, loading the required model, and storing the model reference for subsequent tests.
1119-1257: Implement incomplete tests for better coverage.
These methods (test_search_messages,test_get_messages,test_get_modem_list, etc.) are marked withmarkTestIncomplete(). Completing them will increase coverage and help ensure the underlying functionality is fully tested.
1143-1152: Robust test coverage for_default()method.
Merging array keys from the second array into the first and verifying the final structure exemplifies a solid unit test. This test accurately asserts that overwritten values match the expected output.
320-322:Details
❓ Verification inconclusive
Verify config file accessibility before skipping tests.
If the config file is not found or readable, these tests are skipped. Consider verifying the file is actually created or present in the codebase to ensure the test environment is set up properly.
🏁 Script executed:
#!/bin/bash # Searching for 'gammu-config' to verify its presence in the codebase. fd gammu-configLength of output: 17
Verify Config File Accessibility in Tests
- The
fd gammu-configsearch returned no results, which might indicate that the config file is either not present in the repository or is generated externally.- Ensure that the file referenced by
$this->CI->config->item('gammu_config')exists and is readable during test setup.- If the file is expected to be created dynamically or externally, please update the documentation or test setup instructions to clarify this behavior.
| | | ||
| | $autoload['model'] = array('first_model' => 'first'); | ||
| */ | ||
| $autoload['model'] = array('Phonebook_model'); |
There was a problem hiding this comment.
I faintly recall that this used to be required otherwise everywhere since this was required it used to load later which used to be explicitly mentioned. Does removing this no more cause that?
There was a problem hiding this comment.
When I removed it, I think I have added a statement to load the model at the places where it was actually needed.
There was a problem hiding this comment.
I did it either in this PR, or in the previous ones that contained all the fixes required for the tests to pass .
|
Html5 wasn't a big deal, actually 😅 |
+ option to disable monkey patching the faulty lines in case of errors might be wrong because of it + check ci3 patch is applied
Otherwise we could have errors like undefined property Install::
It is then easier to test
…rs.php) DBVars.php has the credentials
We now start DBMS and create users with ./utils/db_create_user.sh Filling the DB is now managed in the test suite
….php, ConfigFile.php, TestSuiteFile.php) Install_test: add tests for each function of the controller (contains DBSetup.php) + use credentials from DBVars.php
It causes issues with ci-phpunit-test. Error reported is: ErrorException: Undefined property: Kalkun:: (inside controller/Kalkun.php::index()
Which is html5 compliant
The element “a” must not appear as a descendant of the “button” element
It is not use anywhere anyway
It is already added by view/main/users/users_list.php
tenzap
force-pushed
the
feature-223-tests-and-html5
branch
from
daf71b7 to
e088c46
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (6)
tests/controllers/Messages_my_folder_test.php (1)
143-150: Repetition of “conversation_grouping” toggle logicThe block that writes to
kalkun_settings.php, resets the instance and updates user settings re-appears in >20 methods (e.g. lines 143-150, 254-259, 375-382, 431-437, …).
Extract this into a helper (e.g.withConversationGrouping(bool $flag, callable $cb)) to keep tests concise and maintainable.tests/controllers/Kalkun_test.php (1)
1123-1134: Same boolean-vs-string problem in multiple-number validationReplicate the fix from the previous comment for
phone_number_validation_multiple()to ensure strictness.tests/models/Gammu_model_test.php (1)
1119-1160: NumerousmarkTestIncomplete()blocks remain unimplementedAll the placeholder tests (
test_search_messages,test_get_messages, …) are still empty.
Leaving them incomplete permanently skews the coverage report and may hide future regressions.Please implement or remove them, or add a clear roadmap so the team knows when they’ll be delivered.
tests/testutils/DBSetup.php (3)
1493-1499: Hard-coded API token in test dataA real-looking SHA-256 string is committed in plain text. Even in tests this trains bad habits and may leak if reused elsewhere.
Recommend replacing with an obviously dummy value and reading from env when sensitive.
167-190: 🛠️ Refactor suggestion
create_db()/drop_db()do not check command exit status for MySQL & SQLiteAs previously noted, only the PostgreSQL branch validates return codes.
If a command fails (e.g., wrong credentials), tests proceed with a half-configured database, yielding erratic failures.Adopt
exec()with a captured$retval, mirroring the PostgreSQL logic, or throw on non-zero exit codes.
958-960:⚠️ Potential issueSequence refresh targets the wrong table – will corrupt
sentitems_ID_seqThe sequence for
sentitemsis reset from the max inbox ID, producing duplicate-key errors after inserts.- $CI->db->query('select setval(\'"sentitems_ID_seq"\'::regclass, (select max("ID") from "inbox"))'); + $CI->db->query('select setval(\'"sentitems_ID_seq"\'::regclass, (select max("ID") from "sentitems"))');
🧹 Nitpick comments (28)
tests/libraries/Install_info_test.php (2)
62-82: Inconsistent object initialization between test methods.This test method creates a new instance of
Install_infodirectly, while other test methods use mocking. This inconsistency could be addressed by moving the initialization to the setUp method.Also note that this test uses the instance property that wasn't initialized in setUp.
1-170: Suggestions for test class improvement.While the test coverage is good, consider these improvements:
- Add a
tearDownmethod for cleanup if needed- Add tests for any public methods not yet covered
- Consider using annotations/attributes for newer PHP versions instead of docblock comments
- Standardize the approach to creating the test subject (either use mocking consistently or initialize in setUp)
media/css/base.css (10)
468-484: Consistent class-based paging selectors
The migration from#pagingto.pagingand the associated.current_pagerules correctly replaces ID selectors with classes, improving reusability. Consider consolidating common properties (e.g.,text-decoration: underlineon links) into a shared selector to reduce duplication.
486-527: Refactor simplepaging rules to reduce redundancy
The.simplepagingblock handles layout and active states well, but there are multiple scattered.current_pagerules and repeated list styling. You could merge the two.simplepaging .current_pagedefinitions and group shared list-item resets into a single selector (e.g.,.simplepaging ul, .simplepaging ul li { … }). This will make future maintenance easier.
532-554: Dashboard class selector updates look correct
Switching from#dash_box_titlebar/#dash_boxto.dash_box_titlebar/.dash_boxis consistent. The new padding, border and font-weight settings preserve the original design. Consider addingbox-sizing: border-box;to these containers to ensure consistent sizing when padding or borders change.
916-919: Centering login logo using auto margins
Usingmargin-left: auto; margin-right: autowithwidth: fit-contentsuccessfully centers the logo block. Note thatfit-contentmay have spotty support in older browsers—consider usingdisplay: table; margin: 50px auto 15px;or explicit width settings if you need broader compatibility.
1116-1124: Phonebook name link styling via class
Replacing#pbkname awith.pbkname ais on target. For accessibility, add a:focusstyle alongside:hoverto ensure keyboard users see the same visual feedback.
1202-1218: Button group styling – consider modern layout
The.bttn-group divfloats work, but using flexbox on.bttn-group(e.g.,display: flex; gap: 0.5ex;) would eliminate floats and manualmargin-righthacks. This also simplifies spacing and responsive behavior.
843-845: Apply collapse and spacing reset to.nice-table
Addingborder-collapse: collapseandborder-spacing: 0is ideal for uniform table rendering. Consider grouping all table-reset rules (including.sms_search_formand.detail_area) into a shared utility if you foresee more tables needing identical resets.
1220-1226: Shortcuts table cell alignment
Thevertical-align: topon rows andtext-align: centeron section cells achieves the desired layout. You might merge these into a single rule (.shortcuts tr, .shortcuts td.section) to tidy up the CSS.
1228-1236: Sysinfo column sizing via nth-child
Specifying widths for the first two columns gives precise control. Beware that changes to column order in HTML will break these rules—if the structure may evolve, consider adding utility classes to the markup instead.
1238-1251: Detail area table reset and column widths
The reset rules (border-collapse,border-spacing, zero padding) align with modern CSS standards. As with Sysinfo, injecting explicit classes on columns (instead of relying on:first-child/:nth-child) could make future layout changes safer.tests/models/Gammu_model_get_messages_test.php (1)
592-603: Consider implementing incomplete tests.Two test methods are marked as incomplete. This is valid during development, but consider implementing these in a future update to improve test coverage.
tests/plugins/Soap_test.php (1)
48-59: Factor repetitive session boot-strapping into a helperFive times the exact same anonymous function initializes the
$_SESSIONarray. Extracting this into a private method (or aTestSessionHelpertrait) will:
- keep the test body short,
- guarantee the session is configured consistently,
- ease future changes (e.g. additional attributes).
Also applies to: 82-93, 120-130, 153-164, 186-197
tests/models/Gammu_model_get_conversation_test.php (2)
190-240: Large data generators slow the suite – consider fixtures or data providersEach test inserts 25×5 (and for multipart 25×5×4) rows, repeated across many engines and trash variants. This balloons runtime and memory while most assertions only need a handful of records to prove ordering / filtering.
Suggestions:
- Reduce
$sendersto the minimum that exercises the edge you test (e.g. 3).- Extract a reusable
buildInboxFixtures($senders, $messagesPerSender)helper so the numbers are tunable in one place.- Use SQLite in memory for unit-level tests where engine-specific behaviour is irrelevant.
Also applies to: 274-325, 570-620
102-112: Session & model boot-strap code is duplicated dozens of timesJust like in
Soap_test.php, the same bootstrap sequence (reset instance → load session → populate$_SESSION→ load DB & model) is copied to every test. Encapsulate it insetUp()or a private helper to keep individual tests focused on what they check rather than how.Also applies to: 169-178, 339-348, 555-564
tests/controllers/Daemon_test.php (2)
284-290:ob_end_flush()/ob_get_clean()can raise “failed to delete buffer” noticesThese functions require that an output buffer is actually open.
When PHPUnit is executed with--stderror different buffering settings, the current buffer stack may be empty and the call will emit warnings, breaking the test.Guard the calls:
- ob_end_flush(); - ob_get_clean(); + if (ob_get_level() > 0) { + @ob_end_flush(); + @ob_get_clean(); + }
143-165: Temporary plugin files are never cleaned upThe dynamically-created plugin (
tester_*) is written intoAPPPATH/pluginsbut never removed.
When the same test suite is re-run locally or in a long-lived CI workspace, stale plugins can pollute subsequent runs.Add a
tearDown()(or registerunlink()in__destruct) to delete the plugin directory after each test.tests/controllers/Messages_folder_test.php (1)
240-256: Heavy duplication – consider a small helper to toggle conversation groupingAlmost every test block rewrites
kalkun_settings.phpto enable/disable conversation grouping and then callsresetInstance().
Extracting this into a private helper (e.g.setConversationGrouping(bool $flag)) would:• reduce 80+ identical lines,
• centralise the logic should the config path ever change,
• improve readability.private function setConversationGrouping(bool $enabled): void { $cfg = new ConfigFile(APPPATH.'config/testing/kalkun_settings.php'); $cfg->write('<?php $config[\'conversation_grouping\'] = '.($enabled?'TRUE':'FALSE').';'); $this->resetInstance(); }Then each test only needs
$this->setConversationGrouping(true);.tests/controllers/Messages_test.php (1)
321-326: URL construction can double-encode partsManually building the
$urlinside aforeachwithout distinguishing already-encoded pieces risks double encoding when a value already contains%xx.Consider:
$params = array_map('rawurlencode', $search_data); $this->assertRedirect('messages/search/advanced/' . implode('/', $params));tests/controllers/Messages_my_folder_test.php (2)
260-268: SharedDBSetup::$current_setupmutations may bleed between testsThe test updates the mutable singleton and relies on
resetInstance()to flush CodeIgniter, but does not roll back DB mutations.
When tests are executed in parallel or reordered, state leakage can occur.
Ensure each test either:
- Runs in a transaction that is rolled back, or
- Creates a fresh database/schema per test case.
274-288: Magic numbers obscure intention
$id_offset = 2534;& other offsets (1530,1276, …) appear many times.
Introduce a semantic constant or compute the offset viaDBSetuphelpers so future schema changes do not require search-&-replace.$id_offset = DBSetup::getInboxStartId($id_user); // example helpertests/controllers/Kalkun_test.php (3)
170-176: Brittle redirect assertion relies on internal CI output bufferAccessing
$this->CI->output->_status['redirect']pokes into framework internals which may change.
Prefer the first-class helper fromci-phpunit-test:$this->assertRedirect(site_url(), 302);or, if not available, wrap the internal logic in a helper method in
KalkunTestCaseso only one place needs adjustment when CI internals evolve.
851-860: Outdated language value ‘english’Recent CodeIgniter i18n files use lowercase language keys (
'english'became'en').
If the application upgrades language packs, this test will fail.
Consider retrieving the default language via config instead of hard-coding.'language' => $this->CI->config->item('language') ?? 'english',
462-468:disable_outgoingwarning assertion couples to exact markupHTML copy changes (e.g. adding a CSS class) would break the test.
Search for the warning text only:- $expected = '<div class="warning">Outgoing SMS disabled. Contact system administrator.</div>'; - $this->_assertStringContainsString($expected, $data); + $this->_assertStringContainsString('Outgoing SMS disabled', $data);tests/testutils/DBSetup.php (3)
433-435: Sensitive credentials written toconfig/testing/database.phpPasswords are embedded unencrypted in a file that persists after the suite. Consider writing to a temp location and deleting in
tearDown, or masking via CI secrets.Not critical for local testing, but tightening here avoids accidental leaks when logs/artifacts are published.
132-140:mkdir()without permissions / error handlingFailure to create the temp directory (e.g., permission issues) will go unnoticed and later SQLite operations will fail with cryptic messages.
- if ( ! file_exists($dir)) - { - mkdir ($dir); - } + if (!is_dir($dir) && !mkdir($dir, 0700, true) && !is_dir($dir)) { + throw new \RuntimeException("Unable to create temp dir: $dir"); + }
818-833: Consider extracting duplication betweenclosure()andexecute()Both methods iterate over
$this->recordsperforming identical insert/update logic.
Refactor into a privateapplyRecords()to ensure any later bug-fix touches both paths automatically.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
composer.lockis excluded by!**/*.lock,!**/*.lockpatches/Kenjis_CiPhpunitTest/v3.0.4/support_php-8.4.patchis excluded by!**/*.patch
📒 Files selected for processing (107)
.github/workflows/codeigniter.yml(1 hunks).gitignore(1 hunks)application/config/autoload.php(1 hunks)application/controllers/Install.php(3 hunks)application/controllers/Messages.php(5 hunks)application/controllers/Phonebook.php(1 hunks)application/controllers/Users.php(1 hunks)application/helpers/kalkun_helper.php(1 hunks)application/libraries/Install_info.php(1 hunks)application/plugins/blacklist_number/controllers/Blacklist_number.php(1 hunks)application/plugins/blacklist_number/views/index.php(5 hunks)application/plugins/server_alert/controllers/Server_alert.php(1 hunks)application/plugins/server_alert/views/index.php(6 hunks)application/plugins/sms_credit/controllers/Sms_credit.php(2 hunks)application/plugins/sms_credit/views/index.php(6 hunks)application/plugins/sms_credit/views/navigation.php(1 hunks)application/plugins/sms_credit/views/packages.php(2 hunks)application/plugins/sms_member/views/index.php(1 hunks)application/plugins/sms_to_email/views/index.php(3 hunks)application/plugins/sms_to_wordpress/views/index.php(1 hunks)application/plugins/sms_to_xmpp/views/index.php(1 hunks)application/plugins/soap/controllers/Soap.php(1 hunks)application/plugins/soap/views/index.php(7 hunks)application/plugins/stop_manager/controllers/Stop_manager.php(1 hunks)application/plugins/stop_manager/views/index.php(4 hunks)application/plugins/whitelist_number/controllers/Whitelist_number.php(1 hunks)application/plugins/whitelist_number/views/index.php(5 hunks)application/third_party/MX/Loader.php(1 hunks)application/third_party/MX/Modules.php(1 hunks)application/views/js_init/js_keyboard.php(1 hunks)application/views/js_init/js_layout.php(1 hunks)application/views/js_init/message/js_conversation.php(1 hunks)application/views/js_init/message/js_function.php(1 hunks)application/views/js_init/phonebook/js_group.php(1 hunks)application/views/js_init/phonebook/js_phonebook.php(1 hunks)application/views/main/base.php(10 hunks)application/views/main/dashboard/home.php(2 hunks)application/views/main/dashboard/statistic.php(1 hunks)application/views/main/footer.php(1 hunks)application/views/main/forgot_password.php(4 hunks)application/views/main/install/config_setup.php(21 hunks)application/views/main/install/database_setup.php(7 hunks)application/views/main/install/layout.php(2 hunks)application/views/main/install/requirement_check.php(3 hunks)application/views/main/install/welcome.php(0 hunks)application/views/main/layout.php(2 hunks)application/views/main/login.php(4 hunks)application/views/main/messages/compose.php(13 hunks)application/views/main/messages/conversation.php(3 hunks)application/views/main/messages/index.php(2 hunks)application/views/main/messages/message_list.php(1 hunks)application/views/main/messages/navigation.php(2 hunks)application/views/main/password_reset.php(5 hunks)application/views/main/phonebook/contact/add_contact.php(1 hunks)application/views/main/phonebook/contact/index.php(2 hunks)application/views/main/phonebook/contact/navigation.php(1 hunks)application/views/main/phonebook/contact/pbk_list.php(2 hunks)application/views/main/phonebook/group/index.php(1 hunks)application/views/main/phonebook/group/navigation.php(1 hunks)application/views/main/phonebook/group/pbk_list.php(2 hunks)application/views/main/plugin/index.php(1 hunks)application/views/main/search.php(2 hunks)application/views/main/settings/appearance.php(2 hunks)application/views/main/settings/filters.php(3 hunks)application/views/main/settings/general.php(6 hunks)application/views/main/settings/password.php(1 hunks)application/views/main/settings/personal.php(1 hunks)application/views/main/settings/setting.php(1 hunks)application/views/main/users/add_user.php(2 hunks)application/views/main/users/index.php(0 hunks)application/views/main/users/navigation.php(1 hunks)composer.json(1 hunks)media/css/base.css(8 hunks)media/css/blue.css(1 hunks)media/css/compose.css(1 hunks)media/css/install.css(4 hunks)tests/.gitignore(1 hunks)tests/controllers/Daemon_test.php(1 hunks)tests/controllers/Install_test.php(1 hunks)tests/controllers/Kalkun_test.php(1 hunks)tests/controllers/Login_test.php(1 hunks)tests/controllers/Messages_folder_test.php(1 hunks)tests/controllers/Messages_my_folder_test.php(1 hunks)tests/controllers/Messages_test.php(1 hunks)tests/controllers/Phonebook_test.php(1 hunks)tests/controllers/Pluginss_test.php(1 hunks)tests/controllers/Users_test.php(1 hunks)tests/libraries/Install_info_test.php(1 hunks)tests/models/Gammu_model_get_conversation_test.php(1 hunks)tests/models/Gammu_model_get_messages_test.php(1 hunks)tests/models/Gammu_model_test.php(1 hunks)tests/plugins/Blacklist_number_test.php(1 hunks)tests/plugins/Jsonrpc_test.php(1 hunks)tests/plugins/Server_alert_test.php(1 hunks)tests/plugins/Sms_credit_test.php(1 hunks)tests/plugins/Sms_member_test.php(1 hunks)tests/plugins/Sms_to_email_test.php(1 hunks)tests/plugins/Sms_to_twitter_test.php(1 hunks)tests/plugins/Sms_to_wordpress_test.php(1 hunks)tests/plugins/Sms_to_xmpp_test.php(1 hunks)tests/plugins/Soap_test.php(1 hunks)tests/plugins/Stop_manager_test.php(1 hunks)tests/plugins/Whitelist_number_test.php(1 hunks)tests/testutils/ConfigFile.php(1 hunks)tests/testutils/DBSetup.php(1 hunks)tests/testutils/DBVars.php(1 hunks)tests/testutils/GammuSmsdConfigFile.php(1 hunks)
⛔ Files not processed due to max files limit (10)
- tests/testutils/Html4Validator.php
- tests/testutils/Html5Validator.php
- tests/testutils/HtmlValidator.php
- tests/testutils/KalkunTestCase.php
- tests/testutils/TestSuiteFile.php
- tests/testutils/sql-gammu-1.37.4/mysql.sql
- tests/testutils/sql-gammu-1.37.4/pgsql.sql
- tests/testutils/sql-gammu-1.37.4/sqlite.sql
- tests/testutils/sql-gammu-1.42.0/mysql-legacy.sql
- tests/testutils/sql-gammu-1.42.0/mysql.sql
💤 Files with no reviewable changes (2)
- application/views/main/install/welcome.php
- application/views/main/users/index.php
✅ Files skipped from review due to trivial changes (5)
- application/controllers/Messages.php
- application/views/main/messages/index.php
- application/plugins/sms_credit/controllers/Sms_credit.php
- tests/testutils/DBVars.php
- media/css/install.css
🚧 Files skipped from review as they are similar to previous changes (83)
- application/views/main/settings/setting.php
- application/views/js_init/js_keyboard.php
- .gitignore
- tests/testutils/ConfigFile.php
- application/views/js_init/message/js_conversation.php
- application/views/main/messages/message_list.php
- application/plugins/sms_member/views/index.php
- application/controllers/Phonebook.php
- application/plugins/whitelist_number/controllers/Whitelist_number.php
- application/views/main/phonebook/group/navigation.php
- application/plugins/sms_credit/views/packages.php
- application/views/main/phonebook/group/index.php
- application/views/main/settings/appearance.php
- application/plugins/stop_manager/controllers/Stop_manager.php
- application/plugins/sms_credit/views/navigation.php
- application/views/js_init/message/js_function.php
- application/views/js_init/phonebook/js_group.php
- application/views/main/phonebook/contact/navigation.php
- application/plugins/sms_to_wordpress/views/index.php
- application/views/js_init/js_layout.php
- application/views/main/settings/personal.php
- application/views/main/phonebook/contact/add_contact.php
- application/views/main/search.php
- application/plugins/soap/controllers/Soap.php
- application/views/main/settings/general.php
- application/views/main/phonebook/contact/pbk_list.php
- application/views/main/plugin/index.php
- composer.json
- application/views/main/footer.php
- application/third_party/MX/Loader.php
- application/views/main/messages/conversation.php
- media/css/compose.css
- application/views/main/settings/filters.php
- application/views/main/install/layout.php
- application/third_party/MX/Modules.php
- application/views/main/dashboard/statistic.php
- application/plugins/blacklist_number/views/index.php
- application/views/main/dashboard/home.php
- application/plugins/sms_to_xmpp/views/index.php
- application/views/main/phonebook/contact/index.php
- application/plugins/blacklist_number/controllers/Blacklist_number.php
- application/views/main/password_reset.php
- application/plugins/stop_manager/views/index.php
- application/views/main/phonebook/group/pbk_list.php
- application/views/main/settings/password.php
- application/views/main/install/requirement_check.php
- tests/plugins/Sms_credit_test.php
- application/views/main/install/database_setup.php
- application/controllers/Users.php
- application/views/main/messages/navigation.php
- media/css/blue.css
- application/plugins/server_alert/views/index.php
- application/views/main/forgot_password.php
- application/views/main/layout.php
- application/config/autoload.php
- application/plugins/whitelist_number/views/index.php
- application/views/main/users/navigation.php
- application/views/main/messages/compose.php
- application/views/js_init/phonebook/js_phonebook.php
- application/plugins/sms_credit/views/index.php
- application/controllers/Install.php
- application/helpers/kalkun_helper.php
- tests/plugins/Sms_to_wordpress_test.php
- application/views/main/users/add_user.php
- tests/plugins/Sms_to_twitter_test.php
- application/views/main/base.php
- application/views/main/login.php
- application/plugins/sms_to_email/views/index.php
- tests/plugins/Sms_to_email_test.php
- tests/plugins/Whitelist_number_test.php
- tests/plugins/Server_alert_test.php
- tests/plugins/Sms_member_test.php
- tests/.gitignore
- tests/plugins/Stop_manager_test.php
- application/plugins/soap/views/index.php
- application/views/main/install/config_setup.php
- application/plugins/server_alert/controllers/Server_alert.php
- tests/plugins/Sms_to_xmpp_test.php
- tests/controllers/Phonebook_test.php
- tests/plugins/Blacklist_number_test.php
- application/libraries/Install_info.php
- tests/controllers/Login_test.php
- tests/controllers/Pluginss_test.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/controllers/Kalkun_test.php (3)
tests/testutils/KalkunTestCase.php (5)
KalkunTestCase(15-193)ajaxRequest(140-144)crawler_text(93-107)assertValidHtml(146-169)assertValidHtmlSnippet(171-192)tests/testutils/DBSetup.php (3)
DBSetup(16-1971)setup_config(286-347)insert(1016-1099)tests/testutils/ConfigFile.php (1)
ConfigFile(14-16)
tests/testutils/DBSetup.php (4)
tests/testutils/TestSuiteFile.php (2)
__construct(19-24)write(56-75)tests/testutils/DBVars.php (1)
DBVars(12-17)tests/testutils/ConfigFile.php (1)
ConfigFile(14-16)tests/controllers/Pluginss_test.php (2)
Pluginss_test(15-561)reset_plugins_lib_static_members(27-54)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: test (ubuntu-latest, 7.2)
- GitHub Check: test (ubuntu-latest, 7.3)
- GitHub Check: test (ubuntu-latest, 7.4)
- GitHub Check: test (ubuntu-latest, 5.6)
- GitHub Check: test (ubuntu-latest, 8.4)
- GitHub Check: test (ubuntu-latest, 7.0)
- GitHub Check: test (ubuntu-latest, 7.1)
- GitHub Check: test (ubuntu-latest, 8.3)
- GitHub Check: test (ubuntu-latest, 8.1)
- GitHub Check: test (ubuntu-latest, 8.2)
- GitHub Check: test (ubuntu-latest, 8.0)
- GitHub Check: check-code (ubuntu-latest, 8.2)
🔇 Additional comments (35)
tests/libraries/Install_info_test.php (5)
13-15: Good test file setup with clear dependencies.The file properly includes both the library being tested and the parent test case class.
25-33: Good data provider implementation for testing daemon paths.The data provider correctly covers different scenarios for Windows and non-Windows environments with appropriate file extensions.
38-60: Robust test implementation with PHPUnit version handling.The test method properly mocks the
is_windowsmethod to test different environments without depending on the actual environment. The conditional PHPUnit version handling is a good approach for maintaining backward compatibility.
84-126: Comprehensive test coverage for daemon variable path extraction.The data provider and test method cover a wide range of scenarios including different file types, variable names, and error cases.
128-169: Thorough testing of CI_ENV detection with edge cases.The test covers important edge cases including empty strings, null values, commented lines, and different formatting patterns.
media/css/base.css (2)
707-717: Resetting SMS search form table attributes
Good replacement of deprecatedcellpadding/cellspacingattributes withborder-collapseandborder-spacing. The zero padding ontdmatches a clean, CSS-only layout. No issues found.
1253-1279: Settings and SMS-to-email tables layout
The tables undertable.settingsandtable.sms_to_emailcorrectly migrate spacing and alignment from deprecated attributes to CSS. Overall structure is solid—no critical issues. If many table types share patterns, you could abstract shared properties into a base selector or utility class..github/workflows/codeigniter.yml (7)
83-84: Good addition of system dependencies.Installing these system dependencies is necessary for the tests to function correctly. The gammu-smsd is required for SMS functionality testing, and libdbd-sqlite3 enables SQLite database support.
85-86: Improved database setup approach.Using a dedicated script to set up MySQL and PostgreSQL is a more maintainable approach than the previous inline setup. This makes the workflow easier to understand and modify in the future.
87-94: Comprehensive Java availability check.The approach of checking multiple possible Java paths before installing is thorough. This ensures that Java is available for the v.Nu HTML validator without unnecessary installations.
95-104: Good use of action for downloading v.Nu.Using the release-downloader action with specific version pinning is a good practice. It ensures reproducible builds by consistently using the same version of the validator.
105-109: Well-organized validator setup.The steps to install tidy, set up v.Nu, and start the HTTP server are clear and focused. This is essential for HTML5 validation testing.
110-111: Simplified ci-phpunit-test installation.Moving the complex installation logic to a separate script improves workflow readability and maintainability.
113-113: Added HTML5 environment variable.Setting HTML=5 for the PHPUnit tests is aligned with the PR objective to migrate to valid HTML5.
tests/models/Gammu_model_get_messages_test.php (10)
12-13: Good use of test utilities.The test properly includes the necessary utility files for database setup and test case base class.
17-20: Effective test setup.The setUp method properly initializes the test database environment. Using DBSetup for test database configuration is a good practice for test isolation.
22-29: Well-implemented database reloading.The reloadCIwithEngine method correctly handles switching between different database engines, which allows thorough testing across multiple database types.
36-55: Comprehensive error case testing.The test_get_messages_invalid method properly validates that the system responds correctly to invalid input. Good use of exception expectations.
57-88: Good boundary testing.Testing with a user who doesn't have matching messages ensures that the system handles empty result sets correctly.
90-152: Thorough processing flag testing.The test_get_messages_notProcessed method provides detailed verification of message filtering by processed status, including correct handling of multipart messages.
154-202: Effective inbox message retrieval testing.This test verifies basic inbox message retrieval functionality with detailed assertions on message content and attributes.
254-301: Good ordering test.The test_get_messages_inbox_no_limit_no_offset_order_asc tests that message ordering works correctly, which is important for user interface display.
303-349: Thorough multipart message testing.Testing multipart message retrieval ensures that the system correctly handles more complex message formats.
351-399: Comprehensive pagination testing.The test_get_messages_inbox_limit_2_offset_2 method validates that pagination works correctly, which is crucial for handling large message volumes.
tests/testutils/GammuSmsdConfigFile.php (6)
14-17: Good class design.The GammuSmsdConfigFile class properly extends TestSuiteFile and includes a private property to store configuration data. This follows good OOP principles.
18-30: Effective constructor implementation.The constructor initializes basic Gammu configuration and ensures the dummy device directory exists. Using a marker file for cleanup is a good approach.
31-60: Robust database configuration.The configuration logic handles different database engines (MySQL, PostgreSQL, SQLite) appropriately, using the right driver and connection parameters for each.
62-79: Thorough cleanup in destructor.The destructor properly cleans up temporary files and directories, with appropriate checks to avoid removing non-empty directories. This helps prevent test pollution.
81-84: Simple and effective configuration item management.The addItem method provides a clean interface for adding or updating configuration entries.
86-99: Well-implemented content generation.The get_content method effectively formats the configuration data, with sections sorted alphabetically for consistency and readability.
tests/plugins/Jsonrpc_test.php (4)
17-26: Good test initialization.The setUp method properly prepares the environment by removing the installation file if present. The commented line for database setup is fine as it's likely handled in individual test methods.
38-104: Thorough exception handling for network services.The test_send_sms_client method includes robust error handling for different scenarios when the HTTP server might not be available. This makes the test more resilient in different environments.
127-162: Good JSON-RPC error testing.The test_send_sms_empty_data method properly verifies that the JSON-RPC endpoint handles empty data correctly, checking the error response format and codes.
116-126: Well-documented test constraints.The method comments clearly explain the requirements and limitations of the test, including the need for a separate process and PHPUnit version compatibility.
tests/controllers/Messages_test.php (1)
54-58:⚠️ Potential issueHard-coded redirect expectation may mask real regression
$this->assertRedirect('', 302);accepts an empty location string, therefore any redirect target – even the wrong one – will satisfy the assertion.- $this->assertRedirect('', 302); + $this->assertRedirect(site_url(), 302); // or the real expected URLProviding the explicit URL catches unintended changes in controller behaviour.
Likely an incorrect or invalid review comment.
| private $install_info; | ||
|
|
||
| public function setUp() : void | ||
| { | ||
| parent::setUp(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing initialization in setUp method.
The instance property $install_info is declared but not initialized in the setUp method, yet it's used in the test_get_daemon_url method.
public function setUp() : void
{
parent::setUp();
+ $this->install_info = new Install_info();
}| $output = $this->request('GET', 'plugin/soap'); | ||
| $this->assertValidHtml($output); | ||
| $this->markTestIncomplete(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
markTestIncomplete() blocks real assertions – turn TODOs into checks
Both test_index() and test_api() finish with markTestIncomplete(). This makes the tests always pass and hides regressions in the plugin’s HTML. Replace the placeholder with at least one meaningful assertion (e.g. expected status code or DOM element) or drop the test until it is ready.
Also applies to: 100-104
| $this->request->setHeader('Content-Type', 'Content-Type: text/xml;charset=UTF-8'); | ||
|
|
||
| $output = $this->request('POST', 'plugin/soap/api/index/login', $data); | ||
| $expected_session_id = ''; | ||
| $expected = '<ns1:loginResponse xmlns:ns1="urn:Api"><result xsi:type="xsd:string">'.$expected_session_id.'</result></ns1:loginResponse>'; | ||
| $this->_assertStringContainsString($expected, $output); |
There was a problem hiding this comment.
Avoid duplicating the header name in setHeader() calls
Throughout the file the header is set via
$this->request->setHeader('Content-Type', 'Content-Type: text/xml;charset=UTF-8');
The second argument should contain only the value, otherwise every request ends up with a header literally named Content-Type whose value again starts with Content-Type: – producing an invalid header line and breaking some parsers.
-$this->request->setHeader('Content-Type', 'Content-Type: text/xml;charset=UTF-8');
+$this->request->setHeader('Content-Type', 'text/xml; charset=UTF-8');(The change is needed in every occurrence listed in the selected ranges.)
Also applies to: 275-280, 328-334, 381-386, 430-436
| $configFile = new ConfigFile(APPPATH . 'config/testing/config.php'); | ||
| $content = '<?php $config[\'encryption_key\'] = ' . $enc_key . ';'; | ||
| $configFile->write($content); | ||
|
|
||
| $this->resetInstance(); | ||
| $this->install = new Install; | ||
| $output = $this->install->_uses_default_encryption_key(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Writing to the real config file without rollback can taint subsequent tests
test_uses_default_encryption_key() overwrites config/testing/config.php but never restores the original content.
Add a backup → write → finally restore pattern or write to a temporary file injected through $config['config_path'] to keep the suite hermetic.
| $dir = APPPATH . '..'; | ||
|
|
||
| // Store original mode to restore it later on. | ||
| $mode = substr(sprintf('%o', fileperms($dir)), -4); | ||
| $modeint = intval(base_convert($mode, 8, 10)); | ||
|
|
||
| // Set to read-only | ||
| chmod($dir, 0555); | ||
|
|
||
| $output = $this->request('POST', 'install/config_setup', ['remove_install_file' => 'remove']); | ||
| $this->_assertStringContainsString('You must remove the file manually.', $output); | ||
|
|
||
| // restore original mode. | ||
| chmod($dir, $modeint); | ||
| } |
There was a problem hiding this comment.
Use try … finally when tampering with filesystem permissions
If the request throws or an assertion fails, the directory permissions remain read-only, potentially cascading failures to later tests. Wrap the chmod calls in a try … finally so restoration always executes:
-$modeint = intval(base_convert($mode, 8, 10));
-// Set to read-only
-chmod($dir, 0555);
-
-$output = $this->request('POST', 'install/config_setup', ['remove_install_file' => 'remove']);
-…
-// restore original mode.
-chmod($dir, $modeint);
+$modeint = intval(base_convert($mode, 8, 10));
+chmod($dir, 0555);
+try {
+ $output = $this->request('POST', 'install/config_setup', ['remove_install_file' => 'remove']);
+ …
+} finally {
+ chmod($dir, $modeint);
+}📝 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.
| $dir = APPPATH . '..'; | |
| // Store original mode to restore it later on. | |
| $mode = substr(sprintf('%o', fileperms($dir)), -4); | |
| $modeint = intval(base_convert($mode, 8, 10)); | |
| // Set to read-only | |
| chmod($dir, 0555); | |
| $output = $this->request('POST', 'install/config_setup', ['remove_install_file' => 'remove']); | |
| $this->_assertStringContainsString('You must remove the file manually.', $output); | |
| // restore original mode. | |
| chmod($dir, $modeint); | |
| } | |
| $dir = APPPATH . '..'; | |
| // Store original mode to restore it later on. | |
| $mode = substr(sprintf('%o', fileperms($dir)), -4); | |
| $modeint = intval(base_convert($mode, 8, 10)); | |
| chmod($dir, 0555); | |
| try { | |
| $output = $this->request('POST', 'install/config_setup', ['remove_install_file' => 'remove']); | |
| $this->_assertStringContainsString('You must remove the file manually.', $output); | |
| } finally { | |
| chmod($dir, $modeint); | |
| } | |
| } |
| $result = $this->CI->db | ||
| ->where('ID', '1') | ||
| ->get('inbox'); | ||
| $this->assertEquals(6, $result->row()->id_folder); | ||
|
|
||
| $word = '<(Mono)...>'; // Any word that is only once in the textmessage | ||
| $result = $this->CI->db | ||
| ->where('token', $word) | ||
| ->get('b8_wordlist'); | ||
| $this->assertEquals(1, $result->row()->count_spam); | ||
| } |
There was a problem hiding this comment.
Possible null‐dereference when asserting spam word counts
$result = $this->CI->db->where('token', $word)->get('b8_wordlist');
If the query returns no rows, $result->row() is null; accessing ->count_spam will trigger a fatal error and mask the real failure.
- $this->assertEquals(1, $result->row()->count_spam);
+ $this->assertGreaterThan(
+ 0,
+ $result->num_rows(),
+ 'Expected spam word not found in b8_wordlist'
+ );
+ $this->assertEquals(1, $result->row()->count_spam);📝 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.
| $result = $this->CI->db | |
| ->where('ID', '1') | |
| ->get('inbox'); | |
| $this->assertEquals(6, $result->row()->id_folder); | |
| $word = '<(Mono)...>'; // Any word that is only once in the textmessage | |
| $result = $this->CI->db | |
| ->where('token', $word) | |
| ->get('b8_wordlist'); | |
| $this->assertEquals(1, $result->row()->count_spam); | |
| } | |
| $result = $this->CI->db | |
| ->where('ID', '1') | |
| ->get('inbox'); | |
| $this->assertEquals(6, $result->row()->id_folder); | |
| $word = '<(Mono)...>'; // Any word that is only once in the textmessage | |
| $result = $this->CI->db | |
| ->where('token', $word) | |
| ->get('b8_wordlist'); | |
| $this->assertGreaterThan( | |
| 0, | |
| $result->num_rows(), | |
| 'Expected spam word not found in b8_wordlist' | |
| ); | |
| $this->assertEquals(1, $result->row()->count_spam); | |
| } |
| $configFile = new ConfigFile(APPPATH . 'config/testing/kalkun_settings.php'); | ||
| $content = '<?php ' | ||
| . '$config[\'sms_bomber\'] = \'TRUE\';'; | ||
| $configFile->write($content); | ||
|
|
||
| $this->request->setCallablePreConstructor( | ||
| function () { | ||
| if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Temporary config files are written but never cleaned up
ConfigFile->write() alters APPPATH . 'config/testing/kalkun_settings.php', yet the file is not reverted in tearDown().
Parallel or subsequent tests may read stale settings, leading to heisenbugs.
Create a helper that:
- Backs up the original file contents.
- Restores them in
tearDownAfterClass()/tearDown().
| $this->request->setCallablePreConstructor( | ||
| function () { | ||
| if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) | ||
| { | ||
| session_start(); | ||
| } | ||
| $_SESSION['loggedin'] = 'TRUE'; | ||
| $_SESSION['id_user'] = '1'; | ||
| $_SESSION['level'] = 'admin'; | ||
| $_SESSION['username'] = 'kalkun'; | ||
| } | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
session_start() is never executed when the tests are run from CLI
is_cli() === FALSE prevents session_start() from being executed during test runs (PHP-CLI), yet the code goes on to populate $_SESSION.
While manipulating the super-global manually works, certain session-related CodeIgniter helpers expect an active session which could trigger subtle bugs (e.g. headers already sent, missing session cookie).
- if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE)
+ if (session_status() === PHP_SESSION_NONE)Consider dropping the is_cli() guard globally (or call session_start() once in KalkunTestCase::setUp()) to guarantee a consistent test environment.
🏁 Script executed:
#!/bin/bash
# Locate the Messages_test.php file and show lines 1–100 and 35–60 to verify the session logic
FILE=$(rg -l "class Messages_test" -g "tests/controllers/*.php")
echo "Found file: $FILE"
echo "Lines 1–100:"
sed -n '1,100p' "$FILE"
echo "Lines 35–60:"
sed -n '35,60p' "$FILE"Length of output: 3675
Ensure sessions are actually started in your tests
The is_cli() === FALSE guard prevents session_start() from ever running under PHPUnit (PHP-CLI), so although you manually set $_SESSION keys, helpers or code that expect an active session (cookies, headers, etc.) won’t see one—this could trigger subtle test-only failures.
Please update as follows:
- In tests/controllers/Messages_test.php (around the
setCallablePreConstructorin each test):- if (session_status() === PHP_SESSION_NONE && is_cli() === FALSE) + if (session_status() === PHP_SESSION_NONE) { session_start(); }
- Alternatively, add a global session bootstrap in your test base class (tests/testutils/KalkunTestCase.php):
protected function setUp(): void { parent::setUp(); if (session_status() === PHP_SESSION_NONE) { session_start(); } // …existing setup… }
Either approach guarantees a real session is active under CLI and prevents hidden bugs in session-dependent code.
| $this->assertJson($data); | ||
| $data_decoded = json_decode($data); | ||
| if ($expected === TRUE) | ||
| { | ||
| $this->assertEquals('true', $data_decoded); | ||
| } | ||
| else | ||
| { | ||
| $this->assertNotEquals('true', $data_decoded); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
String comparison with JSON boolean hides type errors
json_decode('true') returns bool true, yet the test compares it to string 'true'.
The loose comparison performed by assertEquals() will pass, but any unexpected non-boolean truthy value would also pass.
- $this->assertEquals('true', $data_decoded);
+ $this->assertTrue($data_decoded);Use assertTrue/False to assert exact boolean semantics.
📝 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.
| $this->assertJson($data); | |
| $data_decoded = json_decode($data); | |
| if ($expected === TRUE) | |
| { | |
| $this->assertEquals('true', $data_decoded); | |
| } | |
| else | |
| { | |
| $this->assertNotEquals('true', $data_decoded); | |
| } | |
| $this->assertJson($data); | |
| $data_decoded = json_decode($data); | |
| if ($expected === TRUE) | |
| { | |
| $this->assertTrue($data_decoded); | |
| } | |
| else | |
| { | |
| $this->assertNotEquals('true', $data_decoded); | |
| } |
| $this->assertEquals('', $row->Text); | ||
| $this->assertEquals('+123456', $row->DestinationNumber); | ||
| $this->assertEquals('Unicode_No_Compression', $row->Coding); | ||
| $this->_assertMatchesRegularExpression('/050003..'.sprintf('%02X', $parts).'01/', $row->UDH); | ||
| $this->assertEquals(1, $row->Class); | ||
| $expected_message = sprintf($message_part_template, 1); |
There was a problem hiding this comment.
_assertMatchesRegularExpression() likely a typo – tests may never run
PHPUnit provides assertMatchesRegularExpression() (no leading underscore).
Unless KalkunTestCase defines a custom alias, this will trigger a fatal error at runtime and abort the entire suite.
- $this->_assertMatchesRegularExpression('/050003..'.sprintf('%02X', $parts).'01/', $row->UDH);
+ $this->assertMatchesRegularExpression('/050003..'.sprintf('%02X', $parts).'01/', $row->UDH);Verify whether KalkunTestCase exposes a wrapper. If not, apply the change everywhere _assertMatchesRegularExpression is used.
📝 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.
| $this->assertEquals('', $row->Text); | |
| $this->assertEquals('+123456', $row->DestinationNumber); | |
| $this->assertEquals('Unicode_No_Compression', $row->Coding); | |
| $this->_assertMatchesRegularExpression('/050003..'.sprintf('%02X', $parts).'01/', $row->UDH); | |
| $this->assertEquals(1, $row->Class); | |
| $expected_message = sprintf($message_part_template, 1); | |
| $this->assertEquals('', $row->Text); | |
| $this->assertEquals('+123456', $row->DestinationNumber); | |
| $this->assertEquals('Unicode_No_Compression', $row->Coding); | |
| - $this->_assertMatchesRegularExpression('/050003..'.sprintf('%02X', $parts).'01/', $row->UDH); | |
| + $this->assertMatchesRegularExpression('/050003..'.sprintf('%02X', $parts).'01/', $row->UDH); | |
| $this->assertEquals(1, $row->Class); | |
| $expected_message = sprintf($message_part_template, 1); |
2 participants
Add tests & migrate to valid HTML5
Coderabbit below is somehow wrong in its analysis...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor & Style
Chores