maverick/docbrown
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Update billboard API to support target geolocations (#19857)

Browse source 
* update params to permit target_geolocations

* API docs

* fix param and JSON schema issues

* update docs again
This commit is contained in:
PJ 2023-08-10 19:11:30 +01:00 • committed by GitHub
parent 73927b32f2
commit 9725160709
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 533 additions and 484 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
app/controllers/api/v1/billboards_controller.rb
View file

@ -4,6 +4,9 @@ module Api
before_action :authenticate_with_api_key!
before_action :require_admin
# Enums (like the `display_to` field) raise ArgumentError exceptions on unexpected inputs
rescue_from ArgumentError, with: :error_unprocessable_entity
def index
@billboards = DisplayAd.order(id: :desc).page(params[:page]).per(50)
@billboards = @billboards.search_ads(params[:search]) if params[:search].present?
@ -17,20 +20,14 @@ module Api
def create
@billboard = DisplayAd.new(permitted_params)
result = @billboard.save
render json: @billboard, status: (result ? :ok : :unprocessable_entity)
rescue ArgumentError => e
# enums raise ArgumentError exceptions on unexpected inputs!
render json: { error: e }, status: :unprocessable_entity
@billboard.save!
render json: @billboard, status: :created
end
def update
@billboard = DisplayAd.find(params[:id])
result = @billboard.update(permitted_params)
render json: @billboard, status: (result ? :ok : :unprocessable_entity)
rescue ArgumentError => e
# enums raise ArgumentError exceptions on unexpected inputs!
render json: { error: e }, status: :unprocessable_entity
@billboard.update!(permitted_params)
render json: @billboard
end
def unpublish
@ -53,8 +50,9 @@ module Api
params.permit :approved, :body_markdown, :creator_id, :display_to,
:name, :organization_id, :placement_area, :published,
:tag_list, :type_of, :exclude_article_ids,
:audience_segment_type, :audience_segment_id,
:priority
:audience_segment_type, :audience_segment_id, :priority,
# Permitting twice allows both comma-separated string and array values
:target_geolocations, target_geolocations: []
end
end
end

5
app/models/display_ad.rb
View file

@ -160,9 +160,10 @@ class DisplayAd < ApplicationRecord
overrides = {
"audience_segment_type" => audience_segment_type,
"tag_list" => cached_tag_list,
"exclude_article_ids" => exclude_article_ids.join(",")
"exclude_article_ids" => exclude_article_ids.join(","),
"target_geolocations" => target_geolocations.map(&:to_iso3166)
}
super(options.merge(except: %i[tags tag_list])).merge(overrides)
super(options.merge(except: %i[tags tag_list target_geolocations])).merge(overrides)
end
# rubocop:enable Style/OptionHash

72
spec/requests/api/v1/billboards_spec.rb
View file

@ -4,7 +4,7 @@ RSpec.describe "Api::V1::Billboards" do
let!(:v1_headers) { { "content-type" => "application/json", "Accept" => "application/vnd.forem.api-v1+json" } }
let(:organization) { create(:organization) }
let(:display_ad_params) do
let(:billboard_params) do
{
name: "This is new",
organization_id: organization.id,
@ -13,7 +13,8 @@ RSpec.describe "Api::V1::Billboards" do
body_markdown: "## This ad is a new ad.\n\nYay!",
type_of: "community",
published: true,
approved: true
approved: true,
target_geolocations: "US-WA, CA-BC"
}
end
let!(:ad1) { create(:display_ad, published: true, approved: true, type_of: "in_house") }
@ -39,9 +40,9 @@ RSpec.describe "Api::V1::Billboards" do
describe "POST /api/billboards" do
it "creates a new display_ad" do
post api_billboards_path, params: display_ad_params.to_json, headers: auth_header
post api_billboards_path, params: billboard_params.to_json, headers: auth_header
expect(response).to have_http_status(:success)
expect(response).to have_http_status(:created)
expect(response.media_type).to eq("application/json")
expect(response.parsed_body.keys).to \
contain_exactly("approved", "body_markdown", "cached_tag_list",
@ -52,14 +53,48 @@ RSpec.describe "Api::V1::Billboards" do
"creator_id", "exclude_article_ids",
"audience_segment_type", "audience_segment_id",
"priority", "target_geolocations")
expect(response.parsed_body["target_geolocations"]).to contain_exactly("US-WA", "CA-BC")
end
it "returns a malformed response" do
post api_billboards_path, params: display_ad_params.merge(display_to: "steve").to_json, headers: auth_header
it "also accepts target geolocations as an array" do
post api_billboards_path,
params: billboard_params.merge(target_geolocations: %w[US-WA CA-BC]).to_json,
headers: auth_header
expect(response).to have_http_status(:created)
expect(response.media_type).to eq("application/json")
expect(response.parsed_body.keys).to \
contain_exactly("approved", "body_markdown", "cached_tag_list",
"clicks_count", "created_at", "display_to", "id",
"impressions_count", "name", "organization_id",
"placement_area", "processed_html", "published",
"success_rate", "tag_list", "type_of", "updated_at",
"creator_id", "exclude_article_ids",
"audience_segment_type", "audience_segment_id",
"priority", "target_geolocations")
expect(response.parsed_body["target_geolocations"]).to contain_exactly("US-WA", "CA-BC")
end
it "returns a malformed response with invalid display_to" do
post api_billboards_path, params: billboard_params.merge(display_to: "steve").to_json, headers: auth_header
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")
expect(response.parsed_body.keys).to contain_exactly("error", "status")
expect(response.parsed_body["error"]).to include("'steve' is not a valid display_to")
end
it "returns a malformed response with invalid geolocation" do
expect do
post api_billboards_path,
params: billboard_params.merge(target_geolocations: "US-FAKE").to_json,
headers: auth_header
end.not_to change(DisplayAd, :count)
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")
end
end
@ -84,7 +119,7 @@ RSpec.describe "Api::V1::Billboards" do
describe "PUT /api/billboards/:id" do
it "updates an existing display_ad" do
put api_billboard_path(ad1.id),
params: display_ad_params.merge(name: "Updated!", type_of: "external").to_json,
params: billboard_params.merge(name: "Updated!", type_of: "external").to_json,
headers: auth_header
expect(response).to have_http_status(:success)
@ -102,6 +137,27 @@ RSpec.describe "Api::V1::Billboards" do
"audience_segment_type", "audience_segment_id",
"priority", "target_geolocations")
end
it "also accepts target geolocations as an array" do
put api_billboard_path(ad1.id), params: { target_geolocations: %w[US-FL US-GA] }.to_json, headers: auth_header
expect(response).to have_http_status(:success)
expect(response.media_type).to eq("application/json")
ad1.reload
expect(ad1.target_geolocations).to contain_exactly(
Geolocation.from_iso3166("US-FL"),
Geolocation.from_iso3166("US-GA"),
)
end
it "returns a malformed response with invalid geolocation" do
put api_billboard_path(ad1.id), params: { target_geolocations: "US-FAKE" }.to_json, headers: auth_header
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")
end
end
describe "PUT /api/billboards/:id/unpublish" do

