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

Various small bug fixes #337

Merged
merged 3 commits into from
Nov 25, 2023
Merged

Various small bug fixes #337

merged 3 commits into from
Nov 25, 2023

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Nov 20, 2023

Context

Summary

This PR can be summarized in the following changelog entry:

  • Range of small bug fixes

Relevant technical choices:

Column::register_hooks(): bug fix

The Column::quick_edit_remove_original() method only expects one parameter, so no need to ask WP for 2.

Newsletter::newsletter_signup_form(): bug fix

The wp_nonce_field() function by default displays the field.

This means that, as things were, the nonce field would be echo'd out when the function was called + would add a 1 to the $html string being created.
The resulting $html string would not contain the nonce field and the nonce field would not be between the <form> tags.

Fixed by setting the $display parameter to false.

Ref: https://developer.wordpress.org/reference/functions/wp_nonce_field/

QA: remove redundant code

The explode() function will always return an array, so strict comparing the return value with an empty string will never result in a passing condition.

Ref: https://www.php.net/manual/en/function.explode.php

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

  • Check that the newsletter form still works as expected
  • In the plugin settings, Display tab, section Show original item, enable In a column in the Post list and save
  • clone a post in the post list
  • quick edit the copy and check that you can see Delete reference to original item. The original item this was copied from is: <title of the original post>

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Block/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

QA can test this PR by following these steps:

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label and noted the work hours.

Fixes #

@coveralls
Copy link

coveralls commented Nov 20, 2023

Pull Request Test Coverage Report for Build 6985579179

  • 1 of 2 (50.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 50.203%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ui/newsletter.php 0 1 0.0%
Totals Coverage Status
Change from base Build 6985565176: 0.04%
Covered Lines: 1236
Relevant Lines: 2462

💛 - Coveralls

 The `Column::quick_edit_remove_original()` method only expects one parameter, so no need to ask WP for 2.
The `wp_nonce_field()` function by default _displays_ the field.

This means that, as things were, the nonce field would be echo'd out when the function was called + would add a `1` to the `$html` string being created.
The resulting `$html` string would not contain the nonce field and the nonce field would not be between the `<form>` tags.

Fixed by setting the `$display` parameter to `false`.

Ref: https://developer.wordpress.org/reference/functions/wp_nonce_field/
The `explode()` function will always return an array, so strict comparing the return value with an empty string will never result in a passing condition.

Ref: https://www.php.net/manual/en/function.explode.php
@jrfnl jrfnl force-pushed the JRF/various-small-bug-fixes branch from fbca20c to 4bfa2e0 Compare November 24, 2023 23:35
Copy link
Member

@enricobattocchi enricobattocchi left a comment

Choose a reason for hiding this comment

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

CR + test OK!

@enricobattocchi enricobattocchi merged commit 02c05ef into trunk Nov 25, 2023
23 checks passed
@enricobattocchi enricobattocchi deleted the JRF/various-small-bug-fixes branch November 25, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants