Skip to content

feat(saas-api): Graphql schema for FE integration for tenant management - #2176

Open
andrii-yakovenko-flamingo wants to merge 2 commits into
feature/microsoft-365-audit-eventsfrom
feature/directory-fe-integration
Open

andrii-yakovenko-flamingo wants to merge 2 commits into
feature/microsoft-365-audit-eventsfrom
feature/directory-fe-integration

Conversation

@andrii-yakovenko-flamingo

@andrii-yakovenko-flamingo andrii-yakovenko-flamingo commented Sep 14, 2026

Copy link
Copy Markdown
  • Extend OrganizationFilterOptions with excludeOrganizationIds
  • Extract OrganizationDataLoader name into constant

@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

🦩 Flamingo Code Review

1 finding(s) — 0 action required · 1 recommended · 0 informational

Mode: advisory · 1 defect(s) outside any rule

Inline comments: 1 new


Need another pass? Commits pushed after this review are not reviewed automatically.

  • Review the new commits — the commits added since this review
  • Review the whole diff again — ignoring what was already reviewed

Prefer typing? Comment @flamingo-review, or @flamingo-review full. To review every push on this pull request, add the flamingo-review-always label.

React 👍/👎 on inline comments to teach the reviewer.

Started 2026-09-14 11:17 UTC · updated 2026-09-14 11:17 UTC · workflow run

Comment on lines +106 to +109
Set<String> excludedOrganizationIds = filter.getExcludeOrganizationIds();
if (excludedOrganizationIds != null && !excludedOrganizationIds.isEmpty()) {
criteriaList.add(Criteria.where(ORGANIZATION_ID_FIELD).nin(excludedOrganizationIds));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 [warn/recommended] excludeOrganizationIds filters on organizationId while other filters/sort use _id and other fields — verify field exists and is indexed

The new exclude filter is applied via Criteria.where(ORGANIZATION_ID_FIELD).nin(excludedOrganizationIds), i.e. against the document's organizationId field. Elsewhere in this same class the default/status/category/employee filters are applied directly on the Organization document fields (status, category, numberOfEmployees) and the default sort/cursor field is _id. Callers building the exclude set (e.g. from ToolConnection or existing connections) need to make sure they are populating organizationId values (the business identifier) and not Mongo _id values — passing _id strings into this set would silently produce a no-op filter since the field being compared is organizationId, not _id. There is no validation or documentation in the DTO indicating which id type is expected. Consider naming the field more explicitly (e.g. excludeByOrganizationId) or adding a comment on OrganizationFilterOptions.excludeOrganizationIds and OrganizationQueryFilter.excludeOrganizationIds clarifying it expects the business organizationId, not the Mongo document _id, and add an index recommendation since organizationId is now used in a $nin clause alongside other filters that may run frequently on the customers connection-picker path.

Evidence
            Set<String> excludedOrganizationIds = filter.getExcludeOrganizationIds();
            if (excludedOrganizationIds != null && !excludedOrganizationIds.isEmpty()) {
                criteriaList.add(Criteria.where(ORGANIZATION_ID_FIELD).nin(excludedOrganizationIds));
            }
🤖 Prompt for AI agents
In openframe-data-mongo-sync/src/main/java/com/openframe/data/repository/organization/CustomOrganizationRepositoryImpl.java around lines 106-109, address this code-review finding: excludeOrganizationIds filters on organizationId while other filters/sort use _id and other fields — verify field exists and is indexed.
The new exclude filter is applied via Criteria.where(ORGANIZATION_ID_FIELD).nin(excludedOrganizationIds), i.e. against the document's `organizationId` field. Elsewhere in this same class the default/status/category/employee filters are applied directly on the Organization document fields (status, category, numberOfEmployees) and the default sort/cursor field is `_id`. Callers building the exclude set (e.g. from ToolConnection or existing connections) need to make sure they are populating `organizationId` values (the business identifier) and not Mongo `_id` values — passing `_id` strings into this set would silently produce a no-op filter since the field being compared is `organizationId`, not `_id`. There is no validation or documentation in the DTO indicating which id type is expected. Consider naming the field more explicitly (e.g. `excludeByOrganizationId`) or adding a comment on `OrganizationFilterOptions.excludeOrganizationIds` and `OrganizationQueryFilter.excludeOrganizationIds` clarifying it expects the business `organizationId`, not the Mongo document `_id`, and add an index recommendation since `organizationId` is now used in a $nin clause alongside other filters that may run frequently on the customers connection-picker path.
The flagged code:
```
            Set<String> excludedOrganizationIds = filter.getExcludeOrganizationIds();
            if (excludedOrganizationIds != null && !excludedOrganizationIds.isEmpty()) {
                criteriaList.add(Criteria.where(ORGANIZATION_ID_FIELD).nin(excludedOrganizationIds));
            }
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 35 — react 👍/👎 to teach the reviewer

.status(filterOptions.getStatus())
.lastActivityFrom(filterOptions.getLastActivityFrom())
.lastActivityTo(filterOptions.getLastActivityTo())
.excludeOrganizationIds(filterOptions.getExcludeOrganizationIds())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the idea of this filter?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to return a list of customers that are not assigned to a cloud provider to the FE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants