Filter maintenance.timeline to return data from expected date range based on limit #960
Conversation
… end date for maintenance timeline to align parity with usage timelime
…istent with how the usage timeline is implemented, refactored test_activity_dashboard to reflect the code changes, and cleaned up code to improve readability in model.py and test_activity_dashboard.py in general
|
I have tested locally to ensure that the data is correct, and edge cases are captured as well.
|
backend/api/model.py
Outdated
| @@ -430,7 +429,7 @@ def _update_activity_timeline_data(): | |||
| write_data(csv_string, "activity_dashboard_data/plugin_installs.csv") | |||
|
|
|||
|
|
|||
| def _process_for_timeline(plugin_df, limit): | |||
| def _process_usage_timeline(plugin_df, limit): | |||
There was a problem hiding this comment.
Great idea renaming this one!
backend/api/model.py
Outdated
| @@ -448,6 +447,18 @@ def _process_for_timeline(plugin_df, limit): | |||
| return result | |||
|
|
|||
|
|
|||
| def _process_maintenance_timeline(commit_activity, limit): | |||
| now = datetime.now() | |||
| end_date = datetime(now.year, now.month - 1, 1) if now.month > 1 else datetime(now.year - 1, 12, 1) | |||
There was a problem hiding this comment.
I find the way end_date is calculated in line 434 a lot more readable. Any particular reason you didn't want to do it that way?
There was a problem hiding this comment.
The original intention was to define start_date and end_date as type datetime.datetime, so that we could directly compare start_date <= item_date <= end_date without any conversions, since the implementation on line 434 and line 435 are type datetime.date.
That being said, after some thinking, I do agree the way that end_date is written here is a bit more complex than it needs to be, so I have decided to follow the logic in _process_usage_timeline to generate start_date and end_date.
I have also created the _process_for_dates method that returns start_date, end_date, dates, which is used by both _process_usage_timeline and _process_maintenance_timeline to avoid code duplication.
In order to do the minimal number of conversions, we compare the datetime.date objects like so in the PR: start_date <= item_datetime.date() <= end_date
I have locally tested the current implementation and it works as expected, in the same way outlined in #960 (comment)
…used by both _process_usage_timeline and _process_maintenance_timeline to avoid code duplication
….json contain data in which plugin is lower case to maintain parity with the implementation of install activity
backend/api/model.py
Outdated
| @@ -448,6 +452,15 @@ def _process_for_timeline(plugin_df, limit): | |||
| return result | |||
|
|
|||
|
|
|||
| def _process_maintenance_timeline(commit_activity, limit): | |||
| start_date, end_date, dates = _process_for_dates(limit) | |||
| maintenance_dict = {item_datetime: item for item in commit_activity if | |||
There was a problem hiding this comment.
There's a lot going on here and it's hard to follow in a single line. How about something similar to:
for commit in commit_activity:
commit_date = ...
if start_date <= commit_date <= end_date:
add commit to list
klai95
dismissed
manasaV3’s stale review
feedback has been addressed, and reviewer is unable to review this PR at this time
codemonkey800
added
bug-fix
Issue: #958