Spend hours in goals not displayed

ggeorgg commented 2 years ago

Function in question: https://github.com/mtierltd/timetracker/blob/710cd33a335c077ff40f225c7bc4806079c1298f/lib/Controller/AjaxController.php#L1224

I am having trouble to get the goals displayed correctly. I have set up a 20 hours weekly goal, beginning on 1. September 2021 grafik

Unfortunately, no working interval is taken into consideration for this goal. (There are for sure a lot of working intervals in this time period.) I don't think it is related to some offset or UTC stuff. I think it is related to this line in the code. I tried to debug it a bit and logged variables to a file. Here is what I get right before the if clause: interval: 2021-09-27 $repItem->time: 1630476900

It appears like the if clause can never be true because of the data type mismatch, so that the code can never reach the break condition of this if clause. Therefore all hours are accounted to the "Past Debt in Hours" and "Remaining Hours" but the sum of "Hours spend" is always zero.

Does anybody else have this problem? If needed, I can also share more information about my setup.

Best regards Georg

kadrim commented 2 years ago

yes, exactly my problem.

kadrim commented 2 years ago

working on a patch right now.

kadrim commented 2 years ago

will be ready as Pull-Request next Week, to generate more testing-data

kadrim commented 2 years ago

fix has been implemented, pull request https://github.com/mtierltd/timetracker/pull/156 is available

ggeorgg commented 2 years ago

Thanks for this implemenation. I tried it and the spend hours are now displayed. This looks good now. But if I compare the other columns to my Excel calculation, there is still an error in the calculation. (I guess a new issue should be opened. If yes, just let me know.)

Here is a screenshot of my current working hours grafik

And here is a screenshot of my Excel calculation: grafik

You can see

"Hours spent current week" is correct now.
"Past Debt in Hours": is wrong, I think because of the next point in the list. (Positive number, because we already worked more than expected)
"Remaining Hours (current week?)": Value seems to be correct, but it should be negative in my opinion!
"Total Remaining Hours": Looks like the values "Past Debt in Hours" and "Remaining Hours (current week?)" have been added. But due to the wrong "sign" just mentioned in the point before, this value is completly wrong.

kadrim commented 2 years ago

Can you upload your excel sheet, i can save some time that way as i don't have to create more testdata ;-)

Thanks in advance!

ggeorgg commented 2 years ago

Sure! Here it is: WorkingTime.ods

kadrim commented 2 years ago

i think the problem is somewhere different. The calculation for the functions getStartOfWeek() and getStartOfMonth is wrong. It does calculate correctly the date but always uses the time when the goal was created.

so if you have time-records at 08:00-09:00 and your goal was created at 10:00 those time-records will be excluded.

kadrim commented 2 years ago

i just updated the PR above, could you recheck with your database and worksheet?

ggeorgg commented 2 years ago

I just applied the latest change of your pull request, but it still does not correctly calculate the correct total remaining hours.

kadrim commented 2 years ago

to make it a little bit easier for me to track down the issue, could you please attach a database dump of your test-data?

so alle the table prefixed with oc_timetracker_?

kadrim commented 2 years ago

or can we narrow it down ith some simple entries?

ggeorgg commented 2 years ago

nextcloud.zip WorkingTime-setTime0-0-0-0.ods WorkingTime.ods

I tried to anonymize the data in the nextcloud.zip file. This is the database dump of the current database. I also updated the excel files so that they match the current database status. On Excel file contains the values with your second commit.

kadrim commented 2 years ago

thanks, i'll have a look

kadrim commented 2 years ago

i needed to adjust your SQL slightly to make it work with my user - thanks, now we have a common base.

I am still trying to figure out, where the difference in Past Debt in Hours and Total Remaining Hours comes from, the other two values seem fine.

kadrim commented 2 years ago

i think there are some problems with your data (maybe not related to this PR):

your first task (34), you assume only 12 hours, which will not work. as there is only one goal starting on 01.09. with 20 h/week; your target hours in the worksheet should be adjusted to 20 hours for that line
i am not fully sure if this is a problem but for task 19 and 48 the same week is used

maybe you want to double check the data, maybe you see something odd?

i will continue digging.

kadrim commented 2 years ago

~~fixed another (follow-up)-bug in getWeeksSince and getMonthsSince to also calculate the current week (as we are on sunday - maybe this is wrong?)~~

this would lead to incorrect values for my testdata, so i think the problem lies within the duplicate week in your test-data

kadrim commented 2 years ago

i think the last missing piece is task 48 which is also on 2021W40 and might cause the calculation issue

kadrim commented 2 years ago

~~i just updated the PR.~~ And i think the wrong calculation is not from the goals section but from the report section where there is a duplicate of 2021W40

What do you think?

kadrim commented 2 years ago

as today is a new week, i can see, that there is something odd with Past Debt in Hours. Having a look into that aswell

kadrim commented 2 years ago

another insight: if you have time-records which start at the beginning of the week. there might be a problem with the allocation of the week. Internally this plugin resets all timezones to UTC at multiple places. This was introduced in 870c74a and e090c73

This can and will lead to problems for users not on UTC. Not a big deal in my location (GMT+1) but for others farther away from UTC (both directions), this will pose a serious problem. I think that should be adressed in another PR. Maybe set it once to UTC for the work that needs to be done and then reset it back to the previous timezone.

I think that might also be the case, why you have multiple entries for 2021W40, but i did not inspect that further. maybe you can check this.

I also saw, that for a given goal, only one time-record will be used, ever. I will change that, so that multiple time-records which match that interval will be counted.

kadrim commented 2 years ago

@ggeorgg please apply the latest patch in my PR. This will hopefully migitate your issue, but the problem with the timezones remain and should be adressed seperately. Also please bear in mind my comment to your first workitem

your first task (34), you assume only 12 hours, which will not work. as there is only one goal starting on 01.09. with 20 h/week; your target hours in the worksheet should be adjusted to 20 hours for that line

ggeorgg commented 2 years ago

Here is a screenshot of the report screen filtered for the week CW41 2021: grafik Here are definitley working times available. So it is really interesting, why the report section does not give back a CW41 entry when grouping by week. Here is the same time slot grouped by weekly: grafik I don't know what is wrong with the entry with 3:54:00. My server is located in Germany. My time only differs one or at maximum two ours from UTC. grafik

I think the hours are correct, even though CW41 is missing.

kadrim commented 2 years ago

I think the hours are correct, even though CW41 is missing.

yes, because with my patch both entries for CW40 are added to the tracking time. So i guess this fix is complete then.

@mtierltd can you have a look and if everything is ok, please merge PR https://github.com/mtierltd/timetracker/pull/156 ?

mtierltd / timetracker

Spend hours in goals not displayed #147