Skip to content

fix: make sure fix duplcate fields apply filters#3971

Merged
leogermani merged 1 commit into
trunkfrom
fix/fix-duplicate-fields-cli
May 20, 2025
Merged

fix: make sure fix duplcate fields apply filters#3971
leogermani merged 1 commit into
trunkfrom
fix/fix-duplicate-fields-cli

Conversation

@leogermani

@leogermani leogermani commented May 12, 2025

Copy link
Copy Markdown
Contributor

All Submissions:

Changes proposed in this Pull Request:

The fix duplicate scripts is not accountig for fields added via a filter. This PR fixes it with as little refactor as possible.

How to test the changes in this Pull Request:

  1. Activate Newspack Network
  2. Add some debug to the get_fields_to_check_for_duplicates method
  3. On trunk confirm that the NP_Network Registration Site field is not present
  4. On this branch, confirm it is present - with the NP_ prefix.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@leogermani leogermani self-assigned this May 12, 2025
@leogermani leogermani requested a review from a team as a code owner May 12, 2025 17:55
@leogermani leogermani added the [Status] Needs Review The issue or pull request needs to be reviewed label May 12, 2025
dkoo
dkoo approved these changes May 12, 2025
View reviewed changes

@dkoo dkoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This works, so approving! But nit: couldn't we just use Metadata::get_keys() instead of Metadata::get_all_fields() and avoid having to reapply the newspack_ras_metadata_keys filter?

@github-actions github-actions Bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels May 12, 2025
@leogermani

leogermani commented May 20, 2025

Copy link
Copy Markdown
Contributor Author

This works, so approving! But nit: couldn't we just use Metadata::get_keys() instead of Metadata::get_all_fields() and avoid having to reapply the newspack_ras_metadata_keys filter?

@dkoo thanks!

get_keys has this:

$fields = Donations::is_platform_wc() ? self::get_all_fields() : self::get_basic_fields();

So if the donation platform is not WC, we won't get all the fields... I know this should not be a problem in most cases, but I just wanted to make sure that the CLI command will always get ALL the fields no matter what, even if the site is not properly configured.

We could refactor things on the Metadata class to avoid having to reapply the filter, but I think that would be more complex, and this is just a CLI.. so I thought it was fine

@leogermani leogermani merged commit f361a4e into trunk May 20, 2025
9 checks passed
@leogermani leogermani deleted the fix/fix-duplicate-fields-cli branch May 20, 2025 22:00
@github-actions

github-actions Bot commented May 20, 2025

Copy link
Copy Markdown

Hey @leogermani, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

matticbot pushed a commit that referenced this pull request May 23, 2025
@semantic-release-bot
chore(release): 6.7.0-alpha.1 [skip ci]
0c4fdf3 
# [6.7.0-alpha.1](v6.6.2...v6.7.0-alpha.1) (2025-05-23)

### Bug Fixes

* **404-images:** use JS without modifying content ([#3963](#3963)) ([9f5646b](9f5646b))
* add missing namespace ([#3980](#3980)) ([6d58793](6d58793))
* **emails:** add missing HTML markup in the change-email-cancel template ([#3981](#3981)) ([040ae30](040ae30))
* **ga4:** fire login/registration activities via SSO ([#3965](#3965)) ([8c97515](8c97515))
* hide modal content gate when modal checkout is opened ([#3953](#3953)) ([a503973](a503973))
* **jetpack:** handle the related posts max age option ([#3964](#3964)) ([8aad2b8](8aad2b8))
* make sure fix duplcate fields apply filters ([#3971](#3971)) ([f361a4e](f361a4e))
* namespace Lite Site ([#3975](#3975)) ([e4665ae](e4665ae))
* sync correction status with parent post status ([#3978](#3978)) ([dcd5a12](dcd5a12))

### Features

* add compatibility to network in custom bylines ([#3972](#3972)) ([199a993](199a993))
* add icons repository and remove custom icons ([#3883](#3883)) ([e56d2e0](e56d2e0))
* **analytics:** "My Account" dashboard interactions ([#3949](#3949)) ([22e9590](22e9590))
* **donations:** update notice style and type ([#3962](#3962)) ([3f60ef3](3f60ef3))
* **email-change:** remove env constant requirement ([#3943](#3943)) ([4158bf1](4158bf1))
* **my-account:** apply Newspack UI styles to My Account w/ env constant ([#3951](#3951)) ([e4aa5a2](e4aa5a2))
* **my-account:** full-site takeover template and custom nav menu ([#3974](#3974)) ([5cf8403](5cf8403))
* **woocommerce:** log error notices ([#3952](#3952)) ([1654007](1654007))
@matticbot

matticbot commented May 23, 2025

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 6.7.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Jun 2, 2025
@semantic-release-bot
chore(release): 6.7.0 [skip ci]
abf08de 
# [6.7.0](v6.6.4...v6.7.0) (2025-06-02)

### Bug Fixes

* **404-images:** use JS without modifying content ([#3963](#3963)) ([9f5646b](9f5646b))
* add missing namespace ([#3980](#3980)) ([6d58793](6d58793))
* **emails:** add missing HTML markup in the change-email-cancel template ([#3981](#3981)) ([040ae30](040ae30))
* **ga4:** fire login/registration activities via SSO ([#3965](#3965)) ([8c97515](8c97515))
* hide modal content gate when modal checkout is opened ([#3953](#3953)) ([a503973](a503973))
* **jetpack:** handle the related posts max age option ([#3964](#3964)) ([8aad2b8](8aad2b8))
* make sure fix duplcate fields apply filters ([#3971](#3971)) ([f361a4e](f361a4e))
* namespace Lite Site ([#3975](#3975)) ([e4665ae](e4665ae))
* prevent auto-publishing corrections when scheduling posts ([#4006](#4006)) ([7531832](7531832))
* sync correction status with parent post status ([#3978](#3978)) ([dcd5a12](dcd5a12))

### Features

* add compatibility to network in custom bylines ([#3972](#3972)) ([199a993](199a993))
* add icons repository and remove custom icons ([#3883](#3883)) ([e56d2e0](e56d2e0))
* **analytics:** "My Account" dashboard interactions ([#3949](#3949)) ([22e9590](22e9590))
* **donations:** update notice style and type ([#3962](#3962)) ([3f60ef3](3f60ef3))
* **email-change:** remove env constant requirement ([#3943](#3943)) ([4158bf1](4158bf1))
* **my-account:** apply Newspack UI styles to My Account w/ env constant ([#3951](#3951)) ([e4aa5a2](e4aa5a2))
* **my-account:** full-site takeover template and custom nav menu ([#3974](#3974)) ([5cf8403](5cf8403))
* **woocommerce:** log error notices ([#3952](#3952)) ([1654007](1654007))
@matticbot

matticbot commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 6.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Reviewers

@dkoo dkoo dkoo approved these changes

Assignees

@leogermani leogermani

Labels

released on @alpha released [Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@leogermani @matticbot @dkoo