added: plugin action links class to add Settings and Get Pro links - #1051
Conversation
WalkthroughA new class was added to handle plugin action links in the admin interface, including logic for "Settings" and "Get Pro" links. The admin initialization was updated to register this functionality. New SCSS styles were introduced to visually distinguish these action links, particularly the "Go Pro" link. The translation template was updated to include new strings and references related to these links. Additionally, a PHPUnit test suite was created to verify the behavior of the new plugin action links class. Changes
Sequence Diagram(s)sequenceDiagram
participant WP_Admin as WordPress Admin
participant Admin as Admin
participant PluginActionLinks as Plugin_Action_Links
WP_Admin->>Admin: init()
Admin->>PluginActionLinks: new Plugin_Action_Links()
Admin->>PluginActionLinks: init_hooks()
PluginActionLinks->>WP_Admin: Add filter for plugin_action_links
WP_Admin->>PluginActionLinks: plugin_action_links($links)
PluginActionLinks->>WP_Admin: Return modified $links (Settings, Get Pro)
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @SteveJonesDev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the plugin's administrative interface by adding custom action links to its entry in the WordPress plugin list table. It provides quick access to plugin settings and prominently displays an option for non-pro users to upgrade, with accompanying styling to make the upgrade link stand out.
Highlights
- New Feature: Plugin Action Links: Introduced a dedicated
Plugin_Action_Linksclass to manage and display custom action links ('Settings' and 'Get Pro') directly on the WordPress plugin list table, enhancing user navigation and discoverability. - Admin Integration: The newly created
Plugin_Action_Linksfunctionality has been seamlessly integrated into the main admin initialization process, ensuring the custom links are properly registered and displayed when the admin interface loads. - Styling for 'Get Pro' Link: Custom SCSS has been added to visually distinguish the 'Get Pro' link. It is now styled with bold red text and includes appropriate hover/focus effects, aiming to improve its visibility and encourage user engagement for non-pro users.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
The POT file has changed. Please update the POT file in your PR to keep translations up to date. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new Plugin_Action_Links class to add "Settings" and "Get Pro" links to the plugin's action links in the WordPress admin, along with corresponding styles. The implementation is solid, but I've identified a few areas for improvement, primarily concerning security and code quality. My main feedback points are escaping URLs and refining the SCSS to remove redundant properties.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
admin/class-admin.php (1)
51-52: Add use statement for Plugin_Action_Links class.The class is instantiated but there's no corresponding
usestatement at the top of the file. For consistency with other classes in this file, add the use statement.+use EDAC\Admin\Plugin_Action_Links; use EDAC\Admin\SiteHealth\Information; use EDAC\Admin\Purge_Post_Data; use EDAC\Admin\Post_Save;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
admin/class-admin.php(1 hunks)admin/class-plugin-action-links.php(1 hunks)src/admin/sass/accessibility-checker-admin.scss(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/admin/sass/accessibility-checker-admin.scss (3)
Learnt from: SteveJonesDev
PR: equalizedigital/accessibility-checker#921
File: src/pageScanner/checks/linked-image-alt-present.js:43-50
Timestamp: 2025-04-15T02:30:38.897Z
Learning: In the Accessibility Checker plugin, accessibility rules are separated by specific concerns. For example, linked images have separate rules for checking: (1) missing alt attributes and (2) empty alt attributes. The rule `linked_image_alt_present` specifically checks for the presence of alt attributes on linked images, while a separate rule `img_linked_alt_empty` handles validation of empty alt attributes. Suggestions should respect this separation of concerns.
Learnt from: pattonwebz
PR: equalizedigital/accessibility-checker#927
File: src/pageScanner/checks/img-alt-missing-check.js:35-37
Timestamp: 2025-04-18T14:27:18.140Z
Learning: In the Accessibility Checker plugin, the img_alt_missing rule specifically checks for missing alt attributes on images and image inputs, while empty alt attributes are handled by a separate rule. Each accessibility concern is deliberately separated into individual rules.
Learnt from: pattonwebz
PR: equalizedigital/accessibility-checker#927
File: src/pageScanner/checks/img-alt-missing-check.js:32-32
Timestamp: 2025-04-18T14:27:49.512Z
Learning: In the Accessibility Checker plugin, the img_alt_missing rule specifically checks for missing alt attributes (whether the attribute exists at all), while empty alt attributes (alt="") are handled by a separate rule. This separation of concerns is by design.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: make-pot
🔇 Additional comments (3)
src/admin/sass/accessibility-checker-admin.scss (1)
2005-2016: LGTM! Well-implemented CSS for plugin action links.The styling follows best practices with proper SASS nesting, design system variables, and accessibility-friendly hover/focus states. The BEM naming convention is consistent with the codebase.
admin/class-plugin-action-links.php (2)
39-43: LGTM! Proper URL construction and escaping.The Settings link is properly constructed with admin_url() and esc_html__() for security and internationalization.
52-56: Good accessibility implementation.The aria-label provides excellent context for screen reader users, clearly indicating the link opens in a new window.
… for plugin_action_links
…ugin_action_links
|
The POT file has changed. Please update the POT file in your PR to keep translations up to date. |
SteveJonesDev
changed the title
…ve/pro-190-plugin-action-links-add-get-accessibility-checker-pro-and Update translation files (POT/JSON)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
tests/phpunit/Admin/PluginActionLinksTest.php (4)
32-47: Consider using more descriptive test names.While method existence tests are useful, consider using more descriptive test names that indicate the expected behavior rather than just existence.
- public function test_init_hooks_method_exists() { + public function test_init_hooks_method_is_callable() {- public function test_add_plugin_action_links_method_exists() { + public function test_add_plugin_action_links_method_is_callable() {
104-117: Remove unused parameter from mock function.The mock function has an unused parameter that triggers static analysis warnings.
- function edac_link_wrapper( $url, $source, $campaign, $unused ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable + function edac_link_wrapper( $url, $source, $campaign ) {
99-101: Consider using dependency injection for better testability.Directly defining constants in tests can lead to coupling and makes tests less maintainable. Consider using dependency injection or mocking frameworks.
- // Define the constant as false for this test. - if ( ! defined( 'EDAC_KEY_VALID' ) ) { - define( 'EDAC_KEY_VALID', false ); - } + // Mock the constant check in the actual class instead + // or use a test double that allows dependency injection
88-92: Enhance test assertions for better reliability.Consider using more specific assertions and testing the actual URL structure rather than just string contains checks.
- $this->assertStringContainsString( 'Settings', $first_link, 'Settings link not found' ); - $this->assertStringContainsString( 'accessibility_checker_settings', $first_link, 'Settings link does not point to correct page' ); + $this->assertStringContainsString( 'Settings', $first_link, 'Settings link text not found' ); + $this->assertStringContainsString( 'page=accessibility_checker_settings', $first_link, 'Settings link does not point to correct admin page' ); + $this->assertStringContainsString( 'admin.php', $first_link, 'Settings link does not point to admin area' );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/phpunit/Admin/PluginActionLinksTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/phpunit/Admin/PluginActionLinksTest.php (1)
Learnt from: SteveJonesDev
PR: equalizedigital/accessibility-checker#921
File: src/pageScanner/checks/linked-image-alt-present.js:43-50
Timestamp: 2025-04-15T02:30:38.897Z
Learning: In the Accessibility Checker plugin, accessibility rules are separated by specific concerns. For example, linked images have separate rules for checking: (1) missing alt attributes and (2) empty alt attributes. The rule `linked_image_alt_present` specifically checks for the presence of alt attributes on linked images, while a separate rule `img_linked_alt_empty` handles validation of empty alt attributes. Suggestions should respect this separation of concerns.
🪛 PHPMD (2.15.0)
tests/phpunit/Admin/PluginActionLinksTest.php
114-114: Avoid unused parameters such as '$unused'. (Unused Code Rules)
(UnusedFormalParameter)
177-177: Avoid unused parameters such as '$unused'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.1 | WP latest
- GitHub Check: Integration Test: PHP 8.2 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.1 | WP 6.2
- GitHub Check: make-pot
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.1 | WP latest
- GitHub Check: Integration Test: PHP 8.2 | WP latest (+ ms)
🔇 Additional comments (4)
tests/phpunit/Admin/PluginActionLinksTest.php (4)
1-13: LGTM! Well-structured test class setup.The file header, namespace usage, and class declaration follow PHPUnit best practices.
25-27: LGTM! Standard PHPUnit setUp method.The setUp method properly initializes the class under test.
122-126: Good comprehensive testing of the pro link attributes.The test thoroughly verifies various aspects of the pro link including text, target, CSS class, and accessibility attributes.
194-199: LGTM! Proper test cleanup.The tearDown method properly cleans up filters to maintain test isolation.
| if ( ! defined( 'EDAC_PLUGIN_FILE' ) ) { | ||
| define( 'EDAC_PLUGIN_FILE', __FILE__ ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential test isolation issues with constant definitions.
Defining constants conditionally can cause test isolation issues if tests run in different orders. Consider using a more robust approach for test constants.
- if ( ! defined( 'EDAC_PLUGIN_FILE' ) ) {
- define( 'EDAC_PLUGIN_FILE', __FILE__ );
- }
+ // Use a test-specific constant or mock the dependency
+ if ( ! defined( 'EDAC_PLUGIN_FILE' ) ) {
+ define( 'EDAC_PLUGIN_FILE', dirname( __DIR__, 3 ) . '/accessibility-checker.php' );
+ }📝 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.
| if ( ! defined( 'EDAC_PLUGIN_FILE' ) ) { | |
| define( 'EDAC_PLUGIN_FILE', __FILE__ ); | |
| } | |
| // Use a test-specific constant or mock the dependency | |
| if ( ! defined( 'EDAC_PLUGIN_FILE' ) ) { | |
| define( 'EDAC_PLUGIN_FILE', dirname( __DIR__, 3 ) . '/accessibility-checker.php' ); | |
| } |
🤖 Prompt for AI Agents
In tests/phpunit/Admin/PluginActionLinksTest.php around lines 54 to 56, defining
the constant EDAC_PLUGIN_FILE conditionally can cause test isolation issues when
tests run in different orders. To fix this, refactor the test setup to avoid
defining constants conditionally; instead, define the constant once in a
dedicated bootstrap or setup method that runs before all tests, or mock the
constant's usage to ensure consistent test isolation and prevent side effects
between tests.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/phpunit/Admin/PluginActionLinksTest.php (2)
54-56: Potential test isolation issues with constant definitions.Defining constants conditionally can cause test isolation issues if tests run in different orders. Consider using a more robust approach for test constants.
166-180: Eliminate duplicate mock function definition.The
edac_link_wrapperfunction is defined twice in the file, which is redundant and could cause issues. Consider extracting this to a shared setUp method or using a more robust mocking approach.
🧹 Nitpick comments (2)
tests/phpunit/Admin/PluginActionLinksTest.php (2)
104-117: Remove unused parameter from mock function.The mock function has an unused parameter that should be removed to clean up the code.
- if ( ! function_exists( 'edac_link_wrapper' ) ) { - /** - * Mock the edac_link_wrapper function. - * - * @param string $url The URL to wrap. - * @param string $source The source parameter. - * @param string $campaign The campaign parameter. - * @param bool $unused Unused parameter for compatibility. - * @return string The wrapped URL. - */ - function edac_link_wrapper( $url, $source, $campaign, $unused ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable - return $url . '?utm_source=' . $source . '&utm_campaign=' . $campaign; - } - } + if ( ! function_exists( 'edac_link_wrapper' ) ) { + /** + * Mock the edac_link_wrapper function. + * + * @param string $url The URL to wrap. + * @param string $source The source parameter. + * @param string $campaign The campaign parameter. + * @return string The wrapped URL. + */ + function edac_link_wrapper( $url, $source, $campaign ) { + return $url . '?utm_source=' . $source . '&utm_campaign=' . $campaign; + } + }
98-101: Consider centralizing constant definitions for better test isolation.Multiple test methods define the same constants conditionally, which can lead to test order dependencies. Consider moving these to a shared setUp method.
protected function setUp(): void { $this->plugin_action_links = new Plugin_Action_Links(); + + // Define test constants if not already defined + if ( ! defined( 'EDAC_PLUGIN_FILE' ) ) { + define( 'EDAC_PLUGIN_FILE', dirname( __DIR__, 3 ) . '/accessibility-checker.php' ); + } + if ( ! defined( 'EDAC_KEY_VALID' ) ) { + define( 'EDAC_KEY_VALID', false ); + } + + // Mock the edac_link_wrapper function if it doesn't exist + if ( ! function_exists( 'edac_link_wrapper' ) ) { + /** + * Mock the edac_link_wrapper function. + * + * @param string $url The URL to wrap. + * @param string $source The source parameter. + * @param string $campaign The campaign parameter. + * @return string The wrapped URL. + */ + function edac_link_wrapper( $url, $source, $campaign ) { + return $url . '?utm_source=' . $source . '&utm_campaign=' . $campaign; + } + } }Also applies to: 162-164
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/phpunit/Admin/PluginActionLinksTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/phpunit/Admin/PluginActionLinksTest.php (1)
Learnt from: SteveJonesDev
PR: equalizedigital/accessibility-checker#921
File: src/pageScanner/checks/linked-image-alt-present.js:43-50
Timestamp: 2025-04-15T02:30:38.897Z
Learning: In the Accessibility Checker plugin, accessibility rules are separated by specific concerns. For example, linked images have separate rules for checking: (1) missing alt attributes and (2) empty alt attributes. The rule `linked_image_alt_present` specifically checks for the presence of alt attributes on linked images, while a separate rule `img_linked_alt_empty` handles validation of empty alt attributes. Suggestions should respect this separation of concerns.
🪛 PHPMD (2.15.0)
tests/phpunit/Admin/PluginActionLinksTest.php
114-114: Avoid unused parameters such as '$unused'. (Unused Code Rules)
(UnusedFormalParameter)
177-177: Avoid unused parameters such as '$unused'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.2 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.1 | WP 6.2
- GitHub Check: make-pot
🔇 Additional comments (6)
tests/phpunit/Admin/PluginActionLinksTest.php (6)
1-27: Test structure and setup look good.The test class follows WordPress testing conventions and properly extends
WP_UnitTestCase. The setUp method correctly initializes the class under test.
32-47: Method existence tests are appropriate.These tests ensure the required methods exist on the class, which is good for catching basic structural issues.
97-126: Pro link test logic is comprehensive.The test properly validates the conditional logic for adding the "Get Pro" link and checks all the required attributes including CSS class, target, and text content.
131-155: Link preservation and URL escaping tests are solid.These tests ensure that existing plugin action links are preserved and that URLs are properly escaped, which is important for security and functionality.
160-189: Accessibility test coverage is excellent.The test properly validates that accessibility attributes like
aria-labelare included in the pro link, which aligns well with the accessibility focus of this plugin.
194-200: Proper cleanup in tearDown method.The tearDown method correctly removes filters to prevent test pollution, which is good practice for test isolation.
…ns for Plugin_Action_Links tests
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/phpunit/Admin/PluginActionLinksTest.php (1)
37-43: Address test isolation issues with constant definitions.Defining constants conditionally in test methods can cause test isolation problems when tests run in different orders or when constants are already defined elsewhere in the test suite.
Consider using a more robust approach for test constants:
- // Define constants if not already defined. - if ( ! defined( 'EDAC_PLUGIN_FILE' ) ) { - define( 'EDAC_PLUGIN_FILE', __FILE__ ); - } - - if ( ! defined( 'EDAC_KEY_VALID' ) ) { - define( 'EDAC_KEY_VALID', false ); - } + // Use a test-specific constant path + if ( ! defined( 'EDAC_PLUGIN_FILE' ) ) { + define( 'EDAC_PLUGIN_FILE', dirname( __DIR__, 3 ) . '/accessibility-checker.php' ); + } + + if ( ! defined( 'EDAC_KEY_VALID' ) ) { + define( 'EDAC_KEY_VALID', false ); + }
🧹 Nitpick comments (1)
tests/phpunit/Admin/PluginActionLinksTest.php (1)
46-59: Remove unused parameter from mock function.The static analysis correctly identifies that the
$unusedparameter is not used in the mock function, which creates unnecessary complexity.- // Mock the edac_link_wrapper function if it doesn't exist. - if ( ! function_exists( 'edac_link_wrapper' ) ) { - /** - * Mock the edac_link_wrapper function. - * - * @param string $url The URL to wrap. - * @param string $source The source parameter. - * @param string $campaign The campaign parameter. - * @param bool $unused Unused parameter for compatibility. - * @return string The wrapped URL. - */ - function edac_link_wrapper( $url, $source, $campaign, $unused ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable - return $url . '?utm_source=' . $source . '&utm_campaign=' . $campaign; - } - } + // Mock the edac_link_wrapper function if it doesn't exist. + if ( ! function_exists( 'edac_link_wrapper' ) ) { + /** + * Mock the edac_link_wrapper function. + * + * @param string $url The URL to wrap. + * @param string $source The source parameter. + * @param string $campaign The campaign parameter. + * @return string The wrapped URL. + */ + function edac_link_wrapper( $url, $source, $campaign ) { + return $url . '?utm_source=' . $source . '&utm_campaign=' . $campaign; + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/phpunit/Admin/PluginActionLinksTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/phpunit/Admin/PluginActionLinksTest.php (1)
Learnt from: SteveJonesDev
PR: equalizedigital/accessibility-checker#921
File: src/pageScanner/checks/linked-image-alt-present.js:43-50
Timestamp: 2025-04-15T02:30:38.897Z
Learning: In the Accessibility Checker plugin, accessibility rules are separated by specific concerns. For example, linked images have separate rules for checking: (1) missing alt attributes and (2) empty alt attributes. The rule `linked_image_alt_present` specifically checks for the presence of alt attributes on linked images, while a separate rule `img_linked_alt_empty` handles validation of empty alt attributes. Suggestions should respect this separation of concerns.
🪛 PHPMD (2.15.0)
tests/phpunit/Admin/PluginActionLinksTest.php
56-56: Avoid unused parameters such as '$unused'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Integration Test: PHP 8.1 | WP latest
- GitHub Check: Integration Test: PHP 8.1 | WP 6.2
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.2 | WP latest (+ ms)
- GitHub Check: make-pot
- GitHub Check: Integration Test: PHP 8.1 | WP latest
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.1 | WP 6.2
🔇 Additional comments (2)
tests/phpunit/Admin/PluginActionLinksTest.php (2)
25-30: LGTM! Well-structured test setup.The setUp method properly initializes the test fixture and delegates mock setup to a dedicated method, following good testing practices.
180-185: LGTM! Proper test cleanup.The tearDown method correctly removes filters to prevent test pollution, which is essential for test isolation.
…ty based on EDAC_KEY_VALID
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/phpunit/Admin/PluginActionLinksTest.php (1)
37-43: Potential test isolation issues with constant definitions.Defining constants conditionally can cause test isolation issues if tests run in different orders. This approach was previously flagged and the issue persists.
🧹 Nitpick comments (2)
tests/phpunit/Admin/PluginActionLinksTest.php (2)
56-56: Remove unused parameter from mock function.The
$unusedparameter in the mock function is not used and was flagged by static analysis.- function edac_link_wrapper( $url, $source, $campaign, $unused ) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable + function edac_link_wrapper( $url, $source, $campaign ) {
187-210: Proper handling of architectural limitations.The approach of marking this test as incomplete with clear documentation is appropriate given PHP's constant redefinition limitations. The detailed comments provide excellent guidance for future refactoring.
Consider implementing dependency injection in the Plugin_Action_Links class to make constants testable:
public function __construct( $key_valid = null ) { $this->key_valid = $key_valid ?? ( defined( 'EDAC_KEY_VALID' ) ? EDAC_KEY_VALID : false ); }This would enable proper testing of both scenarios without constant redefinition issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/phpunit/Admin/PluginActionLinksTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/phpunit/Admin/PluginActionLinksTest.php (1)
Learnt from: SteveJonesDev
PR: equalizedigital/accessibility-checker#921
File: src/pageScanner/checks/linked-image-alt-present.js:43-50
Timestamp: 2025-04-15T02:30:38.897Z
Learning: In the Accessibility Checker plugin, accessibility rules are separated by specific concerns. For example, linked images have separate rules for checking: (1) missing alt attributes and (2) empty alt attributes. The rule `linked_image_alt_present` specifically checks for the presence of alt attributes on linked images, while a separate rule `img_linked_alt_empty` handles validation of empty alt attributes. Suggestions should respect this separation of concerns.
🪛 PHPMD (2.15.0)
tests/phpunit/Admin/PluginActionLinksTest.php
56-56: Avoid unused parameters such as '$unused'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (2)
tests/phpunit/Admin/PluginActionLinksTest.php (2)
125-140: Good improvement in test logic for pro link conditions.The test now properly checks the EDAC_KEY_VALID constant and asserts different outcomes based on its value. This addresses the previous feedback about test robustness.
174-182: Excellent accessibility testing approach.The test properly verifies accessibility attributes for the pro link, which is especially important for an accessibility checker plugin. The conditional check ensures the test only runs when the pro link is present.
…nit_hooks method
pattonwebz
left a comment
pattonwebz
left a comment
There was a problem hiding this comment.
Tested this and the links look to go to the correct urls.
The Get Pro link wouldn't show for me though. I suggested a fix for that.
Co-authored-by: William Patton <will@pattonwebz.com>
2 participants
This pull request introduces functionality to add custom action links to the plugin list table, including a "Settings" link and a "Get Pro" link for non-pro users. It also includes related styling for these links. The most important changes are the addition of the
Plugin_Action_Linksclass, integration of its hooks in the admin initialization, and styling updates for the "Get Pro" link.Functional changes:
Plugin_Action_Linksclass: Implements the functionality to display custom plugin action links ("Settings" and "Get Pro") in the plugin list table.Plugin_Action_Linkshooks: Updated theadmin/class-admin.phpfile to initialize the hooks forPlugin_Action_Linksduring admin initialization.Styling changes:
accessibility-checker-admin.scssto include bold red text and hover/focus effects for the "Get Pro" link.Summary by CodeRabbit
New Features
Style