From d2a2d690fc2ea178a7d9ecb350676023cb34aa59 Mon Sep 17 00:00:00 2001 From: Ashwin Date: Sun, 16 Aug 2026 10:23:02 +0530 Subject: [PATCH] feat: add time report feature with CSV export - Implemented a new time report page in the frontend that displays totals by agent, ticket, or account over a specified date range. - Added a CSV export functionality for the time report, allowing users to download their report data. - Created API endpoint for exporting time report data in CSV format. - Developed data models and providers for managing time report data in the mobile application. - Integrated time report navigation into the mobile app, allowing users to access the report from the timesheet screen. - Added unit tests for time report data parsing and API request handling. --- backend/cases/analytics_views.py | 6 + backend/cases/signals.py | 24 +- backend/cases/tests/test_audit_log.py | 56 +++ backend/cases/tests/test_time_reports.py | 352 ++++++++++++++ backend/cases/time_entry_urls.py | 10 + backend/cases/time_reports.py | 203 ++++++++ backend/cases/time_views.py | 157 +++++- backend/common/renderers.py | 42 ++ backend/common/serializer.py | 6 + backend/pyproject.toml | 2 +- backend/uv.lock | 2 +- docs/api/endpoint-index.md | 2 + .../lib/server/v2/tickets-activity.test.js | 109 +++++ frontend/src/lib/server/v2/tickets.js | 26 +- frontend/src/lib/server/v2/timesheet.js | 207 +++++++- frontend/src/lib/server/v2/timesheet.test.js | 157 +++++- frontend/src/lib/v2/format.js | 17 + frontend/src/lib/v2/format.test.js | 29 +- .../routes/(app)/tickets/[id]/+page.server.js | 144 +++++- .../routes/(app)/tickets/[id]/+page.svelte | 458 +++++++++++++++++- .../src/routes/(app)/timesheet/+page.svelte | 12 +- .../(app)/timesheet/report/+page.server.js | 31 ++ .../(app)/timesheet/report/+page.svelte | 217 +++++++++ .../api/time-entries/report/export/+server.js | 49 ++ mobile/lib/config/api_config.dart | 24 + mobile/lib/data/models/time_report.dart | 125 +++++ .../lib/providers/time_report_provider.dart | 127 +++++ mobile/lib/routes/app_router.dart | 11 + .../screens/timesheet/time_report_screen.dart | 385 +++++++++++++++ .../screens/timesheet/timesheet_screen.dart | 11 + .../widgets/tickets/ticket_time_panel.dart | 40 +- mobile/test/unit/time_report_test.dart | 170 +++++++ 32 files changed, 3147 insertions(+), 64 deletions(-) create mode 100644 backend/cases/tests/test_time_reports.py create mode 100644 backend/cases/time_reports.py create mode 100644 backend/common/renderers.py create mode 100644 frontend/src/lib/server/v2/tickets-activity.test.js create mode 100644 frontend/src/routes/(app)/timesheet/report/+page.server.js create mode 100644 frontend/src/routes/(app)/timesheet/report/+page.svelte create mode 100644 frontend/src/routes/api/time-entries/report/export/+server.js create mode 100644 mobile/lib/data/models/time_report.dart create mode 100644 mobile/lib/providers/time_report_provider.dart create mode 100644 mobile/lib/screens/timesheet/time_report_screen.dart create mode 100644 mobile/test/unit/time_report_test.dart diff --git a/backend/cases/analytics_views.py b/backend/cases/analytics_views.py index 48054181..787059a4 100644 --- a/backend/cases/analytics_views.py +++ b/backend/cases/analytics_views.py @@ -30,6 +30,7 @@ from cases.analytics import DEFAULT_SERVICE_DAYS from cases.models import Case from cases.serializer import CaseSerializer from common.permissions import HasOrgContext +from common.renderers import CSV_RENDERERS from common.validators import uuid_param # --------------------------------------------------------------------------- @@ -272,6 +273,11 @@ def _iso_or_blank(value) -> str: class AnalyticsExportView(_AnalyticsBaseView): + # This endpoint answers in CSV, so it has to say it accepts CSV: content + # negotiation runs before the handler, and `src/routes/api/cases/analytics/ + # export/+server.js` sends `Accept: text/csv`, which was answered 406. + renderer_classes = CSV_RENDERERS + @extend_schema( tags=["cases-analytics"], parameters=_FILTER_PARAMS diff --git a/backend/cases/signals.py b/backend/cases/signals.py index d9329a6b..660ab54e 100644 --- a/backend/cases/signals.py +++ b/backend/cases/signals.py @@ -16,7 +16,7 @@ from __future__ import annotations import json import logging -from crum import get_current_user +from crum import get_current_request from django.contrib.contenttypes.models import ContentType from django.db.models.signals import ( m2m_changed, @@ -61,12 +61,22 @@ _TRACKED_SCALAR_FIELDS = ( def _resolve_actor(): - """Return a Profile to record as Activity.user, or None for system actions.""" - user = get_current_user() - if user is None or not getattr(user, "is_authenticated", False): - return None - profile = getattr(user, "profile", None) - return profile + """Return a Profile to record as Activity.user, or None for system actions. + + Read off the request, which `GetProfileAndOrg` has already resolved from + the JWT into the caller's Profile for this org. It used to read + ``get_current_user().profile``, and ``Profile.user`` is a reverse FK named + ``profiles``, so that attribute has never existed on User: the getattr + returned None on every request and the whole audit log was attributed to + "System". A user also has one Profile per org, so ``user.profile`` could + not have named the right one even if it resolved. + + ``request.profile`` is None for an anonymous request and for a customer + portal one (the portal's caller lands on ``request.portal_contact``, and a + Contact is not a Profile), so those still record as system actions. + """ + request = get_current_request() + return getattr(request, "profile", None) def _jsonify(value): diff --git a/backend/cases/tests/test_audit_log.py b/backend/cases/tests/test_audit_log.py index 1b839297..76a67c58 100644 --- a/backend/cases/tests/test_audit_log.py +++ b/backend/cases/tests/test_audit_log.py @@ -172,6 +172,62 @@ class TestActivityFeedAPI: assert response.status_code in (401, 403) +@pytest.mark.django_db +class TestTheActorIsRecorded: + """Who did it, not "System". + + `_resolve_actor` used to read `get_current_user().profile`, an attribute + that cannot exist (`Profile.user` is a reverse FK named `profiles`), so + every row in the audit log was written with `user=None` and every client + rendered the whole timeline as the work of a system account. + """ + + def test_a_change_made_through_the_api_names_the_person_who_made_it( + self, admin_client, admin_profile, case_a + ): + response = admin_client.patch( + f"/api/cases/{case_a.pk}/", + {"status": "Pending"}, + content_type="application/json", + ) + assert response.status_code == 200, response.content + + row = ( + _case_activities(case_a) + .filter(action="STATUS_CHANGED") + .latest("created_at") + ) + assert row.user_id == admin_profile.id + + def test_the_feed_serves_that_person_and_a_readable_verb( + self, admin_client, admin_profile, case_a + ): + admin_client.patch( + f"/api/cases/{case_a.pk}/", + {"priority": "Urgent"}, + content_type="application/json", + ) + + response = admin_client.get(f"/api/cases/{case_a.pk}/activities/") + assert response.status_code == 200 + row = next( + r + for r in response.json()["activities"] + if r["action"] == "PRIORITY_CHANGED" + ) + assert row["user"]["user_details"]["email"] == admin_profile.user.email + # The label off ACTION_CHOICES, so no client keeps its own verb list. + assert row["action_display"] == "Priority Changed" + + def test_work_with_no_request_behind_it_stays_a_system_action(self, case_a): + from cases.signals import _create_activity + + # No request in flight: a Celery task, a management command, the + # auto-stop sweep. Attributing those to a person would be a lie. + _create_activity(case_a, "UPDATE", {"changes": {}}) + assert _case_activities(case_a).latest("created_at").user_id is None + + @pytest.mark.django_db class TestMetadataTruncation: def test_oversized_metadata_truncated(self, admin_client, case_a, org_a): diff --git a/backend/cases/tests/test_time_reports.py b/backend/cases/tests/test_time_reports.py new file mode 100644 index 00000000..0462cbf5 --- /dev/null +++ b/backend/cases/tests/test_time_reports.py @@ -0,0 +1,352 @@ +"""Tests for the time reports: ``/api/time-entries/report/`` and its CSV. + +Covers the grouping and the money in ``cases/time_reports.py``, the window and +filter parsing in ``_report_entries``, and the two rules that decide what a +caller may see: an agent reports on their own time, an admin on the org's. +""" + +from __future__ import annotations + +import csv +import io +from datetime import timedelta +from decimal import Decimal + +import pytest +from crum import impersonate +from django.utils import timezone + +from accounts.models import Account +from cases.models import Case, TimeEntry + +REPORT = "/api/time-entries/report/" +EXPORT = "/api/time-entries/report/export/" + + +def _case(org, creator, name="Sample case", account=None): + with impersonate(creator): + return Case.objects.create( + name=name, status="New", priority="Normal", org=org, account=account + ) + + +def _logged(case, profile, minutes, *, days_ago=0, billable=False, rate=None, **kwargs): + """A stopped entry of `minutes`, ending `days_ago` days back.""" + ended = timezone.now() - timedelta(days=days_ago) + return TimeEntry.objects.create( + org=case.org, + case=case, + profile=profile, + started_at=ended - timedelta(minutes=minutes), + ended_at=ended, + billable=billable, + hourly_rate=Decimal(rate) if rate is not None else None, + **kwargs, + ) + + +@pytest.mark.django_db +class TestGrouping: + def test_groups_by_agent_with_the_billable_split_and_its_value( + self, admin_client, admin_user, admin_profile, user_profile, org_a + ): + case = _case(org_a, admin_user) + _logged(case, admin_profile, 60, billable=True, rate="90") + _logged(case, admin_profile, 30) + _logged(case, user_profile, 45, billable=True, rate="60") + + response = admin_client.get(REPORT, {"group_by": "agent"}) + + assert response.status_code == 200 + body = response.json() + assert body["group_by"] == "agent" + # Ordered by time logged, so the 90 minutes comes first. + assert [r["total_minutes"] for r in body["rows"]] == [90, 45] + first = body["rows"][0] + assert first["billable_minutes"] == 60 + assert first["billable_value"] == "90.00" + assert first["entry_count"] == 2 + assert body["totals"] == { + "total_minutes": 135, + "billable_minutes": 105, + # 90 for the hour, plus 45 minutes at 60/hr. + "billable_value": "135.00", + "entry_count": 3, + } + + def test_groups_by_ticket(self, admin_client, admin_user, admin_profile, org_a): + first = _case(org_a, admin_user, name="Printer down") + second = _case(org_a, admin_user, name="Password reset") + _logged(first, admin_profile, 25) + _logged(second, admin_profile, 50) + + rows = admin_client.get(REPORT, {"group_by": "ticket"}).json()["rows"] + + assert [(r["name"], r["total_minutes"]) for r in rows] == [ + ("Password reset", 50), + ("Printer down", 25), + ] + + def test_groups_by_account_and_names_the_unattributed_bucket( + self, admin_client, admin_user, admin_profile, org_a + ): + account = Account.objects.create(name="Reyes & Co", org=org_a) + _logged(_case(org_a, admin_user, account=account), admin_profile, 40) + # A ticket with no account still logged time, and time nobody can bill + # is the thing this report exists to surface, so it is named, not + # dropped. + _logged(_case(org_a, admin_user, name="Internal"), admin_profile, 20) + + rows = admin_client.get(REPORT, {"group_by": "account"}).json()["rows"] + + assert [(r["name"], r["key"] is None) for r in rows] == [ + ("Reyes & Co", False), + ("No account", True), + ] + + def test_a_running_timer_is_not_counted_until_it_is_stopped( + self, admin_client, admin_user, admin_profile, org_a + ): + case = _case(org_a, admin_user) + _logged(case, admin_profile, 30) + TimeEntry.objects.create( + org=org_a, case=case, profile=admin_profile, started_at=timezone.now() + ) + + body = admin_client.get(REPORT).json() + + assert body["totals"]["total_minutes"] == 30 + assert body["totals"]["entry_count"] == 1 + + def test_names_the_currencies_in_range_so_a_mixed_total_can_be_flagged( + self, admin_client, admin_user, admin_profile, org_a + ): + case = _case(org_a, admin_user) + _logged(case, admin_profile, 60, billable=True, rate="10", currency="USD") + _logged(case, admin_profile, 60, billable=True, rate="10", currency="EUR") + + body = admin_client.get(REPORT).json() + + assert body["currencies"] == ["EUR", "USD"] + + +@pytest.mark.django_db +class TestWindowAndFilters: + def test_defaults_to_the_last_30_days( + self, admin_client, admin_user, admin_profile, org_a + ): + case = _case(org_a, admin_user) + _logged(case, admin_profile, 15, days_ago=2) + _logged(case, admin_profile, 99, days_ago=45) + + body = admin_client.get(REPORT).json() + + assert body["totals"]["total_minutes"] == 15 + assert body["start"] == (timezone.localdate() - timedelta(days=29)).isoformat() + assert body["end"] == timezone.localdate().isoformat() + + def test_an_explicit_window_is_inclusive_at_both_ends( + self, admin_client, admin_user, admin_profile, org_a + ): + case = _case(org_a, admin_user) + _logged(case, admin_profile, 20, days_ago=3) + today = timezone.localdate() + + inside = admin_client.get( + REPORT, + { + "start": (today - timedelta(days=3)).isoformat(), + "end": (today - timedelta(days=3)).isoformat(), + }, + ).json() + outside = admin_client.get( + REPORT, + { + "start": (today - timedelta(days=2)).isoformat(), + "end": today.isoformat(), + }, + ).json() + + assert inside["totals"]["total_minutes"] == 20 + assert outside["totals"]["total_minutes"] == 0 + + def test_filters_by_account_and_by_billable( + self, admin_client, admin_user, admin_profile, org_a + ): + account = Account.objects.create(name="Reyes & Co", org=org_a) + billed = _case(org_a, admin_user, account=account) + _logged(billed, admin_profile, 60, billable=True, rate="50") + _logged(billed, admin_profile, 15) + _logged(_case(org_a, admin_user, name="Internal"), admin_profile, 30) + + by_account = admin_client.get(REPORT, {"account": str(account.id)}).json() + billable_only = admin_client.get( + REPORT, {"account": str(account.id), "billable": "true"} + ).json() + + assert by_account["totals"]["total_minutes"] == 75 + assert billable_only["totals"]["total_minutes"] == 60 + + def test_rejects_a_malformed_date_a_backwards_window_and_a_bad_flag( + self, admin_client + ): + assert ( + admin_client.get( + REPORT, {"start": "16-08-2026", "end": "2026-08-16"} + ).status_code + == 400 + ) + assert ( + admin_client.get( + REPORT, {"start": "2026-08-16", "end": "2026-08-01"} + ).status_code + == 400 + ) + assert admin_client.get(REPORT, {"billable": "yes"}).status_code == 400 + assert admin_client.get(REPORT, {"group_by": "planet"}).status_code == 400 + + def test_unauthenticated_is_refused(self, unauthenticated_client): + assert unauthenticated_client.get(REPORT).status_code in (401, 403) + + +@pytest.mark.django_db +class TestWhoMaySeeWhat: + def test_an_agent_reports_on_their_own_time_only( + self, user_client, admin_user, admin_profile, user_profile, org_a + ): + case = _case(org_a, admin_user) + _logged(case, admin_profile, 60) + _logged(case, user_profile, 20) + + body = user_client.get(REPORT).json() + + assert body["totals"]["total_minutes"] == 20 + assert [r["total_minutes"] for r in body["rows"]] == [20] + + def test_an_agent_asking_for_someone_elses_time_is_refused_not_emptied( + self, user_client, admin_profile + ): + response = user_client.get(REPORT, {"profile": str(admin_profile.id)}) + + # An empty report would read as "that person logged nothing". + assert response.status_code == 403 + + def test_an_admin_may_report_on_one_agent( + self, admin_client, admin_user, admin_profile, user_profile, org_a + ): + case = _case(org_a, admin_user) + _logged(case, admin_profile, 60) + _logged(case, user_profile, 20) + + body = admin_client.get(REPORT, {"profile": str(user_profile.id)}).json() + + assert body["totals"]["total_minutes"] == 20 + + def test_another_org_is_not_in_the_report( + self, admin_client, admin_profile, user_b, profile_b, org_a, org_b + ): + _logged(_case(org_a, admin_profile.user), admin_profile, 10) + _logged(_case(org_b, user_b, name="Theirs"), profile_b, 500) + + body = admin_client.get(REPORT).json() + + assert body["totals"]["total_minutes"] == 10 + + +@pytest.mark.django_db +class TestCsvExport: + def _rows(self, response): + text = b"".join(response.streaming_content).decode() + return list(csv.reader(io.StringIO(text))) + + def test_serves_one_row_per_entry_with_the_billing_columns( + self, admin_client, admin_user, admin_profile, org_a + ): + account = Account.objects.create(name="Reyes & Co", org=org_a) + case = _case(org_a, admin_user, name="Printer down", account=account) + _logged( + case, + admin_profile, + 45, + billable=True, + rate="80", + description="Traced the failed import", + ) + + response = admin_client.get(EXPORT) + + assert response.status_code == 200 + assert response["Content-Type"] == "text/csv" + assert "attachment; filename=" in response["Content-Disposition"] + + header, row = self._rows(response) + assert header[:5] == ["Date", "Agent", "Ticket", "Account", "Description"] + assert row[2] == "Printer down" + assert row[3] == "Reyes & Co" + assert row[4] == "Traced the failed import" + assert row[5] == "45" + assert row[6] == "0.75" + assert row[7] == "yes" + assert row[10] == "60.00" + + def test_a_non_billable_entry_has_no_value_rather_than_a_zero( + self, admin_client, admin_user, admin_profile, org_a + ): + _logged(_case(org_a, admin_user), admin_profile, 30) + + _header, row = self._rows(admin_client.get(EXPORT)) + + assert row[7] == "no" + assert row[8] == "" + assert row[10] == "" + + def test_carries_the_same_window_and_the_same_visibility_as_the_report( + self, user_client, admin_user, admin_profile, user_profile, org_a + ): + case = _case(org_a, admin_user) + _logged(case, admin_profile, 60, description="Not yours") + _logged(case, user_profile, 20, description="Yours") + _logged(case, user_profile, 15, days_ago=90, description="Too old") + + rows = self._rows(user_client.get(EXPORT)) + + assert [r[4] for r in rows[1:]] == ["Yours"] + + def test_an_agent_cannot_export_another_agents_time( + self, user_client, admin_profile + ): + assert ( + user_client.get(EXPORT, {"profile": str(admin_profile.id)}).status_code + == 403 + ) + + +@pytest.mark.django_db +class TestTheDownloadIsReachable: + """A client that says what it wants gets it. + + Content negotiation runs before the handler, so a view with no CSV + renderer answers `Accept: text/csv` with 406 and never runs. Both export + proxies in the SvelteKit app send exactly that header, so both downloads + were broken from the browser while `curl` with no Accept header worked. + """ + + def test_the_time_export_accepts_a_csv_request( + self, admin_client, admin_user, admin_profile, org_a + ): + _logged(_case(org_a, admin_user), admin_profile, 30) + + response = admin_client.get(EXPORT, headers={"accept": "text/csv"}) + + assert response.status_code == 200 + assert response["Content-Type"] == "text/csv" + + def test_the_analytics_export_accepts_one_too(self, admin_client): + response = admin_client.get( + "/api/cases/analytics/export/", + {"metric": "frt"}, + headers={"accept": "text/csv"}, + ) + + assert response.status_code == 200 + assert response["Content-Type"] == "text/csv" diff --git a/backend/cases/time_entry_urls.py b/backend/cases/time_entry_urls.py index c8588663..9a01f5a5 100644 --- a/backend/cases/time_entry_urls.py +++ b/backend/cases/time_entry_urls.py @@ -16,6 +16,16 @@ urlpatterns = [ time_views.TimesheetView.as_view(), name="time_entries_timesheet", ), + path( + "report/", + time_views.TimeReportView.as_view(), + name="time_entries_report", + ), + path( + "report/export/", + time_views.TimeReportExportView.as_view(), + name="time_entries_report_export", + ), path( "unbilled/", time_views.UnbilledEntriesView.as_view(), diff --git a/backend/cases/time_reports.py b/backend/cases/time_reports.py new file mode 100644 index 00000000..7f33c262 --- /dev/null +++ b/backend/cases/time_reports.py @@ -0,0 +1,203 @@ +"""Grouped totals and the CSV rows behind the time reports. + +Kept beside ``time_views.py`` for the same reason ``analytics.py`` sits beside +``analytics_views.py``: the grouping and the money are the parts worth testing +without an HTTP layer wrapped around them. + +Two shapes come out of here. :func:`build_report` answers "where did the time +go", one row per agent, ticket or account. :func:`csv_rows` answers "give me +the entries", one row per :class:`cases.models.TimeEntry`, which is what a +finance team pastes into a spreadsheet and what an invoice is checked against. + +Both read only stopped entries. A running timer has ``duration_minutes`` 0 +until it is stopped, so counting one would report a morning's work as nothing +and quietly drag an agent's total down. +""" + +from __future__ import annotations + +from decimal import ROUND_HALF_UP, Decimal + +from django.db.models import ( + Count, + DecimalField, + ExpressionWrapper, + F, + Q, + Sum, + Value, +) +from django.utils import timezone +from rest_framework import status + +GROUPINGS = ("agent", "ticket", "account") + +# The columns each grouping needs out of the row. The first is the key. +_GROUP_FIELDS = { + "agent": ("profile_id", "profile__user__name", "profile__user__email"), + "ticket": ("case_id", "case__name"), + "account": ("case__account_id", "case__account__name"), +} + +_BILLABLE = Q(billable=True) & Q(hourly_rate__isnull=False) + +# Money, computed in the database at the rate stored on each entry rather than +# today's rate: `hourly_rate` is snapshotted per entry precisely so that a rate +# change next month does not silently rewrite what last month was worth. +# `decimal_places=4` because minutes/60 is a third of a cent on odd durations, +# and rounding is done once, on the sum, not on every row of it. +_VALUE = ExpressionWrapper( + F("duration_minutes") * F("hourly_rate") / Value(Decimal("60")), + output_field=DecimalField(max_digits=16, decimal_places=4), +) + +CSV_COLUMNS = ( + "Date", + "Agent", + "Ticket", + "Account", + "Description", + "Minutes", + "Hours", + "Billable", + "Rate", + "Currency", + "Value", + "Invoice", +) + + +class TimeReportParamError(Exception): + """A query parameter the report will not run on. + + Carries the status as well as the message so the caller does not have to + guess whether a rejected ``profile`` is a 400 or a 403; reading another + agent's time without being an admin is the second one. + """ + + def __init__(self, detail, status_code=status.HTTP_400_BAD_REQUEST): + super().__init__(detail) + self.detail = detail + self.status_code = status_code + + +def money(value): + """A Decimal rounded to cents, as a string. ``None`` becomes ``"0.00"``.""" + amount = Decimal(value or 0) + return str(amount.quantize(Decimal("0.01"), rounding=ROUND_HALF_UP)) + + +def hours(minutes): + """Minutes as decimal hours, to two places: 45 minutes is ``"0.75"``.""" + return str( + (Decimal(minutes or 0) / Decimal(60)).quantize( + Decimal("0.01"), rounding=ROUND_HALF_UP + ) + ) + + +def build_report(entries, group_by): + """Group ``entries`` and return ``{rows, totals, currencies}``. + + ``entries`` is expected to be scoped and filtered already: this function + does not know who is asking, and adding an org filter here would read as + though it did. + + Rows are ordered by time logged, descending, because the question the + report answers is which agent, ticket or account took the most of it. A + row's ``key`` is null for time on tickets with no account; that bucket is + named rather than dropped, since unattributed time is exactly what someone + running this report is looking for. + """ + if group_by not in _GROUP_FIELDS: + raise TimeReportParamError( + f"group_by must be one of {', '.join(GROUPINGS)}, not {group_by!r}." + ) + + fields = _GROUP_FIELDS[group_by] + grouped = ( + entries.values(*fields) + .annotate( + total_minutes=Sum("duration_minutes"), + billable_minutes=Sum("duration_minutes", filter=Q(billable=True)), + billable_value=Sum(_VALUE, filter=_BILLABLE), + entry_count=Count("id"), + ) + .order_by("-total_minutes") + ) + + rows = [] + for row in grouped: + key = row[fields[0]] + rows.append( + { + "key": str(key) if key else None, + "name": _row_name(group_by, row, fields), + "total_minutes": row["total_minutes"] or 0, + "billable_minutes": row["billable_minutes"] or 0, + "billable_value": money(row["billable_value"]), + "entry_count": row["entry_count"], + } + ) + + totals = { + "total_minutes": sum(r["total_minutes"] for r in rows), + "billable_minutes": sum(r["billable_minutes"] for r in rows), + "billable_value": money(sum(Decimal(r["billable_value"]) for r in rows)), + "entry_count": sum(r["entry_count"] for r in rows), + } + + # Every currency present in the range, so a client can say "these totals + # mix currencies" instead of printing one symbol over a sum of two. The + # value column adds them regardless, which is the same compromise the + # timesheet page and the shell's pipeline figures already make. + currencies = sorted(set(entries.values_list("currency", flat=True).distinct())) + + return {"rows": rows, "totals": totals, "currencies": currencies} + + +def _row_name(group_by, row, fields): + if group_by == "agent": + # `User.name` is never blank (it falls back to the email local-part on + # first save), but an entry can outlive nothing here, so the email is + # the backstop rather than an empty cell. + return row[fields[1]] or row[fields[2]] or "Unknown agent" + if group_by == "ticket": + return row[fields[1]] or "Deleted ticket" + return row[fields[1]] or "No account" + + +def csv_rows(entries): + """Yield the header, then one list per entry, for ``csv.writer``. + + A generator over ``.iterator()`` so a year of entries streams instead of + being built in memory: the caller wraps this in a ``StreamingHttpResponse``. + """ + yield list(CSV_COLUMNS) + + queryset = entries.select_related( + "profile__user", "case", "case__account", "invoice" + ).order_by("started_at") + + for entry in queryset.iterator(chunk_size=200): + user = entry.profile.user if entry.profile_id else None + account = entry.case.account if entry.case_id else None + value = ( + money(Decimal(entry.duration_minutes) * entry.hourly_rate / Decimal(60)) + if entry.billable and entry.hourly_rate is not None + else "" + ) + yield [ + timezone.localdate(entry.started_at).isoformat(), + (user.name or user.email) if user else "", + entry.case.name if entry.case_id else "", + account.name if account else "", + entry.description or "", + entry.duration_minutes, + hours(entry.duration_minutes), + "yes" if entry.billable else "no", + str(entry.hourly_rate) if entry.hourly_rate is not None else "", + entry.currency, + value, + entry.invoice.invoice_number if entry.invoice_id else "", + ] diff --git a/backend/cases/time_views.py b/backend/cases/time_views.py index 43095651..9ccd6851 100644 --- a/backend/cases/time_views.py +++ b/backend/cases/time_views.py @@ -14,13 +14,17 @@ Entry-scoped (registered at the project root under ``/api/time-entries/``): * ``PUT /api/time-entries//``: owner or admin * ``DELETE /api/time-entries//``: owner or admin * ``GET /api/time-entries/timesheet/``: week view, grouped by day +* ``GET /api/time-entries/report/``: totals by agent, ticket or account +* ``GET /api/time-entries/report/export/``: the same window as ``text/csv`` """ +import csv from collections import OrderedDict from datetime import datetime, timedelta from django.db import IntegrityError, transaction from django.db.models import Sum +from django.http import StreamingHttpResponse from django.shortcuts import get_object_or_404 from django.utils import timezone from rest_framework import status @@ -28,16 +32,29 @@ from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView +from cases import time_reports + +# The file-like shim `csv.writer` needs to stream instead of buffer. Imported +# rather than copied: `cases/analytics_views.py` already exports its CSV the +# same way, and two of these would be two places to fix a streaming bug. +from cases.analytics_views import _Echo from cases.models import Case, TimeEntry from cases.serializer import ( TimeEntryCreateSerializer, TimeEntrySerializer, TimeEntryUpdateSerializer, ) +from cases.time_reports import TimeReportParamError from common.models import Profile from common.permissions import HasOrgContext, is_org_admin +from common.renderers import CSV_RENDERERS from common.validators import uuid_param +# How far back a report reaches when it is asked for without a window. Long +# enough to cover "what did we do this month", short enough that a stray click +# does not scan a year. +DEFAULT_REPORT_DAYS = 30 + def _visible_entry_qs(profile): """Org-scoped queryset honouring agent-vs-admin visibility.""" @@ -47,6 +64,72 @@ def _visible_entry_qs(profile): return qs.select_related("profile", "profile__user", "case") +def _parse_date(value): + """``YYYY-MM-DD`` to a date, or None when absent. Raises on anything else. + + Shared by the timesheet and the reports so a malformed date is answered + the same way on both, rather than one of them reading it as today. + """ + if not value: + return None + try: + return datetime.strptime(value, "%Y-%m-%d").date() + except (TypeError, ValueError) as exc: + raise ValueError(f"Invalid date {value!r}; expected YYYY-MM-DD.") from exc + + +def _report_entries(request): + """Return ``(entries, start, end)`` for a report request. + + Only stopped entries, inside the window, narrowed by the optional + ``profile``, ``account`` and ``billable`` filters. Visibility is + `_visible_entry_qs`'s: an agent reports on their own time, an admin on the + org's, and asking for somebody else's without being an admin is a 403 + rather than an empty report, because a silent empty answer reads as "that + person logged nothing". + + Raises :class:`TimeReportParamError`, which the views turn into a response. + """ + profile = request.profile + entries = _visible_entry_qs(profile).filter(ended_at__isnull=False) + + target = uuid_param(request.query_params, "profile") + if target: + if target != str(profile.id) and not is_org_admin(profile): + raise TimeReportParamError( + "Only admins can report on another profile's time.", + status.HTTP_403_FORBIDDEN, + ) + entries = entries.filter(profile_id=target) + + account = uuid_param(request.query_params, "account") + if account: + entries = entries.filter(case__account_id=account) + + billable = request.query_params.get("billable") + if billable is not None: + if billable not in ("true", "false"): + raise TimeReportParamError("billable must be 'true' or 'false'.") + entries = entries.filter(billable=(billable == "true")) + + try: + start = _parse_date(request.query_params.get("start")) + end = _parse_date(request.query_params.get("end")) + except ValueError as exc: + raise TimeReportParamError(str(exc)) from exc + + # Both or neither, the same rule the timesheet uses: half a window is + # more likely a mistake than a request to run from a date to whenever. + if start is None or end is None: + end = timezone.localdate() + start = end - timedelta(days=DEFAULT_REPORT_DAYS - 1) + if end < start: + raise TimeReportParamError("end must be on or after start.") + + entries = entries.filter(started_at__date__gte=start, started_at__date__lte=end) + return entries, start, end + + class TimeEntryListCreateView(APIView): permission_classes = (IsAuthenticated, HasOrgContext) @@ -277,8 +360,8 @@ class TimesheetView(APIView): target_profile_id = profile_param try: - start = self._parse_date(request.query_params.get("start")) - end = self._parse_date(request.query_params.get("end")) + start = _parse_date(request.query_params.get("start")) + end = _parse_date(request.query_params.get("end")) except ValueError as exc: return Response({"detail": str(exc)}, status=status.HTTP_400_BAD_REQUEST) if start is None or end is None: @@ -390,11 +473,67 @@ class TimesheetView(APIView): } ) - @staticmethod - def _parse_date(value): - if not value: - return None + +class TimeReportView(APIView): + """``GET /api/time-entries/report/``: where the time went. + + ``?start=&end=`` (YYYY-MM-DD, inclusive, defaulting to the last 30 days), + ``?group_by=agent|ticket|account``, and the optional ``profile``, + ``account`` and ``billable`` filters. One row per group with the minutes, + the billable split, the value at each entry's own rate, and how many + entries went into it. + + The productivity half of the answer. For the billing half, the same + window comes out of ``report/export/`` as one row per entry. + """ + + permission_classes = (IsAuthenticated, HasOrgContext) + + def get(self, request): + group_by = request.query_params.get("group_by") or "agent" try: - return datetime.strptime(value, "%Y-%m-%d").date() - except (TypeError, ValueError) as exc: - raise ValueError(f"Invalid date {value!r}; expected YYYY-MM-DD.") from exc + entries, start, end = _report_entries(request) + report = time_reports.build_report(entries, group_by) + except TimeReportParamError as exc: + return Response({"detail": exc.detail}, status=exc.status_code) + + return Response( + { + "start": start.isoformat(), + "end": end.isoformat(), + "group_by": group_by, + **report, + } + ) + + +class TimeReportExportView(APIView): + """``GET /api/time-entries/report/export/``: the same window as CSV. + + One row per entry, not per group: an invoice is checked against the + entries behind it, and a spreadsheet can group them again in whatever way + the person opening it needs. + + Streams, so a year of entries does not have to be built in memory first. + Same filters and the same visibility as the report above. + """ + + permission_classes = (IsAuthenticated, HasOrgContext) + # Content negotiation runs before the handler, so a download proxy asking + # for `Accept: text/csv` was answered 406 without this. + renderer_classes = CSV_RENDERERS + + def get(self, request): + try: + entries, start, end = _report_entries(request) + except TimeReportParamError as exc: + return Response({"detail": exc.detail}, status=exc.status_code) + + writer = csv.writer(_Echo()) + stream = (writer.writerow(row) for row in time_reports.csv_rows(entries)) + + response = StreamingHttpResponse(stream, content_type="text/csv") + response["Content-Disposition"] = ( + f'attachment; filename="time-{start.isoformat()}-to-{end.isoformat()}.csv"' + ) + return response diff --git a/backend/common/renderers.py b/backend/common/renderers.py new file mode 100644 index 00000000..0f943817 --- /dev/null +++ b/backend/common/renderers.py @@ -0,0 +1,42 @@ +"""Renderers for endpoints that answer with something other than JSON.""" + +import json + +from rest_framework.renderers import BaseRenderer, BrowsableAPIRenderer, JSONRenderer + + +class CsvRenderer(BaseRenderer): + """Lets a view accept ``Accept: text/csv``. + + Nothing is rendered through this in the normal case: the CSV endpoints + build a :class:`django.http.StreamingHttpResponse` themselves so a large + export streams instead of being assembled in memory, and DRF hands a plain + ``HttpResponse`` straight to the client. + + It exists because content negotiation runs *before* the handler. Without a + renderer claiming ``text/csv``, a client that politely says what it wants + is answered 406 and the handler never runs, which is what both CSV exports + did to their own download proxies. + + ``render`` is still implemented, because a view can return a DRF + ``Response`` on the error path (a bad date, a forbidden profile) while + ``text/csv`` is the negotiated type. That body is JSON, and saying so in + bytes beats handing Django a dict it cannot write. + """ + + media_type = "text/csv" + format = "csv" + charset = "utf-8" + + def render(self, data, accepted_media_type=None, renderer_context=None): + if data is None: + return "" + if isinstance(data, str): + return data + return json.dumps(data) + + +# What a CSV endpoint declares: JSON first, so an error keeps its usual shape +# for a client that did not ask for anything in particular, plus CSV, plus the +# browsable renderer DRF's own default list carries for the API explorer. +CSV_RENDERERS = (JSONRenderer, CsvRenderer, BrowsableAPIRenderer) diff --git a/backend/common/serializer.py b/backend/common/serializer.py index e0271477..a8e4f332 100644 --- a/backend/common/serializer.py +++ b/backend/common/serializer.py @@ -445,12 +445,18 @@ class ActivitySerializer(serializers.ModelSerializer): """Activity timeline row, used by audit-log feeds (Cases first).""" user = CommentUserSerializer(read_only=True) + # The label from `Activity.ACTION_CHOICES` ("Time Logged"), so no client + # has to keep its own copy of a verb list that lives on the model. Both + # the web and the Flutter timeline already read this key and were falling + # back to the raw enum, which is why they printed "TIME_LOGGED". + action_display = serializers.CharField(source="get_action_display", read_only=True) class Meta: model = Activity fields = ( "id", "action", + "action_display", "user", "entity_type", "entity_id", diff --git a/backend/pyproject.toml b/backend/pyproject.toml index f8176376..33e04865 100644 --- a/backend/pyproject.toml +++ b/backend/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "django-crm" -version = "1.6.0" +version = "1.7.0" # The one-line summary PyPI shows under the project title and in search results, # so it carries the terms people actually search for ("open source CRM", # "self-hosted CRM", "Django CRM") and names the modules rather than the diff --git a/backend/uv.lock b/backend/uv.lock index 638db6e0..72ccd37f 100644 --- a/backend/uv.lock +++ b/backend/uv.lock @@ -583,7 +583,7 @@ wheels = [ [[package]] name = "django-crm" -version = "1.5.0" +version = "1.7.0" source = { editable = "." } dependencies = [ { name = "cairocffi" }, diff --git a/docs/api/endpoint-index.md b/docs/api/endpoint-index.md index 0a307566..ce02a5ae 100644 --- a/docs/api/endpoint-index.md +++ b/docs/api/endpoint-index.md @@ -224,6 +224,8 @@ Interactive versions of the same schema are served by a running backend at | `/api/tasks/{id}/move/` | PATCH | | `/api/teams/` | GET, POST | | `/api/teams/{id}/` | DELETE, GET, PATCH, PUT | +| `/api/time-entries/report/` | GET | +| `/api/time-entries/report/export/` | GET | | `/api/time-entries/timesheet/` | GET | | `/api/time-entries/unbilled/` | GET | | `/api/time-entries/{id}/` | DELETE, PUT | diff --git a/frontend/src/lib/server/v2/tickets-activity.test.js b/frontend/src/lib/server/v2/tickets-activity.test.js new file mode 100644 index 00000000..b481f1de --- /dev/null +++ b/frontend/src/lib/server/v2/tickets-activity.test.js @@ -0,0 +1,109 @@ +/** + * What the rail reads off a ticket: the activity feed, and the time totals. + * + * Both are plain passthroughs of `ActivitySerializer` and `CaseSerializer` + * keys, and both were wrong in the same way at some point: a key that the API + * does not emit renders as an empty string rather than as an error, so the + * page looks fine and says nothing. These pin the key names. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const apiRequest = vi.fn(); +vi.mock('$lib/api-helpers.js', () => ({ apiRequest: (...a) => apiRequest(...a) })); + +const { getTicket } = await import('./tickets.js'); + +const event = /** @type {any} */ ({ cookies: { get: () => 'token' } }); + +/** A detail payload with no account, so `getTicket` makes exactly one call. */ +function detail(extra = {}) { + return { + cases_obj: { id: 'c1', name: 'Printer down', status: 'New', priority: 'Low' }, + comment_permission: false, + ...extra + }; +} + +describe('the activity feed', () => { + beforeEach(() => { + apiRequest.mockReset(); + }); + + it('reads the time, the verb and the actor from the keys the API sends', async () => { + apiRequest.mockResolvedValue( + detail({ + activities: [ + { + id: 'a1', + action: 'TIME_LOGGED', + action_display: 'Time Logged', + created_at: '2026-08-16T09:00:00Z', + user: { id: 'p1', user_details: { email: 'agent@example.com' } } + } + ] + }) + ); + + const { activity } = await getTicket(event, 'c1'); + + expect(activity).toEqual([ + { + id: 'a1', + action: 'TIME_LOGGED', + label: 'Time Logged', + at: '2026-08-16T09:00:00Z', + by: 'agent@example.com' + } + ]); + }); + + it('falls back to the raw verb, and calls an actorless row nobody', async () => { + // A Celery task or a management command writes rows with no user; the + // page renders "System" for a null `by`, which is the truth there. + apiRequest.mockResolvedValue( + detail({ + activities: [ + { id: 'a2', action: 'ESCALATED', created_at: '2026-08-16T09:00:00Z', user: null } + ] + }) + ); + + const { activity } = await getTicket(event, 'c1'); + + expect(activity[0].label).toBe('ESCALATED'); + expect(activity[0].by).toBe(null); + expect(activity[0].at).toBe('2026-08-16T09:00:00Z'); + }); +}); + +describe('the time totals on the ticket', () => { + beforeEach(() => { + apiRequest.mockReset(); + }); + + it('carries the whole-ticket summary through for the time panel', async () => { + apiRequest.mockResolvedValue( + detail({ + cases_obj: { + id: 'c1', + name: 'Printer down', + status: 'New', + priority: 'Low', + time_summary: { total_minutes: 105, billable_minutes: 45, by_profile: [] } + } + }) + ); + + const { ticket } = await getTicket(event, 'c1'); + + expect(ticket.time_summary.total_minutes).toBe(105); + }); + + it('is null on a row from the list endpoint, which does not carry it', async () => { + apiRequest.mockResolvedValue(detail()); + + const { ticket } = await getTicket(event, 'c1'); + + expect(ticket.time_summary).toBe(null); + }); +}); diff --git a/frontend/src/lib/server/v2/tickets.js b/frontend/src/lib/server/v2/tickets.js index f07dabd1..ac8a83eb 100644 --- a/frontend/src/lib/server/v2/tickets.js +++ b/frontend/src/lib/server/v2/tickets.js @@ -111,6 +111,12 @@ function toRow(row) { paused_at: row.sla_paused_at ?? null, child_count: row.child_count ?? 0, parent: row.parent_summary ?? null, + // Everyone's logged time on this ticket, which is not what + // `/cases//time-entries/` returns to an agent: that list is narrowed + // to their own rows, so a panel adding it up would tell a team of three + // the ticket had taken a third of the time it had. Absent on the list + // endpoint (`slim=true`), hence the null. + time_summary: row.time_summary ?? null, is_open: OPEN_STATUSES.includes(row.status) }; } @@ -264,20 +270,22 @@ export async function getTicket({ cookies }, id) { url: attachmentHref(file.id), at: file.created_at })), - // `ActivitySerializer` names the time `timestamp`, not `created_at`, and - // supplies its own label for the verb. Both are used as given. + // `ActivitySerializer` (`common/serializer.py`) names the time + // `created_at` and the verb `action_display`, and puts the actor's email + // one level in, under `user.user_details`. // - // Worth knowing before reading that serializer: `common/serializer.py` - // defines **two** classes called `ActivitySerializer`, at lines 344 and - // 843. The second shadows the first, so the one with `metadata` and - // `created_at` is unreachable and coding against it silently gets you - // nothing. This maps the one that actually runs. + // This read `row.timestamp` and `row.user.email`, neither of which the + // serializer has ever emitted, so every row in the rail rendered as + // "System · — ago". A comment here used to justify `timestamp` by saying + // two classes named `ActivitySerializer` existed and the shadowing one + // used it; there is one class today, and + // `common/tests/test_activity_serializer_shapes.py` pins its shape. activity: (response.activities ?? []).map((/** @type {any} */ row) => ({ id: row.id, action: row.action, label: row.action_display || row.action, - at: row.timestamp, - by: row.user?.user_details?.email || row.user?.email || null + at: row.created_at, + by: row.user?.user_details?.email ?? null })), // The API's own answer about whether this person may reply, rather than a // guess from their role. It used to disagree with the endpoint. diff --git a/frontend/src/lib/server/v2/timesheet.js b/frontend/src/lib/server/v2/timesheet.js index 746dfbfa..f16087e1 100644 --- a/frontend/src/lib/server/v2/timesheet.js +++ b/frontend/src/lib/server/v2/timesheet.js @@ -1,19 +1,26 @@ /** - * Weekly timesheet: the wiring behind /v2/timesheet. + * Time entries: the wiring behind /v2/timesheet and the ticket's time panel. * * Server-only. `GET /time-entries/timesheet/` returns the caller's own week * grouped into day buckets (every day present, empty or not) with week * totals, the billable split, and a running-timer count. The page reads it * verbatim as `data.week`. * - * The v2 page is always "your timesheet", it has no profile switcher, so this - * layer never passes a `profile`, which also keeps it clear of the endpoint's - * admin-only "another profile" 403. `TimesheetView` already expands `case` and - * `invoice` to `{id, name}` / `{id, invoice_number}` and names the profile, so - * there is no reshaping here. + * The v2 timesheet page is always "your timesheet", it has no profile + * switcher, so this layer never passes a `profile`, which also keeps it clear + * of the endpoint's admin-only "another profile" 403. `TimesheetView` already + * expands `case` and `invoice` to `{id, name}` / `{id, invoice_number}` and + * names the profile, so there is no reshaping here. * - * `stopTimer` is the one write path here: `POST /time-entries//stop/`. - * Nothing else on this page is editable. + * The rest is the ticket panel's surface, ticket-scoped on the way in + * (`/cases//time-entries/`) and entry-scoped on the way back out + * (`/time-entries//`), which is how the API splits it. Every write lands + * here rather than in `tickets.js` so one file holds what the app knows about + * time entries; `stopTimer` is shared by both callers already. + * + * None of these check who owns an entry. The API does: it narrows a list to + * the caller's own rows for a non-admin, and answers 403 on someone else's + * entry. Nothing below should be read as having re-checked that. */ import { apiRequest } from '$lib/api-helpers.js'; @@ -69,3 +76,187 @@ export async function stopTimer({ cookies }, entryId) { return await apiRequest(`/time-entries/${entryId}/stop/`, { method: 'POST' }, { cookies }); } + +/** Longest manual entry the form will send, in minutes. A day of work logged + * in one go is already unusual; more than that is a typo, and the API has no + * ceiling of its own to catch it. */ +const MAX_ENTRY_MINUTES = 1440; + +/** The groupings `/time-entries/report/` accepts. */ +export const REPORT_GROUPINGS = ['agent', 'ticket', 'account']; + +/** + * Where the time went over a window: totals by agent, ticket or account. + * + * `group_by` and `billable` are checked against what the API accepts rather + * than forwarded blind. Both arrive from the query string, so a hand-edited + * URL would otherwise turn a 400 into a 500 page; an unknown value falls back + * to the default instead, which is what the filter bar shows anyway. + * + * The window is passed through as given (YYYY-MM-DD, inclusive). Left off, the + * API answers for the last 30 days and says which days those were, so the page + * never has to guess a range it did not choose. + * + * @param {{ cookies: import('@sveltejs/kit').Cookies }} event + * @param {{ start?: string, end?: string, group_by?: string, billable?: string, account?: string, profile?: string }} [filters] + * @returns {Promise<{ report: any }>} + */ +export async function getTimeReport({ cookies }, filters = {}) { + const qs = new URLSearchParams(); + if (filters.start) qs.set('start', filters.start); + if (filters.end) qs.set('end', filters.end); + qs.set( + 'group_by', + REPORT_GROUPINGS.includes(filters.group_by ?? '') ? String(filters.group_by) : 'agent' + ); + if (filters.billable === 'true' || filters.billable === 'false') { + qs.set('billable', filters.billable); + } + if (filters.account) qs.set('account', filters.account); + if (filters.profile) qs.set('profile', filters.profile); + + const report = await apiRequest(`/time-entries/report/?${qs.toString()}`, {}, { cookies }); + return { report }; +} + +/** + * Every time entry on one ticket, newest first. + * + * Returns the array as given. An agent sees only their own rows here and an + * admin sees the team's, which is the API's decision, not this layer's, so + * the panel must not present the total it can add up from this list as the + * ticket's total. `time_summary` on the ticket is that number. + * + * @param {{ cookies: import('@sveltejs/kit').Cookies }} event + * @param {string} ticketId + * @returns {Promise} + */ +export async function listTicketTime({ cookies }, ticketId) { + if (!ticketId) throw new Error('Which ticket? No ticket id was given.'); + + const entries = await apiRequest(`/cases/${ticketId}/time-entries/`, {}, { cookies }); + return Array.isArray(entries) ? entries : []; +} + +/** + * Start a running timer on a ticket. + * + * The API answers 409 when this person already has one running, anywhere, and + * names the ticket it is on. That id is carried through on the thrown error so + * the page can offer a way to it: "you have a timer running" with no way to + * reach it is a dead end on the one screen that could fix it. + * + * @param {{ cookies: import('@sveltejs/kit').Cookies }} event + * @param {string} ticketId + * @returns {Promise} the started entry + */ +export async function startTicketTimer({ cookies }, ticketId) { + if (!ticketId) throw new Error('Which ticket? No ticket id was given.'); + + return await apiRequest( + `/cases/${ticketId}/time-entries/start/`, + { method: 'POST' }, + { cookies } + ); +} + +/** + * Log a stopped entry after the fact. + * + * The API wants two timestamps; the form asks for one duration, so the window + * is built here as "the last `minutes` minutes", ending now. That is what the + * phone does too (`mobile/lib/widgets/tickets/ticket_time_panel.dart`), and it + * keeps the clock out of the browser's hands: a UTC instant computed on the + * server cannot be a day out because somebody's laptop is set wrong. + * + * It also means an entry cannot be backdated to last Tuesday from here. That + * is the same limit the phone has, and adding a date field would need a + * timezone to resolve it against, which a `` does not send. + * + * `description` is required by `TimeEntryCreateSerializer` and checked here + * too, so an empty box is answered without a round trip. `currency` is the + * org's, passed in by the caller: left off, every entry would be USD and an + * org that invoices in euros could not bill a single one of them, since the + * invoice builder refuses a mixed-currency set. + * + * @param {{ cookies: import('@sveltejs/kit').Cookies }} event + * @param {string} ticketId + * @param {{ minutes: number|string, description: string, billable?: boolean, hourlyRate?: string|number|null, currency?: string|null }} entry + * @returns {Promise} the created entry + */ +export async function logTicketTime({ cookies }, ticketId, entry) { + if (!ticketId) throw new Error('Which ticket? No ticket id was given.'); + + const minutes = Number(entry.minutes); + if (!Number.isInteger(minutes) || minutes < 1 || minutes > MAX_ENTRY_MINUTES) { + throw new Error(`Minutes must be a whole number between 1 and ${MAX_ENTRY_MINUTES}.`); + } + const description = (entry.description ?? '').trim(); + if (!description) throw new Error('Say what the time was spent on.'); + + const ended = new Date(); + const started = new Date(ended.getTime() - minutes * 60000); + + /** @type {Record} */ + const body = { + started_at: started.toISOString(), + ended_at: ended.toISOString(), + description, + billable: Boolean(entry.billable) + }; + + // Optional, and only when it is a number the API will take. A rate is the + // difference between an invoice line worth something and one worth 0.00. + if (entry.hourlyRate !== null && entry.hourlyRate !== undefined && entry.hourlyRate !== '') { + const rate = Number(entry.hourlyRate); + if (!Number.isFinite(rate) || rate < 0) throw new Error('The rate must be a positive number.'); + body.hourly_rate = rate.toFixed(2); + } + if (entry.currency) body.currency = entry.currency; + + return await apiRequest( + `/cases/${ticketId}/time-entries/`, + { method: 'POST', body }, + { cookies } + ); +} + +/** + * Flip one entry between billable and not. + * + * A PUT carrying the single field. The endpoint updates partially, so the + * description, the window, and the rate are left where they are; sending the + * whole entry back would race anyone editing it elsewhere. + * + * @param {{ cookies: import('@sveltejs/kit').Cookies }} event + * @param {string} entryId + * @param {boolean} billable + * @returns {Promise} the updated entry + */ +export async function setEntryBillable({ cookies }, entryId, billable) { + if (!entryId) throw new Error('Which time entry? No entry id was given.'); + + return await apiRequest( + `/time-entries/${entryId}/`, + { method: 'PUT', body: { billable: Boolean(billable) } }, + { cookies } + ); +} + +/** + * Delete an entry. + * + * The API refuses one that has been invoiced, which is the check that matters + * and is not repeated here: an entry can be invoiced between this page + * rendering and the button being pressed, so the answer has to come from the + * server either way. + * + * @param {{ cookies: import('@sveltejs/kit').Cookies }} event + * @param {string} entryId + * @returns {Promise} + */ +export async function deleteEntry({ cookies }, entryId) { + if (!entryId) throw new Error('Which time entry? No entry id was given.'); + + return await apiRequest(`/time-entries/${entryId}/`, { method: 'DELETE' }, { cookies }); +} diff --git a/frontend/src/lib/server/v2/timesheet.test.js b/frontend/src/lib/server/v2/timesheet.test.js index 8dc6934e..1b861135 100644 --- a/frontend/src/lib/server/v2/timesheet.test.js +++ b/frontend/src/lib/server/v2/timesheet.test.js @@ -3,7 +3,15 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const apiRequest = vi.fn(); vi.mock('$lib/api-helpers.js', () => ({ apiRequest: (...a) => apiRequest(...a) })); -const { stopTimer } = await import('$lib/server/v2/timesheet.js'); +const { + stopTimer, + listTicketTime, + startTicketTimer, + logTicketTime, + setEntryBillable, + deleteEntry, + getTimeReport +} = await import('$lib/server/v2/timesheet.js'); // Cast rather than shaping a full Cookies mock, matching leads.test.js and // tags.test.js: stopTimer only ever calls `cookies.get`, and `apiRequest` // itself is mocked above, so nothing here touches @@ -32,3 +40,150 @@ describe('stopTimer', () => { expect(apiRequest).not.toHaveBeenCalled(); }); }); + +describe('the ticket panel', () => { + beforeEach(() => { + apiRequest.mockReset(); + }); + + it('lists a ticket’s entries and hands back an array even when the API does not', async () => { + apiRequest.mockResolvedValue([{ id: 'e1' }]); + expect(await listTicketTime(event, 't1')).toEqual([{ id: 'e1' }]); + expect(apiRequest.mock.calls[0][0]).toBe('/cases/t1/time-entries/'); + + // A 204, an error envelope, or a paginated object would each render as + // `entries.length` of undefined in the panel. + apiRequest.mockResolvedValue(null); + expect(await listTicketTime(event, 't1')).toEqual([]); + }); + + it('starts a timer on the ticket, with no body to be trusted', async () => { + apiRequest.mockResolvedValue({ id: 'e1', ended_at: null }); + await startTicketTimer(event, 't1'); + + const [endpoint, options] = apiRequest.mock.calls[0]; + expect(endpoint).toBe('/cases/t1/time-entries/start/'); + expect(options.method).toBe('POST'); + expect(options.body).toBeUndefined(); + }); + + it('turns minutes into a window ending now', async () => { + apiRequest.mockResolvedValue({ id: 'e1' }); + await logTicketTime(event, 't1', { minutes: '45', description: ' Traced it ' }); + + const [endpoint, options] = apiRequest.mock.calls[0]; + expect(endpoint).toBe('/cases/t1/time-entries/'); + const spanMinutes = + (Date.parse(options.body.ended_at) - Date.parse(options.body.started_at)) / 60000; + expect(spanMinutes).toBe(45); + expect(options.body.description).toBe('Traced it'); + expect(options.body.billable).toBe(false); + // Absent, not null: the API takes the model default for both. + expect('hourly_rate' in options.body).toBe(false); + expect('currency' in options.body).toBe(false); + }); + + it('sends the rate and currency when it is given them', async () => { + apiRequest.mockResolvedValue({ id: 'e1' }); + await logTicketTime(event, 't1', { + minutes: 30, + description: 'Call', + billable: true, + hourlyRate: '85', + currency: 'EUR' + }); + + const { body } = apiRequest.mock.calls[0][1]; + expect(body.hourly_rate).toBe('85.00'); + expect(body.currency).toBe('EUR'); + expect(body.billable).toBe(true); + }); + + it('rejects a duration that is not a whole number of minutes inside a day', async () => { + for (const minutes of ['0', '-30', '1.5', '1441', 'half an hour', '']) { + await expect(logTicketTime(event, 't1', { minutes, description: 'x' })).rejects.toThrow( + /minutes/i + ); + } + expect(apiRequest).not.toHaveBeenCalled(); + }); + + it('rejects an empty description, which the API requires, and a negative rate', async () => { + await expect(logTicketTime(event, 't1', { minutes: 30, description: ' ' })).rejects.toThrow( + /spent on/i + ); + await expect( + logTicketTime(event, 't1', { minutes: 30, description: 'x', hourlyRate: '-5' }) + ).rejects.toThrow(/positive/i); + expect(apiRequest).not.toHaveBeenCalled(); + }); + + it('sends only the billable flag when flipping one, leaving the rest of the entry alone', async () => { + apiRequest.mockResolvedValue({ id: 'e1', billable: true }); + await setEntryBillable(event, 'e1', true); + + const [endpoint, options] = apiRequest.mock.calls[0]; + expect(endpoint).toBe('/time-entries/e1/'); + expect(options.method).toBe('PUT'); + expect(options.body).toEqual({ billable: true }); + }); + + it('deletes by id, and refuses to send a request without one', async () => { + apiRequest.mockResolvedValue(null); + await deleteEntry(event, 'e1'); + expect(apiRequest.mock.calls[0][0]).toBe('/time-entries/e1/'); + expect(apiRequest.mock.calls[0][1].method).toBe('DELETE'); + + apiRequest.mockReset(); + await expect(deleteEntry(event, '')).rejects.toThrow(/entry/i); + await expect(listTicketTime(event, '')).rejects.toThrow(/ticket/i); + await expect(startTicketTimer(event, '')).rejects.toThrow(/ticket/i); + expect(apiRequest).not.toHaveBeenCalled(); + }); +}); + +describe('the time report', () => { + beforeEach(() => { + apiRequest.mockReset(); + apiRequest.mockResolvedValue({ rows: [], totals: {}, currencies: [] }); + }); + + /** The query string the helper sent, as a plain object. */ + function sentParams() { + const [endpoint] = apiRequest.mock.calls[0]; + return Object.fromEntries(new URLSearchParams(endpoint.split('?')[1])); + } + + it('passes the window and the grouping through', async () => { + await getTimeReport(event, { start: '2026-08-01', end: '2026-08-31', group_by: 'account' }); + + expect(sentParams()).toEqual({ + start: '2026-08-01', + end: '2026-08-31', + group_by: 'account' + }); + }); + + it('falls back to grouping by agent rather than forwarding a bad value', async () => { + // Straight off the query string, so this is a hand-edited URL, not a bug. + // Forwarded, the API answers 400 and the page becomes an error screen. + await getTimeReport(event, { group_by: 'planet' }); + + expect(sentParams().group_by).toBe('agent'); + }); + + it('sends the billable filter only when it is one of the two the API takes', async () => { + await getTimeReport(event, { billable: 'false' }); + expect(sentParams().billable).toBe('false'); + + apiRequest.mockClear(); + await getTimeReport(event, { billable: 'maybe' }); + expect('billable' in sentParams()).toBe(false); + }); + + it('leaves the window off entirely when it is not given, so the API picks it', async () => { + await getTimeReport(event, {}); + + expect(sentParams()).toEqual({ group_by: 'agent' }); + }); +}); diff --git a/frontend/src/lib/v2/format.js b/frontend/src/lib/v2/format.js index 27e92ba7..f2d05bdd 100644 --- a/frontend/src/lib/v2/format.js +++ b/frontend/src/lib/v2/format.js @@ -114,6 +114,23 @@ export function relativeTime(iso, now = new Date()) { return relativeDays(iso, now); } +/** + * A duration in minutes as "1h 48m". + * + * Logged time is read in hours: "108m" makes the reader do the division, and + * two of them on the same screen makes them do it twice. Minutes below the + * hour keep their own suffix so a short entry does not render as "0h 12m". + * + * @param {number|string|null|undefined} mins + */ +export function hoursMinutes(mins) { + const v = Number(mins ?? 0); + if (!Number.isFinite(v)) return '0m'; + const m = Math.max(0, Math.round(v)); + const h = Math.floor(m / 60); + return h ? `${h}h ${m % 60}m` : `${m}m`; +} + /** Compact age for a table cell: "12d", "3h". */ export function shortAge(iso, now = new Date()) { const d = parseIso(iso); diff --git a/frontend/src/lib/v2/format.test.js b/frontend/src/lib/v2/format.test.js index 11c47ed1..338f72e3 100644 --- a/frontend/src/lib/v2/format.test.js +++ b/frontend/src/lib/v2/format.test.js @@ -1,5 +1,12 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; -import { shortDate, longDate, daysSince, relativeDays, relativeTime } from '$lib/v2/format.js'; +import { + shortDate, + longDate, + daysSince, + relativeDays, + relativeTime, + hoursMinutes +} from '$lib/v2/format.js'; /** * A date and an instant are not the same kind of value, and the API sends both. @@ -71,6 +78,26 @@ describe('a timestamp is an instant and keeps its zone', () => { }); }); +describe('a duration reads in hours', () => { + it('splits minutes into hours and minutes', () => { + expect(hoursMinutes(108)).toBe('1h 48m'); + expect(hoursMinutes(120)).toBe('2h 0m'); + }); + + it('keeps a short entry in minutes rather than printing 0h', () => { + expect(hoursMinutes(12)).toBe('12m'); + expect(hoursMinutes(0)).toBe('0m'); + }); + + it('never renders a negative or a NaN duration', () => { + // A running timer's elapsed minutes are a subtraction, and a clock that + // disagrees with the server can make it come out below zero. + expect(hoursMinutes(-5)).toBe('0m'); + expect(hoursMinutes(null)).toBe('0m'); + expect(hoursMinutes('not a number')).toBe('0m'); + }); +}); + describe('nothing to render', () => { it('null and an unparseable string both give the placeholder', () => { expect(shortDate(null)).toBe('—'); diff --git a/frontend/src/routes/(app)/tickets/[id]/+page.server.js b/frontend/src/routes/(app)/tickets/[id]/+page.server.js index 7598b6dd..bbfdcf6f 100644 --- a/frontend/src/routes/(app)/tickets/[id]/+page.server.js +++ b/frontend/src/routes/(app)/tickets/[id]/+page.server.js @@ -7,6 +7,14 @@ import { updateTicket } from '$lib/server/v2/tickets.js'; import { getOrgSettings } from '$lib/server/v2/organization.js'; +import { + listTicketTime, + startTicketTimer, + stopTimer, + logTicketTime, + setEntryBillable, + deleteEntry +} from '$lib/server/v2/timesheet.js'; import { readableError } from '$lib/server/v2/form-errors.js'; import { openDescendants, subtreeTruncated, cascadedCount, closeResultMessage } from './close.js'; @@ -22,14 +30,37 @@ import { openDescendants, subtreeTruncated, cascadedCount, closeResultMessage } * falls back to the plain one. Both fall back quietly, and the close action * re-derives everything server-side anyway, so nothing here is trusted. * + * The time entries ride along in the same wave. They are their own request, + * the ticket envelope carries only the `time_summary` totals, and they are + * allowed to fail: a ticket that will not render because the time panel could + * not load is the same bad trade as the tree below. `null` means the fetch + * failed and the panel says so; `[]` means nobody has logged anything. + * * @type {import('./$types').PageServerLoad} */ -export async function load({ cookies, params }) { - const data = await getTicket({ cookies }, params.id); +export async function load({ cookies, params, locals }) { + const [data, timeEntries] = await Promise.all([ + getTicket({ cookies }, params.id), + listTicketTime({ cookies }, params.id).catch(() => null) + ]); + + const time = { + entries: timeEntries, + // Server-derived from the JWT, never the client. Display-only: it tells + // the panel which running timer is this person's to stop, since an admin + // sees the whole team's rows. The API decides who may actually stop one. + viewerUserId: locals.user?.id ?? null, + // What a running timer's elapsed minutes are counted from. The browser + // clock only adds the minutes since this page loaded, the same split the + // timesheet page uses: how long somebody has been working is not a + // question a machine with the wrong date gets to answer. + now: new Date().toISOString() + }; + // `child_count` sits on the ticket itself here. The `server` block with a // `child_count` of its own belongs to the EDIT page's loader, and reading it // from this one is silently always-undefined, so the panel never appeared. - if (!data.ticket?.child_count) return data; + if (!data.ticket?.child_count) return { ...data, time }; const [tree, settings] = await Promise.all([ getTicketTree({ cookies }, params.id).catch(() => null), @@ -38,6 +69,7 @@ export async function load({ cookies, params }) { return { ...data, + time, close: { descendants: openDescendants(tree?.root, params.id), truncated: subtreeTruncated(tree?.root, params.id), @@ -167,5 +199,111 @@ export const actions = { moved: 'Closed', closed: closeResultMessage({ cascade, cascaded: cascadedCount(result) }) }; + }, + + /* + * The five time-panel writes below all report through `timeError` rather + * than the `error` the reply and close actions use. That banner sits at the + * top of the page and the panel does not, so on a phone a shared key puts + * the reason a delete was refused a full screen above the button that was + * pressed. + */ + + /** + * Start the clock on this ticket. + * + * One timer per person, org-wide: the API answers 409 when there is already + * one running and names the ticket it is on. That id is passed back so the + * panel can link to it, because the fix for "you already have a timer + * running" is on that other ticket, not this one. + */ + startTimer: async ({ cookies, params }) => { + try { + await startTicketTimer({ cookies }, params.id); + } catch (/** @type {any} */ err) { + return fail(err?.status === 409 ? 409 : 400, { + timeError: readableError(err, 'Could not start the timer.'), + runningTicketId: err?.body?.running_case_id ?? null + }); + } + + return { timeStarted: true }; + }, + + /** Stop a running timer. The API rejects one that is already stopped, which + * is what a double-submit looks like, so the panel disables the button + * while this is in flight. */ + stopTimer: async ({ cookies, request }) => { + const form = await request.formData(); + const entryId = form.get('entry_id')?.toString() ?? ''; + + try { + await stopTimer({ cookies }, entryId); + } catch (/** @type {any} */ err) { + return fail(400, { timeError: readableError(err, 'Could not stop the timer.') }); + } + + return { timeStopped: true }; + }, + + /** + * Log time that was worked without the timer running. + * + * The currency is the org's, from the JWT, not from the form: what an entry + * is billed in is a fact about the org, and a client that could name it + * could bill an hour in a currency nobody trades. + */ + logTime: async ({ cookies, params, request, locals }) => { + const form = await request.formData(); + const minutes = form.get('minutes')?.toString() ?? ''; + const description = form.get('description')?.toString() ?? ''; + const billable = form.get('billable') === 'on'; + const hourlyRate = form.get('hourly_rate')?.toString() ?? ''; + + try { + await logTicketTime({ cookies }, params.id, { + minutes, + description, + billable, + hourlyRate, + currency: /** @type {any} */ (locals).org_settings?.default_currency ?? null + }); + } catch (/** @type {any} */ err) { + return fail(400, { timeError: readableError(err, 'Could not log this time.') }); + } + + return { timeLogged: true }; + }, + + /** Flip one entry between billable and not. The next value comes from the + * form rather than being derived from what was rendered, so two clicks in + * quick succession cannot land on the same value twice. */ + setBillable: async ({ cookies, request }) => { + const form = await request.formData(); + const entryId = form.get('entry_id')?.toString() ?? ''; + const billable = form.get('billable') === 'true'; + + try { + await setEntryBillable({ cookies }, entryId, billable); + } catch (/** @type {any} */ err) { + return fail(400, { timeError: readableError(err, 'Could not change this entry.') }); + } + + return { timeUpdated: true }; + }, + + /** Delete an entry. Refused by the API once the entry has been invoiced, + * and that message is worth showing as it is: it says what to undo first. */ + deleteTime: async ({ cookies, request }) => { + const form = await request.formData(); + const entryId = form.get('entry_id')?.toString() ?? ''; + + try { + await deleteEntry({ cookies }, entryId); + } catch (/** @type {any} */ err) { + return fail(400, { timeError: readableError(err, 'Could not delete this entry.') }); + } + + return { timeDeleted: true }; } }; diff --git a/frontend/src/routes/(app)/tickets/[id]/+page.svelte b/frontend/src/routes/(app)/tickets/[id]/+page.svelte index a08e2b34..7f742040 100644 --- a/frontend/src/routes/(app)/tickets/[id]/+page.svelte +++ b/frontend/src/routes/(app)/tickets/[id]/+page.svelte @@ -1,14 +1,33 @@ + + + {#snippet crumb()} + Timesheet + {/snippet} + {#snippet sub()} + {shortDate(report.start)} - {shortDate(report.end)} · by {GROUP_NOUN[ + report.group_by + ].toLowerCase()} + {/snippet} + {#snippet actions()} + + + Export CSV + + {/snippet} + + +
+
+
+ + +
+
+ + +
+
+ + +
+
+ + +
+ +
+ +
+ + + 1 + ? `Mixed currencies: ${report.currencies.join(', ')}` + : 'At the rate saved on each entry'} + /> + +
+
+ +
+
+ {#if report.rows.length === 0} +

+ No time logged in this window. Widen the dates, or clear the billable filter. +

+ {:else} +
+ + + + + + + + + + + + {#each report.rows as row (row.key ?? row.name)} + + + + + + + + {/each} + +
{GROUP_NOUN[report.group_by]}EntriesBillableValueLogged
+ {#if report.group_by === 'ticket' && row.key} + {row.name} + {:else if report.group_by === 'account' && row.key} + {row.name} + {:else} + {row.name} + {/if} + {count(row.entry_count)} + {hm(row.billable_minutes)} + + {money(row.billable_value, currency)} + {hm(row.total_minutes)}
+
+ {/if} +
+
+ + diff --git a/frontend/src/routes/api/time-entries/report/export/+server.js b/frontend/src/routes/api/time-entries/report/export/+server.js new file mode 100644 index 00000000..a7fba9d2 --- /dev/null +++ b/frontend/src/routes/api/time-entries/report/export/+server.js @@ -0,0 +1,49 @@ +/** + * CSV export proxy for the time report. + * + * Streams the upstream `text/csv` straight through rather than buffering it, + * so a year of entries starts downloading immediately, and the same for the + * reason `cases/analytics/export` does it: this is the pattern, not a second + * opinion about it. + * + * It exists because the access token lives in an httpOnly cookie. A plain + * `` from the page cannot attach it, so the link points here and + * this handler adds the Authorization header on the server. The query string + * is forwarded as given; the API validates every parameter and decides what + * this caller may see, which is where that decision belongs. + */ + +import { env } from '$env/dynamic/public'; + +const API_BASE_URL = `${env.PUBLIC_DJANGO_API_URL}/api`; + +/** @type {import('./$types').RequestHandler} */ +export async function GET({ cookies, request, url }) { + const accessToken = cookies.get('jwt_access'); + if (!accessToken) return new Response('Unauthorized', { status: 401 }); + + const qs = url.searchParams.toString(); + const upstream = await fetch(`${API_BASE_URL}/time-entries/report/export/${qs ? `?${qs}` : ''}`, { + method: 'GET', + headers: { + Authorization: `Bearer ${accessToken}`, + Accept: 'text/csv' + }, + signal: request.signal + }); + + if (!upstream.ok || !upstream.body) { + return new Response(`Upstream error: ${upstream.status}`, { + status: upstream.status || 502 + }); + } + + return new Response(upstream.body, { + status: 200, + headers: { + 'Content-Type': upstream.headers.get('Content-Type') || 'text/csv', + 'Content-Disposition': + upstream.headers.get('Content-Disposition') || 'attachment; filename="time.csv"' + } + }); +} diff --git a/mobile/lib/config/api_config.dart b/mobile/lib/config/api_config.dart index 9f5a1d38..34e95dfa 100644 --- a/mobile/lib/config/api_config.dart +++ b/mobile/lib/config/api_config.dart @@ -338,6 +338,30 @@ class ApiConfig { static String timesheet({required String start, required String end}) => '$apiBaseUrl/time-entries/timesheet/?start=$start&end=$end'; + /// Where the time went over a window: totals by agent, ticket or account. + /// + /// `groupBy` is one of agent/ticket/account and `billable`, when given, is + /// the string 'true' or 'false'; the API rejects anything else rather than + /// guessing, so the caller passes what the picker chose and nothing else. + /// + /// The CSV export beside this endpoint is deliberately not wired up here. + /// Ticket analytics made the same call: a download belongs on the web, and + /// a phone has nowhere useful to put a spreadsheet. + static String timeReport({ + required String start, + required String end, + required String groupBy, + String? billable, + }) { + final query = StringBuffer( + 'start=$start&end=$end&group_by=${Uri.encodeQueryComponent(groupBy)}', + ); + if (billable != null) { + query.write('&billable=${Uri.encodeQueryComponent(billable)}'); + } + return '$apiBaseUrl/time-entries/report/?$query'; + } + /// Solutions (Knowledge Base). List/create. static String get solutions => '$apiBaseUrl/cases/solutions/'; diff --git a/mobile/lib/data/models/time_report.dart b/mobile/lib/data/models/time_report.dart new file mode 100644 index 00000000..07e90a9b --- /dev/null +++ b/mobile/lib/data/models/time_report.dart @@ -0,0 +1,125 @@ +/// Where the time went, as `/api/time-entries/report/` returns it. +/// +/// The timesheet answers "what did I do this week". This answers the question +/// asked of that data afterwards, usually by somebody else: how much of the +/// month went to this account, and how much of it can be billed. +/// +/// Money arrives as a string, not a double, because the API builds it from +/// Decimal columns and a JSON number would round it on the way. It is parsed +/// once here rather than at every call site. +library; + +/// One grouped row: an agent, a ticket or an account. +class TimeReportRow { + const TimeReportRow({ + required this.name, + this.key, + this.totalMinutes = 0, + this.billableMinutes = 0, + this.billableValue = 0, + this.entryCount = 0, + }); + + /// The id of the thing this row groups, or null where there is none: time + /// on tickets with no account lands in a named bucket rather than being + /// dropped, and unattributed time is exactly what a report is run to find. + final String? key; + final String name; + final int totalMinutes; + final int billableMinutes; + final double billableValue; + final int entryCount; + + bool get isBillableOnly => billableMinutes == totalMinutes; + + factory TimeReportRow.fromJson(Map json) { + return TimeReportRow( + key: json['key'] as String?, + name: json['name'] as String? ?? '', + totalMinutes: json['total_minutes'] as int? ?? 0, + billableMinutes: json['billable_minutes'] as int? ?? 0, + billableValue: _money(json['billable_value']), + entryCount: json['entry_count'] as int? ?? 0, + ); + } +} + +/// The whole report: the window the server actually reported on, the rows, +/// and the totals under them. +class TimeReport { + const TimeReport({ + required this.start, + required this.end, + this.groupBy = 'agent', + this.rows = const [], + this.totalMinutes = 0, + this.billableMinutes = 0, + this.billableValue = 0, + this.entryCount = 0, + this.currencies = const [], + }); + + /// Read back from the response rather than kept from the request: asked for + /// without a window, the API picks the last 30 days, and the header has to + /// name the days actually reported on. + final DateTime start; + final DateTime end; + final String groupBy; + final List rows; + + final int totalMinutes; + final int billableMinutes; + final double billableValue; + final int entryCount; + + /// Every currency present in the window. The value totals add across all of + /// them, so more than one here means no single symbol can label the figure + /// honestly and the screen says so instead of picking one. + final List currencies; + + bool get isEmpty => rows.isEmpty; + + bool get isMixedCurrency => currencies.length > 1; + + String? get currency => currencies.length == 1 ? currencies.first : null; + + int get billableShare => + totalMinutes == 0 ? 0 : ((billableMinutes / totalMinutes) * 100).round(); + + factory TimeReport.fromJson(Map json) { + final totals = (json['totals'] as Map?) ?? const {}; + return TimeReport( + start: _date(json['start']), + end: _date(json['end']), + groupBy: json['group_by'] as String? ?? 'agent', + rows: ((json['rows'] as List?) ?? const []) + .whereType>() + .map(TimeReportRow.fromJson) + .toList(), + totalMinutes: totals['total_minutes'] as int? ?? 0, + billableMinutes: totals['billable_minutes'] as int? ?? 0, + billableValue: _money(totals['billable_value']), + entryCount: totals['entry_count'] as int? ?? 0, + currencies: ((json['currencies'] as List?) ?? const []) + .map((c) => c.toString()) + .toList(), + ); + } +} + +/// A decimal string from the API as a double. Absent or unparseable is 0, +/// never null: a missing total is nothing logged, and every caller would +/// otherwise write the same `?? 0`. +double _money(dynamic value) => + value == null ? 0 : (double.tryParse(value.toString()) ?? 0); + +/// A YYYY-MM-DD string as a local calendar date. +/// +/// Parsed with an explicit midnight rather than `DateTime.parse` on the bare +/// date, so it is local midnight and not UTC midnight: reading a UTC midnight +/// in a zone behind Greenwich lands on the previous day and mislabels the +/// window by one. +DateTime _date(dynamic value) { + final text = value?.toString() ?? ''; + return DateTime.tryParse('${text}T00:00:00') ?? DateTime.now(); +} diff --git a/mobile/lib/providers/time_report_provider.dart b/mobile/lib/providers/time_report_provider.dart new file mode 100644 index 00000000..a394bf76 --- /dev/null +++ b/mobile/lib/providers/time_report_provider.dart @@ -0,0 +1,127 @@ +import 'package:flutter_riverpod/flutter_riverpod.dart'; + +import '../config/api_config.dart'; +import '../data/models/time_report.dart'; +import '../services/api_service.dart'; + +/// What the report is being asked for: a window, a grouping, a billable +/// filter. +/// +/// Held apart from the loaded data for the same reason the timesheet's range +/// is: the filters the user picked have to survive a failed load, and folding +/// them into the response would snap the pickers back on every network error. +class TimeReportFilters { + const TimeReportFilters({ + required this.start, + required this.end, + this.groupBy = 'agent', + this.billable, + }); + + final DateTime start; + final DateTime end; + + /// One of agent/ticket/account. The API rejects anything else, so the + /// picker's values and this string are the same short list. + final String groupBy; + + /// 'true', 'false', or null for everything. Null is not the same as 'false': + /// one asks for all time, the other for non-billable time only. + final String? billable; + + /// The last 30 days, matching what the API picks when asked without a + /// window, so the screen opens on the same report either way. + factory TimeReportFilters.recent() { + final now = DateTime.now(); + final end = DateTime(now.year, now.month, now.day); + return TimeReportFilters( + start: end.subtract(const Duration(days: 29)), + end: end, + ); + } + + TimeReportFilters copyWith({ + DateTime? start, + DateTime? end, + String? groupBy, + String? billable, + bool clearBillable = false, + }) { + return TimeReportFilters( + start: start ?? this.start, + end: end ?? this.end, + groupBy: groupBy ?? this.groupBy, + billable: clearBillable ? null : (billable ?? this.billable), + ); + } + + String get startParam => _param(start); + String get endParam => _param(end); + + static String _param(DateTime d) => + '${d.year.toString().padLeft(4, '0')}-' + '${d.month.toString().padLeft(2, '0')}-' + '${d.day.toString().padLeft(2, '0')}'; +} + +/// The filters the report screen is on. Changing these refetches, because +/// [TimeReportNotifier.build] watches them. +class TimeReportFiltersNotifier extends Notifier { + @override + TimeReportFilters build() => TimeReportFilters.recent(); + + void setGroupBy(String groupBy) => state = state.copyWith(groupBy: groupBy); + + void setBillable(String? billable) => state = billable == null + ? state.copyWith(clearBillable: true) + : state.copyWith(billable: billable); + + void setWindow(DateTime start, DateTime end) => + state = state.copyWith(start: start, end: end); +} + +final timeReportFiltersProvider = + NotifierProvider( + TimeReportFiltersNotifier.new, + ); + +/// Totals over a window, grouped. +/// +/// Read-only. Who may see what is the server's call: an agent's report covers +/// their own time and an admin's covers the org, which is why nothing here +/// sends a `profile` and nothing here filters by one. +class TimeReportNotifier extends AsyncNotifier { + final ApiService _apiService = ApiService(); + + @override + Future build() { + final filters = ref.watch(timeReportFiltersProvider); + return _fetch(filters); + } + + Future _fetch(TimeReportFilters filters) async { + final response = await _apiService.get( + ApiConfig.timeReport( + start: filters.startParam, + end: filters.endParam, + groupBy: filters.groupBy, + billable: filters.billable, + ), + ); + if (!response.success || response.data == null) { + throw Exception(response.message ?? 'Could not load the time report.'); + } + return TimeReport.fromJson(response.data!); + } + + Future refresh() async { + final filters = ref.read(timeReportFiltersProvider); + state = const AsyncValue.loading(); + state = await AsyncValue.guard(() => _fetch(filters)); + } +} + +final timeReportProvider = + AsyncNotifierProvider( + TimeReportNotifier.new, + ); diff --git a/mobile/lib/routes/app_router.dart b/mobile/lib/routes/app_router.dart index e8b35dc3..7e1290f4 100644 --- a/mobile/lib/routes/app_router.dart +++ b/mobile/lib/routes/app_router.dart @@ -57,6 +57,7 @@ import '../screens/solutions/solutions_list_screen.dart'; import '../screens/solutions/solution_detail_screen.dart'; import '../screens/tickets/approvals_inbox_screen.dart'; import '../screens/tickets/ticket_analytics_screen.dart'; +import '../screens/timesheet/time_report_screen.dart'; import '../screens/timesheet/timesheet_screen.dart'; import '../screens/notifications/notifications_screen.dart'; import '../screens/documents/documents_list_screen.dart'; @@ -151,6 +152,10 @@ class AppRoutes { /// path that says where it lives is the one that survives being linked to. static const String timesheet = '/timesheet'; + /// Totals over a window, grouped. Nested under the timesheet on both + /// clients, because it is the same entries asked a different question. + static const String timeReport = '/timesheet/report'; + /// The notification feed. Matches the web app's `/notifications`. static const String notifications = '/notifications'; @@ -403,6 +408,12 @@ final appRouterProvider = Provider((ref) { parentNavigatorKey: _rootNavigatorKey, builder: (context, state) => const TimesheetScreen(), ), + GoRoute( + path: AppRoutes.timeReport, + name: 'timeReport', + parentNavigatorKey: _rootNavigatorKey, + builder: (context, state) => const TimeReportScreen(), + ), GoRoute( path: AppRoutes.notifications, name: 'notifications', diff --git a/mobile/lib/screens/timesheet/time_report_screen.dart b/mobile/lib/screens/timesheet/time_report_screen.dart new file mode 100644 index 00000000..ae6482f6 --- /dev/null +++ b/mobile/lib/screens/timesheet/time_report_screen.dart @@ -0,0 +1,385 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:go_router/go_router.dart'; +import 'package:intl/intl.dart'; +import 'package:lucide_icons_flutter/lucide_icons.dart'; + +import '../../core/theme/theme.dart'; +import '../../data/models/time_report.dart'; +import '../../providers/time_report_provider.dart'; + +/// Where the time went: totals by agent, ticket or account over a window. +/// +/// The timesheet screen is one person's week. This is every window and every +/// grouping of the same entries, and it is what someone opens when the +/// question is about an account's month rather than their own Tuesday. +/// +/// Read-only, and no CSV export: the same call the ticket analytics screen +/// made. A phone has nowhere useful to put a spreadsheet, and the web page +/// carries the download. +class TimeReportScreen extends ConsumerWidget { + const TimeReportScreen({super.key}); + + static const _groups = [ + ('agent', 'Agent'), + ('ticket', 'Ticket'), + ('account', 'Account'), + ]; + + static const _billableFilters = [ + (null, 'All time'), + ('true', 'Billable'), + ('false', 'Non-billable'), + ]; + + @override + Widget build(BuildContext context, WidgetRef ref) { + final filters = ref.watch(timeReportFiltersProvider); + final reportAsync = ref.watch(timeReportProvider); + + return Scaffold( + backgroundColor: AppColors.surfaceDim, + appBar: AppBar( + title: const Text('Time report'), + backgroundColor: AppColors.surface, + elevation: 0, + scrolledUnderElevation: 1, + leading: IconButton( + icon: const Icon(LucideIcons.chevronLeft), + tooltip: 'Back', + onPressed: () => context.pop(), + ), + ), + body: Column( + children: [ + _FilterBar(filters: filters), + Expanded( + child: reportAsync.when( + loading: () => const Center(child: CircularProgressIndicator()), + error: (error, _) => _ErrorState( + message: '$error', + onRetry: () => ref.read(timeReportProvider.notifier).refresh(), + ), + data: (report) => RefreshIndicator( + onRefresh: () => + ref.read(timeReportProvider.notifier).refresh(), + child: _Body(report: report), + ), + ), + ), + ], + ), + ); + } +} + +/// The window, the grouping and the billable filter. +/// +/// Its own bar under the app bar rather than app-bar actions: three controls +/// and a date range do not fit beside a title at 390px, and the range is the +/// label that says which report is on screen. +class _FilterBar extends ConsumerWidget { + const _FilterBar({required this.filters}); + + final TimeReportFilters filters; + + Future _pickWindow(BuildContext context, WidgetRef ref) async { + final picked = await showDateRangePicker( + context: context, + firstDate: DateTime(2000), + lastDate: DateTime.now().add(const Duration(days: 1)), + initialDateRange: DateTimeRange(start: filters.start, end: filters.end), + ); + if (picked == null) return; + ref + .read(timeReportFiltersProvider.notifier) + .setWindow(picked.start, picked.end); + } + + @override + Widget build(BuildContext context, WidgetRef ref) { + final label = + '${DateFormat('d MMM').format(filters.start)} - ' + '${DateFormat('d MMM').format(filters.end)}'; + + return Container( + padding: const EdgeInsets.fromLTRB(12, 8, 12, 10), + decoration: BoxDecoration( + color: AppColors.surface, + border: Border(bottom: BorderSide(color: AppColors.border)), + ), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + // 44px tall, which is the smallest a thumb reliably hits. + SizedBox( + height: 44, + child: OutlinedButton.icon( + onPressed: () => _pickWindow(context, ref), + icon: const Icon(LucideIcons.calendar, size: 16), + label: Text(label), + ), + ), + const SizedBox(height: 8), + SingleChildScrollView( + scrollDirection: Axis.horizontal, + child: Row( + children: [ + for (final (value, label) in TimeReportScreen._groups) + Padding( + padding: const EdgeInsets.only(right: 8), + child: ChoiceChip( + label: Text(label), + selected: filters.groupBy == value, + onSelected: (_) => ref + .read(timeReportFiltersProvider.notifier) + .setGroupBy(value), + ), + ), + Container( + width: 1, + height: 24, + margin: const EdgeInsets.only(right: 8), + color: AppColors.border, + ), + for (final (value, label) in TimeReportScreen._billableFilters) + Padding( + padding: const EdgeInsets.only(right: 8), + child: ChoiceChip( + label: Text(label), + selected: filters.billable == value, + onSelected: (_) => ref + .read(timeReportFiltersProvider.notifier) + .setBillable(value), + ), + ), + ], + ), + ), + ], + ), + ); + } +} + +class _Body extends StatelessWidget { + const _Body({required this.report}); + + final TimeReport report; + + @override + Widget build(BuildContext context) { + return ListView( + // Always scrollable, so pull-to-refresh works on an empty report too. + physics: const AlwaysScrollableScrollPhysics(), + padding: const EdgeInsets.only(bottom: 96), + children: [ + _summary(), + if (report.isEmpty) + Padding( + padding: const EdgeInsets.fromLTRB(16, 24, 16, 0), + child: Text( + 'No time logged in this window. Widen the dates, or clear the ' + 'billable filter.', + style: AppTypography.body.copyWith( + color: AppColors.textSecondary, + ), + ), + ) + else + ...report.rows.map((row) => _row(context, row)), + Padding( + padding: const EdgeInsets.fromLTRB(16, 20, 16, 0), + child: Text( + 'Value is what the billable time is worth at the rate saved on ' + 'each entry, so changing a rate does not rewrite what past months ' + 'were worth. To export these entries as a spreadsheet, open the ' + 'report on the web.', + style: AppTypography.caption.copyWith( + color: AppColors.textSecondary, + height: 1.5, + ), + ), + ), + ], + ); + } + + Widget _summary() { + return Container( + margin: const EdgeInsets.fromLTRB(12, 12, 12, 0), + padding: const EdgeInsets.all(14), + decoration: BoxDecoration( + color: AppColors.surface, + borderRadius: AppLayout.borderRadiusMd, + border: Border.all(color: AppColors.border), + ), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Text( + _hm(report.totalMinutes), + style: AppTypography.h1.copyWith(fontSize: 28, height: 1.1), + ), + const SizedBox(height: 2), + Text( + 'logged, by ${report.groupBy}', + style: AppTypography.caption.copyWith( + color: AppColors.textSecondary, + ), + ), + const SizedBox(height: 12), + Wrap( + spacing: 16, + runSpacing: 6, + children: [ + _stat( + _hm(report.billableMinutes), + 'billable (${report.billableShare}%)', + ), + // One symbol cannot label a sum of two currencies, so a mixed + // window says which ones it holds instead of picking one. + if (report.isMixedCurrency) + _stat(report.currencies.join(', '), 'mixed currencies') + else if (report.billableValue > 0) + _stat( + NumberFormat.simpleCurrency( + name: report.currency, + ).format(report.billableValue), + 'at the saved rates', + ), + _stat('${report.entryCount}', 'entries'), + ], + ), + ], + ), + ); + } + + /// One figure and its label as a single Text, not a Row: a Row inside the + /// Wrap cannot shrink and overflows once the system font is scaled up. + Widget _stat(String value, String label) { + return Text.rich( + TextSpan( + children: [ + TextSpan(text: value, style: AppTypography.labelSmall), + TextSpan( + text: ' $label', + style: AppTypography.caption.copyWith( + color: AppColors.textSecondary, + ), + ), + ], + ), + ); + } + + /// One grouped row. Tappable through to the ticket or the account it names; + /// an agent row and the unattributed bucket have nowhere to go. + Widget _row(BuildContext context, TimeReportRow row) { + final destination = _destination(row); + + return Container( + margin: const EdgeInsets.fromLTRB(12, 12, 12, 0), + decoration: BoxDecoration( + color: AppColors.surface, + borderRadius: AppLayout.borderRadiusMd, + border: Border.all(color: AppColors.border), + ), + child: InkWell( + onTap: destination == null ? null : () => context.push(destination), + borderRadius: AppLayout.borderRadiusMd, + child: Padding( + padding: const EdgeInsets.all(14), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Row( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Expanded( + child: Text( + row.name, + style: AppTypography.labelSmall.copyWith(fontSize: 14), + ), + ), + const SizedBox(width: 10), + Text(_hm(row.totalMinutes), style: AppTypography.labelSmall), + ], + ), + const SizedBox(height: 6), + Wrap( + spacing: 14, + runSpacing: 4, + children: [ + _stat('${row.entryCount}', 'entries'), + if (row.billableMinutes > 0) + _stat(_hm(row.billableMinutes), 'billable'), + if (row.billableValue > 0 && !report.isMixedCurrency) + _stat( + NumberFormat.simpleCurrency( + name: report.currency, + ).format(row.billableValue), + 'worth', + ), + ], + ), + ], + ), + ), + ), + ); + } + + /// Interpolated the way every other screen builds a detail path; the + /// constants in `AppRoutes` carry the `:id` placeholder, not a value. + String? _destination(TimeReportRow row) { + if (row.key == null) return null; + if (report.groupBy == 'ticket') return '/tickets/${row.key}'; + if (report.groupBy == 'account') return '/accounts/${row.key}'; + return null; + } +} + +/// Minutes as "1h 48m". Reports are read in hours, never in minutes. +String _hm(int minutes) { + final m = minutes < 0 ? 0 : minutes; + final h = m ~/ 60; + return h > 0 ? '${h}h ${m % 60}m' : '${m}m'; +} + +class _ErrorState extends StatelessWidget { + const _ErrorState({required this.message, required this.onRetry}); + + final String message; + final VoidCallback onRetry; + + @override + Widget build(BuildContext context) { + return Center( + child: Padding( + padding: const EdgeInsets.all(32), + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + Icon( + LucideIcons.circleAlert, + size: 36, + color: AppColors.textTertiary, + ), + const SizedBox(height: 12), + Text( + message, + textAlign: TextAlign.center, + style: AppTypography.body.copyWith( + color: AppColors.textSecondary, + ), + ), + const SizedBox(height: 16), + OutlinedButton(onPressed: onRetry, child: const Text('Try again')), + ], + ), + ), + ); + } +} diff --git a/mobile/lib/screens/timesheet/timesheet_screen.dart b/mobile/lib/screens/timesheet/timesheet_screen.dart index 2270e300..d2ad06cd 100644 --- a/mobile/lib/screens/timesheet/timesheet_screen.dart +++ b/mobile/lib/screens/timesheet/timesheet_screen.dart @@ -10,6 +10,7 @@ import '../../core/theme/theme.dart'; import '../../data/models/time_entry.dart'; import '../../data/models/timesheet.dart'; import '../../providers/timesheet_provider.dart'; +import '../../routes/app_router.dart'; /// A week of your own logged time. /// @@ -81,6 +82,16 @@ class _TimesheetScreenState extends ConsumerState { backgroundColor: AppColors.surface, elevation: 0, scrolledUnderElevation: 1, + actions: [ + // This screen is one person's week. The report is every window and + // every grouping of the same entries. + IconButton( + constraints: const BoxConstraints(minWidth: 44, minHeight: 44), + icon: const Icon(LucideIcons.chartColumn, size: 20), + tooltip: 'Time report', + onPressed: () => context.push(AppRoutes.timeReport), + ), + ], ), body: Column( children: [ diff --git a/mobile/lib/widgets/tickets/ticket_time_panel.dart b/mobile/lib/widgets/tickets/ticket_time_panel.dart index c59f01fc..16058ee1 100644 --- a/mobile/lib/widgets/tickets/ticket_time_panel.dart +++ b/mobile/lib/widgets/tickets/ticket_time_panel.dart @@ -505,11 +505,16 @@ class _ManualEntrySheetState extends State<_ManualEntrySheet> { }).toList(), ), const SizedBox(height: 16), + // Required, not optional: `TimeEntryCreateSerializer` rejects a + // blank description on a manual entry. The hint used to say + // optional, so leaving it empty got a 400 and a "Failed to add + // entry" snackbar with nothing pointing at the empty box. TextField( controller: _descController, maxLines: 2, + onChanged: (_) => setState(() {}), decoration: InputDecoration( - hintText: 'What did you work on? (optional)', + hintText: 'What did you work on?', filled: true, fillColor: AppColors.gray50, border: OutlineInputBorder( @@ -540,19 +545,26 @@ class _ManualEntrySheetState extends State<_ManualEntrySheet> { const SizedBox(width: 12), Expanded( child: FilledButton( - onPressed: () { - final end = DateTime.now(); - final start = end.subtract(Duration(minutes: _minutes)); - Navigator.pop( - context, - _ManualEntryResult( - startedAt: start, - endedAt: end, - billable: _billable, - description: _descController.text.trim(), - ), - ); - }, + // Disabled until there is something to send, so the + // refusal happens under the empty field rather than as a + // snackbar after a round trip. + onPressed: _descController.text.trim().isEmpty + ? null + : () { + final end = DateTime.now(); + final start = end.subtract( + Duration(minutes: _minutes), + ); + Navigator.pop( + context, + _ManualEntryResult( + startedAt: start, + endedAt: end, + billable: _billable, + description: _descController.text.trim(), + ), + ); + }, child: const Text('Add entry'), ), ), diff --git a/mobile/test/unit/time_report_test.dart b/mobile/test/unit/time_report_test.dart new file mode 100644 index 00000000..a7bb5247 --- /dev/null +++ b/mobile/test/unit/time_report_test.dart @@ -0,0 +1,170 @@ +import 'package:bottle_crm/config/api_config.dart'; +import 'package:bottle_crm/data/models/time_report.dart'; +import 'package:bottle_crm/providers/time_report_provider.dart'; +import 'package:flutter_test/flutter_test.dart'; + +/// The time report: what the API sends, and what the screen asks it for. +/// +/// The money is the part worth pinning. It arrives as a decimal string, +/// because the API builds it from Decimal columns and a JSON number would +/// round it in transit, so anything that parses it has to say so out loud. + +Map payload({ + List>? rows, + Map? totals, + List currencies = const ['USD'], + String groupBy = 'agent', +}) => { + 'start': '2026-08-01', + 'end': '2026-08-31', + 'group_by': groupBy, + 'rows': + rows ?? + [ + { + 'key': 'p1', + 'name': 'Aswani Kumar', + 'total_minutes': 135, + 'billable_minutes': 105, + 'billable_value': '135.50', + 'entry_count': 3, + }, + ], + 'totals': + totals ?? + { + 'total_minutes': 135, + 'billable_minutes': 105, + 'billable_value': '135.50', + 'entry_count': 3, + }, + 'currencies': currencies, +}; + +void main() { + group('parsing a report', () { + test('reads the window, the rows and the totals', () { + final report = TimeReport.fromJson(payload()); + + expect(report.start, DateTime(2026, 8, 1)); + expect(report.end, DateTime(2026, 8, 31)); + expect(report.groupBy, 'agent'); + expect(report.totalMinutes, 135); + expect(report.billableMinutes, 105); + expect(report.billableValue, 135.5); + expect(report.entryCount, 3); + expect(report.rows.single.name, 'Aswani Kumar'); + expect(report.rows.single.billableValue, 135.5); + }); + + test('parses a date as local midnight, not UTC midnight', () { + // A UTC midnight read in a zone behind Greenwich lands on the previous + // day and mislabels the window by one. + final report = TimeReport.fromJson(payload()); + + expect(report.start.hour, 0); + expect(report.start.day, 1); + expect(report.start.isUtc, isFalse); + }); + + test('an empty report is empty rather than broken', () { + final report = TimeReport.fromJson({ + 'start': '2026-08-01', + 'end': '2026-08-31', + 'group_by': 'ticket', + 'rows': >[], + 'totals': {}, + 'currencies': [], + }); + + expect(report.isEmpty, isTrue); + expect(report.totalMinutes, 0); + expect(report.billableValue, 0); + expect(report.billableShare, 0); + expect(report.currency, isNull); + }); + + test('a row with no key is the unattributed bucket, not a broken row', () { + final report = TimeReport.fromJson( + payload( + groupBy: 'account', + rows: [ + { + 'key': null, + 'name': 'No account', + 'total_minutes': 20, + 'billable_minutes': 0, + 'billable_value': '0.00', + 'entry_count': 1, + }, + ], + ), + ); + + expect(report.rows.single.key, isNull); + expect(report.rows.single.name, 'No account'); + }); + + test('two currencies in the window are flagged, not averaged into one', () { + final report = TimeReport.fromJson(payload(currencies: ['EUR', 'USD'])); + + expect(report.isMixedCurrency, isTrue); + // Null, so nothing can put one symbol in front of a sum of two. + expect(report.currency, isNull); + }); + + test('the billable share is a percentage of logged time', () { + final report = TimeReport.fromJson(payload()); + + expect(report.billableShare, 78); + }); + }); + + group('what the screen asks for', () { + test('the default window is the last 30 days, inclusive', () { + final filters = TimeReportFilters.recent(); + + expect(filters.end.difference(filters.start).inDays, 29); + expect(filters.groupBy, 'agent'); + expect(filters.billable, isNull); + }); + + test('the URL carries the window, the grouping and nothing else', () { + final url = ApiConfig.timeReport( + start: '2026-08-01', + end: '2026-08-31', + groupBy: 'account', + ); + + expect(url, contains('start=2026-08-01')); + expect(url, contains('end=2026-08-31')); + expect(url, contains('group_by=account')); + // The API rejects an unknown `billable`, so it is sent only when picked. + expect(url, isNot(contains('billable'))); + }); + + test('the billable filter is sent when it is picked', () { + final url = ApiConfig.timeReport( + start: '2026-08-01', + end: '2026-08-31', + groupBy: 'agent', + billable: 'true', + ); + + expect(url, contains('billable=true')); + }); + + test('clearing the billable filter is not the same as choosing false', () { + // 'false' asks for non-billable time; null asks for all of it. Folding + // the two together would quietly hide every billable hour. + final filters = TimeReportFilters( + start: DateTime(2026, 1, 1), + end: DateTime(2026, 2, 1), + billable: 'false', + ); + + expect(filters.copyWith(clearBillable: true).billable, isNull); + expect(filters.copyWith(groupBy: 'ticket').billable, 'false'); + }); + }); +}