Conversation
ref https://linear.app/ghost/project/retention-offers-churn-prevention-e2474be8b6ad/overview closes https://linear.app/ghost/issue/BER-3410 Using retention offers, publishers can offer existing paid members a discount before they cancel, in order to retain them. For example, "a month on us" or "50% off your next payment". --------- Co-authored-by: Michael Barrett <mike@ghost.org> Co-authored-by: Sodbileg Gansukh <sodbileg.gansukh@gmail.com>
Walkthrough
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Velo CI Failure AnalysisClassification: 🟠 SOFT FAIL
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/test/e2e-browser/utils/e2e-browser-utils.js`:
- Around line 265-266: The current unconditional wait on
page.getByTestId('retention-offer-item').first().waitFor() will hang when no
retention offers exist (retentionOffers empty in offers-index-retention.tsx);
fix by ensuring a retention offer is created in the test setup before calling
this utility OR make the wait resilient: replace the unconditional wait with a
check for presence (e.g. query/count the 'retention-offer-item' locator and only
waitFor() when count > 0) so the helper handles both empty and populated states
safely.
- Around line 268-278: The current code archives only the first offer by using
nth(0); change it to loop until no offers remain so all existing 'offer-item'
elements are archived. Replace the if (hasOfferItems) block with a while loop
that checks await page.getByTestId('offer-item').count() > 0, then for each
iteration click page.getByTestId('offer-item').nth(0), click the 'Archive offer'
button, interact with the confirmation modal (confirmation-modal) by clicking
the 'Archive' role button and waiting for the modal to hide, and ensure you wait
for the offer count to decrease before the next iteration to avoid race
conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fec6063a-7680-45c6-a1a2-96e04647c93c
📒 Files selected for processing (1)
ghost/core/test/e2e-browser/utils/e2e-browser-utils.js
9f3926b to
1d3bf24
Compare
1d3bf24 to
c82224f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/e2e-browser/portal/offers.spec.js (1)
34-37: Extract the Manage offers modal flow into a helper.This wait/open/assert/close sequence is repeated four times in this spec. Pulling it into a small helper will reduce selector drift the next time the offers modal changes.
Also applies to: 115-118, 193-196, 271-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-browser/portal/offers.spec.js` around lines 34 - 37, Extract the repeated wait/open/assert/close flow into a single helper (e.g., openAndAssertOffersModal) that takes offerName; the helper should perform the sequence using sharedPage.getByTestId('offers').getByRole('button', {name: 'Manage tiers'}).waitFor({state: 'hidden'}), then click sharedPage.getByTestId('offers').getByRole('button', {name: 'Manage offers'}), assert await expect(sharedPage.getByTestId('offers-modal')).toContainText(offerName), and finally close with sharedPage.getByTestId('offers-modal').getByRole('button', {name: 'Close'}).click(); replace the four duplicated blocks (lines around 34-37, 115-118, 193-196, 271-274) with calls to openAndAssertOffersModal(offerName) to centralize selectors and reduce drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/test/e2e-browser/portal/offers.spec.js`:
- Around line 87-88: The test is asserting redemption count too loosely by
checking toContainText('1') on the whole offer row; narrow the assertion to the
specific redemption cell instead. Locate the row via
sharedPage.getByTestId('offer-item').filter({hasText: offerName}) (existing
offerRow), then query the redemption/count cell inside that row (e.g. a test id
like 'offer-redemptions' or a specific column locator) and assert the exact text
with toHaveText('1') (or otherwise assert an exact label/value) rather than
using a substring match on the entire row.
---
Nitpick comments:
In `@ghost/core/test/e2e-browser/portal/offers.spec.js`:
- Around line 34-37: Extract the repeated wait/open/assert/close flow into a
single helper (e.g., openAndAssertOffersModal) that takes offerName; the helper
should perform the sequence using
sharedPage.getByTestId('offers').getByRole('button', {name: 'Manage
tiers'}).waitFor({state: 'hidden'}), then click
sharedPage.getByTestId('offers').getByRole('button', {name: 'Manage offers'}),
assert await
expect(sharedPage.getByTestId('offers-modal')).toContainText(offerName), and
finally close with sharedPage.getByTestId('offers-modal').getByRole('button',
{name: 'Close'}).click(); replace the four duplicated blocks (lines around
34-37, 115-118, 193-196, 271-274) with calls to
openAndAssertOffersModal(offerName) to centralize selectors and reduce drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e646e3f9-9af6-43ce-bc1f-52f6bd538a7d
📒 Files selected for processing (3)
ghost/core/test/e2e-browser/admin/tiers.spec.jsghost/core/test/e2e-browser/portal/offers.spec.jsghost/core/test/e2e-browser/utils/e2e-browser-utils.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/e2e-browser/admin/tiers.spec.js
| const offerRow = sharedPage.getByTestId('offer-item').filter({hasText: offerName}); | ||
| await expect(offerRow).toContainText('1'); |
There was a problem hiding this comment.
The redemption-count check is now too weak to prove the count changed.
toContainText('1') on the whole row can match unrelated text in the generated offerName or other numeric fields in the row, so this can go green before any redemption is recorded. Assert against the specific redemption field/cell, or at least an exact redemption label/value, instead of a bare substring on the row.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ghost/core/test/e2e-browser/portal/offers.spec.js` around lines 87 - 88, The
test is asserting redemption count too loosely by checking toContainText('1') on
the whole offer row; narrow the assertion to the specific redemption cell
instead. Locate the row via
sharedPage.getByTestId('offer-item').filter({hasText: offerName}) (existing
offerRow), then query the redemption/count cell inside that row (e.g. a test id
like 'offer-redemptions' or a specific column locator) and assert the exact text
with toHaveText('1') (or otherwise assert an exact label/value) rather than
using a substring match on the entire row.
🤖 Velo CI Failure AnalysisClassification: 🔴 HARD FAIL
|
ref https://linear.app/ghost/project/retention-offers-churn-prevention-e2474be8b6ad/overview
closes https://linear.app/ghost/issue/BER-3410
Using retention offers, publishers can offer existing paid members a discount before they cancel, in order to retain them. For example, "a month on us" or "50% off your next payment".
Co-authored-by: Michael Barrett mike@ghost.org
Co-authored-by: Sodbileg Gansukh sodbileg.gansukh@gmail.com