Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed fetching organization data aggregates when segments are enabled #1420

Merged
merged 5 commits into from
Sep 6, 2023

Conversation

themarolt
Copy link
Contributor

@themarolt themarolt commented Sep 5, 2023

Changes proposed ✍️

What

🤖 Generated by Copilot at e65d8ad

This pull request adds support for segment-based organization data in the backend. It enables the organizationFind endpoint to filter organizations by segmentId if the feature flag is on. It also modifies the OrganizationRepository and OrganizationService classes to use the segmentId parameter to calculate segment-specific metrics.

🤖 Generated by Copilot at e65d8ad

To find an organization by id
We added a parameter to findById
It takes a segmentId to filter
The metrics and data we deliver
If the segments feature is applied

Why

How

🤖 Generated by Copilot at e65d8ad

  • Add logic to handle segment id query parameter in organizationFind endpoint (link, link)
  • Import isFeatureEnabled and FeatureFlag from @/feature-flags module (link)
  • Check if segments feature is enabled and return error if not (link)
  • Pass segment id to OrganizationService.findById method (link)
  • Add logic to filter and aggregate organization data by segment id in OrganizationRepository.findById method (link, link)
    • Add segment id parameter to method signature (link)
    • Modify SQL query to use different logic depending on segment id (link)
      • Use CTEs to join organization data with segment data and aggregate metrics for specific segment (link)
      • Use original logic to join organization data with segment data and aggregate metrics for all segments (link)
  • Add segment id parameter to OrganizationService.findById method (link)
    • Pass segment id to OrganizationRepository.findById method (link)

Checklist ✅

  • Label appropriately with Feature, Improvement, or Bug.
  • Add screehshots to the PR description for relevant FE changes
  • New backend functionality has been unit-tested.
  • API documentation has been updated (if necessary) (see docs on API documentation).
  • Quality standards are met.

@themarolt themarolt added the Bug Created by Linear-GitHub Sync label Sep 5, 2023
@sausage-todd
Copy link
Contributor

Hey @themarolt,
The segments are designed the way that they are supposed to be transparent to the developer. There is no difference in how they work no matter if they're "enabled" or not. They're basically always enabled.
This is supposed to make engineers life easier when building features, so they don't have to think about more than one scenario.

Is there a benefit in changing the behavior based on the feature flag like in this PR?

@themarolt
Copy link
Contributor Author

themarolt commented Sep 5, 2023

Hey @themarolt, The segments are designed the way that they are supposed to be transparent to the developer. There is no difference in how they work no matter if they're "enabled" or not. They're basically always enabled. This is supposed to make engineers life easier when building features, so they don't have to think about more than one scenario.

Is there a benefit in changing the behavior based on the feature flag like in this PR?

We have already this iffed behaviour on member query and organization query endpoint and where we index documents to OpenSearch. This is because we only allow filtering by one segment id (1 leaf or 1 parent or 1 grandparent one) and it's provided as a filter from the frontend. This is because of how OpenSearch works and how we can't really filter on aggregated data - so we pre-build and index aggregated documents for each leaf segment and each parent segment and each grandparent segment. This means that you can't currently filter members/organizations by for example 2 leaf segment ids and 1 parent segment id because there is no such document in the index that has aggregated data for those three segments.

This was more meant to be an error for frontend devs so that they have to provide only one segmentId as a filter. On non segmented environments this is not required and as you suggested I can make one query for both cases and get the default segment id to make repo layer segments agnostic. But I would still leave it here on the REST API layer to prevent getting data by default without providing segmentId as well. Unless we should make a different endpoint for this case?

@sausage-todd
Copy link
Contributor

This was more meant to be an error for frontend devs so that they have to provide only one segmentIdas a filter.

Gotcha. Consider using SequelizeRepository.getStrictlySingleActiveSegment(req) then. Seems like it does what you need, plus it reuses the functionality correclty.

@themarolt themarolt merged commit 94d0665 into main Sep 6, 2023
8 checks passed
@themarolt themarolt deleted the bugfix/get-organization-with-segments-C-2025 branch September 6, 2023 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Created by Linear-GitHub Sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants