Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update distributions after post terms and meta is saved. #938

Merged
merged 4 commits into from
Sep 16, 2022

Conversation

peterwilsoncc
Copy link
Collaborator

@peterwilsoncc peterwilsoncc commented Sep 12, 2022

Description of the Change

This moves the updating of distributions during edits from the save_post hook to the wp_after_insert_post hook.

In updates via both the classic and REST API, WordPress fires this hook after all terms and post meta is updated. This prevents the distribution of legacy data described in the issue.

Closes #399

How to test the Change

  1. Install this mini-plugin that updates post meta with each save
  2. On the main develop branch
    a) Create and distribute a post via the block editor
    b) Update the post
    c) Check the database -- observe the value of the post meta differs between source and distribution (one less)
  3. Switch to this branch
  4. Resave the post in the block editor
  5. observe the post meta matches in both the source and distributed material.

Changelog Entry

Fixed - Ensure post meta and terms have saved prior to distribution.

Credits

Props @peterwilsoncc, @dinhtungdu, @dkotter, @timstl, @faisal-alvi.

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@peterwilsoncc peterwilsoncc marked this pull request as ready for review September 12, 2022 05:15
@peterwilsoncc
Copy link
Collaborator Author

I tried writing an E2E test for this (using tags rather than meta as the UI is already exposed) but I will need some help from some one more familiar with cyrpress.

@jeffpaul jeffpaul added this to the 2.0.0 milestone Sep 12, 2022
@jeffpaul jeffpaul requested review from a team, ravinderk and faisal-alvi and removed request for a team and ravinderk September 12, 2022 16:28
Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@peterwilsoncc Code LGTM but the issue is not regenerating for me in the develop branch.

Please check the video:

dist-938-not-regenerating.mp4

Let me know if I'm not testing it correctly.

@peterwilsoncc
Copy link
Collaborator Author

@faisal-alvi Your tests process is correct, so I am unclear why we are seeing different results. I do have the classic editor installed so I wonder if it's an interaction with that. You'll see in the video that the distributed block on xu-distributor.local shows as a classic block.

Not in the video, I start on this branch so the values are in sync. The values go out of sync once I switch to develop. https://youtu.be/8grzn3YdWXk

Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@peterwilsoncc thanks for the video. I have retried and this time it regenerated for me in the develop and worked fine in the PR branch.

image

@peterwilsoncc peterwilsoncc merged commit 8bdf2dc into develop Sep 16, 2022
@peterwilsoncc peterwilsoncc deleted the fix/399-notify-after-meta-term-updates branch September 16, 2022 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Delay send_notifications until after all meta has saved (Yoast SEO problem).
3 participants