-
Notifications
You must be signed in to change notification settings - Fork 2
Adds Latest Fixes from sfreeman422/mocker #1
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.
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
Conversation
* Removed unnecessary console log * Converted muzzled array to a Map and added a muzzlers Map to track requestors. Added corresponding tests * Added error strings to tests * Removed isMuzzled references in favor of muzzled.has * Removed unnecessary types * Added a 10 message limit to muzzled users * Adjusted linting options in package.json * Added precommit linting for tests also * Changed MAX_SUPPRESIONS to 7
* Bumps axios to avoid security vulnerabilities * adjusted package-lock.json
* Added max muzzles per hour feature * Converted to window.setTimeout and window.clearTimeout * Added proper types to take advantage of NodeJS.Timeout * Added tests for removeMuzzler
* Converted to window.setTimeout and window.clearTimeout * Added proper types to take advantage of NodeJS.Timeout * Fixed muzzle max issue
* added timeout for muzzle removal * Added timeout amount
| web.chat.delete(deleteRequest).catch(e => console.error(e)); | ||
|
|
||
| web.chat.postMessage(postRequest).catch(e => console.error(e)); | ||
| if (muzzled.get(request.event.user)!.suppressionCount < MAX_SUPPRESSIONS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might just be a dumbass but what exactly is this funky syntax doing muzzled.get(request.event.user)!👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essentially, it is a guard that ensure that the result of the get is not undefined. Without this, TS throw an error stating that the result of the get could be undefined... Which is kind of frustrating because we explicitly check that this key exists in the map on line 26.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe lodash get would be a little easier to read? 🤷♂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get(muzzled.get(request.event.user), 'suppressionCount', 0)
| const userId: string = getUserId(request.text); | ||
| const userName: string = getUserName(request.text); | ||
| console.log(request); | ||
| try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is a little weird to me... Why is there a try catch here inside of this synchronous function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are worried about accessing a property on an undefined value perhaps lodash get should be used instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this is because it is possible that addUserToMuzzled returns an error in which case i want to send that error message back to the user, and only the message, not the full error object because the message is the only thing that the user is concerned with. Things like You muzzled too much in the past hour, or That person is already muzzled. Maybe there is a cleaner way to achieve the same functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I try and void the use of try catch basically altogether (as well as in async funcs) but maybe thats just a style choice (I have heard that JS is not optimized to handle try catch blocks). I prefer to handle errors explicitly exactly where they occur instead of having a big dumpster catch block although in this exact case it looks like the error can only happen in one place at the invocation of addUserToMuzzled() so it doesnt really matter
You could do something like this:
export function addUserToMuzzled(
toMuzzle: string,
friendlyMuzzle: string,
requestor: string
) {
return new Promise((resolve, reject) => {
const timeToMuzzle = Math.floor(
Math.random() * (180000 - 30000 + 1) + 30000
);
const minutes = Math.floor(timeToMuzzle / 60000);
const seconds = ((timeToMuzzle % 60000) / 1000).toFixed(0);
if (muzzled.includes(toMuzzle)) {
console.error(
`${requestor} attempted to muzzle ${toMuzzle} but ${toMuzzle} is already muzzled.`
);
return reject(`${friendlyMuzzle} is already muzzled!`);
} else if (muzzled.includes(requestor)) {
console.error(
`User: ${requestor} attempted to muzzle ${toMuzzle} but failed because requestor: ${requestor} is currently muzzled`
);
return reject(`You can't muzzle someone if you are already muzzled!`);
} else {
muzzled.push(toMuzzle);
console.log(
`${friendlyMuzzle} is now muzzled for ${timeToMuzzle} milliseconds`
);
setTimeout(() => removeMuzzle(toMuzzle), timeToMuzzle);
return resolve(
`Succesfully muzzled ${friendlyMuzzle} for ${
+seconds === 60
? minutes + 1 + "m00s"
: minutes + "m" + (+seconds < 10 ? "0" : "") + seconds + "s"
} minutes`
);
}
});
}
muzzleRoutes.post("/muzzle", (req: Request, res: Response) => {
const request: ISlashCommandRequest = req.body;
const userId: string = getUserId(request.text);
const userName: string = getUserName(request.text);
const results = addUserToMuzzled(userId, userName, request.user_id).catch(e =>
res.send(e.message)
);
if (results) res.send(results);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gunna merge this one and do a separate PR for the stuff you mentioned here because I like this better than what I was doing
Adds the following features: