Sync: Introduce helper function and CPT for Activity Log entries#48567
Sync: Introduce helper function and CPT for Activity Log entries#48567coder-karen wants to merge 13 commits intotrunkfrom
Conversation
…lly meant to pass information to the Activity Log
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Automattic For agencies client plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcloud Sso plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 2 files.
1 file is newly checked for coverage.
|
There was a problem hiding this comment.
Great work here, Karen! Here are some generic thoughts before manually testing this:
composer.json and global function approach
Thinking about this more, I wonder if it makes sense to expose logging a custom event to the Jetpack Activity Log via a global function. If we introduced a new class, for example, we'd be more inline with the rest of the Sync package functionality plus because the Sync package already classmaps src/, a new class file under projects/packages/sync/src/ should not require a composer.json autoload.files change.
The new class just needs to be referenced from an existing loaded path, e.g. Actions::init() for CPT registration and Modules\Posts for validation.
Responsibilities:
Activity_Log_Event- owns POST_TYPE = 'jp_act_log_event'
- registers the CPT
- sanitizes title, content, source, severity
- builds the JSON payload for post_content
- validates existing CPT posts before Sync sends them
- creates the post via wp_insert_post()
Modules\Posts- calls
Activity_Log_Event::is_valid_post( $post )before enqueueingjetpack_published_post - otherwise stays focused on Sync behavior
- calls
One more thing I'd like to discuss is preventing edits/deletes from being synced for this CPT. I think we should allow the normal save/delete Sync actions through so the remote site and Cache site DB stay consistent. The important boundary is on the Activity Log side: jp_act_log_event saves/deletes should be blacklisted from generic post activities, and only the initial valid jetpack_published_post should create a custom_event__created Activity Log entry.
Yes, good call. I used a global function because it’s a familiar WordPress-style API and makes the caller side very simple, but I agree a class fits this package better. It avoids the extra autoload.files handling and gives the Activity Log event code one clear home for the CPT, sanitization, payload creation, and validation.
Also agreed on that. I was thinking of the CPT as write-once from an Activity Log perspective, but normal Sync should still keep the remote/cache DB in step like you say. |
fgiannar
left a comment
There was a problem hiding this comment.
Many thanks for working on this, Karen!
Tested manually and it works as described 👍
Left a few inline comments.
| return false; | ||
| } | ||
|
|
||
| foreach ( array( 'title', 'content' ) as $field ) { |
There was a problem hiding this comment.
Perhaps we could reuse the extra checks from build_payload( $data ) here by adding a check like false !== self::build_payload( $data ) after we decode.
This way posts with invalid severity, scalar content that sanitizes to empty, etc. wii not enqueue through jetpack_published_post
There was a problem hiding this comment.
Great idea. Added along with other changes in aebafc0
| /** | ||
| * Logs a custom event to the Jetpack Activity Log. | ||
| * | ||
| * Call this on or after the `init` action so the Activity Log post type and Sync listeners are registered. |
There was a problem hiding this comment.
I saw we added this comment in 813be65 to make it clear that create should be called after init but perhaps we could attempt to register the post type inside this method to be more defensive?
…lso match current behavior, and try to prevent invalid CPTs on creation with core hooks - based on internal reviews
fgiannar
left a comment
There was a problem hiding this comment.
LGTM 🚢
Tested with a Jetpack site (local env) but also in my sandboxed Simple site and everything works as expected 👍
fgiannar
left a comment
There was a problem hiding this comment.
Apologies I missed this!
| 'show_ui' => false, | ||
| 'show_in_menu' => false, | ||
| 'show_in_nav_menus' => false, | ||
| 'show_in_rest' => false, |
There was a problem hiding this comment.
Sorry it just occurred to me that we need this to be true in order to be able to create custom Activity Log entries via the REST API. Probably also need to add 'rest_base' => 'jp-activity-log-events' or similar
Fixes ACTLOG-57
Proposed changes
Adds a Jetpack Sync helper for creating custom customer-visible Activity Log entries.
Automattic\Jetpack\Sync\Activity_Log_Eventas the API for creating custom Activity Log events.jp_act_log_eventCPT for transporting custom Activity Log event payloads through Sync.post_content.titleandcontentsourceseveritylimited toinfo,success,warning, orerrorjetpack_published_postSync pipeline.jp_act_log_eventposts so remote/cache DB state stays consistent.jp_act_log_eventinserts so invalid severity values, empty sanitized content, and malformed payloads are not enqueued throughjetpack_published_post.Related product discussion/links
This is part of an RSM project to add custom entries to the Activity Log. See Linear link above for more.
Requires the matching WPcom Activity Log handling in 215249-ghe-Automattic/wpcom, which translates valid
jp_act_log_eventposts intocustom_event__createdentries and prevents malformed payloads from falling through as generic post publish events.Does this pull request change what data or activity we track or use?
No.
Testing instructions
To test, apply this PR on a test site using the Jetpack Beta tester plugin, for the Jetpack plugin (or apply the PR if testing locally).
yoursite.com/wp-admin/?test_activity_log=1case 'jetpack_published_post':inJetpack_Sync_Server_Replicator