-
Notifications
You must be signed in to change notification settings - Fork 115
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] Outcomes IndexedDB schema #1087
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This makes changing the table name easier in future commits.
This was a change done in a beta version that shouldn't have as you must make a migration if you want to make a schema change. Added comment when the code was changed and the two different states the indexedDB scehma could be in now.
Bumping indexDB to v5 and added a migration. This commit address the two states NotificationClicked's keyPath can be in due an older migration being changed when it shouldn't have. See the previous commit for more details. We are doing the following indexedDB table renames in this commit: * NotificationClicked -> Outcomes.NotificationClicked * NotificationReceived -> Outcomes.NotificationReceived IndexedDB does not allow changing the keyPath to correct it so instead we must create a new table. Only "NotificationClicked" needs to be fixed however we are also renaming "NotificationReceived " and prefixing them with "Outcomes." so they are consistent. Comments in code explain in detail the effected versions and what was wrong with them.
These should have "this" binded to them so they can access class level properties. Became necessary since the introduction of this.migrateOutcomesNotificationClickedTableForV5().
rgomezp
requested changes
Aug 16, 2023
jkasten2
changed the title
WIP - [Fix] Outcomes IndexedDB schema
[Fix] Outcomes IndexedDB schema
Aug 17, 2023
rgomezp
reviewed
Aug 17, 2023
rgomezp
approved these changes
Aug 17, 2023
had to make a few changs to IndexedDb.ts class to make it testable.
jkasten2
force-pushed
the
fix/outcomes_indexeddb_schema
branch
from
August 17, 2023 19:56
cb40c0a
to
38e1431
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
1 Line Summary
Fix errors saving clicked outcomes due to keyPath value changed incorrectly.
Details
We are doing the following indexedDB table renames in this commit:
Modiviation
Fixes the notification click direct outcomes feature. Also it is unknown if the indexedDB errors are causing throwing that is resulting in any other side-effect issues due to errors not being caught.
This is done to correct the keyPath for the "NotificationClicked" indexedDB table, you can't change it so a new table must be created.
Background
Table was created with wrong keyPath of "notification.id" for new visitors for versions from 160000.beta4 to 160000. Writes were attempted as "notificationId" in released 160000 however they may have failed if the visitor was new when those releases were in the wild. However those new on 160000.beta4 to 160000.beta8 will have records saved as "notification.id" that will be converted here.
See related tickets below for more details on the history of the issue.
Validation
Tests
Info
Checklist
Programming Checklist
Interfaces:
Functions:
Typescript:
Other:
elem of array
syntax. PreferforEach
or usemap
context
if possible. Instead, we can pass it to function/constructor so that we don't callOneSignal.context
Screenshots
Info
Checklist
Related Tickets
This change is