refactored: landmark link generation and add unit tests for edac_gene… - #1085
Conversation
…rate_landmark_link function
|
""" WalkthroughA new helper function for generating landmark links was introduced and integrated into the AJAX details response, replacing inline link generation logic. Associated unit tests were added to verify the new function’s behavior. A minor CSS adjustment was made to add left margin to a highlighted element class. Changes
Sequence Diagram(s)sequenceDiagram
participant AJAX Handler
participant Helper Function
participant Client
AJAX Handler->>Helper Function: edac_generate_landmark_link(landmark, selector, post_id)
Helper Function-->>AJAX Handler: Returns HTML link or plain text
AJAX Handler-->>Client: Sends AJAX response with generated landmark link
Estimated code review effort2 (~20 minutes) 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)
✨ 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 significantly enhances the codebase by centralizing the logic for generating landmark links, making the code more modular and easier to maintain. It also introduces robust unit tests to ensure the reliability and correctness of this new functionality, alongside a small styling improvement for better layout.
Highlights
- Code Refactoring: The core logic for generating landmark links has been extracted into a new, reusable function
edac_generate_landmark_linkinincludes/helper-functions.php. - Improved Maintainability: Existing inline landmark link generation in
admin/class-ajax.phphas been replaced with calls to the new helper function, significantly reducing code duplication and improving readability. - Comprehensive Unit Tests: A new test file,
GenerateLandmarkLinkTest.php, has been added with extensive unit tests for theedac_generate_landmark_linkfunction, covering various scenarios including selectors, special characters, custom classes, and nonce verification. - Styling Adjustment: A minor CSS change was introduced in
src/frontendHighlighterApp/sass/app.scssto add amargin-leftto thebodyelement for improved frontend layout spacing.
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. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors landmark link generation into a reusable function and adds unit tests. Review comments address potential XSS vulnerabilities due to incorrect escaping and suggest improvements to CSS styling for better maintainability.
| * @return string The HTML for the landmark link or just the landmark text if no selector. | ||
| */ | ||
| function edac_generate_landmark_link( $landmark, $landmark_selector, $post_id, $css_class = 'edac-details-rule-records-record-landmark-link', $target_blank = true ) { | ||
| $landmark = esc_html( $landmark ); |
There was a problem hiding this comment.
The esc_html() function is called before ucwords(). This can lead to issues if $landmark contains HTML entities, as ucwords() won't properly capitalize them after HTML escaping. It's better to capitalize first, then escape for output to prevent double-encoding or incorrect display of special characters. Consider capitalizing the landmark before escaping1.
function edac_generate_landmark_link( $landmark, $landmark_selector, $post_id, $css_class = 'edac-details-rule-records-record-landmark-link', $target_blank = true ) {
if ( empty( $landmark ) ) {
return '';
}
$landmark = ucwords( $landmark );
$landmark = esc_html( $landmark );Style Guide References
Footnotes
There was a problem hiding this comment.
I don't think it matters for our case because we shouldn't be passing in html entities directly but this comment is technically correct.
We could remove the early escape as it doesn't seem needed at all - or move it to wrap around the ucwords on lines 792 and 797.
| $this->assertStringNotContainsString( '<script>', $result ); | ||
| $this->assertStringNotContainsString( 'alert("xss")', $result ); | ||
| $this->assertStringContainsString( '<Script>Alert("Xss")</Script>', $result ); |
There was a problem hiding this comment.
The assertion assertStringContainsString is used to check for the presence of a specific string within the result. However, after applying esc_html, the special characters in the landmark will be encoded. Therefore, the assertion should check for the encoded string <Script>Alert("Xss")</Script> instead of the raw HTML.
$this->assertStringContainsString( '<Script>Alert("Xss")</Script>', $result );![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
admin/class-ajax.php(1 hunks)includes/helper-functions.php(1 hunks)src/frontendHighlighterApp/sass/app.scss(1 hunks)tests/phpunit/helper-functions/GenerateLandmarkLinkTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
admin/class-ajax.php (1)
includes/helper-functions.php (1)
edac_generate_landmark_link(764-798)
tests/phpunit/helper-functions/GenerateLandmarkLinkTest.php (1)
includes/helper-functions.php (1)
edac_generate_landmark_link(764-798)
🪛 GitHub Actions: Test
tests/phpunit/helper-functions/GenerateLandmarkLinkTest.php
[error] 179-179: PHPUnit failure: Failed asserting that generated landmark link with special characters contains expected escaped string.
[error] 193-193: PHPUnit failure: Failed asserting that generated landmark link with special selector contains expected encoded query string.
[error] 262-262: PHPUnit failure: Failed asserting that two strings are equal in landmark link URL structure test (base64 encoding mismatch).
🪛 GitHub Actions: Coverage Report
tests/phpunit/helper-functions/GenerateLandmarkLinkTest.php
[error] 179-179: PHPUnit failure: Failed asserting that the generated link contains the expected escaped script tag string.
[error] 193-193: PHPUnit failure: Failed asserting that the generated link contains the expected encoded landmark query string.
[error] 262-262: PHPUnit failure: Failed asserting that two strings are equal; expected 'bmF2LnByaW1hcnk=' but got 'bmF2LnByaW1hcnk'.
🔇 Additional comments (5)
src/frontendHighlighterApp/sass/app.scss (1)
38-38: LGTM: Clean styling enhancement.The addition of
margin-left: 15px !important;improves the layout spacing for highlighted elements, which aligns well with the landmark functionality enhancements in this PR.includes/helper-functions.php (1)
753-798: Excellent implementation with proper security and accessibility measures.The function demonstrates solid WordPress development practices:
- Security: Proper input sanitization with
esc_html(), output escaping withesc_url()/esc_attr(), and nonce generation- Accessibility: ARIA labels with internationalization support
- Flexibility: Configurable CSS classes and target attributes
- Edge case handling: Early returns for empty landmarks
The centralization of landmark link logic will improve maintainability and consistency across the codebase.
admin/class-ajax.php (1)
487-490: Clean refactoring that improves maintainability.The replacement of manual landmark link generation with the centralized
edac_generate_landmark_linkhelper function reduces code duplication and ensures consistent link generation across the codebase.tests/phpunit/helper-functions/GenerateLandmarkLinkTest.php (2)
23-43: Well-structured test setup and teardown.The test class properly creates and cleans up test posts, providing a solid foundation for the test scenarios.
48-70: Comprehensive link generation testing.The test thoroughly validates the HTML structure, URL parameters, and accessibility features of the generated landmark links.
…se64 encoded selector in URL
pattonwebz
left a comment
pattonwebz
left a comment
There was a problem hiding this comment.
I tested this and it is working for me. Nice addition of the label :)
I left just one comment here https://github.com/equalizedigital/accessibility-checker/pull/1085/files#r2215895343 but it isn't something I think we need to fix, but you can if you agree with the AI that it's high urgency.
2 participants
This pull request introduces significant improvements to the codebase by refactoring landmark link generation into a reusable function, enhancing maintainability and readability. Additionally, it includes updates to styling and comprehensive unit tests for the new function.
Code Refactoring and Functionality Enhancement:
edac_generate_landmark_link, inincludes/helper-functions.php. This function encapsulates the creation of URLs, ARIA labels, and HTML structure for landmark links, improving code reuse and readability.admin/class-ajax.phpwith calls to the newedac_generate_landmark_linkfunction, reducing code duplication and improving maintainability.Testing:
GenerateLandmarkLinkTest.phpto test various scenarios for theedac_generate_landmark_linkfunction, including cases with and without selectors, special characters, custom CSS classes, and nonce verification. This ensures the function behaves correctly under diverse conditions.Styling Update:
margin-leftproperty to thebodyelement insrc/frontendHighlighterApp/sass/app.scssto enhance layout spacing.Summary by CodeRabbit