feat(condo): DOMA-13022 add subscription field into FindOrganizationsByAddress#7315
feat(condo): DOMA-13022 add subscription field into FindOrganizationsByAddress#7315Alexander-Turkin wants to merge 5 commits intomainfrom
Conversation
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds organization subscription data to resident-domain lookups by extending GraphQL selection and types, resolving subscription via imported subscription field in the service, attaching subscription to finder return objects, and adding tests for subscription endAt propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant GQL as GraphQL Resolver
participant Service as FindOrganizationsByAddressService
participant Finder as Finder Utilities
participant SubField as OrganizationSubscriptionFieldResolver
participant DB as Database
Client->>GQL: query FIND_ORGANIZATIONS_BY_ADDRESS
GQL->>Service: handle request
Service->>Finder: fetch organizations (address key variants)
Finder->>DB: read organization records
DB-->>Finder: organization records (include subscription id)
Finder-->>Service: organizations (+subscription placeholder)
Service->>SubField: resolve ORGANIZATION_SUBSCRIPTION_FIELD per org
SubField->>DB: read subscription contexts/plans
DB-->>SubField: subscription data (endAt, apps, contextId)
SubField-->>Service: resolved subscription objects
Service-->>GQL: enriched organizations with subscription
GQL-->>Client: response with organizations.subscription
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: Ping-pong health check failed 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: dbdd4722ac
ℹ️ 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".
| const organizationsWithSubscription = await Promise.all( | ||
| organizations.map(async (organization) => { | ||
| const subscription = await ORGANIZATION_SUBSCRIPTION_FIELD.resolver(organization, {}, context) | ||
| return { ...organization, subscription } |
There was a problem hiding this comment.
This eagerly calls ORGANIZATION_SUBSCRIPTION_FIELD.resolver for every candidate organization before fetchOrganizationData applies its final filtering, so organizations that are later dropped (for example in the tin && accountNumber branch) still incur full subscription computation. That resolver performs several DB reads (SubscriptionPlan/SubscriptionContext lookups and app-expiration plan loading), so this introduces avoidable N+1 query load and latency for high-density addresses; it should be moved after final eligibility is known (or batched/cached) to avoid production slowdowns.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/condo/domains/resident/schema/FindOrganizationsByAddressService.test.js (3)
947-947: Test name is misleading relative to its assertions.The test title says “null subscription”, but it asserts a defined
subscriptionobject with null fields. Consider renaming for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/resident/schema/FindOrganizationsByAddressService.test.js` at line 947, Rename the test title string for the test starting with test('Should return organization with null subscription when no contexts exist' so it accurately describes the assertion (the response includes a defined subscription object whose fields are null, not a null subscription). For example update the title to "Should return organization with subscription object whose fields are null when no contexts exist" (or similar) in the test declaration to match the asserted behavior in the test.
905-968: Add a test case for multiple active subscription contexts.Current coverage checks only single-context and no-context scenarios. Please add a case with two active paid contexts for the same organization to lock in expected merge/selection behavior in this API response.
Based on learnings: In apps/condo/domains/subscription/schema/ActivateSubscriptionPlanService.js, organizations can have multiple active paid subscription contexts at the same time for the same or different subscription plans. There should be no guard preventing duplicate active paid subscriptions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/resident/schema/FindOrganizationsByAddressService.test.js` around lines 905 - 968, Add a new test in the same suite that creates two active paid subscription contexts for the same organization (use createTestSubscriptionPlan and createTestSubscriptionContext with isTrial: false and different endAt values), call findOrganizationsByAddressByTestClient and locate the org by utils.organization.id; then assert that found.subscription is defined, found.subscription.activeSubscriptionContextId is one of the created context IDs and found.subscription.activeSubscriptionEndAt equals the later endAt (max of the two), and other per-feature endAt fields reflect the expected merged/selected values; reference TestUtils, createTestSubscriptionPlan, createTestSubscriptionContext, findOrganizationsByAddressByTestClient and utils.organization when adding the test.
935-944: Assert the full subscription contract that this endpoint now exposes.The new query/service shape includes additional fields (e.g.,
customizationEndAt,propertiesEndAt,analyticsEndAt,b2bApps,b2cApps) that are not asserted yet. Adding explicit checks here will better protect the API contract from regressions.Also applies to: 964-967
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/resident/schema/FindOrganizationsByAddressService.test.js` around lines 935 - 944, Update the test assertions in FindOrganizationsByAddressService.test.js to cover the full subscription contract returned by the service: add explicit expects on found.subscription for customizationEndAt, propertiesEndAt, analyticsEndAt, b2bApps and b2cApps (and mirror the same checks at the other assertion block referenced around lines 964-967); assert each new field against the expected value (e.g., endAt or null as appropriate for the test scenario) and ensure activeSubscriptionContextId/activeSubscriptionEndAt and existing fields remain asserted.apps/condo/domains/resident/schema/FindOrganizationsByAddressService.js (1)
114-119: Consider avoiding eager per-organization subscription resolution.This adds extra async work for every organization before final response mapping. If this endpoint can return many organizations, consider batching/caching subscription resolution (or resolving lazily only when requested) to reduce query pressure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/domains/resident/schema/FindOrganizationsByAddressService.js` around lines 114 - 119, The current Promise.all over organizations calling ORGANIZATION_SUBSCRIPTION_FIELD.resolver for each organization (in organizationsWithSubscription) eagerly issues a resolver per org and can create high query load; replace this with a batched or cached approach: collect organization IDs from organizations, call a single batch fetch (or use a DataLoader from context) to retrieve all subscriptions in one query (e.g., getSubscriptionsForOrganizationIds or context.loaders.organizationSubscription.loadMany), then map the returned subscriptions back to organizations to produce the same { ...organization, subscription } shape; alternatively resolve subscription lazily only when requested by downstream resolvers rather than pre-resolving in organizationsWithSubscription.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/condo/domains/resident/schema/FindOrganizationsByAddressService.js`:
- Around line 114-119: The current Promise.all over organizations calling
ORGANIZATION_SUBSCRIPTION_FIELD.resolver for each organization (in
organizationsWithSubscription) eagerly issues a resolver per org and can create
high query load; replace this with a batched or cached approach: collect
organization IDs from organizations, call a single batch fetch (or use a
DataLoader from context) to retrieve all subscriptions in one query (e.g.,
getSubscriptionsForOrganizationIds or
context.loaders.organizationSubscription.loadMany), then map the returned
subscriptions back to organizations to produce the same { ...organization,
subscription } shape; alternatively resolve subscription lazily only when
requested by downstream resolvers rather than pre-resolving in
organizationsWithSubscription.
In
`@apps/condo/domains/resident/schema/FindOrganizationsByAddressService.test.js`:
- Line 947: Rename the test title string for the test starting with test('Should
return organization with null subscription when no contexts exist' so it
accurately describes the assertion (the response includes a defined subscription
object whose fields are null, not a null subscription). For example update the
title to "Should return organization with subscription object whose fields are
null when no contexts exist" (or similar) in the test declaration to match the
asserted behavior in the test.
- Around line 905-968: Add a new test in the same suite that creates two active
paid subscription contexts for the same organization (use
createTestSubscriptionPlan and createTestSubscriptionContext with isTrial: false
and different endAt values), call findOrganizationsByAddressByTestClient and
locate the org by utils.organization.id; then assert that found.subscription is
defined, found.subscription.activeSubscriptionContextId is one of the created
context IDs and found.subscription.activeSubscriptionEndAt equals the later
endAt (max of the two), and other per-feature endAt fields reflect the expected
merged/selected values; reference TestUtils, createTestSubscriptionPlan,
createTestSubscriptionContext, findOrganizationsByAddressByTestClient and
utils.organization when adding the test.
- Around line 935-944: Update the test assertions in
FindOrganizationsByAddressService.test.js to cover the full subscription
contract returned by the service: add explicit expects on found.subscription for
customizationEndAt, propertiesEndAt, analyticsEndAt, b2bApps and b2cApps (and
mirror the same checks at the other assertion block referenced around lines
964-967); assert each new field against the expected value (e.g., endAt or null
as appropriate for the test scenario) and ensure
activeSubscriptionContextId/activeSubscriptionEndAt and existing fields remain
asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bd873b5-a5e5-4644-a83f-60e1a46464e8
📒 Files selected for processing (4)
apps/condo/domains/resident/gql.jsapps/condo/domains/resident/schema/FindOrganizationsByAddressService.jsapps/condo/domains/resident/schema/FindOrganizationsByAddressService.test.jsapps/condo/domains/resident/utils/serverSchema/findOrganizationsByAddress.js
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/condo/schema.ts (1)
29344-29344: Tightensubscriptionnullability to match resolver behaviorAt Line 29344,
subscription?: Maybe<OrganizationSubscriptionFeatures>is wider than runtime behavior: the resolver path shown inapps/condo/domains/organization/schema/fields/subscription.js:262-326and:51-77always returns a subscription object. Consider making this field non-null in GraphQL (OrganizationSubscriptionFeatures!) and regenerating this type so clients don’t carry unnecessary null checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/condo/schema.ts` at line 29344, The GraphQL schema's Organization type declares subscription?: Maybe<OrganizationSubscriptionFeatures> but the resolver for the subscription field always returns a non-null object; change the schema so the subscription field is non-null (OrganizationSubscriptionFeatures!) and regenerate the GraphQL types; update the declaration where "subscription" is defined to remove the Maybe/optional, ensure the server schema and generated client types are in sync, and run the codegen/regeneration step that produces the updated types so clients no longer treat subscription as nullable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/condo/schema.ts`:
- Line 29344: The GraphQL schema's Organization type declares subscription?:
Maybe<OrganizationSubscriptionFeatures> but the resolver for the subscription
field always returns a non-null object; change the schema so the subscription
field is non-null (OrganizationSubscriptionFeatures!) and regenerate the GraphQL
types; update the declaration where "subscription" is defined to remove the
Maybe/optional, ensure the server schema and generated client types are in sync,
and run the codegen/regeneration step that produces the updated types so clients
no longer treat subscription as nullable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b81f5854-9794-46b9-9be9-63647579a7cc
📒 Files selected for processing (2)
apps/condo/schema.graphqlapps/condo/schema.ts
| Here, we should have all the necessary data. | ||
| */ | ||
|
|
||
| const organizationsWithSubscription = await Promise.all( |
There was a problem hiding this comment.
You should at least add a commentary why do you resolve the field manually
There was a problem hiding this comment.
We can't get a virtual field with find()
| } | ||
|
|
||
| const result = await Promise.all(organizations.map(fetchOrganizationData)) | ||
| const result = await Promise.all(organizationsWithSubscription.map(fetchOrganizationData)) |
There was a problem hiding this comment.
It is a bit stragne that getting subscription is done not in a similar way
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
2 participants
Summary by CodeRabbit
New Features
Tests