From 0cdcf30c3930625dc3614fbcd5407e8e4e7956ce Mon Sep 17 00:00:00 2001 From: Muhsin <12408980+muhsin-k@users.noreply.github.com> Date: Wed, 3 Jun 2026 13:24:30 +0400 Subject: [PATCH] fix(app-store): address follow-up review comments --- .../fetch_app_store_review_inboxes_job.rb | 13 +++--- app/models/channel/app_store.rb | 10 ++--- .../app_store/send_on_app_store_service.rb | 11 +---- app/services/app_store_connect/client.rb | 27 ++++++++++-- ...age_reply_to_flag_for_channel_app_store.rb | 1 - .../send_on_app_store_service_spec.rb | 4 +- .../services/app_store_connect/client_spec.rb | 44 +++++++++++++++++-- 7 files changed, 78 insertions(+), 32 deletions(-) diff --git a/app/jobs/inboxes/fetch_app_store_review_inboxes_job.rb b/app/jobs/inboxes/fetch_app_store_review_inboxes_job.rb index fc7b079d3..502c64889 100644 --- a/app/jobs/inboxes/fetch_app_store_review_inboxes_job.rb +++ b/app/jobs/inboxes/fetch_app_store_review_inboxes_job.rb @@ -2,12 +2,15 @@ class Inboxes::FetchAppStoreReviewInboxesJob < ApplicationJob queue_as :scheduled_jobs def perform - Inbox.where(channel_type: 'Channel::AppStore').find_each(batch_size: 100) do |inbox| - next if inbox.account.suspended? - next unless inbox.account.feature_enabled?(:channel_app_store) - next unless inbox.channel.sync_due? + Inbox.includes(:account, :channel).where(channel_type: 'Channel::AppStore').find_each(batch_size: 100) do |inbox| + account = inbox.account + channel = inbox.channel - ::Inboxes::FetchAppStoreReviewsJob.perform_later(inbox.channel) + next if account.suspended? + next unless account.feature_enabled?(:channel_app_store) + next unless channel.sync_due? + + ::Inboxes::FetchAppStoreReviewsJob.perform_later(channel) end end end diff --git a/app/models/channel/app_store.rb b/app/models/channel/app_store.rb index 486a9d02a..1173976af 100644 --- a/app/models/channel/app_store.rb +++ b/app/models/channel/app_store.rb @@ -32,15 +32,11 @@ class Channel::AppStore < ApplicationRecord end def fetch_reviews - app_store_client.fetch_reviews + app_store_client.fetch_reviews(since: last_synced_at) end - def reply_to_review(review_id, response_body, response_id: nil) - response = if response_id.present? - app_store_client.update_review_response(review_id, response_body) - else - app_store_client.create_review_response(review_id, response_body) - end + def reply_to_review(review_id, response_body) + response = app_store_client.create_or_update_review_response(review_id, response_body) response['id'] end diff --git a/app/services/app_store/send_on_app_store_service.rb b/app/services/app_store/send_on_app_store_service.rb index c3f6a7f6b..ab5f7418d 100644 --- a/app/services/app_store/send_on_app_store_service.rb +++ b/app/services/app_store/send_on_app_store_service.rb @@ -8,7 +8,7 @@ class AppStore::SendOnAppStoreService < Base::SendOnChannelService def perform_reply validate_feature_enabled! validate_message_support! - source_id = channel.reply_to_review(review_id, reply_content, response_id: existing_response_id) + source_id = channel.reply_to_review(review_id, reply_content) message.update!(source_id: source_id) if source_id.present? Messages::StatusUpdateService.new(message, 'delivered').perform rescue StandardError => e @@ -33,13 +33,4 @@ class AppStore::SendOnAppStoreService < Base::SendOnChannelService def reply_content message.outgoing_content.presence || message.content end - - def existing_response_id - message.conversation.messages - .outgoing - .where.not(id: message.id) - .where.not(source_id: [nil, '']) - .order(created_at: :desc) - .pick(:source_id) - end end diff --git a/app/services/app_store_connect/client.rb b/app/services/app_store_connect/client.rb index ea21db4ac..f4d2a84b5 100644 --- a/app/services/app_store_connect/client.rb +++ b/app/services/app_store_connect/client.rb @@ -7,14 +7,18 @@ class AppStoreConnect::Client get("/v1/apps/#{channel.app_id}")['data'] end - def fetch_reviews + def fetch_reviews(since: nil) reviews = [] next_url = nil loop do payload = next_url ? get_url(next_url) : get(reviews_path, reviews_query) included = Array(payload['included']) - reviews.concat(Array(payload['data']).map { |review| normalize_review(review, included) }) + review_payloads = Array(payload['data']).map { |review| normalize_review(review, included) } + fresh_payloads = fresh_review_payloads(review_payloads, since) + reviews.concat(fresh_payloads) + + break if since.present? && fresh_payloads.size < review_payloads.size next_url = payload.dig('links', 'next') break if next_url.blank? @@ -24,10 +28,10 @@ class AppStoreConnect::Client end def create_review_response(review_id, response_body) - post('/v1/customerReviewResponses', review_response_payload(review_id, response_body))['data'] + create_or_update_review_response(review_id, response_body) end - def update_review_response(review_id, response_body) + def create_or_update_review_response(review_id, response_body) post('/v1/customerReviewResponses', review_response_payload(review_id, response_body))['data'] end @@ -55,6 +59,21 @@ class AppStoreConnect::Client } end + def fresh_review_payloads(review_payloads, since) + return review_payloads if since.blank? + + review_payloads.select { |review_payload| review_created_after?(review_payload, since) } + end + + def review_created_after?(review_payload, since) + created_at = Time.zone.parse(review_payload.dig('review', 'attributes', 'createdDate').to_s) + return true if created_at.blank? + + created_at > since + rescue StandardError + true + end + def get(path, query = {}) request(:get, "#{Channel::AppStore::API_BASE_URL}#{path}", query: query) end diff --git a/db/migrate/20260522080000_repurpose_message_reply_to_flag_for_channel_app_store.rb b/db/migrate/20260522080000_repurpose_message_reply_to_flag_for_channel_app_store.rb index 191bb7266..3af1faac7 100644 --- a/db/migrate/20260522080000_repurpose_message_reply_to_flag_for_channel_app_store.rb +++ b/db/migrate/20260522080000_repurpose_message_reply_to_flag_for_channel_app_store.rb @@ -15,6 +15,5 @@ class RepurposeMessageReplyToFlagForChannelAppStore < ActiveRecord::Migration[7. config.value = config.value.reject { |feature| feature['name'] == 'message_reply_to' } config.save! - GlobalConfig.clear_cache end end diff --git a/spec/services/app_store/send_on_app_store_service_spec.rb b/spec/services/app_store/send_on_app_store_service_spec.rb index 854aa04e3..ae073017f 100644 --- a/spec/services/app_store/send_on_app_store_service_spec.rb +++ b/spec/services/app_store/send_on_app_store_service_spec.rb @@ -26,7 +26,7 @@ RSpec.describe AppStore::SendOnAppStoreService do described_class.new(message: message).perform - expect(channel).to have_received(:reply_to_review).with('review-1', 'Thanks', response_id: nil) + expect(channel).to have_received(:reply_to_review).with('review-1', 'Thanks') expect(message.reload.source_id).to eq('response-1') expect(Messages::StatusUpdateService).to have_received(:new).with(message, 'delivered') end @@ -40,7 +40,7 @@ RSpec.describe AppStore::SendOnAppStoreService do described_class.new(message: message).perform - expect(channel).to have_received(:reply_to_review).with('review-1', 'Updated reply', response_id: 'response-1') + expect(channel).to have_received(:reply_to_review).with('review-1', 'Updated reply') expect(Messages::StatusUpdateService).to have_received(:new).with(message, 'delivered') end diff --git a/spec/services/app_store_connect/client_spec.rb b/spec/services/app_store_connect/client_spec.rb index 14dd66e1e..235f2c9e0 100644 --- a/spec/services/app_store_connect/client_spec.rb +++ b/spec/services/app_store_connect/client_spec.rb @@ -43,6 +43,9 @@ RSpec.describe AppStoreConnect::Client do { id: 'review-1', type: 'customerReviews', + attributes: { + createdDate: '2026-05-20T10:00:00-00:00' + }, relationships: { response: { data: { @@ -72,6 +75,41 @@ RSpec.describe AppStoreConnect::Client do expect(review_payload['response']['id']).to eq('response-1') end + it 'stops fetching when a scheduled sync reaches already synced reviews' do + stub_request(:get, 'https://api.appstoreconnect.apple.com/v1/apps/123456789/customerReviews') + .with(query: { include: 'response', limit: '200', sort: '-createdDate' }) + .to_return( + status: 200, + body: { + data: [ + { + id: 'review-1', + type: 'customerReviews', + attributes: { + createdDate: '2026-05-20T10:00:00-00:00' + } + }, + { + id: 'review-2', + type: 'customerReviews', + attributes: { + createdDate: '2026-05-19T10:00:00-00:00' + } + } + ], + links: { + next: 'https://api.appstoreconnect.apple.com/v1/apps/123456789/customerReviews?page=2' + } + }.to_json, + headers: { 'Content-Type' => 'application/json' } + ) + + review_payloads = described_class.new(channel: channel).fetch_reviews(since: Time.zone.parse('2026-05-20T00:00:00-00:00')) + + expect(review_payloads.pluck('review').pluck('id')).to eq(['review-1']) + expect(WebMock).not_to have_requested(:get, 'https://api.appstoreconnect.apple.com/v1/apps/123456789/customerReviews?page=2') + end + it 'fetches a fresh cached token for each request' do first_token_service = instance_double(AppStoreConnect::TokenService, token: 'first-token') second_token_service = instance_double(AppStoreConnect::TokenService, token: 'second-token') @@ -141,8 +179,8 @@ RSpec.describe AppStoreConnect::Client do end end - describe '#update_review_response' do - it 'updates an existing response' do + describe '#create_or_update_review_response' do + it 'creates or updates a response for a review' do stub_request(:post, 'https://api.appstoreconnect.apple.com/v1/customerReviewResponses') .with( body: { @@ -168,7 +206,7 @@ RSpec.describe AppStoreConnect::Client do headers: { 'Content-Type' => 'application/json' } ) - response = described_class.new(channel: channel).update_review_response('review-1', 'Updated response') + response = described_class.new(channel: channel).create_or_update_review_response('review-1', 'Updated response') expect(response['id']).to eq('response-1') end