From c489971ecf13de83dd682e439a490f8de2b95681 Mon Sep 17 00:00:00 2001 From: Daniel Uber Date: Wed, 12 Jan 2022 09:59:14 -0600 Subject: [PATCH] Allow admin control of auth broadcast messages (was: Don't suggest apple authentication) (#16069) * Don't send auth broadcasts for providers that are in beta Currently we have :apple as a restricted provider (you can enable it, but it's treated as beta here, rather than generally available). While we were correctly checking if you had all GA providers enabled in authenticated_with_all_providers?, we were incorrectly pulling all enabled provider names in find_auth_broadcast (the message to send the user), and picking apple_connect. Since it doesn't make sense to omit apple id login from consideration when checking if all available auth methods are used, then recommend that it be used consistently, capture this "GA" state as a method, and use it both in the test "does this user have all available identity providers enabled?" and the selection "which identity provider can I suggest they setup?" consistently. Since we're about to enable google as an auth source (in #15986) I'll check with Josh if he expects this to be GA on release or in limited beta. * Clean up authenticated_with_all_providers? We have a method identities that returns the enabled identities for the user (a relation), and a method ga_providers that returns a list of enabled and not beta provider symbols. Change the set difference to use Array#all? (which will exit early on the first failure). Efficiency note: while I think this reads better,it's possible this issues a number of small (cheap) queries for identity by user id and provider id, but there's a unique index on (provider, user_id) that should be effective. * Only check providers that have active broadcast messages An admin can stop sending "connect using apple" follow ups by disabling that broadcast. I randomized the enabled/active broadcasts for connection options so they're not always pulling the same (facebook? apple?) option every time. * Clean up lost thought in comment --- .../welcome_notification/generator.rb | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/app/services/broadcasts/welcome_notification/generator.rb b/app/services/broadcasts/welcome_notification/generator.rb index d47ddcbe0..aaad40101 100644 --- a/app/services/broadcasts/welcome_notification/generator.rb +++ b/app/services/broadcasts/welcome_notification/generator.rb @@ -50,7 +50,7 @@ module Broadcasts return if user.created_at.after?(1.day.ago) || authenticated_with_all_providers? || - received_notification?(authentication_broadcast) + received_notification?(authentication_broadcasts) Notification.send_welcome_notification(user.id, authentication_broadcast.id) @notification_enqueued = true @@ -99,11 +99,24 @@ module Broadcasts Comment.where(commentable: welcome_thread, user: user).any? end + def authentication_broadcasts + Broadcast.active.where(type_of: "Welcome").where("title like '%_connect'") + end + + def providers + # we filter the providers to suggest based on the following two criteria + # - provider is enabled + # - an acive provider Broadcast message exists + # Disabling/deactivating a provider_connect broadcast removes + # the provider from suggestions sent to users + @providers ||= + Authentication::Providers.enabled.select do |provider| + authentication_broadcasts.exists?(title: "Welcome Notification: #{provider}_connect") + end + end + def authenticated_with_all_providers? - # ga_providers refers to Generally Available (not in beta) - ga_providers = Authentication::Providers.enabled.reject { |sym| sym == :apple } - enabled_providers = identities.pluck(:provider).map(&:to_sym) - (ga_providers - enabled_providers).empty? + providers.all? { |sym| identities.exists?(provider: sym) } end def user_following_tags? @@ -139,11 +152,11 @@ module Broadcasts end def find_auth_broadcast - missing_identities = Authentication::Providers.enabled.filter_map do |provider| + missing_identities = providers.filter_map do |provider| identities.exists?(provider: provider) ? nil : "#{provider}_connect" end - Broadcast.active.find_by!(title: "Welcome Notification: #{missing_identities.first}") + authentication_broadcasts.find_by!(title: "Welcome Notification: #{missing_identities.sample}") end def find_discuss_ask_broadcast