Fix is_read value in Admin notes

[simranvijwani]

Submission Review Guidelines:

• I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have reviewed my code for security best practices.
• Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

Closes 16504-gh-Automattic/woocommerce.com.

We were getting the below error in logs while deleting any vendor note:

WordPress database error Incorrect integer value: '' for column `wccom`.`wp_wc_admin_notes`.`is_read` at row 1 for query UPDATE `wp_wc_admin_notes` 
SET `name` = 'ww', `type` = 'info', `locale` = 'en_US', `title` = 'ww', `content` = 'ww', `content_data` = '{}', `status` = 'unactioned', `source` = 'wccom-vendor-notes', `date_created` = '2023-12-22 22:12:51', `date_reminder` = NULL, `is_snoozable` = '0', `layout` = 'plain', `image` = '', `is_deleted` = '1', `is_read` = '' 
WHERE `note_id` = '132' made by Automattic\WooCommerce\Admin\Notes\DataStore::update` 

This was happening because is_read value was not set with correct datatype in create and read function in core: https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/src/Admin/Notes/DataStore.php#L66

The datatype for the is_read is tinyint(1).

  `is_read` tinyint(1) NOT NULL DEFAULT 0,

• In this PR, we're adding is_read value to create and read functions and removed unnecessary type casting.

How to test the changes in this Pull Request:

Test for local Woo.com environments

This test is intended for the Marketplace Engineering teams. If you're doing testing for the WooCommerce release, it's fine to disregard this section.

We want to create a Vendor Note on our local WCCOM.

1. Create the zip file from this branch. First comment out or delete this line in plugins/woocommerce/bin/build-zip.sh. It is unnecessary for this test, and demands massive amounts of RAM which your local environment probably doesn't have.

pnpm --filter=@woocommerce/plugin-woocommerce makepot || exit "$?"

2. In your project, do cd plugins/woocommerce and pnpm run build:zip.
3. In the plugins admin of your local WCCOM environment upload the zip file the previous step created in the plugins/woocommerce folder of your WooCommerce project.
4. This should replace the existing version woocommerce version in your local WCCOM.
5. If you get Fatal error: Uncaught Exception: The authoritative table for orders storage can't be changed while there are orders out of sync error after activating the plugin, run wp wc cot sync in CLI to sync your existing orders.
6. Comment out this line of code: 19118-gh-Automattic/woocommerce.com/files#diff-cef8f1318e4c4bf8336fab0cef21d113214145b08456153b726ce2cb030ac533R80 if present.
7. The following steps are done in the vendor notes admin in WCCOM. First create a vendor note at https://woocommerce.test/wp-admin/admin.php?page=wccom-vendor-notes-admin.
8. Confirm the vendor note is getting created. Does it appear in the wp_wc_admin_notes table of your WCCOM database?
9. Delete the vendor note in the admin UI. Check the vendor note is getting deleted. Does the note record in the DB now have 1 for is_deleted?
10. Go to the "Deleted" list in the admin UI, and restore the vendor note. Does the note record in the DB now have 0 for is_deleted?

Test for WooCommerce: activity panel notes

This tests that we haven't broken Activity Panel Notes, which also use this class.

1. Create a PHP file in your wp-content/plugins folder and paste in this content. This is taken from the example in the WooCommerce repo, with some added namespacing. (I've corrected the trunk version of the Markdown file in this PR.)

<?php
/**
 * Plugin Name: WooCommerce Activity Panel Inbox Example Plugin One
 * Plugin URI: https://woo.com/
 * Description: An example plugin.
 * Author: Automattic
 * Author URI: https://woo.com/
 * Text Domain: wapi-example-one
 * Version: 1.0.0
 */

use Automattic\WooCommerce\Admin\Notes\Note as Note;

class WooCommerce_Activity_Panel_Inbox_Example_Plugin_One {
	const NOTE_NAME = 'wapi-example-plugin-one';

	/**
	 * Adds a note to the merchant' inbox.
	 */
	public static function add_activity_panel_inbox_welcome_note() {
		if ( ! class_exists( 'Automattic\WooCommerce\Admin\Notes\Notes' ) ) {
			return;
		}

		if ( ! class_exists( 'WC_Data_Store' ) ) {
			return;
		}

		$data_store = WC_Data_Store::load( 'admin-note' );

		// First, see if we've already created this kind of note so we don't do it again.
		$note_ids = $data_store->get_notes_with_name( self::NOTE_NAME );
		foreach( (array) $note_ids as $note_id ) {
			$note         = Notes::get_note( $note_id );
			$content_data = $note->get_content_data();
 			if ( property_exists( $content_data, 'getting_started' ) ) {
				return;
			}
		}

		// Otherwise, add the note
		$activated_time = current_time( 'timestamp', 0 );
		$activated_time_formatted = date( 'F jS', $activated_time );
		$note = new Note();
		$note->set_title( __( 'Getting Started', 'wapi-example-plugin-one' ) );
		$note->set_content(
			sprintf(
				/* translators: a date, e.g. November 1st */
				__( 'Plugin activated on %s.', 'wapi-example-plugin-one' ),
				$activated_time_formatted
			)
		);
		$note->set_content_data( (object) array(
			'getting_started'     => true,
			'activated'           => $activated_time,
			'activated_formatted' => $activated_time_formatted,
		) );
		$note->set_type( Note::E_WC_ADMIN_NOTE_INFORMATIONAL );
		$note->set_layout('plain');
		$note->set_image('');
		$note->set_name( self::NOTE_NAME );
		$note->set_source( 'wapi-example-plugin-one' );
		$note->set_layout('plain');
		$note->set_image('');
		// This example has two actions. A note can have 0 or 1 as well.
		$note->add_action(
			'settings',
			__( 'Open Settings', 'wapi-example-plugin-one' ),
			'?page=wc-settings&tab=general'
		);
		$note->add_action(
			'settings',
			__( 'Learn More', 'wapi-example-plugin-one' ),
			'https://github.com/woocommerce/woocommerce-admin/tree/main/docs'
		);
		$note->save();
	}

	/**
	 * Removes any notes this plugin created.
	 */
	public static function remove_activity_panel_inbox_notes() {
		if ( ! class_exists( 'Notes' ) ) {
			return;
		}

		Notes::delete_notes_with_name( self::NOTE_NAME );
	}
}

function wapi_example_one_activate() {
	WooCommerce_Activity_Panel_Inbox_Example_Plugin_One::add_activity_panel_inbox_welcome_note();
}
register_activation_hook( __FILE__, 'wapi_example_one_activate' );

function wapi_example_one_deactivate() {
	WooCommerce_Activity_Panel_Inbox_Example_Plugin_One::remove_activity_panel_inbox_notes();
}
register_deactivation_hook( __FILE__, 'wapi_example_one_deactivate' );

2. Go to /wp-admin/plugins.php. Activate the plugin "WooCommerce Activity Panel Inbox Example Plugin One".

3. View the wp_wc_admin_notes table in your local DB, and notice that the plugin has added a new record.

4. Go to a WooCommerce admin page like wp-admin/admin.php?page=wc-settings&tab=general. Click on the "Activity" button in the top right of your screen and notice this note that the plugin has created.

5. Click on the "Open Settings" button in the note.
6. Refresh the DB record and note that the record is marked is_read.
7. View the Activity Inbox and see that the note no longer appears.

Changelog entry

• Automatically create a changelog entry from the details below.

Significance

• Patch
• Minor
• Major

Type

• Fix - Fixes an existing bug
• Add - Adds functionality
• Update - Update existing functionality
• Dev - Development related task
• Tweak - A minor adjustment to the codebase
• Performance - Address performance issues
• Enhancement - Improvement to existing functionality

Message

Fix is_read value in Admin notes.

Comment

[github-actions]

Test Results Summary

Commit SHA: a63502f

Test 🧪	Passed ✅	Failed 🚨	Broken 🚧	Skipped ⏭️	Unknown ❔	Total 📊	Duration ⏱️
API Tests	259	0	0	2	0	261	0m 38s
E2E Tests	331	0	0	26	0	357	8m 16s

---

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

[github-actions]

Hi @Dan-Q, @andfinally, @raicem,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[github-actions]

Hi @andfinally, @raicem,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[github-actions]

Hi @andfinally, @raicem,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[andfinally]

I like this simpler approach: it's more minimal than earlier versions of this PR. And it fixes the WCCOM issue 16504-gh-Automattic/woocommerce.com. It would be helpful to have a more detailed explanation of why we need to do this, though. In particular, what goes wrong when we cast is_deleted or is_read as Boolean in DataStore::read?

I tried this test from woocommerce/woocommerce-admin#6322, and it created Notes with is_deleted and is_read set to 1 in the DB table as expected.

add_action( 'plugins_loaded', function () {
	$note = new Automattic\WooCommerce\Admin\Notes\Note();
	$note->set_name( 'wc-undismissable-note' );
	$note->set_title( 'Undismissable note' );
	$note->set_content( 'I don\'t wanna dismiss a thing' );
	$note->set_content_data( (object) array() );
	$note->set_type( 'info' );
	$note->set_source( 'woocommerce-admin' );
	$note->add_action( 'learn-more', __( 'Learn more', 'woocommerce' ), 'https://woocommerce.com/', 'actioned', true );
	$note->set_is_deleted( true );
	$note->set_is_read( 1 );
	$note->save();

	$note = new Automattic\WooCommerce\Admin\Notes\Note( $note->get_id() );
	var_dump( $note->get_is_deleted() ); // Should return `true`.
	exit;
} );

BTW, I noticed that the return types defined for Note::get_is_read and Note::get_is_deleted are incorrect – they should be boolean instead of array, same as Note::get_is_snoozable.

[simranvijwani]

@andfinally, I have noted your suggestion related to fixing the return type of get_is_read and get_is_deleted in phpdoc.

Since we're having the discussion in the PR: https://github.com/Automattic/woocommerce.com/pull/19118, I'll commit the suggestion after our discussion.

[simranvijwani]

@andfinally, I have moved the typecasting to the update function and fixed the return type of get_is_read and get_is_deleted in phpdoc. I'd appreciate it if you could review the PR again.

[raicem]

@andfinally Hey, would you like me to review this? I noticed it's been sitting here for a while.

[andfinally]

That would be great, thanks @raicem! I'm reviewing again myself right now, but would love to have another pair of eyes to have some assurance we won't break Notes.

--- EDIT ---

I've figured out how to test the basic core Activity Notes functionality – added to the test steps.

[andfinally]

Thanks Simran! Changes look logical, fix the original problem in WCCOM, and as far as I can tell, don't break Activity Notes. 👍

[andfinally]

@raicem I've corrected the example plugin code for Activity Panel notes in #44504 – it would be awesome if you get a chance to give that a go while testing this.

[raicem]

Looks good and works as described, thanks @simranvijwani and @andfinally!

[andfinally]

I'm struggling to get the automated test builds to run on this one. Rebasing didn't help, unfortunately.

[raicem]

@andfinally I see tests fail because of a particular failure at woocommerce/tests/legacy/unit-tests/woocommerce-admin/notes/class-wc-tests-notes-data-store.php:48.

$this->assertEquals( $note->get_is_snoozable(), '0' !== $read_note->get_is_snoozable() );

I read we converted isSnoozeable to be a boolean and not a string, so that might be failing this test. Maybe changing that to this will fix it?

$this->assertEquals( $note->get_is_snoozable(), false === $read_note->get_is_snoozable() );

[andfinally]

Thanks Cem, excellent point! This got the build working again.