fix(AlertsReports): making log retention "None" option valid #27554
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27554 +/- ##
==========================================
+ Coverage 67.36% 70.11% +2.74%
==========================================
Files 1909 1913 +4
Lines 74744 75999 +1255
Branches 8329 8396 +67
==========================================
+ Hits 50352 53285 +2933
+ Misses 22342 20652 -1690
- Partials 2050 2062 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@fisjac would you mind including a screenshot to aid reviewers in understanding the context of allowing log retention to be "None". |
| @@ -46,7 +46,7 @@ def run(self) -> None: | |||
| ) | |||
| try: | |||
| row_count = ReportScheduleDAO.bulk_delete_logs( | |||
| report_schedule, from_date, commit=False | |||
| report_schedule, from_date, commit=True | |||
There was a problem hiding this comment.
This is somewhat interesting. With commit=False I don't see where this command actually commits. My sense is there should be a db.session.commit() on line 60 to ensure we commit once rather than for every report schedule.
BTW I'm working on a PR which will ensure that we handle the "unit of work" correctly.
There was a problem hiding this comment.
@fisjac you marked this as resolved, yet I don't believe the comment was addressed, i.e., the run method should commit once—at line 60 per the previous suggestion—rather than on a per report schedule basis.
There was a problem hiding this comment.
@john-bodley I had forgotten to push some changes up. This should be covered in the latest push.
| @@ -320,7 +320,7 @@ class ReportSchedulePutSchema(Schema): | |||
| log_retention = fields.Integer( | |||
| metadata={"description": log_retention_description, "example": 90}, | |||
| required=False, | |||
| validate=[Range(min=1, error=_("Value must be greater than 0"))], | |||
| validate=[Range(min=0, error=_("Value must be 0 or greater"))], | |||
There was a problem hiding this comment.
What does a log retention of 0 mean and how does this differ from None?
There was a problem hiding this comment.
The current behavior is such that the "None" option is a display for the value 0, but 0 isn't accepted on the backend.
The schema is expecting for log_retention to be an integer value indicating the # of days the logs will be retained. I've preserved the initial logic that the None value is stored as 0, but enabled 0 to be an acceptable value.
The log_retention is then passed to the log_prune function which removes any logs with a date less than the current date - log retention (0) days.
| value={ | ||
| typeof currentAlert?.log_retention === 'number' | ||
| ? currentAlert?.log_retention | ||
| : ALERT_REPORTS_DEFAULT_RETENTION | ||
| } |
There was a problem hiding this comment.
Let's double check if this can actually go
There was a problem hiding this comment.
The default log_retention value is specified as a number, and the dropdown options include only integers.
Additionally, there backend validation schema only allows integers. There shouldn't be a situation which calls for typeof !== 'number'
fisjac
force-pushed
the
ar/log-retention-none
branch
from
24000c9
to
1f64964
Compare
fisjac
force-pushed
the
ar/log-retention-none
branch
from
1f64964
to
0f91bbe
Compare
switch to batch session commit
fisjac
force-pushed
the
ar/log-retention-none
branch
from
0f91bbe
to
244267b
Compare
michael-s-molina
added
the
v4.0
(cherry picked from commit b7f3e0b)
mistercrunch
added
🍒 4.0.1
🍒 4.0.2
🏷️ bot
SUMMARY
This PR implements two changes:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION