diff --git a/app/controllers/admin/settings/general_settings_controller.rb b/app/controllers/admin/settings/general_settings_controller.rb index 6dd4ff429513d..98984584e4c11 100644 --- a/app/controllers/admin/settings/general_settings_controller.rb +++ b/app/controllers/admin/settings/general_settings_controller.rb @@ -28,17 +28,25 @@ def authorization_resource end def settings_params - params.require(:settings_general)&.permit( + params.require(:settings_general)&.merge(parsed_countries)&.permit( settings_keys.map(&:to_sym), social_media_handles: ::Settings::General::SOCIAL_MEDIA_SERVICES, meta_keywords: ::Settings::General.meta_keywords.keys, credit_prices_in_cents: ::Settings::General.credit_prices_in_cents.keys, + billboard_enabled_countries: ISO3166::Country.codes, ) end def settings_keys ::Settings::General.keys + SPECIAL_PARAMS_TO_ADD end + + def parsed_countries + countries = params[:settings_general][:billboard_enabled_countries] + return {} unless countries + + { billboard_enabled_countries: JSON.parse(countries) } + end end end end diff --git a/app/helpers/admin/settings_helper.rb b/app/helpers/admin/settings_helper.rb new file mode 100644 index 0000000000000..1149f6bdf914a --- /dev/null +++ b/app/helpers/admin/settings_helper.rb @@ -0,0 +1,11 @@ +module Admin + module SettingsHelper + def billboard_enabled_countries_for_editing + ::Settings::General.billboard_enabled_countries.to_json + end + + def billboard_all_countries_for_editing + ISO3166::Country.all.to_h { |country| [country.alpha2, country.common_name] } + end + end +end diff --git a/app/javascript/billboard/locations/index.jsx b/app/javascript/billboard/locations/index.jsx new file mode 100644 index 0000000000000..0c01ff0e9d8ed --- /dev/null +++ b/app/javascript/billboard/locations/index.jsx @@ -0,0 +1,62 @@ +import { h } from 'preact'; +import { useCallback } from 'preact/hooks'; +import PropTypes from 'prop-types'; +import { MultiSelectAutocomplete } from '@crayons'; + +export { SelectedLocation } from './templates'; + +export const Locations = ({ + defaultValue = [], + allLocations, + inputId, + onChange, + placeholder = 'Enter a country name...', + template, +}) => { + const autocompleteLocations = useCallback( + (query) => { + return new Promise((resolve) => { + queueMicrotask(() => { + const suggestions = []; + const caseInsensitiveQuery = query.toLowerCase(); + Object.keys(allLocations).forEach((name) => { + if (name.toLowerCase().indexOf(caseInsensitiveQuery) > -1) { + suggestions.push(allLocations[name]); + } + }); + resolve(suggestions); + }); + }); + }, + [allLocations], + ); + + return ( + + ); +}; + +const locationsShape = PropTypes.shape({ + name: PropTypes.string.isRequired, + code: PropTypes.string.isRequired, + withRegions: PropTypes.bool, +}); + +Locations.propTypes = { + defaultValue: PropTypes.arrayOf(locationsShape), + allLocations: PropTypes.objectOf(locationsShape).isRequired, + inputId: PropTypes.string.isRequired, + onChange: PropTypes.func.isRequired, + placeholder: PropTypes.string, + template: PropTypes.elementType, +}; diff --git a/app/javascript/billboard/locations/templates.jsx b/app/javascript/billboard/locations/templates.jsx new file mode 100644 index 0000000000000..440705d779080 --- /dev/null +++ b/app/javascript/billboard/locations/templates.jsx @@ -0,0 +1,48 @@ +import { h } from 'preact'; +import { useCallback } from 'preact/hooks'; +import { ButtonNew as Button, Icon } from '@crayons'; +import { Close } from '@images/x.svg'; + +/** + * Higher-order component that returns a template responsible for the layout of + * a selected location + * + * @returns {h.JSX.ElementType} + */ +export const SelectedLocation = ({ + displayName, + onNameClick, + label, + ExtraInfo, +}) => { + const Template = ({ onEdit: _, onDeselect, ...location }) => { + const onClick = useCallback(() => onNameClick(location), [location]); + + return ( +
+ + +
+ ); + }; + + Template.displayName = displayName; + return Template; +}; diff --git a/app/javascript/packs/admin/billboardEnabledCountries.jsx b/app/javascript/packs/admin/billboardEnabledCountries.jsx new file mode 100644 index 0000000000000..cb8b0a69afef1 --- /dev/null +++ b/app/javascript/packs/admin/billboardEnabledCountries.jsx @@ -0,0 +1,91 @@ +import { h, render } from 'preact'; +import { Locations, SelectedLocation } from '../../billboard/locations'; + +const RegionMarker = ({ withRegions }) => { + return ( + + {withRegions ? 'Including' : 'Excluding'} regions + + ); +}; + +function parseDOMState(hiddenField) { + const countriesByCode = JSON.parse(hiddenField.dataset.allCountries); + + const allCountries = {}; + for (const [code, name] of Object.entries(countriesByCode)) { + allCountries[name] = { name, code }; + } + const existingSetting = JSON.parse(hiddenField.value); + const selectedCountries = Object.keys(existingSetting).map((code) => ({ + name: countriesByCode[code], + code, + withRegions: existingSetting[code] === 'with_regions', + })); + + return { allCountries, selectedCountries }; +} + +function syncSelectionsToDOM(hiddenField, countries) { + const newValue = countries.reduce((value, { code, withRegions }) => { + value[code] = withRegions ? 'with_regions' : 'without_regions'; + return value; + }, {}); + hiddenField.value = JSON.stringify(newValue); +} + +/** + * Sets up and renders a Preact component to handle searching for and enabling + * countries for targeting (and, per country, to enable region-level targeting). + */ +function setupEnabledCountriesEditor() { + const editor = document.getElementById('billboard-enabled-countries-editor'); + const hiddenField = document.querySelector('.geolocation-multiselect'); + + if (!(editor && hiddenField)) return; + + const { allCountries, selectedCountries } = parseDOMState(hiddenField); + let currentSelections = selectedCountries; + + function setCountriesSelection(countries) { + currentSelections = countries; + syncSelectionsToDOM(hiddenField, currentSelections); + } + + function updateRegionSetting(country) { + const selected = currentSelections.find( + (selectedCountry) => selectedCountry.code === country.code, + ); + selected.withRegions = !selected.withRegions; + syncSelectionsToDOM(hiddenField, currentSelections); + renderLocations(); + } + + const EnabledCountry = SelectedLocation({ + displayName: 'EnabledCountry', + onNameClick: updateRegionSetting, + label: 'Toggle region targeting', + ExtraInfo: RegionMarker, + }); + + function renderLocations() { + render( + , + editor, + ); + } + + renderLocations(); +} + +if (document.readyState !== 'loading') { + setupEnabledCountriesEditor(); +} else { + document.addEventListener('DOMContentLoaded', setupEnabledCountriesEditor); +} diff --git a/app/lib/constants/settings/general.rb b/app/lib/constants/settings/general.rb index 0fa8723be455a..534897b1ba6bc 100644 --- a/app/lib/constants/settings/general.rb +++ b/app/lib/constants/settings/general.rb @@ -8,6 +8,9 @@ def self.details ahoy_tracking: { description: I18n.t("lib.constants.settings.general.ahoy_tracking.description") }, + billboard_enabled_countries: { + description: I18n.t("lib.constants.settings.general.billboard_enabled_countries.description") + }, contact_email: { description: I18n.t("lib.constants.settings.general.contact_email.description"), placeholder: "hello@example.com" diff --git a/app/models/billboard.rb b/app/models/billboard.rb index 8b44dca14d84a..2d5c85406fd01 100644 --- a/app/models/billboard.rb +++ b/app/models/billboard.rb @@ -160,7 +160,7 @@ def validate_tag def validate_geolocations target_geolocations.each do |geo| - unless geo.valid? + unless geo.valid?(:targeting) errors.add(:target_geolocations, I18n.t("models.billboard.invalid_location", location: geo.to_iso3166)) end end diff --git a/app/models/geolocation.rb b/app/models/geolocation.rb index 9ec81d598e56a..2b5603763df8b 100644 --- a/app/models/geolocation.rb +++ b/app/models/geolocation.rb @@ -30,11 +30,10 @@ def deserialize(geo_ltrees) FEATURE_FLAG = :billboard_location_targeting ISO3166_SEPARATOR = "-".freeze LTREE_SEPARATOR = ".".freeze - # Maybe this should be a config of some kind...? - PERMITTED_COUNTRIES = [ - ISO3166::Country.code_from_name("United States"), - ISO3166::Country.code_from_name("Canada"), - ].freeze + DEFAULT_ENABLED_COUNTRIES = { + ISO3166::Country.code_from_name("United States") => :with_regions, + ISO3166::Country.code_from_name("Canada") => :with_regions + }.freeze attr_reader :country_code, :region_code @@ -60,11 +59,13 @@ def initialize(country_code, region_code = nil) @region_code = region_code end - # validates :country_code, inclusion: { in: ISO3166::Country.codes } - validates :country_code, inclusion: { in: PERMITTED_COUNTRIES } + validates :country_code, inclusion: { + in: ->(_) { Settings::General.billboard_enabled_countries.keys } + } validates :region_code, inclusion: { in: ->(geolocation) { ISO3166::Country.region_codes_if_exists(geolocation.country_code) } }, allow_nil: true + validate :valid_region_for_targeting, on: :targeting def ==(other) country_code == other.country_code && region_code == other.region_code @@ -88,6 +89,13 @@ def to_sql_query_clause(column_name = :target_geolocations) "'#{lquery}' ~ #{column_name}" end + def valid_region_for_targeting + return if region_code.nil? || + Settings::General.billboard_enabled_countries[country_code] == :with_regions + + errors.add(:region_code, "was provided on a country with region targeting disabled") + end + def errors_as_sentence errors.full_messages.to_sentence end diff --git a/app/models/settings/general.rb b/app/models/settings/general.rb index 487bebb7dc6f6..c11cfa09931c6 100644 --- a/app/models/settings/general.rb +++ b/app/models/settings/general.rb @@ -72,6 +72,10 @@ class General < Base setting :payment_pointer, type: :string setting :stripe_api_key, type: :string, default: ApplicationConfig["STRIPE_SECRET_KEY"] setting :stripe_publishable_key, type: :string, default: ApplicationConfig["STRIPE_PUBLISHABLE_KEY"] + # Billboard-related. Not sure this is the best place for it, but it's a start. + setting :billboard_enabled_countries, type: :hash, default: Geolocation::DEFAULT_ENABLED_COUNTRIES, validates: { + enabled_countries_hash: true + } # Newsletter # diff --git a/app/services/settings/general/upsert.rb b/app/services/settings/general/upsert.rb index db63ddc69ec80..f556da0964103 100644 --- a/app/services/settings/general/upsert.rb +++ b/app/services/settings/general/upsert.rb @@ -26,6 +26,7 @@ def self.clean_params(settings) settings[param] = settings[param]&.downcase&.delete(" ") if settings[param] end settings[:credit_prices_in_cents]&.transform_values!(&:to_i) + settings[:billboard_enabled_countries]&.transform_values!(&:to_sym) settings end diff --git a/app/validators/enabled_countries_hash_validator.rb b/app/validators/enabled_countries_hash_validator.rb new file mode 100644 index 0000000000000..bfa9e7b7e403e --- /dev/null +++ b/app/validators/enabled_countries_hash_validator.rb @@ -0,0 +1,21 @@ +class EnabledCountriesHashValidator < ActiveModel::EachValidator + VALID_HASH_VALUES = %i[with_regions without_regions].freeze + + def validate_each(record, attribute, value) + if value.blank? || !value.is_a?(Hash) + record.errors.add(attribute, + options[:message] || I18n.t("validators.iso3166_hash_validator.is_blank")) + return + end + + unless value.keys.all? { |key| ISO3166::Country.codes.include? key } + record.errors.add(attribute, + options[:message] || I18n.t("validators.iso3166_hash_validator.invalid_key")) + end + + return if value.values.all? { |value| VALID_HASH_VALUES.include? value } + + record.errors.add(attribute, + options[:message] || I18n.t("validators.iso3166_hash_validator.invalid_value")) + end +end diff --git a/app/views/admin/settings/forms/_monetization.html.erb b/app/views/admin/settings/forms/_monetization.html.erb index db0d56621512d..328b6e64763e8 100644 --- a/app/views/admin/settings/forms/_monetization.html.erb +++ b/app/views/admin/settings/forms/_monetization.html.erb @@ -32,6 +32,23 @@ value: Settings::General.payment_pointer, placeholder: Constants::Settings::General.details[:payment_pointer][:placeholder] %> + + <% if FeatureFlag.enabled?(Geolocation::FEATURE_FLAG) %> +
+ <%= admin_config_label :billboard_enabled_countries %> + <%= admin_config_description Constants::Settings::General.details[:billboard_enabled_countries][:description] %> +
+
+ + + <%= javascript_packs_with_chunks_tag "admin/billboardEnabledCountries", defer: true %> + <% end %> <%= render "update_setting_button", f: f %> diff --git a/config/locales/lib/en.yml b/config/locales/lib/en.yml index 056a6eecc91a3..ed41e2db6e04d 100644 --- a/config/locales/lib/en.yml +++ b/config/locales/lib/en.yml @@ -96,6 +96,8 @@ en: general: ahoy_tracking: description: Track visits and events - data is stored in your database. A restart is required for changes to this configuration to take effect. + billboard_enabled_countries: + description: Countries that can be geotargeted by billboards. Click on a selected country to toggle targeting on a sub-country level. contact_email: description: Used for contact links. Please provide an email address where users can get in touch with you or your team. credit: diff --git a/config/locales/lib/fr.yml b/config/locales/lib/fr.yml index f11274f503a12..747b2887468c7 100644 --- a/config/locales/lib/fr.yml +++ b/config/locales/lib/fr.yml @@ -96,6 +96,8 @@ fr: general: ahoy_tracking: description: Track visits and events - data is stored in your database. A restart is required for changes to this configuration to take effect. + billboard_enabled_countries: + description: Countries that can be geotargeted by billboards. Click on a selected country to toggle targeting on a sub-country level. contact_email: description: Used for contact links. Please provide an email address where users can get in touch with you or your team. credit: diff --git a/config/locales/models/en.yml b/config/locales/models/en.yml index aeeb2006815a3..9693f13ca15a8 100644 --- a/config/locales/models/en.yml +++ b/config/locales/models/en.yml @@ -42,7 +42,7 @@ en: experience4: Have experience level four experience5: Have experience level five billboard: - invalid_location: '%{location} is not a supported ISO 3166-2 code' + invalid_location: '%{location} is not an enabled target ISO 3166-2 code' broadcast: single_active: You can only have one active announcement broadcast comment: diff --git a/config/locales/models/fr.yml b/config/locales/models/fr.yml index 9fc64cdc737f2..be5975293df34 100644 --- a/config/locales/models/fr.yml +++ b/config/locales/models/fr.yml @@ -41,7 +41,7 @@ fr: experience4: Have experience level four experience5: Have experience level five billboard: - invalid_location: "%{location} n'est pas un code pris en charge de la norme ISO 3166-2" + invalid_location: "%{location} n'est pas un code cible activé de la norme ISO 3166-2" broadcast: single_active: Vous ne pouvez avoir qu'une seule diffusion d'annonce active comment: diff --git a/config/locales/validators/en.yml b/config/locales/validators/en.yml index a015352b37118..982f904ddb478 100644 --- a/config/locales/validators/en.yml +++ b/config/locales/validators/en.yml @@ -17,3 +17,7 @@ en: is_reserved: is reserved valid_domain_csv_validator: invalid_list_format: must be a comma-separated list of valid domains + iso3166_hash_validator: + is_blank: must be provided + invalid_key: contains an invalid ISO 3166 code + invalid_value: must have values `:with_regions` or `:without_regions` diff --git a/config/locales/validators/fr.yml b/config/locales/validators/fr.yml index c6b68c4bc9b89..00f4b7a17a7b7 100644 --- a/config/locales/validators/fr.yml +++ b/config/locales/validators/fr.yml @@ -17,3 +17,7 @@ fr: is_reserved: est réservé valid_domain_csv_validator: invalid_list_format: doit être une liste de domaines valides, séparés par des virgules. + iso3166_hash_validator: + is_blank: doit être fourni + invalid_key: contient un code non valide de la norme ISO 3166-2 + invalid_value: doit avoir des valeurs `:with_regions` ou `:without_regions` diff --git a/cypress/e2e/seededFlows/adminFlows/billboards/editBillboards.spec.js b/cypress/e2e/seededFlows/adminFlows/billboards/editBillboards.spec.js index 9097a87b7c64f..3d89538aa1fa1 100644 --- a/cypress/e2e/seededFlows/adminFlows/billboards/editBillboards.spec.js +++ b/cypress/e2e/seededFlows/adminFlows/billboards/editBillboards.spec.js @@ -138,7 +138,7 @@ describe('Billboards Form', () => { cy.findByRole('button', { name: 'Save Billboard' }).click(); cy.get('#flash-0').should(($flashMessage) => { expect($flashMessage).to.not.contain( - 'is not a supported ISO 3166-2 code', + 'is not an enabled target ISO 3166-2 code', ); }); }); @@ -151,10 +151,10 @@ describe('Billboards Form', () => { cy.get('#flash-0').should(($flashMessage) => { // We currently support only the US and CA expect($flashMessage).to.contain( - 'MX-CMX is not a supported ISO 3166-2 code', + 'MX-CMX is not an enabled target ISO 3166-2 code', ); expect($flashMessage).to.not.contain( - 'US-NY is not a supported ISO 3166-2 code', + 'US-NY is not an enabled target ISO 3166-2 code', ); }); }); diff --git a/cypress/e2e/seededFlows/adminFlows/config/monetizationSection.spec.js b/cypress/e2e/seededFlows/adminFlows/config/monetizationSection.spec.js new file mode 100644 index 0000000000000..7ee6e7be44029 --- /dev/null +++ b/cypress/e2e/seededFlows/adminFlows/config/monetizationSection.spec.js @@ -0,0 +1,155 @@ +describe('Monetization section', () => { + beforeEach(() => { + cy.testSetup(); + cy.fixture('users/adminUser.json').as('user'); + cy.get('@user').then((user) => cy.loginUser(user)); + }); + + function withinMonetizationSection(callback) { + cy.findByText('Monetization') + .as('monetizationSectionHeader') + .parent() + .within(() => { + cy.get('@monetizationSectionHeader').click(); + callback(); + }); + } + + context('when location targeting is not enabled', () => { + beforeEach(() => { + cy.visit('/admin/customization/config'); + }); + + it('does not show countries for billboard geotargeting', () => { + withinMonetizationSection(() => { + cy.findByText('Billboard enabled countries').should('not.exist'); + cy.findByRole('combobox').should('not.exist'); + cy.findByLabelText('Enabled countries for targeting').should( + 'not.exist', + ); + }); + }); + }); + + context('when location targeting is enabled', () => { + beforeEach(() => { + cy.enableFeatureFlag('billboard_location_targeting'); + cy.visit('/admin/customization/config'); + }); + + it('autocompletes countries for selection', () => { + withinMonetizationSection(() => { + cy.findByText('Billboard enabled countries').should('exist'); + + cy.findByLabelText('Enabled countries for targeting').as('textbox'); + cy.findByRole('listbox').should('not.exist'); + + cy.get('@textbox').type('ger'); + cy.findByRole('listbox') + .should('contain.text', 'Germany') + .should('contain.text', 'Algeria') + .should('contain.text', 'Niger') + .should('contain.text', 'Nigeria'); + + cy.get('@textbox').clear(); + cy.findByRole('listbox').should('not.exist'); + + cy.get('@textbox').type('IND'); + cy.findByRole('listbox') + .should('contain.text', 'Indonesia') + .should('contain.text', 'India'); + + cy.get('@textbox').clear(); + cy.findByRole('listbox').should('not.exist'); + + cy.get('@textbox').type('Uni'); + cy.findByRole('listbox') + .should('contain.text', 'Réunion') + .should('contain.text', 'Tunisia') + .should('contain.text', 'United Arab Emirates') + .should('contain.text', 'United Kingdom'); + }); + }); + + it('allows the user to select and deselect countries', () => { + withinMonetizationSection(() => { + cy.findByRole('combobox') + .as('selections') + .findByLabelText('France') + .should('not.exist'); + cy.get('@selections').findByLabelText('Canada').should('exist'); + cy.get('@selections').findByLabelText('United States').should('exist'); + + cy.findByLabelText('Enabled countries for targeting').type('Fra'); + cy.findByRole('listbox').findByText('France').click(); + cy.get('@selections').findByLabelText('France').should('exist'); + cy.get('@selections').findByLabelText('Canada').should('exist'); + cy.get('@selections').findByLabelText('United States').should('exist'); + + cy.findByRole('button', { name: 'Remove Canada' }).click(); + cy.get('@selections').findByLabelText('France').should('exist'); + cy.get('@selections').findByLabelText('Canada').should('not.exist'); + cy.get('@selections').findByLabelText('United States').should('exist'); + + cy.findByRole('button', { name: 'Update Settings' }).click(); + }); + + cy.findByTestId('snackbar') + .should('be.visible') + .should('have.text', 'Successfully updated settings.'); + + cy.reload(); + withinMonetizationSection(() => { + cy.findByRole('combobox').as('selections'); + cy.get('@selections').findByLabelText('France').should('exist'); + cy.get('@selections').findByLabelText('Canada').should('not.exist'); + cy.get('@selections').findByLabelText('United States').should('exist'); + }); + }); + + it('allows the user to enable and disable region targeting', () => { + const toggle = (country, { expectedText }) => { + cy.get('@selections') + .findByRole('group', { name: country }) + .findByRole('button', { name: 'Toggle region targeting' }) + .click(); + + cy.get('@selections') + .findByRole('group', { name: country }) + .should('contain.text', expectedText); + }; + + withinMonetizationSection(() => { + cy.findByRole('combobox').as('selections'); + cy.get('@selections') + .findByRole('group', { name: 'United States' }) + .should('contain.text', 'Including regions'); + cy.get('@selections') + .findByRole('group', { name: 'Canada' }) + .should('contain.text', 'Including regions'); + + toggle('United States', { expectedText: 'Excluding regions' }); + toggle('United States', { expectedText: 'Including regions' }); + + toggle('Canada', { expectedText: 'Excluding regions' }); + + cy.findByRole('button', { name: 'Update Settings' }).click(); + }); + + cy.findByTestId('snackbar') + .should('be.visible') + .should('have.text', 'Successfully updated settings.'); + + cy.reload(); + withinMonetizationSection(() => { + cy.findByRole('combobox').as('selections'); + cy.get('@selections') + .findByRole('group', { name: 'United States' }) + .should('contain.text', 'Including regions'); + cy.get('@selections') + .findByRole('group', { name: 'Canada' }) + .should('contain.text', 'Excluding regions'); + }); + }); + }); +}); diff --git a/package.json b/package.json index a02cd1befe252..bf25fd1ba6232 100644 --- a/package.json +++ b/package.json @@ -108,6 +108,7 @@ "@babel/plugin-proposal-class-properties": "^7.16.0", "@babel/plugin-proposal-export-default-from": "^7.22.5", "@babel/plugin-syntax-dynamic-import": "^7.8.3", + "@babel/plugin-transform-private-methods": "^7.22.5", "@babel/plugin-transform-react-jsx": "^7.22.5", "@babel/preset-env": "^7.22.10", "@babel/preset-typescript": "^7.22.5", diff --git a/spec/models/billboard_spec.rb b/spec/models/billboard_spec.rb index 1f9a4f71e8e1e..1ef2969580427 100644 --- a/spec/models/billboard_spec.rb +++ b/spec/models/billboard_spec.rb @@ -361,7 +361,7 @@ it "does not permit them" do expect(billboard).not_to be_valid - expect(billboard.errors_as_sentence).to include("NOT-REAL is not a supported ISO 3166-2 code") + expect(billboard.errors_as_sentence).to include("NOT-REAL is not an enabled target ISO 3166-2 code") end end @@ -370,7 +370,45 @@ it "does not permit them" do expect(billboard).not_to be_valid - expect(billboard.errors_as_sentence).to include("CA-FAKE is not a supported ISO 3166-2 code") + expect(billboard.errors_as_sentence).to include("CA-FAKE is not an enabled target ISO 3166-2 code") + end + end + + context "with valid ISO 3166-2 country codes that are not enabled" do + let(:geo_input) { %w[MX NG] } + + it "does not permit them" do + expect(billboard).not_to be_valid + expect(billboard.errors_as_sentence).to include("MX is not an enabled target ISO 3166-2 code") + expect(billboard.errors_as_sentence).to include("NG is not an enabled target ISO 3166-2 code") + end + end + + context "with countries that don't have region targeting enabled" do + let(:geo_input) { %w[FR-BRE NL GB US-CA] } + let(:enabled_countries) do + { + "US" => :with_regions, + "FR" => :without_regions, + "NL" => :without_regions, + "GB" => :without_regions + } + end + + it "permits country-level targets but not region-level ones" do + allow(Settings::General).to receive(:billboard_enabled_countries).and_return(enabled_countries) + + expect(billboard).not_to be_valid + expect(billboard.errors_as_sentence).to include("FR-BRE is not an enabled target ISO 3166-2 code") + + # Remove faulty region targeting + billboard.target_geolocations = %w[FR NL GB US-CA] + expect(billboard).to be_valid + + # Allow French region targeting + enabled_countries["FR"] = :with_regions + billboard.target_geolocations = geo_input + expect(billboard).to be_valid end end end diff --git a/spec/models/geolocation_spec.rb b/spec/models/geolocation_spec.rb index 2eceadb6376f2..703da3f4f84cf 100644 --- a/spec/models/geolocation_spec.rb +++ b/spec/models/geolocation_spec.rb @@ -3,15 +3,15 @@ RSpec.describe Geolocation do describe "validations" do describe "country validation" do - it "allows only the US and Canada (for now)" do - united_states = described_class.new("US") - canada = described_class.new("CA") - netherlands = described_class.new("NL") - india = described_class.new("IN") - south_africa = described_class.new("ZA") - unassigned_country = described_class.new("ZZ") - invalid_code_country = described_class.new("not iso3166") - + let(:united_states) { described_class.new("US") } + let(:canada) { described_class.new("CA") } + let(:netherlands) { described_class.new("NL") } + let(:india) { described_class.new("IN") } + let(:south_africa) { described_class.new("ZA") } + let(:unassigned_country) { described_class.new("ZZ") } + let(:invalid_code_country) { described_class.new("not iso3166") } + + it "allows only the US and Canada by default" do expect(united_states).to be_valid expect(canada).to be_valid expect(netherlands).not_to be_valid @@ -20,27 +20,68 @@ expect(unassigned_country).not_to be_valid expect(invalid_code_country).not_to be_valid end + + it "respects the specified enabled locations in the settings" do + allow(Settings::General).to receive(:billboard_enabled_countries).and_return( + "CA" => :without_regions, + "IN" => :without_regions, + "ZA" => :without_regions, + ) + + expect(united_states).not_to be_valid + expect(canada).to be_valid + expect(netherlands).not_to be_valid + expect(india).to be_valid + expect(south_africa).to be_valid + expect(unassigned_country).not_to be_valid + expect(invalid_code_country).not_to be_valid + end end describe "region validation" do + # WA: State of Washington (within the US) + # QC: Province of Québec (within Canada) + let(:us_with_us_region) { described_class.new("US", "WA") } + let(:canada_with_canada_region) { described_class.new("CA", "QC") } + let(:us_with_canada_region) { described_class.new("US", "QC") } + let(:canada_with_us_region) { described_class.new("CA", "WA") } + it "allows an empty region" do without_region = described_class.new("US") expect(without_region).to be_valid end it "allows only regions that are a part of the country" do - # WA: State of Washington (within the US) - # QC: Province of Québec (within Canada) - us_with_us_region = described_class.new("US", "WA") - canada_with_canada_region = described_class.new("CA", "QC") - us_with_canada_region = described_class.new("US", "QC") - canada_with_us_region = described_class.new("CA", "WA") - expect(us_with_us_region).to be_valid expect(canada_with_canada_region).to be_valid expect(us_with_canada_region).not_to be_valid expect(canada_with_us_region).not_to be_valid end + + # rubocop:disable RSpec/NestedGroups + context "when the country does not allow region targeting" do + before do + allow(Settings::General).to receive(:billboard_enabled_countries).and_return( + "US" => :without_regions, + "CA" => :with_regions, + ) + end + + it "allows regions for the country by default (for querying)" do + expect(us_with_us_region).to be_valid + expect(canada_with_canada_region).to be_valid + expect(us_with_canada_region).not_to be_valid + expect(canada_with_us_region).not_to be_valid + end + + it "does not allow regions for the country in a targeting context" do + expect(us_with_us_region).not_to be_valid(:targeting) + expect(canada_with_canada_region).to be_valid(:targeting) + expect(us_with_canada_region).not_to be_valid(:targeting) + expect(canada_with_us_region).not_to be_valid(:targeting) + end + end + # rubocop:enable RSpec/NestedGroups end end diff --git a/spec/models/settings/general_spec.rb b/spec/models/settings/general_spec.rb index e892d13d1dd67..3c626367af782 100644 --- a/spec/models/settings/general_spec.rb +++ b/spec/models/settings/general_spec.rb @@ -49,5 +49,41 @@ expect { described_class.feed_pinned_article_id = article.id }.not_to raise_error end end + + describe "validating :billboard_enabled_countries" do + it "does not accept non-hash or empty values" do + expect { described_class.billboard_enabled_countries = "string" }.to raise_error(ActiveRecord::RecordInvalid) + expect { described_class.billboard_enabled_countries = [] }.to raise_error(ActiveRecord::RecordInvalid) + expect { described_class.billboard_enabled_countries = {} }.to raise_error(ActiveRecord::RecordInvalid) + end + + it "accepts any valid ISO 3166-2 codes as keys" do + countries = ISO3166::Country.codes.sample(5).index_with { :with_regions } + + expect { described_class.billboard_enabled_countries = countries }.not_to raise_error + end + + it "does not accept invalid ISO 3166-2 codes as keys" do + countries = { "XX" => :with_regions, "ZZ" => :with_regions } + + expect { described_class.billboard_enabled_countries = countries }.to raise_error(ActiveRecord::RecordInvalid) + end + + it "allows 'with_regions' and 'without_regions' marker as values to enable/disable region targeting" do + countries = { "CA" => :with_regions, "ZA" => :with_regions, "GB" => :without_regions } + + expect { described_class.billboard_enabled_countries = countries }.not_to raise_error + end + + it "does not allow arbitrary strings or symbols as values" do + countries = { "CA" => :with_regions, "US" => "string" } + other_countries = { "CA" => :with_regions, "US" => :string } + + expect { described_class.billboard_enabled_countries = countries }.to raise_error(ActiveRecord::RecordInvalid) + expect do + described_class.billboard_enabled_countries = other_countries + end.to raise_error(ActiveRecord::RecordInvalid) + end + end end end diff --git a/spec/queries/billboards/filtered_ads_query_spec.rb b/spec/queries/billboards/filtered_ads_query_spec.rb index 4e6a61c75abcd..855fc49e4115c 100644 --- a/spec/queries/billboards/filtered_ads_query_spec.rb +++ b/spec/queries/billboards/filtered_ads_query_spec.rb @@ -284,6 +284,38 @@ def filter_billboards(**options) targets_maine_alberta_and_ontario, ) end + + it "shows only billboards with no targeting if the user's location is an unsupported country" do + filtered = filter_billboards(location: "FR-BRE") + expect(filtered).to include(no_targets) + expect(filtered).not_to include( + targets_canada, + targets_new_york_and_canada, + targets_california_and_texas, + targets_quebec_and_newfoundland, + targets_maine_alberta_and_ontario, + ) + end + + it "correctly shows targeted billboards if country but not region targeting is enabled" do + allow(Settings::General).to receive(:billboard_enabled_countries).and_return( + "FR" => :without_regions, + "US" => :with_regions, + "CA" => :with_regions, + ) + + targets_france_and_canada = create_billboard(target_geolocations: "CA, FR") + + filtered = filter_billboards(location: "FR-BRE") + expect(filtered).to include(no_targets, targets_france_and_canada) + expect(filtered).not_to include( + targets_canada, + targets_new_york_and_canada, + targets_california_and_texas, + targets_quebec_and_newfoundland, + targets_maine_alberta_and_ontario, + ) + end end end end diff --git a/spec/requests/api/v1/billboards_spec.rb b/spec/requests/api/v1/billboards_spec.rb index 46079703387bb..e0887db11e0b0 100644 --- a/spec/requests/api/v1/billboards_spec.rb +++ b/spec/requests/api/v1/billboards_spec.rb @@ -94,7 +94,7 @@ expect(response).to have_http_status(:unprocessable_entity) expect(response.media_type).to eq("application/json") expect(response.parsed_body.keys).to contain_exactly("error", "status") - expect(response.parsed_body["error"]).to include("US-FAKE is not a supported ISO 3166-2 code") + expect(response.parsed_body["error"]).to include("US-FAKE is not an enabled target ISO 3166-2 code") end end @@ -158,7 +158,7 @@ expect(response).to have_http_status(:unprocessable_entity) expect(response.media_type).to eq("application/json") expect(response.parsed_body.keys).to contain_exactly("error", "status") - expect(response.parsed_body["error"]).to include("US-FAKE is not a supported ISO 3166-2 code") + expect(response.parsed_body["error"]).to include("US-FAKE is not an enabled target ISO 3166-2 code") end end