-
Notifications
You must be signed in to change notification settings - Fork 43
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
3.0.0 #333
Conversation
Motivation: allows users to export their settings to a txt file
Motivation: will allow us to mark the migration as complete in the DB
Motivation: add new header with explanation, migration, button. Clean up stackable menu & outdated tabs.
Motivation: allow us to load the old or new plugin depending on migration status
Motivation: referrer nonce was incorrect & redirect was to the wrong page
Motivation: based on the option `onesignal_plugin_migrated` we serve either the old version or the new one.
@rgomezp im on my phone, so can't see full context - why are you swapping the v2 code to have v16 SDK, and be defer? Should that not remain as is? V16 is in v3 folder, no? |
we are loading v16 for v2 also. this will be the simplest way to make things work. if we move the sdk_files into the subdirectories we can break existing integrations |
Got it - So, is the SDK version that is used this based on the plugin version they use? If not, it should be, since folks will be using v15 methods, especially if they have other prompts which they needed to add via custom code. |
Yep plugin versions < 3 will still be on v15 and 3> will use 16. However, if they have custom code + the plugin, it will break when they upgrade to v3. This is fine since it's a major version, but we should consider adding to the docs. |
Cool! where's the logic? Since I can only see a straight swap of the SDK code? EDIT: |
I suppose it would be breaking for people who are customizing the code instead of just using the plugin dashboard. Is that your concern? |
Yes – Since the 'original' V3 wasn't released due to concern for the same breaking change. Therefore, only after clicking the Migrate button should the SDK version change. |
070baaf
to
4cc3a16
Compare
echo "importScripts('https://localhost:3001/dev_sdks/OneSignalSDK.js');"; | ||
} else { | ||
echo "importScripts('https://cdn.onesignal.com/sdks/OneSignalSDKWorker.js');"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is removal of this file going to create weird behavior if someone already has this service worker registered and the browser tries to update it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't. We deprecated that some time ago even on the player model so shouldn't be necessary anymore. We use query params to update the worker.
@@ -1 +1 @@ | |||
importScripts('https://cdn.onesignal.com/sdks/OneSignalSDKWorker.js'); | |||
importScripts("https://cdn.onesignal.com/sdks/web/v16/OneSignalSDK.sw.js"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little curious that we have chosen to version our hosted Service Worker. Is there any documentation about why we decided to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to support both the player model SDK as well as the new user model SDK which use different workers
if (is_wp_error($response)) { | ||
error_log('API request failed: ' . $response->get_error_message()); | ||
} | ||
$json = json_decode(wp_remote_retrieve_body($response)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to break if $response is an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't break. It'll fail silently
<div id="os_options"> | ||
<label for="os_segment">Send to segment</label> | ||
<select name="os_segment" id="os_segment"> | ||
<option value="All">All</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a little odd to have an "All" option hardcoded when most users will also have a "Total Subscribers" segment or similar. Wdyt about removing this hard-coded option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's best to have one hardcoded option, in case the segment list isn't returned for any reason.
We have renamed our Default segments over the years, so it's hard to decide on the correct Segment name to choose here.
"All" is what the current plugin uses, so I added this to keep the same behaviour of targeting all subscribed subscriptions.
Motivation: show the v3 plugin to fresh installs and the v2 plugin to existing installs
Motivation: provide more detail on how to complete the migration
Motivation: we want to be able to check if the api key is a legacy key or a "rich" key in order to know what headers to send. We put that method here.
Motivation: we should support legacy and rich keys
Motivation: improve styles and also add a badge denoting what type the key is (legacy or rich). Add form validation via jquery: checks to see if keys are valid
Motivation: get v2 to start using the v16 API
Motivation: to ensure a smooth transition, we will handle SW installation in the plugin. We need to update v3 to do that
ceed42b
to
8a06b63
Compare
readme.txt
Outdated
= 3.0.0 = | ||
WARNING: this update contains breaking changes. | ||
We are moving all OneSignal plugin settings to the OneSignal.com dashboard. | ||
If you have made any OneSignal custom-code modifications (not via UI) updating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WARNING: this update contains changes that may break specific setups, as detailed below:
If you have made any code modifications to your Wordpress installation to directly interact with the OneSignal SDK, this version will break your implementation. Please revert your code modifications prior to upgrading and use our improved dashboard functionality to configure your OneSignal integration instead.
Motivation: notifications are also sent when the post is updated.
Motivation: link to the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Needs link to new docs on the V3 new user settings page
- Needs improved release notes to warn about breakage for users with modifications to their installation
- Needs improved release line items about why the new version is great
- Needs release note that notifications are not sent by default, and the user will have to opt-in per post
One line summary
Introduce hybrid support for V2 and V3 of the OneSignal plugin, enabling smooth migration and improved user experience.
Description
This update introduces a hybrid implementation of the OneSignal WordPress plugin to support the transition from V2 to V3. The primary goal is to ensure a seamless migration for existing users while providing a simplified experience for new users.
Key Features and Changes
Migration Flow
onesignal_plugin_migrated
option in the database is set totrue
upon successful migration.Dynamic Plugin Loading
onesignal_plugin_migrated
isfalse
or unset.onesignal_plugin_migrated
istrue
.New User Experience
Supporting Enhancements
.txt
file.legacy
andrich
keys.Post-Release Cleanup
This update lays the foundation for ongoing improvements while maintaining stability for current users.
This change is