-
-
Notifications
You must be signed in to change notification settings - Fork 40
fix(levels): fix issue where xp gets reset if db errors #1018
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
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactor get_xp_and_level to fail fast on database errors or missing data by raising exceptions instead of returning default XP/level values, ensuring XP isn’t reset silently on errors. Sequence diagram for error handling in get_xp_and_levelsequenceDiagram
participant Caller
participant LevelsController
participant Database
Caller->>LevelsController: get_xp_and_level(member_id, guild_id)
LevelsController->>Database: find_one(where={member_id, guild_id})
alt Record found and has xp/level
LevelsController-->>Caller: return (xp, level)
else Record missing or error occurs
LevelsController-->>Caller: raise ValueError
end
Class diagram for updated LevelsController error handlingclassDiagram
class LevelsController {
+get_xp_and_level(member_id: int, guild_id: int) tuple[float, int]
+get_last_message_time(member_id: int, guild_id: int) datetime | None
-_fail(msg: str) NoReturn
}
LevelsController : get_xp_and_level() raises ValueError on error or missing data
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @electron271 - I've reviewed your changes - here's some feedback:
- The docstring and signature claim it can return None for missing records, but the code always returns a tuple – update the docs or adjust the return logic to match.
- Catching all Exceptions and rethrowing as ValueError may obscure real issues; consider catching only DB errors and/or introducing a custom exception type for clarity.
- Defining and using a dedicated error type instead of ValueError (e.g. LevelsRetrievalError) would help callers distinguish record-not-found vs. system failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The docstring and signature claim it can return None for missing records, but the code always returns a tuple – update the docs or adjust the return logic to match.
- Catching all Exceptions and rethrowing as ValueError may obscure real issues; consider catching only DB errors and/or introducing a custom exception type for clarity.
- Defining and using a dedicated error type instead of ValueError (e.g. LevelsRetrievalError) would help callers distinguish record-not-found vs. system failures.
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1018 +/- ##
========================================
- Coverage 9.63% 9.62% -0.01%
========================================
Files 123 123
Lines 10414 10421 +7
Branches 1279 1280 +1
========================================
Hits 1003 1003
- Misses 9307 9315 +8
+ Partials 104 103 -1
*This pull request uses carry forward flags. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Deploying tux with
|
Latest commit: |
67179a7
|
Status: | ✅ Deploy successful! |
Preview URL: | https://3b360d6b.tux-afh.pages.dev |
Branch Preview URL: | https://level-fix.tux-afh.pages.dev |
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.
Looks like it does what its supposed to!
giving it a final test |
looks good |
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.
seems good
Description
if db errors people get their level reset
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
I have followed all of these guidelines.
How Has This Been Tested? (if applicable)
Please describe how you tested your code. e.g describe what commands you ran, what arguments, and any config stuff (if applicable)
Screenshots (if applicable)
Please add screenshots to help explain your changes.
Additional Information
Please add any other information that is important to this PR.