535 tour url services #538
Conversation
WalkthroughThis pull request introduces enhancements for tour-related functionality across multiple components. In the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant R as Router
participant V as Validator
participant TC as TourController
participant TS as TourService
C->>R: GET /get_tour_by_url with { url }
R->>V: Validate request body (url)
V-->>R: Validation passed
R->>TC: Call getTourByUrl(req, res)
TC->>TS: getTourByUrl(url)
TS-->>TC: Return tour data
TC-->>R: Response with tour JSON
R-->>C: Send tour data response
sequenceDiagram
participant C as Client
participant GC as GuideController
participant TS as TourService
participant GL as GuideLogService
C->>GC: GET /guides?url=... (or incomplete guides)
GC->>TS: getTourByUrl(url)
TS-->>GC: Return tour data
alt Incomplete Guides Request
GC->>GL: getCompleteToursLogs(userId)
GL-->>GC: Return complete tour logs
end
GC-->>C: Respond with { banner, popup, hint, helperLink, tour }
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: 5
🧹 Nitpick comments (5)
backend/src/utils/tour.helper.js (1)
40-42: Mom's spaghetti! DRY principle violation spotted!The URL validation logic is duplicated between
tourValidatorandbodyUrlValidator. Consider extracting the common validation chain into a reusable constant:+const urlValidationChain = body('url') + .isString() + .withMessage('url is required') + .custom(validateUrl) + .withMessage('Invalid value for url'); const tourValidator = [ - body('url').isString().withMessage('url is required').custom(validateUrl).withMessage('Invalid value for url'), + urlValidationChain, // ... other validations ]; const bodyUrlValidator = [ - body('url').isString().withMessage('url is required').custom(validateUrl).withMessage('Invalid value for url'), + urlValidationChain, ];backend/src/controllers/guide.controller.js (1)
14-16: URL validation logic is duplicated! Mom's spaghetti!Extract the common URL validation logic into a reusable function to avoid duplication:
+const validateUrlInput = (url) => { + if (!url || typeof url !== 'string') { + return { error: { errors: [{ msg: 'URL is missing or invalid' }] } }; + } + return null; +}; async getGuidesByUrl(req, res) { try { const { url } = req.body; - if (!url || typeof url !== 'string') { - return res.status(400).json({ errors: [{ msg: 'URL is missing or invalid' }] }); + const validationError = validateUrlInput(url); + if (validationError) { + return res.status(400).json(validationError.error); } // ... rest of the codeAlso applies to: 34-36
backend/src/controllers/tour.controller.js (1)
45-54: Yo, we need some additional validation checks, eh?The method looks solid overall, but we should add some validation to handle empty or malformed URLs, just like mom's spaghetti - you gotta cook it right.
Here's a suggested improvement:
async getTourByUrl(req, res) { try { const { url } = req.body; + if (!url || typeof url !== 'string') { + return res.status(400).json({ + errors: [{ msg: 'Valid URL is required, eh?' }], + }); + } const tours = await TourService.getTourByUrl(url); + if (!tours || tours.length === 0) { + return res.status(404).json({ + errors: [{ msg: 'No tours found for the specified URL' }], + }); + } res.status(200).json(tours); } catch (err) { const { statusCode, payload } = internalServerError('GET_TOUR_BY_URL_ERROR', err.message); res.status(statusCode).json(payload); } }backend/src/service/guidelog.service.js (2)
85-97: Opportunity to optimize the database query, eh?The implementation looks clean like a fresh Canadian snow, but we could add an index to improve query performance when you're nervous with sweaty palms.
Add an index on the frequently queried columns:
CREATE INDEX idx_guidelog_user_complete_type ON guide_logs(userId, completed, guideType);This will help when your knees are weak but the query needs to be strong.
9-18: Let's keep the transaction handling consistent across methods, eh?The transaction handling in
addGuideLogis solid, but we should consider adding similar transaction support to other write operations that might be added in the future.Consider creating a helper method to standardize transaction handling:
async function withTransaction(callback) { const transaction = await db.sequelize.transaction(); try { const result = await callback(transaction); await transaction.commit(); return result; } catch (err) { await transaction.rollback(); throw err; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/controllers/guide.controller.js(1 hunks)backend/src/controllers/tour.controller.js(1 hunks)backend/src/routes/tour.routes.js(1 hunks)backend/src/service/guidelog.service.js(5 hunks)backend/src/service/tour.service.js(3 hunks)backend/src/utils/tour.helper.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/models/index.js (1)
51-52: The new association naming is cleaner and more intuitive!Changing from
popupstostepsbetter represents the relationship betweenTourandTourPopupmodels. Each popup is really a step in the tour sequence.Consider also adding an
orderfield toTourPopupif not already present, to maintain the sequence of steps in a tour.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/models/index.js(1 hunks)backend/src/service/tour.service.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (22.x)
🔇 Additional comments (4)
backend/src/service/tour.service.js (4)
2-2: Mom's spaghetti looks good! Import is properly placed!The Sequelize Op import is correctly added and necessary for the new query operations.
24-41: Knees weak! Error handling needs strengthening!The error handling could be improved:
- Replace console.log with proper logging service
- Include the actual error in the thrown Error message for better debugging
However, the implementation itself is solid with proper use of:
- Active flag filtering
- Related steps inclusion
- Sequelize operators
43-57: Arms are heavy! Same error handling issues here!Vomit on his sweater already! Consider pagination for large datasets!
The Op.notIn operation could be inefficient for large sets of IDs. Consider adding pagination or limiting the result set size.
async getIncompleteTourByUrl(url, ids) { try { return await Tour.findAll({ where: { url, id: { [Op.notIn]: ids, }, }, + limit: 100, // Add reasonable limit + offset: 0, // Add offset for pagination }); } catch (err) { console.log(err); throw new Error('Error finding incomplete tours by url'); } }
94-94: He's nervous, but the semicolon looks calm and ready!The added semicolon maintains consistent code formatting.
Describe your changes
create get_tour_by_url route similar to popup
create getIncompleteToursByUrl in tour.service
create getCompleteTours in guidelog.service
call getCompleteTours in getGuidesByUrl and getIncompleteGuidesByUrl in guide.controller like we did for other guides
Issue number
#535
Please ensure all items are checked off before requesting a review: