diff --git a/CHANGELOG.md b/CHANGELOG.md index 499ef9d..b77d78f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,39 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- `list()` refuses a `where=` filter naming the `Assignments` collection on four + further collections - `PortfolioEpics`, `TestPlanRuns`, `InboundAssignables` + and `OutboundAssignables` - raising `ValueError` before any request is sent, + as it already did for the six typed assignable collections. TargetProcess + answers such a filter with HTTP 200 and the *unfiltered* rows, so code that + appeared to narrow one of these collections by assignee was in fact receiving + every row; it now raises rather than returning silently-wrong results. Each + collection was confirmed against an unfiltered control on the same collection + before being added. Only `list()`'s `where=` is affected - reading + `InboundAssignables` or `OutboundAssignables` through `include=` is unchanged. +- The refusal's message states both ways a path under `Assignments` fails: a + filter on one of the collection's fields is accepted and ignored (HTTP 200), + while `Assignments.Count` is rejected by the query parser (HTTP 400). + +### Fixed + +- The eight numeric fields on the `RoleEffort` and `Time` read models no longer + carry a `ge=0` bound - `InitialEstimate`, `Effort`, `EffortCompleted`, + `EffortToDo`, `TimeSpent` and `TimeRemain` on `RoleEffort`, and `Spent` and + `Remain` on `Time`. Each is supplied by the server, and a constraint on a + server-supplied value fails the *whole entity* rather than the field, so one + out-of-range record would have aborted an entire `list()` page with a + `ParseError` - the rule `AssignableEntity` already stated and these two models + broke. A consumer relying on `ValidationError` for a negative value on these + fields, including on assignment, no longer receives one; `times.upsert` keeps + its write-side check, where a range error is actionable. +- `Time`'s class docstring and its `project` field description say what a + custom-activity entry's `project` holds. Such an entry has no `Assignable`, + and the field is populated all the same, matching that activity's own project + on every entry observed. + ## [0.2.1] - 2026-09-18 ### Fixed diff --git a/SPEC.md b/SPEC.md index 9123fbd..37bcd06 100644 --- a/SPEC.md +++ b/SPEC.md @@ -85,12 +85,12 @@ itself. local `skip` counter, since that can silently skip or repeat records if TP ever returns a non-linear continuation URL. - **Unrecognised query input**: TP answers a query parameter it does not - recognise, and a `where=` naming a nested collection path it does not - filter on, with HTTP 200 and the unfiltered, unsorted rows - - indistinguishable from success. The confirmed case is a filter on the - `Assignments` collection of an assignable, which returns the same rows as - the unfiltered request, while the `Assignment` entity's own - `GeneralUser.Id` filter applies. + recognise, and a `where=` naming a nested collection path it does not filter + on, with HTTP 200 and the unfiltered, unsorted rows - indistinguishable from + success. The confirmed case is a filter on the `Assignments` collection of an + assignable, which returns the same rows as the unfiltered request, while the + `Assignment` entity's own `GeneralUser.Id` filter applies. A collection + `Count` is the loud exception, rejected with HTTP 400 - `Comments.Count` too. - **Bulk endpoint**: writable collections expose `POST /api/v1/{collection}/bulk`, taking a JSON array of entity objects - an object carrying an `Id` updates that entity, an object without one @@ -506,10 +506,10 @@ entity back to TP. Fields TP computes and returns carry no `ge`/`le` bound. A constraint on a server-supplied value fails the **whole entity** rather than the field, so one out-of-range roll-up would abort an entire `list()` page with a `ParseError` - -and TP documents no bounds on effort, progress or flow metrics to assert. -`RoleEffort` and `Time` keep the bounds they were published with, and -`times.upsert` validates on the write side, where the value originates and a -range error is actionable. +and TP documents no bounds on effort, progress or flow metrics to assert. The +`RoleEffort` and `Time` join models carry none either; `times.upsert` validates +on the write side, where the value originates and a range error is actionable. +The attached-content byte-size fields still carry `ge=0`. `custom_fields` (`CustomFields`) sits on the `Entity` base, so custom-field values are reachable on every entity type: fetched with @@ -688,7 +688,7 @@ at send time, and outside the set; `prettify` stays deliberately absent (see above). **Filter paths.** `BaseResource.ignored_filter_paths` maps the leading segment -of a `where=` path TP silently ignores on a collection to the reason and the +of a `where=` path TP will not filter on for a collection to the reason and the route to use instead. The known set is declared once, as `ASSIGNABLE_IGNORED_FILTER_PATHS` in `resources/base.py`, and referenced by the six assignable managers (`UserStory`, `Bug`, `Task`, `Feature`, `Epic`, @@ -696,7 +696,10 @@ six assignable managers (`UserStory`, `Bug`, `Task`, `Feature`, `Epic`, `order_by` or `include` - matching a path's leading segment in any casing outside quoted values, and refuses with a `ValueError` naming the collection and the route. `entities` mirrors the refusal for every spelling of those six -collections and for the polymorphic `Assignable` / `Assignables` collection. +collections and of the untyped Assignable-derived collections confirmed so far - +`Assignable`, `PortfolioEpic`, `TestPlanRun`, `InboundAssignable` and +`OutboundAssignable`, in `_UNTYPED_ASSIGNABLE_COLLECTIONS`. That is what has +been checked, not every candidate; the TestPlan family is unexamined. | Ignored path | Route instead | | --- | --- | diff --git a/src/targetprocess/_joins.py b/src/targetprocess/_joins.py index a7a8921..c98d285 100644 --- a/src/targetprocess/_joins.py +++ b/src/targetprocess/_joins.py @@ -73,33 +73,39 @@ class RoleEffort(Entity): Breaks an Assignable's effort down by Role. + None of the numeric fields carries a range constraint. Each arrives from + the server on a read, whoever set it there, and ``AssignableEntity`` gives + the rule for that class of field: a constraint on a server-supplied value + fails the **whole entity** rather than the field, so one bad figure would + abort a whole ``list()`` page. Nothing validates one on the way in either - + the resource layer writes ``**fields`` and dumps no model - so unlike a + duration (see ``times.upsert``) this model has no write-side counterpart. + Attributes: - initial_estimate: Initial effort estimate >= 0 - effort: Total effort >= 0 - effort_completed: Completed effort >= 0 - effort_todo: Remaining effort >= 0 - time_spent: Time spent >= 0 - time_remain: Time remaining >= 0 + initial_estimate: Initial effort estimate + effort: Total effort + effort_completed: Completed effort + effort_todo: Remaining effort + time_spent: Time spent + time_remain: Time remaining assignable: Assignable (work item) reference role: Role reference """ # Effort / time tracking initial_estimate: float | None = Field( - default=None, alias="InitialEstimate", ge=0, description="Initial estimate" + default=None, alias="InitialEstimate", description="Initial estimate" ) - effort: float | None = Field(default=None, alias="Effort", ge=0, description="Total effort") + effort: float | None = Field(default=None, alias="Effort", description="Total effort") effort_completed: float | None = Field( - default=None, alias="EffortCompleted", ge=0, description="Completed effort" + default=None, alias="EffortCompleted", description="Completed effort" ) effort_todo: float | None = Field( - default=None, alias="EffortToDo", ge=0, description="Remaining effort" - ) - time_spent: float | None = Field( - default=None, alias="TimeSpent", ge=0, description="Time spent" + default=None, alias="EffortToDo", description="Remaining effort" ) + time_spent: float | None = Field(default=None, alias="TimeSpent", description="Time spent") time_remain: float | None = Field( - default=None, alias="TimeRemain", ge=0, description="Time remaining" + default=None, alias="TimeRemain", description="Time remaining" ) # Relationships @@ -157,15 +163,27 @@ class Time(Entity): ``assignable``). They are declared so an entry's origin is readable without a second fetch. + A null ``assignable`` does not imply a null ``project``: a + ``CustomActivity`` entry carries a project too, matching that activity's + own on every entry observed. Whether TP copies it from the activity or from + the project the entry was filed under cannot be told apart from that, since + an activity is itself scoped to one project. A narrowed read + (``result_include=``) leaves the reference ``None``, as it does any other. + + ``spent`` and ``remain`` carry no range constraint, for the reason + ``AssignableEntity`` gives. ``times.upsert`` validates on the way in + instead, where the value originates. + Attributes: - spent: Hours spent >= 0 - remain: Hours remaining >= 0 + spent: Hours spent + remain: Hours remaining is_estimation: Entry records an estimate rather than actual time date: Date/time the work is logged against description: Free-text description of the work assignable: Assignable (work item) reference user: User the time is logged for - project: Project reference (TP derives it from the Assignable) + project: Project reference - the Assignable's, or on a custom-activity + entry the activity's own project role: Role reference user_story: User story the entry was logged against, if any task: Task the entry was logged against, if any @@ -177,8 +195,8 @@ class Time(Entity): """ # Effort / time tracking - spent: float | None = Field(default=None, alias="Spent", ge=0, description="Hours spent") - remain: float | None = Field(default=None, alias="Remain", ge=0, description="Hours remaining") + spent: float | None = Field(default=None, alias="Spent", description="Hours spent") + remain: float | None = Field(default=None, alias="Remain", description="Hours remaining") is_estimation: bool | None = Field( default=None, alias="IsEstimation", description="Entry is an estimation" ) @@ -194,7 +212,11 @@ class Time(Entity): default=None, alias="Assignable", description="Assignable (work item)" ) user: UserRef | None = Field(default=None, alias="User", description="User") - project: EntityRef | None = Field(default=None, alias="Project", description="Project") + project: EntityRef | None = Field( + default=None, + alias="Project", + description="Project - from the Assignable, or the CustomActivity's own project", + ) role: EntityRef | None = Field(default=None, alias="Role", description="Role") # Narrow back-references: at most one is populated per entry. diff --git a/src/targetprocess/resources/base.py b/src/targetprocess/resources/base.py index afcb259..880c882 100644 --- a/src/targetprocess/resources/base.py +++ b/src/targetprocess/resources/base.py @@ -179,17 +179,25 @@ def _resolve_by_name[N: NamedEntity](candidates: Sequence[N], name: str, *, what return matches[0] -# The where= paths TargetProcess accepts and silently ignores on every collection -# whose entity type is an Assignable, keyed by the path's leading collection -# segment and mapped to the reason and the route to use instead. This is the one -# place the known set is declared: the assignable managers reference it and the -# generic entities path mirrors it. An entry is added only with live evidence - -# an unfiltered control returning the same rows. +# The where= paths TargetProcess will not filter on for a collection whose +# entity type is an Assignable, keyed by the path's leading collection segment +# and mapped to the reason and the route to use instead. This is the one place +# the known set is declared: the assignable managers reference it, and the +# generic entities path mirrors it for those managers' collections and for the +# Assignable-derived collections that have no typed manager. An entry is added +# only with live evidence - an unfiltered control returning the same rows. +# +# The refusal covers the whole Assignments prefix because no path under it is +# usable, though the two failure shapes differ: a filter on one of the +# collection's fields is accepted and ignored, while Assignments.Count is +# refused by the query parser. Both are stated, so the message is true +# whichever path a caller wrote. ASSIGNABLE_IGNORED_FILTER_PATHS: dict[str, str] = { "Assignments": ( - "TargetProcess accepts a filter on the Assignments collection and " - "ignores it, answering HTTP 200 with the unfiltered rows; query the " - "join entity instead - client.assignments.list(" + "TargetProcess does not filter on the Assignments collection: a filter " + "on one of its fields is accepted and ignored, answering HTTP 200 with " + "the unfiltered rows, and Assignments.Count is rejected outright with " + "HTTP 400; query the join entity instead - client.assignments.list(" 'where="GeneralUser.Id eq ", include=["Assignable"]) - and read ' "the work item off each assignment's assignable" ), @@ -202,7 +210,7 @@ def _resolve_by_name[N: NamedEntity](candidates: Sequence[N], name: str, *, what def check_filter_paths(where: str | None, ignored: Mapping[str, str], *, resource: str) -> None: - """Refuse a ``where=`` filter naming a path TargetProcess is known to ignore. + """Refuse a ``where=`` filter naming a path TargetProcess will not filter on. A name in ``ignored`` matches only as the leading segment of a dotted path, in any casing: never preceded by a word character or a dot, and @@ -258,10 +266,11 @@ class BaseResource[T: Entity]: TP serves in a shape the model cannot hold, each mapped to the reason and the route to use instead; ``get`` and ``list`` refuse them with ``ValueError`` before any request is sent. - ignored_filter_paths: Leading ``where=`` path segments TP accepts - and silently ignores on this collection, each mapped to the - reason and the route to use instead; ``list`` refuses them with - ``ValueError`` before any request is sent. + ignored_filter_paths: Leading ``where=`` path segments TP will not + filter on for this collection - accepting and ignoring some, + rejecting others - each mapped to the reason and the route to use + instead; ``list`` refuses them with ``ValueError`` before any + request is sent. """ entity_type: str # Override in subclass @@ -361,7 +370,7 @@ def check_custom_field_names( @classmethod def check_where(cls, where: str | None) -> None: - """Refuse a ``where=`` naming a path this collection silently ignores. + """Refuse a ``where=`` naming a path this collection will not filter on. Applies :func:`check_filter_paths` with ``ignored_filter_paths``. A collection with nothing declared there accepts every filter, as @@ -530,7 +539,7 @@ async def list( ValueError: Both ``order_by`` and ``order_by_desc`` were passed, ``skip`` is negative, ``innertake`` is negative, ``include`` names a field this collection cannot hydrate, or ``where`` - names a path TP silently ignores on it (see + names a path TP will not filter on for it (see ``ignored_filter_paths``) (raised when iteration begins) AuthenticationError: Invalid credentials ForbiddenError: Insufficient permissions diff --git a/src/targetprocess/resources/entities.py b/src/targetprocess/resources/entities.py index d9432fd..dd54971 100644 --- a/src/targetprocess/resources/entities.py +++ b/src/targetprocess/resources/entities.py @@ -88,13 +88,44 @@ def _check_include(entity_type: str, include: list[str] | None) -> None: guarded.check_include(include) +# Assignable-derived collections with no typed manager, so reachable only +# through this accessor - which makes keying them here the only place the +# refusal can be applied. Each one's ``/meta`` declares an ``Assignments`` +# collection alongside the rest of the assignable surface (``AssignedUser``, +# ``Times``, ``RoleEfforts``, ``Impediments``, ``TeamStates``). Named in the +# singular; ``_spellings`` derives the plural TP addresses the collection by. +# +# ``PortfolioEpic``, ``TestPlanRun``, ``InboundAssignable`` and +# ``OutboundAssignable`` were each confirmed live, against an unfiltered control +# on the same collection. The polymorphic ``Assignable`` is carried on the +# narrower argument that it returns a superset of the same rows as the six typed +# collections, so it cannot behave differently from them. +# +# This is what has been checked, not every candidate: the collections were named +# for checking rather than enumerated from the instance, so an untyped +# Assignable-derived collection missing here is unproven, not cleared. The +# TestPlan family is the obvious one left - ``TestPlanRun`` is covered and +# ``TestPlan`` is not. Add one the same way, with its own unfiltered control. +# +# ``Inbound``/``OutboundAssignables`` are also read as ``include=`` collection +# properties of an item, which is unaffected: the refusal is on ``list()``'s +# ``where=`` only. +_UNTYPED_ASSIGNABLE_COLLECTIONS: tuple[str, ...] = ( + "Assignable", + "PortfolioEpic", + "TestPlanRun", + "InboundAssignable", + "OutboundAssignable", +) + + # The where= counterpart: the ignored-filter declaration of every assignable # collection (``BaseResource.ignored_filter_paths``), keyed by each spelling the # caller may use, so the generic path refuses the same filters before any # request rather than returning the unfiltered rows TP answers with. Keyed -# spelling -> reasons rather than spelling -> class, because the polymorphic -# ``Assignables`` collection has no typed manager and is the same server -# behaviour on a superset of the same rows. The walk test in +# spelling -> reasons rather than spelling -> class, because the collections in +# ``_UNTYPED_ASSIGNABLE_COLLECTIONS`` have no typed manager to consult and are +# the same server behaviour on the same rows. The walk test in # ``tests/test_resources/test_ignored_filter_paths.py`` covers this map. _IGNORED_FILTER_PATHS: dict[str, dict[str, str]] = { spelling: resource.ignored_filter_paths @@ -107,7 +138,11 @@ def _check_include(entity_type: str, include: list[str] | None) -> None: RequestsResource, ) for spelling in _spellings(resource.entity_type) -} | dict.fromkeys(_spellings("Assignable"), ASSIGNABLE_IGNORED_FILTER_PATHS) +} | { + spelling: ASSIGNABLE_IGNORED_FILTER_PATHS + for collection in _UNTYPED_ASSIGNABLE_COLLECTIONS + for spelling in _spellings(collection) +} def _check_where(entity_type: str, where: str | None) -> None: @@ -290,7 +325,7 @@ async def list( ``skip`` is negative, ``innertake`` is negative, ``include`` names a field the typed resource for this collection refuses to hydrate, or ``where`` names a path TP - silently ignores on an assignable collection (raised when + will not filter on for an assignable collection (raised when iteration begins) AuthenticationError: Invalid credentials ForbiddenError: Insufficient permissions diff --git a/src/targetprocess/resources/times.py b/src/targetprocess/resources/times.py index 3507cf9..03674bf 100644 --- a/src/targetprocess/resources/times.py +++ b/src/targetprocess/resources/times.py @@ -118,9 +118,11 @@ def _supplied_mutable_fields( ValueError: ``spent`` or ``remain`` is not a finite, non-boolean number ``>= 0``. Checked here, ahead of ``find_for_day`` and any create/update, so a bad value is rejected before a request is - sent - not written and then hit as a confusing ``ParseError`` on - the next read-back (``Time.spent``/``Time.remain`` carry - ``ge=0``). + sent. This is the only range check on a duration, and deliberately + so: ``Time.spent`` and ``Time.remain`` constrain nothing, because a + constraint on a server-supplied value fails the whole entity and + would abort a whole ``list()`` page with a ``ParseError``. The write + path is where the value originates, so it is where the check belongs. """ _validate_duration("spent", spent) if remain is not None: diff --git a/tests/test_resources/test_ignored_filter_paths.py b/tests/test_resources/test_ignored_filter_paths.py index f60e9eb..37473c1 100644 --- a/tests/test_resources/test_ignored_filter_paths.py +++ b/tests/test_resources/test_ignored_filter_paths.py @@ -1,4 +1,4 @@ -"""Tests for the refusal of where= paths TargetProcess accepts and silently ignores. +"""Tests for the refusal of where= paths TargetProcess will not filter on. A filter on the Assignments collection of an assignable answers HTTP 200 with the unfiltered rows, so the assignable managers and the generic entities path refuse @@ -37,6 +37,18 @@ EpicsResource: "Epics", RequestsResource: "Requests", } + +# The Assignable-derived collections with no typed manager, so reachable only +# through the generic accessor: the collection name TP addresses, mapped to the +# singular entity type it is declared as. The four beyond Assignable were each +# confirmed live against an unfiltered control on the same collection. +UNTYPED_ASSIGNABLE_COLLECTIONS: dict[str, str] = { + "Assignables": "Assignable", + "PortfolioEpics": "PortfolioEpic", + "TestPlanRuns": "TestPlanRun", + "InboundAssignables": "InboundAssignable", + "OutboundAssignables": "OutboundAssignable", +} IGNORED_WHERE = "Assignments.GeneralUser.Id eq 1" @@ -166,6 +178,21 @@ def test_the_known_set_is_declared_once_and_shared(): assert resource_cls.ignored_filter_paths is ASSIGNABLE_IGNORED_FILTER_PATHS, resource_cls +def test_the_reason_states_both_observed_failure_shapes(): + """One message covers both: a field filter is ignored, Assignments.Count is rejected. + + The prefix is refused whole because no path under it filters, but the two + shapes fail differently, and a caller who wrote either should read + something true. Pinned here so neither half can be dropped silently. + """ + from targetprocess.resources.base import ASSIGNABLE_IGNORED_FILTER_PATHS + + reason = ASSIGNABLE_IGNORED_FILTER_PATHS["Assignments"] + assert "HTTP 200" in reason + assert "Assignments.Count" in reason and "HTTP 400" in reason + assert "client.assignments" in reason + + @pytest.mark.asyncio @pytest.mark.parametrize( "entity_type", @@ -193,6 +220,45 @@ async def test_entities_refuse_the_filter_for_every_assignable_spelling(entity_t handler.list.assert_not_called() +@pytest.mark.asyncio +@pytest.mark.parametrize( + "entity_type", + [ + spelling + for collection, singular in UNTYPED_ASSIGNABLE_COLLECTIONS.items() + for spelling in (collection, singular) + ], +) +async def test_untyped_assignable_collections_refuse_a_filter_on_assignments(entity_type: str): + """Each Assignable-derived collection without a typed manager refuses it too. + + These have no resource class to carry the declaration, so the generic + accessor's map is the only place the refusal can live - and the only route + a caller has to them. Both spellings TP accepts are covered. + """ + handler = AsyncMock(spec=RequestHandler) + entities = EntitiesResource(Mock(spec=TargetProcessClient), handler) + + with pytest.raises(ValueError, match="client.assignments") as excinfo: + async for _ in entities.list(entity_type, where=IGNORED_WHERE): + pass + assert f"is not supported on {entity_type}:" in str(excinfo.value) + handler.list.assert_not_called() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("entity_type", sorted(UNTYPED_ASSIGNABLE_COLLECTIONS)) +async def test_untyped_assignable_collections_forward_a_valid_filter_unchanged(entity_type: str): + """Only the Assignments prefix is refused on them; their own fields pass through.""" + where = "(EntityState.IsFinal eq 'false') and (AssignedUser.Id eq 7)" + handler = AsyncMock(spec=RequestHandler) + handler.list, seen = scripted_list([]) + entities = EntitiesResource(Mock(spec=TargetProcessClient), handler) + + assert [item async for item in entities.list(entity_type, where=where)] == [] + assert (seen["entity_type"], seen["where"]) == (entity_type, where) + + @pytest.mark.asyncio @pytest.mark.parametrize("entity_type", ["Comment", "Project"]) async def test_entities_forward_the_filter_where_nothing_is_declared(entity_type: str): @@ -206,10 +272,18 @@ async def test_entities_forward_the_filter_where_nothing_is_declared(entity_type def test_every_manager_with_ignored_filter_paths_is_mirrored_on_the_generic_path(): - """The generic map reaches each declaring manager by every spelling, plus Assignables.""" + """The generic map reaches each declaring manager, and each untyped collection. + + Two dimensions, and they are independent: the *paths* refused (one, + ``Assignments``) and the *collections* the refusal is applied to. Widening + coverage to a collection adds spellings here and no path there. + """ from targetprocess.resources.assignables import AssignableResource from targetprocess.resources.base import ASSIGNABLE_IGNORED_FILTER_PATHS - from targetprocess.resources.entities import _IGNORED_FILTER_PATHS + from targetprocess.resources.entities import ( + _IGNORED_FILTER_PATHS, + _UNTYPED_ASSIGNABLE_COLLECTIONS, + ) # AssignableResource declares the set once for the six managers; it is their # base class rather than a manager, so it has no collection to mirror. @@ -227,11 +301,18 @@ def test_every_manager_with_ignored_filter_paths_is_mirrored_on_the_generic_path for spelling in {*_spellings(resource_cls.entity_type), collection, collection.upper()}: reached = _IGNORED_FILTER_PATHS.get(spelling.casefold()) assert reached is resource_cls.ignored_filter_paths, spelling - for spelling in ("assignable", "assignables"): - assert _IGNORED_FILTER_PATHS[spelling] is ASSIGNABLE_IGNORED_FILTER_PATHS + # The untyped collections this suite names are exactly the ones the module + # declares, so the two cannot drift apart unnoticed. + assert set(_UNTYPED_ASSIGNABLE_COLLECTIONS) == set(UNTYPED_ASSIGNABLE_COLLECTIONS.values()) + + for singular in UNTYPED_ASSIGNABLE_COLLECTIONS.values(): + for spelling in {*_spellings(singular), singular.upper()}: + reached = _IGNORED_FILTER_PATHS.get(spelling.casefold()) + assert reached is ASSIGNABLE_IGNORED_FILTER_PATHS, spelling declared_spellings = frozenset().union( - *(_spellings(cls.entity_type) for cls in ASSIGNABLE_COLLECTIONS), _spellings("Assignable") + *(_spellings(cls.entity_type) for cls in ASSIGNABLE_COLLECTIONS), + *(_spellings(singular) for singular in UNTYPED_ASSIGNABLE_COLLECTIONS.values()), ) assert set(_IGNORED_FILTER_PATHS) <= declared_spellings diff --git a/tests/test_role_effort.py b/tests/test_role_effort.py index 072360f..caa06ec 100644 --- a/tests/test_role_effort.py +++ b/tests/test_role_effort.py @@ -1,7 +1,6 @@ """Tests for RoleEffort model.""" import pytest -from pydantic import ValidationError from targetprocess.models import RoleEffort @@ -37,9 +36,25 @@ def test_role_effort_minimal_parsing() -> None: assert re.effort is None and re.role is None -def test_role_effort_effort_validation() -> None: - with pytest.raises(ValidationError): - RoleEffort.model_validate({"Id": 2, "ResourceType": "RoleEffort", "Effort": -1.0}) +@pytest.mark.parametrize( + ("alias", "attribute"), + [ + ("InitialEstimate", "initial_estimate"), + ("Effort", "effort"), + ("EffortCompleted", "effort_completed"), + ("EffortToDo", "effort_todo"), + ("TimeSpent", "time_spent"), + ("TimeRemain", "time_remain"), + ], +) +def test_role_effort_parses_a_negative_roll_up_the_server_sent(alias: str, attribute: str) -> None: + # Every one of these is a server-computed roll-up, so none constrains its + # range: a constraint on a server-supplied value fails the whole entity + # rather than the field, and one out-of-range roll-up would abort a whole + # list() page with a ParseError. Reporting what TP sent is worth more than + # asserting an invariant the API never promised. + effort = RoleEffort.model_validate({"Id": 2, "ResourceType": "RoleEffort", alias: -1.0}) + assert getattr(effort, attribute) == -1.0 def test_role_effort_has_no_name_field() -> None: diff --git a/tests/test_time.py b/tests/test_time.py index e2b55f4..da130e7 100644 --- a/tests/test_time.py +++ b/tests/test_time.py @@ -3,7 +3,6 @@ from datetime import datetime import pytest -from pydantic import ValidationError from targetprocess.models import Time @@ -71,10 +70,14 @@ def test_time_pascal_case_accessors() -> None: assert time.ResourceType == "Time" -@pytest.mark.parametrize("field", ["Spent", "Remain"]) -def test_time_rejects_negative_hours(field: str) -> None: - with pytest.raises(ValidationError): - Time.model_validate({"Id": 2, "ResourceType": "Time", field: -1.0}) +@pytest.mark.parametrize(("alias", "attribute"), [("Spent", "spent"), ("Remain", "remain")]) +def test_time_parses_a_negative_duration_the_server_sent(alias: str, attribute: str) -> None: + # Neither field constrains its range, because both are server-supplied and a + # constraint would fail the whole entity rather than the field - one + # out-of-range record would abort a whole list() page. The range check lives + # on the write path, in times.upsert, where the value originates. + time = Time.model_validate({"Id": 2, "ResourceType": "Time", alias: -1.0}) + assert getattr(time, attribute) == -1.0 def test_time_declares_the_narrow_back_references() -> None: @@ -96,12 +99,23 @@ def test_time_declares_the_narrow_back_references() -> None: def test_time_back_reference_covers_a_custom_activity_entry() -> None: # A CustomActivity entry has no Assignable at all - the back-reference is # the only record of what the time was logged against. + # + # Project is populated all the same, and this is the shape live TP sends: a + # null Assignable alongside a set Project, which TP takes from the custom + # activity's own project rather than from the (absent) Assignable. Asserted + # here so the docstring's claim has a test behind it. time = Time.model_validate( - {"Id": 4, "CustomActivity": {"Id": 47, "Name": "Admin"}, "Assignable": None} + { + "Id": 4, + "CustomActivity": {"Id": 47, "Name": "Admin"}, + "Assignable": None, + "Project": {"Id": 300, "Name": "Platform"}, + } ) assert time.assignable is None assert time.custom_activity is not None assert time.custom_activity.name == "Admin" + assert time.project is not None and time.project.id == 300 def test_time_tolerates_unmodelled_keys() -> None: