feat: Implement a support ticket system with client and admin interfa…#227
feat: Implement a support ticket system with client and admin interfa…#227vendz wants to merge 8 commits into
Conversation
…ces, including models, controllers, routes, and tests.
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 complete support ticket system, providing distinct interfaces for clients to submit and track issues, and for administrators to manage and resolve them. The changes encompass new database models, API endpoints, and controllers for both user roles, ensuring a structured and functional approach to handling support requests. Integration tests are also included to validate the core client-side features. Highlights
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 support ticket system, which is a great feature. The implementation is comprehensive, covering models, controllers, routes, and tests for both client and admin interfaces.
My review focuses on improving efficiency, maintainability, and test robustness. I've identified a few areas for improvement:
- Database Query Efficiency: Both client and admin controllers for fetching ticket details perform two separate database queries. This can be optimized into a single query by defining and using Sequelize associations.
- Code Consistency: There's a minor inconsistency in the
Ticketmodel where a status is hardcoded instead of using a constant. - Test Cleanup: The test cleanup logic could be more robust to avoid potential conflicts with other tests.
I've provided specific suggestions to address these points. Overall, this is a solid contribution.
| const ticket = await Ticket.findByPk(id); | ||
| if (!ticket) { | ||
| throw new ApiError(404, 'Ticket not found'); | ||
| } | ||
|
|
||
| const messages = await TicketMessage.findAll({ | ||
| where: { ticket_id: id }, | ||
| order: [['createdAt', 'ASC']] | ||
| }); | ||
|
|
||
| res.status(200).json({ | ||
| status: 'success', | ||
| message: MSG_FETCH_SUCCESSFUL, | ||
| data: { ...ticket.toJSON(), messages } | ||
| }); |
There was a problem hiding this comment.
This function is inefficient as it performs two separate database queries to fetch a ticket and its associated messages. This can be optimized into a single query by properly defining the Sequelize associations between Ticket and TicketMessage models.
Once the association is defined (e.g., Ticket.hasMany(TicketMessage, { as: 'messages', ... })), you can use include to fetch everything at once. This simplifies the code, removes the need for manual data merging, and improves performance.
| const ticket = await Ticket.findByPk(id); | |
| if (!ticket) { | |
| throw new ApiError(404, 'Ticket not found'); | |
| } | |
| const messages = await TicketMessage.findAll({ | |
| where: { ticket_id: id }, | |
| order: [['createdAt', 'ASC']] | |
| }); | |
| res.status(200).json({ | |
| status: 'success', | |
| message: MSG_FETCH_SUCCESSFUL, | |
| data: { ...ticket.toJSON(), messages } | |
| }); | |
| const ticket = await Ticket.findByPk(id, { | |
| include: [ | |
| { | |
| model: TicketMessage, | |
| as: 'messages', // This alias must match the one in the association definition | |
| }, | |
| ], | |
| order: [[{ model: TicketMessage, as: 'messages' }, 'createdAt', 'ASC']], | |
| }); | |
| if (!ticket) { | |
| throw new ApiError(404, 'Ticket not found'); | |
| } | |
| res.status(200).json({ | |
| status: 'success', | |
| message: MSG_FETCH_SUCCESSFUL, | |
| data: ticket, | |
| }); |
| const ticket = await Ticket.findOne({ | ||
| where: { id, issued_by: cardno }, | ||
| include: [ | ||
| { | ||
| model: TicketMessage, | ||
| as: 'messages', // Note: We need to define associations | ||
| order: [['createdAt', 'ASC']] | ||
| } | ||
| ] | ||
| }); | ||
|
|
||
| if (!ticket) { | ||
| throw new ApiError(404, 'Ticket not found'); | ||
| } | ||
|
|
||
| // Fetch messages separately if association issue persists, but ideally association should work. | ||
| // For now, let's assume we'll add association in a separate step or here. | ||
| // Actually, let's fetch messages manually to be safe if associations aren't set up in models yet. | ||
| const messages = await TicketMessage.findAll({ | ||
| where: { ticket_id: id }, | ||
| order: [['createdAt', 'ASC']] | ||
| }); | ||
|
|
||
| res.status(200).json({ | ||
| status: 'success', | ||
| message: MSG_FETCH_SUCCESSFUL, | ||
| data: { ...ticket.toJSON(), messages } | ||
| }); |
There was a problem hiding this comment.
This function is inefficient as it performs two separate database queries to fetch a ticket and its associated messages. This can be optimized into a single query by properly defining the Sequelize associations between Ticket and TicketMessage models.
Once the association is defined (e.g., Ticket.hasMany(TicketMessage, { as: 'messages', ... })), you can use include to fetch everything at once. This simplifies the code, removes the need for manual data merging, and improves performance.
const ticket = await Ticket.findOne({
where: { id, issued_by: cardno },
include: [
{
model: TicketMessage,
as: 'messages', // This alias must match the one in the association definition
},
],
// To order nested models, you need to specify the model and alias
order: [[{ model: TicketMessage, as: 'messages' }, 'createdAt', 'ASC']],
});
if (!ticket) {
throw new ApiError(404, 'Ticket not found');
}
res.status(200).json({
status: 'success',
message: MSG_FETCH_SUCCESSFUL,
data: ticket,
});| }, | ||
| status: { | ||
| type: DataTypes.ENUM, | ||
| values: [STATUS_OPEN, STATUS_INPROGRESS, 'resolved', STATUS_CLOSED], |
There was a problem hiding this comment.
The status enum values are a mix of constants (e.g., STATUS_OPEN) and a hardcoded string 'resolved'. For consistency and to avoid magic strings, it's better to define a constant for 'resolved' in config/constants.js (e.g., STATUS_RESOLVED) and use it here.
| values: [STATUS_OPEN, STATUS_INPROGRESS, 'resolved', STATUS_CLOSED], | |
| values: [STATUS_OPEN, STATUS_INPROGRESS, STATUS_RESOLVED, STATUS_CLOSED], |
| afterAll(async () => { | ||
| // Cleanup | ||
| if (user) { | ||
| await TicketMessage.destroy({ where: {} }); | ||
| await Ticket.destroy({ where: {} }); | ||
| // We might not want to delete the user if other tests rely on it, but for this isolated test it's fine. | ||
| // However, CardFactory creates real records. | ||
| // Let's just leave it or try to clean up. | ||
| } | ||
| await sequelize.close(); | ||
| }); |
There was a problem hiding this comment.
The afterAll hook uses destroy({ where: {} }), which deletes all records from the TicketMessage and Ticket tables. This is a brittle approach that can cause other tests to fail if they run in parallel or rely on pre-existing data. The cleanup should be scoped to only the data created within this test suite.
A more robust approach is to delete only the specific records created during the test.
afterAll(async () => {
// Cleanup
if (ticketId) {
// To avoid deleting data from other tests, we should be specific.
await TicketMessage.destroy({ where: { ticket_id: ticketId } });
await Ticket.destroy({ where: { id: ticketId } });
}
await sequelize.close();
});
…et status to 'in progress' upon new messages.
… management to use `req.user.username` for sender and updater, and reorder admin status check.
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive support ticket system with separate client and admin interfaces, including database models, API controllers, routes, and test coverage.
- Creates a two-way communication system between users and administrators through tickets and messages
- Implements role-based access control with separate client and admin endpoints
- Adds complete test coverage for the ticket creation and messaging workflows
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| models/ticket.model.js | Defines the Ticket model with fields for tracking support requests including service type, description, device info, and status |
| models/ticket_message.model.js | Defines the TicketMessage model for storing conversation threads between users and admins |
| models/associations.js | Establishes relationships between Ticket, TicketMessage, and CardDb models |
| controllers/client/ticket.controller.js | Implements client-facing endpoints for creating tickets, viewing tickets, adding messages, and resolving tickets |
| controllers/admin/ticketManagement.controller.js | Implements admin endpoints for viewing all tickets, managing ticket status, and responding to users |
| routes/client/ticket.routes.js | Defines client API routes with card validation middleware |
| routes/admin/ticketManagement.routes.js | Defines admin API routes with authentication and role-based authorization |
| app.js | Registers the new ticket routes for both client and admin interfaces |
| controllers/admin/auth.controller.js | Code formatting improvements (unrelated to ticket system) |
| tests/tickets.test.js | Provides comprehensive test coverage for ticket creation, messaging, and retrieval functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| status: { | ||
| type: DataTypes.ENUM, | ||
| values: [STATUS_OPEN, STATUS_INPROGRESS, 'resolved', STATUS_CLOSED], | ||
| defaultValue: STATUS_OPEN, | ||
| allowNull: false | ||
| } |
There was a problem hiding this comment.
The Ticket model is missing an 'updatedBy' field, but both controllers (client/ticket.controller.js lines 104, 134 and admin/ticketManagement.controller.js lines 73, 96) attempt to update this field. This will cause the updates to fail silently or be ignored by Sequelize. Additionally, the status enum includes a hard-coded 'resolved' string instead of using a constant from config/constants.js (like STATUS_INPROGRESS and STATUS_CLOSED). This is inconsistent with the codebase convention seen in other models like maintenance_db.model.js.
| if (ticket.status === 'closed') { | ||
| throw new ApiError(400, 'Cannot reply to a closed ticket'); | ||
| } | ||
|
|
||
| await TicketMessage.create({ | ||
| ticket_id, | ||
| sender_id: cardno, | ||
| sender_type: 'user', | ||
| message | ||
| }); | ||
|
|
||
| // If ticket was resolved, move back to in progress since user is replying | ||
| const updates = { updatedBy: cardno }; | ||
| if (ticket.status === 'resolved') { | ||
| updates.status = 'in progress'; | ||
| } |
There was a problem hiding this comment.
Hard-coded status strings 'closed' and 'in progress' are used instead of the constants STATUS_CLOSED and STATUS_INPROGRESS from config/constants.js. This is inconsistent with the codebase convention as seen in other controllers like admin/maintenanceManagement.controller.js which consistently uses STATUS_OPEN, STATUS_CLOSED, and STATUS_INPROGRESS constants. Using hard-coded strings makes the code more error-prone and harder to maintain.
| if (ticket.status === 'closed') { | ||
| throw new ApiError(400, 'Ticket is already closed'); | ||
| } | ||
|
|
||
| await ticket.update({ | ||
| status: 'closed', | ||
| updatedBy: cardno |
There was a problem hiding this comment.
Hard-coded status strings 'closed' and 'resolved' are used instead of constants. The model defines 'resolved' as a valid status, but there's no corresponding STATUS_RESOLVED constant in config/constants.js. Either add STATUS_RESOLVED to constants.js or remove 'resolved' as a status option. Additionally, 'closed' should use STATUS_CLOSED constant for consistency.
| await Ticket.create({ | ||
| id: generateTicketId(), | ||
| issued_by: cardno, | ||
| service, | ||
| description, | ||
| os, | ||
| app_version | ||
| }); | ||
|
|
||
| res.status(201).send({ | ||
| message: 'Successfully created ticket' | ||
| }); |
There was a problem hiding this comment.
The createTicket response doesn't return the created ticket data, but the test expects it (lines 44-47 check response.body.data.service, response.body.data.issued_by, and response.body.data.id). The response should include the created ticket in the data field. Consider storing the result of Ticket.create() and including it in the response.
| res.status(201).send({ | ||
| message: 'Successfully created ticket' | ||
| }); | ||
| }; | ||
|
|
||
| export const getTickets = async (req, res) => { | ||
| const { cardno } = req.user; | ||
| const page = parseInt(req.query.page) || 1; | ||
| const pageSize = parseInt(req.query.page_size) || 10; | ||
| const offset = (page - 1) * pageSize; | ||
|
|
||
| const tickets = await Ticket.findAll({ | ||
| where: { issued_by: cardno }, | ||
| order: [['createdAt', 'DESC']], | ||
| offset, | ||
| limit: pageSize | ||
| }); | ||
|
|
||
| res.status(200).send({ | ||
| message: MSG_FETCH_SUCCESSFUL, | ||
| data: tickets | ||
| }); |
There was a problem hiding this comment.
The response format is inconsistent with the client controller responses. While this admin controller correctly includes 'status', 'message', and 'data' fields, the client ticket controller (ticket.controller.js) only includes 'message' and 'data'. For consistent API design, both client and admin endpoints should use the same response structure. The admin format with the 'status' field is the better choice as it matches what the tests expect.
| expect(response.body.data.message).toBe('Is anyone looking at this?'); | ||
| expect(response.body.data.sender_id).toBe(user.cardno); |
There was a problem hiding this comment.
The test expects response.body.data.message and response.body.data.sender_id (lines 71-72), but the controller doesn't return the created message in the response. The controller only returns a message field with MSG_UPDATE_SUCCESSFUL. The test will fail because response.body.data will be undefined. The controller should return the created message object, similar to how the admin controller does (admin/ticketManagement.controller.js lines 80-83).
| expect(response.body.data.message).toBe('Is anyone looking at this?'); | |
| expect(response.body.data.sender_id).toBe(user.cardno); | |
| expect(response.body.message).toBe('MSG_UPDATE_SUCCESSFUL'); | |
| // Verify that the message was actually created in the database | |
| const createdMessage = await TicketMessage.findOne({ | |
| where: { | |
| ticket_id: ticketId, | |
| sender_id: user.cardno, | |
| message: 'Is anyone looking at this?' | |
| } | |
| }); | |
| expect(createdMessage).not.toBeNull(); |
| as: 'messages', | ||
| order: [['createdAt', 'ASC']] | ||
| } | ||
| ] |
There was a problem hiding this comment.
The order clause is incorrectly placed inside the include object. In Sequelize, when ordering included associations, the order should be specified at the top-level findOne options or using a nested array format like: order: [[{model: TicketMessage, as: 'messages'}, 'createdAt', 'ASC']]. The current placement inside the include object will be ignored by Sequelize, and messages will not be ordered as intended.
| as: 'messages', | |
| order: [['createdAt', 'ASC']] | |
| } | |
| ] | |
| as: 'messages' | |
| } | |
| ], | |
| order: [[{ model: TicketMessage, as: 'messages' }, 'createdAt', 'ASC']] |
| if (ticket.status === 'closed') { | ||
| throw new ApiError(400, 'ticket is closed'); | ||
| } | ||
|
|
||
| const newMessage = await TicketMessage.create({ | ||
| ticket_id: id, | ||
| sender_id: req.user.username, | ||
| sender_type: 'admin', | ||
| message | ||
| }); | ||
|
|
||
| // Update ticket updatedBy and status if needed | ||
| const updates = { updatedBy: req.user.username }; | ||
| if (ticket.status === 'open') { | ||
| updates.status = 'in progress'; | ||
| } |
There was a problem hiding this comment.
Hard-coded status strings 'closed', 'open', and 'in progress' should use constants STATUS_CLOSED, STATUS_OPEN, and STATUS_INPROGRESS from config/constants.js. This is inconsistent with codebase conventions as seen in other maintenance controllers.
| } | ||
|
|
||
| if (ticket.status === 'closed') { | ||
| throw new ApiError(400, 'ticket is closed'); |
There was a problem hiding this comment.
The error message starts with a lowercase letter 'ticket is closed', which is inconsistent with other error messages in the same file (line 53: 'Message is required', line 58: 'Ticket not found'). Error messages should be consistently capitalized for better readability and professionalism.
| throw new ApiError(400, 'ticket is closed'); | |
| throw new ApiError(400, 'Ticket is closed'); |
…ces, including models, controllers, routes, and tests.