Skip to content
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

Add date range for utt report #9

Merged
merged 31 commits into from
Aug 31, 2018
Merged

Add date range for utt report #9

merged 31 commits into from
Aug 31, 2018

Conversation

kitchoi
Copy link
Contributor

@kitchoi kitchoi commented Aug 19, 2018

This pull request adds --from and --to to the utt report command for aggregating activities in a given date range. This provides the basic requirement for #4

Example:

$ utt report --from 2013-07-01 --to 2013-12-31

With date range support, shortcuts such as utt report week or utt report January can be implemented later.

I too have use cases where I needed to get a weekly or monthly report.
The "Details" section is skipped as I assume it would be too long, esp for yearly report?

@kitchoi
Copy link
Contributor Author

kitchoi commented Aug 19, 2018

I added test/unit/test_report.py when I wasn't able to launch Docker. I can remove it if this is deemed redundant or inconsistent with how integration tests are run.

Copy link
Owner

@larose larose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank your for this pull request! I added two comments.

Once the current activity issue is fixed, I'll merge your pull request and make a new release.

activity = Activity(entry_pair[0].datetime, entry_pair[1]).clip(
start_datetime, end_datetime)
if (activity.duration > datetime.timedelta() and
activity.name.name != HELLO):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we don't rely on "hello". The name of the first entry in a day can be anything. By convention it is "hello" since most users are probably using utt hello. However, there is a slight probability that explicitly checking for "hello" is a breaking change for a few users.

This doesn't need to be addressed, but if you find a way to avoid this breaking change that would be ideal.

today = now.date()
if report_date == today and entries and entries[-1].datetime < now and \
not disable_current_activity:
if entries and entries[-1].datetime < now and not disable_current_activity:
Copy link
Owner

@larose larose Aug 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current activity should only be added if the user looks at today's report. By removing report_date == today the current activity is now added even when the user looks at, say, yesterday's report. This creates a bogus current activity:

$ cat /root/.local/share/utt/utt.log   
2018-08-18 22:00 hello
2018-08-18 22:10 foo
$ utt --now "2018-08-19 23:00" report 2018-08-18

----------------------- Saturday, Aug 18, 2018 (week 33) -----------------------

Working Time: 1 day, 1h00 (0h10 + 1 day, 0h50) [25h00]
Break   Time: 0h00 [0h00]

----------------------------------- Projects -----------------------------------

(1 day, 1h00) : -- Current Activity --, foo

---------------------------------- Activities ----------------------------------

(1 day, 0h50) : -- Current Activity --
(0h10) : foo


----------------------------------- Details ------------------------------------

(0h10) 22:00-22:10 foo
(1 day, 0h50) 22:10-23:00 -- Current Activity --

@kitchoi
Copy link
Contributor Author

kitchoi commented Aug 20, 2018

Thank you @larose for reviewing and for pointing out the breaking changes.
I did notice the assumption made to ignore overnight activities. What I did not notice is that this assumption may be used by the users as a feature.

What I just pushed are changes to ignore activities that span over the mid-night, and this should address both the "alternative hello" usage and the bogus current activity when logs are organized daily.

However I should point out that this assumption on people not working overnight is not true for me and for my colleagues! One of my colleagues worked around by adding an activity close to midnight, i.e. at 23:59. Please can I add an option to include overnight activities in the report? I can do that in a separate PR or this one, if you'd prefer.

activity = Activity(prev_entry.datetime, next_entry)
# For preserving existing behaviour, skip activities spanning
# over midnights
if prev_entry.datetime.date() != next_entry.datetime.date():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I probably should check if the activity is already ignored via its type.

@kitchoi
Copy link
Contributor Author

kitchoi commented Aug 20, 2018

I added a few more commits to handle warnings about the overnight activities being ignored... it would seem wrong to me not to warn about them as they are actively being filtered.

@larose
Copy link
Owner

larose commented Aug 20, 2018

I tried you latest code and I still have an unwanted current activity (that is shown as an ignored overnight activity):

cat /root/.local/share/utt/utt.log
2018-08-18 22:00 hello
2018-08-18 22:10 foo

2018-08-20 23:31 hello
utt --now "2018-08-19 23:00" report 2018-08-18
WARN: Ignored 1 overnight activity, total time: 1 day, 0h50

----------------------- Saturday, Aug 18, 2018 (week 33) -----------------------

Working Time: 0h10 [0h10]
Break   Time: 0h00 [0h00]

----------------------------------- Projects -----------------------------------

(0h10) : foo

---------------------------------- Activities ----------------------------------

(0h10) : foo


----------------------------------- Details ------------------------------------

(0h10) 22:00-22:10 foo

I think we can support overnight activity when using --from and --to while keeping the right behavior when a user looks at a previous one-day report if we do something like that:

$ git diff
diff --git a/utt/cmd_report.py b/utt/cmd_report.py
index 0f3e0af..412c613 100644
--- a/utt/cmd_report.py
+++ b/utt/cmd_report.py
@@ -66,7 +66,7 @@ def execute(args):
     collect_from_date = min(today, collect_from_date)
     entries = list(util.entries_from_file(args.data_filename))
     _add_current_entry(entries, args.now, args.current_activity,
-                       args.no_current_activity)
+                       args.no_current_activity, report_start_date, report_end_date)
     activities, ignored_overnights = (_collect_activities(
         collect_from_date, collect_to_date, entries))
 
