Refactor Popup Validations #548
Conversation
Develop -> master
Develop -> Master
WalkthroughThis pull request updates documentation and environment settings while refactoring popup validation. The README now includes a demo link, updated installation instructions, and revised project names. The backend’s environment file replaces old database credentials with new ones. Additionally, the popup controller has been simplified by removing in-method validations, with corresponding validation middleware now added in the routes and new validation logic introduced in the popup helper. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant Validation
participant Controller
participant Database
Client->>Router: Sends popup request
Router->>Validation: Execute validation middleware
Validation-->>Router: Return validation result
Router->>Controller: Forward validated request
Controller->>Database: Process popup operation
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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
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: 2
🧹 Nitpick comments (4)
backend/src/utils/popup.helper.js (2)
24-35: Enhance URL validation logicThe current URL validation could be improved to handle edge cases and provide better error messages.
Consider this improved implementation:
- .custom((value, {req}) => { - const needsUrl = ['open url', 'open url in a new tab'].includes(req.body.closeButtonAction); - return !needsUrl || (needsUrl && value) - }) + .custom((value, {req}) => { + const needsUrl = ['open url', 'open url in a new tab'].includes(req.body.closeButtonAction); + if (needsUrl && !value?.trim()) { + throw new Error('URL is required when close button action is set to open URL'); + } + return true; + }) - .withMessage('URL is required when close button action is set to open URL') - .bail() - .custom((value) => { - if (!value) return true; - const url = new URL(value) - return ['http:', 'https:'].includes(url.protocol) || value.startsWith('/'); - }) + .bail() + .custom((value) => { + if (!value?.trim()) return true; + try { + const url = new URL(value.startsWith('/') ? `http://dummy.com${value}` : value); + return ['http:', 'https:'].includes(url.protocol) || value.startsWith('/'); + } catch (error) { + throw new Error('Invalid URL format'); + } + })
37-46: Improve error handling in actionUrl validationThe current implementation swallows the specific error message from the URL constructor.
Consider this enhanced version:
- .custom((value) => { - if(!value) return true; - try { - const url = new URL(value); - return ['http:', 'https:'].includes(url.protocol) - }catch { - return false - } - }) + .custom((value) => { + if (!value?.trim()) return true; + try { + const url = new URL(value); + if (!['http:', 'https:'].includes(url.protocol)) { + throw new Error('URL must use HTTP or HTTPS protocol'); + } + return true; + } catch (error) { + throw new Error(error.message || 'Invalid URL format'); + } + })README.md (2)
275-275: Fix grammar in DataHall descriptionThere's a grammatical error in the article usage.
-[DataHall](https://github.com/bluewave-labs/datahall), an secure file sharing application, aka dataroom. +[DataHall](https://github.com/bluewave-labs/datahall), a secure file sharing application, aka dataroom.🧰 Tools
🪛 LanguageTool
[misspelling] ~275-~275: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...s://github.com/bluewave-labs/datahall), an secure file sharing application, aka da...(EN_A_VS_AN)
242-252: Add language identifier to code blockThe code block is missing a language identifier.
-``` +```javascript window.bwApiBaseUrl = 'https://guidefox-demo.bluewavelabs.ca/api/';🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
242-242: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(5 hunks)backend/.env(1 hunks)backend/src/controllers/popup.controller.js(0 hunks)backend/src/routes/popup.routes.js(1 hunks)backend/src/utils/popup.helper.js(2 hunks)
💤 Files with no reviewable changes (1)
- backend/src/controllers/popup.controller.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
242-242: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 LanguageTool
README.md
[misspelling] ~275-~275: Use “a” instead of ‘an’ if the following word doesn’t start with a vowel sound, e.g. ‘a sentence’, ‘a university’.
Context: ...s://github.com/bluewave-labs/datahall), an secure file sharing application, aka da...
(EN_A_VS_AN)
|
This PR has been closed due to issues with the forked repo branch. A new PR addressing the same issue has been raised and is under review. |
Refactored Popup Validations in Backend
Updated the exisiting popup validations in popup.controller.js file using express validator and placed them in popup.helper.js file.
This PR addresses the issue #387
Below is a summary of all the changes:
Added
add_popup, delete_popup/:id, edit_popup/:id, get_popup/:id, get_popup_by_url
Removed
Existing functions in popup.helper.js file such as validatePopupSize, validatePopupSizeWrapper, validateRelativeUrl, validateUrl have been left as is since they might be useful in another implementations.