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

feat(discord-bot): Implement Discord Bot Prototype #468

Open
wants to merge 14 commits into
base: beta
Choose a base branch
from

Conversation

PongDev
Copy link
Member

@PongDev PongDev commented Jan 25, 2023

Why did you create this PR

  • Implement New Discord Bot for CU Get Reg

What did you do

  • Implement Discord Bot Prototype

Demo

https://dev.cugetreg.com

Checklist

  • Deploy a demo
  • Check browsers compatibility
  • Wrote coverage tests

@leomotors
Copy link
Member

leomotors commented Jan 25, 2023

Please fix conflict so I can review

don't force push

@saenyakorn
Copy link
Member

Can you explain more about the features of this bot. It would be great if you put some image what bot can do here.

Please explain as much as possible you can. So, we'll review this feature later

@leomotors leomotors changed the title [Discort-Bot] Implement Discort Bot Prototype feat(discord-bot): Implement Discord Bot Prototype Jan 25, 2023
@PongDev
Copy link
Member Author

PongDev commented Jan 25, 2023

Please fix conflict so I can review

don't force push

I merge wrong branch ;w;

@PongDev
Copy link
Member Author

PongDev commented Jan 25, 2023

Can you explain more about the features of this bot. It would be great if you put some image what bot can do here.
Please explain as much as possible you can. So, we'll review this feature later

This is a prototype of a discord bot, for now it main feature is summary user usage for 1 week, however the core module of this bot support many metric and dimension for used in further.

