diff --git a/AGENTS.md b/AGENTS.md index 615b678..a3995d6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -110,7 +110,7 @@ Cross-platform push via the `noticed` (v3) and `action_push_native` gems. APNs f - `json_response` for parsing JSON API responses - `create_new_auth_token` for generating auth headers (Devise Token Auth) - Fixtures in test/fixtures/ and seed data in db/fixtures/test/ -- Run tests: `bin/rails test` (614 tests, 1594 assertions) +- Run tests: `bin/rails test` (624 tests, 1934 assertions) ### Internationalization - Responses are in English (default) or Japanese, picked by the `SetLocale` concern (included in `ApplicationController` and `NonApiApplicationController`). It wraps `process_action`, not `around_action`, so `rescue_from` handlers and `rate_limit` responses are localized too. diff --git a/CHANGELOG.md b/CHANGELOG.md index 9660169..2904278 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## [Unreleased] +- API change: `Shop#time_zone` and `Shopkeeper#time_zone` must be a Rails time zone name (`ActiveSupport::TimeZone.all.map(&:name)`, e.g. `Tokyo`, `London`); anything else, IANA ids such as `Asia/Tokyo` included, gets 422 "Time zone is not included in the list" from `PATCH /shops/:id`, `POST /shops`, sign-up and `PATCH /shopkeeper_auth` instead of being stored. `test/config/time_zones_test.rb` pins the list against `test/fixtures/files/rails_time_zones.tsv`, which the iOS and Android apps embed for their time zone pickers (#155) - Add optional Sentry error monitoring (`sentry-ruby` / `sentry-rails` 7.1). Off unless `SENTRY_DSN` is set (a Kamal secret read from `NATIVEAPPTEMPLATEAPI_SENTRY_DSN`, optional); sends only from production, only unhandled errors (no 4xx, no tracing, profiling or breadcrumbs), with the shopkeeper's ID as the only user data, the account ID as a tag, devise_token_auth headers dropped, email addresses masked and `filter_parameters` applied. The README's "Error monitoring (Sentry)" covers turning it on, what is sent and the recommended Sentry privacy settings. New tests: `test/config/sentry_test.rb`, `test/integration/sentry_context_test.rb` (#154) - API change: tell the apps when the organization in the URL is gone. An `//api/...` request for a deleted account now gets `404 {"code":404,"error_code":"account_not_found","error_message":...}` JSON (was a `302` to `/`), and a signed-in shopkeeper who is no longer a member gets `403 {"code":403,"error_code":"not_account_member","error_message":...}` (was Pundit's generic 401, which the apps treated as a sign-out). Messages are in English and Japanese and follow the usual locale rules. Pundit denials inside an organization the shopkeeper belongs to stay 401; non-API paths still redirect (#153) - Call a shop 店舗 rather than ショップ in the Japanese messages (model name, item tag's shop attribute, the shop limit error, the sample shop description), matching 店舗名 diff --git a/app/models/shop.rb b/app/models/shop.rb index 25a63d5..3e3c343 100644 --- a/app/models/shop.rb +++ b/app/models/shop.rb @@ -8,6 +8,12 @@ class Shop < ApplicationRecord has_many :item_tags, dependent: :destroy validates :name, presence: true + # Rails time zone names only (the apps send ActiveSupport::TimeZone#name); + # IANA ids such as "Asia/Tokyo" are rejected. Checked only on change so a + # legacy value does not block unrelated updates + validates :time_zone, + inclusion: {in: ->(_) { ActiveSupport::TimeZone.all.map(&:name) }}, + if: :will_save_change_to_time_zone? validate :limit_count, on: :create after_create :create_sample_item_tag diff --git a/app/models/shopkeeper.rb b/app/models/shopkeeper.rb index 49490bf..997fd67 100644 --- a/app/models/shopkeeper.rb +++ b/app/models/shopkeeper.rb @@ -31,6 +31,13 @@ class Shopkeeper < ApplicationRecord validates :locale, inclusion: {in: ->(_) { I18n.available_locales.map(&:to_s) }} + # Rails time zone names only (the apps send ActiveSupport::TimeZone#name); + # IANA ids such as "Asia/Tokyo" are rejected. Checked only on change so a + # legacy value does not block unrelated updates + validates :time_zone, + inclusion: {in: ->(_) { ActiveSupport::TimeZone.all.map(&:name) }}, + if: :will_save_change_to_time_zone? + # override devise method to include additional info as opts hash def send_confirmation_instructions(opts = {}) generate_confirmation_token! unless @raw_confirmation_token diff --git a/test/config/time_zones_test.rb b/test/config/time_zones_test.rb new file mode 100644 index 0000000..5969cb2 --- /dev/null +++ b/test/config/time_zones_test.rb @@ -0,0 +1,19 @@ +require "test_helper" + +# Shop#time_zone and Shopkeeper#time_zone accept only the names in +# ActiveSupport::TimeZone.all. The iOS app (TimeZoneData.swift, checked by +# TimeZoneDataTest) and the Android app (TimeZones.kt, checked by +# TimeZonesTest) embed this same list of names and labels for their time zone +# pickers, so a change here makes the apps send names the API rejects or miss +# new ones. When this test fails after a Rails or tzdata upgrade, update +# test/fixtures/files/rails_time_zones.tsv and both apps. +class TimeZonesTest < ActiveSupport::TestCase + test "Rails time zones match the list the apps embed" do + # namelabel per line, generated from activesupport 8.1.4 with + # ActiveSupport::TimeZone.all.map { |z| [z.name, z.to_s].join("\t") } + expected = file_fixture("rails_time_zones.tsv").readlines(chomp: true).map { |line| line.split("\t") } + + assert_equal 152, expected.size + assert_equal expected, ActiveSupport::TimeZone.all.map { |zone| [zone.name, zone.to_s] } + end +end diff --git a/test/controllers/api/v1/shopkeeper/shops_controller_test.rb b/test/controllers/api/v1/shopkeeper/shops_controller_test.rb index 090068b..bef986d 100644 --- a/test/controllers/api/v1/shopkeeper/shops_controller_test.rb +++ b/test/controllers/api/v1/shopkeeper/shops_controller_test.rb @@ -84,6 +84,17 @@ class Api::V1::Shopkeeper::ShopsControllerTest < ActionDispatch::IntegrationTest assert_equal ["Open late", "Osaka"], [@shop.description, @shop.time_zone] end + test "update rejects an unknown time zone with 422" do + patch api_v1_shopkeeper_shop_url(@shop), + params: {shop: {time_zone: "Bogus"}}, + headers: @shopkeeper.create_new_auth_token + + assert_response :unprocessable_entity + assert_equal 422, response.parsed_body["code"] + assert_equal "Time zone is not included in the list", response.parsed_body["error_message"] + assert_equal "Tokyo", @shop.reload.time_zone + end + test "update returns validation error with blank name" do patch api_v1_shopkeeper_shop_url(@shop), params: {shop: {name: ""}}, diff --git a/test/controllers/shopkeeper_auth/registrations_controller_test.rb b/test/controllers/shopkeeper_auth/registrations_controller_test.rb index 293b505..a06c72c 100644 --- a/test/controllers/shopkeeper_auth/registrations_controller_test.rb +++ b/test/controllers/shopkeeper_auth/registrations_controller_test.rb @@ -33,6 +33,25 @@ class ShopkeeperAuth::RegistrationsControllerTest < ActionDispatch::IntegrationT assert_equal "Osaka", shopkeeper.reload.time_zone end + test "rejects an unknown time zone on update with 422" do + shopkeeper.create_default_account + patch shopkeeper_registration_url, params: {time_zone: "Bogus"}, headers: shopkeeper.create_new_auth_token + + assert_response :unprocessable_entity + assert_equal 422, response.parsed_body["code"] + assert_equal "Time zone is not included in the list", response.parsed_body["error_message"] + assert_equal "Tokyo", shopkeeper.reload.time_zone + end + + test "rejects an unknown time zone on sign up with 422" do + assert_no_difference ["Shopkeeper.count", "Account.count", "Shop.count"] do + post shopkeeper_registration_url, params: {email: "bogus-zone@example.com", name: "Bogus Zone", password: "password", time_zone: "Bogus", current_platform: "ios"} + end + + assert_response :unprocessable_entity + assert_equal "Time zone is not included in the list", response.parsed_body["error_message"] + end + test "delete current shopkeeper" do assert_difference "Shopkeeper.count", -1 do delete shopkeeper_registration_url, headers: shopkeeper.create_new_auth_token diff --git a/test/fixtures/files/rails_time_zones.tsv b/test/fixtures/files/rails_time_zones.tsv new file mode 100644 index 0000000..a7f057c --- /dev/null +++ b/test/fixtures/files/rails_time_zones.tsv @@ -0,0 +1,152 @@ +International Date Line West (GMT-12:00) International Date Line West +American Samoa (GMT-11:00) American Samoa +Midway Island (GMT-11:00) Midway Island +Hawaii (GMT-10:00) Hawaii +Alaska (GMT-09:00) Alaska +Pacific Time (US & Canada) (GMT-08:00) Pacific Time (US & Canada) +Tijuana (GMT-08:00) Tijuana +Arizona (GMT-07:00) Arizona +Mazatlan (GMT-07:00) Mazatlan +Mountain Time (US & Canada) (GMT-07:00) Mountain Time (US & Canada) +Central America (GMT-06:00) Central America +Central Time (US & Canada) (GMT-06:00) Central Time (US & Canada) +Chihuahua (GMT-06:00) Chihuahua +Guadalajara (GMT-06:00) Guadalajara +Mexico City (GMT-06:00) Mexico City +Monterrey (GMT-06:00) Monterrey +Saskatchewan (GMT-06:00) Saskatchewan +Bogota (GMT-05:00) Bogota +Eastern Time (US & Canada) (GMT-05:00) Eastern Time (US & Canada) +Indiana (East) (GMT-05:00) Indiana (East) +Lima (GMT-05:00) Lima +Quito (GMT-05:00) Quito +Atlantic Time (Canada) (GMT-04:00) Atlantic Time (Canada) +Caracas (GMT-04:00) Caracas +Georgetown (GMT-04:00) Georgetown +La Paz (GMT-04:00) La Paz +Puerto Rico (GMT-04:00) Puerto Rico +Santiago (GMT-04:00) Santiago +Newfoundland (GMT-03:30) Newfoundland +Asuncion (GMT-03:00) Asuncion +Brasilia (GMT-03:00) Brasilia +Buenos Aires (GMT-03:00) Buenos Aires +Montevideo (GMT-03:00) Montevideo +Greenland (GMT-02:00) Greenland +Mid-Atlantic (GMT-02:00) Mid-Atlantic +Azores (GMT-01:00) Azores +Cape Verde Is. (GMT-01:00) Cape Verde Is. +Casablanca (GMT+00:00) Casablanca +Dublin (GMT+00:00) Dublin +Edinburgh (GMT+00:00) Edinburgh +Lisbon (GMT+00:00) Lisbon +London (GMT+00:00) London +Monrovia (GMT+00:00) Monrovia +UTC (GMT+00:00) UTC +Amsterdam (GMT+01:00) Amsterdam +Belgrade (GMT+01:00) Belgrade +Berlin (GMT+01:00) Berlin +Bern (GMT+01:00) Bern +Bratislava (GMT+01:00) Bratislava +Brussels (GMT+01:00) Brussels +Budapest (GMT+01:00) Budapest +Copenhagen (GMT+01:00) Copenhagen +Ljubljana (GMT+01:00) Ljubljana +Madrid (GMT+01:00) Madrid +Paris (GMT+01:00) Paris +Prague (GMT+01:00) Prague +Rome (GMT+01:00) Rome +Sarajevo (GMT+01:00) Sarajevo +Skopje (GMT+01:00) Skopje +Stockholm (GMT+01:00) Stockholm +Vienna (GMT+01:00) Vienna +Warsaw (GMT+01:00) Warsaw +West Central Africa (GMT+01:00) West Central Africa +Zagreb (GMT+01:00) Zagreb +Zurich (GMT+01:00) Zurich +Athens (GMT+02:00) Athens +Bucharest (GMT+02:00) Bucharest +Cairo (GMT+02:00) Cairo +Harare (GMT+02:00) Harare +Helsinki (GMT+02:00) Helsinki +Jerusalem (GMT+02:00) Jerusalem +Kaliningrad (GMT+02:00) Kaliningrad +Kyiv (GMT+02:00) Kyiv +Pretoria (GMT+02:00) Pretoria +Riga (GMT+02:00) Riga +Sofia (GMT+02:00) Sofia +Tallinn (GMT+02:00) Tallinn +Vilnius (GMT+02:00) Vilnius +Baghdad (GMT+03:00) Baghdad +Istanbul (GMT+03:00) Istanbul +Kuwait (GMT+03:00) Kuwait +Minsk (GMT+03:00) Minsk +Moscow (GMT+03:00) Moscow +Nairobi (GMT+03:00) Nairobi +Riyadh (GMT+03:00) Riyadh +St. Petersburg (GMT+03:00) St. Petersburg +Volgograd (GMT+03:00) Volgograd +Tehran (GMT+03:30) Tehran +Abu Dhabi (GMT+04:00) Abu Dhabi +Baku (GMT+04:00) Baku +Muscat (GMT+04:00) Muscat +Samara (GMT+04:00) Samara +Tbilisi (GMT+04:00) Tbilisi +Yerevan (GMT+04:00) Yerevan +Kabul (GMT+04:30) Kabul +Almaty (GMT+05:00) Almaty +Astana (GMT+05:00) Astana +Ekaterinburg (GMT+05:00) Ekaterinburg +Islamabad (GMT+05:00) Islamabad +Karachi (GMT+05:00) Karachi +Tashkent (GMT+05:00) Tashkent +Chennai (GMT+05:30) Chennai +Kolkata (GMT+05:30) Kolkata +Mumbai (GMT+05:30) Mumbai +New Delhi (GMT+05:30) New Delhi +Sri Jayawardenepura (GMT+05:30) Sri Jayawardenepura +Kathmandu (GMT+05:45) Kathmandu +Dhaka (GMT+06:00) Dhaka +Urumqi (GMT+06:00) Urumqi +Rangoon (GMT+06:30) Rangoon +Bangkok (GMT+07:00) Bangkok +Hanoi (GMT+07:00) Hanoi +Jakarta (GMT+07:00) Jakarta +Krasnoyarsk (GMT+07:00) Krasnoyarsk +Novosibirsk (GMT+07:00) Novosibirsk +Beijing (GMT+08:00) Beijing +Chongqing (GMT+08:00) Chongqing +Hong Kong (GMT+08:00) Hong Kong +Irkutsk (GMT+08:00) Irkutsk +Kuala Lumpur (GMT+08:00) Kuala Lumpur +Perth (GMT+08:00) Perth +Singapore (GMT+08:00) Singapore +Taipei (GMT+08:00) Taipei +Ulaanbaatar (GMT+08:00) Ulaanbaatar +Osaka (GMT+09:00) Osaka +Sapporo (GMT+09:00) Sapporo +Seoul (GMT+09:00) Seoul +Tokyo (GMT+09:00) Tokyo +Yakutsk (GMT+09:00) Yakutsk +Adelaide (GMT+09:30) Adelaide +Darwin (GMT+09:30) Darwin +Brisbane (GMT+10:00) Brisbane +Canberra (GMT+10:00) Canberra +Guam (GMT+10:00) Guam +Hobart (GMT+10:00) Hobart +Melbourne (GMT+10:00) Melbourne +Port Moresby (GMT+10:00) Port Moresby +Sydney (GMT+10:00) Sydney +Vladivostok (GMT+10:00) Vladivostok +Magadan (GMT+11:00) Magadan +New Caledonia (GMT+11:00) New Caledonia +Solomon Is. (GMT+11:00) Solomon Is. +Srednekolymsk (GMT+11:00) Srednekolymsk +Auckland (GMT+12:00) Auckland +Fiji (GMT+12:00) Fiji +Kamchatka (GMT+12:00) Kamchatka +Marshall Is. (GMT+12:00) Marshall Is. +Wellington (GMT+12:00) Wellington +Chatham Is. (GMT+12:45) Chatham Is. +Nuku'alofa (GMT+13:00) Nuku'alofa +Samoa (GMT+13:00) Samoa +Tokelau Is. (GMT+13:00) Tokelau Is. diff --git a/test/models/shop_test.rb b/test/models/shop_test.rb index c46b25a..74304a2 100644 --- a/test/models/shop_test.rb +++ b/test/models/shop_test.rb @@ -69,4 +69,32 @@ def setup end end end + + # The apps send Rails time zone names (ActiveSupport::TimeZone#name) + test "should accept every Rails time zone name" do + shop = @account.shops.new(name: "Test Shop", created_by: @shopkeeper) + ActiveSupport::TimeZone.all.map(&:name).each do |time_zone| + shop.time_zone = time_zone + assert shop.valid?, time_zone + end + end + + # ActiveSupport::TimeZone["Asia/Tokyo"] finds a zone, but "Asia/Tokyo" is an + # IANA id, not one of the Rails names the apps send and list, so reject it + test "should reject a time zone that is not a Rails time zone name" do + shop = @account.shops.new(name: "Test Shop", created_by: @shopkeeper) + ["Bogus", "", "Asia/Tokyo"].each do |time_zone| + shop.time_zone = time_zone + assert_not shop.valid?, time_zone + assert_includes shop.errors[:time_zone], "is not included in the list" + end + end + + # A stored zone that is not a Rails name must not block unrelated updates + test "should not revalidate an unchanged time zone" do + shop = @account.shops.create!(name: "Test Shop", created_by: @shopkeeper) + shop.update_column(:time_zone, "Asia/Tokyo") + shop.name = "Renamed" + assert shop.valid? + end end diff --git a/test/models/shopkeeper_test.rb b/test/models/shopkeeper_test.rb index 62a7444..30ed0f5 100644 --- a/test/models/shopkeeper_test.rb +++ b/test/models/shopkeeper_test.rb @@ -85,6 +85,26 @@ class ShopkeeperTest < ActiveSupport::TestCase end end + # The apps send Rails time zone names (ActiveSupport::TimeZone#name) + test "should accept every Rails time zone name" do + shopkeeper = shopkeepers(:one) + ActiveSupport::TimeZone.all.map(&:name).each do |time_zone| + shopkeeper.time_zone = time_zone + assert shopkeeper.valid?, time_zone + end + end + + # ActiveSupport::TimeZone["Asia/Tokyo"] finds a zone, but "Asia/Tokyo" is an + # IANA id, not one of the Rails names the apps send and list, so reject it + test "should reject a time zone that is not a Rails time zone name" do + shopkeeper = shopkeepers(:one) + ["Bogus", "", "Asia/Tokyo"].each do |time_zone| + shopkeeper.time_zone = time_zone + assert_not shopkeeper.valid?, time_zone + assert_includes shopkeeper.errors[:time_zone], "is not included in the list" + end + end + test "should have many shops through accounts" do shopkeeper = shopkeepers(:one) shopkeeper.create_default_account @@ -287,4 +307,12 @@ class ShopkeeperTest < ActiveSupport::TestCase assert_nil item_tag.created_by_id end end + + # A stored zone that is not a Rails name must not block unrelated updates + test "should not revalidate an unchanged time zone" do + shopkeeper = shopkeepers(:one) + shopkeeper.update_column(:time_zone, "Asia/Tokyo") + shopkeeper.name = "Renamed" + assert shopkeeper.valid? + end end