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

Invite management #297

Merged
merged 10 commits into from
Mar 14, 2024
Merged

Invite management #297

merged 10 commits into from
Mar 14, 2024

Conversation

zuuring
Copy link
Member

@zuuring zuuring commented Mar 7, 2024

Notes

This adds a new feature to valkyrie for managing invites into discord both for create and deleting invite.

You can use the /create-invite command which will generate an invite for the channel that the command is being sent from. With the default settings of 7 days expiry and 10 uses.

Defense implementation

This now includes an interaction discordClient.on("channelCreate") to check if a channel has been made with ext-*-audit format and will generate an invite link into that channel based off this. This will also create a matching role with channel access called Defense: role

Screenshots

Screenshot 2024-03-12 at 15 37 06

Role assignments

Since this is only the component to create/delete invites, we would need to still tie this together with the role-management.ts implementation. That being that when a new member joins the discord server, we monitor the discordClient.on("GuildMemberAdd") event and check the inviteID used, then assign role tied to that. Whether that should always assume the channel to which an invite is generated a role should be assigned to that once the invite is used (?).

To-do

  • Setup on test discord
  • Automated case example, invites on specific channel creation
  • Add global command /create-invite
  • Linking to role assignment (?)

zuuring added 2 commits March 7, 2024 12:10
### Notes
This adds a new feature to valkyrie for  managing invites into discord both for create and deleting invite.

Currently there is a hardcoded command `!create-invite` that will generate a new invite for the specific channel the command was iniatied in.

With the default settings of 24 hours expiry and 10 uses, however we can further this by defining set parameters. More or less just a PoC implementation on creating an invite based on the set parameters.
This adds a implementation into the invite script which generates an invite to the specific channel based of a channel being made in the defense category with `ext-*-audit` format.

To-do: Link with role-manager to whirl up a role with the same channel name, granting access to that channel only.
@zuuring
Copy link
Member Author

zuuring commented Mar 8, 2024

Just added the ext-*-audit channel invite functionality. Generates an invite to the specific channel based of a channel being made in the defense category with ext-*-audit format.

To-do: Link with role-manager to whirl up a role with the same channel name, granting access to that channel only.

zuuring added 4 commits March 11, 2024 14:01
This commit goes through and resolves the following:
- Remove `action: delete` from invite script
- Refactor to pull `guildID` directly into create invite function
- Clean up code and redudent string / calls
This commit adds a new discord command `/create-invite` that allows you to generate a new invite within the channel that the command is run from.

Max age / max uses: Included are two parameters, `max-age` and `max-uses` that optionally let you specify the amount of time the invite link will be available for, and the number of invites. By default this will be set to 24 hours and for only 10 uses.
Once a new `ext-*-audit` channel has been created, this will now create a role with the exact same name as the channel, and assign permissions to only view that channel.

Now will need to get a role assignment going when a new user joins the server based off the invite.
@zuuring
Copy link
Member Author

zuuring commented Mar 11, 2024

Added the permission / role creation in the last commit, so now when ext-*-audit channel is created, we create a role with the exact same name and grant it permission to that channel.

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Left a bunch of thoughts. This feels very close to deployable now tbh.

discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
const invite = await channelOrigin.createInvite({
maxAge: numericMaxAge,
maxUses: numericMaxUses,
unique: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. Wonder what happens if this is off lol.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Shadowfiend Looks like it defaults to 24 hours, infinite uses = 0 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I just meant the unique property.

discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
zuuring added 2 commits March 12, 2024 15:31
This commit adds the following major changes

- Fix `globals.ts` to have proper seconds (since it was calculating based off milliseconds and not seconds). Updated `github-auth.ts` to reflect this change.

- Setup proper channel role mapping. Now it will name the new role `Defense: clientName` based on the `ext-channel-audit` name pattern.

- Fix typescript issues

- Remove max-age/max-uses from `/create-invite` command
@zuuring zuuring requested a review from Shadowfiend March 12, 2024 13:35
@zuuring
Copy link
Member Author

zuuring commented Mar 12, 2024

@Shadowfiend One thing to note on the globals.ts file. I fixed this since SECOND was set to 1000 instead of 1, making all day, week, month calculations incorrect., so added in MILLISECOND = 1000 and updated the ref in github-auth.ts. Can retract this change if it's worth saving for another PR.

@zuuring zuuring marked this pull request as ready for review March 12, 2024 13:40
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Left a handful more comments; the only blocker here is the client name handling for the role—let's get that fixed up and then we can ship, the rest can happen in another PR.

discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
Comment on lines 95 to 96
const maxAge = WEEK
const maxUses = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point we've repeated our default age and uses 3 times. I think what I'd suggest here is to make createInvite return an object ({ url: string; maxAge: number; maxUses: number }) and be entirely in charge of the default age and uses (using its parameter defaults). That way we cut most of this down.

I would also consider making createInvite take a TextChannel instead of a channel id, since all callers have access to the object. That way you can also eliminate the extra channel lookup by id that we do in createInvite.

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've got this all fixed now so we only have those defaults on maxAge / maxUses in the main func. Now they are set as they should be as parameter defaults, so if needed we can easily add custom values down the road. And instead of passing channel.id, it just sends the channel.object and runs invite from there.

discord-scripts/invite-management.ts Outdated Show resolved Hide resolved
lib/globals.ts Outdated
@@ -1,3 +1,7 @@
export const HOST = process.env.HUBOT_HOST
export const SECOND = 1000
export const MILLISECOND = 1000
export const SECOND = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be 1 * MILLISECOND—if we need to do a division later on by MILLISECOND to have the right precision for e.g. the Discord API, let's do that there.

We can probably drop a comment on these indicating that they're in millisecond precision.

The reasoning to do that rather than reindex everything against seconds is:

  • More precision is good in many cases, until it's time to drop to lower precision.
  • 30 * MILLISECOND (as used below) isn't correct—we're not setting an interval for 30 milliseconds, but rather for 30 seconds. The goal of the constants here is so that the phrase that uses it reflects the intent, and to do that across most use cases successfully, we need to use the higher precision as the basis.

Copy link
Member Author

@zuuring zuuring Mar 13, 2024

Choose a reason for hiding this comment

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

Ok that makes sense! So then for the Discord API, since we need exact seconds for DAY, WEEK instances we will have to do WEEK / MILLISECOND in-order to get the desired maxAge: 604800 vs 604800000 by default. Unless you'd prefer we just add that into the global.ts scope, something along the lines of WEEK_IN_SECONDS.

Since otherwise DiscordAPI comes back with DiscordAPIError[50035]: Invalid Form Body max_age[NUMBER_TYPE_MAX]: int value should be less than or equal to 604800.

Edit: I've actually just updated the main invite-management.ts with a / MILLISECOND to account for this if that makes more sense!

discord-scripts/invite-management.ts Show resolved Hide resolved
zuuring added 2 commits March 13, 2024 13:34
This commits goes through the review changes and resolves these issues:

- Remove extra consts on `maxAge/maxUses`
- Rather than passing `channel.id`, pass the object instead
- Suggested fix on split/slice for audit channel name (thank youuu!)
Make sure error logging in place incase client-name not found, skip role creation
@zuuring zuuring requested a review from Shadowfiend March 13, 2024 12:27
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

This looks good. I think I'm going to… Land it?

channelId: string,
maxAge = WEEK,
channel: TextChannel,
maxAge = (1 * WEEK) / MILLISECOND,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking, but let's do this conversion inside, when we call channel.createInvite—so the caller can always pass things and receive things in terms of our base time unit (the ms) throughout the codebase, and we only convert when we touch the external entity that requires a different denomination.

@@ -1,6 +1,6 @@
export const HOST = process.env.HUBOT_HOST
export const MILLISECOND = 1000
export const SECOND = 1
export const SECOND = 1 * MILLISECOND
Copy link
Contributor

Choose a reason for hiding this comment

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

Realizing this should really be phrased as:

export const MILLISECOND = 1 // Base unit of time for our code.
export const SECOND = 1000 * MILLISECOND
...

Comment on lines +112 to +115
if (clientName) {
const roleName = clientName
? `Defense: ${clientName}`
: `Defense: ${channel.name}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we have the if (clientName) check now, I think this can just be:

Suggested change
if (clientName) {
const roleName = clientName
? `Defense: ${clientName}`
: `Defense: ${channel.name}`
if (clientName) {
const roleName = `Defense: ${clientName}`

@Shadowfiend Shadowfiend merged commit 17cb5f1 into main Mar 14, 2024
7 checks passed
@Shadowfiend Shadowfiend deleted the invite-management branch March 14, 2024 02:04
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.

2 participants