-
-
Notifications
You must be signed in to change notification settings - Fork 734
User Events Management system #1269
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
Conversation
The user events cog file uses the channel and role IDs from the constants file
f642bdb to
73174ad
Compare
ks129
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First batch of reviews
| if not self.check_if_user_is_organizer(ctx.author.id, event_name): | ||
| await ctx.send("You can only modify your events!") | ||
| return | ||
| data = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this 1 key to call and don't assign it to variable.
| self.user_event_coord_channel = None | ||
| self.user_event_announcement_channel = None | ||
| self.user_events_list_channel = None | ||
| self.user_event_voice_channel = None | ||
|
|
||
| # Load required roles. | ||
| self.developers_role = None | ||
| self.user_event_ongoing_role = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is place where adding type hint to variable is good.
| self.user_event_coord_channel = self.bot.get_channel(Channels.user_event_coordinators) | ||
| self.user_event_announcement_channel = self.bot.get_channel(Channels.user_event_announcements) | ||
| self.user_events_list_channel = self.bot.get_channel(Channels.user_event_list) | ||
| self.user_event_voice_channel = self.bot.get_channel(Channels.user_event_voice) | ||
|
|
||
| # Load required roles. | ||
| self.developers_role = self.guild.get_role(Roles.verified) | ||
| self.user_event_ongoing_role = self.guild.get_role(Roles.user_event_ongoing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use here get or fetch pattern. When some channel/role is not found, this should unload cog and show warning about it, because everything is required there. Also, use asyncio.Event to wait for items loading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discordpy does not provide the fetch method for channels.
I am going for the following pattern:
- using properties like before
- add a custom property decorator that caches the value and unloads the cog if the property value is None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discord.py have fetch_channel method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, right, how did I miss that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if I check if get_channel or get_role returns None, then I unload the cog with appropriate logging instead of fetch_channel raising an error and unloading the cog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must be ready that channels or role(s) is not in cache, so we need to fetch them.
| } | ||
|
|
||
| NOT_SCHEDULED = "Not scheduled" | ||
| LIVE = "Live" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this a constant, if it's only used once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These strings had getter functions before, I asked moving them from functions to constants.
|
Current state of this PR: Organization Issue. @janine9vn what would you like to do with this PR? I believe you had some suggestions/plans? |
|
I've chatted with @RohanJnr about converting this code for a different but very similar purpose. He'll be using a new branch and PR but has requested this stay open for a bit longer to reference some implementation details. |
|
@janine9vn Thank you for the update. |
|
@RohanJnr do you still need this PR to exist? |
|
Closing this as a new one will be opened for #1603 . |
closes #1172
This PR uses the new API endpoints and features implemented in python-discord/site#410
Hence, this PR should be merged only after the Site PR is merged and deployed.
Commands/Functionality the Cog provides:
How does this work?
Whenever a new user event is created, it will be listed in a new channel called #user-events-list (name can be flexible). This channel is used to display all the user events and also show their status such as:
DateTime in readable formatThe event message ID will be stored in the site DB for further usage.
Users will be able to subscribe to the event by reacting to the ✔️ reaction on the message.
When the user event is scheduled, the status in the event message is edited.


Also, another message is sent in the #user-event-announcement channel, like the one below:

Conditions
Note: All the above checks are handled by the site in form of validation checks, the bot just catches the
ResponseCodeErrorexception.Channels used
Roles used