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.

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

Refactored and made the code look better #9

Merged
merged 18 commits into from
Jul 22, 2023
Merged

Refactored and made the code look better #9

merged 18 commits into from
Jul 22, 2023

Conversation

RomaDevWorld
Copy link
Contributor

So I basically removed highly deeply nested code
I also reformatted the code using prettier to make it more readable, and ESLint to fix some errors

I recommend you cloning my code and testing it before merging.

Have a good one!

@Arisamiga Arisamiga added documentation Improvements or additions to documentation enhancement New feature or request labels Jul 20, 2023
@Arisamiga Arisamiga self-requested a review July 20, 2023 10:37
events/messageCreate.js Outdated Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
Copy link
Owner

@Arisamiga Arisamiga left a comment

Choose a reason for hiding this comment

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

Thank you for contributing to the repository! :D

There seem to be some problems that are preventing the bot from running accordingly.

I have outlined some of the problems above but other problems are:

  • Bot always thinks there is an ongoing ticket and because of that, it doesn't create a channel.
  • Bot deleting collector even if it exists
  • Reactions not being handled correctly
  • eslint not being configured with prettier so it could conflict with prettier
  • Some of the if statements are incorrectly handled

If these changes worked on your setup please be sure that you are including all the changes 🙂

@RomaDevWorld
Copy link
Contributor Author

I made some changes

  1. Fixed checkMessage in main.js by adding id property to messages.fetch and removing cache between channels.fetch
  2. Fixed message.content.startsWith in messageCreate.ts by adding a singe exclamation mark
  3. Added prettier plugin to .eslintrc.js so it will work properly with prettier

if (messageReaction.emoji.name === '📩') {
let channelname = `ticket-${user.username}`;
channelname = channelname.replace(/\s/g, '-').toLowerCase();
if (!messageReaction.message.guild.channels.cache.find((channel) => channel.name === channelname) || !config.one_app) {
Copy link
Owner

Choose a reason for hiding this comment

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

This if statement should not have !'s and should not be a ||

Copy link
Owner

@Arisamiga Arisamiga left a comment

Choose a reason for hiding this comment

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

There are still some issues that I have highlighted above but also the following:

  • There are a lot of if statements that have "!" that shouldn't be there
  • Bot deleting collector even if it exists
  • Some of the if statements are incorrectly handled
  • Reactions are not being handled based on the role

Please be sure to first test it before requesting a review 🙂

@RomaDevWorld
Copy link
Contributor Author

So.. Tell me again, why do you prefer using client.channels.fetch instead of client.channels.cache.get?

The thing is that in the first case if the channel doesn't exist it will throw an error and crash an entire application.
I'll use client.channels.cache.get since it works just fine :)

@RomaDevWorld
Copy link
Contributor Author

  1. Collectors and tickets won't be deleted if they still exist
  2. Only authorized roles and “Admins” can close/delete ticket
  3. The misunderstood "if" statement was fixed and now works as expected.

P.S: I've tested it, and haven't found any issues

Fell free to point me to any new problems you'll find!

main.js Outdated
const checkMessage = async (id, channelid) => {
const channel = await client.channels.cache.get(channelid);
if (!channel) return;
const channelMessage = await channel?.messages.fetch(id);
Copy link
Owner

Choose a reason for hiding this comment

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

Might want to add a .catch to prevent the application from crashing because if there is an error it is most likely because the message doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if message doesn't exist the channelMessage will just be undefined. No error here.

Copy link
Owner

Choose a reason for hiding this comment

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

If the message doesn't exist the channelMessage will crash the application because Discord returns a 404 error code with DiscordAPIError: Unknown Message so I suggest adding a .catch there.

main.js Show resolved Hide resolved
let channel = messageReaction.message.guild.channels.cache.find((channel) => channel.id === messageReaction.message.channel.id);
switch (messageReaction.emoji.name) {
case '🔒':
if (!checkUser(messageReaction.message, user) || !config.allow_user_lock) {
Copy link
Owner

Choose a reason for hiding this comment

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

Since this uses "!" you should use "&&" not "||"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why tho? Am I missing something?

If the user is not authorized to close the channel, or config doesn't allow this then respond with "Only Staff can lock the channels"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see.

(Allow users to lock their own tickets)

return messageReaction.message.channel.send('Channel Locked 🔒');
case '⛔':
if (!messageReaction.message.guild.channels.cache.find((c) => c.name.toLowerCase() === channel.name)) return;
if (!checkUser(messageReaction.message, user) || !config.allow_user_delete) {
Copy link
Owner

Choose a reason for hiding this comment

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

Since this uses "!" you should use "&&" not "||"

Copy link
Owner

@Arisamiga Arisamiga left a comment

Choose a reason for hiding this comment

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

There are some minor issues which I have outlined above!

Also might wanna install the config for prettier: ESLint couldn't find the config "prettier" to extend from. :)

@RomaDevWorld
Copy link
Contributor Author

RomaDevWorld commented Jul 22, 2023

Made the changes that you mentioned.
Also added nodemon to automatically refresh the application on file save
npm run dev

Copy link
Owner

@Arisamiga Arisamiga left a comment

Choose a reason for hiding this comment

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

Thanks so much for contributing 🙂 Everything is working Great!

@Arisamiga Arisamiga merged commit 95dc1d4 into Arisamiga:main Jul 22, 2023
@RomaDevWorld
Copy link
Contributor Author

I also recomend you learning and using typescript in your future javascript projects. It can be a little bit hard to learn and understand, but when you do it will make your projects 10x better!

Stay safe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants