feat: Add endpoint and controller to check for events on a travel date.#250
feat: Add endpoint and controller to check for events on a travel date.#250vendz wants to merge 3 commits into
Conversation
Summary of ChangesHello @vendz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new feature designed to enhance the travel booking system by providing visibility into upcoming events. It adds a dedicated API endpoint and controller logic that queries the event database for any 'Utsavs' scheduled at the 'Research Centre' from the current day, returning a simple indication of their presence. This allows for better planning or awareness regarding events coinciding with travel dates. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new endpoint /events to check for Utsav events. The implementation in the controller has a critical bug that will cause a runtime error due to an undefined variable. Additionally, it uses a hardcoded string instead of a constant. There's also a potential logic issue where the function's behavior (checking for any future events) doesn't seem to match its name and the PR's intent (checking for events on a specific travel date). I've provided comments with suggestions to fix these issues.
| end_date: { | ||
| [Op.gte]: today | ||
| }, | ||
| location: { | ||
| [Op.eq]: 'Research Centre' | ||
| } |
There was a problem hiding this comment.
There are a couple of issues in this query:
- The
Opobject from Sequelize is not defined, which will cause aReferenceError. You should useSequelize.Opinstead, asSequelizeis already imported. - The string
'Research Centre'is hardcoded. It's better to use theRESEARCH_CENTREconstant from../../config/constants.jsfor consistency and maintainability. Please remember to import it.
| end_date: { | |
| [Op.gte]: today | |
| }, | |
| location: { | |
| [Op.eq]: 'Research Centre' | |
| } | |
| end_date: { | |
| [Sequelize.Op.gte]: today | |
| }, | |
| location: { | |
| [Sequelize.Op.eq]: RESEARCH_CENTRE | |
| } |
| export const checkEventsOnTravelDate = async (req, res) => { | ||
| const today = moment().format('YYYY-MM-DD'); | ||
|
|
||
| const utsavs = await UtsavDb.findAll({ | ||
| where: { | ||
| end_date: { | ||
| [Op.gte]: today | ||
| }, | ||
| location: { | ||
| [Op.eq]: 'Research Centre' | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| return res.status(200).send({ | ||
| message: 'fetched results', | ||
| data: { | ||
| utsav: utsavs.length > 0 | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The function name checkEventsOnTravelDate and the PR title suggest it checks for events on a specific date, but the implementation checks for any events from today onwards and does not accept a date parameter. This might be misleading or not what the user of the API expects.
If the goal is to check for a specific date, you should get the date from the request query and adjust the database query. For example:
// Get date from query, default to today
const travelDate = req.query.date ? moment(req.query.date).format('YYYY-MM-DD') : moment().format('YYYY-MM-DD');
// ... later in the query
const utsavs = await UtsavDb.findAll({
where: {
start_date: { [Sequelize.Op.lte]: travelDate },
end_date: { [Sequelize.Op.gte]: travelDate },
location: { [Sequelize.Op.eq]: RESEARCH_CENTRE }
}
});If checking from today onwards is the intended behavior, consider renaming the function to something like checkUpcomingEvents to better reflect its purpose.
There was a problem hiding this comment.
Pull request overview
This PR adds a new endpoint to check for upcoming events (utsavs) at the Research Centre. The endpoint returns whether there are any active events, which can be useful for travel booking considerations.
Key changes:
- New GET
/eventsendpoint in the travel booking routes - Controller function
checkEventsOnTravelDatethat queries the UtsavDb for active events - Minor formatting improvement (spacing fix) in the existing
CancelTravelfunction
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| routes/client/travelBooking.routes.js | Adds new /events route and imports the checkEventsOnTravelDate controller function |
| controllers/client/travelBooking.controller.js | Implements checkEventsOnTravelDate function to query events, adds UtsavDb import, and fixes spacing in existing code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const checkEventsOnTravelDate = async (req, res) => { | ||
| const today = moment().format('YYYY-MM-DD'); | ||
|
|
||
| const utsavs = await UtsavDb.findAll({ | ||
| where: { | ||
| end_date: { | ||
| [Op.gte]: today | ||
| }, | ||
| location: { | ||
| [Op.eq]: 'Research Centre' | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| return res.status(200).send({ | ||
| message: 'fetched results', | ||
| data: { | ||
| utsav: utsavs.length > 0 | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The function uses the Op operator from Sequelize but it's not imported. You need to add the import statement at the top of the file.
| export const checkEventsOnTravelDate = async (req, res) => { | ||
| const today = moment().format('YYYY-MM-DD'); | ||
|
|
||
| const utsavs = await UtsavDb.findAll({ | ||
| where: { | ||
| end_date: { | ||
| [Op.gte]: today | ||
| }, | ||
| location: { | ||
| [Op.eq]: 'Research Centre' | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| return res.status(200).send({ | ||
| message: 'fetched results', | ||
| data: { | ||
| utsav: utsavs.length > 0 | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The PR title indicates this endpoint should check for events on a travel date, but the function doesn't accept any date parameter from the request. It only checks against today's date. Consider accepting a date parameter from req.query to allow checking events for a specific travel date, which would align with the PR's stated purpose.
| location: { | ||
| [Op.eq]: 'Research Centre' | ||
| } |
There was a problem hiding this comment.
Using Op.eq for simple equality checks is unnecessary. You can directly assign the value in the where clause instead of using the operator. This would simplify the code.
| location: { | |
| [Op.eq]: 'Research Centre' | |
| } | |
| location: 'Research Centre' |
| export const checkEventsOnTravelDate = async (req, res) => { | ||
| const today = moment().format('YYYY-MM-DD'); | ||
|
|
||
| const utsavs = await UtsavDb.findAll({ | ||
| where: { | ||
| end_date: { | ||
| [Op.gte]: today | ||
| }, | ||
| location: { | ||
| [Op.eq]: 'Research Centre' | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| return res.status(200).send({ | ||
| message: 'fetched results', | ||
| data: { | ||
| utsav: utsavs.length > 0 | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The new controller function lacks test coverage. Other client controller tests exist in the tests/controllers/client directory, such as adhyayanBooking, flatBooking, and mumukshuBooking. Consider adding tests for this new endpoint to maintain consistency with the existing test coverage in the codebase.
| }); | ||
|
|
||
| return res.status(200).send({ | ||
| message: 'fetched results', |
There was a problem hiding this comment.
The response message 'fetched results' uses lowercase, which is inconsistent with other success messages in the codebase that typically use title case or uppercase for the first letter, such as 'Fetched data' (line 64) in the same file.
| message: 'fetched results', | |
| message: 'Fetched results', |
…oking fetch response.
No description provided.