Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for accessing a DID Group’s requirement as a first-class relationship, and covers it with a VCR-backed test to validate deserialization of the included requirements resource.
Changes:
- Expose
DidGroup.requirementviaRelationField("requirement"). - Add a new test that fetches a DID group with
include=requirementand asserts parsed requirement attributes/enums. - Add a VCR cassette fixture for the new test scenario.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/didww/resources/did_group.py |
Adds the requirement relationship to the DidGroup model and removes the old requirement_id() helper. |
tests/resources/test_did_group.py |
Adds a new test validating include("requirement") and the parsed Requirement fields. |
tests/fixtures/did_groups/show_with_requirement.yaml |
New VCR cassette recording the DID group “show” response including a requirements resource. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| requirement = RelationField("requirement") | ||
|
|
||
| class Meta: | ||
| type = "did_groups" | ||
|
|
||
| def requirement_id(self): | ||
| return self._relationship_id("requirement") | ||
|
|
||
|
|
There was a problem hiding this comment.
requirement_id() was removed from DidGroup. Since this is a public model method and there’s no direct replacement for retrieving the relationship ID without resolving/loading the related resource, this is a breaking API change. Consider keeping requirement_id() (possibly deprecated) as a thin wrapper around _relationship_id("requirement") to preserve backwards compatibility while still exposing the new requirement relation.
Fivell
force-pushed
the
feat/did-group-requirement-relationship
branch
from
60af240 to
1405da5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/didww/resources/did_group.py
Outdated
| def requirement_id(self): | ||
| return self._relationship_id("requirement") |
There was a problem hiding this comment.
DidGroup.requirement_id() is newly added but not covered by tests. Since other resources validate relationship ID accessors (e.g., Requirement.country_id()), add an assertion in the DID group tests to confirm requirement_id() returns the expected relationship ID (and returns None when the relationship is absent, if applicable).
| requirement = dg.requirement | ||
| assert requirement is not None | ||
| assert requirement.id == "c3d4e5f6-a1b2-7890-abcd-ef1234567890" | ||
| assert requirement.personal_proof_qty == 1 | ||
| assert requirement.business_proof_qty == 1 | ||
| assert requirement.id == "8da1e0b2-047c-4baf-9c57-57143f09b9ce" | ||
| assert requirement.identity_type == IdentityType.ANY | ||
| assert requirement.personal_area_level == AreaLevel.WORLDWIDE |
There was a problem hiding this comment.
This test validates the resolved dg.requirement object, but it doesn’t assert the new DidGroup.requirement_id() accessor added in this PR. Add an assertion like assert dg.requirement_id() == <expected id> to ensure the relationship ID can be accessed without resolving the relationship.
Fivell
force-pushed
the
feat/did-group-requirement-relationship
branch
from
1405da5 to
25d1e83
Compare
DIDWW API returns links-only (no data linkage) for relationships without ?include=, making _relationship_id() always return None. When include is used, the full object is already available. Removed from: City, AvailableDid, Requirement, DidGroup, and base. Replaced _id() test assertions with direct .id checks on included objects.
Fivell
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _null_relationship(self, key): | ||
| """Write null data into the ORM relationship, like Java's field = null.""" | ||
| self.raw_object.relationships[key] = {"data": None} | ||
| self._mark_relationship_dirty(key) | ||
|
|
There was a problem hiding this comment.
Removing DidwwApiModel._relationship_id/_relationship_ids breaks existing internal usage in examples (e.g., examples/did_trunk_assignment.py still calls result._relationship_id(...)), which will now raise AttributeError. Either update the examples to use resolved relationship objects (e.g., include the relationship and read .voice_in_trunk.id/.voice_in_trunk_group.id) or keep a supported helper for extracting relationship IDs from the raw relationship data.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
requirementRelationField to DidGroup_relationship_id()/_relationship_ids()methods from City, AvailableDid, Requirement, DidGroup, and base class — DIDWW API returns links-only (no data linkage) without?include=, so these always returned None.idon included relationship objectsTest plan