Skip to content

Conversation

MattyTheHacker
Copy link
Member

No description provided.

@MattyTheHacker MattyTheHacker linked an issue May 18, 2025 that may be closed by this pull request
@MattyTheHacker MattyTheHacker self-assigned this May 18, 2025
@MattyTheHacker MattyTheHacker added the enhancement New feature or request label May 18, 2025
@MattyTheHacker MattyTheHacker force-pushed the 453-action-reminders branch from 20e6524 to 546327d Compare May 18, 2025 22:53
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Implements automatic reminders for assigned committee actions by introducing new configuration options and a background task.

  • Adds environment settings for enabling reminders and configuring their interval.
  • Introduces get_user_actions helper to centralize action-fetching logic and refactors existing list commands to use it.
  • Defines a new CommitteeActionsTrackingRemindersTaskCog to run a periodic Discord reminder task.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
config.py Added _setup_committee_actions_reminders and _setup_committee_actions_reminders_interval to parse new env vars.
cogs/committee_actions_tracking.py Added get_user_actions overloads, refactored list commands, and created the reminders task cog.
cogs/init.py Registered CommitteeActionsTrackingRemindersTaskCog.
Comments suppressed due to low confidence (1)

cogs/committee_actions_tracking.py:205

  • [nitpick] There are no tests covering the background reminders task. Adding unit tests for the enabled/disabled path and interval parsing would ensure this feature works reliably.
@tasks.loop(**settings["COMMITTEE_ACTIONS_REMINDERS_INTERVAL"])

Copy link
Member

@CarrotManMatt CarrotManMatt left a comment

Choose a reason for hiding this comment

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

Still not finished...


if not action_board_message or action_board_message.author != self.bot.user:
action_board_message = await action_board_channel.send(
content="**Committee Actions Tracking Board**\n"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
content="**Committee Actions Tracking Board**\n"
content="## Committee Actions Tracking Board\n"

)

await action_board_message.edit(
content=(f"**Committee Actions Tracking Board**\n{all_actions_message}"),
Copy link
Member

Choose a reason for hiding this comment

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

Is it intended for the header to be sent as a separate message on line 111, then also as part of the content of this message?

)
+ ' '
+ f'{action.description} '
+ f'({AssignedCommitteeAction.Status(action.status).label})'
Copy link
Member

Choose a reason for hiding this comment

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

Is action.status not already an instance of AssignedCommitteeAction.Status? Why does it's value need to be used to create a new instance rather than just accessing action.status.label?

Copy link
Member

Choose a reason for hiding this comment

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

Missing documentation for 3 new config settings in .env.example.

Comment on lines +999 to +1003
all_actions: dict[str, list[AssignedCommitteeAction]] = await self._get_all_actions()
filtered_actions: dict[str, list[AssignedCommitteeAction]] = {
discord_id: actions
for discord_id, actions in all_actions.items()
if not status or any(action.status == status for action in actions)
Copy link
Member

Choose a reason for hiding this comment

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

Use new query argument to filter actions by status rather than additional comprehension.

await self._get_filtered_actions(query=Q(status__isnull=True) | Q(status=status))

if action.discord_member.discord_id == discord_id
)
}"
for discord_id, actions in filtered_actions.items()
)

await ctx.respond(content=all_actions_message)
Copy link
Member

Choose a reason for hiding this comment

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

Remove single-use variable all_actions_message. Literal string can be declared as argument to function call.

Comment on lines +235 to +243
user_actions: list[AssignedCommitteeAction] = [
action
async for action in AssignedCommitteeAction.objects.filter(
status=status,
discord_member__discord_id=int(action_user.id),
)
]

return user_actions
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
user_actions: list[AssignedCommitteeAction] = [
action
async for action in AssignedCommitteeAction.objects.filter(
status=status,
discord_member__discord_id=int(action_user.id),
)
]
return user_actions
action_user = (action_user,)

Comment on lines +115 to +117
all_actions: dict[
str, list[AssignedCommitteeAction]
] = await self._get_incomplete_actions()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
all_actions: dict[
str, list[AssignedCommitteeAction]
] = await self._get_incomplete_actions()
all_actions: "Mapping[str, Collection[AssignedCommitteeAction]" = await self._get_incomplete_actions()

)

await action_board_message.edit(
content=(f"**Committee Actions Tracking Board**\n{all_actions_message}"),
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary single-use variable all_actions_message. Literal string can be defined within function call argument.

Comment on lines +245 to +261
actions: list[AssignedCommitteeAction] = [
action async for action in AssignedCommitteeAction.objects.select_related().all()
]

committee_actions: dict[discord.Member, list[AssignedCommitteeAction]] = {
committee: [
action
for action in actions
if str(action.discord_member.discord_id) == str(committee.id)
and action.status in status
]
for committee in action_user
}

return {
committee: actions for committee, actions in committee_actions.items() if actions
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
actions: list[AssignedCommitteeAction] = [
action async for action in AssignedCommitteeAction.objects.select_related().all()
]
committee_actions: dict[discord.Member, list[AssignedCommitteeAction]] = {
committee: [
action
for action in actions
if str(action.discord_member.discord_id) == str(committee.id)
and action.status in status
]
for committee in action_user
}
return {
committee: actions for committee, actions in committee_actions.items() if actions
}
return {
inner_action_user: user_actions
for inner_action_user in action_user
if (user_actions := cls._get_user_actions(action_user=inner_action_user, status=status)) is not None
}

Where CommitteeActionsTrackingBaseCog._get_user_actions() is defined as:

@classmethod
async def _get_user_actions(cls, action_user: discord.Member | discord.User, status: "Collection[str]") -> "Collection[AssignedCommitteeAction] | None":
    filtered_user_actions: "Queryset[AssignedCommitteeAction]" = AssignedCommitteeAction.objects.filter(discord_member__discord_id=action_user.id, status__in=status)

    if not await filtered_user_actions.aexists():
        return None

    return [action async for action in filtered_user_actions]  # NOTE: List comprehension is required because wrapping with `list()` is not supported for asynchronous Django queries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request sync Keep this PR up to date with it's base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement persistent actions tracker Implement reminders for actions
4 participants