fix: do not trigger site.changed when draft posts are deleted#22523
Conversation
WalkthroughThe update modifies the Assessment against linked issues
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@ErisDS can you please have a look :) |
|
Hey there @im-adithya, I see you've made quite a few PRs with bugfixes, really appreciate the effort 🙏 We have a working practice of always adding a unit test to demonstrate a bug fix, otherwise it's liable to either not be fixed properly, or regress again. How do you feel about adding tests to your changes? Cheers! |
|
Note from our bot: Some changes have been requested on this pull request. Updating your code is great, but won't notify us, so please leave a comment so that we (and our bot) can see when you've made the changes. Thank you 🙏 |
|
Hey Hannah! Thanks for noticing, would be more than happy to have them reviewed :) Absolutely, I added the test now. The test files (atleast the ones in e2e-webhooks) only had the checks for single addition/deletion, so I only checked deletion of a single draft post. Let me know if I should add for bulk deletion too! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/e2e-webhooks/site.test.js (1)
157-157: Minor inconsistency in variable naming.The variable name
requestWasReceiveduses a different spelling compared torequestWasRecievedon line 81. Consider standardizing the spelling across all tests for better code consistency.- const requestWasReceived = await Promise.race([ + const requestWasRecieved = await Promise.race([Though I personally recommend fixing the other occurrence to use the correct spelling "received" instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/test/e2e-webhooks/site.test.js(1 hunks)
🔇 Additional comments (1)
ghost/core/test/e2e-webhooks/site.test.js (1)
127-165: Well-structured test case for verifying draft post deletion behavior.This test case correctly verifies that the
site.changedevent is not triggered when draft posts are deleted, which aligns with the PR objective. The test follows the same pattern as other tests in the file and effectively validates the fix for issue #21699.
|
Hi @im-adithya. Thank you for picking up my bug report! Do you see a way to apply the bugfix to draft pages as well? I checked 5.115.0, and deleting draft pages is also triggering the site.changed webhook. |
5233f32 to
a9a70fb
Compare
Applies the draft-aware cache invalidation guard to pages and lightens the status lookups to fetch only the status column. Adds e2e-webhooks coverage for the bulk-posts path, draft pages (single + bulk), and guards that published post/page deletion still invalidates the cache.
a9a70fb to
4c4cecf
Compare
Fixes #21699
Description
This adds a check to see if all of the posts being deleted are drafted, and only sets
cacheInvalidateif not (sincesite.changedis triggered because ofX-Cache-Invalidateheader)Checklist
yarn test:allandyarn lint)