533 create the backend for tours #534
Conversation
WalkthroughThis pull request overhauls the tour functionality by modifying the tour configuration, database models, migrations, API routes, controllers, and services. It removes several legacy fields while introducing new styling and size attributes. Additionally, new database tables and associations (for tour popups) are created, and comprehensive validation rules are incorporated into both the API and utility layers. The test suites have been updated accordingly to reflect these changes, ensuring better transaction handling and request payload validation. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as Routes
participant C as TourController
participant S as TourService
participant DB as Database
U->>R: POST /api/tour/add_tour
R->>C: Forward request with tourValidator applied
C->>S: createTour(data, userId)
S->>DB: Begin transaction
S->>DB: Insert Tour record and validate fields
S->>DB: Bulk create TourPopup records (if steps provided)
DB-->>S: Return inserted records
S->>DB: Commit transaction
S-->>C: Return tour object with associated popups
C-->>R: Send success response
R-->>U: HTTP 200 with tour details
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: 4
🔭 Outside diff range comments (1)
backend/src/test/e2e/tour.test.mjs (1)
32-222: Add test for concurrent tour creation.The end-to-end tests should verify that concurrent tour creation works correctly.
+ it('should handle concurrent tour creation correctly', async () => { + const newSteps = [tourPopup().build()]; + const newTour = { ...tour().withoutId().build(), steps: newSteps }; + const promises = Array(5).fill(null).map(() => + chai.request + .execute(app) + .post('/api/tour/add_tour') + .set('Authorization', `Bearer ${token}`) + .send(newTour) + ); + const responses = await Promise.all(promises); + responses.forEach(res => { + expect(res).to.have.status(201); + expect(res.body).to.have.property('id'); + }); + const ids = responses.map(res => res.body.id); + const uniqueIds = new Set(ids); + expect(uniqueIds.size).to.equal(promises.length); + });
🧹 Nitpick comments (14)
backend/src/service/tour.service.js (1)
24-44: Transactional integrity is top-notchLike mom's spaghetti, it’s nourishing to see createTour wrapped in a transaction. The approach prevents partial data from messing up your database. As a refinement, consider validating whether steps is an array to avoid potential crashes.
backend/src/models/Tour.js (2)
44-53: buttonTextColor adheres to accessibilityIt’s bright as a spotlight. With a well-validated default, folks won’t have to second-guess. Checking contrast might be good in future enhancements.
54-58: Note about size field as an ENUMThough it’s well-structured, changes to the size array in settings might be slow to update. If your code is as unstoppable as a rap verse, consider a more flexible approach if new sizes roll in frequently.
backend/migrations/0012-1.1-tour.js (2)
1-1: Trim the fat on 'use strict'
In modern Node, modules are already strict by default. Removing this line might help keep the code base lean and free from redundancy.-'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: Update the misleading comment
The comment mentions dropping the “guide_logs” table, which seems off. Please align the comment with the actual table name "tours" to avoid confusion.- // Drop the guide_logs table + // Drop the tours tablebackend/src/routes/tour.routes.js (1)
16-24: Unify the route naming and structure
Though everything is functioning, consistent REST naming (e.g. GET/POST/PUT/DELETE on /tours/:id) can help keep your arms from feeling too heavy later on. Consolidating endpoints can improve clarity and maintainability.backend/src/models/TourPopup.js (1)
47-53: Consider tracking creation and modification times
Currently, timestamps are disabled. If you foresee a future need to log when a popup was created or updated, setting timestamps to true might be beneficial. Otherwise, you’re good to go!backend/src/utils/tour.helper.js (1)
19-33: These validation rules are spittin' straight facts! 🎤Comprehensive validation rules for all tour properties. However, consider adding:
- Maximum length validation for strings
- Range validation for integers
const tourValidator = [ body('headerColor').optional().custom(isValidHexColor).withMessage('Invalid value for headerColor'), body('textColor').optional().custom(isValidHexColor).withMessage('Invalid value for textColor'), body('buttonBgColor').optional().custom(isValidHexColor).withMessage('Invalid value for buttonBgColor'), body('buttonTextColor').optional().custom(isValidHexColor).withMessage('Invalid value for buttonTextColor'), - body('url').isString().withMessage("url is required").custom(validateUrl).withMessage('Invalid value for url'), + body('url').isString().withMessage("url is required").isLength({ max: 2048 }).custom(validateUrl).withMessage('Invalid value for url'), body('size').isIn(settings.tour.size).withMessage('Invalid value for size'), - body('finalBtnText').isString().withMessage('Invalid value for finalBtnText'), + body('finalBtnText').isString().isLength({ max: 100 }).withMessage('Invalid value for finalBtnText'), body('active').optional().isBoolean().withMessage('Invalid value for active'), body('steps').isArray().withMessage('steps must be an array'), - body('steps.*.title').isString().withMessage('Invalid value for title'), + body('steps.*.title').isString().isLength({ max: 255 }).withMessage('Invalid value for title'), - body('steps.*.description').isString().withMessage('Invalid value for description'), + body('steps.*.description').isString().isLength({ max: 1000 }).withMessage('Invalid value for description'), - body('steps.*.targetElement').isString().withMessage('Invalid value for targetElement'), + body('steps.*.targetElement').isString().isLength({ max: 255 }).withMessage('Invalid value for targetElement'), - body('steps.*.order').isInt().withMessage('Invalid value for order'), + body('steps.*.order').isInt({ min: 1 }).withMessage('Invalid value for order'), ];backend/migrations/0013-1.1-tour-popup.js (2)
1-2: Yo, that 'use strict' is like yesterday's leftovers! 🍝The 'use strict' directive is redundant in ES modules.
-'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)
7-56: This migration's transaction handling is clean like mom's spaghetti! 🍝Solid implementation with proper error handling and rollback. Consider adding indexes for frequently queried columns.
await queryInterface.createTable( TABLE_NAME, { // ... existing columns ... }, { transaction } ); + +// Add indexes for frequently queried columns +await queryInterface.addIndex( + TABLE_NAME, + ['tourId', 'order'], + { + name: 'idx_tour_popup_tour_order', + transaction + } +);backend/src/test/mocks/tour.mock.js (2)
67-164: Consider using an enum for the size property.The
sizeproperty inTourBuilderaccepts 'small' as a value, but there's no clear indication of other valid values.+ const TOUR_SIZES = { + SMALL: 'small', + MEDIUM: 'medium', + LARGE: 'large', + } as const; class TourBuilder { constructor(id) { this.tour = { id: id, headerColor: '#999', textColor: '#444', buttonBgColor: '#ff00ff', buttonTextColor: '#fff', url: '/url', - size: 'small', + size: TOUR_SIZES.SMALL, finalBtnText: 'text', active: true, }; }
166-176: Consider adding validation for the steps array length.The
toursListarray creates 3 steps for each tour without any validation for minimum or maximum steps.const toursList = new Array(10) .fill(null) .map((_, i) => TourBuilder.tour(i + 1).build()) .map((tour, i) => { + const MIN_STEPS = 1; + const MAX_STEPS = 5; + const numSteps = Math.min(Math.max(3, MIN_STEPS), MAX_STEPS); - const steps = new Array(3).fill(null).map((_, j) => TourPopupBuilder.tourPopup(j + 1, tour.id).build()); + const steps = new Array(numSteps).fill(null).map((_, j) => TourPopupBuilder.tourPopup(j + 1, tour.id).build()); tour.steps = steps;backend/src/test/unit/services/tour.test.js (1)
13-26: Consider using a more descriptive name for the transaction mock.The transaction mock could be more descriptive to indicate its purpose in testing.
beforeEach(() => { commit = sinon.spy(); rollback = sinon.spy(); - sinon.stub(db.sequelize, 'transaction').callsFake(async () => ({ + const mockTransaction = { + commit, + rollback, + }; + sinon.stub(db.sequelize, 'transaction').resolves(mockTransaction); - commit, - rollback, - })); });backend/src/test/e2e/tour.test.mjs (1)
10-21: Consider using a test data factory.The test setup could benefit from a dedicated test data factory to reduce duplication.
+ const createTestData = (overrides = {}) => ({ + token: null, + tour: null, + ...overrides, + }); const dbReadyOptions = { resources: ['tcp:localhost:5432'], delay: 1000, timeout: 30000, interval: 1000, };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/config/settings.js(1 hunks)backend/migrations/0012-1.1-tour.js(1 hunks)backend/migrations/0013-1.1-tour-popup.js(1 hunks)backend/postman/Tours.postman_collection.json(6 hunks)backend/src/controllers/tour.controller.js(1 hunks)backend/src/models/Tour.js(3 hunks)backend/src/models/TourPopup.js(1 hunks)backend/src/models/index.js(2 hunks)backend/src/routes/tour.routes.js(1 hunks)backend/src/service/tour.service.js(1 hunks)backend/src/test/e2e/tour.test.mjs(7 hunks)backend/src/test/mocks/tour.mock.js(3 hunks)backend/src/test/unit/controllers/tour.test.js(1 hunks)backend/src/test/unit/services/tour.test.js(1 hunks)backend/src/utils/tour.helper.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
backend/migrations/0013-1.1-tour-popup.js
[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/migrations/0012-1.1-tour.js
[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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (30)
backend/src/service/tour.service.js (6)
1-2: Solid introduction for the DB connectionDespite the spaghetti-like complexities of large codebases, these lines are well-seasoned. The import for the database models is conventional and neat.
4-4: Appropriate import of TourPopupPalms are sweaty, but this model import is stable. You’ve introduced TourPopup precisely—ensuring the relationship with Tour remains in sync.
11-13: Ensure null results are handled gracefullyArms feel heavy, but the getTourById method stands strong. Consider verifying that callers handle null responses properly and give 404 or a helpful error.
15-15: getTourByUserId is clear and functionalKnees might be weak in some code, but here it’s rock-solid. This method is straightforward—just be consistent with the naming convention in the rest of the codebase.
47-72: updateTour method demonstrates robust error handlingDespite the tension, your usage of transactions ensures consistency. Good job discarding old Popups before creating new ones. Keep the momentum, but consider concurrency checks if multiple users might edit the same tour simultaneously.
74-88: deleteTour logic is cohesiveThis method is simpler than memorizing rap lyrics—destroying related rows first is the right call. Transaction usage is properly placed and the code’s easy to read.
backend/src/models/Tour.js (7)
2-3: Import of validation helpers looks correctMom’s spaghetti might be all about the sauce, but these imports are about strong validation. Hats off to you for referencing the utility libraries directly.
14-22: headerColor field—on-point validationEven when your palms get sweaty, letting isHexColor validate underscores your readiness for any hex code. Default value complements the brand colours.
24-33: textColor default ensures good readabilityDropping a safe hex like #344054 is clean. Keep that lyrical groove going by preserving consistent colour usage across your UI.
34-43: buttonBgColor to highlight CTA effectivelyThe purple (#7F56D9) is bold. Validation flows well, and it’s easy to tweak afterwards if your theme evolves.
59-63: finalBtnText field is cleanly implementedStraightforward with a default "Complete tour". The vow to brevity keeps your code simple like a good hook.
64-75: URL validation helps avoid misroutingLyrically, it’s crucial not to go off-track. This custom validation is beneficial—just ensure all valid domain formats are accounted for.
76-80: active field fosters easy togglingChannelling that energy, a boolean default of true is typical and simple to parse anywhere else in the code.
backend/src/controllers/tour.controller.js (7)
1-2: Controller imports maintain clarityNo sweaty confusion here—dependencies are loaded logically. The structure remains tidy and quick to comprehend.
5-10: getAllTours method is neatThis is your straightforward retrieval logic. If an error appears, you’re returning a well-defined status—your flow doesn’t choke under pressure.
15-30: getTourById method ensures user feedbackArms might be heavy, but your 404 response covers it. The error handling is consistent, giving confidence to front-end developers.
34-43: getToursByUserId fosters multi-user functionalityWe see you referencing req.user.id—just ensure your authentication middleware is rock-solid so that user data is correct.
45-53: createTour elegantly ties user contextCapturing createdBy from req.user ensures that ownership is tracked. Transaction coverage in the service complements this well.
56-73: updateTour method handles missing records gracefullyYour code checks for a null response and returns 404. The structure is easy to follow; you keep transactions in the service.
75-91: deleteTour method completes the cycle smoothlyThis final method returns a 404 if missing, or success if found. The code is as steady as a classic track. Great job.
backend/config/settings.js (1)
32-34: Yo, this tour size configuration is straight fire! 🔥The simplified configuration with predefined sizes makes it cleaner and more maintainable.
backend/src/models/index.js (1)
51-52: These associations are tight like a choreographed dance! 🕺Perfect bidirectional relationship between Tour and TourPopup models with clear aliases.
backend/src/test/mocks/tour.mock.js (1)
1-65: LGTM! The TourPopupBuilder implementation follows best practices.The builder pattern is well implemented with clear validation methods and a fluent interface.
backend/postman/Tours.postman_collection.json (7)
3-6: INFO Section IDs Updated:
The _postman_id and _exporter_id have been updated to "7544a42b-6230-4711-977e-fc455fb6a05d" and "21904853" respectively. Ensure these values match your current Postman export configuration—they’re as fresh as a new verse when your palms are sweaty!
17-17: Updated 'Fetch all Tours' Endpoint:
The bearer token value and URL have been updated to include the /api prefix. This change tightens up the endpoint path like a crisp beat. Make sure the token and routing align with your backend expectations.Also applies to: 25-32
40-70: New 'Fetch all Tours By user Id' Endpoint Added:
A fresh GET request for fetching tours by user ID has been incorporated, complete with the new /api prefix and updated token details. Ensure that the corresponding backend logic is harmonised with this change—no missteps like off-beat rhymes!
71-102: New 'Fetch Tour by Id' Endpoint Reviewed:
The endpoint to retrieve a tour by its ID is now set up with a correctly structured URL and updated authentication. Double-check that the backend correctly handles the provided tour ID parameter. Keep it smooth and steady, just like your best flow!
133-134: Updated 'New Tour' Request:
The POST request for creating a new tour now rocks an updated bearer token and a refreshed payload that includes new styling attributes like headerColor, textColor, and more. Ensure the payload structure fully lines up with your backend validations. This update is as crisp as your kick drum—make sure it drops perfectly!Also applies to: 142-142, 150-157
166-205: Refined 'Edit Tour' Endpoint:
The PUT request for editing a tour has been overhauled with an updated token, a refined request body (notably updating finalBtnText), and a revised URL path reflecting the new API structure. Verify that these changes are in sync with the backend’s expectations; you want your remix to hit all the right notes, without any slip-ups.
207-237: Streamlined 'Delete Tour' Endpoint:
The DELETE request has been updated with the new bearer token and a correctly formatted URL (with /api/tour/delete_tour/3). Confirm that the backend deletion logic handles this update gracefully—so nothing falls apart like misremembered lyrics on stage.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/src/test/unit/services/tour.test.js (1)
1-12: Yo dawg, these imports are straight fire but could use some organization! 🔥Consider grouping related imports together:
- Test framework imports (mocha, chai, sinon)
- Application imports (db, services)
- Test utilities (mocks)
const { describe, it, afterEach, beforeEach } = require('mocha'); const { expect } = require('chai'); const sinon = require('sinon'); + const db = require('../../../models/index.js'); -const mocks = require('../../mocks/tour.mock.js'); const tourService = require('../../../service/tour.service.js'); + +const mocks = require('../../mocks/tour.mock.js');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/test/unit/services/tour.test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (1)
backend/src/test/unit/services/tour.test.js (1)
40-52: Mom's spaghetti moment: We still need to validate those step orders! 🍝The test should verify that steps are created in the correct order, as mentioned in the previous review.
const createdTour = await tourService.createTour({ ...newTour, steps: newSteps }); expect(createdTour).to.be.deep.equal(newTour); expect(TourMock.create.called).to.be.true; expect(TourPopupMock.bulkCreate.called).to.be.true; const params = TourMock.create.getCall(0).args[0]; expect(params).to.be.deep.equal(newTour); expect(commit.called).to.be.true; +const bulkCreateParams = TourPopupMock.bulkCreate.getCall(0).args[0]; +expect(bulkCreateParams).to.be.deep.equal(newSteps); +expect(bulkCreateParams[0].order).to.be.equal(1); +expect(bulkCreateParams[1].order).to.be.equal(2);
There was a problem hiding this comment.
In another PR, can you also implement
get_tour_by_urlroute similar to popupgetIncompleteToursByUrlintour.serviceandgetCompleteToursinguidelog.serviceand add them togetGuidesByUrlandgetIncompleteGuidesByUrlinguide.controllerlike we did for other guides?
I'll create a new issue for this one
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
backend/src/utils/tour.helper.js (1)
19-36: Let's beef up that steps array validation! 💪While the validation is solid, we could make the steps array validation more robust.
- body('steps').isArray().withMessage('steps must be an array'), + body('steps') + .isArray() + .withMessage('steps must be an array') + .custom((value) => value.length > 0) + .withMessage('steps array cannot be empty'),backend/src/models/Tour.js (1)
64-75: Yo, let's add some constraints to that URL field! 📏The URL field could use a max length constraint to prevent super long URLs.
url: { type: DataTypes.STRING, + type: DataTypes.STRING(2048), allowNull: false, defaultValue: '/', validate: { customValidation(value) { if (!validateUrl(value)) { throw new Error('Invalid value for URL'); } }, }, },backend/migrations/0012-1.1-tour.js (2)
1-1: Yo, that 'use strict' is redundant in modules! 🚫JavaScript modules 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)
109-110: Fix that comment, it's referencing the wrong table! 📝The comment mentions 'guide_logs' but should reference 'tours'.
- // Drop the guide_logs table + // Drop the tours table await queryInterface.dropTable(TABLE_NAME, { transaction });backend/postman/Tours.postman_collection.json (1)
117-125: Mom's spaghetti... I mean, let's make this documentation better! 📝The request body structure is solid, but adding example descriptions would make it more user-friendly.
Add descriptions in the raw JSON:
{ + // Hex color code for the header "headerColor": "#999", + // Hex color code for the text "textColor": "#444", // ... more fields with descriptions "steps": [ + // Each step represents a tour popup { "title": "Step 1", // ... more fields with descriptions } ] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/migrations/0012-1.1-tour.js(1 hunks)backend/postman/Tours.postman_collection.json(1 hunks)backend/src/models/Tour.js(3 hunks)backend/src/test/e2e/tour.test.mjs(7 hunks)backend/src/test/mocks/tour.mock.js(3 hunks)backend/src/utils/tour.helper.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/test/e2e/tour.test.mjs
🧰 Additional context used
🪛 Biome (1.9.4)
backend/postman/Tours.postman_collection.json
[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)
backend/migrations/0012-1.1-tour.js
[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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (5)
backend/src/utils/tour.helper.js (2)
5-8: Yo dawg, let's secure that URL regex! 🔒The current URL regex is too permissive and could allow malicious URLs through. Let's tighten it up!
-const URL_REGEX = - /(?:https?:\/\/(?:www\.)?|www\.)?[a-zA-Z0-9@:%._+~#=]{2,256}\.[a-z]{2,6}\b([-\d@:%_+.~#?&//=]*)|(?:\/[a-zA-Z0-9@:%._+~#&//=]*)/gi; +const URL_REGEX = + /^(?:https?:\/\/(?:[\w-]+\.)+[\w-]+(?:\/[\w-./?%&=]*)?|\/[\w-./?%&=]+)$/i; -const RELATIVE_URL_REGEX = /^\/([a-zA-Z0-9_-]+\/?)+$/; +const RELATIVE_URL_REGEX = /^\/[\w-./]+$/;
10-17: LGTM! Clean URL validation implementation! 👊The two-step validation with URL constructor and regex fallback is solid.
backend/src/models/Tour.js (1)
98-102: Hold up! That foreign key name ain't right! 🤔The foreign key should be 'tourId' since it references the Tour model.
Tour.hasMany(models.TourPopup, { - foreignKey: 'tourPopupId', + foreignKey: 'tourId', as: 'steps', onDelete: 'CASCADE', });backend/postman/Tours.postman_collection.json (2)
2-7: Yo, the collection metadata is on point! 🎯The collection info is properly structured with all required fields.
170-194: Clean as mom's spaghetti! 🍝The DELETE endpoint is properly structured with the correct HTTP method and URL pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/test/mocks/tour.mock.js (3)
1-11: Yo! Add some parameter validation to keep it real! 🍝The constructor accepts parameters without validation. Consider adding checks for
idandtourIdto prevent invalid test data.constructor(id, tourId) { + if (typeof id !== 'number' || typeof tourId !== 'number') { + throw new Error('id and tourId must be numbers'); + } this.tourPopup = { id: id, title: `title ${id}`, description: `description ${id}`, targetElement: `.element${id}`, order: id, tourId, }; }
71-74: Mom's spaghetti! Add some color validation! 🎨Consider adding hex color validation to prevent invalid color codes.
+ const isValidHexColor = (color) => /^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$/.test(color); constructor(id) { this.tour = { id: id, - headerColor: '#999', - textColor: '#444', - buttonBackgroundColor: '#ff00ff', - buttonTextColor: '#fff', + headerColor: isValidHexColor('#999') ? '#999' : '#000000', + textColor: isValidHexColor('#444') ? '#444' : '#000000', + buttonBackgroundColor: isValidHexColor('#ff00ff') ? '#ff00ff' : '#000000', + buttonTextColor: isValidHexColor('#fff') ? '#fff' : '#000000',
166-176: Yo! Let's make these magic numbers more explicit! 🔢Extract magic numbers into named constants for better maintainability.
+ const TOTAL_TOURS = 10; + const STEPS_PER_TOUR = 3; - const toursList = new Array(10) + const toursList = new Array(TOTAL_TOURS) .fill(null) .map((_, i) => TourBuilder.tour(i + 1).build()) .map((tour, i) => { - const steps = new Array(3).fill(null) + const steps = new Array(STEPS_PER_TOUR).fill(null) .map((_, j) => TourPopupBuilder.tourPopup(j + 1, tour.id).build());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/test/mocks/tour.mock.js(3 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)
101-104: Mom's spaghetti! These method names need consistency! 🍝Some method names aren't following camelCase convention.
Also applies to: 106-109, 131-134, 136-139
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/test/e2e/tour.test.mjs (3)
32-221: Comprehensive validation test suite for POST /api/tour/add_tour.The test suite thoroughly covers all validation scenarios for the tour creation endpoint. However, consider adding edge cases for maximum field lengths and special characters.
Consider adding these test cases:
+ it('should return 400 if field lengths exceed maximum', async () => { + const longString = 'a'.repeat(256); + const res = await chai.request + .execute(app) + .post('/api/tour/add_tour') + .set('Authorization', `Bearer ${token}`) + .send({ ...tour().build(), title: longString, steps: [] }); + expect(res).to.have.status(400); + });
474-529: Consider consolidating similar test suites.The test suites for
/api/tour/all_toursand/api/tour/toursshare similar setup and assertions. Consider using a shared setup function to reduce code duplication.+ const setupTourTests = async () => { + const login = await chai.request.execute(app).post('/api/auth/register').send(user().build()); + const token = login.body.token; + await chai.request + .execute(app) + .post('/api/team/invite') + .set('Authorization', `Bearer ${token}`) + .send({ invitedEmail: validList[1].email, role: 'member' }); + // ... rest of the setup + return { token }; + };Also applies to: 530-585
1-629: Consider adding transaction rollback for test cleanup.Instead of using
force: truewhich drops and recreates tables, consider using transactions for test cleanup. This would be more efficient for test execution.+ beforeEach(async () => { + await db.sequelize.transaction(async (t) => { + // Your test setup here + context.transaction = t; + }); + }); + afterEach(async () => { + if (context.transaction) { + await context.transaction.rollback(); + } + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/test/e2e/tour.test.mjs(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (4)
backend/src/test/e2e/tour.test.mjs (4)
1-8: LGTM! Well-structured imports and setup.The test file has clean organization of imports, properly separating testing frameworks, database models, and mock data.
223-271: LGTM! Solid delete endpoint test coverage.The delete endpoint tests cover authentication, non-existent resources, and successful deletion scenarios.
272-473: Thorough validation test suite for PUT /api/tour/edit_tour/:id.The test suite effectively mirrors the validation checks from the create endpoint, ensuring consistency in validation across endpoints.
586-627: LGTM! Well-structured get tour endpoint tests.The test suite for retrieving a single tour covers all necessary scenarios including authentication, non-existent resources, and successful retrieval.
Describe your changes
Add new tour route on the backend with tour popup table
Issue number
#533
Please ensure all items are checked off before requesting a review: