Skip to content

[user model] alpha1 fixes (round 4) #1201

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 6, 2023
Merged

[user model] alpha1 fixes (round 4) #1201

merged 5 commits into from
Jan 6, 2023

Conversation

@nan-li
Copy link
Contributor

@nan-li nan-li commented Jan 6, 2023 •
edited
Loading

Description

One Line Summary

More fixes (round 4).

Details

  • printing and toDictionary fixes for push subscription state and permission state.
  • push subscription store should not remove user defaults when user will change. We were getting a bug not able to init the user from cache due to missing push sub model
  • use 409 response code to Identify User to make the switch to that user

Manual testing

iPhone 13 physical on staging and production.

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

nan-li added 4 commits January 6, 2023 02:10
@nan-li
permission + subscription state: description/toDictionary
c26fd7b 
* Before, printing a permission state or subscription state gave good information. Let's make sure they have the `description` method overridden still.
@nan-li
push sub store won't remove models from user defaults
3d2818c 
* As a model store, the push subscription model store was also clearing user defaults when the user will change. Don't do that, but all other stores still will.
@nan-li
[nits]
a5808c4
@nan-li
explicitly check for 409 on Identify User response
e1ed60a 
* We were switching to the user when the Identify User request returns any failure, but we should really check that the code is `409` to go down this path. It could be some other error.
@nan-li nan-li changed the base branch from main to major_release_5.0.0 January 6, 2023 11:36
@nan-li
no longer use a staging app
c0e7b8f 
* We already pointed requests to production
* But now revert the app back to bundle ID `com.onesignal.example`
@nan-li nan-li force-pushed the user_model/alpha_1_round_4 branch from 88845bb to c0e7b8f Compare January 6, 2023 20:20
@nan-li nan-li merged commit 61ad43c into major_release_5.0.0 Jan 6, 2023
@nan-li nan-li deleted the user_model/alpha_1_round_4 branch January 6, 2023 20:45
nan-li added a commit that referenced this pull request Oct 30, 2023
@nan-li
Merge pull request #1201 from OneSignal/user_model/alpha_1_round_4
ad3a1c0 
[user model] alpha1 fixes (round 4)
nan-li added a commit that referenced this pull request Oct 30, 2023
@nan-li
Merge pull request #1201 from OneSignal/user_model/alpha_1_round_4
f498ecd 
[user model] alpha1 fixes (round 4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fhboswell fhboswell Awaiting requested review from fhboswell

@jennantilla jennantilla Awaiting requested review from jennantilla

1 more reviewer

@emawby emawby emawby approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nan-li @emawby