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/317/verbose /admin announce summary #441

Merged
merged 10 commits into from
Jun 15, 2022

Conversation

Vyvy-vi
Copy link
Collaborator

@Vyvy-vi Vyvy-vi commented Jun 3, 2022

Resolves #317

This PR adds a summary report file that is sent when the /admin announce command is used. We might need to refactor this a bit in future and use the praise username instead of discord ids.

Screenshot 2022-06-04 at 12 27 33 AM
Screenshot 2022-06-04 at 12 27 49 AM

Copy link
Collaborator

@mattyg mattyg left a comment

Choose a reason for hiding this comment

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

Awesome!

I think we need to replace the discord ids with user names before it effectively resolves #317.

@Vyvy-vi
Copy link
Collaborator Author

Vyvy-vi commented Jun 7, 2022

I think we need to replace the discord ids with user names before it effectively resolves #317.

This can be resolved when #377 is implemented. We can't use discord usernames if the user isn't present in the discord.
One temp. solution may be to fetch all the UserAccounts associated with the User and to return a more detailed output -

User DiscordUsername TelegramUsername Status Reason
A A_on_discord_old_account A_on_Telegram Undelivered User Not Found in discord
B B_on_discord B_on_Telegram Delivered -
C C_on_discord C_on_Telegram Undelivered Can't DM User

Would this add a significant overhead to the process?

@kristoferlund
Copy link
Member

This can be resolved when #377 is implemented. We can't use discord usernames if the user isn't present in the discord.

How could the user not be on Discord? In order to be a quantifier you need to activate your account, linking UserAccount to User.

@Vyvy-vi
Copy link
Collaborator Author

Vyvy-vi commented Jun 8, 2022

How could the user not be on Discord? In order to be a quantifier, you need to activate your account, linking UserAccount to User.

People can leave the discord server after activating, or they can be kicked/banned for some reason, or their discord account might have been deleted.

@kristoferlund
Copy link
Member

People can leave the discord server after activating, or they can be kicked/banned for some reason, or their discord account might have been deleted.

Sure. But we will still have a UserAccount in our database we can use to provide a username.

@Vyvy-vi
Copy link
Collaborator Author

Vyvy-vi commented Jun 9, 2022

That's true. It might add some extra API calls, but we can add that 👍

@Vyvy-vi
Copy link
Collaborator Author

Vyvy-vi commented Jun 10, 2022

The code isn't as DRY as I'd like, however, I think we can work on that when we make a separate function for tabularisation (or if we opt to use a CSV file, instead).

Here's the new version -
Screenshot 2022-06-11 at 12 59 17 AM
Screenshot 2022-06-11 at 12 58 57 AM

@Vyvy-vi Vyvy-vi requested a review from mattyg June 10, 2022 19:37
@Vyvy-vi
Copy link
Collaborator Author

Vyvy-vi commented Jun 10, 2022

also, how to type this properly? -

interface FailedToDmUsersList {
  invalidUsers: string[];
  notFoundUsers: string[];
  closedDmUsers: string[];
  unknownErrorUsers: string[];
}

const failed: FailedToDmUsersList = {
  invalidUsers: <string[]>[],
  notFoundUsers: <string[]>[],
  closedDmUsers: <string[]>[],
  unknownErrorUsers: <string[]>[]
};

__
sema-logo  Summary: 🛠️ This code needs a fix

@mattyg
Copy link
Collaborator

mattyg commented Jun 12, 2022

also, how to type this properly? -

Since you already defined the type in the interface, you can just create an object that implements that type. The type string[] allows for an empty array (as long as any items you subsequently add to it are strings).

So like this:

interface FailedToDmUsersList {
  invalidUsers: string[];
  notFoundUsers: string[];
  closedDmUsers: string[];
  unknownErrorUsers: string[];
}

const failed: FailedToDmUsersList = {
  invalidUsers: [],
  notFoundUsers: [],
  closedDmUsers: [],
  unknownErrorUsers: []
};

mattyg
mattyg previously approved these changes Jun 12, 2022
Copy link
Collaborator

@mattyg mattyg left a comment

Choose a reason for hiding this comment

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

awesome!

@kristoferlund kristoferlund self-requested a review June 15, 2022 07:46
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.

Looks great @Vyvy-vi! I made two small text changes. Now merging.

@kristoferlund kristoferlund merged commit e9e2b2a into main Jun 15, 2022
@kristoferlund kristoferlund deleted the feat/317/verbose-announce-summary branch June 15, 2022 10:50
@kristoferlund kristoferlund mentioned this pull request Jun 20, 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.

verbose output from praise bot when sending mass messages
3 participants