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

Issue #17 : Enable deleting settings in snapshot wp-admin/post page. #84

Merged
merged 16 commits into from Oct 20, 2016

Conversation

Projects
None yet
4 participants
@kienstra
Contributor

kienstra commented Aug 24, 2016

Request For Code Review

Hi @westonruter,
Could you please review this pull request for Issue #17?

kienstra added some commits Aug 24, 2016

Issue #17 : Enable deleting settings in snapshot wp-admin/post page.
Create link 'Remove setting,' which toggles to 'Restore setting' on click.
It also sets a hidden text field for each setting to be removed.
On saving, these appear in [ 'customize_snapshot_remove_settings' ].
Filter post content  on 'content_save_pre,' when this is saved.
If  has settings to be removed, filter them out.
Also, add simple styling of this 'Remove setting' link.
It appears red when setto 'remove,' and blue when set to 'restore.'
Issue #17 : Fix failing unit test: property of non-object error.
Before evaluating ->post_type, pass it to isset().
Short-circuit the conditional if it's not set.
&&
wp_verify_nonce( $_REQUEST[ static::SLUG ], static::SLUG . '_settings' )
&&
! ( defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE )

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

We should be using the new wp_doing_ajax function if it is available, so this can be replaced with:

! ( function_exists( 'wp_doing_ajax' ) ? wp_doing_ajax() : defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE )

This will likely yield benefits for doing unit testing without having to be in a separate Ajax unit test, since the return value of wp_doing_ajax() can be filtered as opposed to being a fixed constant.

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

Oops. 😊 I misread the constant here. Nevermind 😄

}
$setting_ids_to_unset = $_REQUEST[ $key_for_settings ];
$data = json_decode( $post->post_content );

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

Instead of getting $post->post_content should it not be using the $content that is already provided?

This comment has been minimized.

@kienstra

kienstra Sep 3, 2016

Contributor

That's a good point. This commit corrects that.

&&
( static::SLUG === $post->post_type )
&&
! empty( $_REQUEST[ $key_for_settings ] )

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

Should this be checked to see if it is_array?

This comment has been minimized.

@kienstra

kienstra Sep 4, 2016

Contributor

That's a good idea. This commit adds an is_array call to $should_fliter_content.

