fix: enforce ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH
#32053
Conversation
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
32fd9cf to
0ea2597
Compare
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset/commands/report/execute.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- Chat with Korbit on issues we post by tagging @korbit-ai in your reply.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Naming ✅ Database Operations ✅ Documentation ✅ Logging ✅ Error Handling ✅ Systems and Environment ✅ Objects and Data Structures ✅ Readability and Maintainability ✅ Asynchronous Processing ✅ Design Patterns ✅ Third-Party Libraries ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
Note
Korbit Pro is free for open source projects 🎉
Looking to add Korbit to your team? Get started with a free 2 week trial here
0ea2597 to
90e9328
Compare
| @@ -300,13 +300,16 @@ def _get_screenshots(self) -> list[bytes]: | |||
| ) | |||
| user = security_manager.find_user(username) | |||
|
|
|||
| max_width = app.config["ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH"] | |||
There was a problem hiding this comment.
@betodealmeida Maybe we should deprecate the ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH feature flag to remove it in a major version. It seems that this feature should always be enabled in case alerts and reports are too.
There was a problem hiding this comment.
Oh, it's not a feature, just a configuration value.
| ("equals_max", 1600, 1600, 800, 1600), | ||
| ], | ||
| ) | ||
| def test_screenshot_width_calculation( |
SUMMARY
We only enforce
ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTHwhen an alert/report is created. This means that if the feature flag is later changed to a smaller value it might not have any effect on pre-existing alerts/reports.This PR adds a check during screenshot generation.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION