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

151 in-person, hybrid, and virtual support #217

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

liumegan228
Copy link
Collaborator

No description provided.

Comment on lines 480 to 494
helperRoles.forEach((role) => {
this._queues.map(async (virtualQueue, id) => {
if (virtualQueue.queueName === role) {
const inPersonCollection = this._in_person_queues.get(id);
if (inPersonCollection) {
if (inPersonCollection.has(helper.room)) {
const inPersonQueue = inPersonCollection.get(helper.room);
await inPersonQueue?.closeQueue(helperMember);
if (inPersonQueue?.activeHelperIds.size === 0) {
inPersonCollection.delete(helper.room);
}
}
}
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this intentional? This will create a bunch of promise arrays but doesn't wait for them to either reject or resolve

Copy link
Collaborator

@tomli380576 tomli380576 Apr 5, 2024

Choose a reason for hiding this comment

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

line 484 and 485 can be combined into

if (inPersonCollection?.has(helper.room))

and you can use non-null assert safely on 486

const inPersonQueue = inPersonCollection.get(helper.room)!;

to get rid of all the ?. that come afterwards

* @throws {ServerError} if no such queue
*/
getInPersonQueueById(parentCategoryId: Snowflake, room: string): HelpQueue {
const collection = this._in_person_queues.get(parentCategoryId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: camelCase not snake_case

.getQueueById(queueChannel.parentCategoryId)
.removeStudent(interaction.member);
await Promise.all(
server.getAllQueuesWithHelpee(queueChannel.parentCategoryId, interaction.member)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this remove the student from ALL the queues they are in?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh nvm it removes the student from all queues under related to that queueChannel

@@ -260,7 +341,7 @@ async function start(interaction: ButtonInteraction<'cached'>): Promise<void> {
ButtonNames.Start,
'staff'
);
await server.openAllOpenableQueues(member, true);
await server.openAllOpenableQueues(member, "virtual", "", true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can use function overloading to avoid passing in an unused room value when setting === 'virtual'

Copy link
Collaborator

Choose a reason for hiding this comment

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

See example here

helpStart: new Date(),
helpedMembers: [],
activeState: 'active', // always start with active state
member: helperMember
};

const helper: Helper = (setting !== 'virtual') ? {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Invert the condition, check for equalitysetting === 'virtual' instead of inequality.

const helper: Helper = (setting === 'virtual') ? {
    helpSetting: setting,
    ...baseHelper
} : {
    helpSetting: setting,
    room: room,
    ...baseHelper
};

Copy link
Collaborator

@tomli380576 tomli380576 Apr 5, 2024

Choose a reason for hiding this comment

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

i think you also don't need the type annotation for helper

Copy link
Collaborator

@tomli380576 tomli380576 left a comment

Choose a reason for hiding this comment

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

I feel like the AttendingServer class is doing too much. Try moving some of the functionalities to the HelpQueue class or create some type of queue manager class to handle logic such as keeping track of all in-person and virtual queues, get queues that have a specific student, etc.

Comment on lines +595 to +596
const emptyCollection = new Collection<string, HelpQueue>();
this._in_person_queues.set(parentCategory.id, emptyCollection);
Copy link
Collaborator

Choose a reason for hiding this comment

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

type params are not required here, simply do

this._in_person_queues.set(parentCategory.id, new Collection());

and the types will be inferred correctly.

@@ -860,6 +958,7 @@ class AttendingServer {

if (memberIsStudent) {
// filter queues where some member of that voice channel is a helper of that queue
// FIXME: in_person_queues too
Copy link
Collaborator

Choose a reason for hiding this comment

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

Notice that we have this.queues instead of this._queues here. If get queues() points to all the queues, you don't need to manually update any of the logic here.

@@ -262,8 +271,9 @@ class QueueDisplay {
* @param viewModel the data to put into the table
* @returns the ascii table as a `string` in a code block
*/
private getQueueAsciiTable(viewModel: QueueViewModel): string {
const table = new AsciiTable3();
private getQueueAsciiTable(viewModel: QueueViewModel, isVirtual: boolean): string {
Copy link
Collaborator

@tomli380576 tomli380576 Mar 14, 2024

Choose a reason for hiding this comment

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

isVirtual should be part of the viewModel

}
});

collector.on('collect', async (bi) => {
Copy link
Collaborator

@tomli380576 tomli380576 Mar 14, 2024

Choose a reason for hiding this comment

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

nit: buttonInteraction instead of bi

isTriggeredByMemberWithRoles(server, interaction.member, ButtonNames.JoinInPerson, 'student');

await interaction.reply({
...SimpleEmbed(`Processing button \`Join In-Person\` ...`),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: use plain string here to avoid escaping the backticks

'Processing button `Join In-Person` ...'

Comment on lines +1119 to +1121
inPersonQueues.forEach(queue => {
virtualQueue.inPersonViewModels.push(queue.getViewModel());
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use

virtualQueue.inPersonViewModels = inPersonQueues.map(queue => queue.getViewModel())

to avoid mutating .length of an array

Copy link
Collaborator

Choose a reason for hiding this comment

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

the garbage collector will clean up the old array once the process lose reference to it, so you don't need to manually set it to an empty array

): Promise<HelpQueue> {
const everyoneRole = queueChannel.textChannel.guild.roles.everyone;
const display = new QueueDisplay(queueChannel);
const queueExtensions: QueueExtension[] = environment.disableExtensions
const queueExtensions: QueueExtension[] = environment.disableExtensions || location !== undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

why does specifying the location disable all the extensions?

}
const id = interaction.channel?.parent?.id;
if (!id) {
throw new Error('Invalid option selected:');
Copy link
Collaborator

Choose a reason for hiding this comment

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

use the CommandParseError class here

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.

3 participants