From 5c7d78c0dc70296a5733b134863a0e399254cb61 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 13 Jul 2022 12:22:34 +0100 Subject: [PATCH 1/2] Use feature name instead of id --- lib/flagsmith_client_analytics_processor.ex | 24 +++++++------- test/flagsmith_analytics_processor_test.exs | 35 ++++++++++++--------- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/lib/flagsmith_client_analytics_processor.ex b/lib/flagsmith_client_analytics_processor.ex index 048aa64..608d3f4 100644 --- a/lib/flagsmith_client_analytics_processor.ex +++ b/lib/flagsmith_client_analytics_processor.ex @@ -28,10 +28,10 @@ defmodule Flagsmith.Client.Analytics.Processor do ################################# @doc """ - Given a `t:Flagsmith.Schemas.Features.FeatureState.t/0` or + Given a `t:Flagsmith.Schemas.Features.FeatureState.t/0` or `t:Flagsmith.Schemas.Features.Feature.t/0` or `t:Flagsmith.Schemas.Flag.t/0` and an `t:Flagsmith.Schemas.Environment.t/0` or `t:Flagsmith.Configuration.t/0` add or - increment the call count for feature id to be reported to the analytics endpoint. + increment the call count for feature name to be reported to the analytics endpoint. If `:enable_analytics` in the configuration value of the environment isn't true it's a no op and returns (:noop), otherwise if the feature/flag doesn't have an id it returns an error. @@ -55,7 +55,7 @@ defmodule Flagsmith.Client.Analytics.Processor do to_track, %Configuration{enable_analytics: true} = config ) do - case extract_feature_id(to_track) do + case extract_feature_name(to_track) do id when is_binary(id) or is_integer(id) -> do_track(id, config) @@ -66,11 +66,11 @@ defmodule Flagsmith.Client.Analytics.Processor do def track(_, _), do: :noop - defp extract_feature_id(feature) do + defp extract_feature_name(feature) do case feature do - %Schemas.Features.FeatureState{feature: %{id: id}} -> id - %Schemas.Features.Feature{id: id} -> id - %Schemas.Flag{feature_id: id} -> id + %Schemas.Features.FeatureState{feature: %{name: name}} -> name + %Schemas.Features.Feature{name: name} -> name + %Schemas.Flag{feature_name: name} -> name _ -> :invalid_feature end end @@ -111,7 +111,7 @@ defmodule Flagsmith.Client.Analytics.Processor do end @doc """ - Starts and links a gen_server represented by this module, using a + Starts and links a gen_server represented by this module, using a `t:Flagsmith.Configuration.t/0` as the basis to derive its registration name and inner details. """ @@ -133,7 +133,7 @@ defmodule Flagsmith.Client.Analytics.Processor do end ################################# - ########### Statem Implementation / Internal + ########### Statem Implementation / Internal ################################# @doc false @@ -200,10 +200,10 @@ defmodule Flagsmith.Client.Analytics.Processor do def handle_event({:timeout, :dump}, nil, _, _data), do: {:keep_state_and_data, [{:next_event, :internal, :dump}]} - # a cast for tracking a feature_id simply sets or updates that feature id track + # a cast for tracking a feature_name simply sets or updates that feature name track # in the tracking map - def handle_event(:cast, {:track, feature_id}, _, %{tracking: tracking} = data) do - new_tracking = Map.update(tracking, feature_id, 1, &(&1 + 1)) + def handle_event(:cast, {:track, feature_name}, _, %{tracking: tracking} = data) do + new_tracking = Map.update(tracking, feature_name, 1, &(&1 + 1)) {:keep_state, %{data | tracking: new_tracking}, []} end diff --git a/test/flagsmith_analytics_processor_test.exs b/test/flagsmith_analytics_processor_test.exs index f5d9a76..96d9500 100644 --- a/test/flagsmith_analytics_processor_test.exs +++ b/test/flagsmith_analytics_processor_test.exs @@ -11,7 +11,7 @@ defmodule Flagsmith.Client.Analytics.Processor.Test do @api_url Flagsmith.Configuration.default_url() @api_paths Flagsmith.Configuration.api_paths() - # setup Mox to verify any expectations + # setup Mox to verify any expectations setup :verify_on_exit! # we start the supervisor as a supervised process so it's shut down on every test @@ -77,7 +77,7 @@ defmodule Flagsmith.Client.Analytics.Processor.Test do :ok end - test "dosn't start if enable_analytics is false", %{config: config} do + test "doesn't start if enable_analytics is false", %{config: config} do config = %{config | enable_analytics: false} {:ok, environment} = Flagsmith.Client.get_environment(config) @@ -105,21 +105,22 @@ defmodule Flagsmith.Client.Analytics.Processor.Test do {:ok, environment} = Flagsmith.Client.get_environment(config) # get a flag from that environment - assert %Schemas.Flag{feature_id: id, enabled: true} = - Flagsmith.Client.get_flag(environment, "secret_button") + feature_name_1 = "secret_button" + assert %Schemas.Flag{enabled: true} = + Flagsmith.Client.get_flag(environment, feature_name_1) # assert that now there's a processor for the environment key we've been using pid = Flagsmith.Client.Analytics.Processor.whereis("test_key") assert is_pid(pid) # assert it's tracking correctly: - # - the tracking map should have 1 key being the id of the flag we retrieved + # - the tracking map should have 1 key being the name of the flag we retrieved # with the value being 1 assert {:on, %Flagsmith.Client.Analytics.Processor{ configuration: ^config, dump: 60_000, - tracking: %{^id => 1} = tracking_map_1 + tracking: %{^feature_name_1 => 1} = tracking_map_1 }} = :sys.get_state(pid) # assert there's only 1 key on the tracking map @@ -129,12 +130,12 @@ defmodule Flagsmith.Client.Analytics.Processor.Test do assert Flagsmith.Client.is_feature_enabled(environment, "secret_button") # assert that the processor is still alive and that now the tracking for - # that feature id is at 2 + # that feature name is at 2 assert {:on, %Flagsmith.Client.Analytics.Processor{ configuration: ^config, dump: 60_000, - tracking: %{^id => 2} = tracking_map_2 + tracking: %{^feature_name_1 => 2} = tracking_map_2 }} = :sys.get_state(pid) # assert there's still only 1 key on the tracking map @@ -142,19 +143,20 @@ defmodule Flagsmith.Client.Analytics.Processor.Test do # assert other features track correctly too # get another flag from that environment - assert %Schemas.Flag{feature_id: id_2, enabled: false} = - Flagsmith.Client.get_flag(environment, "header_size") + feature_name_2 = "header_size" + assert %Schemas.Flag{enabled: false} = + Flagsmith.Client.get_flag(environment, feature_name_2) - refute Flagsmith.Client.is_feature_enabled(environment, "header_size") + refute Flagsmith.Client.is_feature_enabled(environment, feature_name_2) # assert that the processor is still alive and that now the tracking for - # the previous feature id is still at 2, and for the new one, id_2, is at + # the previous feature is still at 2, and for the new one, is at # 2 too assert {:on, %Flagsmith.Client.Analytics.Processor{ configuration: ^config, dump: 60_000, - tracking: %{^id => 2, ^id_2 => 2} = tracking_map_3 + tracking: %{^feature_name_1 => 2, ^feature_name_2 => 2} = tracking_map_3 }} = :sys.get_state(pid) # assert there's now 2 keys on the tracking map @@ -172,13 +174,16 @@ defmodule Flagsmith.Client.Analytics.Processor.Test do # change the dump timeout assert :ok = :gen_statem.call(pid, {:update_dump_rate, 1}) + # use an example feature name that we know exists + feature_name = "secret_button" + # we need to set an additional expectation since it will call the analytics # endpoint expect(Tesla.Adapter.Mock, :call, fn tesla_env, _options -> assert_request( tesla_env, - body: "{\"17985\":1}", + body: "{\"#{feature_name}\":1}", query: [], headers: [{@environment_header, "test_key"}], url: Path.join([@api_url, @api_paths.analytics]) <> "/", @@ -196,7 +201,7 @@ defmodule Flagsmith.Client.Analytics.Processor.Test do # attempts to dump, meaning this inadvertently tests that after a dump the # tracking map is effectively empty - assert Flagsmith.Client.is_feature_enabled(environment, "secret_button") + assert Flagsmith.Client.is_feature_enabled(environment, feature_name) assert Flagsmith.Test.Helpers.wait_until( fn -> From 563033f7d46552e9e3aa7a921e8c1071cd37f3a1 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Wed, 13 Jul 2022 12:48:52 +0100 Subject: [PATCH 2/2] formatting --- test/flagsmith_analytics_processor_test.exs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/flagsmith_analytics_processor_test.exs b/test/flagsmith_analytics_processor_test.exs index 96d9500..6ebf3e3 100644 --- a/test/flagsmith_analytics_processor_test.exs +++ b/test/flagsmith_analytics_processor_test.exs @@ -106,8 +106,7 @@ defmodule Flagsmith.Client.Analytics.Processor.Test do # get a flag from that environment feature_name_1 = "secret_button" - assert %Schemas.Flag{enabled: true} = - Flagsmith.Client.get_flag(environment, feature_name_1) + assert %Schemas.Flag{enabled: true} = Flagsmith.Client.get_flag(environment, feature_name_1) # assert that now there's a processor for the environment key we've been using pid = Flagsmith.Client.Analytics.Processor.whereis("test_key") @@ -144,6 +143,7 @@ defmodule Flagsmith.Client.Analytics.Processor.Test do # assert other features track correctly too # get another flag from that environment feature_name_2 = "header_size" + assert %Schemas.Flag{enabled: false} = Flagsmith.Client.get_flag(environment, feature_name_2)