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

[5.0.0] Differentiate login errors and add logs #1841

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

nan-li
Copy link
Contributor

@nan-li nan-li commented Sep 15, 2023

Description

One Line Summary

Differentiate login failure cases for (1) conflict response and (2) other general 400-based errors, and add a log clarifying expected conflict error.

Details

Motivation

We have had reports that developers are troubled to receive a 409 error response for login

1. Differentiate login failure cases

  • The implementation of login with external_id has not changed in this PR. We still try to create the user on both failures (as before)
  • We add new ExecutionResult type of FAIL_CONFLICT to differentiate this case from general failure case of FAIL_NORETRY

2. Add logs to login failures

  • Seeing a 409 error response can be worrisome for developers, when this response is fine and expected.
  • We cannot remove the 409 error logged so let them know we are handling the response

Example of logs:

D/OneSignal: [OpRepo] LoginUserOperationExecutor(operation: [{"name":"login-user","appId":"APP_ID","onesignalId":"local-xxx","externalId":"abcd","existingOnesignalId":"xxx","id":"1cacee4c-9ecb-45ed-aa6c-a8c0eb47c5b1"}, {"name":"transfer-subscription","appId":"APP_ID","subscriptionId":"yyy","onesignalId":"local-xxx","id":"31216d16-70cd-4194-af7d-539415dc7fdb"}])
D/OneSignal: [OpRepo] IdentityOperationExecutor(operations: [{"name":"set-alias","appId":"APP_ID","onesignalId":"ONESIGNAL_ID","label":"external_id","value":"abcd"}])
D/OneSignal: [DefaultDispatcher-worker-2] HttpClient: PATCH apps/APP_ID/users/by/onesignal_id/xxx/identity - {"identity":{"external_id":"abcd"}}
D/OneSignal: [DefaultDispatcher-worker-2] HttpClient: PATCH apps/APP_ID/users/by/onesignal_id/xxx/identity - FAILED STATUS: 409
W/OneSignal: [DefaultDispatcher-worker-2] HttpClient: PATCH RECEIVED JSON: {"errors":[{"code":"user-2","title":"One or more Aliases claimed by another User","meta":{"external_id":"abcd"}}]}
D/OneSignal: [OpRepo] LoginUserOperationExecutor now handling 409 response with "code": "user-2" by switching to user with "external_id": "abcd"

Scope

There is no implementation detail changes to login. We still handle login failures the same way but log differently when it fails due to a 409 conflict (which is a correct expected response) and another unexpected error response.

Testing

Unit testing

None, we should add tests though.

Manual testing

Android emulator on API 33

  • Tested login to existing external_id, receive a 409, see the log explaining we are handling the case, and see we successfully switch to that user
  • Also tested a login flow that results in a 400 response and see the other error log.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

* The implementation of login with external_id has not changed
* Add new ExecutionResult type of FAIL_CONFLICT to differentiate this case from general failure case of FAIL_NORETRY
* Add logs for both type of responses, but still try to create the user on both failures (as before)
Base automatically changed from user_model/fix_notifications_replacing_each_other to user-model/main September 22, 2023 19:47
@emawby emawby merged commit 9a5c959 into user-model/main Sep 22, 2023
1 of 2 checks passed
@emawby emawby deleted the user_model/add_login_409_logs branch September 22, 2023 19:47
@jennantilla jennantilla mentioned this pull request Sep 25, 2023
jinliu9508 pushed a commit that referenced this pull request Jan 31, 2024
[5.0.0] Differentiate login errors and add logs
jinliu9508 pushed a commit that referenced this pull request Jan 31, 2024
[5.0.0] Differentiate login errors and add logs
jinliu9508 pushed a commit that referenced this pull request Feb 6, 2024
[5.0.0] Differentiate login errors and add logs
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.

2 participants