This prototype design based on issue 439. (#439)

Example feature
image

@saenyakorn
Copy link
Member

What does the report look like when the bot send a it into Discord?

@PongDev
Copy link
Member Author

PongDev commented Jan 25, 2023

image

Look like this for example, actually it support both text message and image but text message format is not yet discuss to other, so it has core code and template for that for now. However from the code, text command can be code in very short time (estimate may be < 1 week)

image

The bot use slash command to register channel to send report and report will send at specific cron job time

@PongDev
Copy link
Member Author

PongDev commented Jan 25, 2023

Note that this is only a prototype of CU Get Reg Bot, for further improvement I think we should discuss for more further information to used in development

package.json Outdated
Comment on lines 136 to 141
"slate-react": "^0.82.0",
"styled-components": "^5.3.5",
"tough-cookie": "^4.0.0",
"tslib": "^2.4.0",
"uuid": "^8.3.2",
"winston": "^3.8.2",
Copy link
Member

Choose a reason for hiding this comment

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

why are these guys moved to devDependencies? these are prod deps

Copy link
Member Author

Choose a reason for hiding this comment

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

I will move it, it may due to fix merge conflict

Comment on lines +1 to +18
{
"extends": ["../../.eslintrc.json"],
"ignorePatterns": ["!**/*"],
"overrides": [
{
"files": ["*.ts", "*.tsx", "*.js", "*.jsx"],
"rules": {}
},
{
"files": ["*.ts", "*.tsx"],
"rules": {}
},
{
"files": ["*.js", "*.jsx"],
"rules": {}
}
]
}
Copy link
Member

Choose a reason for hiding this comment

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

what is this file here for? doesn't this literally ignore every error?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is copy from apps/api/.eslintrc.json

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you recommend how I should lint this apps?

Copy link
Member

Choose a reason for hiding this comment

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

@saenyakorn did u add this in api?

Comment on lines 54 to 56
get commands(): Map<string, ICommand> {
return this.commandList
}
Copy link
Member

Choose a reason for hiding this comment

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

it's best if you keep the getter and private variable the same name, but prefix private var with underscore _.

Suggested change
get commands(): Map<string, ICommand> {
return this.commandList
}
get commands(): Map<string, ICommand> {
return this._commands
}

Copy link
Member Author

@PongDev PongDev Jan 26, 2023

Choose a reason for hiding this comment

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

I will do as your recommended. Also can you provided me convention reference, so I can know more info.

Copy link
Member

@bombnp bombnp Jan 27, 2023

Choose a reason for hiding this comment

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

i don't really have it, it's what i've seen and it really is easier to understand

apps/discord-bot/src/core/CUGetRegCommands.ts Outdated Show resolved Hide resolved
import { PingCommand } from '../command/ping'
import { RegisterReportChannelCommand } from '../command/registerReportChannel'

export const CUGetRegCommands: ICommand[] = [PingCommand, RegisterReportChannelCommand]
Copy link
Member

Choose a reason for hiding this comment

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

Also i think it's unnecessary to prefix everything with CUGetReg. Leave things as like commands, hooks, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see

apps/discord-bot/src/core/CUGetRegScheduler.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,53 @@
import { GoogleAnalyticData } from './google-analytic'

export class GoogleAnalyticDataExtended extends GoogleAnalyticData {
Copy link
Member

Choose a reason for hiding this comment

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

why not just put this in GoogleAnalyticData if we're only gonna use the extended version only?

Copy link
Member

Choose a reason for hiding this comment

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

especially since GoogleAnalyticData is our own class.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for reusable of class, since GoogleAnalyticsData is core system of google analytics which directly use google analytics api, however GoogleAnalyticsDataExtended is only a set of command that a shortcut of calling core analytics command.

So the main reason is to separate between the necessary one and the shortcut one, however any other solution is accept if you have recommended.

Copy link
Member

Choose a reason for hiding this comment

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

There's literally no reason to split up the code into an "Extended" version, when the subclass is already our code in our repo and you're always going to use the extended one. This is way harder to read imo. Put it in the same class.

In fact, the only core method that interfaces directly with the google analytics api is the getMetrics method, which it literally just an http client.

}

try {
await command.execute(client, interaction)
Copy link
Member

Choose a reason for hiding this comment

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

no need to await, command.execute(..) is not async func
image

apps/discord-bot/src/hook/ready.ts Show resolved Hide resolved
}

get db(): IDatabase {
return this.database
return this._database
Copy link
Member

Choose a reason for hiding this comment

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

same goes here

Comment on lines 54 to 56
get commands(): Map<string, ICommand> {
return this.commandList
return this._commandList
}
Copy link
Member

Choose a reason for hiding this comment

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

i meant get commands() should match the private variable name _commands not _commandList

@bombnp
Copy link
Member

bombnp commented Jan 27, 2023

Why is there so many package.json changes (package name ordering is out of place, versions are upgraded)? That should not happen when you add new packages for your new app.

Since we're using the same package.json for every single app, changing so many dependencies could break other apps and should be avoided.

@bombnp
Copy link
Member

bombnp commented Jan 27, 2023

In fact, i'd say do this:

  1. Delete node_modules folder
  2. Revert package.json and pnpm lock file back to its original commit
  3. Selectively add new packages that you need for discord bot (prod dependencies and dev dependencies). This should NOT mess up other dependencies.
  4. Commit and push new package.json and lock file to this branch

@saenyakorn
Copy link
Member

Because we may have other features for Discord bot like #438, I would like you to use discord-nestjs, the powerful DiscordJS-wrapper library based on Nestjs, as a main library for implementing Discord bot. Since, Nestjs is fully come with Typescript and Decorator that make other easy to read through the code.

@saenyakorn
Copy link
Member

saenyakorn commented Jan 27, 2023

Here is the example repos that implemented with discord-nestjs. Feel free to copy the code from them.

@bombnp
Copy link
Member

bombnp commented Jan 27, 2023

hold up this might conflict package.json with turborepo migration in #470

@PongDev
Copy link
Member Author

PongDev commented Jan 30, 2023

hold up this might conflict package.json with turborepo migration in #470

do you mean hold the pull request or hold the migration?

@bombnp
Copy link
Member

bombnp commented Jan 30, 2023

hold up this might conflict package.json with turborepo migration in #470

do you mean hold the pull request or hold the migration?

hold the PR, as it would conflict the package.json a lot. U could keep on implementing the bot tho (like as @saenyakorn suggested), just be prepared to fix the package.json conflict

@bombnp
Copy link
Member

bombnp commented Feb 2, 2023

@PongDev #470 is merged.

@bombnp
Copy link
Member

bombnp commented Feb 2, 2023

u'll need to create a new package.json in your app directory with your own dependencies. No more piling every dependencies into single package.json at root.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants