533 create the backend for tours #545
Conversation
WalkthroughThe changes introduce a new Sequelize migration and model for handling a Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as Validator Middleware
participant V as validateUrl Function
participant S as Server Controller
participant DB as Database (Sequelize)
C->>M: Sends tour configuration request
M->>V: Validate URL field
V-->>M: Returns valid/invalid result
alt URL Valid
M->>S: Forwards validated request
S->>DB: Executes DB operation (create/update tour/tour_popup)
DB-->>S: Returns operation result
S-->>C: Sends success response
else URL Invalid
M-->>C: Returns error response
end
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 12
🔭 Outside diff range comments (4)
frontend/src/components/Links/Settings/Settings.jsx (1)
97-105:⚠️ Potential issueMom's spaghetti moment: we're not using Formik values in onSubmit!
The onSubmit handler is using the local
stateinstead of Formik'svalues. This bypasses Formik's form state management and validation, potentially leading to inconsistencies.Fix this by using Formik's values:
- await handleClose(null, state); + await handleClose(null, values);backend/src/service/helperLink.service.js (1)
77-80:⚠️ Potential issuePotential transaction leak in early return
The transaction is committed before returning null, but this might be premature if subsequent operations fail.
- t.commit(); return null; + await t.rollback(); + return null;frontend/src/scenes/links/NewLinksPopup.jsx (1)
116-124: 🛠️ Refactor suggestionYo, this error handling needs some love!
The error handling could be more robust by adding specific error types and messages.
if (err instanceof AxiosError) { - msg = err.response.data.errors[0]; + msg = err.response?.data?.errors?.[0] ?? 'Network error occurred'; } else { - msg = err; + msg = err?.message ?? 'An unexpected error occurred'; }frontend/src/scenes/links/LinkPageComponents/LinkAppearance.jsx (1)
178-180:⚠️ Potential issueYo, these PropTypes are outdated!
The handleSaveHelper prop is still required in PropTypes but has been removed from the component parameters.
-LinkAppearance.propTypes = { - handleSaveHelper: PropTypes.func.isRequired, -};
🧹 Nitpick comments (29)
backend/src/service/tour.service.js (1)
43-56:getIncompleteTourByUrl
Filtering out IDs is a nice approach. Just keep an eye on performance if the list of IDs grows to a massive plate.frontend/src/scenes/popup/PopupPageComponents/PopupAppearance/PopupAppearance.jsx (3)
7-7: Double-check the spelling ofapperanceSchema.
Might be a small slip, much like mixing up “mom’s spaghetti” with “mom’s confetti.” Consider renaming toappearanceSchemafor clarity.
20-36: Handle error feedback in thetry/catchblock.
Right now the catch is empty, leaving no trace if your save fails. Adding logs or user feedback can help ensure your code doesn’t freeze up like stage fright.
47-74: Solid pattern for managing color fields.
If your data array grows, consider more modular solutions to keep the sauce from spilling.backend/migrations/0012-1.1-tour.js (2)
1-1: Remove the redundant'use strict'directive.
This file is already in strict mode, so you can drop it like a hot mic.- 'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
109-109: Correct the comment referencing “guide_logs”.
The code drops thetourstable, so align the comment, or else it’s like mixing up tracks on stage.- // Drop the guide_logs table + // Drop the tours tablebackend/src/test/mocks/tour.mock.js (1)
91-159: Comprehensive invalid property methods.
They’ll help you stress-test your logic like you’re prepping for a rap battle. Consider dynamic generation for further coverage.frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx (2)
30-38: Pass form values to onSave for more flexibility.Knees weak: consider passing Formik’s values directly to onSave() so the external logic can operate with precise data. This helps prevent partial updates and ensures your spaghetti doesn’t get tangled mid-dinner.
83-99: Consolidate setPopupContent calls for simpler logic.When juggling multiple calls, your code can slip through your fingers like sauced noodles. Restrict to one authoritative state update if possible, reducing complexity and mess.
backend/src/models/TourPopup.js (1)
32-35: Add index onorderfield for performanceYo! The
orderfield's gonna be queried frequently to display tour steps in sequence. Let's make it fast like Rap God!order: { type: DataTypes.INTEGER, allowNull: false, + index: true, },backend/migrations/0011-1.1-helperlink.js (2)
1-1: Drop that 'use strict' like it's hot! 🎤The 'use strict' directive is redundant in modules as they are automatically in strict mode.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
49-51: Yo, we're losing those error deets! 📝The catch block swallows error details, making it harder to debug issues.
- } catch { + } catch (error) { + console.error('Migration failed:', error); await transaction.rollback(); + throw error; }frontend/src/utils/popupHelper.js (1)
38-38: Yo, there's a typo in your beats! 🎵The schema name has a typo: "apperanceSchema" should be "appearanceSchema"
-export const apperanceSchema = Yup.object().shape({ +export const appearanceSchema = Yup.object().shape({backend/migrations/0013-1.1-tour-popup.js (1)
1-1: Yo, that 'use strict' is looking kinda sus!Since we're in a module, 'use strict' is automatically enabled. Let's drop it like it's hot!
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
backend/src/utils/tour.helper.js (1)
10-17: URL validation needs some extra sauce!The current URL validation might miss some edge cases. Consider adding:
- Protocol validation for absolute URLs
- Path validation for relative URLs
const validateUrl = (url) => { try { - new URL(url); + const urlObj = new URL(url, 'http://example.com'); + return urlObj.protocol === 'http:' || urlObj.protocol === 'https:'; return true; } catch { - return RELATIVE_URL_REGEX.test(url); + return url.startsWith('/') && RELATIVE_URL_REGEX.test(url); } };frontend/src/services/linksProvider.jsx (1)
5-13: Consider making the default URL more flexibleThe default URL 'https://' might be too restrictive when absolutePath is false. Consider using an empty string as the default and validating the URL format based on the absolutePath flag.
- url: 'https://', + url: '',backend/src/service/helperLink.service.js (1)
92-96: Enhance error logging for better debuggingThe current error handling loses valuable context. Consider including more details in the error message.
- console.log(e); + console.error('Helper update failed:', { helperId: id, error: e.message }); await t.rollback(); - throw new Error('Error updating helper'); + throw new Error(`Error updating helper: ${e.message}`);frontend/src/services/helperLinkService.js (1)
58-60: Remove redundant error loggingThere's duplicate error logging which adds noise to the console.
- console.log(error.message); console.error('Create helper link error:', error.message);backend/src/controllers/guide.controller.js (1)
54-55: Remove commented code and update implementationThere's commented out code that should either be removed or implemented properly.
- //const helperLinkIds = completeHelperLogs.map(log => log.guideId); const tourIds = completeTourLogs.map((log) => log.guideId); - //helperLinkService.getIncompleteHelpers(helperLinkIds), helperLinkService.getAllHelpersWithLinks(),Also applies to: 61-62
backend/src/service/guidelog.service.js (1)
85-97: Yo dawg, let's add some input validation!Consider adding validation for the userId parameter to prevent potential issues with invalid inputs.
async getCompleteToursLogs(userId) { + if (!userId || typeof userId !== 'number') { + throw new Error('Invalid userId provided'); + } try { return await GuideLog.findAll({frontend/src/products/Popup/PopupComponent.jsx (1)
42-50: Check these button actions, fam!The string literals in handleButtonClick should be constants to prevent typos and maintain consistency.
+const BUTTON_ACTIONS = { + CLOSE: 'close the popup', + OPEN_URL: 'open url', + OPEN_URL_NEW: 'open url in new page', +}; const handleButtonClick = () => { - if (buttonAction === 'close the popup') { + if (buttonAction === BUTTON_ACTIONS.CLOSE) {frontend/src/scenes/links/LinkPageComponents/LinkAppearance.jsx (1)
68-91: Add URL validation pattern, homie!Consider adding a URL pattern validation to ensure valid URLs are entered.
<input type="text" id="url" + pattern="^(\/|https?:\/\/).*" + title="URL must start with '/' or 'http(s)://'" className={`${styles.appearance__input} ${ errors.url && styles.error }`}frontend/src/scenes/popup/CreatePopupPage.jsx (2)
81-84: Yo, let's level up that error handling!Remove the console.log and enhance the error handling to provide more context about what went wrong.
- console.log({ error }); + // Log to monitoring service in production + const errorMessage = `Failed to fetch popup data: ${error.message}`; emitToastError(error);
204-210: Props documentation needs some sauce!Add JSDoc comments to document the purpose and expected values of each prop.
+/** + * @param {Object} props + * @param {boolean} [props.autoOpen=false] - Whether to automatically open the dialog + * @param {boolean} props.isEdit - Whether the popup is being edited + * @param {(string|number)} props.itemId - The ID of the popup being edited + * @param {Function} props.setItemsUpdated - Callback to update items state + * @param {Function} props.setIsEdit - Callback to update edit state + */ CreatePopupPage.propTypes = { autoOpen: PropTypes.bool, isEdit: PropTypes.bool, itemId: PropTypes.oneOfType([PropTypes.string, PropTypes.number]), setItemsUpdated: PropTypes.func, setIsEdit: PropTypes.func.isRequired, };backend/src/test/unit/services/tour.test.js (1)
53-63: These test cases need more flavor!The error handling test could be more specific about the expected error scenarios.
- it('createTour - should throw an error if the tour is not created', async () => { + it('createTour - should rollback transaction and throw error when tour creation fails', async () => { const newTour = tour(1).build(); - TourMock.create = sinon.stub(Tour, 'create').rejects(); + const expectedError = new Error('Database connection failed'); + TourMock.create = sinon.stub(Tour, 'create').rejects(expectedError); try { await tourService.createTour(newTour); } catch (error) { - expect(error.message).to.be.equal('Error'); + expect(error.message).to.be.equal(expectedError.message); } expect(TourMock.create.called).to.be.true; expect(rollback.called).to.be.true; });backend/src/test/unit/services/helperLink.test.js (1)
31-44: Clean up those commented specs, fam!Remove or implement the commented-out test expectations. If they're valuable test cases, they should be uncommented and fixed; otherwise, they should be removed to maintain code cleanliness.
Also applies to: 54-70, 223-241
frontend/src/tests/scenes/links/NewLinksPopup.test.jsx (1)
176-183: Verify test coverage for new input fields.The test now expects 7 input fields in the appearance form, with new properties like 'url', 'absolutePath', and 'active'. However, there are no test cases verifying the behavior of these new fields.
Consider adding test cases to verify:
- URL validation
- Absolute path toggle behavior
- Active state changes
Would you like me to help generate additional test cases for these new fields?
backend/src/test/e2e/tour.test.mjs (2)
162-209: Thorough validation for step properties.Comprehensive test coverage for step properties:
- title validation
- description validation
- targetElement validation
- order validation
However, there's no test case for duplicate order values in steps.
Consider adding a test case to verify that steps with duplicate order values are handled appropriately.
Would you like me to help generate a test case for duplicate order validation?
210-221: Verify tour creation with steps.The test case successfully verifies tour creation with steps, but it only tests with a single step.
Consider adding test cases for:
- Multiple steps
- Maximum number of allowed steps
- Step order sequence validation
Would you like me to help generate these additional test cases?
🛑 Comments failed to post (12)
frontend/src/components/Links/Settings/Settings.jsx (1)
144-147: 🛠️ Refactor suggestion
Knees weak, arms are heavy with this dual state management!
The component maintains two sources of truth by syncing both Formik state and local state in each onChange handler. This is repetitive and could lead to synchronization issues.
Consider using only Formik's state management. If you need to access form values outside of Formik's scope, use the
valuesprop provided by Formik's render prop function. Here's a cleaner approach:- onChange={(e) => { - handleChange(e); - setState((prev) => ({ ...prev, id: e.target.value })); - }} + onChange={handleChange}Apply similar changes to all input handlers. If you need the current form values, use them directly from Formik:
const currentValues = values; // from Formik's render propsAlso applies to: 161-164, 188-191, 215-221
backend/src/models/Tour.js (1)
98-102:
⚠️ Potential issuePossible mismatch in the foreign key naming.
foreignKey: 'tourPopupId'might typically betourIdfor a hasMany relationship, unless there’s a compelling reason otherwise. Double-check so you don’t end up losing yourself in the DB references.- foreignKey: 'tourPopupId', + foreignKey: 'tourId',📝 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.Tour.hasMany(models.TourPopup, { - foreignKey: 'tourPopupId', + foreignKey: 'tourId', as: 'steps', onDelete: 'CASCADE', });frontend/src/scenes/popup/PopupPageComponents/PopupContent/PopupContent.jsx (1)
157-159: 🛠️ Refactor suggestion
Remove deprecated propType references.
There’s vomit on your sweater already if devs see an unused propType. “buttonAction” appears unreferenced here, so clearing it out will keep your code as clean as freshly wiped counters.
frontend/src/utils/guideHelper.js (1)
6-6: 🛠️ Refactor suggestion
Add a defensive check for error array existence.
Arms are heavy if error.response.data.errors is undefined: referencing .length will explode like overcooked spaghetti. Wrap in a condition to avoid TypeError meltdowns.
- for (let i = 0; i < error.response.data.errors.length; i++) { + const errs = error.response?.data?.errors; + if (Array.isArray(errs)) { + for (let i = 0; i < errs.length; i++){ + toastEmitter.emit(TOAST_EMITTER_KEY, 'An error occurred: ' + errs[i].msg); + } + }Committable suggestion skipped: line range outside the PR's diff.
backend/src/routes/tour.routes.js (1)
12-12:
⚠️ Potential issueAdd rate limiting to public endpoint
/get_tour_by_urlYo! This endpoint is exposed without authentication and could be vulnerable to abuse. We should protect it like mom's spaghetti - don't want it getting cold!
Add rate limiting middleware:
-router.get('/get_tour_by_url', bodyUrlValidator, handleValidationErrors, tourController.getTourByUrl); +router.get('/get_tour_by_url', + rateLimit({ windowMs: 15 * 60 * 1000, max: 100 }), + bodyUrlValidator, + handleValidationErrors, + tourController.getTourByUrl +);📝 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.router.get('/get_tour_by_url', rateLimit({ windowMs: 15 * 60 * 1000, max: 100 }), bodyUrlValidator, handleValidationErrors, tourController.getTourByUrl );frontend/src/utils/linkHelper.js (1)
34-37: 🛠️ Refactor suggestion
Add URL sanitization before validation
The URL validation is tight, but we should sanitize it first - don't want any sus characters like mom's spaghetti stains!
url: Yup.string() .required('URL is required') + .transform(value => value.trim().toLowerCase()) .test('url', 'Invalid value for URL', validateUrl) .max(2000, 'URL must be at most 2000 characters'),📝 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.url: Yup.string() .required('URL is required') .transform(value => value.trim().toLowerCase()) .test('url', 'Invalid value for URL', validateUrl) .max(2000, 'URL must be at most 2000 characters'),frontend/src/utils/popupHelper.js (1)
6-13: 🛠️ Refactor suggestion
URL validation needs more sauce! 🌶️
The current URL validation might allow malicious URLs. Consider adding protocol and domain restrictions.
const validateUrl = (url) => { try { - new URL(url); + const urlObj = new URL(url); + const allowedProtocols = ['http:', 'https:']; + if (!allowedProtocols.includes(urlObj.protocol)) { + return false; + } return true; } catch { return RELATIVE_URL_REGEX.test(url); } };📝 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.const validateUrl = (url) => { try { const urlObj = new URL(url); const allowedProtocols = ['http:', 'https:']; if (!allowedProtocols.includes(urlObj.protocol)) { return false; } return true; } catch { return RELATIVE_URL_REGEX.test(url); } };backend/src/models/HelperLink.js (1)
64-73: 🛠️ Refactor suggestion
URL validation looking a bit shaky!
The URL validation could throw errors silently. Let's make it more explicit:
validate: { customValidation(value) { - return validateUrl(value); + if (!validateUrl(value)) { + throw new Error('Invalid URL format'); + } }, },📝 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.url: { type: DataTypes.STRING(255), allowNull: false, validate: { customValidation(value) { if (!validateUrl(value)) { throw new Error('Invalid URL format'); } }, }, defaultValue: '/', },frontend/src/services/helperLinkService.js (1)
23-39: 🛠️ Refactor suggestion
Enhance validation for URL and active fields
The validation function should check the new required fields and validate URL format.
const validateHelper = (helper, links) => { + const urlPattern = /^(https?:\/\/|\/)/; if (!helper?.title) { throw new Error('Header is required'); } + if (helper?.url && !urlPattern.test(helper.url)) { + throw new Error('URL must start with http://, https://, or /'); + } // ... rest of validation };📝 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.const validateHelper = (helper, links) => { const urlPattern = /^(https?:\/\/|\/)/; if (!helper?.title) { throw new Error('Header is required'); } if (helper?.url && !urlPattern.test(helper.url)) { throw new Error('URL must start with http://, https://, or /'); } if (!helper?.headerBackgroundColor) { throw new Error('Header background color is required'); } if (!helper?.linkFontColor) { throw new Error('Link font color is required'); } if (!helper?.iconColor) { throw new Error('Icon color is required'); } if (links.some((it) => !it?.title || !it?.url)) { throw new Error('Title and URL are required'); } };backend/src/controllers/guide.controller.js (1)
67-68:
⚠️ Potential issueFix missing error response in getIncompleteGuidesByUrl
The error handling is incomplete and doesn't send a response to the client.
- internalServerError('GET_INCOMPLETE_GUIDES_BY_URL_ERROR', error.message); + const { payload, statusCode } = internalServerError('GET_INCOMPLETE_GUIDES_BY_URL_ERROR', error.message); + res.status(statusCode).json(payload);📝 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.const { payload, statusCode } = internalServerError('GET_INCOMPLETE_GUIDES_BY_URL_ERROR', error.message); res.status(statusCode).json(payload); }frontend/src/products/Popup/PopupComponent.jsx (1)
97-110: 🛠️ Refactor suggestion
Yo, these PropTypes are fire but need more validation!
The buttonAction prop should use an enum of allowed values to prevent potential runtime errors.
- buttonAction: PropTypes.string, + buttonAction: PropTypes.oneOf(['close the popup', 'open url', 'open url in new page']),📝 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.PopupComponent.propTypes = { header: PropTypes.string, content: PropTypes.string.isRequired, previewBtnText: PropTypes.string, headerBackgroundColor: PropTypes.string, headerColor: PropTypes.string, textColor: PropTypes.string, buttonBackgroundColor: PropTypes.string, buttonTextColor: PropTypes.string, buttonAction: PropTypes.oneOf(['close the popup', 'open url', 'open url in new page']), popupSize: PropTypes.string, actionButtonUrl: PropTypes.string, isReal: PropTypes.bool, };backend/postman/Tours.postman_collection.json (1)
137-146:
⚠️ Potential issueCritical JSON formatting issue in the edit tour request!
There’s an unexpected artifact—{buttonBackgroundColor—inserted in the bearer token block on line 142. This is breaking the JSON syntax. Please remove this extraneous text so that the authentication object conforms to proper JSON formatting.Suggested Diff:
- {buttonBackgroundColor - "key": "token", + { + "key": "token",📝 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."name": "edit tour", "request": { "auth": { "type": "bearer", "bearer": [ { "key": "token", "value": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpZCI6MywiZW1haWwiOiJkZWJvcmEuci5zZXJyYUBnbWFpbC5jb20iLCJpYXQiOjE3Mzg5NDcxMjIsImV4cCI6MTczODk1MDcyMn0.wvFH4YmJ1JfqNU6-qskdLMGQJzI6p2Yc5uEhuf7wVHI", "type": "string" }🧰 Tools
🪛 Biome (1.9.4)
[error] 142-142: Property key must be double quoted
(parse)
[error] 143-143: expected
:but instead found"key"Remove "key"
(parse)
[error] 143-143: expected
,but instead found:Remove :
(parse)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/test/mocks/tour.mock.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (1)
backend/src/test/mocks/tour.mock.js (1)
167-177: This test data generation is straight fire! 🔥The way you're generating test data is consistent and follows the established pattern. The array mapping with alternating
createdByproperty is clean like Eminem's verses.
Describe your changes
After the meeting, I realized the tour popup was missing a field. This PR is meant to add this field
Issue number
#533
Please ensure all items are checked off before requesting a review: