fix(condo): DOMA-11706 use address service client for property meter import#7840
fix(condo): DOMA-11706 use address service client for property meter import#7840abshnko wants to merge 4 commits into
Conversation
…adrress resolve instead of billing PropertyResolver
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe meter registration service now delegates address normalization and organization property matching to a shared resolver returning resolved addresses and matching properties. Tests cover recognized, organization-unmatched, and unrecognized addresses. ChangesProperty meter resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MeterService as RegisterPropertyMetersReadingsService
participant Resolver as resolvePropertyMeterAddressesForOrganization
participant AddressService as addressService
participant Property as Keystone Property
MeterService->>Resolver: readings and organizationId
Resolver->>AddressService: bulkSearch reading addresses
AddressService-->>Resolver: normalized addresses and addressKeys
Resolver->>Property: find organization properties by addressKey
Property-->>Resolver: matching properties
Resolver-->>MeterService: resolvedAddresses and properties
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 619bdd4498
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| readings, | ||
| addressService = createInstance(), | ||
| }) { | ||
| const uniqueAddresses = [...new Set(readings.map(({ address }) => address).filter(Boolean))] |
There was a problem hiding this comment.
When a caller supplies a FIAS house id through reading.addressInfo.globalId (or relies on it because address is empty/ambiguous), this helper never includes that value in the address-service lookup; it only normalizes reading.address. The previous PropertyResolver path consumed addressInfo and searched fiasId:<globalId>, so those rows now produce no addressKey and the mutation returns PROPERTY_NOT_FOUND even when the organization property exists. Include the addressInfo.globalId/fiasId: candidate when building the lookup set.
Useful? React with 👍 / 👎.
![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 (2)
apps/condo/domains/meter/utils/serverSchema/resolvePropertyMeterAddressesForOrganization.spec.js (1)
11-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test for the "address not recognized" case.
The resolver has three main outcomes: property matched, property not in organization, and address not recognized. Only the first two are tested. The "address not recognized" path (when
bulkSearchreturns noaddressKeyornormalizedAddress) setspropertyAddressto{ error: ERRORS.ADDRESS_NOT_RECOGNIZED_VALUE }andpropertiesto{}— this should be covered.✨ Suggested test
test('returns address not recognized error when bulkSearch fails to normalize address', async () => { const address = 'г Иваново, мкр Московский, д 14А к 1' const addressService = { bulkSearch: jest.fn().mockResolvedValue({ map: {}, addresses: {}, }), } find.mockResolvedValue([]) const result = await resolvePropertyMeterAddressesForOrganization({ organizationId: 'organization-1', tin: '1234567890', readings: [{ address }], addressService, }) expect(result.resolvedAddresses[address].addressResolve.propertyAddress).toEqual({ error: ERRORS.ADDRESS_NOT_RECOGNIZED_VALUE, }) expect(result.resolvedAddresses[address].addressResolve.properties).toEqual({}) expect(find).not.toHaveBeenCalled() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/meter/utils/serverSchema/resolvePropertyMeterAddressesForOrganization.spec.js` around lines 11 - 97, Add a test in the resolvePropertyMeterAddressesForOrganization suite covering bulkSearch returning empty map and addresses results. Assert propertyAddress equals { error: ERRORS.ADDRESS_NOT_RECOGNIZED_VALUE }, properties is {}, and find is not called when the address cannot be recognized.Source: Coding guidelines
apps/condo/domains/meter/schema/RegisterPropertyMetersReadingsService.js (1)
163-164: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a Map for property lookups inside the readings loop.
properties.find()is O(n) per reading, making the loop O(n×m). AMap<addressKey, property>built once before the loop gives O(1) lookups.⚡ Proposed refactor
const meterReadingForSearchingDuplicates = await getMeterReadingsForSearchingDuplicates(readings, meters, properties, 'PropertyMeterReading') + const propertiesByAddressKey = new Map(properties.map(p => [p.addressKey, p])) + for (const reading of readings) { const meterNumber = reading.meterNumber.trim() const addressKey = get(resolvedAddresses, [reading.address, 'addressResolve', 'propertyAddress', 'addressKey']) let readingSource = get(reading, 'readingSource') if (isNil(readingSource)) { readingSource = { id: OTHER_METER_READING_SOURCE_ID } } if (isEmpty(meterNumber)) { resultRows.push(new GQLError(ERRORS.INVALID_METER_NUMBER, context)) continue } const dateValidationError = getDateStrValidationError(context, locale, reading) if (dateValidationError) { resultRows.push(dateValidationError) continue } - const property = properties.find((p) => p.addressKey === addressKey) + const property = propertiesByAddressKey.get(addressKey)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/condo/domains/meter/schema/RegisterPropertyMetersReadingsService.js` around lines 163 - 164, Replace the repeated properties.find() lookup in the readings loop with a Map keyed by addressKey, constructing it once before the loop and retrieving properties via map.get(addressKey). Remove the temporary console.log while updating the lookup logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/condo/domains/meter/schema/RegisterPropertyMetersReadingsService.js`:
- Line 164: Remove the debug console.log statement inside the readings loop in
RegisterPropertyMetersReadingsService, ensuring no property, address, reading,
or resolvedAddresses data is emitted through unstructured logging; retain the
surrounding reading-resolution logic unchanged.
In
`@apps/condo/domains/meter/utils/serverSchema/resolvePropertyMeterAddressesForOrganization.js`:
- Line 66: Remove the debug console.log from
resolvePropertyMeterAddressesForOrganization before the merge; do not replace it
with unstructured logging, and use the project’s structured Pino logger only if
diagnostic output is required, avoiding sensitive address or property data.
---
Nitpick comments:
In `@apps/condo/domains/meter/schema/RegisterPropertyMetersReadingsService.js`:
- Around line 163-164: Replace the repeated properties.find() lookup in the
readings loop with a Map keyed by addressKey, constructing it once before the
loop and retrieving properties via map.get(addressKey). Remove the temporary
console.log while updating the lookup logic.
In
`@apps/condo/domains/meter/utils/serverSchema/resolvePropertyMeterAddressesForOrganization.spec.js`:
- Around line 11-97: Add a test in the
resolvePropertyMeterAddressesForOrganization suite covering bulkSearch returning
empty map and addresses results. Assert propertyAddress equals { error:
ERRORS.ADDRESS_NOT_RECOGNIZED_VALUE }, properties is {}, and find is not called
when the address cannot be recognized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32de843c-f64d-45cb-847b-7f529a8e5a4b
📒 Files selected for processing (3)
apps/condo/domains/meter/schema/RegisterPropertyMetersReadingsService.jsapps/condo/domains/meter/utils/serverSchema/resolvePropertyMeterAddressesForOrganization.jsapps/condo/domains/meter/utils/serverSchema/resolvePropertyMeterAddressesForOrganization.spec.js
|
3 participants
Summary by CodeRabbit