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

Widgets - Simple Payments: fix a few issues #9863

Merged
merged 2 commits into from
Jul 9, 2018

Conversation

eliorivero
Copy link
Contributor

@eliorivero eliorivero commented Jul 4, 2018

Follow-up to #9859

Changes proposed in this Pull Request:

  • fix syntax error $errors.add( 'illegal_params' );
  • add missing message parameter in ->add
  • add missing jetpack translation domain
  • replace undefined functions require_lib and tracks_record_event

Testing instructions

Test with PHP 5.2 and ensure the widget works normally.

Proposed changelog entry for your changes:

PHP 5.2 Compatibility for Simple Payments Widget

@eliorivero eliorivero added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review This PR is ready for review. labels Jul 4, 2018
@eliorivero eliorivero requested a review from a team as a code owner July 4, 2018 12:39
@eliorivero eliorivero force-pushed the fix/simple-payments-widget branch 2 times, most recently from 03aad3d to 244b196 Compare July 4, 2018 12:41
@Automattic Automattic deleted a comment from jetpackbot Jul 4, 2018
@oskosk oskosk force-pushed the fix/simple-payments-widget branch from 244b196 to d51304a Compare July 4, 2018 15:52
@oskosk
Copy link
Contributor

oskosk commented Jul 4, 2018

@eliorivero I Merged Mike's PR and rebased this one

@oskosk oskosk added this to the 6.4 milestone Jul 4, 2018
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jul 5, 2018
require_lib( 'tracks/client' );
tracks_record_event( $current_user, 'simple_payments_button_' . $event_action, $event_properties );
jetpack_require_lib( 'tracks/client' );
jetpack_tracks_record_event( $current_user, 'simple_payments_button_' . $event_action, $event_properties );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this block of code is for WordPress.com, where jetpack_* functions are not available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 👍

@rodrigoi rodrigoi added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 5, 2018
@eliorivero eliorivero force-pushed the fix/simple-payments-widget branch from d51304a to 2390cd0 Compare July 5, 2018 16:20
…d syntax error and missing translation domain
@eliorivero eliorivero force-pushed the fix/simple-payments-widget branch from 2390cd0 to 592aebb Compare July 5, 2018 16:30
@@ -96,7 +96,7 @@ class="field-currency widefat jetpack-simple-payments-form-product-currency"
name="<?php echo esc_attr( $this->get_field_name( 'form_product_currency' ) ); ?>">
<?php foreach( Jetpack_Simple_Payments_Widget::$supported_currency_list as $code => $currency ) {?>
<option value="<?php echo esc_attr( $code ) ?>"<?php selected( $instance['form_product_currency'], $code ); ?>>
<?php esc_html_e( $code . ' ' . $currency ) ?>
<?php echo esc_html( "$code $currency" ); ?>
Copy link
Contributor Author

@eliorivero eliorivero Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing this here because esc_html_e is meant to be used when translating and displaying, not just displaying.

@rodrigoi rodrigoi added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jul 5, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jul 5, 2018
@jetpackbot
Copy link
Collaborator

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

Generated by 🚫 dangerJS

@rodrigoi
Copy link
Contributor

rodrigoi commented Jul 5, 2018

Just for the record, this is blocking D14005-code, that enables the SP Widget on WordPress.com.

@brbrr brbrr added the [Status] Ready to Merge Go ahead, you can push that green button! label Jul 6, 2018
@gwwar
Copy link
Contributor

gwwar commented Jul 6, 2018

@oskosk any idea on the timeline when this could be merged? I believe we'll want this in before we land on wpcom for fusion to sync properly.

@dereksmart dereksmart merged commit 85f0771 into master Jul 9, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jul 9, 2018
@dereksmart dereksmart deleted the fix/simple-payments-widget branch July 9, 2018 18:11
oskosk pushed a commit that referenced this pull request Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants