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

Feature Request: Hide Upsell after Post Deletion #44

Closed
mrwweb opened this issue Sep 20, 2018 · 18 comments

Comments

@mrwweb
Copy link

commented Sep 20, 2018

Yoast's plugin recently started trying to upsell at the top of the All Posts screen every time you delete a post (page, anything). Super annoying. Would love to see that hidden!

@mrwweb

This comment has been minimized.

Copy link
Author

commented Sep 20, 2018

screenshot:

image

@senlin

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2018

hi @mrwweb - thanks for the report.

That is just crazy! Will definitely ~~~get rid of that soon~~~ try to find a way to get rid of that! Thanks for the screenshot to visualise the issue.

@senlin senlin added the todo label Sep 20, 2018
@senlin senlin self-assigned this Sep 20, 2018
@senlin senlin added the help wanted label Sep 20, 2018
@senlin

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2018

Hmmm, this is going to be more difficult than I had hoped :(

The notice has very generic classes: yoast-alert notice notice-warning is-dismissible, so there is a risk that if I do a display: none; on that, other things will not be displayed either.

This file is responsible for adding the actions: https://github.com/Yoast/wordpress-seo/blob/trunk/admin/watchers/class-slug-change-watcher.php#L26-L33

If anyone has an idea on how to disable this, input is much appreciated!

@mrwweb

This comment has been minimized.

Copy link
Author

commented Sep 20, 2018

It looks to me like that whole file is only there for the purpose of the upsells, so could you just remove the actions wholesale?

@senlin

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2018

Yeah, unfortunately it's not as easy as that.

senlin added a commit that referenced this issue Sep 20, 2018
@senlin

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2018

Going the CSS way as I can't seem to get rid of it in another way.

senlin added a commit that referenced this issue Sep 20, 2018
* release date September 20, 2018
* add option to hide the Premium ad that shows after a Post, Page or Taxonomy is deleted (see #44)
@senlin

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2018

@mrwweb you can find the new release at https://github.com/senlin/so-clean-up-wp-seo/releases/tag/v3.8.0 and soon it will be available on the WP Plugins Directory. Keep in mind to check the Settings page to tick the new option box.

@senlin senlin closed this Sep 20, 2018
@mrwweb

This comment has been minimized.

Copy link
Author

commented Sep 21, 2018

FYI: I can confirm that the ads no longer appear.

However, I didn't need to check the box in settings nor did I save the settings yet. The following notice is on the settings page:

Notice: Undefined index: hide_post_deletion_premium_ad on wp-content/plugins/so-clean-up-wp-seo/admin/class-so-clean-up-wp-seo-admin-api.php:58

I don't have time to go digging, but you likely need to implement a default value for each option with something like if( isset( $option_value ) { ...

@senlin

This comment has been minimized.

Copy link
Owner

commented Sep 21, 2018

Hmmm, thing is we do set default values already at https://github.com/senlin/so-clean-up-wp-seo/blob/develop/includes/class-so-clean-up-wp-seo.php#L425-L455

@afragen - do you happen to know what's going on with this?

@senlin senlin reopened this Sep 21, 2018
@afragen

This comment has been minimized.

Copy link
Collaborator

commented Sep 21, 2018

I’m away until next week, but I’ll take a look when I get back.

@afragen

This comment has been minimized.

Copy link
Collaborator

commented Sep 21, 2018

Might need to activate and deactivate the plugin as the defaults only seem to be set during activation. Since the default wasn’t previously present, you get an error.

Just running the code in my head as I don’t have a laptop with me.

@senlin

This comment has been minimized.

Copy link
Owner

commented Sep 23, 2018

If that is indeed how it works, then we might need to rethink that as that is not very user friendly.

@afragen

This comment has been minimized.

Copy link
Collaborator

commented Sep 23, 2018

The PHP warning is only a warning. Just put in the changes that a deactivation/deactivation is needed.

Otherwise you’ll need to write an update procedure to ensure any empty settings are filled.

@mrwweb

This comment has been minimized.

Copy link
Author

commented Sep 24, 2018

Seems like it may be worth redoing how options are handled in this plugin. The general best practice I know and has served me well is to:

  1. Only save options to the database that the user has explicitly set
  2. Provide default values for all options (but don't save them to the DB)
  3. Save all plugin options in a single option as an array

So in the plugin you handle defaults and avoid warnings is something like this:

<?php
// get the option, set empty array as default value if there's nothing there
$my_plugin_options = get_option( 'my_plugin_options', array() );
// check if a specific option exists, use it if so. set false (or whatever) as default otherwise
$my_plugin_option = isset( $my_plugin_options['a_specific_option'] ) ? $my_plugin_options['a_specific_option'] : false;
// ... do stuff with $my_plugin_options['a_specific_option'] now

You could obviously abstract that pattern to a function to speed things up or make a merged array of defaults and user-set options early on, but that's just different implementations of the same best practice.

@senlin senlin removed the bug label Sep 24, 2018
@senlin

This comment has been minimized.

Copy link
Owner

commented Sep 24, 2018

@mrwweb - Thanks for your thoughts and input Mark.

Only save options to the database that the user has explicitly set

Regarding this the idea behind the plugin is that people install it to clear up the mess, so instead having the user tick 20 boxes, the default setting is that almost everything is checked.

Of course I am happy to be persuaded otherwise if it turns out that it simply works easier to have them set by the user. In that case adding a new option would also mean that the user has to revisit the settings?

@afragen

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2018

Perhaps a simple

$this->options = array_merge( $this->get_defaults(), $this->options );

At the end of the CUWS constructor.

This would ensure that any added settings would have default values.

Of course you would need to save this array again.

@afragen

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2018

I have an idea about how to do this. I’ll submit a PR.

afragen added a commit to afragen/so-clean-up-wp-seo that referenced this issue Sep 25, 2018
@senlin senlin closed this in #45 Sep 25, 2018
senlin added a commit that referenced this issue Sep 25, 2018
* release date September 25, 2018
* refactor options to ensure that additional settings will have default values. Fixes issue #44
@senlin

This comment has been minimized.

Copy link
Owner

commented Sep 25, 2018

New version just has been released on the WP Plugins Directory too.

Thanks for all your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.