implemented property management - #316
Open
henry-casper wants to merge 21 commits into
Open
Conversation
Co-authored-by: Copilot <[email protected]>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @henry-casper, your pull request is larger than the review limit of 150000 diff characters
Contributor
Reviewer's GuideImplements end-to-end property management capabilities across API, persistence, GraphQL, admin UI, and verification tests, including permissions, soft-delete semantics, and test-time event handler wiring. Sequence diagram for soft-deleted property removal via GraphQLsequenceDiagram
actor AdminUser
participant AdminUI as AdminUI_Properties
participant GraphQL as GraphQL_Server
participant Resolvers as PropertyResolvers
participant Service as PropertyApplicationService
participant Repo as PropertyRepository
participant DB as MongoDB
AdminUser->>AdminUI: click RemoveProperty
AdminUI->>GraphQL: propertyDelete(input.id)
GraphQL->>Resolvers: Mutation.propertyDelete
Resolvers->>Service: requestDelete({ id })
Service->>Repo: getById(id)
Repo->>DB: findById(id).populate(['community','owner'])
Repo-->>Service: Property aggregate
Service-->>Repo: aggregate.requestDelete()
Repo->>DB: save({ isDeleted: true })
Repo-->>Service: deleted aggregate
Service-->>Resolvers: PropertyMutationResult{ status.success }
Resolvers-->>GraphQL: propertyDelete payload
GraphQL-->>AdminUI: success, property removed from list
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds end-to-end property management across the domain, persistence, GraphQL API, community-admin UI, and verification suites.
Changes:
- Adds property CRUD, permissions, role resolution, and soft deletion.
- Adds guarded admin property list/create/detail pages.
- Adds extensive Storybook, acceptance, and E2E coverage plus dependency security overrides.
Reviewed changes
Copilot reviewed 128 out of 129 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-workspace.yaml |
Updates security overrides and audit exceptions. |
packages/ocom/ui-community-route-admin/src/section-layout.graphql |
Queries property-management permission. |
packages/ocom/ui-community-route-admin/src/pages/properties.tsx |
Adds property routes. |
packages/ocom/ui-community-route-admin/src/pages/properties.stories.tsx |
Tests guarded property page states. |
packages/ocom/ui-community-route-admin/src/pages/properties-list.tsx |
Adds property-list page layout. |
packages/ocom/ui-community-route-admin/src/pages/properties-list.stories.tsx |
Covers list page states. |
packages/ocom/ui-community-route-admin/src/pages/properties-detail.tsx |
Adds property-detail page. |
packages/ocom/ui-community-route-admin/src/pages/properties-detail.stories.tsx |
Covers detail page states. |
packages/ocom/ui-community-route-admin/src/pages/properties-create.tsx |
Adds property-create page. |
packages/ocom/ui-community-route-admin/src/pages/properties-create.stories.tsx |
Covers create page rendering. |
packages/ocom/ui-community-route-admin/src/index.tsx |
Registers property menu and route. |
packages/ocom/ui-community-route-admin/src/components/properties-route-guard.container.tsx |
Enforces route permission. |
packages/ocom/ui-community-route-admin/src/components/properties-route-guard.container.stories.tsx |
Covers guard outcomes. |
packages/ocom/ui-community-route-admin/src/components/properties-list.tsx |
Renders the property table. |
packages/ocom/ui-community-route-admin/src/components/properties-list.stories.tsx |
Covers property-table states. |
packages/ocom/ui-community-route-admin/src/components/properties-list.container.tsx |
Loads and navigates properties. |
packages/ocom/ui-community-route-admin/src/components/properties-list.container.stories.tsx |
Covers list-container behavior. |
packages/ocom/ui-community-route-admin/src/components/properties-list.container.graphql |
Defines property-list query. |
packages/ocom/ui-community-route-admin/src/components/properties-detail.tsx |
Adds edit and removal form. |
packages/ocom/ui-community-route-admin/src/components/properties-detail.stories.tsx |
Covers detail interactions. |
packages/ocom/ui-community-route-admin/src/components/properties-detail.container.tsx |
Handles update and deletion. |
packages/ocom/ui-community-route-admin/src/components/properties-detail.container.stories.tsx |
Covers detail-container flows. |
packages/ocom/ui-community-route-admin/src/components/properties-detail.container.graphql |
Defines detail CRUD operations. |
packages/ocom/ui-community-route-admin/src/components/properties-create.tsx |
Adds property creation form. |
packages/ocom/ui-community-route-admin/src/components/properties-create.stories.tsx |
Covers create-form validation. |
packages/ocom/ui-community-route-admin/src/components/properties-create.container.tsx |
Handles property creation. |
packages/ocom/ui-community-route-admin/src/components/properties-create.container.stories.tsx |
Covers creation outcomes. |
packages/ocom/ui-community-route-admin/src/components/properties-create.container.graphql |
Defines create mutation. |
packages/ocom/persistence/src/datasources/readonly/property/property/property.read-repository.ts |
Adds filtered property reads. |
packages/ocom/persistence/src/datasources/readonly/property/property/property.read-repository.test.ts |
Tests read filtering and population. |
packages/ocom/persistence/src/datasources/readonly/property/property/property.data.ts |
Defines property data source. |
packages/ocom/persistence/src/datasources/readonly/property/property/index.ts |
Exposes property repository. |
packages/ocom/persistence/src/datasources/readonly/property/index.ts |
Builds property read context. |
packages/ocom/persistence/src/datasources/readonly/index.ts |
Registers property read context. |
packages/ocom/persistence/src/datasources/domain/property/property/property.repository.ts |
Adds population and soft-delete saving. |
packages/ocom/persistence/src/datasources/domain/property/property/property.repository.soft-delete.test.ts |
Tests soft-delete persistence. |
packages/ocom/graphql/src/schema/types/property.resolvers.ts |
Adds property query and mutation resolvers. |
packages/ocom/graphql/src/schema/types/property.graphql |
Defines property GraphQL API. |
packages/ocom/graphql/src/schema/types/member.resolvers.ts |
Adds role lookup fallback. |
packages/ocom/graphql/src/schema/types/member.resolvers.additional.test.ts |
Updates role resolver coverage. |
packages/ocom/graphql/src/schema/types/end-user-role.graphql |
Exposes property permissions. |
packages/ocom/domain/src/domain/contexts/property/property/index.ts |
Exports property domain types. |
packages/ocom/data-sources-mongoose-models/src/models/property/property.model.ts |
Adds deletion flag and location changes. |
packages/ocom/application-services/src/index.ts |
Registers property services. |
packages/ocom/application-services/src/contexts/property/property/update.ts |
Implements property updates. |
packages/ocom/application-services/src/contexts/property/property/request-delete.ts |
Implements deletion requests. |
packages/ocom/application-services/src/contexts/property/property/query-by-id.ts |
Adds property lookup. |
packages/ocom/application-services/src/contexts/property/property/query-by-community-id.ts |
Adds community property lookup. |
packages/ocom/application-services/src/contexts/property/property/index.ts |
Composes property operations. |
packages/ocom/application-services/src/contexts/property/property/create.ts |
Implements property creation. |
packages/ocom/application-services/src/contexts/property/index.ts |
Builds property service context. |
packages/ocom/application-services/src/contexts/community/member/query-by-id-with-role.ts |
Adds populated member lookup. |
packages/ocom/application-services/src/contexts/community/member/index.ts |
Registers member-role lookup. |
packages/ocom-verification/verification-shared/src/scenarios/property/property-management.feature |
Specifies property CRUD behavior. |
packages/ocom-verification/verification-shared/src/scenarios/property/property-authorization.feature |
Specifies authorization behavior. |
packages/ocom-verification/verification-shared/src/pages/property-form.page.ts |
Adds shared property-form page object. |
packages/ocom-verification/verification-shared/src/pages/properties-list.page.ts |
Adds shared property-list page object. |
packages/ocom-verification/verification-shared/src/pages/index.ts |
Exports property page objects. |
packages/ocom-verification/e2e-tests/src/step-definitions/index.ts |
Registers property E2E steps. |
packages/ocom-verification/e2e-tests/src/shared/support/graphql-response.ts |
Adds GraphQL response helpers. |
packages/ocom-verification/e2e-tests/src/contexts/property/tasks/view-property-details.ts |
Adds detail-view task. |
packages/ocom-verification/e2e-tests/src/contexts/property/tasks/view-properties-list.ts |
Adds list-view task. |
packages/ocom-verification/e2e-tests/src/contexts/property/tasks/update-property.ts |
Adds update task. |
packages/ocom-verification/e2e-tests/src/contexts/property/tasks/ensure-property-exists.ts |
Adds conditional creation task. |
packages/ocom-verification/e2e-tests/src/contexts/property/tasks/delete-property.ts |
Adds removal task. |
packages/ocom-verification/e2e-tests/src/contexts/property/tasks/create-property.ts |
Adds creation task. |
packages/ocom-verification/e2e-tests/src/contexts/property/tasks/become-property-manager.ts |
Provisions E2E property managers. |
packages/ocom-verification/e2e-tests/src/contexts/property/step-definitions/index.ts |
Loads property steps. |
packages/ocom-verification/e2e-tests/src/contexts/property/questions/property-screen.ts |
Adds property-screen assertions. |
packages/ocom-verification/e2e-tests/src/contexts/property/notes/property-notes.ts |
Defines E2E property state. |
packages/ocom-verification/e2e-tests/src/contexts/property/interactions/submit-property-save.ts |
Captures update outcomes. |
packages/ocom-verification/e2e-tests/src/contexts/property/interactions/submit-property-create.ts |
Captures creation outcomes. |
packages/ocom-verification/e2e-tests/src/contexts/property/interactions/record-property-notes.ts |
Records list baselines. |
packages/ocom-verification/e2e-tests/src/contexts/property/interactions/open-property-detail.ts |
Opens property details. |
packages/ocom-verification/e2e-tests/src/contexts/property/interactions/open-properties-list.ts |
Opens property lists. |
packages/ocom-verification/e2e-tests/src/contexts/property/interactions/open-create-property-form.ts |
Opens creation form. |
packages/ocom-verification/e2e-tests/src/contexts/property/interactions/open-admin-portal.ts |
Opens provisioned admin portal. |
packages/ocom-verification/e2e-tests/src/contexts/property/interactions/fill-property-form.ts |
Fills property forms. |
packages/ocom-verification/e2e-tests/src/contexts/property/interactions/confirm-property-removal.ts |
Confirms property deletion. |
packages/ocom-verification/e2e-tests/src/contexts/property/abilities/admin-portal-page.ts |
Adds property navigation helpers. |
packages/ocom-verification/acceptance-ui/tsconfig.json |
Includes admin route sources. |
packages/ocom-verification/acceptance-ui/src/step-definitions/index.ts |
Registers property UI steps. |
packages/ocom-verification/acceptance-ui/src/contexts/property/tasks/properties-screen.ts |
Renders property acceptance screens. |
packages/ocom-verification/acceptance-ui/src/contexts/property/tasks/manage-property.ts |
Implements UI CRUD tasks. |
packages/ocom-verification/acceptance-ui/src/contexts/property/step-definitions/index.ts |
Loads property UI steps. |
packages/ocom-verification/acceptance-ui/src/contexts/property/questions/property-screen.ts |
Adds UI screen assertions. |
packages/ocom-verification/acceptance-ui/src/contexts/property/questions/property-outcome.ts |
Adds mocked outcome questions. |
packages/ocom-verification/acceptance-ui/src/contexts/property/notes/property-ui-notes.ts |
Defines UI scenario state. |
packages/ocom-verification/acceptance-api/src/world.ts |
Registers property API abilities. |
packages/ocom-verification/acceptance-api/src/step-definitions/index.ts |
Registers property API steps. |
packages/ocom-verification/acceptance-api/src/shared/graphql/property-operations.ts |
Defines verification GraphQL operations. |
packages/ocom-verification/acceptance-api/src/shared/abilities/update-property.ts |
Adds update ability. |
packages/ocom-verification/acceptance-api/src/shared/abilities/provision-resident-member.ts |
Provisions unauthorized residents. |
packages/ocom-verification/acceptance-api/src/shared/abilities/index.ts |
Exports property abilities. |
packages/ocom-verification/acceptance-api/src/shared/abilities/graphql-client.ts |
Adds principal context headers. |
packages/ocom-verification/acceptance-api/src/shared/abilities/delete-property.ts |
Adds deletion ability. |
packages/ocom-verification/acceptance-api/src/shared/abilities/create-property.ts |
Adds creation ability. |
packages/ocom-verification/acceptance-api/src/shared/abilities/actor-auth.ts |
Tracks end-user tokens and context. |
packages/ocom-verification/acceptance-api/src/servers/api-graphql-test-server.ts |
Passes test principal context. |
packages/ocom-verification/acceptance-api/src/mock-application-services.ts |
Registers handlers and end-user validation. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/view-property-details.ts |
Adds API detail-view task. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/view-properties-list.ts |
Adds API list-view task. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/update-property.ts |
Adds API update task. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/update-property-input.ts |
Maps update inputs. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/provision-resident-member.ts |
Arranges resident actors. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/delete-property.ts |
Adds API deletion task. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/create-property.ts |
Adds API creation task. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/become-property-manager.ts |
Arranges property managers. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/attempt-update-property.ts |
Captures rejected updates. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/attempt-delete-property.ts |
Captures rejected deletions. |
packages/ocom-verification/acceptance-api/src/contexts/property/tasks/attempt-create-property.ts |
Captures rejected creations. |
packages/ocom-verification/acceptance-api/src/contexts/property/step-definitions/index.ts |
Loads property API steps. |
packages/ocom-verification/acceptance-api/src/contexts/property/questions/viewed-property.ts |
Reads viewed property data. |
packages/ocom-verification/acceptance-api/src/contexts/property/questions/property-retrievable.ts |
Checks post-deletion retrieval. |
packages/ocom-verification/acceptance-api/src/contexts/property/questions/property-operation-outcome.ts |
Reads operation outcomes. |
packages/ocom-verification/acceptance-api/src/contexts/property/questions/property-named.ts |
Finds properties by name. |
packages/ocom-verification/acceptance-api/src/contexts/property/questions/property-manager-permission.ts |
Verifies role permission. |
packages/ocom-verification/acceptance-api/src/contexts/property/questions/property-field.ts |
Reads property fields. |
packages/ocom-verification/acceptance-api/src/contexts/property/questions/properties-list.ts |
Queries community properties. |
packages/ocom-verification/acceptance-api/src/contexts/property/notes/property-notes.ts |
Defines API scenario state. |
packages/ocom-verification/acceptance-api/package.json |
Adds verification dependencies. |
codegen.yml |
Maps GraphQL Property to domain type. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…scade npm latest (4.13.2, also 4.13.1) point at CDN artifacts that 404 (Azure.Functions.Cli.linux-x64.<version>.zip missing), breaking the unpinned global install. 4.13.0 is the newest release with a working artifact (verified via ranged GET -> HTTP 206). Also add succeeded() to the func-tools/Playwright install conditions and replace always() on the Playwright verify step, so a failed install no longer cascades into misleading 'pnpm: command not found' errors. Co-authored-by: Copilot <[email protected]>
…unique name index to active properties
- getById now treats soft-deleted properties as not found, preventing
update/delete mutations against hidden records (PR review P1)
- getAll filters out soft-deleted documents
- unique {community, propertyName} index is now partial on
{isDeleted: false} so deleted property names can be reused (PR review P2)
- added compensating {community, isDeleted} index for listing queries
- covered by repository unit tests, index contract tests, and two new
acceptance-api scenarios
Co-authored-by: Copilot <[email protected]>
…tibility
Reverts the partial {isDeleted: false} unique index and the compensating
{community, isDeleted} index so the PR requires no manual index migration
on deployed databases (createIndex with changed options would conflict
with the existing index). Deleted property names remain reserved.
Keeps the P1 fix: soft-deleted properties are still excluded from the
write repository (getById/getAll), so they cannot be mutated.
Co-authored-by: Copilot <[email protected]>
- Scope property reads to community members: property/propertiesByCommunityId now verify the actor's membership in the target community (Unauthorized otherwise) - Require canManageProperties for admin property updates via a public assertCanManageProperties guard on the Property aggregate - Forward explicit nulls for bedrooms/bathrooms/squareFeet so numeric listing details can be cleared end to end (UI container, resolver, command) - Evict deleted properties from the Apollo cache after propertyDelete - Resolve Property.owner through the member read model so nested account fields are GraphQL-safe - Pin func-tools CI cache to exact version key; inexact hits no longer skip installation of the pinned Core Tools version - Drain in-flight integration event handlers before per-scenario DB reset and skip the mock server dev seed under tests (SKIP_DEV_SEED) to stop acceptance cross-scenario contamination - Note: member navigation finding was a false positive (MemberReadRepo.isAdmin already includes canManageProperties) Co-authored-by: Copilot <[email protected]>
The spec mandates all admin-side property queries enforce propertyPermissions.canManageProperties. The previous fix only verified community membership, letting residents without the permission read the property directory. Reads now load the acting member's role and require canManageProperties in the target community; the contradictory resident-can-view scenario is replaced with rejection scenarios for both list and details. Co-authored-by: Copilot <[email protected]>
…ry application services Property read authorization now lives in the application services and is bound to the request's current member/community context: - Expose the request-scoped passport on DataSources so application services can evaluate domain visas on read operations. - Guard Property queryById/queryByCommunityId with the property visa (canManageProperties or system account). The member passport is built from the request's x-member-id/x-community-id hints and the MemberPropertyVisa denies cross-community roots, so a manager acting under a different community context is rejected even if they hold manage permissions elsewhere. - Drop the resolver-level membership lookup that authorized via any membership matching the requested community; resolvers now only require a verified user and delegate authorization to the services. - New acceptance scenarios: a manager who switches communities can no longer view their original community's list or property details. Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 185 out of 186 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (3)
packages/ocom/graphql/src/schema/types/property.resolvers.ts:218
- An explicit
nullfortagsis currently ignored, but the new form sendstags: nullwhen the field is cleared. Updating a property after removing all tags therefore leaves the old tags persisted. Convertnullto an empty array while preservingundefinedas “unchanged.”
packages/ocom/ui-community-route-admin/src/components/properties-create.container.tsx:25 - The mutation loading state is discarded, so the Create Property button remains enabled while a request is in flight. A double click can submit two create mutations and produce duplicate/error feedback. Pass the mutation's loading state through
PropertiesCreatetoPropertyForm.submittingso the button displays loading and blocks repeat submission.
packages/ocom/ui-community-route-admin/src/components/format-display-address.ts:29 countryis accepted and queried for list rows but is never included in the formatted address. International addresses can therefore render identically despite different countries, and a country-only address incorrectly renders asN/A. Include the trimmed country as the final address part and update the tests accordingly.
Backend:
- Scope the unique { community, propertyName } index to active documents
(partialFilterExpression on isDeleted) so a removed property's name can
be reused; seed docs set isDeleted explicitly since raw bulkWrite
upserts bypass schema defaults
- Friendly duplicate-name handling: PropertyReadRepo.isPropertyNameTaken
pre-checks in create (always) and update (rename only) app services,
with E11000 duplicate-key mapping in PropertyMutationResolver as the
race backstop; message: "A property with this name already exists"
- Align mongoose maxlengths with domain VOs (bedDescriptions and
additional-amenity items 40->100, floorPlan 2000->2048)
Frontend (shared admin PropertyForm):
- Clearing comma-list fields submits [] so stored lists are cleared
(tags, amenities, images, floorPlanImages, bedDescriptions,
additional-amenity amenities)
- Country/State become dropdowns (United States; 50 states + DC labeled
by full name storing 2-letter codes), searchable and clearable
- Number-field UX: $ prefix and 2-decimal precision on money fields,
months/sq ft suffixes on lease/lot size, spinner controls only on
max guests/bedrooms/bathrooms
- Inline validation mirrors domain VOs (email regex, integer ranges,
per-item comma-list lengths, string maxlengths) with
scroll-to-first-error on submit
- Details page gains Save & Close (returns to list only after a
confirmed save); create button shows loading and blocks double submit
- Storybook coverage for dropdowns, adornments, validation, Save & Close,
and submitting states
Staff-role enforcement (fixes verification suite reds):
- staffRoleUpdate and staffUserAssignRole enforce enterprise-app-role
permissions per the staff-user feature scenarios
Verification: scenario sample values use month-scale lease terms and the
canonical "United States" country; new Serenity scenarios and page-object
support across acceptance-api (70), acceptance-ui (56), and e2e (47).
Deps: bump transitive browserslist to 4.28.8 (Snyk SNYK-JS-BROWSERSLIST-
18854715 / 18856271).
Ops note: one-time dropIndex of the old { community, propertyName }
unique index on the dev Cosmos properties collection; the partial index
builds on next boot.
Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 199 out of 200 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/ocom/ui-community-route-admin/src/components/property-form.validation.ts:19
- Tags are also capped at 50 by
Property.normalizeTags, but the new form callscommaListRule('tag', 100)without a total-item limit. Submitting 51+ tags therefore passes validation and reports success while the aggregate silently truncates the list. ApplymaxItems: 50to the tags rule so users get an inline error instead of data loss.
- authorize community before any lookups in property create to prevent name/ID probing (queryById distinct responses are spec-mandated; update/delete already authorized first) - default missing bedroomDetails/additionalAmenities to empty arrays in listing-detail adapter so sparse lean reads cannot crash MongoosePropArray.map - fix TS2339 in acceptance-ui property-screen questions via Actor cast; re-export PropertyMutationInput and annotate LastPropertyUpdateInput to fix TS4023 - property route guard now validates both memberId and communityId; added OtherCommunityRouteDenied story - new probe scenario in property-authorization.feature, adapter feature steps, unauthorized-create unit test Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 199 out of 200 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
packages/ocom/ui-community-route-admin/src/components/property-form.tsx:569
- This validator only checks the 0.5 increment, so values such as
1000.5pass client validation even though the domainBathroomsvalue object rejects anything above 1000. Add the domain's 0–1000 range check here so invalid values are reported inline rather than failing after submission.
…d bathrooms inline - fall back to the guest passport when member/community principal hints mismatch so the admin route guard renders its 403 page instead of a request-wide error (unit tests added) - cap tags at the aggregate's 50-entry limit inline (commaListRule maxItems) so saves cannot silently drop tags; new @ui-only scenario - mirror the domain Bathrooms range (0-1000, 0.5 steps) inline via halfStepRangeRule; new @ui-only scenario - reviewed but unchanged: staff-role permission-flag forwarding is pre-existing main design out of PR scope; the partial unique index needs no migration per requestedchanges.md (pre-release, empty collections, documented dropIndex ops step) Co-authored-by: Copilot <[email protected]>
… gate staff-role update by target tier - domain: Property.normalizeTags now throws 'At most 50 tag entries are allowed' instead of silently slicing; message matches the admin UI rule so direct GraphQL callers get an explicit error - application-services: replace broad forMember try/catch with explicit membership pre-validation (member belongs to end user and to community); known mismatches fall back to guest, unexpected errors propagate - graphql: staffRoleUpdate now loads the target role and gates on its persisted enterpriseAppRole as well as the requested one; blank enterpriseAppRole values are rejected, closing the tier-gate bypass - domain: remove two stale duplicate scenarios from staff-role.feature that had no test implementation (pre-existing vitest-cucumber pairing failure, latent behind the turbo cache) - acceptance: new @api-only scenarios for 51-tag rejection and staff-role update gates; UpdateProperty ability/steps now support tags Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 204 out of 205 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/ocom/data-sources-mongoose-models/src/models/property/property.model.ts:214
- The partial index alone does not make soft deletion safe for deployed data. Existing property documents have no
isDeletedfield, so they are excluded from this{ isDeleted: false }index; and an already-deployed non-partial unique index is not replaced merely by changing the Mongoose schema. This can either keep blocking name reuse or allow duplicate active names after the index is replaced. Add a deployment migration that backfillsisDeleted: falseand explicitly replaces the old index before relying on this constraint.
packages/ocom/graphql/src/schema/types/property.graphql:166 - This new list API is unbounded: the repository materializes every property (including nested listing/media data) and the UI only paginates after receiving the full result. Community growth will therefore increase query latency and server/client memory even when the user views ten rows. Expose server-side pagination (for example
first/afteror page/limit) and apply the limit in the repository.
…n cross-community update test - graphql: propertyCreate/propertyUpdate return the committed entity directly (matching the community/member mutation pattern) instead of chaining a post-commit queryById whose read failures were reported as mutation failures, prompting clients to retry committed writes into duplicate-name errors - persistence: PropertyRepository.save populates refs still stored as raw ObjectIds (e.g. a reassigned owner) so the committed aggregate serializes owner/community in mutation payloads without a separate read - acceptance: post-success persistence probes in the create/update abilities now throw PropertyPostCommitProbeError; attempt tasks rethrow it so negative scenarios can never record a committed write as the expected rejection - acceptance: the cross-community update scenario returns the actor to their original membership and verifies the property name is unchanged via an authorized principal Declined (per requestedchanges.md): switching comma-separated fields to array-native controls — the comma-list pattern is the codified design for all five list fields Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 206 out of 207 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
packages/ocom/ui-community-route-admin/src/components/property-form.validation.ts:20
- This comment no longer matches the aggregate behavior:
tagsnow throwsPropertyTagsExceedsMaximumErrorwhen normalization leaves more than 50 tags; it does not silently truncate them. Keeping this as the stated source for frontend validation can lead future changes to preserve the wrong contract.
packages/ocom/ui-community-route-admin/src/components/property-form.tsx:267 - The submit intent is only reset after a successful mutation. If “Save & Close” fails client-side validation, Ant Design never calls
onFinish, so the ref remainssaveAndClose; after correcting the field, submitting with Enter invokesonSubmitAndCloseeven though the user did not choose that action again. Reset the intent when form validation fails.
…av access, XSS, cache, country cascade Backend: - staffUserAssignRole resolves the target via StaffRole.queryById and fails closed on missing/blank enterpriseAppRole; buildStaffRoleUpdateCommand rejects a blank persisted tier for non-TechAdmin callers - property(id:) returns null for unauthorized properties (deny by omission), closing the existence oracle between missing and foreign properties - Property read-repo getById filters isDeleted in the DB predicate and the domain repository overrides get() with the soft-delete-aware lookup - propertyUpdate treats tags: null as clearing the list, consistent with the other list fields Frontend: - UrlLinkPreview only linkifies http(s) URLs (javascript:/data: render as inert text) and anchors carry rel="noopener noreferrer" - Property detail query uses fetchPolicy network-only so cached entities cannot render under another community without re-authorization - Admin portal entry points (communities dropdown, accounts community list) now admit non-admin members whose role grants canManageProperties with an ACCEPTED account, via shared canAccessAdminPortal/hasAcceptedAccountForUser helpers in ui-shared; queries extended with accounts/role fields - Country/State cascade per requestedchanges.md: countries.json local asset (249 ISO countries; US and Canada carry states), country Select writes the stored name, state renders a Select for US/Canada (code value, name label) and a free-text input otherwise, and changing country clears the subdivision; unknown stored values render as-is Note: the CVHP source countries.json was unrecoverable (runtime-fetched public asset, absent from the sibling repo and its history), so the asset was regenerated to the exact shape the spec defines. Verification: page objects resolve the State field by control id (the "United States" option label contains "State"), field tables apply country before subdivision, and the select-both task picks country first. New scenarios: assign-role privilege gate (API), unknown property id not found (API), switching country clears state (UI). Acceptance-api 76, acceptance-ui 59, e2e 47 green; stories cover inert unsafe URLs, the cascade, and property-manager admin entries. Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 209 out of 221 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
packages/ocom/graphql/src/schema/types/member.resolvers.ts:111
- This fallback introduces an N+1 query for
membersForCurrentEndUser. That repository returns aggregation results with an unpopulated role ObjectId, and both newly expanded UI queries requestrolefor every member, so each field resolution callsqueryByIdWithRoleseparately (in addition to the existingisAdminlookup). Batch these resolutions per request with a DataLoader backed by the newqueryByIdsWithRolemethod.
packages/ocom/ui-community-route-admin/src/components/property-form.validation.ts:20 - This comment is now outdated:
Property.normalizeTagsno longer silently truncates after 50 entries; it throwsAt most 50 tag entries are allowed. Update the mirrored-domain documentation so future validation changes are based on the actual contract.
- Replace countries.json with a typed countries.ts module: the app build compiles admin sources under NodeNext, where JSON imports require an import attribute (TS1543 in CI); a TS asset compiles in every module mode. Revert the round-6 resolveJsonModule/json-include tsconfig additions that are no longer needed. - Treat explicit null bedroomDetails/additionalAmenities as a deliberate clear in the property mutations, mapping them to [] like tags, so a null no longer reports success while silently retaining rows. - Cap bedroom-detail and additional-amenity rows at 50 in the PropertyListingDetail entity, consistent with the tags limit, so unbounded row submissions cannot grow documents toward MongoDB's size limit; covered by feature-paired entity scenarios. - Stop the backend-derived isAdmin flag from bypassing the ACCEPTED- account requirement in canAccessAdminPortal: isAdmin treats canManageProperties itself as an admin permission, so members whose role grants property management now always need an ACCEPTED account for the current user before navigation offers the admin portal; admins through other permissions keep the legacy entry points. Unit tests plus stories mirroring the backend derivation pin the behavior. Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 209 out of 224 changed files in this pull request and generated 4 comments.
Suppressed comments (4)
packages/ocom/ui-community-route-accounts/src/components/community-list.stories.tsx:223
- This typename does not exist in the added schema: the nested type is
EndUserRolePropertyPermissions. Because these story args satisfy props containing a generated fragment type, the literalPropertyPermissionsis incompatible with generated types and can also make Apollo treat the mock as a different object type.
packages/ocom/graphql/src/schema/types/member.resolvers.ts:111 - For
membersForCurrentEndUser, roles arrive unpopulated from the aggregate repository, so this fallback executesqueryByIdWithRoleonce per member. The newly added dropdown/account operations requestrolefor every row, creating an N+1 query path despite the new batchedqueryByIdsWithRoleAPI. Resolve roles through a request-scoped DataLoader (and share it with other member-role consumers).
packages/ocom/graphql/src/schema/types/property.graphql:166 - This new collection field has no pagination or limit, and the repository loads every property before the UI applies client-side pagination. Community growth therefore increases database work, response size, and owner-resolution batches without bound. Add cursor pagination (or at minimum bounded
first/offsetarguments) and have the table request pages from the server.
apps/ui-community/.storybook/apollo-mocks.ts:66 - This mock typename does not match the schema added in this PR; the field's concrete type is
EndUserRolePropertyPermissions. Returning an impossible typename makes the Storybook response diverge from production and can prevent Apollo fragment/type matching on this object.
| @@ -135,6 +138,7 @@ overrides: | |||
| 'webpack-dev-server>http-proxy-middleware': 3.0.7 | |||
| joi: ^17.13.4 | |||
| brace-expansion@2: 2.0.3 | |||
| 'nanoid@<3.3.17': '>=3.3.17' | |||
Comment on lines
+20
to
+23
| const { data: membersData, loading: membersLoading } = useQuery(AdminMemberListContainerMembersDocument, { | ||
| variables: { communityId: props.data.communityId }, | ||
| skip: !props.data.communityId, | ||
| }); |
Comment on lines
+45
to
+48
| const { data: membersData, loading: membersLoading } = useQuery(AdminMemberListContainerMembersDocument, { | ||
| variables: { communityId: communityId ?? '' }, | ||
| skip: !communityId, | ||
| }); |
Comment on lines
+54
to
+58
| hasPermissions: (data: unknown) => { | ||
| const adminData = data as AdminMenuData; | ||
| const canManageProperties = adminData?.member?.role?.permissions?.propertyPermissions?.canManageProperties ?? false; | ||
| // Mirror the backend: property management also requires an ACCEPTED account for the current user. | ||
| return canManageProperties && hasAcceptedAccountForUser(adminData?.member?.accounts, adminData?.currentEndUserId); |
The acceptance-ui cucumber suites render application sources (ui-community-route-*, ui-staff-route-*, ui-staff-shared) via relative imports, and those sources import @ocom/ui-shared and @cellix/ui-core, whose package exports resolve to dist/. None of these packages were declared by @ocom-verification/acceptance-ui, so turbo's ^build never ordered their builds before test:coverage:acceptance. On a cold CI cache the suite raced the builds and crashed at support-code load with ERR_MODULE_NOT_FOUND (@ocom/ui-shared/dist/index.js), failing every scenario with an all-zero coverage table; locally a stale dist masked the gap. Reproduced by deleting ui-shared/ui-core dist. Declare the six rendered app packages as devDependencies (their own manifests already chain to ui-shared/ui-core), which fixes both test:acceptance and test:coverage:acceptance ordering. knip cannot see this usage because the imports are relative, so the packages are listed in the workspace ignoreDependencies with the build-ordering rationale. Verified with a cold-start turbo run: dist removed, builds execute first, 59/59 scenarios pass with staff coverage restored. Co-authored-by: Copilot <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 211 out of 226 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/ocom/graphql/src/schema/types/member.resolvers.ts:111
- This fallback introduces an N+1 query for every list that requests
Member.role.membersForCurrentEndUseris built by an aggregation that does not populate roles, so each returned member reaches this line and executes a separatequeryByIdWithRolecall. Use a request-scoped DataLoader backed by the newly addedqueryByIdsWithRole, or populate roles in the originating list query, so all role lookups are batched.
| permissions: { | ||
| propertyPermissions: { | ||
| canManageProperties: true, | ||
| __typename: 'PropertyPermissions' as const, |
| permissions: { | ||
| __typename: 'EndUserRolePermissions' as const, | ||
| propertyPermissions: { | ||
| __typename: 'PropertyPermissions' as const, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Implement end-to-end property management with full-field CRUD, community authorization, soft deletion, and comprehensive verification coverage.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Chores: