7720: Invoice worklog-list hour summation

[jeppekroghitk]

Link to ticket

#7720

Description

Add sum to worklog list visible during creation of invocie worklog-line.

The selected hours summation changes when worklogs are selected in the list.

Screenshot of the result

Checklist

• My code is covered by test cases.
• My code passes our test (all our tests).
• My code passes our static analysis suite.
• My code passes our continuous integration process.

[tuj]

Generally fine.
According to Claude there is an issue with the sum when an invoice isBilled.
Please investigate this.

---

AI review:

Review of PR #328 — "7720: Invoice worklog-list hour summation"

Context

PR #328 (feature/7720-invoice-worklog-list-sum → develop, author jeppekroghitk, review
requested from tuj) adds an hour-summation bar to the worklog picker used when building a
WORKLOG invoice entry (/admin/invoices/{invoice}/entries/{invoiceEntry}/worklogs). It shows
two numbers: Timer i alt (server-rendered total for the current filter) and Valgte timer
(updated client-side as checkboxes change). The action row was moved out of the table into a
sticky div, and the filter query builder was extracted so the list and the sum can never drift
apart.

10 files, +151/−18. Branch is checked out locally; review below is from reading the diff and
surrounding code. Nothing has been executed — tests and static analysis have not been run.

What is good

• createFilterDataQueryBuilder() extraction (src/Repository/WorklogRepository.php:79) makes the
  list and the sum share one filter definition — they can't diverge by construction.
• hasSelectedHoursTarget guard in updateSelectedHours() keeps the shared entry-select
  controller safe for templates/invoices/index.html.twig, which uses the same controller with no
  such target.
• updateSelectedHours() is called from connect(), so worklogs already owned by the entry
  (pre-checked) are counted on load.
• (int) $sum with the null-on-empty comment is correct — SUM() returns NULL, not 0.
• Moving the button row out of <tr class="sticky-row"> into a div fixes sticky-on-table-row,
  which never worked reliably. sticky-row was used only here, so removing the CSS class is clean.
• colspan="8" → "9" is a correct drive-by fix — the table has always had 9 columns.
• The repository tests compare the SQL sum against the sum of findByFilterData(). That is the
  assertion that would catch join row-multiplication (hydration de-dupes root entities, SUM does
  not), so it is the right thing to assert — including the version-filter variant.

Findings, most important first

1. "Timer i alt" counts rows that cannot be selected — decide the intended semantics

templates/invoice_entry/worklogs.html.twig:47 sets disabled = worklog.isBilled or owned_by_other_invoice_entry, and a disabled row renders no checkbox at all. The total comes
from the unfiltered-by-that-condition repository sum, so:

• "Valgte timer" can never reach "Timer i alt" whenever the list contains billed worklogs.
• On a recorded invoice (all its worklogs become isBilled) the bar reads
  Valgte timer: 0 / Timer i alt: X — there is no recorded-invoice guard on this route
  (src/Controller/InvoiceEntryWorklogController.php:37), so this state is reachable.

Ask the author which is intended. If the total should mean "hours you could select", the sum has to
mirror the template's disabled condition (exclude isBilled = true, and invoiceEntry not this
entry). If it is deliberately "total of the filtered list", it is worth saying so in the label or
the PR description.

2. The second query is avoidable — the list is not paginated

findByFilterData() already returns every matching worklog (no Paginator, no
knp_pagination in this template), so the controller holds all the seconds in memory before it
asks the database to add them up again. sumTimeSpentSecondsByFilterData() re-runs the whole
filter, including the MEMBER OF EXISTS subqueries for version/epics.

Summing over $worklogs in the controller removes a full query and the new repository method.
Caveat: findByFilterData() is typed iterable, so a controller-side sum wants
is_array()/iterator_to_array() care rather than a blind double iteration.

Counter-argument for keeping it as is: if this list ever gets paginated, the SQL sum is the only
correct implementation, and it now has tests. Reasonable either way — worth a comment so it is a
decision rather than an accident.

3. InvoiceFullFlowTest asserts the total through a Tailwind class

tests/Integration/Controller/InvoiceFullFlowTest.php:183:
$sums->filter('span.font-bold')->last()->text() couples the test to a utility class and to DOM
ordering — the selected-hours span has the same class, and only ->last() distinguishes them.
Give the total a stable hook (a data-total-hours attribute, or a totalHours Stimulus target
mirroring selectedHours) and select on that.

Also, the assertion is only > 0. The test already resolves WorklogRepository, so it could
assert the exact expected total and actually verify the number rather than its presence.

4. The new client-side logic has no automated coverage

The selected-hours sum is the feature, and it is pure JS. The project has no JS test harness
(package.json has eslint/encore only), so this is a manual-verification item, not a missing-test
demand. Paths worth clicking through before approving:

• initial load of an entry that already owns worklogs → "Valgte timer" is non-zero on arrival;
• toggle-all on, then off → sum tracks both ways (note selectAll starts true and ignores
  current page state — pre-existing behaviour, not introduced here);
• a filter result where every row is disabled → sum stays 0, no console error;
• a filter result with no rows at all → the empty-state row and the sums bar both render.

5. Repository tests break the file's existing pattern

The three new tests use $this->entityManager->getRepository(InvoiceEntry::class), while all five
existing tests in the file use InvoiceEntryRepository from the container. Using the existing
pattern would be more consistent — but note that is exactly why the phpstan baseline entry
$entityManager is never read, only written could be dropped, so it is either this or removing the
now-unused property. Minor; mention, don't block.

They also depend on fixtures having non-zero seconds for admin@test.local and for
$project->getVersions()->first() (assertGreaterThan(0, $expected)), which will fail loudly if
fixtures change. Consistent with the rest of the suite's style.

6. Nits

• Rounding is inconsistent with the rows. The time column prints
  worklog.timeSpentSeconds / 60 / 60 unrounded, while total and selected round to 2 decimals — the
  displayed row values will not always add up to the displayed sums. Rounding also happens twice, in
  PHP (round($x / 3600, 2)) and again in JS (Math.round(x * 100) / 100).
• round() in the controller is presentation logic. Passing seconds and formatting in Twig
  (ideally one macro/filter shared with the row) keeps hour formatting in one place.
• Accessibility: aria-live="polite" on the selected-hours span would announce the value change.
• PR template compliance: the template requires a ticket link and, for UI changes, a screenshot.
  Confirm both are in the PR body — this is a UI change, and I could not verify the body reliably.
• Visual check: the table lost margin-bottom and the bar gained border-t. Worth a look at the
  spacing between the bar and bottom-actions.