feat(domain): model Annex VII state-of-health parameters - #56
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 17 |
| Duplication | 2 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
This PR successfully models the Annex VII state-of-health parameter sets as requested, utilizing a sum type to differentiate between EV and Stationary/LMT configurations. The implementation correctly handles disclosure classifications and conservative mapping for industrial batteries.
While Codacy reports the PR is up to standards, two main issues require attention: a value mismatch between the sector catalog and the schema enum for 'SLI' batteries, and a lack of range-validation in the plugin layer for the new percentage fields. There is also a notable amount of code duplication in test utilities that should be centralized to simplify future domain model migrations.
About this PR
- There is a systemic pattern of duplicating
BatteryDataandPassportconstruction logic acrossbenches,crates/dpp-tests, anddpp-domain. Consider centralizing these into a shared factory utility (e.g., under atest-utilsfeature indpp-domain) to reduce the manual effort required for future domain model updates.
Test suggestions
- StateOfHealth enum correctly round-trips through JSON serialization/deserialization for both EV and Stationary sets.
- Stationary/LMT parameter set fails to deserialize if unconditional parameters (capacity or self-discharge) are missing.
- Mapping logic in dpp-rules correctly identifies the required parameter set based on battery type (EV, LMT, Industrial, vs others).
- Disclosure classification enforcement: assert that 'individual' class data is hidden from Public and Authority audiences.
- Schema-level validation: reject an EV battery payload that attempts to include stationary-specific parameters.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| "industrial", | ||
| "ev", | ||
| "lmt", | ||
| "sli" |
There was a problem hiding this comment.
🟡 MEDIUM RISK
The product category 'sli' in the sector catalog is inconsistent with the value 'starting-lighting-ignition' defined in the v2.2.0 JSON schema enum. These should be aligned to ensure that values derived from the catalog pass schema validation.
| recycled_content_lithium_pct: Some(6.0), | ||
| recycled_content_nickel_pct: Some(9.0), | ||
| state_of_health_pct: Some(100.0), | ||
| state_of_health: None, |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: The BatteryData and Passport construction here is duplicated from crates/dpp-tests/tests/battery_end_to_end.rs. Centralizing these sample constructors into a shared factory module in the domain crate would ensure consistency and reduce the labor required for future schema evolutions.
Try running the following prompt in your IDE agent:
Extract the battery passport construction logic from
benches/src/aas.rsandcrates/dpp-tests/tests/battery_end_to_end.rsinto a shared factory function within a test-utility module incrates/dpp-domain.
| recycled_content_lithium_pct: None, | ||
| recycled_content_nickel_pct: None, | ||
| state_of_health_pct: None, | ||
| state_of_health: None, |
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: This BatteryData construction is duplicated from crates/dpp-domain/src/test_support.rs. Consider exposing sample_battery_data as a public test utility in dpp-domain (e.g., under a test-utils feature) to avoid boilerplate duplication across crates.
Try running the following prompt in your IDE agent:
Move
sample_battery_dataincrates/dpp-domain/src/test_support.rsto a public module or expose it via atest-utilsfeature indpp-domain, then refactorbenches/src/validation.rsto use it instead of a local struct literal.
Item 1 of the six left open by the citation audit. Replaces the flat
stateOfHealthPctwith the Annex VII Part A parameter sets, and fixes a disclosure defect found on the way.The disclosure defect
stateOfHealthPctwas not in the battery sector's disclosure map at all, so it fell through to the default and was served publicly.Annex XIII point 4(b) puts "information on the state of health of the battery pursuant to Article 14" in the individual-battery set, which Art. 77(2)(c) grants only to persons with a legitimate interest — and Art. 77(2)(b) does not grant to notified bodies or market surveillance. Both
stateOfHealthand the deprecatedstateOfHealthPctnow carry theindividualclass added in #53. This is that class's first real use.The model
Annex VII Part A is two disjoint lists, so
StateOfHealthis a sum type, not a struct of optionals — a flat struct would make "an EV battery with an ohmic resistance but no SOCE" representable, which the annex does not permit.The optionality is the annex's own wording, not a modelling choice. Items 1 and 4 of the stationary list are unconditional; items 2, 3 and 5 are each qualified "where possible". Items 1 and 4 are therefore
f64and the restOption<f64>, and a payload omitting either required item fails to deserialize.dpp_rules::batteries::degradationgainsannex_vii_parameter_set_for, mapping a battery type onto the applicable list. That module was a placeholder; this is its first real rule. Portable and SLI map toNone— Art. 14(1) names only stationary storage, LMT and EV batteries.One documented imprecision: Art. 3 defines a stationary battery energy storage system as a subset of industrial batteries, and
batteryTypecannot distinguish them."industrial"maps to the stationary list, which is the conservative direction — an industrial battery outside Annex VII Part A merely has the parameters treated as expected rather than surplus.Schema
New battery v2.2.0, now
currentSchemaVersion. v1.0.0–v2.1.0 stay registered so existing passports remain verifiable.stateOfHealthis aoneOfover the two parameter sets, each withadditionalProperties: false.That last detail matters: serde ignores unknown fields, so the guarantee that an EV payload cannot smuggle in a stationary parameter lives in the schema, not the type. There is a test asserting it at that layer rather than pretending the type enforces it.
Not in this PR
Annex VII Part B (expected lifetime: energy throughput, capacity throughput, harmful events, full equivalent cycles). It overlaps the existing
expectedLifetimeCyclesand the EFC work already indpp-rules, and deserves its own pass.Verification
just checkgreen: 813 tests, plugin suites included.just build-pluginsgreen.Nine new tests, including round-trip coverage of both parameter sets, the required/optional split, the schema-level rejection of a mixed payload, and one asserting the
individualclassification withholds state of health from authorities.