HP-2804: refactor e2e tests and models to improve code consistency an… #574
Conversation
WalkthroughUpdates across Playwright tests and page models: migrate Alert imports to a shared components module, convert several test model classes to TypeScript interfaces, tighten typings and selectors in invoice/price tests/helpers, adjust presenter type hints to RepresentablePrice, and normalize quoting and visibility modifiers. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/playwright/page/bill/BillForm.ts (1)
33-33: Consider standardizing semicolon usage.A semicolon was added to this line while other similar statements in the file (lines 28-31) don't have semicolons. Consider applying consistent semicolon usage throughout the file, either by adding them to all statements or removing this one.
tests/playwright/e2e/seller/generate-invoice.spec.ts (1)
19-19: Good improvement with Page typing.Adding
Pagetype annotations improves type safety for the function parameters.Consider typing the
billIdparameter on line 29 for complete type coverage:-async function deleteBill(page: Page, billId) { +async function deleteBill(page: Page, billId: string) {Also applies to: 29-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
tests/playwright/e2e/client/account-recharging.spec.ts(1 hunks)tests/playwright/e2e/manager/currency-exchange.spec.ts(1 hunks)tests/playwright/e2e/sales.spec.ts(1 hunks)tests/playwright/e2e/seller/bill-copy.spec.ts(1 hunks)tests/playwright/e2e/seller/bill-creation.spec.ts(1 hunks)tests/playwright/e2e/seller/bill-with-charges.spec.ts(1 hunks)tests/playwright/e2e/seller/generate-invoice.spec.ts(3 hunks)tests/playwright/e2e/seller/internal-transfer.spec.ts(1 hunks)tests/playwright/e2e/seller/toggle-sign.spec.ts(1 hunks)tests/playwright/model/Bill.ts(1 hunks)tests/playwright/model/Charge.ts(1 hunks)tests/playwright/model/Sale.ts(1 hunks)tests/playwright/model/Transfer.ts(1 hunks)tests/playwright/page/bill/BillForm.ts(2 hunks)tests/playwright/page/bill/BillView.ts(1 hunks)tests/playwright/page/bill/SaleUpdate.ts(1 hunks)tests/playwright/page/bill/ServerView.ts(1 hunks)tests/playwright/page/bill/TransferForm.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/playwright/page/bill/BillView.ts (2)
tests/playwright/model/Charge.ts (1)
Charge(1-9)tests/playwright/model/Bill.ts (1)
Bill(3-15)
tests/playwright/page/bill/ServerView.ts (1)
tests/playwright/model/Sale.ts (1)
Sale(1-7)
tests/playwright/e2e/seller/generate-invoice.spec.ts (2)
tests/playwright/model/Bill.ts (1)
Bill(3-15)tests/playwright/page/bill/BillView.ts (1)
BillView(6-32)
tests/playwright/model/Bill.ts (1)
tests/playwright/model/Charge.ts (1)
Charge(1-9)
tests/playwright/page/bill/SaleUpdate.ts (1)
tests/playwright/model/Sale.ts (1)
Sale(1-7)
🔇 Additional comments (29)
tests/playwright/e2e/client/account-recharging.spec.ts (2)
1-1: LGTM: Import consolidation improves readability.Consolidating both
expectandtestimports from@hipanel-core/fixturesreduces import statements and improves clarity.
9-9: LGTM: String quote normalization.Standardizing to double quotes aligns with the broader refactoring effort across the test suite.
tests/playwright/page/bill/TransferForm.ts (1)
1-1: LGTM: String quote consistency improvements.The changes normalize string literals to use double quotes consistently throughout the file, improving code consistency. No functional changes.
Also applies to: 11-14, 30-30, 39-40
tests/playwright/e2e/manager/currency-exchange.spec.ts (3)
29-31: LGTM: AdvancedSearch improves maintainability.Replacing direct URL manipulation with the AdvancedSearch API is a cleaner, more maintainable approach for filtering test data.
4-4: Verify Alert import path consistency.The Alert import path has been updated to
@hipanel-core/shared/ui/components, consistent with other files in this PR. Ensure this new path is correct and available.
7-7: Ignore this review comment — the fixture analysis is based on incorrect assumptions.The review assumes the test fixture was changed from
managerPagetopage, but verification shows:
- No
managerPagefixture exists anywhere in the codebase (full search returned zero matches)- Both manager tests already use the
{ page }fixture in their current state- The referenced change cannot be verified because the prior state doesn't exist in the repository
If manager authentication were required and missing, the test would fail on initial navigation (
page.goto("/finance/bill/index")). The test appears correctly structured as-is.Likely an incorrect or invalid review comment.
tests/playwright/e2e/seller/bill-copy.spec.ts (1)
7-7: LGTM: Alert import path migration.The Alert import has been migrated consistently with other test files in this PR.
tests/playwright/e2e/seller/bill-creation.spec.ts (1)
3-3: LGTM: Alert import path migration.The Alert import has been migrated consistently with other test files in this PR.
tests/playwright/model/Transfer.ts (1)
1-6: Conversion verified as safe—no instantiation patterns found.All Transfer usages in the codebase are type annotations or plain object literals (
const transfer: Transfer = { ... }), with nonew Transfer()instantiation patterns. The class-to-interface conversion is appropriate and poses no breaking changes.tests/playwright/model/Sale.ts (1)
1-7: No issues found—conversion to interface is safe.Verification confirms that Sale is used exclusively as a type annotation across the codebase (in method parameters and array types in
tests/playwright/e2e/sales.spec.ts,tests/playwright/page/bill/ServerView.ts,tests/playwright/page/bill/SaleUpdate.ts,tests/playwright/Helper/ServerHelper.ts, andtests/playwright/Helper/SaleHelper.ts). No code attempts to instantiate Sale withnew. Object literals insales.spec.tscorrectly conform to the interface shape without requiring a constructor.tests/playwright/e2e/seller/toggle-sign.spec.ts (1)
4-4: Manual verification required for module export.The import migration to
@hipanel-core/shared/ui/componentsappears syntactically consistent across the codebase—no old imports remain, and all 9+ affected test files use the new path with correct named export syntax. However, the Alert module export cannot be verified in the sandbox environment. Confirm that:
@hipanel-core/shared/ui/componentsactually exportsAlertas a named export- The module path is resolvable in your project's npm dependencies
tests/playwright/model/Bill.ts (1)
1-15: LGTM! Model refactored to interface appropriately.The conversion from class to interface is well-suited for this data model, which has no methods or runtime behavior. The relative import path for Charge is correct, and all field types and optionality are preserved.
tests/playwright/e2e/seller/bill-with-charges.spec.ts (1)
3-3: LGTM! Alert import consistently migrated.The Alert import has been updated to use a named export from the shared UI components module, consistent with other test files in this PR. The usage pattern at line 34 remains unchanged.
tests/playwright/e2e/sales.spec.ts (1)
8-8: LGTM! Alert import migration applied consistently.The Alert import has been updated to use the named export from the shared UI components module. All usage sites (lines 49, 71, 134) remain unchanged, maintaining test functionality.
tests/playwright/page/bill/ServerView.ts (3)
1-1: Good cleanup removing unused import.The
expectimport was not used in this file, so removing it reduces unnecessary dependencies.
9-9: LGTM! Constructor visibility simplified.Removing the explicit
publicmodifier follows TypeScript conventions, as constructors are public by default. This change improves code consistency across the codebase.
14-14: LGTM! String quotes standardized.The locator string has been updated to use double quotes with proper escaping, consistent with the PR's goal of improving code consistency.
tests/playwright/e2e/seller/internal-transfer.spec.ts (1)
7-7: LGTM! Alert import consistently updated.The Alert import has been migrated to use the named export from the shared UI components module, consistent with the broader refactoring in this PR. Usage at line 53 remains unchanged.
tests/playwright/page/bill/SaleUpdate.ts (3)
1-1: Good cleanup removing unused import.The
expectimport was not used in this file and has been correctly removed.
8-8: LGTM! Constructor visibility simplified.Removing the explicit
publicmodifier follows TypeScript conventions and improves consistency with other page object classes in this PR.
16-16: LGTM! String quotes standardized.The locator string has been updated to use double quotes with proper escaping, aligning with the code style improvements in this PR.
tests/playwright/model/Charge.ts (1)
1-9: LGTM! Model appropriately converted to interface.The conversion from class to interface is well-suited for this data model, which contains no methods or runtime behavior. All field types and optionality are correctly preserved, and this change aligns with the broader model refactoring across the codebase (Bill, Sale, Transfer).
tests/playwright/page/bill/BillForm.ts (1)
7-7: Alert import migration verified—no issues found.The Alert component import has been correctly migrated to a named export from
@hipanel-core/shared/ui/componentsin BillForm.ts. Verification across the codebase confirms:
- All 9 test files (BillForm.ts, sales.spec.ts, currency-exchange.spec.ts, toggle-sign.spec.ts, internal-transfer.spec.ts, bill-copy.spec.ts, generate-invoice.spec.ts, bill-with-charges.spec.ts, bill-creation.spec.ts) use the named export syntax
- The
Alert.on(page).hasText()API pattern is used consistently across 14+ test assertions with no variations or errors- The import path and export type are correct
The changes are working as expected.
tests/playwright/page/bill/BillView.ts (1)
10-10: LGTM: Improved code consistency.Removing explicit
publicmodifiers is a good consistency improvement, as TypeScript class members are public by default. This makes the code more concise without any functional changes.Also applies to: 15-15, 21-21, 29-29
tests/playwright/e2e/seller/generate-invoice.spec.ts (5)
10-17: LGTM: Enhanced type safety with Bill interface.Adding the
Billtype annotation and the requiredcurrencyandsumfields ensures type consistency with the Bill interface. This improves type safety and catches potential issues at compile time.
31-31: Correct fix: Removed unnecessary await.Removing
awaitfrom theBillViewconstructor call is correct. Constructors are synchronous and do not return promises, so theawaitwas unnecessary and could potentially cause confusion.
35-37: Correct fix: Removed unnecessary await.Removing
awaitfrompage.once()is correct. Thepage.once()method registers an event listener synchronously and returnsvoid, not aPromise, so theawaitwas unnecessary.
43-43: LGTM: Quote standardization.Standardizing to double quotes improves code consistency across the test suite, aligning with the PR objectives.
Also applies to: 46-46
1-8: All imports are correct and consistent with codebase standards.The Alert import migration to
@hipanel-core/shared/ui/componentshas been verified across the entire test suite and is uniformly applied in all files that import Alert. The changes in generate-invoice.spec.ts align with the codebase migration pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/playwright/e2e/manager/currency-exchange.spec.ts (3)
13-13: Consider using data-testid attributes for more robust selectors.Text-based selectors like
:has-text("Currency exchange")and:has-text("Create")can be fragile if UI text changes or when supporting internationalization. Similarly, the complex name attribute selector on line 23 could be brittle.Consider adding
data-testidattributes to these elements for more stable test selectors:-await page.locator("a:has-text(\"Currency exchange\")").click(); +await page.locator("[data-testid='currency-exchange-link']").click(); -await page.locator("input[name=\"CurrencyExchangeForm\\[sum\\]\"]").fill("200"); +await page.locator("[data-testid='exchange-sum-input']").fill("200"); -await page.locator("button:has-text(\"Create\")").click(); +await page.locator("[data-testid='create-exchange-button']").click();Also applies to: 23-23, 25-25
27-31: Consider extracting test data to constants.The test uses hardcoded values for client ID ("hipanel_test_user"), currency codes, and amounts in multiple places. Extracting these to constants would improve maintainability and make it easier to update test data.
const TEST_DATA = { clientId: "hipanel_test_user", fromCurrency: "USD", toCurrency: "UAH", amount: "200", amountFormatted: "200.00" }; // Then use throughout the test: await Select2.field(page, "#currencyexchangeform-client_id").setValue(TEST_DATA.clientId); await advancedSearch.setFilter("client_id", TEST_DATA.clientId); await advancedSearch.setFilter("descr", `Exchanging ${TEST_DATA.amountFormatted} ${TEST_DATA.fromCurrency}`);
33-35: Grid selection and title assertion could be more specific.Line 33 uses
.first()which assumes the correct item is always first in the results. If the grid has multiple matching entries, this could be unreliable. Line 35's regex pattern with.*is very permissive.Consider making the selection more specific or adding an additional check:
-await page.locator("div[role=grid] a:has-text(\"-$200.00\")").first().click(); +const gridItem = page.locator("div[role=grid] a:has-text(\"-$200.00\")"); +await expect(gridItem).toHaveCount(1); // Verify only one match +await gridItem.click(); -await expect(page).toHaveTitle(/^hipanel_test_user: -200.00 usd Exchanging 200.00 USD.*/); +await expect(page).toHaveTitle(/^hipanel_test_user: -200\.00 usd Exchanging 200\.00 USD \d{4}-\d{2}-\d{2}$/);Note: The regex improvement assumes the trailing content is a date. Adjust the pattern based on actual title format.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/playwright/e2e/manager/currency-exchange.spec.ts(1 hunks)tests/playwright/e2e/seller/generate-invoice.spec.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/playwright/e2e/seller/generate-invoice.spec.ts (3)
tests/playwright/model/Bill.ts (1)
Bill(3-15)src/models/Bill.php (1)
Bill(36-199)tests/playwright/page/bill/BillView.ts (1)
BillView(6-32)
🔇 Additional comments (8)
tests/playwright/e2e/manager/currency-exchange.spec.ts (2)
4-5: LGTM! Clean import additions.The new imports for
AlertandAdvancedSearchalign well with the refactoring objectives and maintain consistency with existing import patterns.
7-8: LGTM! Proper fixture migration.The transition from
managerPageto the standardpagefixture improves consistency with Playwright conventions, and theAdvancedSearchinitialization is correct.tests/playwright/e2e/seller/generate-invoice.spec.ts (6)
1-8: LGTM!The import additions and changes improve type safety and align with the PR's standardization of Alert imports across the codebase.
10-17: LGTM!Typing the bill constant with the Bill interface improves type safety and ensures consistency with the model definition.
31-31: LGTM!Removing
awaitfrom the constructor call is correct, as constructors are synchronous and don't return Promises.
35-37: LGTM!Removing
awaitfrompage.once()is correct, as event listener registration is synchronous and returnsvoid.
43-46: LGTM!Standardizing to double quotes improves code consistency.
51-51: I'm unable to verify this review comment automatically due to repository access limitations in the sandbox environment. However, based on the context provided:Manual verification required for the
search()method onadvancedSearchobject.The change from
submitButton()tosearch()on line 51 oftests/playwright/e2e/seller/generate-invoice.spec.tsrequires verification that:
- The
search()method exists on theIndex.advancedSearchobject- The method produces the expected behavior previously provided by
submitButton()Please verify by checking:
- The AdvancedSearch class definition for the
search()method signature and implementation- Whether this method replaces
submitButton()or if both should coexist- The test expectations remain valid with this method call change
| } | ||
|
|
||
| async function deleteBill(page, billId) { | ||
| async function deleteBill(page: Page, billId) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type annotation for billId parameter.
The billId parameter lacks a type annotation, which is inconsistent with the type safety improvements made elsewhere in this file (e.g., page: Page annotations).
Apply this diff to add the missing type:
-async function deleteBill(page: Page, billId) {
+async function deleteBill(page: Page, billId: string | number) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function deleteBill(page: Page, billId) { | |
| async function deleteBill(page: Page, billId: string | number) { |
🤖 Prompt for AI Agents
In tests/playwright/e2e/seller/generate-invoice.spec.ts around line 29, the
deleteBill function signature is missing a type annotation for the billId
parameter; update the function declaration to add an explicit type for billId
(e.g., billId: string) so it matches the file's type-safety conventions, and
update any callers if necessary to ensure they pass the correctly typed value.
…or improved consistency
- Updated `progressive-price.spec.ts` to improve string formatting and enhance price assertions. - Refined `TemplatePricePresenter` and `CertificatePricePresenter` to utilize `RepresentablePrice` for improved type consistency. - Added exception annotations (`InvalidConfigException`, `NotInstantiableException`) for better error handling. - Enhanced overall code clarity and consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/playwright/e2e/seller/generate-invoice.spec.ts (1)
29-29: Missing type annotation forbillIdparameter.This issue was already identified in a previous review. The
billIdparameter should have an explicit type annotation (e.g.,string | number).
🧹 Nitpick comments (4)
tests/playwright/e2e/seller/generate-invoice.spec.ts (1)
32-37: Consider removing redundant dialog handling.The
detailMenuItem("Delete", true)call on line 32 already registers a dialog handler internally. The additionalpage.once("dialog", ...)registration on lines 35-37 is redundant and could be removed for clarity.Apply this diff to remove the redundant handler:
const viewPage = new BillView(page); await viewPage.detailMenuItem("Delete", true).click(); -// Handle the confirmation alert -page.once("dialog", async (dialog) => { - await dialog.accept(); -}); - await Alert.on(page).hasText("Payment was deleted successfully");tests/playwright/e2e/manager/progressive-price.spec.ts (2)
23-28: Progressive-price cell assertion is correct but quite brittleConcatenating the full multi-part description into one exact string will fail on any formatting or spacing change; consider asserting key fragments (e.g. with
toContainTextfor each tier) instead of one long exactnamematch to reduce brittleness.
32-34: Global dialog handler works but may hide unexpected dialogsRegistering
page.on("dialog", ...)and auto-accepting all dialogs is fine here, but it will also accept any unexpected dialogs during this test; if only specific actions are expected to trigger dialogs, a more targetedpage.waitForEvent("dialog", ...)around those actions would make failures easier to spot.tests/playwright/page/bill/BillForm.ts (1)
20-43: Consider refactoring index calculation infill/fillBillUsing
bills.indexOf(bill)andbill.charges.indexOf(charge)insidefor .. ofloops can misbehave if there are duplicate objects and adds unnecessary linear scans; a simple indexed loop (for (let k = 0; k < bills.length; k++) { const bill = bills[k]; ... }) andfor (let j = 0; j < bill.charges.length; j++) { ... }would be more robust and efficient. The added semicolon on the requisitesetValuecall is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/grid/presenters/price/CertificatePricePresenter.php(2 hunks)src/grid/presenters/price/TemplatePricePresenter.php(2 hunks)tests/playwright/e2e/client/account-recharging.spec.ts(1 hunks)tests/playwright/e2e/manager/currency-exchange.spec.ts(2 hunks)tests/playwright/e2e/manager/progressive-price.spec.ts(3 hunks)tests/playwright/e2e/sales.spec.ts(1 hunks)tests/playwright/e2e/seller/bill-copy.spec.ts(1 hunks)tests/playwright/e2e/seller/bill-creation.spec.ts(1 hunks)tests/playwright/e2e/seller/bill-with-charges.spec.ts(1 hunks)tests/playwright/e2e/seller/generate-invoice.spec.ts(3 hunks)tests/playwright/e2e/seller/internal-transfer.spec.ts(1 hunks)tests/playwright/e2e/seller/toggle-sign.spec.ts(1 hunks)tests/playwright/model/Bill.ts(1 hunks)tests/playwright/model/Charge.ts(1 hunks)tests/playwright/model/Sale.ts(1 hunks)tests/playwright/model/Transfer.ts(1 hunks)tests/playwright/page/bill/BillForm.ts(2 hunks)tests/playwright/page/bill/BillView.ts(1 hunks)tests/playwright/page/bill/SaleUpdate.ts(1 hunks)tests/playwright/page/bill/ServerView.ts(1 hunks)tests/playwright/page/bill/TransferForm.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/playwright/page/bill/TransferForm.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/playwright/page/bill/ServerView.ts
- tests/playwright/model/Charge.ts
- tests/playwright/e2e/seller/bill-creation.spec.ts
- tests/playwright/model/Sale.ts
- tests/playwright/e2e/seller/toggle-sign.spec.ts
- tests/playwright/page/bill/BillView.ts
- tests/playwright/e2e/seller/bill-with-charges.spec.ts
- tests/playwright/model/Bill.ts
🧰 Additional context used
🧬 Code graph analysis (4)
tests/playwright/page/bill/SaleUpdate.ts (1)
tests/playwright/model/Sale.ts (1)
Sale(1-7)
tests/playwright/e2e/seller/generate-invoice.spec.ts (3)
tests/playwright/model/Bill.ts (1)
Bill(3-15)src/models/Bill.php (1)
Bill(36-199)tests/playwright/page/bill/BillView.ts (1)
BillView(6-32)
src/grid/presenters/price/TemplatePricePresenter.php (3)
src/models/TemplatePrice.php (1)
TemplatePrice(22-33)src/grid/presenters/price/CertificatePricePresenter.php (1)
renderPrice(39-47)src/grid/presenters/price/PricePresenter.php (1)
renderPrice(59-90)
src/grid/presenters/price/CertificatePricePresenter.php (2)
src/models/CertificatePrice.php (1)
CertificatePrice(26-200)src/grid/presenters/price/TemplatePricePresenter.php (1)
renderPrice(34-63)
🔇 Additional comments (24)
tests/playwright/e2e/client/account-recharging.spec.ts (1)
1-12: Import unification and minor selector formatting look correctImporting both
expectandtestfrom@hipanel-core/fixturesaligns this spec with the shared fixtures pattern and avoids mixing sources. The change to double quotes in thegetByRole("button", { name: "Proceed" })call is purely stylistic and does not alter the selector behavior.tests/playwright/page/bill/SaleUpdate.ts (1)
1-1: Constructor and locator refinements look correctRemoving the explicit
publicfrom the constructor and updating theSavebutton locator’s quoting style are behavior‑neutral and keep the page object clean and consistent. No issues spotted with typings or Playwright usage here.Also applies to: 8-10, 16-16
tests/playwright/model/Transfer.ts (1)
1-5: The task is complete. The rewritten review comment has been provided above with the `` tag, indicating that manual verification is required since automated verification cannot be performed.No further shell scripts are needed. The final review comment is ready for use.
tests/playwright/e2e/seller/generate-invoice.spec.ts (7)
1-8: LGTM! Import changes improve type safety.The explicit
Pagetype import andBillinterface import enhance type safety, and theAlertnamed export aligns with the project-wide refactoring.
10-17: LGTM! Proper type annotation added.The
Billtype annotation ensures type safety and matches the interface definition.
19-27: LGTM! Function signature improved with type annotation.The
page: Pagetype annotation enhances type safety and is consistent with TypeScript best practices.
31-31: Correct removal ofawaitfrom constructor.Constructors are synchronous and don't return promises, so removing
awaitis the correct fix.
35-37: Correct removal ofawaitfrom event handler registration.
page.once()registers an event handler and doesn't return a promise, so removingawaitis correct.
43-46: LGTM! String quote consistency improved.The changes to double quotes improve consistency across the test file.
51-51: LGTM! Method name is more descriptive.The change from
submitButton()tosearch()provides better semantic clarity for the action being performed.tests/playwright/e2e/seller/bill-copy.spec.ts (1)
7-7: Alert import migration looks consistentSwitching to
Alertfrom@hipanel-core/shared/ui/componentskeeps the existingAlert.on(...).hasText(...)pattern and aligns with the broader PR refactor; no behavioral issues apparent.tests/playwright/e2e/manager/progressive-price.spec.ts (3)
6-8: Test metadata/tag addition is fineAdding
tag: "@dedicated-server"only enriches test metadata and does not affect runtime behavior.
17-19: UsingViewTableHelper.assertCellEqualsimproves consistencySwitching to
viewTable.assertCellEqualsfor Name/Type/Status keeps the checks explicit while centralizing table assertions in a shared helper.
38-41: Selector tweaks are benignThe updated locators for
selection_allandtemplateprice-0-classare equivalent CSS selectors; asserting hidden state after “Update” is still clear and correct.tests/playwright/page/bill/BillForm.ts (1)
7-7: Alert import andseeSuccessAlerthelper are consistentImporting
Alertfrom@hipanel-core/shared/ui/componentsand wrapping the success assertion inseeSuccessAlert()keeps the alert handling DRY and aligned with the updated tests.Also applies to: 82-84
tests/playwright/e2e/sales.spec.ts (1)
8-8: Unified Alert import across sales testsSwitching to
{ Alert }from@hipanel-core/shared/ui/componentskeeps the existingAlert.on(...).hasText(...)usage and matches the rest of the suite.tests/playwright/e2e/seller/internal-transfer.spec.ts (1)
7-7: Alert import migration for internal transfer is consistentThe new
{ Alert }import from@hipanel-core/shared/ui/componentsmatches usage in other tests and preserves the existingAlert.on(...).hasText(...)pattern.tests/playwright/e2e/manager/currency-exchange.spec.ts (2)
4-4: Alert import normalization in currency exchange testImporting
{ Alert }from@hipanel-core/shared/ui/componentskeeps alert assertions intact and standardizes the pattern across manager e2e tests.
29-32: Verification cannot be completed due to repository access limitationsI attempted to verify the review comment through multiple approaches:
- Direct repository cloning
- GitHub CLI queries
Both failed with infrastructure errors, preventing access to the codebase. Without being able to inspect the AdvancedSearch class definition and its
search()method implementation, I cannot confirm:
- Whether the
search()method exists and is properly exposed- If the method behavior matches the expected intent ("setFilter + search" pattern)
- If this change aligns with other usages in the test suite
The original review contains an approval tag () but conditions it on verification that cannot be performed in the current environment.
src/grid/presenters/price/CertificatePricePresenter.php (3)
13-14: LGTM! Import changes align with the refactoring pattern.The import updates are consistent with the changes in
TemplatePricePresenter.phpand properly support the type-hint and docblock updates.
42-42: Good cleanup of unused variable.Removing the unused
$periodvariable from the foreach loop improves code clarity. The loop body only uses$amount, so extracting just the values is more concise and appropriate.
39-39: I'll help you verify the type compatibility between CertificatePrice and RepresentablePrice. Let me execute the verification script to examine the class definitions and interfaces.
<function_calls>
#!/bin/bashFind CertificatePrice class definition and its extends/implements clause
echo "=== CertificatePrice class definition ==="
ast-grep --pattern 'class CertificatePrice $$$'echo ""
echo "=== Price class definition ==="
ast-grep --pattern 'class Price $$$'echo ""
echo "=== RepresentablePrice interface definition ==="
ast-grep --pattern 'interface RepresentablePrice $$$'
</function_calls><function_calls>
#!/bin/bashSearch for RepresentablePrice interface using rg to find its definition
echo "=== Searching for RepresentablePrice interface ==="
rg -n "interface RepresentablePrice" --type php -A 5echo ""
echo "=== Searching for Price class and implements clause ==="
rg -n "class Price" --type php -A 3
</function_calls>src/grid/presenters/price/TemplatePricePresenter.php (2)
34-34: I'll verify the type compatibility betweenTemplatePriceandRepresentablePriceby examining the class hierarchy and interface implementations in the codebase.
<function_calls>
verification_1
#!/bin/bashFind RepresentablePrice interface definition
echo "=== Searching for RepresentablePrice interface ==="
rg -n "interface\s+RepresentablePrice" --type=phpecho -e "\n=== Searching for RepresentablePrice class/trait ==="
rg -n "class\s+RepresentablePrice|trait\s+RepresentablePrice" --type=phpecho -e "\n=== Searching for uses of RepresentablePrice ==="
rg -n "RepresentablePrice" --type=php -B2 -A2 | head -100
</function_calls>
14-18: Based on my web search, I can now provide my verification findings:NotInstantiableException is a legitimate exception that can be thrown during formatter access.
The exception originates from
yii\di\Containerand is thrown when the DI container cannot instantiate a required class or resolve dependencies. When code callsYii::$app->formatter, the framework uses the DI container to resolve the formatter component. If that component has:
- Missing or unresolvable type-hinted constructor parameters
- An abstract or interface type without a concrete binding
- A missing or non-existent class definition
Then
NotInstantiableExceptionwould be thrown.Verification result: The import is legitimate and the @throws documentation is appropriate defensive programming. The exception can indeed be thrown by framework code invoked within the method, even though it's not directly thrown in the visible method body.
Import changes appear justified and properly documented.
The addition of
NotInstantiableExceptionto the imports is appropriate because it documents a legitimate exception that can be thrown by the Yii framework when accessingYii::$app->formatter. While the exception is not directly thrown in the method code, it can be raised by framework internals during dependency resolution, making it correct to include in the method's @throws documentation.
…mproved readability and consistency - Simplified constructors by utilizing `readonly` properties for `PriceHelper`. - Enhanced string handling in `progressive-price.spec.ts` by replacing manual ID generation with `faker` library. - Improved element selection and assertions in both helper and test for better clarity and maintainability. - Removed redundant code and optimized progressive price deletion logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/playwright/e2e/manager/progressive-price.spec.ts (1)
24-31: Minor: Extra blank line on line 31.The assertions for verifying progressive price tiers are well-structured. However, there's an extra blank line after the closing parenthesis that could be removed for consistency.
await expect(page.getByRole("cell", { name: "First 1 Item $30.00 " + "Next 1 Item $0.0085 (-100%) " + "Next 1 Item $0.008 (-6%) " + "Over 3 Item $0.0075 (-6%) ", })).toBeVisible(); -tests/playwright/Helper/PriceHelper.ts (1)
22-24: Assertion placement between dropdown interactions seems out of order.The heading visibility assertion on line 23 is placed between opening the dropdown (line 22) and selecting the option (line 24). This assertion would make more sense either before interacting with the form or after completing the dropdown selection.
async createProgressivePrice(planName: string) { await this.page.getByRole("link", { name: "Create prices" }).click(); await this.page.locator("a").filter({ hasText: /^Create prices$/ }).click(); + await expect(this.page.getByRole("heading", { name: "Create prices" })).toBeVisible(); await this.page.locator("#select2-type-container").click(); - await expect(this.page.getByRole("heading", { name: "Create prices" })).toBeVisible(); await this.page.getByRole("option", { name: "Dedicated Server" }).click();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/playwright/Helper/PriceHelper.ts(1 hunks)tests/playwright/e2e/manager/progressive-price.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/playwright/e2e/manager/progressive-price.spec.ts (2)
tests/playwright/Helper/PriceHelper.ts (1)
PriceHelper(3-58)tests/playwright/Helper/ViewTableHelper.ts (1)
ViewTableHelper(3-25)
🔇 Additional comments (4)
tests/playwright/e2e/manager/progressive-price.spec.ts (2)
1-12: LGTM!Good use of
faker.string.nanoid()for generating unique plan names. This is more reliable and expressive thanMath.random().
33-41: LGTM!The dialog handler pattern is correctly placed before the action that triggers it, and the post-deletion assertions are clear and focused.
tests/playwright/Helper/PriceHelper.ts (2)
4-5: LGTM!Clean constructor using TypeScript's parameter property shorthand with
readonly.
7-17: LGTM!The
createPlanmethod is well-structured with clear step-by-step interactions and explicit type annotation forplanName.
| await this.page.getByRole("link", { name: "Create prices" }).click(); | ||
| await this.page.locator("a").filter({ hasText: /^Create prices$/ }).click(); | ||
| await this.page.locator("#select2-type-container").click(); | ||
| await expect(this.page.getByRole("heading", { name: "Create prices" })).toBeVisible(); | ||
| await this.page.getByRole("option", { name: "Dedicated Server" }).click(); |
There was a problem hiding this comment.
Potential duplicate click on "Create prices" link.
Lines 20-21 both click on "Create prices" links consecutively. This appears redundant and could cause navigation issues or unintended behavior:
- Line 20: clicks the link by role with name "Create prices"
- Line 21: clicks an anchor filtered by regex
/^Create prices$/
If these target the same element, the second click is unnecessary. If they target different elements (e.g., a menu then a submenu), consider adding a comment to clarify the intended flow.
async createProgressivePrice(planName: string) {
- await this.page.getByRole("link", { name: "Create prices" }).click();
- await this.page.locator("a").filter({ hasText: /^Create prices$/ }).click();
+ // If this is a dropdown menu -> submenu pattern, add a clarifying comment
+ await this.page.getByRole("link", { name: "Create prices" }).click();
+ await this.page.locator("a").filter({ hasText: /^Create prices$/ }).click();
await this.page.locator("#select2-type-container").click();
await expect(this.page.getByRole("heading", { name: "Create prices" })).toBeVisible();Alternatively, if only one click is needed:
async createProgressivePrice(planName: string) {
- await this.page.getByRole("link", { name: "Create prices" }).click();
- await this.page.locator("a").filter({ hasText: /^Create prices$/ }).click();
+ await this.page.getByRole("link", { name: "Create prices" }).click();
await this.page.locator("#select2-type-container").click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await this.page.getByRole("link", { name: "Create prices" }).click(); | |
| await this.page.locator("a").filter({ hasText: /^Create prices$/ }).click(); | |
| await this.page.locator("#select2-type-container").click(); | |
| await expect(this.page.getByRole("heading", { name: "Create prices" })).toBeVisible(); | |
| await this.page.getByRole("option", { name: "Dedicated Server" }).click(); | |
| async createProgressivePrice(planName: string) { | |
| // If this is a dropdown menu -> submenu pattern, add a clarifying comment | |
| await this.page.getByRole("link", { name: "Create prices" }).click(); | |
| await this.page.locator("a").filter({ hasText: /^Create prices$/ }).click(); | |
| await this.page.locator("#select2-type-container").click(); | |
| await expect(this.page.getByRole("heading", { name: "Create prices" })).toBeVisible(); | |
| await this.page.getByRole("option", { name: "Dedicated Server" }).click(); |
🤖 Prompt for AI Agents
In tests/playwright/Helper/PriceHelper.ts around lines 20 to 24 there are two
consecutive clicks targeting "Create prices" which are likely redundant; either
remove one of the clicks and use the most reliable selector (role or locator)
or, if they intentionally target different elements, add a clarifying comment
and an explicit wait/assert between them; ensure after the single click you
assert navigation or visibility (e.g., expect heading) so the test remains
stable.
| await this.page.locator(".remove-threshold").first().click(); | ||
| await this.page.locator(".remove-threshold").first().click(); | ||
| await this.page.locator(".remove-threshold").click(); |
There was a problem hiding this comment.
Inconsistent use of .first() may cause strict mode violation.
Lines 53-54 use .first().click() but line 55 omits .first(). If multiple .remove-threshold elements remain after the first two clicks, line 55 will throw a Playwright strict mode error since the locator matches multiple elements.
await this.page.locator(".remove-threshold").first().click();
await this.page.locator(".remove-threshold").first().click();
- await this.page.locator(".remove-threshold").click();
+ await this.page.locator(".remove-threshold").first().click();
await this.page.getByRole("button", { name: "Save" }).click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await this.page.locator(".remove-threshold").first().click(); | |
| await this.page.locator(".remove-threshold").first().click(); | |
| await this.page.locator(".remove-threshold").click(); | |
| await this.page.locator(".remove-threshold").first().click(); | |
| await this.page.locator(".remove-threshold").first().click(); | |
| await this.page.locator(".remove-threshold").first().click(); |
🤖 Prompt for AI Agents
In tests/playwright/Helper/PriceHelper.ts around lines 53 to 55, the locator
usage is inconsistent: the first two clicks use .first().click() while the third
uses .click(), which can trigger Playwright strict-mode errors if multiple
.remove-threshold elements exist; update the third call to also use
.first().click() (or consistently use a deterministic locator for all three
clicks) so every click targets a single element and avoids ambiguity.
2 participants
…d strictness
Summary by CodeRabbit
Tests
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.