Possible bug? Blank timecard entries in timecard reports

[cmajel]

While using timecard data for the project reflections analysis, I noticed many entries that looked like this, where there would be 0 hours billed and 0% allocation:

I am unsure what would cause this, how this would be used, or if this is a straight up bug. I assume this would happen if a person submitted a tockline but kept it empty.

My assumption is this is a bug that would be corrected by not allowing empty entries.

Submitting this for other eyes and thoughts.

[cantsin]

Might be related: #1458

[neilmb]

Some research notes: This situation has always been present in the Tock database since the earliest days. But, changes made in October 2021 to introduce weekly billing made it MUCH more common.

SELECT to_char(created, 'YYYY-MM') AS year_month, COUNT(1) 
FROM hours_timecardobject 
WHERE hours_spent = 0 AND project_allocation = 0 
GROUP BY year_month 
ORDER BY year_month ASC;

year_month    count
2016-01	1
2016-08	1
2016-10	2
2016-11	40
2016-12	224
2017-01	234
2017-02	207
2017-03	196
2017-04	257
2017-05	206
2017-06	211
2017-07	235
2017-08	222
2017-09	208
2017-10	91
2017-11	121
2017-12	124
2018-01	78
2018-02	65
2018-03	98
2018-04	96
2018-05	139
2018-06	85
2018-07	77
2018-08	79
2018-09	51
2018-10	82
2018-11	37
2018-12	68
2019-01	53
2019-02	39
2019-03	37
2019-04	45
2019-05	47
2019-06	54
2019-07	24
2019-08	15
2019-09	20
2019-10	27
2019-11	30
2019-12	33
2020-01	32
2020-02	10
2020-03	10
2020-04	20
2020-05	21
2020-06	24
2020-07	40
2020-08	17
2020-09	15
2020-10	50
2020-11	35
2020-12	41
2021-01	35
2021-02	38
2021-03	35
2021-04	43
2021-05	6
2021-06	16
2021-07	17
2021-08	31
2021-09	29
2021-10	487
2021-11	526
2021-12	575
2022-01	686
2022-02	511
2022-03	569
2022-04	633
2022-05	541
2022-06	666
2022-07	701
2022-08	723
2022-09	796
2022-10	666
2022-11	717
2022-12	788
2023-01	763
2023-02	591
2023-03	848
2023-04	755
2023-05	657
2023-06	852
2023-07	576
2023-08	931
2023-09	809
2023-10	786
2023-11	781
2023-12	932
2024-01	939
2024-02	727
2024-03	436

[neilmb]

Unfortunately there are many different code paths that affect this. clean methods for both TimecardObjectForm and TimecardInlineFormset in tock/hours/forms.py have special logic that sometimes does complicated things.

https://github.com/18F/tock/blob/main/tock/hours/forms.py#L285 appears to be trying to raise an error that would disallow having timecardobject entries where both hours_spent and project_allocation are zero. Unfortunately that error can never be raised because form.cleaned_data.get("project_allocation") is a string and so it can never == 0.

https://github.com/18F/tock/blob/main/tock/hours/forms.py#L298 appears to be trying to remove line items from submitted time cards that had 0 hours spent and 0 project allocation. But, as above, that condition can never be true so those items don't get removed.

[neilmb]

Finally, there's a commented test here https://github.com/18F/tock/blob/main/tock/hours/tests/test_forms.py#L285-L298 that appears to have been removed when we were adding weekly billing, but never fixed or added back.

But, strangely enough https://github.com/18F/tock/blob/main/tock/hours/tests/test_forms.py#L275-L283 is an explicit test that putting 0 hours_spent for a project IS a valid timecard entry. Unfortunately that test doesn't specify why it should be valid. Is it because it should be okay so that it can trigger the DELETE logic from earlier?

[neilmb]

One of the hardest things about solving this problem is that I don't think that we have a UX design for what SHOULD happen when "empty" timecard entries are submitted. There are clues in this historical code about what people thought should happen at different times, but no clear design for what should occur.

The lack of design research resources for Tock can make it particularly hard to address bugs like this without just guessing what employee users AND data-analysis users want.

[neilmb]

I pushed a branch exploring this at https://github.com/18F/tock/tree/nmb/zero-hours

It fixes the string type comparison with project_allocation and adds a test case for the error message that is never triggered. It also makes a few more test cases fail because it changes the existing behavior with 0 entries.

...
======================================================================
FAIL: test_timecard_has_0_hours_for_project (hours.tests.test_forms.TimecardInlineFormSetTests)
Test timecard form data has an 0 hours spent value for
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tock/hours/tests/test_forms.py", line 284, in test_timecard_has_0_hours_for_project
    self.assertTrue(formset.is_valid())
AssertionError: False is not true

======================================================================
FAIL: test_timecard_has_empty_project (hours.tests.test_forms.TimecardInlineFormSetTests)
Test timecard form data has an empty hours spent value for
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tock/hours/tests/test_forms.py", line 274, in test_timecard_has_empty_project
    self.assertTrue(formset.is_valid())
AssertionError: False is not true

----------------------------------------------------------------------
Ran 388 tests in 111.694s

FAILED (failures=2, skipped=6)