[SHD-1879] Productionize poc#560
Conversation
|
A change to documentation files was detected in your PR. Please visit this link to preview changes: https://portal-staging.powerapp.cloud/docs?filters[kind]=all&filters[user]=all&filters[namespaceFilter]=productionize-poc |
…g, validation, and RuboCop fixes
| # Return relation, not array, so downstream code can continue querying | ||
| Audiences::ConfigurableAdapter.active_audiences_users.merge(matching_users) |
There was a problem hiding this comment.
We can't assume this is a relation because it's configurable, so it could be anything. It doesn't seem like the code is doing it from what I can tell, just mentioning because of the above comment.
There was a problem hiding this comment.
| module Audiences | ||
| module Integrations |
There was a problem hiding this comment.
This is all legacy groups, and users, right?
There was a problem hiding this comment.
Yes, this will be removed when we remove support for Audiences::ExternalUser/Audiences::Group.
| # Proc that transforms a user record to the hash format Audiences expects | ||
| # Required keys: id, external_id, display_name, active, groups (array with id) | ||
| # | ||
| # Example: | ||
| # config.to_audiences_hash_proc = ->(user) { | ||
| # { | ||
| # id: user.scim_id, # Maps provider-specific field to generic 'id' | ||
| # external_id: user.external_id, | ||
| # display_name: user.display_name, | ||
| # active: user.active, | ||
| # groups: user.scim_groups.map { |g| { id: g.scim_id, ... } } | ||
| # } | ||
| # } | ||
| config_accessor :to_audiences_hash_proc |
There was a problem hiding this comment.
This could be part of the adapter instead, perhaps?
There was a problem hiding this comment.
Same of the other to_*_hash_proc
There was a problem hiding this comment.
I put the to_audiences_hash_proc in configuration because it's the integration point between Audiences and our models. If we want to move it into the adapter, we'd need an adapter subclass to configure this. Are you suggesting we create a new class and configure it there instead of here? Something like:
# Connect Server creates subclass
class TwoPercentAdapter < Audiences::ConfigurableAdapter
def to_audiences_hash
{ ... TwoPercent-specific ... }
end
end
config.adapter_class = TwoPercentAdapter
| Audiences::Group.create!(scim_id: scim_id, display_name: "#{resource_type} #{scim_id}", | ||
| external_id: scim_id, resource_type: resource_type, **attrs) | ||
| # Mode-aware factory - creates appropriate model based on configuration | ||
| def create_group(external_id: next_scim_id, resource_type: "Groups", external_users: [], **attrs) |
There was a problem hiding this comment.
It is weird to have the factories behaving differently depending on the configuration. It would be more clear to have different factories and different tests, which would test the behavior of audiences depending on the "feature flag". Not a blocker since the idea is to remove this config/flag.
2 participants
This PR decouples SCIM from Audiences.
The ticket for this PR is here.