-
Notifications
You must be signed in to change notification settings - Fork 2
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
Remove-once-frequency #142
Conversation
Reviewer's Guide by SourceryThis pull request removes the 'ONCE' frequency option from the application. The changes include updating the Frequency enum, modifying the logic for handling scheduled transactions, and adding a database migration to remove the 'once' frequency from the database. File-Level Changes
Tips
|
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 @EduardSchwarzkopf - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider implementing a downgrade function in the migration file to make this change reversible if needed.
- Ensure that all parts of the system that might have used the 'ONCE' frequency have been updated and tested thoroughly to prevent any potential breaks in functionality.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
||
def upgrade() -> None: | ||
# ### commands auto generated by Alembic - please adjust! ### | ||
op.execute(f" DELETE FROM {Frequency.__tablename__} WHERE label = '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.
🚨 suggestion (security): Use parameterized queries instead of f-strings for SQL execution
While f-strings work here, it's safer to use parameterized queries to prevent potential SQL injection vulnerabilities. Consider using SQLAlchemy's text() function with bind parameters for this operation.
op.execute(f" DELETE FROM {Frequency.__tablename__} WHERE label = 'once'") | |
from sqlalchemy import text | |
from app.models import Frequency | |
op.execute( | |
text("DELETE FROM :table WHERE label = :label"), | |
{"table": Frequency.__tablename__, "label": "once"} | |
) |
Summary by Sourcery
Remove the 'ONCE' frequency option from both the codebase and the database, including updating the relevant function logic and adding a migration script.
Enhancements:
Deployment: