Skip to content
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

feat: use new header to set polling interval #519

Merged
merged 7 commits into from
Feb 15, 2024
Merged

Conversation

BernardGatt
Copy link
Contributor

Updates the polling interval remotely through the new X-Gist-Queue-Polling-Interval header.

Part of: INAPP-12243

@BernardGatt BernardGatt requested a review from a team February 13, 2024 15:01
Copy link

github-actions bot commented Feb 13, 2024

Sample app builds 📱

Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.


  • CocoaPods-FCM: bernard/INAPP-12243 (1707921548)
  • APN-UIKit: bernard/INAPP-12243 (1707921519)

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

Attention: 1173 lines in your changes are missing coverage. Please review.

Comparison is base (9dad7f3) 56.11% compared to head (dac11b1) 56.13%.
Report is 155 commits behind head on main.

Files Patch % Lines
...s/MessagingInApp/Gist/EngineWeb/AnyEncodable.swift 0.00% 158 Missing ⚠️
.../MessagingInApp/Gist/Managers/MessageManager.swift 14.74% 133 Missing ⚠️
...agingInApp/Gist/Managers/MessageQueueManager.swift 25.00% 66 Missing ⚠️
...gingPush/UserNotificationsFramework/Wrappers.swift 0.00% 61 Missing ⚠️
...essagingInApp/Gist/Managers/ModalViewManager.swift 0.00% 60 Missing ⚠️
...sFramework/UserNotificationsFrameworkAdapter.swift 6.25% 60 Missing ⚠️
Sources/MessagingInApp/Gist/Gist.swift 26.25% 59 Missing ⚠️
...rces/MessagingInApp/Gist/EngineWeb/EngineWeb.swift 42.16% 48 Missing ⚠️
...es/MessagingInApp/Gist/Managers/QueueManager.swift 0.00% 44 Missing ⚠️
...gingInApp/Gist/Views/GistModalViewController.swift 0.00% 43 Missing ⚠️
... and 51 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #519      +/-   ##
==========================================
+ Coverage   56.11%   56.13%   +0.01%     
==========================================
  Files         103      132      +29     
  Lines        1153     3718    +2565     
==========================================
+ Hits          647     2087    +1440     
- Misses        506     1631    +1125     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@levibostian
Copy link
Member

I see the lint/formatter CI check is failing. I checked the CI job and I think the lint/formatter is failing in the diff check. Indicating that the formatting you ran on your development machine is different from what the CI generated.

Perhaps your development environment is not up-to-date with the latest tooling this code base uses? If you could double check this doc to see if you have the binny tool installed and updated to the latest version, then run make format on your machine and push up the code then I think the lint errors will go away.

@levibostian
Copy link
Member

In regards to the sample apps not compiling, the CI job fails with error message, "Could not install WWDR certificate".

This is a known issue with the tools that we use to compile our apps. It doesn't happen too often so retrying the CI job takes care of it most of the time.

@@ -31,7 +32,7 @@ enum BaseNetwork {
URLSession.shared.dataTask(with: urlRequest, completionHandler: { data, response, error in
if let error = error { completionHandler(.failure(error)) }
guard let data = data, let response = response as? HTTPURLResponse,
(200 ... 299).contains(response.statusCode)
(200 ... 304).contains(response.statusCode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

304 is content not modified, I know we were relying on it before but were returning failure apparently, so just wanted to check nothing breaks from before right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change, we used to rely on the default NSURLSession's behavior to cache any 304 responses, it used to take 304s and convert them to the latest 200 responses automatically, we can't have that anymore because otherwise, the new header would be part of that cached response.

if let newPollingIntervalString = headers["x-gist-queue-polling-interval"] as? String,
let newPollingInterval = Double(newPollingIntervalString),
newPollingInterval != Gist.shared.messageQueueManager.interval {
DispatchQueue.main.async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need DispatchQueue.main here? because the method setup is also using DispatchQueue.main.asyncAfter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using that because of the polling timer being created in the setup call.

@BernardGatt BernardGatt merged commit 05a1ebd into main Feb 15, 2024
10 checks passed
@BernardGatt BernardGatt deleted the bernard/INAPP-12243 branch February 15, 2024 07:11
github-actions bot pushed a commit that referenced this pull request Feb 15, 2024
## [2.12.0](2.11.1...2.12.0) (2024-02-15)

### Features

* use new header to set polling interval ([#519](#519)) ([05a1ebd](05a1ebd))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants