-
Notifications
You must be signed in to change notification settings - Fork 396
Non Subscription Events Manager #5726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
polmiro
wants to merge
5
commits into
main
Choose a base branch
from
rc-ads-events-manager
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
+1,864
−53
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e63905e to
93994b1
Compare
📸 Snapshot Test7 modified, 872 unchanged
🛸 Powered by Emerge Tools |
f728371 to
fa592ed
Compare
Separates feature event flushing into its own method: - Add flushFeatureEvents to EventsManagerType protocol - Implement flushFeatureEvents with existing flush logic - Update flushEvents to delegate to flushFeatureEvents - Update Purchases.flushPaywallEvents to call flushFeatureEvents - Add flushFeatureEvents to MockEventsManager This prepares EventsManager for supporting multiple event types.
Adds core infrastructure for storing and serializing ad events: - StoredAdEvent: Model for ad events with user context - StoredAdEventSerializer: JSON serialization/deserialization - AdEventStore: File-based persistent storage with fetch/clear operations - AdEventStoreTests: Comprehensive test coverage Clean implementation without legacy migration code since feature is behind compiler flag.
2dbdbb2 to
dc07272
Compare
Implements complete ad events lifecycle in EventsManager: - Add flushAdEvents to EventsManagerType protocol - AdEventStore integration with new initialization parameter - Implement track(adEvent:) with proper serialization and storage - Update flushEvents to flush both feature and ad events - Network request handling via InternalAPI.postAdEvents - Comprehensive error handling and logging (EventsManagerStrings) - Full test coverage (EventsManagerAdEventsTests) - Mock support for ad events in test infrastructure Network layer additions: - AdEventsRequest: HTTP request body for ad events endpoint - PostAdEventsOperation: Network operation for posting ad events - InternalAPI.postAdEvents: Async method for posting ad events
dc07272 to
8c25b9d
Compare
The backend expects `impression_id` but we were sending `ad_instance_id`. This renames the field throughout the codebase: - AdEvent.swift: Update AdEventData protocol and all event classes - AdEventsRequest.swift: Update serialization field name - All test files: Update to use impressionId parameter - Objective-C API: Update parameter names The field will be serialized as `impression_id` via snake_case conversion.
- Add AdEventsIntegrationTests.swift following BaseBackendIntegrationTests pattern - Add internal flushAdEvents() method in Purchases for test access - Add 4 test cases: tracking/flushing, empty flush, clearing after flush, persistence 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
ffaf024 to
396e62c
Compare
|
@RCGitBot please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Integrate non-subscription events tracking into EventsManager with storage, network requests, and comprehensive test coverage. I will do a more advanced flushing as a separate improvement, possibly taking into account the age of the events, when the last flushed happned and/or the number of events stored. For now just following the same pattern used for FeatureEvent.
Changes
AdEventStorefor persisting ad events to diskPostAdEventsOperationandAdEventsRequestfor backend communicationEventsManagerwith flushing logicadInstanceIdtoimpressionIdto match backend schema (fixes 400 errors)Test Coverage
AdEventStoreTests: Storage/serialization (12 tests)AdEventsRequestTests: Network request formatting (6 tests)EventsManagerAdEventsTests: Manager integration (15 tests)PurchasesAdEventsTests: Public API (4 tests)AdEventsIntegrationTests: Backend integration (4 tests)