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

Saved Groups with Complex Targeting Conditions #1978

Merged
merged 20 commits into from Jan 4, 2024
Merged

Conversation

jdorn
Copy link
Member

@jdorn jdorn commented Dec 21, 2023

Features and Changes

  • Rename "Inline Groups" to ID Lists
  • Rename "Runtime Groups" to Condition Groups and add support for arbitrary targeting conditions
  • Expand documentation on targeting in general

This is solving the same problem as #1838 but in a different, simpler way with fewer backwards incompatible changes and a cleaner UI.

image

Testing:

  • Migration works for legacy saved groups (both inline and runtime)
  • Create a new saved group (both kinds)
  • Update saved groups (both kinds)
  • Add id list to Target by Attribute
  • Add both kinds of groups to Saved Group Targeting
  • Verify SDK payload renders correctly
  • Archetype testing
  • Verify $groups attribute is added automatically when you had been using runtime groups
  • REST endpoints for saved groups
  • Referencing ID Lists from within Condition Groups

Copy link

github-actions bot commented Dec 21, 2023

Deploy preview for docs ready!

✅ Preview
https://docs-bs8dflkjk-growthbook.vercel.app

Built with commit 68c96e4.
This pull request is being automatically deployed with vercel-action

Copy link

github-actions bot commented Dec 21, 2023

Your preview environment pr-1978-bttf has been deployed.

Preview environment endpoints are available at:

@jdorn jdorn marked this pull request as ready for review December 29, 2023 01:48
@jdorn jdorn requested a review from bttf December 29, 2023 14:11
Copy link
Collaborator

@bttf bttf left a comment

Choose a reason for hiding this comment

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

Some of it above my head but from what I can tell it checks out. Left a few comments / questions

docs/docs/features/targeting.mdx Outdated Show resolved Hide resolved
docs/docs/features/targeting.mdx Outdated Show resolved Hide resolved
@@ -20,38 +26,27 @@ const savedGroupSchema = new mongoose.Schema({
dateUpdated: Date,
values: [String],
source: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is source deprecated? If so, should we consider cleaning it up in the migration?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is deprecated. Since I'm using this field in the JIT migration, I thought it might be safest to leave it in the mongoose schema, even if it's not being set or queried anymore.

group: CreateSavedGroupProps
): Promise<SavedGroupInterface> {
const newGroup = await SavedGroupModel.create({
...group,
id: uniqid("grp_"),
organization,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible that we have saved groups in the DB without an org assigned?

(Is this something we'd want to include in the migration?)

Copy link
Member Author

Choose a reason for hiding this comment

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

No, saved groups have always had an organization field set.

// Already has $groups attribute
if (attributeSchema.some((a) => a.property === "$groups")) return;

// If user has permissions to manage attributes, auto-add $groups attribute
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed the 'auto-add' endpoint added to the organization router/controller but having trouble understanding it. It seems like a legacy way of storing saved groups. Is that correct? Could you explain a little bit about it?

Copy link
Member Author

Choose a reason for hiding this comment

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

We had the concept of "runtime" saved groups. It would look something like this:

{
  id: "sg_abc123",
  organization: "org_abc123",
  source: "runtime",
  attributeKey: "admins"
}

When we rendered this into the SDK payload, we would convert it to a condition looking for this special $groups attribute:

{
  defaultValue: false,
  rules: [
    {
      condition: { "$groups": {"$elemMatch": {"$eq": "admins"}}},
      force: true
    }
  ]
}

And we told people to pass that $groups attribute into their SDK:

new GrowthBook({
  attributes: {
    $groups: ["admins", "beta-users"]
  }
})

Since this was a special attribute, we didn't require people to add it to the SDK Configuration -> Attributes page to be able to use it.

Now that we switched to use the <ConditionInput> component for these groups, it's a pretty bad experience if you go try and edit one of these old runtime groups. If that attribute doesn't exit, it defaults to the advanced JSON editing view which is hard to use.

So this is a little utility endpoint to auto-add a $groups attribute to their org if it's missing and they had previously set up a runtime group. If it fails to run for whatever reason, it's not the end of the world. The user experience will just be slightly degraded until they manually define the attribute in the UI.

jdorn and others added 2 commits January 3, 2024 14:44
Co-authored-by: Adnan Chowdhury <2374625+bttf@users.noreply.github.com>
Co-authored-by: Adnan Chowdhury <2374625+bttf@users.noreply.github.com>
@jdorn jdorn merged commit 175d25c into main Jan 4, 2024
6 checks passed
@jdorn jdorn deleted the saved-group-conditions-2 branch January 4, 2024 16:29
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.

None yet

2 participants