From ba01552d92d843146870ef5bf72c118b7b32cf09 Mon Sep 17 00:00:00 2001 From: Aga Dufrat Date: Wed, 6 Sep 2023 14:38:15 +0100 Subject: [PATCH] Remove HTTP feature flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rendering of recommended related links was conditional on a feature flag, which is set via a header within our Fastly VCL. As an outcome of RFC-163, we are trying to simplify our CDN configuration by removing functionality that’s no longer used, or by moving logic to other parts of the stack. This change was suggested in a comment on RFC-163: "I think the flag for Whitehall recommended links should be removed. It was put in as a safety mechanism for the introduction of the links, in case something really inappropriate was found. There are ways to manually override the links now, and we've never used the feature flag." Related content is component is defined in govuk_publishing_components gem and this is where the overriding is happening (see: https://github.com/alphagov/govuk_publishing_components/blob/5d713ff637de14bec6abc784c35ee63a0cbc2c75/lib/govuk_publishing_components/presenters/content_item.rb#L70-L74) It's now recommended to create feature flags as environment variables in helm charts. See the docs: https://github.com/alphagov/govuk-developer-docs/blob/33f1f0434aa988419df0687a829367e3f769816f/source/kubernetes/manage-app/set-env-var/index.html.md?plain=1#L7-L21 The value can then be read in Rails config: ``` config.unreleased_features = ENV["UNRELEASED_FEATURES"].present? ``` --- app/controllers/content_items_controller.rb | 22 +++--- app/lib/services.rb | 6 -- app/models/feature_toggler.rb | 15 ---- app/models/http_feature_flags.rb | 21 ------ config/initializers/feature_flags.rb | 9 --- .../content_items_controller_test.rb | 69 +----------------- test/models/feature_toggler_test.rb | 66 ----------------- test/models/http_feature_flags_test.rb | 72 ------------------- 8 files changed, 13 insertions(+), 267 deletions(-) delete mode 100644 app/models/feature_toggler.rb delete mode 100644 app/models/http_feature_flags.rb delete mode 100644 config/initializers/feature_flags.rb delete mode 100644 test/models/feature_toggler_test.rb delete mode 100644 test/models/http_feature_flags_test.rb diff --git a/app/controllers/content_items_controller.rb b/app/controllers/content_items_controller.rb index 76ee10035..7d62d34bc 100644 --- a/app/controllers/content_items_controller.rb +++ b/app/controllers/content_items_controller.rb @@ -31,7 +31,6 @@ def show elsif is_history_page? show_history_page else - set_use_recommended_related_links_header set_access_control_allow_origin_header if request.format.atom? set_guide_draft_access_token if @content_item.is_a?(GuidePresenter) render_template @@ -139,9 +138,8 @@ def set_guide_draft_access_token def load_content_item content_item = Services.content_store.content_item(content_item_path) - if Services.feature_toggler.use_recommended_related_links?(content_item["links"], request.headers) - content_item["links"]["ordered_related_items"] = content_item["links"].fetch("suggested_ordered_related_items", []) - end + + content_item["links"]["ordered_related_items"] = ordered_related_items(content_item["links"]) if content_item["links"] @content_item = PresenterBuilder.new( content_item, @@ -150,6 +148,14 @@ def load_content_item ).presenter end + def ordered_related_items(links) + return [] if links["ordered_related_items_overrides"].present? + + links["ordered_related_items"].presence || links.fetch( + "suggested_ordered_related_items", [] + ) + end + def format_banner_links(links, type) links.each.with_index(1).map do |(title, base_path), index| view_context.link_to( @@ -205,14 +211,6 @@ def set_access_control_allow_origin_header response.headers["Access-Control-Allow-Origin"] = "*" end - def set_use_recommended_related_links_header - response.headers["Vary"] = [response.headers["Vary"], FeatureFlagNames.recommended_related_links].compact.join(", ") - - related_links_request_header = RequestHelper.get_header(FeatureFlagNames.recommended_related_links, request.headers) - required_header_value = Services.feature_toggler.feature_flags.get_feature_flag(FeatureFlagNames.recommended_related_links) - response.headers[FeatureFlagNames.recommended_related_links] = (related_links_request_header == required_header_value).to_s - end - def set_expiry expires_in( @content_item.cache_control_max_age(request.format), diff --git a/app/lib/services.rb b/app/lib/services.rb index 97629a385..cfeae95fb 100644 --- a/app/lib/services.rb +++ b/app/lib/services.rb @@ -12,12 +12,6 @@ def self.content_store ) end - def self.feature_toggler - @feature_toggler ||= FeatureToggler.new( - HttpFeatureFlags.instance, - ) - end - def self.publishing_api @publishing_api ||= GdsApi::PublishingApi.new( Plek.find("publishing-api"), diff --git a/app/models/feature_toggler.rb b/app/models/feature_toggler.rb deleted file mode 100644 index 2c3e2f94f..000000000 --- a/app/models/feature_toggler.rb +++ /dev/null @@ -1,15 +0,0 @@ -class FeatureToggler - attr_reader :feature_flags - - def initialize(feature_flags) - @feature_flags = feature_flags - end - - def use_recommended_related_links?(content_item_links, request_headers) - return false if content_item_links.nil? - - content_item_links.fetch("ordered_related_items", []).empty? && - content_item_links.fetch("ordered_related_items_overrides", []).empty? && - @feature_flags.feature_enabled?(FeatureFlagNames.recommended_related_links, request_headers) - end -end diff --git a/app/models/http_feature_flags.rb b/app/models/http_feature_flags.rb deleted file mode 100644 index 4a50273d0..000000000 --- a/app/models/http_feature_flags.rb +++ /dev/null @@ -1,21 +0,0 @@ -class HttpFeatureFlags - def initialize - @feature_flags = {} - end - - def add_http_feature_flag(feature_flag_name, val) - @feature_flags[RequestHelper.headerise(feature_flag_name)] = val - end - - def get_feature_flag(feature_flag_name) - @feature_flags[RequestHelper.headerise(feature_flag_name)] - end - - def feature_enabled?(feature_flag_name, request_headers) - get_feature_flag(feature_flag_name) == RequestHelper.get_header(feature_flag_name, request_headers) - end - - def self.instance - @instance ||= HttpFeatureFlags.new - end -end diff --git a/config/initializers/feature_flags.rb b/config/initializers/feature_flags.rb deleted file mode 100644 index 5e9b0353b..000000000 --- a/config/initializers/feature_flags.rb +++ /dev/null @@ -1,9 +0,0 @@ -module FeatureFlagNames - def self.recommended_related_links - "Govuk-Use-Recommended-Related-Links" - end -end - -Rails.application.reloader.to_prepare do - HttpFeatureFlags.instance.add_http_feature_flag(FeatureFlagNames.recommended_related_links, "true") -end diff --git a/test/controllers/content_items_controller_test.rb b/test/controllers/content_items_controller_test.rb index e21d129b1..8cd57808a 100644 --- a/test/controllers/content_items_controller_test.rb +++ b/test/controllers/content_items_controller_test.rb @@ -132,19 +132,7 @@ class ContentItemsControllerTest < ActionController::TestCase assert_equal content_item["title"], assigns[:content_item].title end - test "gets item from content store and keeps existing ordered_related_items when feature flag header not specified" do - HttpFeatureFlags.instance.add_http_feature_flag(FeatureFlagNames.recommended_related_links, "true") - content_item = content_store_has_schema_example("case_study", "case_study") - - get :show, params: { path: path_for(content_item) } - assert_response :success - assert_equal content_item["links"]["ordered_related_items"], assigns[:content_item].content_item["links"]["ordered_related_items"] - end - - test "gets item from content store and keep existing ordered_related_items when feature flag header is specified but links already exist" do - HttpFeatureFlags.instance.add_http_feature_flag(FeatureFlagNames.recommended_related_links, "true") - request.headers["HTTP_GOVUK_USE_RECOMMENDED_RELATED_LINKS"] = "true" - + test "gets item from content store and keeps existing ordered_related_items when links already exist" do content_item = content_store_has_schema_example("guide", "guide") get :show, params: { path: path_for(content_item) } @@ -154,10 +142,7 @@ class ContentItemsControllerTest < ActionController::TestCase assert_equal content_item["links"]["ordered_related_items"], assigns[:content_item].content_item["links"]["ordered_related_items"] end - test "gets item from content store and does not change ordered_related_items when feature flag header is specified but link overrides exist" do - HttpFeatureFlags.instance.add_http_feature_flag(FeatureFlagNames.recommended_related_links, "true") - request.headers["HTTP_GOVUK_USE_RECOMMENDED_RELATED_LINKS"] = "true" - + test "gets item from content store and does not change ordered_related_items when link overrides exist" do content_item = content_store_has_schema_example("guide", "guide-with-related-link-overrides") get :show, params: { path: path_for(content_item) } @@ -168,23 +153,7 @@ class ContentItemsControllerTest < ActionController::TestCase assert_nil content_item["links"]["ordered_related_items"] end - test "gets item from content store and keeps ordered_related_items when feature flag header is specified but recommended links turned off" do - HttpFeatureFlags.instance.add_http_feature_flag(FeatureFlagNames.recommended_related_links, "false") - request.headers["HTTP_GOVUK_USE_RECOMMENDED_RELATED_LINKS"] = "true" - - content_item = content_store_has_schema_example("case_study", "case_study") - - get :show, params: { path: path_for(content_item) } - assert_response :success - assert_empty content_item["links"]["ordered_related_items"], "Content item should have existing related links" - assert_not_empty content_item["links"]["suggested_ordered_related_items"], "Content item should have existing suggested related links" - assert_equal [], assigns[:content_item].content_item["links"]["ordered_related_items"] - end - - test "gets item from content store and replaces ordered_related_items when feature flag header is specified and there are no existing links or overrides" do - HttpFeatureFlags.instance.add_http_feature_flag(FeatureFlagNames.recommended_related_links, "true") - request.headers["HTTP_GOVUK_USE_RECOMMENDED_RELATED_LINKS"] = "true" - + test "gets item from content store and replaces ordered_related_items there are no existing links or overrides" do content_item = content_store_has_schema_example("case_study", "case_study") get :show, params: { path: path_for(content_item) } @@ -194,38 +163,6 @@ class ContentItemsControllerTest < ActionController::TestCase assert_equal assigns[:content_item].content_item["links"]["ordered_related_items"], content_item["links"]["suggested_ordered_related_items"] end - test "sets the Govuk-Use-Recommended-Links response header to false when request header is not set" do - HttpFeatureFlags.instance.add_http_feature_flag(FeatureFlagNames.recommended_related_links, "true") - content_item = content_store_has_schema_example("case_study", "case_study") - - get :show, params: { path: path_for(content_item) } - - assert_includes response.headers["Vary"], FeatureFlagNames.recommended_related_links - assert_equal "false", response.headers[FeatureFlagNames.recommended_related_links] - end - - test "sets the Govuk-Use-Recommended-Links response header to false when request header is set to false" do - HttpFeatureFlags.instance.add_http_feature_flag(FeatureFlagNames.recommended_related_links, "true") - request.headers["HTTP_GOVUK_USE_RECOMMENDED_RELATED_LINKS"] = "false" - content_item = content_store_has_schema_example("case_study", "case_study") - - get :show, params: { path: path_for(content_item) } - - assert_includes response.headers["Vary"], FeatureFlagNames.recommended_related_links - assert_equal "false", response.headers[FeatureFlagNames.recommended_related_links] - end - - test "sets the Govuk-Use-Recommended-Links response header to true when request header is set to true" do - HttpFeatureFlags.instance.add_http_feature_flag(FeatureFlagNames.recommended_related_links, "true") - request.headers["HTTP_GOVUK_USE_RECOMMENDED_RELATED_LINKS"] = "true" - content_item = content_store_has_schema_example("case_study", "case_study") - - get :show, params: { path: path_for(content_item) } - - assert_includes response.headers["Vary"], FeatureFlagNames.recommended_related_links - assert_equal "true", response.headers[FeatureFlagNames.recommended_related_links] - end - test "sets the expiry as sent by content-store" do content_item = content_store_has_schema_example("case_study", "case_study") stub_content_store_has_item(content_item["base_path"], content_item, max_age: 20) diff --git a/test/models/feature_toggler_test.rb b/test/models/feature_toggler_test.rb deleted file mode 100644 index bcd633f90..000000000 --- a/test/models/feature_toggler_test.rb +++ /dev/null @@ -1,66 +0,0 @@ -require "test_helper" - -class FeatureTogglerTest < ActiveSupport::TestCase - test "use_recommended_related_links returns false when content item has no links attribute" do - content_item = {} - instance = setup_feature_toggler_with_feature_enabled(true) - - use_recommended_links = instance.use_recommended_related_links?(content_item["links"], @request_headers) - - assert_equal(false, use_recommended_links) - end - - test "use_recommended_related_links returns false when content item has existing ordered_related_items" do - content_item = { "links" => { "ordered_related_items" => [{ "content_id" => "1234" }] } } - instance = setup_feature_toggler_with_feature_enabled(true) - - use_recommended_links = instance.use_recommended_related_links?(content_item["links"], @request_headers) - - assert_equal(false, use_recommended_links) - end - - test "use_recommended_related_links returns false when headers are not correct" do - content_item = { "links" => {} } - instance = setup_feature_toggler_with_feature_enabled(false) - - use_recommended_links = instance.use_recommended_related_links?(content_item["links"], {}) - - assert_equal(false, use_recommended_links) - end - - test "use_recommended_related_links returns true when content item has no ordered_related_items attribute and headers are correct" do - content_item = { "links" => {} } - instance = setup_feature_toggler_with_feature_enabled(true) - - use_recommended_links = instance.use_recommended_related_links?(content_item["links"], @request_headers) - - assert_equal(true, use_recommended_links) - end - - test "use_recommended_related_links returns true when content item has no items in ordered_related_items attribute and headers are correct" do - content_item = { "links" => { "ordered_related_items" => [] } } - instance = setup_feature_toggler_with_feature_enabled(true) - - use_recommended_links = instance.use_recommended_related_links?(content_item["links"], @request_headers) - - assert_equal(true, use_recommended_links) - end - - test "feature_flags attr_reader delegates to instance of feature_flags" do - feature_flags = HttpFeatureFlags.new - feature_toggler = FeatureToggler.new(feature_flags) - - assert_equal feature_flags, feature_toggler.feature_flags - end - - def setup - @request_headers = { 'HTTP_GOVUK_USE_RECOMMENDED_RELATED_LINKS': "true" } - end - - def setup_feature_toggler_with_feature_enabled(feature_enabled) - feature_flags = HttpFeatureFlags.new - feature_flags.stubs(:feature_enabled?).returns(feature_enabled) - - FeatureToggler.new(feature_flags) - end -end diff --git a/test/models/http_feature_flags_test.rb b/test/models/http_feature_flags_test.rb deleted file mode 100644 index 3abe9848c..000000000 --- a/test/models/http_feature_flags_test.rb +++ /dev/null @@ -1,72 +0,0 @@ -require "test_helper" - -class HttpFeatureFlagsTest < ActiveSupport::TestCase - test "instance should create a singleton instance" do - instance = HttpFeatureFlags.instance - instance.add_http_feature_flag("TEST_HEADER", "show") - - new_instance = HttpFeatureFlags.instance - feature_enabled = new_instance.feature_enabled?("TEST_HEADER", "HTTP_TEST_HEADER" => "show") - - assert_equal(true, feature_enabled) - end - - test "add_http_feature_flag should set a new feature flag" do - instance = HttpFeatureFlags.new - - feature_enabled = instance.feature_enabled?("USE_MAGIC", "HTTP_USE_MAGIC" => "only_at_weekends") - assert_equal(false, feature_enabled) - - instance.add_http_feature_flag("USE_MAGIC", "only_at_weekends") - feature_enabled = instance.feature_enabled?("USE_MAGIC", "HTTP_USE_MAGIC" => "only_at_weekends") - assert_equal(true, feature_enabled) - end - - test "feature_enabled? should return false when feature flag has not been set" do - instance = HttpFeatureFlags.new - - feature_enabled = instance.feature_enabled?("USE_MAGIC", "HTTP_USE_MAGIC" => "only_at_weekends") - assert_equal(false, feature_enabled) - end - - test "feature_enabled? should return false when header has not been set" do - instance = HttpFeatureFlags.new - - instance.add_http_feature_flag("USE_MAGIC", "only_at_weekends") - feature_enabled = instance.feature_enabled?("USE_MAGIC", {}) - assert_equal(false, feature_enabled) - end - - test "feature_enabled? should return false when header has been set but does not match specified value" do - instance = HttpFeatureFlags.new - - instance.add_http_feature_flag("USE_MAGIC", "only_at_weekends") - feature_enabled = instance.feature_enabled?("USE_MAGIC", "HTTP_USE_MAGIC" => "all_the_time") - assert_equal(false, feature_enabled) - end - - test "feature_enabled? should return true when headers has been set and matches specified value" do - instance = HttpFeatureFlags.new - - instance.add_http_feature_flag("USE_MAGIC", "only_at_weekends") - feature_enabled = instance.feature_enabled?("USE_MAGIC", "HTTP_USE_MAGIC" => "only_at_weekends") - assert_equal(true, feature_enabled) - end - - test "get_feature_flag returns nil when feature flag does not exist" do - instance = HttpFeatureFlags.new - - feature_flag_value = instance.get_feature_flag("USE_MAGIC") - - assert_nil feature_flag_value - end - - test "get_feature_flag returns feature flag value when feature flag exists" do - instance = HttpFeatureFlags.new - - instance.add_http_feature_flag("USE_MAGIC", "only_at_weekends") - feature_flag_value = instance.get_feature_flag("USE_MAGIC") - - assert_equal "only_at_weekends", feature_flag_value - end -end