-
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.4 - Release - Add back features, bug fixes #339
Conversation
Motivation: we're unregistering the legacy SW (.php extension) anyway. Logic led to defaulting to false & using root scope
Motivation: important feature / should not have been removed
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.
✅ with a couple of non-blocking questions
$onesignal_settings['app_rest_api_key'] = sanitize_text_field($_POST['onesignal_rest_api_key']); | ||
} | ||
// Save the auto send notifications setting | ||
$auto_send = isset($_POST['onesignal_auto_send']) ? 1 : 0; |
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.
Question: Is it enough to check that onesignal_auto_send
exists here, or should we ensure that its value is set to 1?
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.
Good question. It won't come through at all from the POST action if unchecked.
foreach ($json->segments as $segment) { | ||
$selected = isset($post->os_meta['os_segment']) && $post->os_meta['os_segment'] === $segment->name ? 'selected' : ''; | ||
echo '<option value="' . esc_attr($segment->name) . '"' . $selected . '>' . esc_html($segment->name) . '</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.
Question: this might be overly cautious but do we need to add a check for if $segment->name
is set?
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.
Doesn't hurt
Motivation: was previously breaking the page if say, the App ID was incorrect
Motivation: logic was incorrect + required an API key even if not changing it
Motivation: highly requested feature was removed in v3
Motivation: ship 3.0.4 - Added features: auto-send on publish, UTM tags - Bug fixes: validation issues on settings form
One line summary
Release 3.0.4 with new features, bug fixes, and clean-up.
Description
This update introduces several improvements and fixes in preparation for the 3.0.4 release:
New Features
Bug Fixes
onesignal-metabox.php
: Fixed issues where incorrect App IDs or similar API errors would break the page.Clean-up
readme.txt
fileRelease Notes
This change is