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

fix(analytics): extend_session parameter #5973

Merged
merged 2 commits into from
Dec 31, 2021
Merged

fix(analytics): extend_session parameter #5973

merged 2 commits into from
Dec 31, 2021

Conversation

phjudge
Copy link
Contributor

@phjudge phjudge commented Dec 29, 2021

Description

This PR modifies the event parameters so that the extend_session parameter is cast to the correct value.

analytics().logEvent('event', { extend_session: 1 })

Motivation

From Firebase documentation (under the heading "Adjust app session timeout"):

An app session begins to time out when an app is moved to the background, but you have the option to extend that session by including an extend_session parameter (with a value of 1) with events you send while the app is in the background. This is useful if your app is frequently used in the background, (e.g. as with navigation and music apps.)

We found in development that the extend_session parameter wasn't being respected through react-native-firebase because they were set to the incorrect values. Android docs specify a long value of 1. iOS docs specify a value of @YES.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

We've used this same code in production, applied via patch-package.

On Android, LogCat shows a helpful message once the value is correctly set:

V/FA: EXTEND_SESSION param attached: initiate a new session or extend the current active session

iOS doesn't show the same confirmation message, but I can confirm through our own app analytics session data that it works.

🔥

@vercel
Copy link

vercel bot commented Dec 29, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/6uffq1LtEfaenCzyy9iaiBDB6xgs
✅ Preview: Canceled

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/5bvPJna8zSsc6DbKxVkn4iYa5Gaw
✅ Preview: https://react-native-firebase-git-fork-phjudge-extend-2962db-invertase.vercel.app

@CLAassistant
Copy link

CLAassistant commented Dec 29, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Dec 30, 2021

Codecov Report

Merging #5973 (44a8466) into main (8efda50) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 44a8466 differs from pull request most recent head cdf83bf. Consider uploading reports for the commit cdf83bf to get more accurate results

@@             Coverage Diff              @@
##               main    #5973      +/-   ##
============================================
- Coverage     53.04%   53.02%   -0.01%     
  Complexity      622      622              
============================================
  Files           208      208              
  Lines         10171    10174       +3     
  Branches       1618     1619       +1     
============================================
  Hits           5394     5394              
- Misses         4523     4526       +3     
  Partials        254      254              

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Interesting! This LGTM - subtle problem, nice fix

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Dec 30, 2021
@mikehardy mikehardy merged commit 23fdf61 into invertase:main Dec 31, 2021
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Dec 31, 2021
@phjudge phjudge deleted the extend-session-param branch January 4, 2022 16:32
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.

3 participants