@@ -110,8 +110,8 @@ def _collect_activities(start_date, end_date, entries):
 
 
 def _add_current_entry(entries, now, current_activity_name,
-                       disable_current_activity):
-    if entries and entries[-1].datetime < now and not disable_current_activity:
+                       disable_current_activity, report_start_date, report_end_date):
+    if now.date() == report_start_date and now.date() == report_end_date and entries and entries[-1].datetime < now and not disable_current_activity:
         entries.append(Entry(now, current_activity_name, True))

and we remove the filtering of overnight activities.

Here are the high-level requirement for current activity:

  • If a user is looking at today's report: add current activity (unless he disabled it).
  • If a user is looking at a previous one-day report: don't add a current activity.

I realize those requirements should be tests and I'll try to add them.

@larose
Copy link
Owner

larose commented Aug 21, 2018

I added a commit with the test: 4b87d0a

The commit is on top of yours. You can add it to you PR.

@kitchoi
Copy link
Contributor Author

kitchoi commented Aug 21, 2018

Thank you for testing and clarifying the requirements for the current activity.

If I understand correctly, the --no-current-activity flag is implicitly set if "now" is not within the report date range? i.e. Would it make sense to check now.date() >= report_start_date and now.date() <= report_end_date instead of now.date() == report_start_date and now.date() == report_end_date?

And regarding your previous comment:

The name of the first entry in a day can be anything. By convention it is "hello" since most users are probably using utt hello. However, there is a slight probability that explicitly checking for "hello" is a breaking change for a few users. This doesn't need to be addressed, but if you find a way to avoid this breaking change that would be ideal.

With your suggested changes where overnight activities are not filtered (yay!), users who do not use utt hello but use a different start-of-day entry name will find that activity in the report, clipped to start from midnight. So the breaking change will be there. I tend to think this is not a big deal, as it is a misuse on the user part that can be fixed by editing the log.

I will try to work on this later tonight. Thanks again!

@larose
Copy link
Owner

larose commented Aug 30, 2018

There seems to be a bug. Here is the output of utt when I don't have anything today, just entries yesterday:

$ ./run report

----------------------- Thursday, Aug 30, 2018 (week 35) -----------------------

Working Time: 17h54 (0h00 + 17h54) [25h54]
Break   Time: 0h00 [0h00]

----------------------------------- Projects -----------------------------------

(17h54) : -- Current Activity --

---------------------------------- Activities ----------------------------------

(17h54) : -- Current Activity --


----------------------------------- Details ------------------------------------

(17h54) 00:00-17:54 -- Current Activity --

If I don't have anything today. I expect to see nothing. Not starting the clock automatically at midnight.

After all the effort you made (plus this PR: #6), I feel that supporting overnight activity in the report command is maybe not possible without introducing a breaking change.

Maybe we should add a new argument --overnight to the add command that does the hack of adding an entry at 23:59, a hello at 00:00 and the activity name at the current time. What do you think?

@kitchoi
Copy link
Contributor Author

kitchoi commented Aug 31, 2018

Thanks @larose for giving this another look! I am really grateful.

I will be happy to implement the workaround for adding the 23:59 and midnight entries, if there is really no better way to incorporate overnight activities without breaking the application for others. But first of all I hope you could explain to me why the new behaviour you posted above is a bug, because I am not sure it is.

Just to confirm that I understood the scenario: You have a current activity spanning from yesterday (sometime) to now, i.e. you have not added any entry (not even hello) since that sometime yesterday. You are fetching the report for today only and you expect the report for today to be empty.

Why is an empty report expected? Is that because that the current activity does not exist if one never adds any entry for today? Why is that? If I could understand this, maybe there is a better way to fix it without the workaround.

A bit more on why I think the new behaviour is expected: Say I have been travelling since yesterday 9PM, and it is 11AM now and I am still travelling (forget about timezone for now). If I finish my travel, and type utt add travel, then I should expect to find 11 hours of travel for today. If I have not entered utt add travel, then it is still a current activity, so 11 hours of current activity for today. Current activity should not be skipped on today's report just because I have not named the activity, given that I have not added the --no-current-activity in my command. My mental model is that there is always a current activity, you just choose whether to report it.

From the point of view of a user who never works overnight, this new "absurd" current activity indicates that they have forgotten to say hello to utt.

If we should workaround with the 23:59/midnight entries, then I would probably want to make that workaround automatic as it is likely that I, as a user, will forget to add the --overnight switch. The definition of midnight is also tricky if you change time zones. For me and my colleagues (also fellow users of utt), overnight activities and switching time zones tend to happen at the same time :(

@larose larose merged commit 4c2c8f7 into larose:master Aug 31, 2018
@larose
Copy link
Owner

larose commented Aug 31, 2018

You did convince me that this is the right behavior. Hopefully there won't be too many complaints :)

I just released version 1.8 with your changes. Thank you for your contribution!

@kitchoi
Copy link
Contributor Author

kitchoi commented Sep 2, 2018

Thank you very much @larose!

@kitchoi kitchoi deleted the date-range branch September 2, 2018 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants