From bb5977d17abe0f644ff41398c4540f51c4866683 Mon Sep 17 00:00:00 2001 From: Richard Towers Date: Thu, 20 Jun 2024 13:58:57 +0100 Subject: [PATCH 1/3] Set time_zone to London in all GOV.UK apps Our conventions for rails apps say that we should set time_zone to London: https://docs.publishing.service.gov.uk/manual/conventions-for-rails-applications.html#specify-london-as-the-timezone In practice however, only about half of the apps do so. Based on the apps running in integration: Apps which set the time_zone to London: AccountApi Collections CollectionsPublisher ContentPublisher Collections Frontend GovernmentFrontend Static EmailAlertApi Frontend GovernmentFrontend GovukChat LocationsApi Maslow PlacesManager Publisher ReleaseApp SearchApiV2 Signon SmartAnswers SpecialistPublisher Static Whitehall Apps which leave the time_zone as it's default UTC: AuthenticatingProxy Contacts ContentDataAdmin ContentPerformanceManager ContentStore ContentTagger ContentStore RouterApi EmailAlertFrontend Feedback FinderFrontend HmrcManualsApi LinkCheckerApi LocalLinksManager ManualsPublisher RouterApi SearchAdmin SearchV2Evaluator ServiceManualPublisher ShortUrlManager Support SupportApi Transition TravelAdvicePublisher Having the apps use inconsistent time_zones feels like a recipe for trouble. config.time_zone has to be set before active_support.initialize_time_zone runs, so we specify that our initializer must run before it. Unfortunately this means that there's no way for applications to set their own value of time_zone. I think this is probably desirable for consumers of govuk_app_config though - I can't think of a legitimate reason why we'd want some apps using different time_zones. Technically this is a breaking change, as it could change behaviour in apps which are using timezone-specific methods and expecting to get UTC times out. Hopefully we're already explicit about when we want UTC and when we want local time though. Once this is merged, I'll remove the bit in the docs about setting time_zone, as it will be automatic. --- lib/govuk_app_config.rb | 1 + lib/govuk_app_config/govuk_timezone.rb | 5 +++++ lib/govuk_app_config/railtie.rb | 4 ++++ spec/lib/govuk_timezone_spec.rb | 12 ++++++++++++ 4 files changed, 22 insertions(+) create mode 100644 lib/govuk_app_config/govuk_timezone.rb create mode 100644 spec/lib/govuk_timezone_spec.rb diff --git a/lib/govuk_app_config.rb b/lib/govuk_app_config.rb index 2546419..939bb23 100644 --- a/lib/govuk_app_config.rb +++ b/lib/govuk_app_config.rb @@ -9,5 +9,6 @@ if defined?(Rails) require "govuk_app_config/govuk_json_logging" require "govuk_app_config/govuk_content_security_policy" + require "govuk_app_config/govuk_timezone" require "govuk_app_config/railtie" end diff --git a/lib/govuk_app_config/govuk_timezone.rb b/lib/govuk_app_config/govuk_timezone.rb new file mode 100644 index 0000000..4f5c259 --- /dev/null +++ b/lib/govuk_app_config/govuk_timezone.rb @@ -0,0 +1,5 @@ +module GovukTimezone + def self.configure(config) + config.time_zone = "London" + end +end diff --git a/lib/govuk_app_config/railtie.rb b/lib/govuk_app_config/railtie.rb index 63ccc77..1d76f67 100644 --- a/lib/govuk_app_config/railtie.rb +++ b/lib/govuk_app_config/railtie.rb @@ -14,6 +14,10 @@ class Railtie < Rails::Railtie end end + initializer "govuk_app_config.configure_timezone", before: "active_support.initialize_time_zone" do |app| + GovukTimezone.configure(app.config) + end + config.before_initialize do GovukJsonLogging.configure if ENV["GOVUK_RAILS_JSON_LOGGING"] end diff --git a/spec/lib/govuk_timezone_spec.rb b/spec/lib/govuk_timezone_spec.rb new file mode 100644 index 0000000..8feee74 --- /dev/null +++ b/spec/lib/govuk_timezone_spec.rb @@ -0,0 +1,12 @@ +require "spec_helper" +require "govuk_app_config/govuk_timezone" + +RSpec.describe GovukError do + describe ".configure" do + it "should set time_zone to London" do + config = double(:config) + expect(config).to receive(:time_zone=).with("London") + GovukTimezone.configure(config) + end + end +end From 83e58ac3efd0abd8e5768edb114cd22baded6d10 Mon Sep 17 00:00:00 2001 From: Richard Towers Date: Fri, 21 Jun 2024 11:37:36 +0100 Subject: [PATCH 2/3] Document changes to Time zone in README --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 87a84d9..995e2e8 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,7 @@ Adds the basics of a GOV.UK application: - Statsd client for reporting stats (deprecated; use Prometheus instead) - Rails logging - Content Security Policy generation for frontend apps +- Sets the time zone to London ## Installation @@ -181,6 +182,9 @@ GovukContentSecurityPolicy.configure Some frontend apps support languages that are not defined in the i18n gem. This provides them with our own custom rules for these languages. +## Time zone + +This gem sets `config.time_zone` to `"London"` - this cannot currently be overridden in application config. ## License From e804b350664898d4f5e94950908087e1f4e5708c Mon Sep 17 00:00:00 2001 From: Richard Towers Date: Fri, 21 Jun 2024 13:02:17 +0100 Subject: [PATCH 3/3] Add logging / error handling to GovukTimezone The more I think about it, the more I worry that having the gem forcibly set time_zone to London without any warnings could lead to a confusing / hard to debug problem down the line. For example, someone trying to set config.time_zone explicitly to UTC, or trying to set it to something else entirely, and then being confused when it's still London. There's no way I can think of to distinguish the "time_zone is UTC because that's the default" situation from the "time_zone is UTC because it's been set explicitly" situation. Logging an info message at least increases the chance that someone will notice what's going on if they are trying to set it to UTC. We're also logging the situation where time_zone is set to London - this config is fairly widespread, and it will become redundant following this change to the gem. However it's harmless, so I don't think it's worth chasing up all the other edge cases. All other values of time_zone should be considered errors, as it would be very confusing to have them silently overridden. --- lib/govuk_app_config/govuk_timezone.rb | 9 +++++++++ spec/lib/govuk_timezone_spec.rb | 26 +++++++++++++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/lib/govuk_app_config/govuk_timezone.rb b/lib/govuk_app_config/govuk_timezone.rb index 4f5c259..9d80f08 100644 --- a/lib/govuk_app_config/govuk_timezone.rb +++ b/lib/govuk_app_config/govuk_timezone.rb @@ -1,5 +1,14 @@ module GovukTimezone def self.configure(config) + case config.time_zone + when "UTC" + Rails.logger.info "govuk_app_config changing time_zone from UTC (the default) to London" + when "London" + Rails.logger.info "govuk_app_config always sets time_zone to London - there is no need to set config.time_zone in your app" + else + raise "govuk_app_config prevents configuring time_zones other than London - config.time_zone was set to #{config.time_zone}" + end + config.time_zone = "London" end end diff --git a/spec/lib/govuk_timezone_spec.rb b/spec/lib/govuk_timezone_spec.rb index 8feee74..9c41005 100644 --- a/spec/lib/govuk_timezone_spec.rb +++ b/spec/lib/govuk_timezone_spec.rb @@ -3,10 +3,30 @@ RSpec.describe GovukError do describe ".configure" do - it "should set time_zone to London" do - config = double(:config) - expect(config).to receive(:time_zone=).with("London") + let(:config) { Rails::Railtie::Configuration.new } + let(:logger) { instance_double("ActiveSupport::Logger") } + + before do + allow(Rails).to receive(:logger).and_return(logger) + end + + it "should override the default UTC time_zone to London" do + config.time_zone = "UTC" + expect(logger).to receive(:info) + GovukTimezone.configure(config) + expect(config.time_zone).to eq("London") + end + + it "should leave time_zones set to London as London" do + config.time_zone = "London" + expect(logger).to receive(:info) GovukTimezone.configure(config) + expect(config.time_zone).to eq("London") + end + + it "should raise an error if configured with any other time zone" do + config.time_zone = "Shanghai" + expect { GovukTimezone.configure(config) }.to raise_error(/govuk_app_config prevents configuring time_zones/) end end end