public function enqueue_admin_scripts() {
global $post;
$handle = 'customize-snapshots-admin';
if ( isset( $post->post_type ) && ( Post_Type::SLUG === $post->post_type ) && ( 'publish' !== $post->post_status ) ) {

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

Should this be looking at the $hook arg passed in? Or should it be checking what is returned by get_current_screen()?

This comment has been minimized.

@kienstra

kienstra Sep 4, 2016

Contributor

Hi @weston, it looks like checking the $hook argument in this admin_enqueue_scripts callback should be enough.
This commit adds an expression to the conditional:
( 'post.php' === $hook )
The WP_Screen object that get_current_screen() returns has some useful properties, including:

id = "custom_snapshot",
base = "post",
post_type = "customize_snapshot"

But the conditional already checks for most of these:
if ( ( 'post.php' === $hook ) && isset( $post->post_type ) && ( Post_Type::SLUG === $post->post_type ) && ( 'publish' !== $post->post_status ) ) {

var $link, linkText, linkActions, dataSlug, initializeLink;
$link = $( '.snapshot-toggle-setting-removal' );
linkText = [ 'Remove setting', 'Restore setting' ];

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

These strings need to be translated, and exported from PHP into JS.

This comment has been minimized.

@kienstra

kienstra Sep 5, 2016

Contributor

That's a good point. This commit outputs the translated strings via PHP.

@@ -360,6 +362,7 @@ public function render_data_metabox( $post ) {
echo '<li>';
echo '<details open>';
echo '<summary><code>' . esc_html( $setting_id ) . '</code> ';
echo '<a href="#" id="' . esc_attr( $setting_id ) . '" class="snapshot-toggle-setting-removal remove"></a>';

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

I suggest you output the actual strings here. You can do like:

echo '<a href="#" id="' . esc_attr( $setting_id ) . '" data-text-restore="<?php esc_attr_e( 'Restore setting', 'customize-snapshots' ) ?>" class="snapshot-toggle-setting-removal remove"><?php esc_html_e( 'Remove setting', 'customize-snapshots' ) ?></a>';

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

Then you can toggle between these strings in the JS.

This comment has been minimized.

@kienstra

kienstra Sep 3, 2016

Contributor

Thanks, @westonruter. This line in a new commit outputs the strings, and this function toggles between the strings in JavaScript.

clickedLinkAction = $( this ).data( dataSlug );
settingId = $( this ).attr( 'id' );
this.isLinkSetToRemoveSetting = function() {

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

Humm. You're adding these functions to the jQuery object itself. Maybe better to create a component object that you attach the functions to, and then call its methods in a click handler instead? You can see a pattern for how to create a component in https://github.com/xwp/wp-customize-snapshots/blob/develop/js/customize-snapshots.js and see also how it gets exported to global scope so that the component can be interacted with by other scripts, or even by you on the console.

This comment has been minimized.

@kienstra

kienstra Sep 3, 2016

Contributor

Thanks, @westonruter. This commit attaches the functions to component. But I still need to export component object.

@coveralls

This comment has been minimized.

coveralls commented Aug 28, 2016

Coverage Status

Coverage decreased (-0.8%) to 90.191% when pulling 625ff99 on feature/allow-deleting-settings into 7d1e373 on develop.

.addClass( 'snapshot-setting-removed' );
};
this.constructHiddenInputWithValue = function() {

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

Better for this function to be more of a pure function that doesn't rely on its closure scope. So it can be at the top level as component.constructHiddenInputWithValue() and it can take a settingId as its argument. It is much easier to test the function like this, and there is only ever one copy, whereas here there is a copy for each time the user clicks.

This comment has been minimized.

@kienstra

kienstra Sep 3, 2016

Contributor

Thanks, this commit moves constructHiddenInputWithValue into the component object.

initializeLink();
$link.on( 'click', function( event ) {

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

The function body should be refactored, to make the logic for the functions to not rely on this but be passed the context as an explicit argument. So hideSettingAndChangeLinkText can take an argument for the setting details jQuery object, for example, instead of relying on the inherited this context.

This comment has been minimized.

@kienstra

kienstra Sep 3, 2016

Contributor

Thanks, @westonruter. That's a good idea to refactor the functions. This commit does that. Sorry for the delay.

initializeLink = function() {
$link.text( linkText[ 0 ] )
.data( dataSlug, linkActions[ 0 ] );
};

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

This can be eliminated in favor of putting the translated text strings inline in the rendered PHP.

This comment has been minimized.

@kienstra

kienstra Sep 3, 2016

Contributor

Thanks, this commit outputs the translated strings with PHP.

};
this.removeHiddenInputWithValue = function() {
$( 'input[value="' + settingId + '"]' ).remove();

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

Also make sure that the name is customize_snapshot_remove_settings[] right?

This comment has been minimized.

@kienstra

kienstra Sep 3, 2016

Contributor

That's a good point, @westonruter. This commit ensures that the name is customize_snapshot_remove_settings[].

@@ -114,6 +114,7 @@ public function register() {
add_filter( 'display_post_states', array( $this, 'display_post_states' ), 10, 2 );
add_action( 'admin_notices', array( $this, 'show_publish_error_admin_notice' ) );
add_action( 'post_submitbox_minor_actions', array( $this, 'hide_disabled_publishing_actions' ) );
add_filter( 'content_save_pre', array( $this, 'filter_out_settings_if_removed_in_metabox' ), 10 );

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

Good idea to use content_save_pre.

$setting_ids_to_unset = $_REQUEST[ $key_for_settings ];
$data = json_decode( $post->post_content );
foreach ( $setting_ids_to_unset as $setting_id ) {
unset( $data->{ $setting_id } );

This comment has been minimized.

@westonruter

westonruter Aug 28, 2016

Contributor

The second $associative arg should be passed to json_decode() above so that it will be an array so that the familiar bracket notation can be used as opposed to this somewhat-unusual object brace notation.

This comment has been minimized.

@kienstra

kienstra Sep 3, 2016

Contributor

Thanks, this commit uses the second argument of true to return an array.

@westonruter

This comment has been minimized.

Contributor

westonruter commented Aug 28, 2016

@kienstra the functionality works well! Beyond my inline feedback, something else that should be done is a unit test on filter_out_settings_if_removed_in_metabox.

Issue #17 : Refactor customize-snapshots-admin.js methods.
Instead of attaching methods to 'on' event handler,
Attach them to module-scope components object.
Haven't yet implemented a way to export them, as Weston suggested.
@coveralls

This comment has been minimized.

coveralls commented Sep 3, 2016

Coverage Status

Coverage decreased (-0.8%) to 90.18% when pulling 83ad0e9 on feature/allow-deleting-settings into 7d1e373 on develop.

Issue #17 : Change logic for toggling link text.
Using Weston's snippet, output link title in PHP, with localization.
And attach localized text 'Restore settting' to the link's data-text-restore attribute.
On clicking the link, call method changeLinkText.
This toggles the title to the other value stored in the data attribute.
And stores the old title in the data attribute, for the next time it's clicked.
@coveralls

This comment has been minimized.

coveralls commented Sep 3, 2016

Coverage Status

Coverage decreased (-0.8%) to 90.18% when pulling 35d5bca on feature/allow-deleting-settings into 7d1e373 on develop.

Issue #17 : Remove need for object brace notation.
At Weston's suggestion, pass second argument to json_decode.
This will then return an array, instead of an object.
So access the array value with $data[ $setting_id ].
Instead of previous object access with $data->{ $setting_id }.
@coveralls

This comment has been minimized.

coveralls commented Sep 3, 2016

Coverage Status

Coverage decreased (-0.8%) to 90.18% when pulling 92f14c0 on feature/allow-deleting-settings into 7d1e373 on develop.

Issue #17 : Use $content passed to filter, instead of $post->post_con…
…tent.

Also, wp_unslash() the $content.
Otherwise, the json_decode function will return null.
@coveralls

This comment has been minimized.

coveralls commented Sep 3, 2016

Coverage Status

Coverage decreased (-0.8%) to 90.18% when pulling f76b4a6 on feature/allow-deleting-settings into 7d1e373 on develop.

Issue #17 : Add is_array() to $should_filter_content conditional.
Before iterating through the settings to unset,
Ensure that the settings are in an array, as expected.
@coveralls

This comment has been minimized.

coveralls commented Sep 4, 2016

Coverage Status

Coverage decreased (-0.8%) to 90.189% when pulling d1487c8 on feature/allow-deleting-settings into 7d1e373 on develop.

@coveralls

This comment has been minimized.

coveralls commented Sep 4, 2016

Coverage Status

Coverage decreased (-0.8%) to 90.189% when pulling 68fa5ae on feature/allow-deleting-settings into 7d1e373 on develop.

@kienstra

This comment has been minimized.

Contributor

kienstra commented Sep 6, 2016

Thanks For Waiting
I Still Need To Write PHPUnit Tests

Hi @westonruter,
Sorry for the delay in applying your feedback here. I addressed all of your comments above, but I still need to write PHPUnit tests.

@PatelUtkarsh PatelUtkarsh referenced this pull request Sep 16, 2016

Merged

Add snapshot fork feature #90

3 of 3 tasks complete
@westonruter

This comment has been minimized.

Contributor

westonruter commented Sep 21, 2016

@kienstra no apology will be accepted! It is I who probably should apologize for being slow to review PRs 😞

@@ -360,6 +362,7 @@ public function render_data_metabox( $post ) {
echo '<li>';
echo '<details open>';
echo '<summary><code>' . esc_html( $setting_id ) . '</code> ';
echo '<a href="#" id="' . esc_attr( $setting_id ) . '" data-text-restore="' . esc_attr( 'Restore setting', 'customize-snapshots' ) . '" class="snapshot-toggle-setting-removal remove">' . esc_html( 'Remove setting', 'customize-snapshots' ) . '</a>';

This comment has been minimized.

@PatelUtkarsh

PatelUtkarsh Oct 7, 2016

Collaborator

Use translation function esc_attr__ and esc_html__ instade of esc_attr( 'Restore setting', 'customize-snapshots' ) and esc_html( 'Remove setting', 'customize-snapshots' )

@PatelUtkarsh

This comment has been minimized.

Collaborator

PatelUtkarsh commented Oct 12, 2016

In e5accee I've resolved conflict with develop.

@coveralls

This comment has been minimized.

coveralls commented Oct 12, 2016

Coverage Status

Coverage decreased (-0.7%) to 90.085% when pulling e5accee on feature/allow-deleting-settings into 59b8b66 on develop.

@PatelUtkarsh

This comment has been minimized.

Collaborator

PatelUtkarsh commented Oct 19, 2016

I am picking this up as discussed with @jeffpaul and @kienstra.

@coveralls

This comment has been minimized.

coveralls commented Oct 19, 2016

Coverage Status

Coverage decreased (-0.2%) to 90.593% when pulling 8f29076 on feature/allow-deleting-settings into 59b8b66 on develop.

@coveralls

This comment has been minimized.

coveralls commented Oct 20, 2016

Coverage Status

Coverage decreased (-0.2%) to 90.593% when pulling 88a83de on feature/allow-deleting-settings into 59b8b66 on develop.

@westonruter westonruter merged commit 5900671 into develop Oct 20, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the feature/allow-deleting-settings branch Oct 20, 2016

@westonruter westonruter modified the milestone: 0.6.0 Jul 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment