Add user profile details and booked events #469
Conversation
|
@haseebzaki-07 is attempting to deploy a commit to the bunty's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes introduce new functionality to the backend and frontend of the application, focusing on customer and event management. A new asynchronous function Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
|
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. In the meantime, please ensure that your changes align with our CONTRIBUTING.md. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (14)
backend/routes/eventRouter.js (2)
36-37: Fix inconsistent spacing in route definitions.The route definitions have inconsistent spacing around parameters. Maintain consistency with the existing route definitions.
-router.get('/booked-events',authenticateCustomer, getBookedEvents); -router.post('/book',authenticateCustomer, bookEvent); +router.get('/booked-events', authenticateCustomer, getBookedEvents); +router.post('/book', authenticateCustomer, bookEvent);
Line range hint
15-26: Update API documentation in welcome message.The welcome message's endpoints section should be updated to include the new routes.
endpoints: { CreateEvent: "/event/create", GetEvents: "/event/all", + GetBookedEvents: "/event/booked-events", + BookEvent: "/event/book", },backend/routes/customerRouter.js (1)
42-42: Add spaces around middleware function for consistency.The route implementation looks good with proper authentication middleware. However, there should be spaces around the middleware function for better readability.
-router.get('/profile',authenticateCustomer, getCustomerDetail ); +router.get('/profile', authenticateCustomer, getCustomerDetail);backend/controller/customer.controller.js (2)
191-211: Consider adding pagination for related collections.Loading all booked events, orders, and reservations at once could be problematic for users with many records.
Consider implementing pagination for these collections:
- Accept
pageandlimitquery parameters- Use mongoose's
skip()andlimit()in populate options- Return metadata about total counts and available pages
Example implementation:
async function getCustomerDetail(req, res) { const { id } = req.user; const { page = 1, limit = 10 } = req.query; const skip = (page - 1) * limit; try { const user = await Customer.findById(id) .populate({ path: 'bookedEvents', options: { skip, limit }, select: 'title date status price' }) .select('-password -verificationCode -otp'); const totalEvents = await Customer.findById(id) .populate('bookedEvents') .countDocuments(); res.status(200).json({ user, pagination: { total: totalEvents, pages: Math.ceil(totalEvents / limit), current: page } }); } catch (error) { // ... error handling } }
191-193: Add input validation for user ID.While the ID comes from the authentication middleware, it's still good practice to validate it.
Add validation at the start of the function:
async function getCustomerDetail(req, res) { const { id } = req.user; + if (!id || !mongoose.Types.ObjectId.isValid(id)) { + return res.status(400).json({ message: 'Invalid user ID' }); + }backend/controller/event.controller.js (3)
111-111: Use consistent logging mechanismYou're using
console.errorfor error logging here. For consistency and better logging practices, consider usinglogger.erroras used elsewhere in your code.Apply this diff to update the logging:
- console.error("Error booking event:", error); + logger.error("Error booking event:", error);- console.error("Error fetching booked events:", error); + logger.error("Error fetching booked events:", error);Also applies to: 131-131
77-79: Validate IDs before database operationsEnsure that
eventIdanduserIdare valid MongoDB ObjectIds before using them in database queries to prevent potential errors or security issues.You can use
mongoose.Types.ObjectId.isValid(id)to validate the IDs. For example:+ const mongoose = require('mongoose'); ... // Check if eventId is provided and valid if (!eventId || !mongoose.Types.ObjectId.isValid(eventId)) { return res.status(400).json({ message: "Invalid Event ID" }); } ... // Check if userId is valid if (!mongoose.Types.ObjectId.isValid(userId)) { return res.status(400).json({ message: "Invalid User ID" }); }Similarly, in
getBookedEvents:+ const mongoose = require('mongoose'); ... const { id } = req.user; + if (!mongoose.Types.ObjectId.isValid(id)) { + return res.status(400).json({ message: "Invalid User ID" }); + }Also applies to: 82-85, 88-91, 117-117, 121-122
135-135: Fix inconsistent spacing inmodule.exportsThere is inconsistent spacing around commas in the
module.exportsobject. For better readability and code consistency, consider formatting it uniformly.Apply this diff:
- module.exports = { createEvent, getEvents, deleteEvent , getBookedEvents , bookEvent}; + module.exports = { createEvent, getEvents, deleteEvent, getBookedEvents, bookEvent };frontend/src/components/Pages/Dashboard.tsx (6)
18-21: Refactor duplicated authentication token handlingThe logic for retrieving and checking the authentication token is duplicated in both
useEffecthooks. To adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring this logic into a custom hook or utility function.Here's how you might refactor this:
Create a custom hook for authentication:
// In a new file, e.g., src/hooks/useAuth.js import { useNavigate } from 'react-router-dom'; import Cookies from 'js-cookie'; import jwtDecode from 'jwt-decode'; const useAuth = () => { const navigate = useNavigate(); const authToken = Cookies.get('authToken'); if (!authToken) { alert("Please sign in to continue."); navigate('/login'); return null; } try { const decodedToken = jwtDecode(authToken); const userId = decodedToken.sub; return { authToken, userId }; } catch (decodeError) { console.error("Error decoding token:", decodeError); alert("Invalid token. Please log in again."); navigate('/login'); return null; } }; export default useAuth;Update your component to use the custom hook:
+ import useAuth from '../../hooks/useAuth'; const Dashboard = () => { const [profile, setProfile] = useState(null); const [reservations, setReservations] = useState([]); const [events, setEvents] = useState([]); const [error, setError] = useState(null); const [loading, setLoading] = useState(true); const navigate = useNavigate(); const API_URL = process.env.REACT_APP_API_URL; useEffect(() => { - const fetchUserData = async () => { + const fetchUserData = async (authData) => { setLoading(true); - const authToken = Cookies.get('authToken'); - if (!authToken) { - alert("Please sign in to view your profile, reservations, and events."); - navigate('/login'); - return; - } - let userId; - try { - const decodedToken = jwtDecode(authToken); - userId = decodedToken.sub; - } catch (decodeError) { - console.error("Error decoding token:", decodeError); - alert("Invalid token. Please log in again."); - navigate('/login'); - return; - } + const { authToken, userId } = authData; + if (!authToken) return; try { // Fetch Profile Data const profileResponse = await fetch(`${API_URL}/api/user/profile`, { headers: { 'Authorization': `Bearer ${authToken}` }, credentials: 'include', }); // ... rest of the code remains the same } }; + const authData = useAuth(); + if (authData) { + fetchUserData(authData); + } }, [navigate]); useEffect(() => { - const fetchEvents = async () => { - const authToken = Cookies.get('authToken'); - console.log("Testing events fetch with token:", authToken); - if (!authToken) { - console.error("No auth token found"); - return; - } + const fetchEvents = async (authData) => { + const { authToken } = authData; + if (!authToken) return; try { const eventsResponse = await fetch(`${API_URL}/api/event/booked-events`, { headers: { 'Authorization': `Bearer ${authToken}` }, credentials: 'include', }); // ... rest of the code remains the same } }; + const authData = useAuth(); + if (authData) { + fetchEvents(authData); + } }, [API_URL]); // ... rest of the component };This refactor improves code maintainability and ensures consistent authentication handling across your component.
Also applies to: 80-86
81-81: Remove debuggingconsole.logstatementThe
console.log("Testing events fetch with token:", authToken);is likely used for debugging purposes. It's advisable to remove such statements from production code to prevent unnecessary console output and potential exposure of sensitive information.Apply this diff to remove the console log:
- console.log("Testing events fetch with token:", authToken);
83-86: Ensure consistent authentication error handlingIn the event of a missing authentication token, the first
useEffectalerts the user and redirects them to the login page, while the seconduseEffectonly logs an error. For a consistent user experience, consider handling authentication errors uniformly.Modify the code to navigate the user to the login page when the token is missing:
if (!authToken) { - console.error("No auth token found"); - return; + alert("Please sign in to view your events."); + navigate('/login'); + return; }
96-104: Handle errors by updating the error stateIn the
fetchEventsfunction, errors are logged to the console but not reflected in the UI. To improve user experience, consider updating theerrorstate and displaying an error message.Modify the catch block to update the error state:
} catch (error) { console.error("Error fetching events:", error.message); + setError(error.message); }And update the rendering logic to display the error:
{error && ( <p className="text-red-500 text-center" aria-live="assertive"> {error} </p> )}
130-133: Avoid using array index as key in listsUsing the array index as the
keyprop in lists can lead to rendering issues, especially if items are added, removed, or reordered. Instead, use a unique identifier from the data items.For reservations:
- reservations.map((reservation, index) => ( - <ReservationCard key={index} reservation={reservation} /> + reservations.map((reservation) => ( + <ReservationCard key={reservation._id} reservation={reservation} />For events:
- events.map((event, index) => ( - <EventCard key={index} event={event} /> + events.map((event) => ( + <EventCard key={event._id} event={event} />Ensure that
reservation._idandevent._idare unique identifiers available in your data.Also applies to: 141-144
155-164: Enhance accessibility with semantic HTML elementsConsider using semantic HTML elements like
<section>and<article>to improve the accessibility and structure of your components.For example, in
ProfileCardandEventCardcomponents:- const ProfileCard = ({ profile }) => ( - <div className="bg-white dark:bg-amber-800 rounded-lg shadow-md p-6 mb-10"> + const ProfileCard = ({ profile }) => ( + <section className="bg-white dark:bg-amber-800 rounded-lg shadow-md p-6 mb-10"> {/* Content */} - </div> + </section>- const EventCard = ({ event }) => ( - <div className="bg-white dark:bg-amber-800 rounded-lg shadow-md p-4"> + const EventCard = ({ event }) => ( + <article className="bg-white dark:bg-amber-800 rounded-lg shadow-md p-4"> {/* Content */} - </div> + </article>This improves the semantic structure of your HTML and aids assistive technologies.
Also applies to: 181-194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
backend/controller/customer.controller.js(2 hunks)backend/controller/event.controller.js(2 hunks)backend/routes/customerRouter.js(2 hunks)backend/routes/eventRouter.js(2 hunks)frontend/src/components/Pages/Dashboard.tsx(4 hunks)
🔇 Additional comments (4)
backend/routes/eventRouter.js (1)
7-9: LGTM! Controller imports are properly structured.
The new controller function imports follow the existing pattern and maintain consistent naming conventions.
backend/routes/customerRouter.js (2)
8-8: LGTM!
The import statement follows the existing pattern and is properly placed within the destructured import.
Line range hint 14-42: Consider standardizing authentication approach across routes.
I notice that some routes use passport.authenticate("jwt") while others use authenticateCustomer. Consider standardizing the authentication approach across all protected routes for better maintainability and security consistency.
Let's verify the authentication approaches used across the codebase:
backend/controller/customer.controller.js (1)
227-228: LGTM! Export changes are correct.
The new function is properly exported alongside existing ones.
| router.get('/booked-events',authenticateCustomer, getBookedEvents); | ||
| router.post('/book',authenticateCustomer, bookEvent); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to new routes.
The new routes should follow the error handling pattern used in other routes (like the root route) to maintain consistency and reliability.
-router.get('/booked-events', authenticateCustomer, getBookedEvents);
+router.get('/booked-events', authenticateCustomer, async (req, res) => {
+ try {
+ await getBookedEvents(req, res);
+ } catch (error) {
+ logger.error("Error in /booked-events route:", error);
+ res.status(500).json({ error: "Internal server error" });
+ }
+});
-router.post('/book', authenticateCustomer, bookEvent);
+router.post('/book', authenticateCustomer, async (req, res) => {
+ try {
+ await bookEvent(req, res);
+ } catch (error) {
+ logger.error("Error in /book route:", error);
+ res.status(500).json({ error: "Internal server error" });
+ }
+});Committable suggestion skipped: line range outside the PR's diff.
💡 Codebase verification
Based on the search results from both attempts, I can generate the final response as we have enough evidence that test coverage is missing.
Test coverage missing for booking-related endpoints
The new routes /booked-events and /book lack corresponding test cases. No test files were found in the backend directory that cover these event booking functionalities.
- Add test cases for
getBookedEventscontroller in the backend - Add test cases for
bookEventcontroller in the backend - Include both successful and error scenarios in the test coverage
🔗 Analysis chain
Verify test coverage for new routes.
Ensure that appropriate test cases are added for the new booking-related endpoints.
Let me try another search strategy to look for test files and test cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test files covering the new routes
# Look for test files related to event booking
echo "Searching for event booking test files..."
fd -e test.js -e spec.js -e test.ts -e spec.ts . | grep -i "event\|book"
# Search for test cases within test files
echo "Searching for booking-related test cases..."
rg -i "describe.*book|test.*book|it.*book" -t js -t ts
Length of output: 562
Script:
#!/bin/bash
# Search for test files in backend directory since the routes are in backend
echo "Searching for test files in backend..."
fd -e test.js -e spec.js . backend/
# Search specifically for event controller tests
echo "Searching for event controller test content..."
rg -l "event.controller|eventController" -g "*.test.js" -g "*.spec.js"
# Look for any test cases mentioning events or booking
echo "Searching for event/booking test cases..."
rg "describe|it" -A 2 -g "*.test.js" -g "*.spec.js" | grep -i "event\|book"
Length of output: 769
| const user = await Customer.findById(id) | ||
| .populate('bookedEvents') // Populate booked events if needed | ||
| .populate('orders') // Populate orders if needed | ||
| .populate('reservations') // Populate reservations if needed | ||
| .select('-password -verificationCode -otp'); // Exclude sensitive fields |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize database queries for better performance.
The multiple .populate() calls without field selection could impact performance, especially when dealing with large datasets.
Consider applying these optimizations:
const user = await Customer.findById(id)
- .populate('bookedEvents') // Populate booked events if needed
- .populate('orders') // Populate orders if needed
- .populate('reservations') // Populate reservations if needed
+ .populate('bookedEvents', 'title date status price')
+ .populate('orders', 'orderNumber totalAmount status')
+ .populate('reservations', 'date status')
.select('-password -verificationCode -otp');📝 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 user = await Customer.findById(id) | |
| .populate('bookedEvents') // Populate booked events if needed | |
| .populate('orders') // Populate orders if needed | |
| .populate('reservations') // Populate reservations if needed | |
| .select('-password -verificationCode -otp'); // Exclude sensitive fields | |
| const user = await Customer.findById(id) | |
| .populate('bookedEvents', 'title date status price') | |
| .populate('orders', 'orderNumber totalAmount status') | |
| .populate('reservations', 'date status') | |
| .select('-password -verificationCode -otp'); // Exclude sensitive fields |
| const isAlreadyBooked = customer.bookedEvents.some( | ||
| (bookedEvent) => bookedEvent.toString() === eventId | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use .equals() method for ObjectId comparisons
When comparing MongoDB ObjectIds, it's better to use the .equals() method instead of converting them to strings for accurate comparisons.
Apply this diff to improve the comparison:
- const isAlreadyBooked = customer.bookedEvents.some(
- (bookedEvent) => bookedEvent.toString() === eventId
- );
+ const isAlreadyBooked = customer.bookedEvents.some(
+ (bookedEvent) => bookedEvent.equals(eventId)
+ );📝 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 isAlreadyBooked = customer.bookedEvents.some( | |
| (bookedEvent) => bookedEvent.toString() === eventId | |
| ); | |
| const isAlreadyBooked = customer.bookedEvents.some( | |
| (bookedEvent) => bookedEvent.equals(eventId) | |
| ); |
| module.exports = { createEvent, getEvents, deleteEvent }; | ||
| const bookEvent = async (req, res) => { | ||
| const { eventId } = req.body; | ||
| const userId = req.user; |
There was a problem hiding this comment.
Incorrect assignment of userId
In line 73, const userId = req.user; assigns the entire user object to userId. Since Customer.findById(userId) expects an ID, you should extract the user ID from req.user.
Apply this diff to fix the issue:
- const userId = req.user;
+ const userId = req.user.id; // or req.user._idCommittable suggestion skipped: line range outside the PR's diff.
| @@ -1,32 +1,32 @@ | |||
| import { useEffect, useState } from 'react'; | |||
| import { useNavigate } from 'react-router-dom'; | |||
| import Cookies from 'js-cookie'; | |||
| import { jwtDecode } from 'jwt-decode'; | |||
| import {jwtDecode} from 'jwt-decode'; // Import correction | |||
There was a problem hiding this comment.
Incorrect import statement for 'jwt-decode'
The jwt-decode library exports jwtDecode as a default export, so you should import it without curly braces.
Here's the corrected import statement:
-import {jwtDecode} from 'jwt-decode'; // Import correction
+import jwtDecode from 'jwt-decode'; // Import correction📝 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.
| import {jwtDecode} from 'jwt-decode'; // Import correction | |
| import jwtDecode from 'jwt-decode'; // Import correction |
| const [error, setError] = useState(null); | ||
| const [loading, setLoading] = useState(true); | ||
| const navigate = useNavigate(); | ||
| const API_URL = import.meta.env.VITE_BACKEND_URL || 'http://localhost:3000'; | ||
| const API_URL = 'http://localhost:3000'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding the API URL
Hardcoding the API URL can cause issues when deploying to different environments. Consider using environment variables to manage the API base URL.
Here's how you can modify it:
- const API_URL = 'http://localhost:3000';
+ const API_URL = process.env.REACT_APP_API_URL;Make sure to set REACT_APP_API_URL in your environment configuration files.
Committable suggestion skipped: line range outside the PR's diff.
|
@RamakrushnaBiswal Please review this PR! |
RamakrushnaBiswal
merged commit 723e358
into
RamakrushnaBiswal:main
RamakrushnaBiswal
added
enhancement
fixes #444
Add User details and booked events to the profile page
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor