Issue #17 : Enable deleting settings in snapshot wp-admin/post page. by kienstra · Pull Request #84 · xwp/wp-customize-snapshots

kienstra · 2016-08-24T08:56:59Z

Request For Code Review

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

73c5b28 : Create link 'Remove setting', which toggles to 'Restore setting' on click.
It also sets a hidden input field for each setting to be removed.
On saving, these appear in $_REQUEST[ 'customize_snapshot_remove_settings' ].
Filter post content on content_save_pre when this is saved.
If $_REQUEST has settings to be removed, filter them out.
Also, add simple styling of this 'Remove setting' link.
It appears red when it has the text "Remove setting, and blue otherwise.

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.'

Before evaluating ->post_type, pass it to isset(). Short-circuit the conditional if it's not set.

westonruter · 2016-08-28T00:21:50Z

+			&&
+			wp_verify_nonce( $_REQUEST[ static::SLUG ], static::SLUG . '_settings' )
+			&&
+			! ( defined( 'DOING_AUTOSAVE' ) && DOING_AUTOSAVE )


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.

Oops. 😊 I misread the constant here. Nevermind 😄

… into feature/allow-deleting-settings

…names

westonruter · 2016-08-28T00:39:20Z

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


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

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

coveralls · 2016-08-28T00:47:59Z

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

westonruter · 2016-08-28T00:49:25Z

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


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.

Thanks, this commit moves constructHiddenInputWithValue into the component object.

westonruter · 2016-08-28T00:56:12Z

@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.

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 · 2016-09-03T17:59:18Z

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

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 · 2016-09-03T19:37:47Z

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

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 · 2016-09-03T20:12:29Z

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

…tent. Also, wp_unslash() the $content. Otherwise, the json_decode function will return null.

coveralls · 2016-09-03T20:51:30Z

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

Before iterating through the settings to unset, Ensure that the settings are in an array, as expected.

coveralls · 2016-09-04T17:17:49Z

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

coveralls · 2016-09-04T19:50:28Z

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

kienstra · 2016-09-06T04:51:35Z

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.

westonruter · 2016-09-21T22:05:06Z

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

PatelUtkarsh · 2016-10-07T11:09:43Z

 			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>';


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

PatelUtkarsh · 2016-10-12T09:32:38Z

In e5accee I've resolved conflict with develop.

coveralls · 2016-10-12T09:33:24Z

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

PatelUtkarsh · 2016-10-19T06:40:38Z

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

coveralls · 2016-10-19T09:45:14Z

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

…ompat

coveralls · 2016-10-20T00:08:40Z

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

Ryan Kienstra added 4 commits August 24, 2016 05:31

Issue #17 - Fix ESLint errors, including removing extra parentheses.

3a60aaa

Issue #17 : Correct ESLint error by removing parameter .

6d4b74b

Issue #17 : Fix failing unit test: property of non-object error.

48a70cc

Before evaluating ->post_type, pass it to isset(). Short-circuit the conditional if it's not set.

westonruter reviewed Aug 28, 2016
View reviewed changes

westonruter added 2 commits August 27, 2016 17:22

Merge branch 'develop' of https://github.com/xwp/wp-customize-snapshots…

e925589

… into feature/allow-deleting-settings

Chose line-through to mark removed settings; use unabbreviated class …

625ff99

…names

westonruter reviewed Aug 28, 2016
View reviewed changes

Issue #17 : Refactor customize-snapshots-admin.js methods.

83ad0e9

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.

Issue #17 : Remove need for object brace notation.

92f14c0

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 }.

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

f76b4a6

…tent. Also, wp_unslash() the $content. Otherwise, the json_decode function will return null.

Issue #17 : Add is_array() to $should_filter_content conditional.

d1487c8

Before iterating through the settings to unset, Ensure that the settings are in an array, as expected.

Issue #17 : In enqueuing, verify that $hook argument is 'post.php'.

68fa5ae

PatelUtkarsh mentioned this pull request Sep 16, 2016

Add snapshot fork feature #90

Merged

3 tasks

PatelUtkarsh suggested changes Oct 7, 2016

View reviewed changes

Merge 'develop' in 'feature/allow-deleting-settings'

e5accee

PatelUtkarsh added 2 commits October 19, 2016 12:14

Use translation function for escaping

8fc727c

Add unit-test for delete setting

8f29076

PatelUtkarsh approved these changes Oct 19, 2016

View reviewed changes

Replace testing trunk with testing latest due to lack of changesets c…

88a83de

…ompat

westonruter merged commit 5900671 into develop Oct 20, 2016

westonruter deleted the feature/allow-deleting-settings branch October 20, 2016 00:22

westonruter modified the milestone: 0.6.0 Jul 6, 2017

westonruter mentioned this pull request Jul 24, 2017

Hide Remove Setting link when viewing a published changeset #141

Merged

Uh oh!

Conversation

kienstra commented Aug 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

westonruter Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

westonruter Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

kienstra Sep 3, 2016

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

kienstra Sep 3, 2016

Choose a reason for hiding this comment

Uh oh!

westonruter commented Aug 28, 2016

Uh oh!

coveralls commented Sep 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kienstra commented Sep 6, 2016

Uh oh!

westonruter commented Sep 21, 2016

Uh oh!

PatelUtkarsh Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

PatelUtkarsh commented Oct 12, 2016

Uh oh!

coveralls commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PatelUtkarsh commented Oct 19, 2016

Uh oh!

coveralls commented Oct 19, 2016

Uh oh!

coveralls commented Oct 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kienstra commented Aug 24, 2016 •

edited

Loading

coveralls commented Aug 28, 2016 •

edited

Loading

coveralls commented Sep 3, 2016 •

edited

Loading

coveralls commented Sep 3, 2016 •

edited

Loading

coveralls commented Sep 3, 2016 •

edited

Loading

coveralls commented Sep 3, 2016 •

edited

Loading

coveralls commented Sep 4, 2016 •

edited

Loading

coveralls commented Sep 4, 2016 •

edited

Loading

coveralls commented Oct 12, 2016 •

edited

Loading

coveralls commented Oct 20, 2016 •

edited

Loading