feat(corrections): add corrections and clarifications behind feature flag #3638

chickenn00dle · 2024-12-18T20:53:10Z

All Submissions:

Have you followed the Newspack Contributing guideline?
Does your code follow the WordPress' coding standards and VIP Go coding standards?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes proposed in this Pull Request:

Closes https://app.asana.com/0/1208894036683495/1208945901567236 and https://app.asana.com/0/1208894036683495/1208945901567238

This PR adds the corrections feature behind the NEWSPACK_CORRECTIONS_ENABLED feature flag. Note this is a quick and simple implementation based on an already existing custom plugin strictly for the purpose of getting ready for a site migration.

Editor:

Frontend:

How to test the changes in this Pull Request:

Enable the corrections feature flag in wp-config (NEWSPACK_CORRECTIONS_ENABLED)
Edit any post as admin
Scroll to the bottom of the content editor and verify the corrections metabox is present
Select the activate corrections checkbox, select a location, and add some corrections
As a reader visit the post on the front end
Verify the corrections are added to the post
As admin again, select a different corrections location
As reader again, visit the post and verify the corrections have moved
As admin again, uncheck the activate corrections checkbox and save
As reader again, visit the post and verify the corrections are NOT added to the post
As admin, delete a revision and save
As reader, verify the revisions appear as expected on the frontend
As admin, remove the feature flag from wp-config and edit any post
Verify corrections are no longer present

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?

chickenn00dle · 2025-01-07T23:01:07Z

TODO:

Update corrections
Refresh on corrections submit

chickenn00dle · 2025-01-08T22:33:26Z

src/other-scripts/corrections/index.js

+		let hasSavedPost = false;
+		const unsubscribe = subscribe( () => {
+			// Return early if no new corrections have been added.
+			if ( ! newCorrectionsCount ) {
+				return;
+			}
+			const isSavingPost = select( 'core/editor' ).isSavingPost();
+			const isAutosavingPost = select('core/editor').isAutosavingPost();
+
+			if ( isSavingPost && ! isAutosavingPost && ! hasSavedPost ) {
+				hasSavedPost = true;
+			}
+
+			if ( ! isSavingPost && hasSavedPost ) {
+				// Unsubscribe from the store.
+				unsubscribe();
+				window.location.href = window.location.href;
+			}
+		} );
+	}
+} );


This is a super hacky way to force reload the page. We need to reload the page when adding new corrections so the metabox fields can repopulate with the respective correction ids (needed for updates and deleting). Ideally we remove this when we are able to fully work on the feature

chickenn00dle · 2025-01-08T22:35:04Z

Gonna mark this one ready for review since the core functionality is working and this should at least get us to a point where we can begin migrating corrections.

After working on this, my thought is we should re-implement corrections as a new block. I did start working on this a bit, but realized this would imply some design decisions, so for the time being I've gone ahead and just pushed up a rework of our existing corrections custom plugin.

cc @leogermani

leogermani

It works and serves the purpose of unblocking migration.

There is just one warning being thrown at delete_corrections method.

I left a few other non blocking comments

Another non-blocking comment: I think "activate corrections" and "position" should be a global setting, and not something per post... We can discuss it and I don't think this will be a blocker for migration now.... well... maybe we can get rid of the "activate" so it doesn't get in the way.

Finally, WDYT of adding a short README to the folder describing how the data is stored, to inform LEs that will work on the migration?

leogermani · 2025-01-09T15:18:34Z

includes/corrections/class-corrections.php

