Skip to content

Commit c275ea9

Browse files
authored
[SHD-1959] Reactive audience recalculation (#535)
[SHD-1359](https://runway.powerhrg.com/backlog_items/SHD-1359) This pull request introduces significant changes to the audiences context and criteria/group association logic, refactoring how group criteria are handled, stored, and queried. Here’s a summary of the main changes: - Moves from storing group info as JSON directly on criteria to a proper join table (audiences_criterion_groups). - Refactors queries, associations, and serialization to use ActiveRecord associations. - Updates controller, model, and test code accordingly. - Adds new database migrations for the join table and column rename. - Cleans up and improves group membership notification logic.
1 parent f76297a commit c275ea9

27 files changed

+450
-152
lines changed

audiences/app/controllers/audiences/contexts_controller.rb

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@
33
module Audiences
44
class ContextsController < ApplicationController
55
def show
6-
render_context Audiences::Context.load(params.require(:key))
6+
render json: Audiences::Context.load(params.require(:key))
77
end
88

99
def update
10-
render_context Audiences.update(params.require(:key), **context_params)
10+
render json: Audiences.update(params.require(:key), **context_params)
1111
end
1212

1313
def users
@@ -32,18 +32,6 @@ def current_criterion
3232
@current_criterion ||= current_context.criteria.find(params[:criterion_id])
3333
end
3434

35-
def render_context(context)
36-
json_setting = {
37-
only: %i[match_all],
38-
methods: %i[count],
39-
include: {
40-
criteria: { only: %i[id groups], methods: %i[count] },
41-
},
42-
}
43-
44-
render json: { extra_users: context.extra_users, **context.as_json(json_setting) }
45-
end
46-
4735
def context_params
4836
params.permit(
4937
:match_all,

audiences/app/jobs/audiences/application_job.rb

Lines changed: 0 additions & 6 deletions
This file was deleted.

audiences/app/models/audiences/context.rb

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,15 @@ class Context < ApplicationRecord
1313
autosave: true,
1414
dependent: :destroy
1515

16+
has_many :context_extra_users, class_name: "Audiences::ContextExtraUser"
17+
has_many :extra_users, class_name: "Audiences::ExternalUser",
18+
through: :context_extra_users,
19+
source: :external_user
20+
21+
scope :relevant_to, ->(group) do
22+
joins(:criteria).merge(Criterion.relevant_to(group))
23+
end
24+
1625
before_save if: :match_all do
1726
self.criteria = []
1827
self.extra_users = []
@@ -26,17 +35,31 @@ def users
2635

2736
delegate :count, to: :users
2837

38+
def as_json(...)
39+
{
40+
match_all: match_all,
41+
count: count,
42+
extra_users: extra_users,
43+
criteria: criteria,
44+
}.as_json(...)
45+
end
46+
2947
private
3048

3149
def notify_subscriptions
3250
Notifications.publish(self)
3351
end
3452

3553
def matching_external_users
36-
return ExternalUser.all if match_all
54+
match_all ? ExternalUser.all : matching_extra_users.or(matching_criteria)
55+
end
56+
57+
def matching_extra_users
58+
ExternalUser.where(id: extra_users.select(:id))
59+
end
3760

38-
criteria_scope = criteria.any? ? ExternalUser.matching_any(*criteria) : ExternalUser.none
39-
ExternalUser.from_scim(*extra_users).or(criteria_scope)
61+
def matching_criteria
62+
criteria.any? ? ExternalUser.matching_any(*criteria) : ExternalUser.none
4063
end
4164
end
4265
end
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
# frozen_string_literal: true
2+
3+
module Audiences
4+
class ContextExtraUser < ApplicationRecord
5+
belongs_to :context
6+
belongs_to :external_user
7+
end
8+
end

audiences/app/models/audiences/criterion.rb

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,45 @@ class Criterion < ApplicationRecord
55
belongs_to :context, class_name: "Audiences::Context"
66
validates :groups, presence: true
77

8+
has_many :criterion_groups, autosave: true, dependent: :destroy
9+
has_many :groups, through: :criterion_groups
10+
11+
scope :relevant_to, ->(group) do
12+
joins(:criterion_groups).where(criterion_groups: { group: group })
13+
end
14+
15+
# Maps an array of attribute hashes to Criterion objects.
16+
#
17+
# Each attribute hash should have a :groups key, whose value is a hash
18+
# mapping resource types (e.g., "Departments", "Territories") to arrays of group hashes,
19+
# each containing an :id key (the SCIM group ID).
20+
#
21+
# Example input:
22+
#
23+
# [
24+
# { groups: { Departments: [{ id: "1" }] } },
25+
# { groups: { Territories: [{ id: "2" }], Departments: [{ id: "3" }] } },
26+
# ]
27+
#
28+
# Returns an array of new Criterion objects, each initialized with the corresponding group criterion.
29+
#
30+
# @param [Array<Hash>] criteria Array of attribute hashes describing groups
31+
# @return [Array<Criterion>] Array of Criterion objects
832
def self.map(criteria)
933
Array(criteria).map do |attrs|
10-
attrs["groups"] = attrs["groups"]&.to_h do |resource_type, groups|
11-
[resource_type, Audiences::Group.from_scim(resource_type, *groups).as_json]
34+
groups = attrs["groups"]&.flat_map do |resource_type, scim_groups|
35+
Audiences::Group.from_scim(resource_type, *scim_groups).to_a
1236
end
13-
new(attrs)
37+
new(groups: groups)
1438
end
1539
end
1640

41+
def as_json(...)
42+
groups = self.groups.group_by(&:resource_type)
43+
44+
{ id: id, count: count, groups: groups }.as_json(...)
45+
end
46+
1747
def users
1848
Audiences::ExternalUser.matching(self)
1949
.instance_exec(&Audiences.default_users_scope)
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
# frozen_string_literal: true
2+
3+
module Audiences
4+
class CriterionGroup < ApplicationRecord
5+
belongs_to :group
6+
belongs_to :criterion
7+
end
8+
end

audiences/app/models/audiences/external_user.rb

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@
33
module Audiences
44
class ExternalUser < ApplicationRecord
55
has_many :group_memberships, dependent: :destroy
6-
has_many :groups, through: :group_memberships
6+
has_many :groups, through: :group_memberships, dependent: :destroy
7+
8+
has_many :context_extra_users, class_name: "Audiences::ContextExtraUser", dependent: :destroy
9+
has_many :contexts, through: :context_extra_users, source: :context
710

811
if Audiences.config.identity_class
912
belongs_to :identity, class_name: Audiences.config.identity_class, # rubocop:disable Rails/ReflectionClassName
@@ -13,6 +16,15 @@ class ExternalUser < ApplicationRecord
1316
inverse_of: false
1417
end
1518

19+
after_commit if: :active_previously_changed?, on: %i[create update destroy] do
20+
group_contexts = groups.flat_map do |group|
21+
Audiences::Context.relevant_to(group).to_a
22+
end
23+
match_all_contexts = Audiences::Context.where(match_all: true)
24+
25+
Audiences::Notifications.publish(*[*contexts, *group_contexts, *match_all_contexts].uniq)
26+
end
27+
1628
scope :search, ->(display_name) do
1729
where(arel_table[:display_name].matches("%#{display_name}%"))
1830
end
@@ -23,12 +35,13 @@ class ExternalUser < ApplicationRecord
2335
end
2436

2537
scope :matching, ->(criterion) do
26-
groups = (criterion.try(:groups) || criterion).values.reject(&:empty?)
27-
return none if groups.empty?
38+
return none if criterion.groups.empty?
2839

29-
groups.reduce(self) do |scope, group|
30-
group_ids = Audiences::Group.where(scim_id: group.pluck("id")).pluck(:id)
31-
scope.where(id: Audiences::GroupMembership.where(group_id: group_ids).select(:external_user_id))
40+
criterion.groups
41+
.group_by(&:resource_type)
42+
.values
43+
.reduce(self) do |scope, groups|
44+
scope.where(id: Audiences::GroupMembership.where(group_id: groups.pluck(:id)).select(:external_user_id))
3245
end
3346
end
3447

audiences/app/models/audiences/group.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
module Audiences
44
class Group < ApplicationRecord
55
has_many :group_memberships, dependent: :destroy
6-
has_many :external_users, through: :group_memberships
6+
has_many :external_users, through: :group_memberships, dependent: :destroy
77

88
validates :display_name, presence: true
99
validates :external_id, presence: true

audiences/app/models/audiences/group_membership.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,11 @@ module Audiences
44
class GroupMembership < ApplicationRecord
55
belongs_to :external_user
66
belongs_to :group
7+
8+
after_commit on: %i[create destroy] do
9+
relevant_groups = Audiences::Context.relevant_to(group)
10+
11+
Audiences::Notifications.publish(*relevant_groups.to_a)
12+
end
713
end
814
end
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
# frozen_string_literal: true
2+
3+
class CreateAudiencesCriterionGroups < ActiveRecord::Migration[6.1]
4+
def change
5+
create_table :audiences_criterion_groups do |t|
6+
t.references :criterion, foreign_key: false
7+
t.references :group, foreign_key: false
8+
9+
t.timestamps
10+
end
11+
end
12+
end

0 commit comments

Comments
 (0)