chore: update Firebase BoM; refactor: auth actions

jfmcquade commented 4 months ago

PR Checklist

[x] PR title descriptive (can be used in release notes)

Description

Updates the Android Firebase library versions, in an attempt to address the warning in #2133.

Also refactors the auth actions to use a new auth namespace:

google_auth action is replaced by auth: sign_in_google
- google_auth syntax is still supported for compatibility, but should be considered deprecated
Adds a new action: auth: sign_out, which signs the user out through Firebase. Currently the only use case for this is debugging

See example_google_auth template below for the actions in use.

Git Issues

Closes #2133

Screenshots

Demo of example_google_auth template:

The initial behaviour on clicking the "Sign in with Google" button was confusing to me (see demo below), especially as it didn't match what I observed when testing the feature as part of reviewing #2517 (see screen recording in this comment). This meant that I spent much longer than I'd like to admit trying to debug the seemingly broken signInWithGoogle functionality, finding it to be also "broken" on master. In fact, the feature was working as expected, but I was already logged in on my emulator and there is no confirmation that the user is logged in. Whilst this lack of feedback is still an issue (although the signed in user is stored in a field), I was able to debug by adding a new auth_sign_out action, which allows me to test the Google auth functionality. The console logs in the demo below are temporary debug logs, now removed.

https://github.com/IDEMSInternational/parenting-app-ui/assets/64838927/1dc097e9-34dc-4d87-9bbf-91c71126e666

jfmcquade commented 4 months ago

@chrismclarke I've updated the PR description with the results of my testing the Firebase Auth functionality. I needed to remove a deprecated dependency in order to get gradle to build the project, see inline comment.

Do you think we need to extensively test all other native features, or should we be reasonably safe to merge?

chrismclarke commented 4 months ago

@chrismclarke I've updated the PR description with the results of my testing the Firebase Auth functionality. I needed to remove a deprecated dependency in order to get gradle to build the project, see inline comment.

Do you think we need to extensively test all other native features, or should we be reasonably safe to merge?

Thanks for testing auth and catching the issue, looking through the changelog that's the only breaking change I can identify as likely to impact, however still worth very quickly testing all native firebase sdks. As it stands that is just:

Authentication Tested working with new BOM

Crashlytics Not exposed in authoring so I just hardcoded a quick exception to log at startup and can confirm working as expected Crashlytics Link

Performance Not exposed, a placeholder PerformanceMonitorService was created when we wanted to analyze some app startup issues people were having but seems like never actually used, so I'd say can ignore and decide in future whether to implement or remove)

So I'd say should be good to go

jfmcquade commented 4 months ago

Thanks @chrismclarke for the review and additional testing. I've updated to include a new auth action namespace in this PR, as I didn't like the idea of putting in a new action with a poor syntax.

@esmeetewinkel I don't think there's any more testing that needs to be done, but I've requested a review just to make you aware of the changes to the auth actions (there legacy syntax is still supported, so there are no breaking changes)

jfmcquade commented 4 months ago

I was wondering whether click | auth: sign_in_google and click | auth: sign_out is inconsistent (i.e. whether it should be sign_out_google). Is it true that, if in future we would have multiple sign in platforms (Google, email, some Apple platform) then the sign ins for the different platforms would be separate actions whereas sign_out signs the user out of any platform they may be signed in to?

This inconsistency does reflect a genuine difference: it is not possible to be signed in by more than one method at a time, the user is either signed in (via some method) or signed out. So whilst we may end up with various sign in actions corresponding to various auth "providers", e.g. sign_in_email, sign_in_apple, there will only ever by one sign_out method which will sign out the user regardless of which method they used to sign in.

IDEMSInternational / open-app-builder