pass in empty string if no resources are defined#910
Conversation
WalkthroughThe update introduces a conditional check in the Changes
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 (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/middleware/entryIssueDetails.middleware.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: run-tests / test
GeorgeGoodall-GovUk
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/views/check/results/shareResults.html (3)
36-55: Review ofcopyShareLinkFunction Implementation
The implementation correctly uses the Clipboard API with a fallback to ensure robust copy functionality. One suggestion is to validate that the button element selected bydocument.querySelector('.app-c-button--secondary-quiet')is notnullbefore accessing its properties (e.g.innerText). This extra check will safeguard against potential runtime errors in cases where the button element might not exist in the DOM for any reason.Suggested diff:
- const button = document.querySelector('.app-c-button--secondary-quiet'); - const originalText = button.innerText; + const button = document.querySelector('.app-c-button--secondary-quiet'); + if (!button) { + console.error("Button element for copying share link was not found."); + return; + } + const originalText = button.innerText;
57-92: Review offallbackCopyTextFunction Implementation
The fallback function is robust in its strategy for copying text via a temporary textarea element and includes well-managed error handling. As a minor refinement, consider utilising the moderntextArea.remove()method instead ofdocument.body.removeChild(textArea)for cleaning up the element. This can improve code readability and aligns with modern browser APIs, provided that browser compatibility is ensured.Optional diff:
- document.body.removeChild(textArea); + textArea.remove();
94-99: Review ofupdateButtonTextHelper Function
This helper function is clearly implemented and effectively provides user feedback by updating the button text temporarily. In a similar vein to the earlier suggestion, you might consider adding a safeguard check to confirm that thebuttonelement is defined before modifying itsinnerText, thereby preventing potential runtime errors if the function is inadvertently called with an undefined button.Optional safeguard addition:
function updateButtonText(temporaryText, originalText, button) { - button.innerText = temporaryText; + if (!button) return; + button.innerText = temporaryText; setTimeout(() => { - button.innerText = originalText; + if(button) { button.innerText = originalText; } }, 2000); }
This reverts commit 50f539c.
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
| from issue i | ||
| LEFT JOIN issue_type it ON i.issue_type = it.issue_type | ||
| WHERE resource = '${req.resources[0].resource}' | ||
| WHERE resource = '${req.resources[0]?.resource || ''}' |
There was a problem hiding this comment.
Actually this will return all resources with empty - we probably want to return 0 values
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
DilwoarH
deleted the
763-bug-cannot-read-properties-of-undefined-reading-resource
branch
5 participants
Preview Link
https://submit-pr-910.herokuapp.com/
What type of PR is this? (check all applicable)
Description
This PR addresses a potential issue with accessing the
resourceproperty ofreq.resources[0]. The change ensures that ifreq.resources[0]is undefined, the query will not break by providing a default empty string. This improves the robustness of the SQL query in thefetchIssueCountfunction.if no resources are defined, the middleware chain will go on to find now issues and therefor show a 404 page instead of a generic error page
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
navigating to here: https://submit-pr-910.herokuapp.com/organisations/local-authority:BUC/tree-preservation-order/reference%20values%20are%20not%20unique/reference/entry/368
now shows a 404 instead of a general error
Added/updated tests?
QA sign off
Summary by CodeRabbit