+	 * @param array $correction_ids correction ids.
+	 */
+	public static function delete_corrections( $correction_ids ) {
+		$stored_correction_ids = get_post_meta( $post_id, self::CORRECTIONS_IDS_META, true );


$post_id doesn't seem to be defined here

Oops! Good catch. Fixed in fea9890

leogermani · 2025-01-09T15:32:09Z

includes/corrections/class-corrections.php

+			'public_queryable' => true,
+			'query_var'        => true,
+			'rewrite'          => [ 'slug' => 'correction' ],
+			'show_ui'          => true,


I don't think we want the UI for that

Also fixed in fea9890

leogermani · 2025-01-09T15:34:33Z

includes/corrections/class-corrections.php

+					'post_status'  => 'publish',
+					'meta_input'   => [
+						self::CORRECTION_POST_ID_META => $post_id,
+						self::CORRECTION_DATE_META    => $correction['date'],


why not store the date using post_date?

I thought about this, but also considered corrections will be viewed in other contexts outside of a post (shortcode, archive, etc.) and so it might be useful to distinguish between the actual date a correction is published and the date assigned to a correction by a publisher as these might not always be the same.

Right, I actually thought that the date the correction was posted is not relevant, so we could use this field as the correction date.. no need to have two.

But I get your point. The only thing is that if we go this route, get_corrections will have to order posts by a postmeta value, that will be a string. If we used the post_date this would be working out of the box

That's a fair point. I've updated this to use post_date instead in 13dbe29

leogermani · 2025-01-09T15:36:53Z

includes/corrections/class-corrections.php

+	 * @param int   $post_id     The post ID.
+	 * @param array $corrections The corrections.
+	 */
+	public static function save_corrections( $post_id, $corrections ) {


nit: naming it add_corrections (or insert) would make the purpose of this method clearer

Good suggestion! I am not very good at naming things. Fixed in fea9890

chickenn00dle · 2025-01-09T16:57:38Z

Thanks for the review @leogermani!

Another non-blocking comment: I think "activate corrections" and "position" should be a global setting, and not something per post... We can discuss it and I don't think this will be a blocker for migration now.... well... maybe we can get rid of the "activate" so it doesn't get in the way.

I think position makes sense as a global setting, but do think it would be useful for publishers to be able to set whether corrections are active at the post level. For example, being able to publish a correction but not set it to active until it has been approved/verified. That said, I am open to any decision here.

Finally, WDYT of adding a short README to the folder describing how the data is stored, to inform LEs that will work on the migration?

Great idea! Added one here: https://github.com/Automattic/newspack-plugin/blob/fea98909ba23a62584c9f97bd3a2adc97bf1be2e/includes/corrections/README.md

Let me know if I need to expand anywhere!

leogermani · 2025-01-09T17:24:37Z

For example, being able to publish a correction but not set it to active until it has been approved/verified. That said, I am open to any decision here.

Makes sense. We don't need to decide this now, it has little impact on the migration. Can I just suggest that this should be active by default?

I'm thinking about LEs migrating data and wanting to look at the site to see if the data is there. Ideally they wouldn't need to add a "active = true" for every post and we'll think about it later

chickenn00dle · 2025-01-09T18:31:55Z

Makes sense. We don't need to decide this now, it has little impact on the migration. Can I just suggest that this should be active by default?

I'm thinking about LEs migrating data and wanting to look at the site to see if the data is there. Ideally they wouldn't need to add a "active = true" for every post and we'll think about it later

That's fair. I've updated this in 0017f16 so when the meta is not present corrections will be shown.

leogermani

Thanks for the revisions!

github-actions · 2025-01-11T14:38:18Z

Hey @chickenn00dle, 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! ❤️

chickenn00dle · 2025-01-11T14:50:55Z

Removing needs-changelog label since this feature is strictly to prep for a migration and won't actually launch quite yet

# [5.12.0-alpha.1](v5.11.1...v5.12.0-alpha.1) (2025-01-14) ### Bug Fixes * **cli:** verify-reader CLI command ([#3660](#3660)) ([c639af7](c639af7)) * **recaptcha:** replace alerts with generic errors ([#3627](#3627)) ([44ef2d2](44ef2d2)) * remove newspack_corrections_ids meta ([#3675](#3675)) ([dad258b](dad258b)) ### Features * **corrections:** add corrections and clarifications behind feature flag ([#3638](#3638)) ([ea745cf](ea745cf)) ### Performance Improvements * **data-events:** queue dispatches to execute on shutdown ([#3616](#3616)) ([510a1a0](510a1a0))

matticbot · 2025-01-14T21:14:57Z

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

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [5.12.0](v5.11.3...v5.12.0) (2025-01-20) ### Bug Fixes * **cli:** verify-reader CLI command ([#3660](#3660)) ([c639af7](c639af7)) * **recaptcha:** replace alerts with generic errors ([#3627](#3627)) ([44ef2d2](44ef2d2)) * remove newspack_corrections_ids meta ([#3675](#3675)) ([dad258b](dad258b)) * **wcs:** migrate-expired-subscriptions handle manual subscriptions ([#3663](#3663)) ([e0f32e8](e0f32e8)) ### Features * **corrections:** add corrections and clarifications behind feature flag ([#3638](#3638)) ([ea745cf](ea745cf)) ### Performance Improvements * **data-events:** queue dispatches to execute on shutdown ([#3616](#3616)) ([510a1a0](510a1a0))

matticbot · 2025-01-20T18:03:42Z

🎉 This PR is included in version 5.12.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

chickenn00dle added the wip label Dec 18, 2024

chickenn00dle force-pushed the feat/add-corrections-and-clarifications branch from 62eb69c to 86453c4 Compare December 18, 2024 22:55

chickenn00dle added 4 commits January 7, 2025 10:34

feat(corrections): add base code and feature flag

2d7e857

feat: add corrections custom post type

7f2b7c4

fix: refactor scripts and styles

bf109bb

feat(corrections): render on frontend

efdc46c

chickenn00dle force-pushed the feat/add-corrections-and-clarifications branch from 86453c4 to efdc46c Compare January 7, 2025 22:37

chickenn00dle added 2 commits January 7, 2025 17:45

fix(corrections): handle shortcode

af06330

fix: filter get input

0208f5d

feat: update corrections

4bae5aa

chickenn00dle removed the wip label Jan 8, 2025

chickenn00dle marked this pull request as ready for review January 8, 2025 21:03

chickenn00dle requested a review from a team as a code owner January 8, 2025 21:03

chickenn00dle force-pushed the feat/add-corrections-and-clarifications branch 4 times, most recently from e743872 to 63d997c Compare January 8, 2025 21:36

feat: add location

94659b0

chickenn00dle force-pushed the feat/add-corrections-and-clarifications branch from 63d997c to 94659b0 Compare January 8, 2025 21:38

fix: reload on new correction

6aed1c0

chickenn00dle commented Jan 8, 2025

View reviewed changes

chickenn00dle added the [Status] Needs Review The issue or pull request needs to be reviewed label Jan 8, 2025

leogermani requested changes Jan 9, 2025

View reviewed changes

fix: add readme and fix errors

fea9890

chickenn00dle force-pushed the feat/add-corrections-and-clarifications branch from 7a49db8 to fea9890 Compare January 9, 2025 16:48

chickenn00dle requested a review from leogermani January 9, 2025 16:57

chickenn00dle added 2 commits January 9, 2025 12:48

fix: use post_date to store correction date

13dbe29

fix: make corrections active by default

0017f16

leogermani approved these changes Jan 9, 2025

View reviewed changes

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 Jan 9, 2025

chickenn00dle merged commit ea745cf into trunk Jan 11, 2025
9 checks passed

github-actions bot added the needs-changelog label Jan 11, 2025

chickenn00dle deleted the feat/add-corrections-and-clarifications branch January 11, 2025 14:38

chickenn00dle removed the needs-changelog label Jan 11, 2025

matticbot added the released on @alpha label Jan 14, 2025

matticbot added the released label Jan 20, 2025

chickenn00dle mentioned this pull request Jan 23, 2025

fix(corrections): replace deprecated sanitize method #3694

Merged

6 tasks

leogermani mentioned this pull request Jan 27, 2025

feat: add corrections archive template Automattic/newspack-theme#2431

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(corrections): add corrections and clarifications behind feature flag #3638

feat(corrections): add corrections and clarifications behind feature flag #3638

chickenn00dle commented Dec 18, 2024 •

edited

Loading

chickenn00dle commented Jan 7, 2025 •

edited

Loading

chickenn00dle Jan 8, 2025 •

edited

Loading

chickenn00dle commented Jan 8, 2025 •

edited

Loading

leogermani left a comment

leogermani Jan 9, 2025

chickenn00dle Jan 9, 2025

leogermani Jan 9, 2025

chickenn00dle Jan 9, 2025

leogermani Jan 9, 2025

chickenn00dle Jan 9, 2025

leogermani Jan 9, 2025

chickenn00dle Jan 9, 2025

leogermani Jan 9, 2025

chickenn00dle Jan 9, 2025

chickenn00dle commented Jan 9, 2025

leogermani commented Jan 9, 2025

chickenn00dle commented Jan 9, 2025

leogermani left a comment

github-actions bot commented Jan 11, 2025

chickenn00dle commented Jan 11, 2025

matticbot commented Jan 14, 2025

matticbot commented Jan 20, 2025

feat(corrections): add corrections and clarifications behind feature flag #3638

feat(corrections): add corrections and clarifications behind feature flag #3638

Conversation

chickenn00dle commented Dec 18, 2024 • edited Loading

All Submissions:

Changes proposed in this Pull Request:

How to test the changes in this Pull Request:

Other information:

chickenn00dle commented Jan 7, 2025 • edited Loading

chickenn00dle Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

chickenn00dle commented Jan 8, 2025 • edited Loading

leogermani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chickenn00dle commented Jan 9, 2025

leogermani commented Jan 9, 2025

chickenn00dle commented Jan 9, 2025

leogermani left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 11, 2025

chickenn00dle commented Jan 11, 2025

matticbot commented Jan 14, 2025

matticbot commented Jan 20, 2025

chickenn00dle commented Dec 18, 2024 •

edited

Loading

chickenn00dle commented Jan 7, 2025 •

edited

Loading

chickenn00dle Jan 8, 2025 •

edited

Loading

chickenn00dle commented Jan 8, 2025 •

edited

Loading