Skip to content

Conversation

@scottc943
Copy link
Contributor

@scottc943 scottc943 commented Dec 27, 2024

…rrect moderator from a single case view instead of whoever runs the command returning as the moderator.

Description

Fixes #678
Allows us to view the correct moderator within the single case view now. For example when we run /case view case:5 it would show the correct moderator that took that case instead of the moderator that ran the command.

Guidelines

  • My code follows the style guidelines of this project (formatted with Ruff)

  • I have performed a self-review of my own code

  • I have commented my code, particularly in hard-to-understand areas

  • I have made corresponding changes to the documentation if needed

  • My changes generate no new warnings

  • I have tested this change

  • Any dependent changes have been merged and published in downstream modules

  • I have added all appropriate labels to this PR

  • [ X] I have followed all of these guidelines.

How Has This Been Tested? (if applicable)

I have tested my code via running the dev tux instance on the development server and testing that the moderator returns correctly.

Screenshots (if applicable)

N/A

Additional Information

N/A

Summary by Sourcery

Bug Fixes:

  • Fixed a bug where running /case view would show the user who ran the command as the moderator instead of the moderator who originally took the case.

…rrect moderator from a single case view instead of whoever runs the command returning as the moderator.
@scottc943 scottc943 self-assigned this Dec 27, 2024
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Dec 27, 2024

Reviewer's Guide by Sourcery

This PR fixes a bug where the incorrect moderator was being displayed in the single case view. The fix involves retrieving the moderator from the case.case_moderator_id instead of using the command issuer as the moderator.

Sequence diagram for case view moderator retrieval

sequenceDiagram
    participant User as Discord User
    participant Bot as Tux Bot
    participant DB as Case Database

    User->>Bot: /case view case:5
    Bot->>DB: Get case details
    DB-->>Bot: Return case with moderator_id
    Bot->>Bot: Convert moderator_id to Member
    Bot-->>User: Display case with correct moderator
Loading

State diagram for case view moderator resolution

stateDiagram-v2
    [*] --> CommandReceived
    CommandReceived --> FetchCase: /case view
    FetchCase --> ModeratorResolution
    ModeratorResolution --> DisplayCase: Convert moderator_id to Member
    ModeratorResolution --> ErrorState: Conversion failed
    DisplayCase --> [*]
    ErrorState --> [*]
Loading

File-Level Changes

Change Details Files
Retrieve the correct moderator from the database
  • Changed the moderator retrieval logic to fetch the moderator using the case.case_moderator_id.
  • Used the commands.MemberConverter to convert the moderator ID to a discord.Member object.
  • Ensures that the correct moderator is displayed in the single case view regardless of who invoked the command.
  • Fixes issue [BUG] -single case view shows the wrong moderator  #678 where the command issuer was incorrectly displayed as the moderator.
tux/cogs/moderation/cases.py

Assessment against linked issues

Issue Objective Addressed Explanation
#678 Fix the bug where single case view shows the command executor as moderator instead of the actual case moderator

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @scottc943 - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 260 to 262
moderator = await commands.MemberConverter().convert(ctx, str(case.case_moderator_id))

if isinstance(moderator, discord.Member):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Add error handling for MemberConverter().convert() to handle cases where the moderator is no longer in the server

The conversion could fail if the moderator has left the server. Consider adding a try-except block to handle MemberNotFound and other potential exceptions, falling back to using the ID or a placeholder value.

Suggested change
moderator = await commands.MemberConverter().convert(ctx, str(case.case_moderator_id))
if isinstance(moderator, discord.Member):
try:
moderator = await commands.MemberConverter().convert(ctx, str(case.case_moderator_id))
except (commands.MemberNotFound, commands.errors.MemberNotFound):
# Create a placeholder value when moderator is no longer in server
moderator = f"Unknown Moderator (ID: {case.case_moderator_id})"
if isinstance(moderator, (discord.Member, str)):

AmilieCoding
AmilieCoding previously approved these changes Dec 29, 2024
Copy link

@AmilieCoding AmilieCoding left a comment

Choose a reason for hiding this comment

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

Looks good to me (Guess not, silly me)

Copy link
Member

@electron271 electron271 left a comment

Choose a reason for hiding this comment

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

please fix ci

@AmilieCoding AmilieCoding dismissed their stale review December 29, 2024 03:25

Shouldn't be approved. Is in fact not functional.

…verter

Refactor the code to handle cases where the moderator is not found as a
member by using UserConverter as a fallback. This ensures that the
moderator information is correctly retrieved even if they are not a
current member of the server.
@kzndotsh
Copy link
Contributor

fixed ci issues

@kzndotsh kzndotsh merged commit 9c60f4c into main Dec 29, 2024
9 checks passed
@kzndotsh kzndotsh deleted the issue-678-fix-single-case-case-view branch December 29, 2024 18:32
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.

[BUG] -single case view shows the wrong moderator

5 participants