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

Added bot message for the first time praisers #516

Merged
merged 5 commits into from
Jul 26, 2022

Conversation

nebs-dev
Copy link
Contributor

@nebs-dev nebs-dev commented Jul 18, 2022

Resolves #459

@nebs-dev nebs-dev requested a review from kristoferlund July 18, 2022 08:07
@nebs-dev nebs-dev self-assigned this Jul 18, 2022
@nebs-dev nebs-dev changed the title WIP: Added bot message for the first time praisers Added bot message for the first time praisers Jul 20, 2022
@nebs-dev nebs-dev changed the base branch from development to main July 20, 2022 13:38
@@ -67,6 +68,9 @@ export const praiseHandler: CommandHandler = async (
}

const userAccount = await getUserAccount(member as GuildMember);
const praiseItems = await PraiseModel.find({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q) Wouldn't this become an expensive call?
Could we send this message to users when they activate their praise account instead of sending it on first praise?

__
sema-logo  Summary: ❓ I have a question  |  Tags: Inefficient

Copy link
Member

Choose a reason for hiding this comment

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

We don't have any huge performance concerns here since praising is relatively infequent. But yes, this does seem like an overly expensive call. To begin with I'd suggest we try doing PraiseModel.countDocuments( instead.

Copy link
Collaborator

@Vyvy-vi Vyvy-vi Jul 26, 2022

Choose a reason for hiding this comment

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

that should make this more feasible 👍

packages/discord-bot/src/utils/praiseEmbeds.ts Outdated Show resolved Hide resolved
Copy link
Member

@kristoferlund kristoferlund left a comment

Choose a reason for hiding this comment

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

@nebs-dev see comment from @Vyvy-vi regarding expensive call.

(I also made a small text change based on the other comment by @Vyvy-vi)

@nebs-dev nebs-dev requested a review from kristoferlund July 26, 2022 08:29
Copy link
Member

@kristoferlund kristoferlund left a comment

Choose a reason for hiding this comment

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

Great! ✨

@kristoferlund kristoferlund merged commit a3df01c into main Jul 26, 2022
@kristoferlund kristoferlund deleted the enh/bot_first_time_praisers branch July 26, 2022 15:54
@kristoferlund kristoferlund mentioned this pull request Aug 24, 2022
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.

Bot message to first time praisers
3 participants