18F / tock

We use Tock to track and report our time at 18F
https://18f.gsa.gov/2015/05/21/TockingTime/
Other
120 stars 37 forks source link

Prevent Inaccurate Labor Details When Timecards Have No Time Logged #1820

Closed edwintorres closed 3 weeks ago

edwintorres commented 1 month ago

Description:

There was an issue with an invoice that was generated this week. Although the invoice had the correct information overall, the labor detail was incorrect. Specifically, the labor detail included an entry for a person who had submitted a timecard for the project in September but had reported 0 hours.

This resulted in labor details being included for that person, even though no actual work was billed for the month.

Steps to Reproduce:

Expected Behavior:

Labor details should only be generated when a timecard has actual hours logged for the project. If a timecard is submitted with 0 hours, no labor details should be included for that person in the invoice or logged to the database.

Actual Behavior:

Labor details were generated for a person who submitted a timecard with 0 hours logged, leading to incorrect information in the invoice.

Suggested Solution:

Update the system so that timecards with 0 hours logged do not generate labor details in the invoice or not saved to the Tock database. This would prevent similar errors in the future where incorrect labor details are included without actual billing information.

edwintorres commented 1 month ago

Submitted by @rrefoy https://gsa-tts.slack.com/archives/C041X137T/p1729698228952729

nateborr commented 1 month ago

@edwintorres Can you provide direct reproduction steps for creating an invoice that demonstrates the issue? Or can you confirm what table in the Tock DB stores the labor details?

I'm looking at an employee report in staging and exploring the Tock DB schema and it's not clear to me what the labor detail data is.

nateborr commented 4 weeks ago

I discussed the issue with the finance team and they've updated the logic in their data pull for the labor detail script to exclude any blank timecard elements, which they expect to resolve the issue. A fix either in the labor detail script or in Tock itself would be more robust, and not dependent on assumptions about the data being passed between systems.

I recommend we update the labor detail script to filter out any timecard rows for a project/employee/week that have zero values for both hours and allocation. That effectively accepts that zero values are a valid user input in Tock.

Updating the labor detail script to filter out the invalid timecard rows has the advantages of avoiding changes for the Tock user experience and being usable for any existing, historical Tock data. The change appears reasonably straightforward but the main disadvantage is that the script is harder to maintain than Tock and it's infrequently run, making bugs harder to smoke out operationally.

If a timecard entry with zero values for both hours and allocation percentage is invalid, it would be most intuitive to update Tock to disallow those entries, most likely by overriding the clean() method on the TimecardObject class to add a validation. The main downsides I see to that approach are:

  1. Without an effort to scrub the existing data, invalid timecard objects would still be in the Tock DB, which could cause validation issues for timecard amendments or unexpected behavior when running the invoicing script against older data.
  2. The additional validation may cause friction for Tock users. In particular, we have anecdotal reports that some users lean on zero-valued timecard lines from period to period, to avoid the toil of having to look up billing codes for projects they intermittently contribute to.
nateborr commented 3 weeks ago

I confirmed with a Finance team member that the Recoverable Amount column in the input CSV file is not expected to have a nonzero value when the hours and allocation are both 0.