43
spec/requests/api/v1/docs/display_ads_spec.rb
View file

@ -13,11 +13,11 @@ RSpec.describe "api/v1/display_ads" do
before { user.add_role(:admin) }
path "/api/display_ads" do
describe "get all display ads" do
get("display ads") do
describe "GET /display_ads" do
get("Billboards") do
tags "display ads"
description(<<-DESCRIBE.strip)
This endpoint allows the client to retrieve a list of all display ads.
This endpoint allows the client to retrieve a list of all billboards.
DESCRIBE
produces "application/json"
@ -40,11 +40,11 @@ RSpec.describe "api/v1/display_ads" do
end
end
describe "create a new display ad" do
post("display ads") do
describe "POST /display_ads" do
post "Create a billboard" do
tags "display ads"
description(<<-DESCRIBE.strip)
This endpoint allows the client to create a new display ad.
This endpoint allows the client to create a new billboard.
DESCRIBE
produces "application/json"
@ -59,13 +59,14 @@ RSpec.describe "api/v1/display_ads" do
display_to: "all",
approved: true,
published: true,
placement_area: placement_area
placement_area: placement_area,
target_geolocations: "US-WA, CA-BC"
}
end
let(:placement_area) { "post_comments" }
response(200, "successful") do
response "201", "A billboard" do
schema type: :object,
items: { "$ref": "#/components/schemas/DisplayAd" }
let(:"api-key") { api_secret.secret }
@ -93,18 +94,18 @@ RSpec.describe "api/v1/display_ads" do
end
path "/api/display_ads/{id}" do
describe "GET a single display ad" do
get("display ad") do
describe "GET /display_ads/:id" do
get "A billboard (by id)" do
tags "display ads"
description(<<-DESCRIBE.strip)
This endpoint allows the client to retrieve a single display ad, via its id.
This endpoint allows the client to retrieve a single billboard, via its id.
DESCRIBE
produces "application/json"
parameter name: :id,
in: :path,
required: true,
description: "The ID of the display ad.",
description: "The ID of the billboard.",
schema: {
type: :integer,
format: :int32,
@ -128,7 +129,7 @@ RSpec.describe "api/v1/display_ads" do
run_test!
end
response "404", "Unknown DisplayAd ID" do
response "404", "Unknown Billboard ID" do
let(:"api-key") { api_secret.secret }
let(:id) { 10_000 }
add_examples
@ -138,11 +139,11 @@ RSpec.describe "api/v1/display_ads" do
end
end
describe "PUT update an ad" do
put("display ads") do
describe "PUT /display_ads/:id" do
put "Update a billboard by ID" do
tags "display ads"
description(<<-DESCRIBE.strip)
This endpoint allows the client to update the attributes of a single display ad, via its id.
This endpoint allows the client to update the attributes of a single billboard, via its id.
DESCRIBE
produces "application/json"
@ -151,7 +152,7 @@ RSpec.describe "api/v1/display_ads" do
parameter name: :id,
in: :path,
required: true,
description: "The ID of the display ad.",
description: "The ID of the billboard.",
schema: {
type: :integer,
format: :int32,
@ -194,11 +195,11 @@ RSpec.describe "api/v1/display_ads" do
end
path "/api/display_ads/{id}/unpublish" do
describe "PUT to unpublish an ad" do
put("unpublish") do
describe "PUT /display_ads/:id/unpublish" do
put "Unpublish a billboard" do
tags "display ads"
description(<<-DESCRIBE.strip)
This endpoint allows the client to remove a display ad from rotation by un-publishing it.
This endpoint allows the client to remove a billboard from rotation by un-publishing it.
DESCRIBE
produces "application/json"
@ -206,7 +207,7 @@ RSpec.describe "api/v1/display_ads" do
parameter name: :id,
in: :path,
required: true,
description: "The ID of the display ad to unpublish.",
description: "The ID of the billboard to unpublish.",
schema: {
type: :integer,
format: :int32,

5
spec/swagger_helper.rb
View file

@ -412,7 +412,7 @@ The default maximum value can be overridden by \"API_PER_PAGE_MAX\" environment
placement_area: { type: :string, enum: DisplayAd::ALLOWED_PLACEMENT_AREAS,
description: "Identifies which area of site layout the ad can appear in" },
tag_list: { type: :string, description: "Tags on which this ad can be displayed (blank is all/any tags)" },
article_exclude_ids: { type: :string,
exclude_article_ids: { type: :string,
nullable: true,
description: "Articles this ad should *not* appear on (blank means no articles are disallowed, and this ad can appear next to any/all articles). Comma-separated list of integer Article IDs" }, # rubocop:disable Layout/LineLength
audience_segment_id: { type: :integer,
@ -420,6 +420,9 @@ The default maximum value can be overridden by \"API_PER_PAGE_MAX\" environment
audience_segment_type: { type: :string,
enum: AudienceSegment.type_ofs.keys,
description: "Specifies a group of users who will see this billboard (must match audience_segment_id if both provided)" },
target_geolocations: { type: :array,
items: { type: :string },
description: "Locations to show this billboard in (blank means it will be shown in all locations). Specified as a comma-separated list or array of ISO 3166-2 country and optionally region codes)" },
display_to: { type: :string, enum: DisplayAd.display_tos.keys, default: "all",
description: "Potentially limits visitors to whom the ad is visible" },
type_of: { type: :string, enum: DisplayAd.type_ofs.keys, default: "in_house",

870
swagger/v1/api_v1.json
View file

File diff suppressed because it is too large Load diff