Feat/Setting Privilege Fee Per License Type #749
Conversation
## Walkthrough
This change introduces support for jurisdiction-specific privilege fees per license type throughout the backend. Configuration files for multiple jurisdictions are updated to include a new `privilegeFees` field, specifying fees for each license type. The backend data models, schemas, and validation logic are updated to handle and require `privilegeFees` instead of a single `jurisdictionFee`. Fee calculation logic in the purchase client is refactored to use the per-license-type fees, and related test data and assertions are updated accordingly. API models and schemas are also adjusted to reflect the new structure, deprecating the old `jurisdictionFee` property.
## Changes
| Files/Paths | Change Summary |
|--------------------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `backend/compact-connect/common_constructs/stack.py` | Renamed property `license_types` to `license_type_names` and added a new property `license_types` returning the original dictionary; updated docstrings. |
| `backend/compact-connect/compact-config/.../*.yml` | Added new `privilegeFees` sections to jurisdiction configuration files for various states and compacts, specifying per-license-type fees. |
| `backend/compact-connect/lambdas/nodejs/tests/lib/jurisdiction-client.test.ts`,<br>`backend/compact-connect/lambdas/nodejs/tests/sample-records.ts` | Updated test data to include `privilegeFees` for jurisdictions; marked `jurisdictionFee` as deprecated. |
| `backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py` | Added `JurisdictionPrivilegeFee` class and `privilege_fees` property to `Jurisdiction`; deprecated `jurisdiction_fee`. |
| `backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/api.py` | Added `JurisdictionPrivilegeFeeResponseSchema`; updated `JurisdictionOptionsResponseSchema` to use `privilegeFees` list and deprecated `jurisdictionFee`. |
| `backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/record.py` | Added `JurisdictionPrivilegeFeeRecordSchema`; updated `JurisdictionRecordSchema` to require `privilegeFees` and deprecate `jurisdictionFee`. |
| `backend/compact-connect/lambdas/python/common/tests/resources/dynamo/jurisdiction.json` | Added `privilegeFees` array to jurisdiction test JSON. |
| `backend/compact-connect/lambdas/python/custom-resources/tests/function/test_handlers/test_compact_configuration_uploader.py` | Updated test data generation and assertions to use `privilegeFees` per compact; deprecated `jurisdictionFee`; standardized decimal formatting. |
| `backend/compact-connect/lambdas/python/purchases/purchase_client.py` | Refactored fee calculation to use `privilegeFees` per license type; updated function signatures and error handling; military discount logic now applies to per-license fee. |
| `backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py` | Updated tests to use `licenseFee` and set amounts in `privilegeFees` array; removed reliance on single `jurisdictionFee`. |
| `backend/compact-connect/stacks/api_stack/v1_api/api_model.py` | Updated schemas and models to use `license_type_names` for enums; added `privilegeFees` property and deprecated `jurisdictionFee` in purchase privilege options schema. |
| `backend/compact-connect/stacks/persistent_stack/compact_configuration_upload.py` | Enhanced jurisdiction config validation to require and check `privilegeFees` for all license types, detect duplicates and unknowns, and improved error messages. |
| `backend/compact-connect/tests/resources/snapshots/COMPACT_CONFIGURATION_UPLOADER_BETA_ENV_INPUT.json`,<br>`backend/compact-connect/tests/resources/snapshots/PURCHASE_PRIVILEGE_OPTIONS_RESPONSE_SCHEMA.json` | Updated test snapshots to include `privilegeFees` in jurisdiction objects and schemas. |
| `backend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py` | Updated expected data to include `privilegeFees`; marked `jurisdictionFee` as deprecated. |
| `backend/compact-connect/tests/smoke/smoke_tests_env_example.json` | Added `ENVIRONMENT_NAME` key to smoke test environment config. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant API
participant PurchaseClient
participant Jurisdiction
Client->>API: Request privilege purchase (licenseTypeAbbreviation)
API->>PurchaseClient: process_charge_on_credit_card_for_privilege_purchase(...)
PurchaseClient->>Jurisdiction: Get privilege_fees
PurchaseClient->>PurchaseClient: _calculate_jurisdiction_fee(jurisdiction, licenseTypeAbbreviation, ...)
alt privilege fee found
PurchaseClient->>PurchaseClient: Apply military discount if applicable
PurchaseClient-->>API: Return calculated fee
else privilege fee not found
PurchaseClient-->>API: Raise error
end
API-->>Client: Respond with result or errorAssessment against linked issues
Suggested reviewers
Poem
|
landonshumway-ia
force-pushed
the
feat/fee-per-license-type
branch
from
bf9ed1d to
59f3dc3
Compare
several jurisdictions have reported that they set different privilege fees based on the type of license for which the privilege record is being purchased. This deprecates the old jurisdictionFee field in place of a 'privilegeFees' field, which is a list of objects defining the amount for every license type supported by the compact.
landonshumway-ia
force-pushed
the
feat/fee-per-license-type
branch
from
59f3dc3 to
032f9a7
Compare
landonshumway-ia
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
backend/compact-connect/compact-config/octp/kentucky.yml (1)
4-9: Consider adding a deprecation comment for jurisdictionFee.Both the old
jurisdictionFeeand newprivilegeFeesare present, which could cause confusion about which takes precedence. Consider adding a comment indicating thatjurisdictionFeeis deprecated and will be removed in a future update.backend/compact-connect/compact-config/octp/mississippi.yml (1)
4-9: Consider adding a deprecation comment for jurisdictionFee.As with the Kentucky configuration, both the old
jurisdictionFeeand newprivilegeFeesfields are present. Consider adding a comment indicating thatjurisdictionFeeis deprecated to clarify the transition to the new fee structure.backend/compact-connect/compact-config/octp/alabama.yml (1)
4-4: DeprecatejurisdictionFeewith a comment
SincejurisdictionFeeis now superseded byprivilegeFees, annotate it with a YAML comment referencing issue #635 to signal its removal in a future release.backend/compact-connect/compact-config/coun/florida.yml (1)
4-4: Annotate deprecation ofjurisdictionFee
Add a YAML comment tojurisdictionFee: 15indicating deprecation (issue #635) for consistency across all jurisdiction configs.backend/compact-connect/compact-config/coun/kentucky.yml (1)
4-4: AnnotatejurisdictionFeedeprecation
Include a comment onjurisdictionFee: 100marking it as deprecated (issue #635) to clearly communicate its upcoming removal.backend/compact-connect/tests/resources/snapshots/PURCHASE_PRIVILEGE_OPTIONS_RESPONSE_SCHEMA.json (1)
99-102: Add deprecation notice tojurisdictionFeedescription.Since
jurisdictionFeeis being deprecated in favor of the newprivilegeFeesfield, it would be helpful to include a deprecation notice in its description to guide API consumers."jurisdictionFee": { - "description": "The fee for the jurisdiction", + "description": "The fee for the jurisdiction (DEPRECATED: Use privilegeFees instead)", "type": "number" },backend/compact-connect/tests/resources/snapshots/COMPACT_CONFIGURATION_UPLOADER_BETA_ENV_INPUT.json (1)
373-373: Consider adding comments aboutjurisdictionFeedeprecation.To maintain consistency with the sample-records.ts file, consider adding deprecation comments for the
jurisdictionFeeproperty in these configurations as well.Add a comment similar to this before each
jurisdictionFeeentry:// deprecated - to be removedAlso applies to: 253-253, 283-283, 313-313, 343-343
backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py (1)
111-111: EnsurelicenseFeenaming is consistent with the rest of the codebase.The parameter name
licenseFeeis used in the test data, while the actual field in the model is calledprivilegeFees. Consider using more consistent naming to avoid confusion, especially since this is test data that might be referenced by other developers.- {'postalCode': 'oh', 'jurisdictionName': 'ohio', 'licenseFee': 100.00}, + {'postalCode': 'oh', 'jurisdictionName': 'ohio', 'privilegeFee': 100.00},Then update the iteration code accordingly:
for licensee_fee in jurisdiction['privilegeFees']: # DynamoDB loads this as a decimal - licensee_fee['amount'] = Decimal(jurisdiction_test_item['licenseFee']) + licensee_fee['amount'] = Decimal(jurisdiction_test_item['privilegeFee'])Also applies to: 348-349
backend/compact-connect/lambdas/nodejs/tests/sample-records.ts (1)
252-334: Verify the amount values are appropriate for testing.The sample data uses "100" for all license types. While this is fine for basic testing, consider adding variety in the test data (different amounts for different license types) to better validate that the code correctly handles different fees per license type.
'privilegeFees': [ { 'licenseTypeAbbreviation': 'aud', - 'amount': '100' + 'amount': '100' }, { 'licenseTypeAbbreviation': 'slp', - 'amount': '100' + 'amount': '125' } ],backend/compact-connect/lambdas/python/custom-resources/tests/function/test_handlers/test_compact_configuration_uploader.py (1)
227-232: Inconsistent deprecation comment.There's an inconsistency in the deprecation comment, using "when frontend is updated" instead of the standard GitHub issue reference used elsewhere.
For consistency with other comments in the codebase, consider changing:
- # deprecated - will be removed when frontend is updated + # deprecated - will be removed as part of https://github.com/csg-org/CompactConnect/issues/636
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
backend/compact-connect/common_constructs/stack.py(1 hunks)backend/compact-connect/compact-config/aslp/kentucky.yml(1 hunks)backend/compact-connect/compact-config/aslp/nebraska.yml(1 hunks)backend/compact-connect/compact-config/aslp/ohio.yml(1 hunks)backend/compact-connect/compact-config/coun/florida.yml(1 hunks)backend/compact-connect/compact-config/coun/kentucky.yml(1 hunks)backend/compact-connect/compact-config/coun/nebraska.yml(1 hunks)backend/compact-connect/compact-config/coun/ohio.yml(1 hunks)backend/compact-connect/compact-config/octp/alabama.yml(1 hunks)backend/compact-connect/compact-config/octp/arkansas.yml(1 hunks)backend/compact-connect/compact-config/octp/kentucky.yml(1 hunks)backend/compact-connect/compact-config/octp/louisiana.yml(1 hunks)backend/compact-connect/compact-config/octp/mississippi.yml(1 hunks)backend/compact-connect/compact-config/octp/nebraska.yml(1 hunks)backend/compact-connect/compact-config/octp/ohio.yml(1 hunks)backend/compact-connect/lambdas/nodejs/tests/lib/jurisdiction-client.test.ts(2 hunks)backend/compact-connect/lambdas/nodejs/tests/sample-records.ts(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/api.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/record.py(3 hunks)backend/compact-connect/lambdas/python/common/tests/resources/dynamo/jurisdiction.json(1 hunks)backend/compact-connect/lambdas/python/custom-resources/tests/function/test_handlers/test_compact_configuration_uploader.py(9 hunks)backend/compact-connect/lambdas/python/purchases/purchase_client.py(4 hunks)backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(9 hunks)backend/compact-connect/stacks/persistent_stack/compact_configuration_upload.py(3 hunks)backend/compact-connect/tests/resources/snapshots/COMPACT_CONFIGURATION_UPLOADER_BETA_ENV_INPUT.json(6 hunks)backend/compact-connect/tests/resources/snapshots/PURCHASE_PRIVILEGE_OPTIONS_RESPONSE_SCHEMA.json(2 hunks)backend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py(1 hunks)backend/compact-connect/tests/smoke/smoke_tests_env_example.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/api.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (1)
amount(55-56)
backend/compact-connect/lambdas/python/custom-resources/tests/function/test_handlers/test_compact_configuration_uploader.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (4)
compact(74-75)jurisdiction_name(66-67)postal_abbreviation(70-71)privilege_fees(84-85)
🔇 Additional comments (56)
backend/compact-connect/common_constructs/stack.py (2)
62-64: Good refactoring to improve property naming clarity.Renaming the original
license_typestolicense_type_namesmore clearly indicates that this property returns a flattened list of license type names across all compacts, which is important for the new per-license fee structure.
66-69: Well-structured addition of the raw license types accessor.The new
license_typesproperty provides direct access to the structured dictionary of license types by compact, which will be useful for validating that configurations include fees for all required license types. Good use of the cached_property decorator for performance.backend/compact-connect/compact-config/octp/kentucky.yml (1)
5-9: Successfully implemented per-license privilege fees.The new
privilegeFeesstructure correctly defines fees for each license type ("ot" and "ota"), implementing the PR's goal of supporting license-specific fees.backend/compact-connect/lambdas/python/common/tests/resources/dynamo/jurisdiction.json (1)
9-18: Correctly updated test fixture with new privilegeFees structure.The test data now includes the new
privilegeFeesarray with appropriate license types and amounts, ensuring that unit tests will properly validate the new fee structure. Good alignment with the configuration changes.backend/compact-connect/compact-config/octp/mississippi.yml (1)
5-9: Consistently implemented per-license privilege fees.The
privilegeFeesstructure is implemented consistently with other configuration files, defining the same license types ("ot" and "ota") with appropriate fee amounts.backend/compact-connect/tests/smoke/smoke_tests_env_example.json (1)
14-15: Review placeholder environment variables and documentation.
You’ve added"CC_TEST_PROVIDER_USER_PASSWORD"and the new"ENVIRONMENT_NAME"entries for smoke tests. Ensure these placeholders are clearly documented (e.g., in README or comments) and that real credentials are never committed. Also verify that the smoke‐test harness actually consumesENVIRONMENT_NAMEand that its value is overridden in CI pipelines.backend/compact-connect/compact-config/octp/louisiana.yml (1)
5-9: AddprivilegeFeesfor per-license-type fee configuration.
You’ve introducedprivilegeFeeswith entries for"ot"and"ota"in Louisiana’s OCTP config. This aligns with the PR’s goal to move from a singlejurisdictionFeeto license-specific fees.
Please confirm:
- All supported license types for OCTP in Louisiana are represented here.
- The
compact_configuration_uploadlogic will flag or log the now-deprecatedjurisdictionFeeto guide its eventual removal.backend/compact-connect/compact-config/octp/nebraska.yml (1)
5-9: IntroduceprivilegeFeesand preserve legacy fallback.
Nebraska’s OCTP config now includesprivilegeFeesfor"ot"and"ota". Ensure:
- This list covers every valid license type for Nebraska’s OCTP compact.
- The upload/validation step warns about the existing
jurisdictionFeeso teams can migrate off it cleanly.backend/compact-connect/compact-config/octp/arkansas.yml (1)
5-9: ValidateprivilegeFeesentries and correct fee amounts.
Arkansas’s OCTP config mirrorsjurisdictionFee: 3withprivilegeFeesfor"ot"and"ota"atamount: 3. Please verify:
- That
3is indeed the intended per-license-type fee.- No additional license types are missing from this list.
Also ensure the uploader flags mixed use ofjurisdictionFeeandprivilegeFees.backend/compact-connect/compact-config/coun/ohio.yml (1)
5-7: Add OhioprivilegeFeesfor LPC license type.
You’ve added a singleprivilegeFeesentry for"lpc"withamount: 100. Confirm:
- The COUN compact’s Ohio jurisdiction only requires
"lpc".- The schema and uploader validation will enforce no missing or extraneous license types.
Also plan for deprecatingjurisdictionFeein upcoming releases.backend/compact-connect/compact-config/coun/nebraska.yml (1)
5-7: Approve addition ofprivilegeFees
The newprivilegeFeesblock is correctly structured withlicenseTypeAbbreviation: "lpc"andamount: 100, aligning with the updated per‑license fee model.backend/compact-connect/compact-config/aslp/kentucky.yml (1)
5-9: Approve addition ofprivilegeFees
TheprivilegeFeessection correctly enumerates the "aud" and "slp" license type fees to replace the deprecated singlejurisdictionFee.backend/compact-connect/compact-config/octp/ohio.yml (1)
5-9: Approve addition ofprivilegeFees
TheprivilegeFeesentries for "ot" and "ota" are properly defined and match the updated fee schema requirements.backend/compact-connect/compact-config/aslp/nebraska.yml (1)
5-9: Approve addition ofprivilegeFees
TheprivilegeFeeslist correctly covers the "aud" and "slp" license types in line with the ASLP compact’s per‑license fee update.backend/compact-connect/compact-config/aslp/ohio.yml (1)
5-9: Approve addition ofprivilegeFees
The newprivilegeFeesblock defines the "aud" and "slp" license type fees correctly, fully supporting the migration away from the singlejurisdictionFee.backend/compact-connect/compact-config/octp/alabama.yml (1)
5-9: IntroduceprivilegeFeesto support license-type-specific fees
The newprivilegeFeeslist correctly defines fees for the"ot"and"ota"license types. Ensure that these abbreviations align with the compact’s supported license types in the data model and validation logic.backend/compact-connect/compact-config/coun/florida.yml (1)
5-7: IntroduceprivilegeFeesreplacing the singlejurisdictionFee
The addedprivilegeFeesentry for license type"lpc"aligns with the new fee model. Confirm that downstream validation and tests account for this per-license-type structure.backend/compact-connect/compact-config/coun/kentucky.yml (1)
5-7: AddprivilegeFeessection
The new privilege fee for license type"lpc"is correctly defined. Verify that the compact configuration uploader enforces coverage of every license type required by the Kentucky compact.backend/compact-connect/lambdas/nodejs/tests/lib/jurisdiction-client.test.ts (4)
26-32: Verify issue reference in deprecation comment
The deprecation comment forjurisdictionFeepoints to issue #636, but this PR resolves issue #635. Please confirm the correct issue number and update the link for accurate traceability.
30-53: IncludeprivilegeFeesin sample data
TheprivilegeFeesarray is correctly structured for the first jurisdiction entry. This ensures tests validate per-license-type fees as per the updated data model.
110-113: Verify deprecation comment issue reference
The deprecation comment for the second jurisdiction’sjurisdictionFeealso references issue #636. Confirm and align this with the correct issue #635 to maintain consistency.
114-137: AddprivilegeFeesfor the second jurisdiction sample
AppendingprivilegeFeesfor Nebraska mirrors the production configuration. Ensure test expectations for the number and content of fee entries match real-world license types.backend/compact-connect/tests/smoke/purchasing_privileges_smoke_tests.py (1)
66-72: Confirm deprecation comment and expectedprivilegeFees
The smoke test now expects aprivilegeFeesarray alongside the deprecatedjurisdictionFee. Please verify the deprecation link (#636vs#635) and ensure the license-type entries ("aud"and"slp") comprehensively cover the Kentucky compact’s license types.backend/compact-connect/tests/resources/snapshots/PURCHASE_PRIVILEGE_OPTIONS_RESPONSE_SCHEMA.json (1)
103-121: The newprivilegeFeesstructure looks good.The addition of the
privilegeFeesstructure correctly implements the requirement to support different privilege fees based on license type. It's well-structured as an array of objects with appropriate required fields.backend/compact-connect/tests/resources/snapshots/COMPACT_CONFIGURATION_UPLOADER_BETA_ENV_INPUT.json (2)
224-233: TheprivilegeFeesimplementation for Ohio looks good.The structure matches the schema requirements and contains entries for both license types (aud and slp).
255-264: Consistent implementation ofprivilegeFeesacross all jurisdictions.The implementation is consistent across all jurisdictions, with each jurisdiction defining fees for their respective license types. This aligns well with the PR objective to set different privilege fees depending on license type.
Also applies to: 285-294, 315-324, 345-354, 375-384
backend/compact-connect/lambdas/python/purchases/tests/unit/test_purchase_client.py (2)
119-122: Good implementation for handlingprivilegeFeesin test data.The updated code correctly iterates through the
privilegeFeeslist to set the fee amount for each license type. This ensures the test data is consistent with the new fee structure while maintaining compatibility with existing tests.
230-231:✅ Verification successful
Verify that
license_type_abbreviationis properly handled.I notice the
license_type_abbreviationparameter is passed through toprocess_charge_for_licensee_privilegesin multiple test cases, which aligns with the new per-license-type fee structure. Ensure the actual implementation of this method correctly filters theprivilegeFeesarray by license type.To verify the implementation is correct, consider checking if the purchase_client.py logic looks for the matching license type:
Also applies to: 304-305, 357-358, 452-453
🏁 Script executed:
#!/bin/bash # Search for the relevant code in purchase_client.py that uses license_type_abbreviation to find the appropriate fee rg -A 5 "def process_charge_for_licensee_privileges" backend/compact-connect/lambdas/python/purchases/purchase_client.py rg -A 10 "license_type_abbreviation" backend/compact-connect/lambdas/python/purchases/purchase_client.pyLength of output: 6500
License type filtering is correctly implemented
The
_calculate_jurisdiction_feehelper uses:next( (fee for fee in jurisdiction.privilege_fees if fee.license_type_abbreviation == license_type_abbr), None )to select the fee matching the passed‑in
license_type_abbreviation, so the existing tests align with the implementation. No further changes required.backend/compact-connect/lambdas/nodejs/tests/sample-records.ts (2)
248-249: Good documentation of deprecated fields.The comments clearly indicate that the
jurisdictionFeefield is deprecated and will be removed in a future issue, providing the issue number for reference. This helps other developers understand the transition plan.Also applies to: 323-324
252-275: Complete and consistent implementation ofprivilegeFeesin sample data.The new
privilegeFeesfield is correctly implemented in both the DynamoDB format (with nested 'L' and 'M' types) and the unmarshalled format. The structure matches what's defined in the schema and includes multiple license types, which aligns with the project requirements.Also applies to: 325-334
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/__init__.py (3)
44-57: Well-structured data model for privilege fees.The new
JurisdictionPrivilegeFeeclass provides a clean interface for accessing license fee data with appropriate property accessors. This implementation aligns with existing design patterns in the codebase.
77-81: Appropriate deprecation of jurisdiction_fee property.Clear documentation with the issue reference helps other developers understand why this field is deprecated and what to use instead.
83-85: Properly implemented privilege_fees property.The implementation correctly converts raw data into typed objects, providing a consistent access pattern. This supports the new per-license-type fee structure requested by jurisdictions.
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/record.py (3)
23-26: Well-defined schema for license-specific fees.The schema correctly defines required fields with appropriate types and constraints, matching the new data model.
38-38: Correctly implemented required privilegeFees field.The field is properly defined as a required list of nested schema objects, enforcing the new data structure.
61-62: Appropriate handling of deprecated field.The
jurisdictionFeefield is correctly marked as optional with a deprecation comment, maintaining backward compatibility while guiding developers toward the new structure.backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/jurisdiction/api.py (4)
3-3: Updated imports to support new field types.The import statement has been properly updated to include the
Listfield which is used for the newprivilegeFeesproperty.
23-26: Well-structured API response schema for privilege fees.The schema correctly defines the required fields with appropriate types, aligning with the data model.
38-38: Properly implemented privilegeFees in API response schema.The field is correctly defined as a required list of nested schema objects, ensuring API responses include the new fee structure.
43-44: Appropriate handling of deprecated field in API response.The
jurisdictionFeefield is correctly marked as optional with a deprecation comment, maintaining backward compatibility while indicating future removal.backend/compact-connect/lambdas/python/custom-resources/tests/function/test_handlers/test_compact_configuration_uploader.py (4)
15-16: Good use of constants for compact abbreviations.Replacing hardcoded strings with named constants improves code readability and maintainability.
44-52: Well-implemented test data generation for privilege fees.The updated function correctly generates different license-specific fees based on compact type, accurately reflecting the new data structure requirements.
56-58: Appropriate inclusion of both new and deprecated fee fields in test data.The test data includes both the new
privilegeFeesstructure and the deprecatedjurisdictionFeefield with a clear comment, ensuring tests validate backward compatibility.
141-145: Correct test assertions for the new privilege fees structure.The test expectations have been updated to include the new
privilegeFeesstructure with precise decimal handling, ensuring accurate validation of the configuration uploader's behavior.backend/compact-connect/stacks/persistent_stack/compact_configuration_upload.py (3)
143-143: Appropriate stack retrieval for license type validation.The addition of retrieving the current stack instance enables access to license type information needed for validation.
190-194: Good enhancement to validation call.The function call has been expanded to include the compact abbreviation, jurisdiction object, and license types specific to that compact, which enables more thorough validation.
205-242: Well-implemented jurisdiction fee validation.The validation logic is thorough, covering several important cases:
- Ensuring all license types have corresponding fees defined
- Detecting duplicate license type fees
- Checking for unknown license types not defined in the compact
- Requiring privilege fees to be defined
The implementation is robust and provides clear error messages that will help administrators quickly fix configuration issues.
backend/compact-connect/stacks/api_stack/v1_api/api_model.py (3)
157-157: Correctly updated references to license type names.All references to
self.stack.license_typeshave been properly updated toself.stack.license_type_namesto align with the refactoring in the Stack class, where license types was renamed to represent a flattened list of names.Also applies to: 528-528, 977-977, 1019-1019, 1156-1156, 1639-1639
819-820: Properly documented deprecation.The
jurisdictionFeeproperty is clearly marked as deprecated with a reference to the corresponding issue for future removal, which follows good practices for API versioning.Also applies to: 834-838
839-850: Well-structured schema for new privilege fees.The new
privilegeFeesproperty is correctly implemented as an array of objects, each containing a license type abbreviation and an amount. The schema definition is consistent with the rest of the API model and includes all necessary fields.backend/compact-connect/lambdas/python/purchases/purchase_client.py (6)
44-46: Well-documented function signature update.The
_calculate_jurisdiction_feefunction's signature has been updated to include the license type abbreviation parameter, and the docstring has been properly updated to document this change.Also applies to: 51-52, 54-55
57-71: Robust implementation of license type-specific fee calculation.The implementation efficiently finds the matching fee for the specified license type using a generator expression with
next(). It includes proper error handling and logging when a fee for the specified license type is not found.
72-82: Correctly updated military discount logic.The military discount logic has been updated to apply to the license type-specific fee rather than a generic jurisdiction fee, maintaining the discount functionality with the new fee structure.
132-149: Well-documented parameter propagation in privilege cost calculation.The
_get_total_privilege_costfunction has been updated to include the license type abbreviation parameter and properly passes it to_calculate_jurisdiction_fee. The docstring has been updated to reflect this change.Also applies to: 152-156
355-359: Consistently updated payment processing.The payment processing code has been updated to pass the license type abbreviation to the fee calculation function, ensuring consistent application of the new fee structure.
409-414: Proper propagation of license type parameter in fee calculation.The
_get_total_privilege_costcall has been updated to include the license type abbreviation parameter, ensuring the correct total cost calculation.
...nd/compact-connect/tests/resources/snapshots/PURCHASE_PRIVILEGE_OPTIONS_RESPONSE_SCHEMA.json
Outdated
Show resolved
Hide resolved
backend/compact-connect/stacks/persistent_stack/compact_configuration_upload.py
Outdated
Show resolved
Hide resolved
landonshumway-ia
changed the title
backend/compact-connect/stacks/persistent_stack/compact_configuration_upload.py
Outdated
Show resolved
Hide resolved
|
@jlkravitz This is ready for your review. Thanks |
several jurisdictions have reported that they set different
privilege fees based on the type of license for which the
privilege record is being purchased. This deprecates the old
jurisdictionFee field in place of a 'privilegeFees' field, which
is a list of objects defining the amount for every license type
supported by the compact.
Testing List
Closes #635
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores