Skip to content

fix: move optional module settings from Syndication to own class#3858

Merged
dkoo merged 3 commits into
epic/iafrom
fix/optional-modules-class
Mar 26, 2025
Merged

fix: move optional module settings from Syndication to own class#3858
dkoo merged 3 commits into
epic/iafrom
fix/optional-modules-class

Conversation

@dkoo

@dkoo dkoo commented Mar 24, 2025

Copy link
Copy Markdown
Contributor

All Submissions:

Changes proposed in this Pull Request:

#3783 introduced another optional module, but one that's not related to the Syndication section in our dashboard. Since #3783 expects the optional module settings to be available in the Newspack\Settings class, which has been deprecated with its methods moved to other classes throughout the wizard app, it's currently throwing a fatal.

This PR moves all methods and constants related to optional modules to a new Optional_Modules class for clarity.

How to test the changes in this Pull Request:

  1. On epic/ia, observe a fatal error when loading any WP admin page.
  2. Check out this branch and confirm the fatal no longer occurs.
  3. Confirm that the settings in the Newspack > Settings > Syndication page display and update as expected.
  4. Confirm that testing instructions in feat(woo-member-commenting): optional module for member commenting #3783 work as expected.

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?

@dkoo dkoo requested a review from a team as a code owner March 24, 2025 20:23
@dkoo dkoo added the [Status] Needs Review The issue or pull request needs to be reviewed label Mar 24, 2025
@dkoo dkoo self-assigned this Mar 24, 2025
@dkoo dkoo mentioned this pull request Mar 24, 2025
6 tasks

@adekbadek adekbadek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Changes look and work ok, though I don't see the fatal on epic/ia.

Comment thread tests/unit-tests/syndication.php
@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 Mar 25, 2025
@miguelpeixe

Copy link
Copy Markdown
Member

@dkoo is there anything blocking this merge? We should get IA to alphas.

@dkoo dkoo merged commit 0e12017 into epic/ia Mar 26, 2025
@dkoo dkoo deleted the fix/optional-modules-class branch March 26, 2025 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Information Architecture [Status] Approved The pull request has been reviewed and is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants