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

Update historical notices to use the new render_notice method. #2409

Merged
merged 19 commits into from
May 22, 2023

Conversation

anaemnesis
Copy link
Contributor

@anaemnesis anaemnesis commented Apr 21, 2023

Fixes #2378

Changes proposed in this Pull Request

This PR updates historical licenses to use the new WP_Job_Manager_Admin_Notices::render_notice method.

This change changes the render_notice notice method from a private static function to a public static function and then updates the notice display per the renders at #2382.

One item I have chosen to forgo for this change is the grouped update notification. I've decided this as the immediate request is to update historical notices to use the render_notice method, whereas the grouped update notifications would require the creation of new, non-historical notices.

One additional item I noticed is that the update notices don't inherit styling on non-WPJM pages:

Screenshot from 2023-05-04 11-26-57

To address this I could either move the relevant wp_enqueue_style and wp_register_style outside of the if ( in_array( $screen->id, apply_filters(.... conditional, or, add more screen IDs to the check. I've opted to move the style as it's not possible to anticipate all of the potential screens the notices might display on, or even if it was, the check would be so long as to be pointless.

To prevent a conflict with WC styling, I've also made use of a ! important declaration. I don't know if, instead, we want to copy into WPJM the relevant notice styles and have our own wpjm-notice class instead. I'm not keen on ! important so if we want to go to in that direction, I'm all for it. The declaration does seem the most straightforward though, and it's specific to our notices.

Without this change, notices lose their padding on certain WC pages (like WooCommerce --> Orders). For example:

Screenshot from 2023-05-04 11-59-24

Testing instructions

  • Checkout branch
  • Install a couple of paid WPJM plugins, but don't input a license key
  • Notices should display with the updated formatting as shown below
  • If you've tested this prior and an update notice isn't showing, use wp option delete wp_job_manager_dismissed_notices through WP CLI (or otherwise delete this option) to force the notice to display.

Screenshot from 2023-05-04 11-03-16

  • To show an error notice, comment out the code to show the missing license notice
    if ( empty( $licence['licence_key'] ) ) {
    add_filter(
    'wpjm_admin_notices',
    function ( $notices ) use ( $product_slug, $plugin_data ) {
    return $this->add_missing_licence_notice( $notices, $product_slug, $plugin_data );
    }
    );
    } elseif ( ! empty( $licence['errors'] ) ) {
  • change the elseif into an if, and then the check to show an error notice from ! empty to empty
    } elseif ( ! empty( $licence['errors'] ) ) {
    add_filter(
    'wpjm_admin_notices',
    function ( $notices ) use ( $product_slug, $plugin_data ) {
    return $this->add_error_licence_notice( $notices, $product_slug, $plugin_data );
    }
    );
    }

- Makes the `render_notice` method public so that it's callable from `html-license-key-error.php and `html-license-key-notice.php`

- HTML removed from both files as this is provided as part of the `render_notice` method.

- Notices are now sent in the format of the API request the `render_notice` method expects.
@anaemnesis anaemnesis self-assigned this Apr 21, 2023
- Makes the `render_notice` method public so that it's callable from `html-license-key-error.php and `html-license-key-notice.php`

- HTML removed from both files as this is provided as part of the `render_notice` method.

- Notices are now sent in the format of the API request the `render_notice` method expects.
This change loads notice styling throughout WP Admin, rather than restrict it to certain pages. This is because historical notices display throughout WP Admin and not just on WPJM pages.

WC styling was also interferring with WPJMs notice display. This has been addressed with a well-considered `! important` declaration.
@@ -11,7 +11,7 @@ $notice-success: #43af99;

// Spacing.
gap: 10px;
padding: 10px 12px;
padding: 10px 12px !important;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not totally cool with this, but after testing a few other options, it's the most straightforward option and limited to WPJM notice styling.

Another option is to rename notice to wpjm-notice and copy the relevant styling over to admin-notices.scss.

Copy link
Contributor

Choose a reason for hiding this comment

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

I realized an issue that we're also overriding an style that increases the padding in order to not have the dismiss button over the text.

Screenshot 2023-05-05 at 16 28 33

Maybe a solution could be to add the !important only on the padding-top and padding-bottom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what's happening there, yeah. I've gone with the separated padding route and I'm not seeing any issues with that.

Committed here b63ae05

includes/admin/class-wp-job-manager-admin.php Show resolved Hide resolved
@anaemnesis anaemnesis marked this pull request as ready for review May 4, 2023 11:00
@anaemnesis anaemnesis requested review from aaronfc and a team May 4, 2023 11:00
Base automatically changed from feature/reengaging-customers to trunk May 5, 2023 05:52
@anaemnesis
Copy link
Contributor Author

Noting that since the feature branch has been closed by @aaronfc, approval of this PR will result in a merge directly to trunk, triggering a deployment !!

@anaemnesis anaemnesis linked an issue May 5, 2023 that may be closed by this pull request
Copy link
Contributor

@renatho renatho left a comment

Choose a reason for hiding this comment

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

Hey! In general, it's looking very good!
I just left comments about 2 things I noticed.

Noting that since the feature branch has been closed by @aaronfc, approval of this PR will result in a merge directly to trunk, triggering a deployment !!

About this, I think it's not the case for the Job Manager plugin. :)

@@ -11,7 +11,7 @@ $notice-success: #43af99;

// Spacing.
gap: 10px;
padding: 10px 12px;
padding: 10px 12px !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

I realized an issue that we're also overriding an style that increases the padding in order to not have the dismiss button over the text.

Screenshot 2023-05-05 at 16 28 33

Maybe a solution could be to add the !important only on the padding-top and padding-bottom?

includes/admin/class-wp-job-manager-admin.php Show resolved Hide resolved

$notice = [
'level' => 'error',
'dismissible' => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that with this change, the dismissal is not being persisted anymore.

It seems we have a method checking the wpjm_hide_notice query string, but in this case, it's dismissing the notice without adding the query string or running any request.

Do you know if it's working in any other place to see what is different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was addressed in the refactor 15393a2

This sets the padding on the wpjm-admin-notice class to use the explicit top/right/bottom/left values. This way, the top and bottom styling can be overriden while the left/right are left alone.
- Revert `render_notice` to a `private` method
- Remove `html-licence-key-error.php` and `html-licence-key-notice.php`
- Add two new methods, `add_missing_licence_notice` and `add_error_licence_notice`
- Hook these two methods into the `wpjm_admin_notice` filter
- These can both be used as examples for adding notice to WPJM in the future
- Make sure 'License' follows not the UK standard
Comment on lines 565 to 573
public function licence_error_notices() {
$screen = get_current_screen();
if (
null === $screen ||
in_array( $screen->id, [ 'job_listing_page_job-manager-addons' ], true ) ||
! current_user_can( 'update_plugins' )
) {
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After speaking with @aaronfc we realised that null was being encountered when it ought not have been. This was removed as we decided that (1) error notices and (2) missing license notices should be displayed on the add-ons page.

include 'views/html-licence-key-error.php';
add_filter(
'wpjm_admin_notices',
function ( $notices ) use ( $product_slug, $plugin_data ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to avoid the anonymous functions here, but I'm not aware of another way to pass $product_slug and $plugin_data to the methods called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm good call!

I am fine with using an anonymous function here. The idea to not use an anonymous function is to be able to "unhook" it programatically. But in this case we can always "hook" later and remove the notice_id that we want to remove.

I can think of alternative ways like storing in a private attribute the data needed or something like that, but I don't like it. Good to go with the anonymous function 🚀

@anaemnesis
Copy link
Contributor Author

anaemnesis commented May 17, 2023

@aaronfc @renatho After speaking with Aaron, I completely refactored the notice displays to use the new wpjm_admin_notice filter. Testing instructions have been updated, and I worked through a couple of hangups with Aaron's help. But we're good for another round of testing.

15393a2

Edit: oh hang on, apparently we have unit tests now 👀
Edit: Fixed up. Failing tests now pertain to PHP version conflicts in the test suite itself

@anaemnesis anaemnesis requested a review from renatho May 17, 2023 19:06
I should have just reverted the commit that updated the spelling of licence, but this also fixes it. Lesson learned.
Copy link
Contributor

@renatho renatho left a comment

Choose a reason for hiding this comment

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

Hi Katja!

Thank you for working on that!

In my new test, it didn't show the notices. Maybe it's related to the order of hooks where we're using the wpjm_admin_notices now? Let me know if you need any help with that. ;)

Also, as we talked, if we replace the "license" with "licence" in this PR, we could try to avoid the apply_filters and the log_event functions to avoid breaking filters and losing metrics. Does it make sense? I'd advocate to not rename the strings too to not lose translations of what is already translated. But it's less critical if we really want this change.

*/
public function licence_error_notices() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the plugins open for the users we should take care to not remove the public functions to avoid errors with any type of integration.

It seems just a rename right? So in this case, we could deprecate this method, and inside it, call the maybe_add_licence_error_notices. So if someone is using it, they will see the deprecation warning instead of an error, and the function will continue working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronfc and I had discussed renaming, so it's just a rename that's right.

I've not deprecated a method before, but that sounds like a plan to me. I'll get to looking at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deprecated here ed89a13

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking nice to me. Good catch, @renatho !

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for changing that, Katja! And sorry for the late delay on reviewing it!

Just instead of maybe_add_license_error_notices() we should use WP_Job_Manager_Helper::maybe_add_license_error_notices to reference the correct method. Not something critical and urgent though. 😉

This commit hard-resets to `trunk`, and then selectively applies the changes we want. Too much had crept in with the wording errancy.
…' into add/update-legacy-notice-styling

# Conflicts:
#	includes/helper/class-wp-job-manager-helper.php
@anaemnesis
Copy link
Contributor Author

Also, as we talked, if we replace the "license" with "licence" in this PR, we could try to avoid the apply_filters and the log_event functions to avoid breaking filters and losing metrics. Does it make sense? I'd advocate to not rename the strings too to not lose translations of what is already translated. But it's less critical if we really want this change.

We did, and completely agree. I've gone ahead and hard-reset and reapplied changes:

The notices are showing for me though, so let me run through them again and see if I can replicate.

- This change deprecates the `licence_error_notices()` method and calls inside it, the `maybe_add_licens_error_notices()` method. Added for compatibility in case anyone was using the method as it was.
@anaemnesis
Copy link
Contributor Author

In my new test, it didn't show the notices. Maybe it's related to the order of hooks where we're using the wpjm_admin_notices now? Let me know if you need any help with that. ;)

I think this is due to previous testing. Can you delete the wp_job_manager_dismissed_notices option and see if that forces the notice to display anew for you? In WP CLI that would be wp option delete wp_job_manager_dismissed_notices.

@anaemnesis anaemnesis requested a review from renatho May 20, 2023 20:09
This change adds the $product_slug to the `data-dismiss-notice` property to ensure different addons use different properties names.
Copy link
Contributor

@aaronfc aaronfc left a comment

Choose a reason for hiding this comment

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

Works great! 🚀

I left some comments but are mostly about naming. Once that's fixed I am happy with this being merged.

Thanks for working on this! <3

Comment on lines 13 to 20
* Helper functions used in WP Job Manager regarding addons and licences.
*
* @package wp-job-manager
* @since 1.29.0
*/
class WP_Job_Manager_Helper {
/**
* License messages to display to the user.
* Licence messages to display to the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would leave this as they were (with s).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed d9e812b

*/
public function licence_error_notices() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking nice to me. Good catch, @renatho !

) {
return;
}
_deprecated_function( __METHOD__, '1.33.0', 'maybe_add_license_error_notices()' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you call the method 'license' but the real name you set is 'licence'.

I would go with 's' everywhere (except from the deprecated method, which obviously has to be kept like it was before).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed d9e812b

return;
}
_deprecated_function( __METHOD__, '1.33.0', 'maybe_add_license_error_notices()' );
$this->maybe_add_license_error_notices();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is a typo here. The method is actually called "licence" (but I think you should rename it to "license").

include 'views/html-licence-key-error.php';
add_filter(
'wpjm_admin_notices',
function ( $notices ) use ( $product_slug, $plugin_data ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm good call!

I am fine with using an anonymous function here. The idea to not use an anonymous function is to be able to "unhook" it programatically. But in this case we can always "hook" later and remove the notice_id that we want to remove.

I can think of alternative ways like storing in a private attribute the data needed or something like that, but I don't like it. Good to go with the anonymous function 🚀

* @param array $plugin_data The plugin data.
* @return array
*/
public function add_missing_licence_notice( $notices, $product_slug, $plugin_data ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be private, right?

* @param array $plugin_data The plugin data.
* @return array
*/
public function add_error_licence_notice( $notices, $product_slug, $plugin_data ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, can be private :)

esc_html( $plugin_data['Name'] )
),
];
$notices[ 'wpjm_licence_notice_' . $product_slug ] = $notice;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add here "missing" to the license notice ID. Also, since the dismissing mechanism is different I think we can rename it freely (and also name it "license" with an S).

Final name: 'wpjm_missing_license_notice_' . $product_slug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Committed (for here and above) ea758b0

Updates the new methods, and a string, to use the preferred `s` spelling, rathern than the `c` spelling
- Make our two new helper methods private rather than public
- Change the missing-license notice attribute to `wpjm_missing_license_notice_`
Copy link
Contributor

@aaronfc aaronfc left a comment

Choose a reason for hiding this comment

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

Looks good, works well! 🚀

@anaemnesis anaemnesis merged commit 80642b3 into trunk May 22, 2023
@anaemnesis anaemnesis deleted the add/update-legacy-notice-styling branch May 22, 2023 11:10
@yscik yscik added this to the 1.41.0 milestone Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revamp unlicensed notices design.
4 participants