Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: cleanup and renaming controllers #9

Open
wants to merge 3 commits into
base: AI-Code-Review
Choose a base branch
from
Open

feat: cleanup and renaming controllers #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aceb7fc
feat: cleanup and renaming controllers
xedsvg Jun 11, 2023
9e1f369
fix: mongo events and variations on client orders
xedsvg Jun 17, 2023
73e6004
feat: initial owner view lockdown
xedsvg Jun 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions backend/.eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
module.exports = {
'env': {
'browser': true,
'es2021': true,
'node': true
},
'extends': 'eslint:recommended',
'overrides': [
],
'parserOptions': {
'ecmaVersion': 'latest',
'sourceType': 'module'
},
'rules': {
'indent': [
'error',
'tab'
],
'linebreak-style': [
'error',
'unix'
],
'quotes': [
'error',
'single'
],
'semi': [
'error',
'always'
]
}
};
xedsvg marked this conversation as resolved.
Show resolved Hide resolved
68 changes: 34 additions & 34 deletions backend/app/controllers/appController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,46 @@
const { getRestaurantFromId, getRestaurantFromName, getRestaurantFromHostname } = require('../datamodels');
const { getRestaurantById, getRestaurantByName, getRestaurantByHostname } = require('../datamodels');

const getRestaurantById = async (req, res) => {
const { params: { restaurantId } } = req;
console.log(req.params);
const getRestaurantByIdController = async (req, res) => {
const { params: { restaurantId } } = req;
console.log(req.params);

try {
const result = await getRestaurantFromId(restaurantId);
res.send(result);
} catch (error) {
res.sendStatus(404);
}
try {
const result = await getRestaurantById(restaurantId);
res.send(result);
} catch (error) {
res.sendStatus(404);
}
};

const getRestaurantByName = async (req, res) => {
const { params: { restaurantName } } = req;
const getRestaurantByNameController = async (req, res) => {
const { params: { restaurantName } } = req;

try {
const result = await getRestaurantFromName(restaurantName);
res.send(result);
} catch (error) {
res.sendStatus(404);
}
try {
const result = await getRestaurantByName(restaurantName);
res.send(result);
} catch (error) {
res.sendStatus(404);
}
};


const hostnameCheck = async (req, res) => {
const { body: { hostname } } = req;
if (!hostname) {
res.sendStatus(404);
} else {
try {
const result = await getRestaurantFromHostname(hostname);
if(!result) res.sendStatus(404);
res.send(result);
} catch (error) {
res.sendStatus(404);
}
}
const getRestaurantByHostnameController = async (req, res) => {
const { body: { hostname } } = req;
if (!hostname) {
res.sendStatus(404);
} else {
try {
const result = await getRestaurantByHostname(hostname);
if (!result) res.sendStatus(404);
res.send(result);
} catch (error) {
res.sendStatus(404);
}
}
};

module.exports = {
getRestaurantById,
getRestaurantByName,
hostnameCheck,
getRestaurantByIdController,
getRestaurantByNameController,
getRestaurantByHostnameController,
};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to have changed the names of three functions for querying a restaurant in a data model and turned them into controller functions that can handle incoming requests from users. The changes mainly involve renaming the original function names to fit with the new naming conventions. Additional changes involve using more expressive, descriptive variable names for better readability.

One minor issue is that in the hostnameCheck function, if result is undefined, it sends back 404, which implies that the resource isn't found, which could be misleading. A better approach might be sending back 204 (no content) instead.

Overall, the code patch looks appropriate as long as there are no issues with the data models and the request handling of the endpoints works as intended.

61 changes: 33 additions & 28 deletions backend/app/controllers/eventsController/db/onOrderChange.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,43 @@
const { Orders } = require("../../../db/models");
const { Orders } = require('../../../db/models');

module.exports = async (mongoEvent, io) => {
if (mongoEvent.operationType === "insert") {
const ownerRoomName = `${mongoEvent.fullDocument.restaurantId}:owner`;
const tabRoomName = `${mongoEvent.fullDocument.restaurantId}:${mongoEvent.fullDocument.tabId}`;
//todo remove duplicate code from here. make a function that returns
//the order and use it here and in controllers
const order = await Orders.findById(mongoEvent.fullDocument._id)
.populate('items')
.populate({
path: 'tabId',
populate: {
path: 'tableId'
}
}).exec();
if (mongoEvent.operationType === 'insert') {
const ownerRoomName = `${mongoEvent.fullDocument.restaurantId}:owner`;
const tabRoomName = `${mongoEvent.fullDocument.restaurantId}:${mongoEvent.fullDocument.tabId}`;
//todo remove duplicate code from here. make a function that returns
//the order and use it here and in controllers
const order = await Orders.findById(mongoEvent.fullDocument._id)
.populate({
path: 'items',
populate: {
path: 'recipe'
}
})
.populate({
path: 'tabId',
populate: {
path: 'tableId'
}
}).exec();

io.to(ownerRoomName).emit("order:new", { order });
console.log(`New order sent in room: "${ownerRoomName}"`);
io.to(ownerRoomName).emit('order:new', { order });
console.log(`New order sent in room: "${ownerRoomName}"`);

io.to(tabRoomName).emit("order:new", { order });
console.log(`New order sent in room: "${tabRoomName}"`);
}
io.to(tabRoomName).emit('order:new', { order });
console.log(`New order sent in room: "${tabRoomName}"`);
}

if (mongoEvent.operationType === "update") {
const order = await Orders.findById(mongoEvent.documentKey._id);
const ownerRoomName = `${order.restaurantId}:owner`;
const tabRoomName = `${order.restaurantId}:${order.tabId}`;
if (mongoEvent.operationType === 'update') {
const order = await Orders.findById(mongoEvent.documentKey._id);
const ownerRoomName = `${order.restaurantId}:owner`;
const tabRoomName = `${order.restaurantId}:${order.tabId}`;

io.to(ownerRoomName).emit("order:update", { id: order._id, status: order.status });
console.log(`New order update sent in room: "${ownerRoomName}"`);
io.to(ownerRoomName).emit('order:update', { id: order._id, status: order.status });
console.log(`New order update sent in room: "${ownerRoomName}"`);

io.to(tabRoomName).emit("order:update", { id: order._id, status: order.status });
console.log(`New order update sent in room: "${tabRoomName}"`);
io.to(tabRoomName).emit('order:update', { id: order._id, status: order.status });
console.log(`New order update sent in room: "${tabRoomName}"`);


}
}
};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code patch seems to be functioning correctly. Here are some suggestions for improvement:

  1. In both insert and update conditions, it's good practice to include an else block to handle unexpected operation types.

  2. In the insert condition, the todo message suggesting that a function should be created to remove duplicate code is appropriate. This would improve readability and reduce potential bugs if this functionality is used in multiple places.

  3. In the update condition, it might be better to use the updated document rather than retrieving it from the database again with Orders.findById(mongoEvent.documentKey._id). This could save a significant performance cost in situations where there are many updates.

  4. It may be beneficial to include error handling code to gracefully handle database and socket connection errors.

  5. The lack of a newline at the end of the file may cause issues in some text editors. Adding a newline at the end of the file can avoid potential problems.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch is an event handler that listens to MongoDB change stream events and emits order updates to owner and table rooms via socket.io. Here are some improvement suggestions and bug risks to consider:

  1. Improve function organization: The duplicate code comment suggests a potential refactoring opportunity to extract a reusable function for retrieving orders. It would be good to separate this functionality into a standalone controller or service.

  2. Use consistent quotes: The code mixes single- and double-quotes for string literals. It's better to pick one style and use it consistently throughout the codebase.

  3. Add error handling: The code does not include any error handling logic. It's important to handle errors that may occur during database operations to prevent unexpected crashes and provide better feedback to end-users.

  4. Avoid magic strings: The code relies on hard-coded strings such as "insert" and "update" to determine the operation type. It would be better to define constants or enums to minimize the risk of typos and make the code more readable.

  5. Potential bug risk: In the update case, the code retrieves the updated order using mongoEvent.documentKey._id, which assumes that the _id field is present in the change stream event. If this assumption is incorrect, the code will fail.

  6. Consider using async/await: The format of the code is currently using callbacks with exec(). Async/await could simplify the code and make it easier to read.

  7. Logging improvements: Consider adding more context to the log messages, such as the event type (i.e., insert or update), to make it clearer what actions are being performed. Also, ensure consistent capitalization of log messages.

  8. Overall readability: Consider improving the formatting of the code as there are inconsistencies in indentations and spacing that can affect readability. Additionally, there are some unnecessary comments and commented-out code that can be removed.

  9. Potential performance optimization: The code currently populates the items and tabId fields of the order object in both the insert and update cases. It may be possible to optimize performance by selectively populating only the necessary fields based on the operation type to reduce the amount of data retrieved from the database.

87 changes: 43 additions & 44 deletions backend/app/controllers/eventsController/db/onTabChange.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,47 @@
const { Tabs } = require("../../../db/models");
const { Tabs } = require('../../../db/models');
const propagationDelay = 1000;

module.exports = async (mongoEvent, io) => {
if (mongoEvent.operationType === "insert") {
// Give some time for the document to be created and propagated
setTimeout(() => {
console.log(' ');

const tab = mongoEvent.fullDocument;
const ownerRoomName = `${tab.restaurantId}:owner`;

io.to(ownerRoomName).emit("tab:open", { tableId: tab.tableId, tabId: tab._id });
console.log(`[Events] [Delayed ${propagationDelay/1000}s] New tab open request sent in room: "${ownerRoomName}"`);
}, propagationDelay);
}

if (mongoEvent.operationType === "update") {

// Call waiter
if (mongoEvent.updateDescription.updatedFields?.callWaiter === 'called') {
console.log(' ');
const tab = await Tabs.findById(mongoEvent.documentKey._id)
.populate('tableId')
.exec();

const ownerRoomName = `${tab.restaurantId}:owner`;

io.to(ownerRoomName).emit("waiter:notification", { tableNo: tab.tableId.tableNo, tabId: tab._id });
console.log(`[Events] New waiter request sent in room: "${ownerRoomName}"`);
}

// Close tab
if (mongoEvent.updateDescription.updatedFields?.status === 'closed') {
console.log(' ');
const tab = await Tabs.findById(mongoEvent.documentKey._id);

const ownerRoomName = `${tab.restaurantId}:owner`;
const tabRoomName = `${tab.restaurantId}:${tab._id}`;

io.to(ownerRoomName).emit("tab:closed", { tableId: tab.tableId, tabId: tab._id });
console.log(`[Events] Tab closed sent in room: "${ownerRoomName}"`);

io.to(tabRoomName).emit("tab:closed", { tabId: tab._id });
console.log(`[Events] Tab closed sent in room: "${tabRoomName}"`);
}
}
if (mongoEvent.operationType === 'insert') {
console.log('Tab inserted');
// Give some time for the document to be created and propagated
setTimeout(() => {
const tab = mongoEvent.fullDocument;
const ownerRoomName = `${tab.restaurantId}:owner`;

io.to(ownerRoomName).emit('tab:open', { tableId: tab.tableId, tabId: tab._id });
console.log(`[Events] [Delayed ${propagationDelay/1000}s] New tab open request sent in room: "${ownerRoomName}"`);
}, propagationDelay);
}

if (mongoEvent.operationType === 'update') {

// Call waiter
if (mongoEvent.updateDescription.updatedFields?.callWaiter === 'called') {
console.log(' ');
const tab = await Tabs.findById(mongoEvent.documentKey._id)
.populate('tableId')
.exec();

const ownerRoomName = `${tab.restaurantId}:owner`;

io.to(ownerRoomName).emit('waiter:notification', { tableNo: tab.tableId.tableNo, tabId: tab._id });
console.log(`[Events] New waiter request sent in room: "${ownerRoomName}"`);
}

// Close tab
if (mongoEvent.updateDescription.updatedFields?.status === 'closed') {
console.log(' ');
const tab = await Tabs.findById(mongoEvent.documentKey._id);

const ownerRoomName = `${tab.restaurantId}:owner`;
const tabRoomName = `${tab.restaurantId}:${tab._id}`;

io.to(ownerRoomName).emit('tab:closed', { tableId: tab.tableId, tabId: tab._id });
console.log(`[Events] Tab closed sent in room: "${ownerRoomName}"`);

io.to(tabRoomName).emit('tab:closed', { tabId: tab._id });
console.log(`[Events] Tab closed sent in room: "${tabRoomName}"`);
}
}
};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code patch is a listener function that reacts to events happening in a Mongo database. The function reacts to two events: new tab creation and existing tab updates.

For new tab creation, the function sends a delayed request to open a new tab in a specific room over socket.io.

For existing tab updates, the function checks whether the callWaiter or status properties have changed to called and closed respectively. For a change in callWaiter, the function emits wait staff notification to the same room the new tab event was emitted earlier. When the status property changes to closed, the function emits an event to both the owner's room and the tab-specific room signaling that the tab has been closed.

The code logic appears solid without any obvious bugs being present. One minor suggestion would be to ensure that the indentation throughout the code is uniform to assist with code readability.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to be functional with no syntax errors. However, there are a few improvements that could be made:

  1. Code formatting - using consistent indentation and adding a newline at the end of the file would make the code more readable.

  2. Use strict equality operator (===) when comparing with a string rather than the loose equality operator (==).

  3. Add error handling - it's important to handle any error that may occur during the execution of the function.

  4. Consider refactoring the code - depending on the size or complexity of the application, separating the different functionality within this event listener into separate methods could make the code more modular, easier to test, and maintain.

61 changes: 34 additions & 27 deletions backend/app/controllers/eventsController/socket/onConnect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,42 @@
const { Restaurants } = require("../../../db/models");
const { Restaurants } = require('../../../db/models');

module.exports = async (socket) => {
if(socket.handshake.query.restaurantId){
const restaurant = await Restaurants.findById(socket.handshake.query.restaurantId);

//todo: implement proper authentication of owner
if(socket.handshake.query.owner === "true") {
console.log("Owner from restaurant " + restaurant.name + " connected!");
socket.join(`${socket.handshake.query.restaurantId}:owner`);
console.log(`Connected owner to room: "${socket.handshake.query.restaurantId}:owner"`);
if(socket.handshake.query.restaurantId){
let restaurant;
try {
restaurant = await Restaurants.findById(socket.handshake.query.restaurantId);
} catch (error) {
console.log(error);
}
if(restaurant){
//todo: implement proper authentication of owner
if(socket.handshake.query.owner === 'true') {
console.log('Owner from restaurant ' + restaurant.name + ' connected!');
socket.join(`${socket.handshake.query.restaurantId}:owner`);
console.log(`Connected owner to room: "${socket.handshake.query.restaurantId}:owner"`);

} else if (socket.handshake.query.tabId != null){
console.log("Client from restaurant " + restaurant?.name + " connected!");
} else if (socket.handshake.query.tabId != null){
console.log('Client from restaurant ' + restaurant?.name + ' connected!');

socket.join(`${socket.handshake.query.restaurantId}:globalNotifications`);
console.log(`Connected client to room (Global Notifications): "${socket.handshake.query.restaurantId}:globalNotifications"`);
socket.join(`${socket.handshake.query.restaurantId}:globalNotifications`);
console.log(`Connected client to room (Global Notifications): "${socket.handshake.query.restaurantId}:globalNotifications"`);

socket.join(`${socket.handshake.query.restaurantId}:${socket.handshake.query.tabId}`);
console.log(`Connected client to room (Tab): "${socket.handshake.query.restaurantId}:${socket.handshake.query.tabId}"`);
socket.join(`${socket.handshake.query.restaurantId}:${socket.handshake.query.tabId}`);
console.log(`Connected client to room (Tab): "${socket.handshake.query.restaurantId}:${socket.handshake.query.tabId}"`);

} else {
try {
delete socket.handshake.query.transport;
delete socket.handshake.query.EIO;
delete socket.handshake.query.t;
} catch (e) {
console.log("Error deleting query params from socket handshake: " + e);
}
console.log("Disconnected client that had the following query: " + JSON.stringify(socket.handshake.query));
socket.disconnect();
}
}
} else {
try {
delete socket.handshake.query.transport;
delete socket.handshake.query.EIO;
delete socket.handshake.query.t;
} catch (e) {
console.log('Error deleting query params from socket handshake: ' + e);
}
console.log('Disconnected client that had the following query: ' + JSON.stringify(socket.handshake.query));
socket.disconnect();
}
}

}

};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to be improving the existing code. Here are some observations:

  • The indentation style is inconsistent and may confuse developers.
  • The try-catch block is added when calling the Restaurants.findById() method, which is a good practice to handle errors that could occur while querying the database.
  • A check is added to see if the restaurant exists before proceeding with further operations. This is a good approach for avoiding null or undefined errors.
  • The owner/client logs and room joins seem to be implemented correctly, considering the current use case. However, proper authentication for the owner should be implemented in the future.
  • In the else section, deleting unnecessary query params from socket handshake is being done, which is okay as it helps avoid leaking sensitive information.
  • Overall, there don't seem to be any bug risks at the moment. However, depending on how this code will be used, there are still areas that can be improved.

4 changes: 2 additions & 2 deletions backend/app/controllers/eventsController/socket/onDisconnect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
module.exports = (socket) => {
console.log("Client disconnected");
};
console.log('Client disconnected');
};

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the provided code patch, there do not appear to be any significant bug risks or improvement suggestions.

The only change made to the code is the adjustment of indentation and spacing for the log message. The function simply logs a message when a client disconnects.

However, it's worth noting that the line "+\n" (no newline at end of file) could cause issues with some systems that expect a newline character at the end of files. Including a newline character after the closing curly brace could help prevent these issues.

2 changes: 1 addition & 1 deletion backend/app/controllers/eventsController/socket/onRequestPin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module.exports = (socket) => {
console.log("Request pin");
console.log('Request pin');
};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the code patch provided, it seems like a very minor change that replaces double quotes with single quotes and adds (or removes) a newline at the end of the file. There don't appear to be any major bug risks or improvement suggestions that can be made apart from one potential issue:

  • The code may not conform to an existing code style guide, which could impact readability and maintainability.

Some teams have set guidelines on how to format and style their code, for instance using semicolons instead of relying on Automatic Semicolon Insertion (ASI), spaces or tabs for indentation, single or double quotes for strings, or requiring a certain number of blank lines between functions. It's important to follow the coding standards established by the team, the organization, or the open source project so that everyone can read and edit the code without confusion or frustration.

20 changes: 10 additions & 10 deletions backend/app/controllers/eventsController/socket/onSendPin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
const { Tables } = require("../../../db/models");
const { Tables } = require('../../../db/models');

module.exports = async (socket, data) => {
const { pin, tableId } = data;
if(!pin || !tableId) {
socket.emit("tableLockStatus", { error: "Invalid pin or tableId" });
return;
}
const { pin, tableId } = data;
if(!pin || !tableId) {
socket.emit('tableLockStatus', { error: 'Invalid pin or tableId' });
return;
}

const table = await Tables.findById(tableId);
if(table.pin === pin) {
socket.emit("tableLockStatus", { });
}
const table = await Tables.findById(tableId);
if(table.pin === pin) {
socket.emit('tableLockStatus', { });
}
};
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to be a minor change in code formatting, changing double quotes to single quotes and adding whitespace. Therefore there are no observable bug risks.

There is a small improvement suggestion to add a safety check before accessing the table object properties to avoid any potential runtime errors due to undefined values. The check can be added like this:

const table = await Tables.findById(tableId);
if (table && table.pin === pin) {
  socket.emit('tableLockStatus', {});
} else {
  socket.emit('tableLockStatus', { error: 'Invalid pin or tableId' });
}

It's also recommended to have a new line at the end of the file, but this can be considered a personal preference.

Loading