Migrated tiers browser tests to root e2e framework#26980
Merged
Conversation
Migrated 3 valuable tests (create tier, update tier with portal verification, archive/unarchive) from ghost/core browser tests to e2e/. Dropped 2 redundant tests (default prices smoke, tier+offer creation already covered by portal-offers). Added TiersSection page object for reusable tier CRUD interactions.
Contributor
WalkthroughAdds a new Playwright page helper module for Ghost Admin Tiers ( 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
e2e/tests/admin/settings/tiers.test.ts (1)
80-121: Split archive and unarchive into separate tests for single-scenario isolation.This test validates archive behavior, portal absence, unarchive behavior, and portal reappearance in one flow. Splitting improves failure diagnosis and aligns with scenario boundaries.
As per coding guidelines, "Test names should follow ... with one test = one scenario (never mix multiple scenarios)" and "Do not include multiple scenarios in one test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/admin/settings/tiers.test.ts` around lines 80 - 121, Split the combined "archive and unarchive" test into two independent tests: one that verifies archiving moves a newly created tier from active to archived and removes it from portal settings, and a second that verifies unarchiving moves a tier from archived back to active and restores it in portal settings. For each test, create its own tier via SettingsPage.tiersSection.createTier, then call SettingsPage.tiersSection.archiveTier(tierSlug) in the archive test and assert visibility changes and portal absence using SettingsPage.portalSection.openCustomizeModal and portalModal.getByLabel; in the unarchive test first archive the tier (or create it in archived state), then call SettingsPage.tiersSection.unarchiveTier(tierSlug), assert it appears in activeTab and in portal settings, and ensure each test is self-contained (no shared state) and uses the existing helpers (tierCard, activeTab, archivedTab, portalModal) referenced in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/helpers/pages/admin/settings/sections/tiers-section.ts`:
- Around line 91-95: In enableTierInPortal, add a visibility guard using
waitFor() on the portal modal before interacting: after clicking the 'Customize'
button, call portalModal.waitFor({ state: 'visible' }) (or the project's waitFor
helper) to ensure the portalModal (getByTestId('portal-modal')) is visible
before subsequent operations; this replaces any expect() usage in the page
object and prevents flaky interactions.
- Around line 58-69: The editTier method currently uses truthy checks which
prevent clearing fields with empty strings; update editTier (and its calls to
tierDetailModal.getByLabel for labels "Name", "Description", "Monthly price",
"Yearly price") to check for undefined instead of truthiness (e.g., use
data.name !== undefined or Object.prototype.hasOwnProperty.call(data, 'name'))
so empty string values are applied to the form, ensuring TierFormData fields can
be cleared as well as set.
In `@e2e/tests/admin/settings/tiers.test.ts`:
- Around line 17-18: Replace ad-hoc tier test data (e.g., the tierName variable
built with `Test Tier ${Date.now()}` in tiers.test.ts and similar inline
Date.now usages at the other spots) with the standardized factory calls from the
data-factory module; locate the places that create test tier inputs (the
`tierName` initialization and the other inline data at the cited locations) and
call the appropriate factory function (e.g., the tier factory or a
TestDataFactory.createTier-style API) to produce unique, deterministic test
data, then use the factory output in the test assertions and API calls.
- Around line 66-67: The test uses raw selector strings in
page.frameLocator('[data-testid="portal-popup-frame"]') and
portalFrame.locator('[data-test-tier="paid"]').filter({hasText: updatedName}) to
find the portal frame and tier card; replace these with semantic/page-object
locator helpers (e.g. a PortalPopup.getFrameLocator() or
PortalPopup.locatorForTier('paid', updatedName) and a
TiersPage.getPaidTierByName(updatedName)) and update the test to call those
methods instead of using raw CSS/test-id strings so the test follows the E2E
locator policy; ensure the new helpers use the approved semantic/data-testid
locators internally and return the same locator types used by the test.
---
Nitpick comments:
In `@e2e/tests/admin/settings/tiers.test.ts`:
- Around line 80-121: Split the combined "archive and unarchive" test into two
independent tests: one that verifies archiving moves a newly created tier from
active to archived and removes it from portal settings, and a second that
verifies unarchiving moves a tier from archived back to active and restores it
in portal settings. For each test, create its own tier via
SettingsPage.tiersSection.createTier, then call
SettingsPage.tiersSection.archiveTier(tierSlug) in the archive test and assert
visibility changes and portal absence using
SettingsPage.portalSection.openCustomizeModal and portalModal.getByLabel; in the
unarchive test first archive the tier (or create it in archived state), then
call SettingsPage.tiersSection.unarchiveTier(tierSlug), assert it appears in
activeTab and in portal settings, and ensure each test is self-contained (no
shared state) and uses the existing helpers (tierCard, activeTab, archivedTab,
portalModal) referenced in the diff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a53ae8f5-85a5-4e71-85c5-541bf8ba17ad
📒 Files selected for processing (5)
e2e/helpers/pages/admin/settings/sections/index.tse2e/helpers/pages/admin/settings/sections/tiers-section.tse2e/helpers/pages/admin/settings/settings-page.tse2e/tests/admin/settings/tiers.test.tsghost/core/test/e2e-browser/admin/tiers.spec.js
💤 Files with no reviewable changes (1)
- ghost/core/test/e2e-browser/admin/tiers.spec.js
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
e2e/helpers/pages/admin/settings/sections/tiers-section.ts (1)
58-69:⚠️ Potential issue | 🟡 MinorAllow clearing fields in
editTier.Line 59/62/65/68 use truthy checks, so
''cannot be applied to clear existing values. Use!== undefinedchecks instead.Proposed fix
async editTier(data: Partial<TierFormData>): Promise<void> { - if (data.name) { + if (data.name !== undefined) { await this.tierDetailModal.getByLabel('Name').fill(data.name); } - if (data.description) { + if (data.description !== undefined) { await this.tierDetailModal.getByLabel('Description').fill(data.description); } - if (data.monthlyPrice) { + if (data.monthlyPrice !== undefined) { await this.tierDetailModal.getByLabel('Monthly price').fill(data.monthlyPrice); } - if (data.yearlyPrice) { + if (data.yearlyPrice !== undefined) { await this.tierDetailModal.getByLabel('Yearly price').fill(data.yearlyPrice); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/settings/sections/tiers-section.ts` around lines 58 - 69, The editTier method uses truthy checks so passing empty strings cannot clear fields; update the four conditional checks in editTier (for name, description, monthlyPrice, yearlyPrice) to test against !== undefined instead of truthiness, and then call tierDetailModal.getByLabel(...).fill(...) when the value is !== undefined so empty strings will overwrite existing values.
🧹 Nitpick comments (1)
e2e/helpers/pages/admin/settings/sections/tiers-section.ts (1)
91-114: ReusePortalSectionhelpers for portal modal flow.
enableTierInPortalduplicates open/close/locator flow already encapsulated inPortalSection. Centralizing this reduces drift and keeps modal guards consistent.Refactor sketch
+import {PortalSection} from './portal-section'; ... async enableTierInPortal(tierName: string): Promise<void> { - const portalSection = this.page.getByTestId('portal'); - await portalSection.getByRole('button', {name: 'Customize'}).click(); - const portalModal = this.page.getByTestId('portal-modal'); - await portalModal.waitFor({state: 'visible'}); + const portalSection = new PortalSection(this.page); + await portalSection.openCustomizeModal(); + const portalModal = portalSection.portalModal; - const tierCheckbox = portalModal.getByLabel(tierName).first(); + const tierCheckbox = portalSection.tierCheckbox(tierName); if (!await tierCheckbox.isChecked()) { await tierCheckbox.check(); } ... await portalModal.getByRole('button', {name: 'Save'}).click(); - await portalModal.getByRole('button', {name: 'Close'}).click(); + await portalSection.closeCustomizeModal(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/helpers/pages/admin/settings/sections/tiers-section.ts` around lines 91 - 114, The enableTierInPortal method duplicates portal open/close/locator logic that already lives in PortalSection; refactor enableTierInPortal to reuse PortalSection's helpers by replacing the manual open (getByTestId('portal') + Customize click + waitFor visible) and close (Save/Close clicks) and checkbox locators with PortalSection methods (e.g., openModal(), closeModal(), and helpers for toggling checkboxes) so the modal guard/locator behaviour is centralized and maintained by PortalSection; update enableTierInPortal to call PortalSection.openModal(), use PortalSection.toggleTier(tierName)/toggleBillingOption('Monthly'|'Yearly') or equivalent helpers, then call PortalSection.saveAndClose() (or the matching helper names) to remove duplicated modal flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/helpers/pages/admin/settings/sections/tiers-section.ts`:
- Around line 58-69: The editTier method uses truthy checks so passing empty
strings cannot clear fields; update the four conditional checks in editTier (for
name, description, monthlyPrice, yearlyPrice) to test against !== undefined
instead of truthiness, and then call tierDetailModal.getByLabel(...).fill(...)
when the value is !== undefined so empty strings will overwrite existing values.
---
Nitpick comments:
In `@e2e/helpers/pages/admin/settings/sections/tiers-section.ts`:
- Around line 91-114: The enableTierInPortal method duplicates portal
open/close/locator logic that already lives in PortalSection; refactor
enableTierInPortal to reuse PortalSection's helpers by replacing the manual open
(getByTestId('portal') + Customize click + waitFor visible) and close
(Save/Close clicks) and checkbox locators with PortalSection methods (e.g.,
openModal(), closeModal(), and helpers for toggling checkboxes) so the modal
guard/locator behaviour is centralized and maintained by PortalSection; update
enableTierInPortal to call PortalSection.openModal(), use
PortalSection.toggleTier(tierName)/toggleBillingOption('Monthly'|'Yearly') or
equivalent helpers, then call PortalSection.saveAndClose() (or the matching
helper names) to remove duplicated modal flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce1a252b-fef9-4a51-a625-1f508ed0b755
📒 Files selected for processing (4)
e2e/helpers/pages/admin/settings/sections/portal-section.tse2e/helpers/pages/admin/settings/sections/tiers-section.tse2e/helpers/pages/portal/sign-up-page.tse2e/tests/admin/settings/tiers.test.ts
✅ Files skipped from review due to trivial changes (1)
- e2e/tests/admin/settings/tiers.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ghost/core/test/e2e-browser/toe2e/tests/admin/settings/tiers.test.tsTiersSectionpage object with reusable tier CRUD methods (create, edit, archive, unarchive, enable in portal)portal-offers.test.ts)Test plan
yarn lintpasses (0 errors)yarn test:typespassesyarn test tests/admin/settings/tiers.test.ts— all 3 tests pass