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

Add Notification for Tutors (fixes #14) #20

Merged
merged 4 commits into from
Dec 31, 2023

Conversation

AlphaKevin01
Copy link
Collaborator

No description provided.

@AlphaKevin01
Copy link
Collaborator Author

(#14)

Copy link
Owner

@Rdeisenroth Rdeisenroth left a comment

Choose a reason for hiding this comment

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

This will only work if the student joins via the /queue join command. Please also edit the voiceStateUpdateEvent.ts File to cover the voice channel join method.

Also i think it would help if the Tutors get notified when every student leaves and the queue is empty. Maybe a designated guild channel with restricted access for queue events would be a better option instead of notifying every active tutor individually, what do you think? Then we could also have a pinned message there that auto-updates whenever someone joins/leaves the queue.

src/commands/queue/join.ts Outdated Show resolved Hide resolved
@niklhut
Copy link
Collaborator

niklhut commented Nov 8, 2023

I think the idea with DMs is not that bad. This way a tutor only gets notified when they have an active session.
Even though a designated guild channel could be used as well, when only using a channel there is no guarantee an active tutor actually gets notified by this message.
In other words to get notified he would need to activate notifications for every message in this channel either permanently and the receive these updates when he is not in an active sessions as well, or turn the settings for notifications on and off every time he starts and ends his session. The latter would be rather tedious since he already calls the bot command, unless of course the bot could automatically handle the notification permissions, but I'm not sure if that is possible.
Another way to solve this would be that the bot always at mentions all active tutors in his status update messages, so there would be no need for notification permission changes, since you usually get notified for at mentions.

@niklhut niklhut changed the title added notification for tutors Add Notification for Tutors (fixes #14) Nov 8, 2023
@niklhut niklhut linked an issue Nov 8, 2023 that may be closed by this pull request
@niklhut
Copy link
Collaborator

niklhut commented Nov 10, 2023

Should create new role for currently active tutors.

Create notifications for tutors with active role in a separate channel. This means notifications when someone new joins the queue

@AlphaKevin01
Copy link
Collaborator Author

The /setqueueinfo command can be used to bind a text channel to a queue as a queue info channel. This text channel then informs all users with the role active_session about users joining the queue

@Rdeisenroth Rdeisenroth self-requested a review November 30, 2023 17:08
@Rdeisenroth
Copy link
Owner

Oops, c2301d5 wasn't just linting, i also changed the role resolution to use the saved role ids instead of by name, as this would not work with custom role names.

@AlphaKevin01 AlphaKevin01 force-pushed the feature/tutor-notification branch 2 times, most recently from f1410ca to 358a05b Compare December 8, 2023 13:13
@AlphaKevin01
Copy link
Collaborator Author

I have added a new service layer to manage the bot's business logic. In future, I think it would be logical to remove the actual logic from the commands and control it in the services classes in the service folder. This gives the project a better modularity and structure.

The example of the QueueInfoService clearly shows the layer division. What do you think of this approach?

Now to the new functionality:
with the command: /queueinfo set channel queue events
a given channel can be linked to a queue. This channel then logs the specified events. The events include: [join, leave, kick, next, tutor_session_start, tutor_session_stop].

The /queueinfo remove channel queue
command can be used to remove the channel as an infochannel.

Copy link
Owner

@Rdeisenroth Rdeisenroth left a comment

Choose a reason for hiding this comment

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

Small fixes still needed.

src/models/queues.ts Outdated Show resolved Hide resolved
src/utils/general.ts Show resolved Hide resolved
src/service/queue-info/QueueInfoService.ts Show resolved Hide resolved
src/utils/general.ts Show resolved Hide resolved
@Rdeisenroth Rdeisenroth self-requested a review December 30, 2023 23:34
Copy link
Owner

@Rdeisenroth Rdeisenroth left a comment

Choose a reason for hiding this comment

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

LGTM

@Rdeisenroth Rdeisenroth merged commit c514e47 into main Dec 31, 2023
1 check passed
@Rdeisenroth Rdeisenroth deleted the feature/tutor-notification branch December 31, 2023 00:05
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.

Notifications for Tutors
3 participants