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

Sync post status #42

Merged
merged 9 commits into from
Feb 12, 2024
Merged

Sync post status #42

merged 9 commits into from
Feb 12, 2024

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Feb 6, 2024

All Submissions:

Changes proposed in this Pull Request:

Adds post status synchronization. The functional change itself is small (ffec109), most of the diff is a refactor converting the global/base Distributor hooks file to a class (af03b03).

See 1205909204837811-as-1206399433499327/f

How to test the changes in this Pull Request:

  1. In the Hub site, publish a post and distribute it
  2. Observe it's been distributed as expected on the Node site
  3. Trash the post on the Hub
  4. Observe it's trashed on the Node
  5. For a good measure, feel free to test other statuses

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?

@adekbadek adekbadek mentioned this pull request Feb 6, 2024
6 tasks
Base automatically changed from fix/yoast-primary-cat to trunk February 9, 2024 07:48
@adekbadek adekbadek marked this pull request as ready for review February 9, 2024 08:13
@adekbadek adekbadek requested a review from a team as a code owner February 9, 2024 08:13
@leogermani
Copy link
Contributor

@adekbadek The changes look good and work fine, but I want to ask you opinion on whether we should this syncing in such a broad way.

The feature request mentioned specifically the use case of trashing a post.

I'm afraid that if we do it in this way we might introduce some unintentional behaviors and inconsistencies. Looking at distributor repo, you can see that this is not a simple topic, and that's why they don't support it yet.

Look the description in this issue:

10up/distributor#112

And then see that there's a PR that never got merged 10up/distributor#446

So it seems to me that there might be some use cases we are not accounting for and simply syncing the post status in this way might break some editorial flows.

I can't really think of anything but I would feel more comfortable if we only covered what we know is an issue in the current workflow, which is trashing the post (and maybe restoring?). WDYT?

@leogermani
Copy link
Contributor

ah, and I pushed one little fix needed after all the code was put inside a class

@adekbadek
Copy link
Member Author

@leogermani – I've added a check so that only publish and trash statuses are synchronised.

Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Thanks!

@adekbadek adekbadek merged commit fd5d8b9 into trunk Feb 12, 2024
3 checks passed
@adekbadek adekbadek deleted the fix/sync-post-status branch February 12, 2024 19:57
matticbot pushed a commit that referenced this pull request Feb 15, 2024
# [1.2.0-alpha.2](v1.2.0-alpha.1...v1.2.0-alpha.2) (2024-02-15)

### Bug Fixes

* assorted error handling fixes ([#52](#52)) ([234f883](234f883))
* dynamic class property deprecated warnings ([#47](#47)) ([693865c](693865c))
* prevent coauthors capability check infinite loop ([#46](#46)) ([9565cef](9565cef))
* set Yoast primary category ([#41](#41)) ([3457d19](3457d19))

### Features

* sync billing and shipping addresses ([#50](#50)) ([6a05580](6a05580))
* sync publish, trash post statuses ([#42](#42)) ([fd5d8b9](fd5d8b9))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.2.0-alpha.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Feb 20, 2024
# [1.2.0](v1.1.0...v1.2.0) (2024-02-20)

### Bug Fixes

* assorted error handling fixes ([#52](#52)) ([234f883](234f883))
* **data-listeners:** handle no user in wc data update ([4eeefc0](4eeefc0))
* dynamic class property deprecated warnings ([#47](#47)) ([693865c](693865c))
* prevent coauthors capability check infinite loop ([#46](#46)) ([9565cef](9565cef))
* race condition when creating a Node ([1946473](1946473))
* set Yoast primary category ([#41](#41)) ([3457d19](3457d19))

### Features

* **ci:** add epic/* release workflow and rename `master` to `trunk` ([#39](#39)) ([9cee51d](9cee51d))
* sync billing and shipping addresses ([#50](#50)) ([6a05580](6a05580))
* sync publish, trash post statuses ([#42](#42)) ([fd5d8b9](fd5d8b9))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.2.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants