HP-2382 Migrate PartCreateCest.php Codeception Test to Playwright and remove Legacy Test #185
Conversation
… Remove Legacy Test
|
Warning Rate limit exceeded@VadymHrechukha has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis changeset introduces new Playwright page objects and end-to-end test suites for part management. New classes, PartCreateView, PartIndexView, PartReplaceView, PartForm, PartView, SellModal, and BillIndexView encapsulate UI interactions for creating, managing, replacing, and selling parts. Additionally, the PriceWithCurrency class provides a utility for handling currency-based inputs. Complementary test suites validate functionalities such as part creation, copying, deletion, replacement, and selling, including filter application and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Suite
participant PCV as PartCreateView
participant UI as Browser Page
T ->> PCV: navigate()
PCV ->> UI: Open part creation page
T ->> PCV: fillPartFields(data)
PCV ->> UI: Populate form fields with data
T ->> PCV: addPart()/copyPart()/removePart()
PCV ->> UI: Perform corresponding button clicks
T ->> PCV: save()
PCV ->> UI: Click 'Save' button and submit form
sequenceDiagram
participant T as Test Suite
participant PIV as PartIndexView
participant UI as Browser Page
T ->> PIV: navigate()
PIV ->> UI: Open part index page
T ->> PIV: applyFilters(filters)
PIV ->> UI: Set filters and submit search
T ->> PIV: selectPartsToReplace(count)
PIV ->> UI: Select table rows for replacement
T ->> PIV: confirmReplacement()
PIV ->> UI: Click bulk 'Replace' button and verify notification
sequenceDiagram
participant T as Test Suite
participant SM as SellModal
participant UI as Browser Page
T ->> SM: fillSellWindowFields(data)
SM ->> UI: Populate sell modal fields
T ->> SM: confirmSale()
SM ->> UI: Click 'Sell' button
T ->> SM: seePartsWereSold()
SM ->> UI: Verify sale confirmation notification
Poem
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. 🪧 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 (
|
… Remove Legacy Test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
tests/playwright/page/PartCreateView.ts (3)
1-1: Remove unused importThe
expectimport is not used in this class. It's good practice to keep imports clean.-import {expect, Locator, Page} from "@playwright/test"; +import {Locator, Page} from "@playwright/test";
14-23: Replaceanytype with proper interface and add validation checksUsing
anyreduces type safety. Define a proper interface forpartDataand consider adding validation or waiting for elements before interactions.+interface PartData { + partno: string; + src_id: string; + dst_id: string; + serials: string; + move_descr: string; + price: number; + currency: string; + company_id: string; +} -public async fillPartFields(partData: any) { +public async fillPartFields(partData: PartData) { + // Wait for form to be ready + await this.page.waitForSelector('select[id$=partno]'); await this.page.selectOption('select[id$=partno]', partData.partno); await this.page.selectOption('select[id$=src_id]', partData.src_id); await this.page.selectOption('select[id$=dst_ids]', partData.dst_id); await this.page.fill('input[id$=serials]', partData.serials); await this.page.fill('input[id$=move_descr]', partData.move_descr); await this.page.fill('input[id$=price]', partData.price.toString()); - await this.page.click(`li a[data-value=${partData.currency}]`); + await this.page.click(`li a[data-value="${partData.currency}"]`); await this.page.selectOption('select[id$=company_id]', partData.company_id); }
25-39: Add wait conditions for more robust interactionThe current button interactions might fail if elements are not yet ready or visible. Adding wait conditions would make these methods more robust.
public async save() { + await this.page.waitForSelector('button:has-text("Save")'); await this.page.click('button:has-text("Save")'); } public async addPart() { + await this.page.waitForSelector("div.item:last-child button.add-item"); await this.page.click("div.item:last-child button.add-item"); } public async removePart() { + await this.page.waitForSelector("div.item:last-child button.remove-item"); await this.page.click("div.item:last-child button.remove-item"); } public async copyPart() { + await this.page.waitForSelector("div.item:last-child button.copy"); await this.page.click("div.item:last-child button.copy"); }tests/playwright/e2e/manager/part-create.spec.ts (4)
24-25: Avoid using side-effect operators in assertion messagesUsing
++ninside an assertion call can be confusing as it modifies the variable while also using its updated value. This makes the code harder to understand.await partView.addPart(); -expect(await managerPage.locator('div.item').count()).toBe(++n); +n++; +expect(await managerPage.locator('div.item').count()).toBe(n);This comment applies to all similar occurrences throughout the file.
43-61: Extract error messages to constants and add descriptions to assertionsExtracting error messages to constants would make the test more maintainable and adding descriptions to assertions would make test failures more informative.
+const VALIDATION_ERRORS = { + partNo: 'Part No. cannot be blank.', + source: 'Source cannot be blank.', + destination: 'Destination cannot be blank.', + serials: 'Serials cannot be blank.', + moveDesc: 'Move description cannot be blank.', + price: 'Purchase price cannot be blank.', + currency: 'Currency cannot be blank.', +}; test('Ensure part cannot be created without data @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); await partView.navigate(); await partView.save(); - const errorMessages = [ - 'Part No. cannot be blank.', - 'Source cannot be blank.', - 'Destination cannot be blank.', - 'Serials cannot be blank.', - 'Move description cannot be blank.', - 'Purchase price cannot be blank.', - 'Currency cannot be blank.', - ]; + const errorMessages = Object.values(VALIDATION_ERRORS); for (const message of errorMessages) { - await expect(managerPage.locator(`text=${message}`)).toBeVisible(); + await expect(managerPage.locator(`text=${message}`)).toBeVisible({ + message: `Expected error message "${message}" to be visible` + }); } });
87-89: Add timeout to expectations for better error messagesAdding timeouts to expectations will provide clearer error messages if the operation takes longer than expected.
managerPage.on('dialog', async dialog => await dialog.accept()); await managerPage.click('text=Delete'); -await expect(managerPage.locator('text=Part has been deleted')).toBeVisible(); +await expect(managerPage.locator('text=Part has been deleted')).toBeVisible({ + timeout: 5000, + message: 'Expected confirmation message for part deletion to appear' +});
17-41: Add cleanup and simplify button interaction testThe test adds and removes parts several times, which is redundant. A simplified test with cleanup would be more maintainable.
test('Ensure part management buttons work @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); await partView.navigate(); let n = await managerPage.locator('div.item').count(); expect(n).toBe(1); // Test add button await partView.addPart(); n++; expect(await managerPage.locator('div.item').count()).toBe(n); // Test copy button await partView.copyPart(); n++; expect(await managerPage.locator('div.item').count()).toBe(n); // Test remove button await partView.removePart(); n--; expect(await managerPage.locator('div.item').count()).toBe(n); - await partView.addPart(); - expect(await managerPage.locator('div.item').count()).toBe(++n); - - await partView.copyPart(); - expect(await managerPage.locator('div.item').count()).toBe(++n); - - await partView.removePart(); - expect(await managerPage.locator('div.item').count()).toBe(--n); - - await partView.removePart(); - expect(await managerPage.locator('div.item').count()).toBe(--n); - - await partView.removePart(); - expect(await managerPage.locator('div.item').count()).toBe(--n); + + // Cleanup - remove all additional items + while (n > 1) { + await partView.removePart(); + n--; + expect(await managerPage.locator('div.item').count()).toBe(n); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/playwright/e2e/manager/part-create.spec.ts(1 hunks)tests/playwright/page/PartCreateView.ts(1 hunks)
🔇 Additional comments (1)
tests/playwright/page/PartCreateView.ts (1)
17-17:✅ Verification successful
Verify selector and parameter name consistency
The selector uses
dst_ids(plural) but the parameter is nameddst_id(singular). Consider making these consistent to prevent future errors.
🏁 Script executed:
#!/bin/bash # Searching for dst_ids fields in the codebase to verify correct naming rg -A 2 "dst_ids" --type html --type php rg -A 2 "dst_id" --type html --type phpLength of output: 12329
Selector and Parameter Naming Verified
After reviewing the codebase, it is confirmed that both
dst_ids(used in selectors and model attributes) anddst_id(used for singular values in test data and controller transformations) are intentionally and consistently applied throughout the project. The design handles multiple destination IDs via the HTML element (dst_ids) while working with a single selected value (dst_id) where applicable. No changes are needed at this time.
| await this.page.fill('input[id$=serials]', partData.serials); | ||
| await this.page.fill('input[id$=move_descr]', partData.move_descr); | ||
| await this.page.fill('input[id$=price]', partData.price.toString()); | ||
| await this.page.click(`li a[data-value=${partData.currency}]`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add string interpolation safety for currency selector
Use proper string interpolation with quotes to prevent potential XSS or injection issues when concatenating values into selectors.
-await this.page.click(`li a[data-value=${partData.currency}]`);
+await this.page.click(`li a[data-value="${partData.currency}"]`);📝 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.click(`li a[data-value=${partData.currency}]`); | |
| await this.page.click(`li a[data-value="${partData.currency}"]`); |
| test('Ensure a part can be created @hipanel-module-stock @manager', async ({ managerPage }) => { | ||
| const partView = new PartCreateView(managerPage); | ||
| await partView.navigate(); | ||
| await partView.fillPartFields(partData); | ||
| await partView.save(); | ||
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use dynamic test data for each test
Use the function suggested earlier to create unique test data for each test to prevent conflicts between tests.
test('Ensure a part can be created @hipanel-module-stock @manager', async ({ managerPage }) => {
const partView = new PartCreateView(managerPage);
+ const uniquePartData = createPartData();
await partView.navigate();
- await partView.fillPartFields(partData);
+ await partView.fillPartFields(uniquePartData);
await partView.save();
await expect(managerPage.locator('text=Part has been created')).toBeVisible();
});📝 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.
| test('Ensure a part can be created @hipanel-module-stock @manager', async ({ managerPage }) => { | |
| const partView = new PartCreateView(managerPage); | |
| await partView.navigate(); | |
| await partView.fillPartFields(partData); | |
| await partView.save(); | |
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | |
| }); | |
| test('Ensure a part can be created @hipanel-module-stock @manager', async ({ managerPage }) => { | |
| const partView = new PartCreateView(managerPage); | |
| const uniquePartData = createPartData(); | |
| await partView.navigate(); | |
| await partView.fillPartFields(uniquePartData); | |
| await partView.save(); | |
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | |
| }); |
| const partData = { | ||
| partno: 'CHASSIS EPYC 7402P', | ||
| src_id: 'TEST-DS-01', | ||
| dst_id: 'TEST-DS-02', | ||
| serials: `MG_TEST_PART${Date.now()}`, | ||
| move_descr: 'MG TEST MOVE', | ||
| price: 200, | ||
| currency: 'usd', | ||
| company_id: 'Other', | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use dynamic serials for all tests to prevent collisions
The current implementation uses a timestamp for serials, but it's defined once at the top level. This can cause collisions if tests run quickly in sequence.
-const partData = {
+function createPartData() {
+ return {
partno: 'CHASSIS EPYC 7402P',
src_id: 'TEST-DS-01',
dst_id: 'TEST-DS-02',
- serials: `MG_TEST_PART${Date.now()}`,
+ serials: `MG_TEST_PART${Date.now()}_${Math.floor(Math.random() * 1000)}`,
move_descr: 'MG TEST MOVE',
price: 200,
currency: 'usd',
company_id: 'Other',
+ };
};📝 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.
| const partData = { | |
| partno: 'CHASSIS EPYC 7402P', | |
| src_id: 'TEST-DS-01', | |
| dst_id: 'TEST-DS-02', | |
| serials: `MG_TEST_PART${Date.now()}`, | |
| move_descr: 'MG TEST MOVE', | |
| price: 200, | |
| currency: 'usd', | |
| company_id: 'Other', | |
| }; | |
| function createPartData() { | |
| return { | |
| partno: 'CHASSIS EPYC 7402P', | |
| src_id: 'TEST-DS-01', | |
| dst_id: 'TEST-DS-02', | |
| serials: `MG_TEST_PART${Date.now()}_${Math.floor(Math.random() * 1000)}`, | |
| move_descr: 'MG TEST MOVE', | |
| price: 200, | |
| currency: 'usd', | |
| company_id: 'Other', | |
| }; | |
| }; |
| test('Ensure a part can be created and then deleted @hipanel-module-stock @manager', async ({ managerPage }) => { | ||
| const partView = new PartCreateView(managerPage); | ||
| await partView.navigate(); | ||
| await partView.fillPartFields(partData); | ||
| await partView.save(); | ||
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | ||
|
|
||
| await managerPage.click('text=Delete'); | ||
| managerPage.on('dialog', async dialog => await dialog.accept()); | ||
| await expect(managerPage.locator('text=Part has been deleted')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Register dialog handler before triggering dialog and use unique test data
The dialog handler should be registered before any action that might trigger a dialog, and each test should use unique data.
test('Ensure a part can be created and then deleted @hipanel-module-stock @manager', async ({ managerPage }) => {
const partView = new PartCreateView(managerPage);
+ const uniquePartData = createPartData();
await partView.navigate();
- await partView.fillPartFields(partData);
+ await partView.fillPartFields(uniquePartData);
await partView.save();
await expect(managerPage.locator('text=Part has been created')).toBeVisible();
+ // Register dialog handler before clicking delete
+ managerPage.on('dialog', async dialog => await dialog.accept());
await managerPage.click('text=Delete');
- managerPage.on('dialog', async dialog => await dialog.accept());
await expect(managerPage.locator('text=Part has been deleted')).toBeVisible();
});📝 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.
| test('Ensure a part can be created and then deleted @hipanel-module-stock @manager', async ({ managerPage }) => { | |
| const partView = new PartCreateView(managerPage); | |
| await partView.navigate(); | |
| await partView.fillPartFields(partData); | |
| await partView.save(); | |
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | |
| await managerPage.click('text=Delete'); | |
| managerPage.on('dialog', async dialog => await dialog.accept()); | |
| await expect(managerPage.locator('text=Part has been deleted')).toBeVisible(); | |
| }); | |
| test('Ensure a part can be created and then deleted @hipanel-module-stock @manager', async ({ managerPage }) => { | |
| const partView = new PartCreateView(managerPage); | |
| const uniquePartData = createPartData(); | |
| await partView.navigate(); | |
| await partView.fillPartFields(uniquePartData); | |
| await partView.save(); | |
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | |
| // Register dialog handler before clicking delete | |
| managerPage.on('dialog', async dialog => await dialog.accept()); | |
| await managerPage.click('text=Delete'); | |
| await expect(managerPage.locator('text=Part has been deleted')).toBeVisible(); | |
| }); |
| test('Ensure multiple parts can be created @hipanel-module-stock @manager', async ({ managerPage }) => { | ||
| const partView = new PartCreateView(managerPage); | ||
| await partView.navigate(); | ||
| await partView.fillPartFields(partData); | ||
| await partView.addPart(); | ||
| await partView.save(); | ||
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use dynamic test data and add validation for multiple parts
Each test should use unique data, and this test should verify that multiple parts were actually created.
test('Ensure multiple parts can be created @hipanel-module-stock @manager', async ({ managerPage }) => {
const partView = new PartCreateView(managerPage);
+ const uniquePartData = createPartData();
await partView.navigate();
- await partView.fillPartFields(partData);
+ await partView.fillPartFields(uniquePartData);
await partView.addPart();
+ // Fill the second part with different data
+ const secondPartData = createPartData();
+ await partView.fillPartFields(secondPartData);
await partView.save();
await expect(managerPage.locator('text=Part has been created')).toBeVisible();
+ // Verify that multiple parts were created
+ await expect(managerPage.locator('text=Created 2 parts')).toBeVisible();
});📝 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.
| test('Ensure multiple parts can be created @hipanel-module-stock @manager', async ({ managerPage }) => { | |
| const partView = new PartCreateView(managerPage); | |
| await partView.navigate(); | |
| await partView.fillPartFields(partData); | |
| await partView.addPart(); | |
| await partView.save(); | |
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | |
| }); | |
| test('Ensure multiple parts can be created @hipanel-module-stock @manager', async ({ managerPage }) => { | |
| const partView = new PartCreateView(managerPage); | |
| const uniquePartData = createPartData(); | |
| await partView.navigate(); | |
| await partView.fillPartFields(uniquePartData); | |
| await partView.addPart(); | |
| // Fill the second part with different data | |
| const secondPartData = createPartData(); | |
| await partView.fillPartFields(secondPartData); | |
| await partView.save(); | |
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | |
| // Verify that multiple parts were created | |
| await expect(managerPage.locator('text=Created 2 parts')).toBeVisible(); | |
| }); |
… Remove Legacy Test
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/playwright/e2e/manager/part-create.spec.ts (4)
5-14: 🛠️ Refactor suggestionCreate a function to generate unique test data.
The current implementation uses a single
partDataobject with a timestamp that's generated once when the module loads. This can cause data collisions if tests run in quick succession.Replace the static object with a function that generates fresh data for each test:
-const partData = { - partno: 'CHASSIS EPYC 7402P', - src_id: 'TEST-DS-01', - dst_id: 'TEST-DS-02', - serials: `MG_TEST_PART${Date.now()}`, - move_descr: 'MG TEST MOVE', - price: 200, - currency: '$', - company_id: 'Other', -}; +function createPartData() { + return { + partno: 'CHASSIS EPYC 7402P', + src_id: 'TEST-DS-01', + dst_id: 'TEST-DS-02', + serials: `MG_TEST_PART${Date.now()}_${Math.floor(Math.random() * 1000)}`, + move_descr: 'MG TEST MOVE', + price: 200, + currency: '$', + company_id: 'Other', + }; +}
63-69: 🛠️ Refactor suggestionUse dynamic test data for each test run.
Each test should use unique data to prevent conflicts between test runs.
test('Ensure a part can be created @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); + const uniquePartData = createPartData(); await partView.navigate(); - await partView.fillPartFields(partData); + await partView.fillPartFields(uniquePartData); await partView.save(); await expect(managerPage.locator('text=Part has been created')).toBeVisible(); });
71-78: 🛠️ Refactor suggestionUse unique data and add validation for multiple parts creation.
The test should use unique data for each part and verify that multiple parts were actually created.
test('Ensure multiple parts can be created @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); + const uniquePartData = createPartData(); await partView.navigate(); - await partView.fillPartFields(partData); + await partView.fillPartFields(uniquePartData); await partView.addPart(); + // Fill the second part with different data + const secondPartData = createPartData(); + await partView.fillPartFields(secondPartData, 1); // Assuming fillPartFields can accept an index await partView.save(); await expect(managerPage.locator('text=Part has been created')).toBeVisible(); + // Verify that multiple parts were created + await expect(managerPage.locator('text=Parts have been created successfully')).toBeVisible(); + // Or check for a specific count message if available + // await expect(managerPage.locator('text=Created 2 parts')).toBeVisible(); });
80-90:⚠️ Potential issueRegister dialog handler before triggering the dialog.
The dialog handler should be registered before any action that might trigger a dialog to ensure it catches the dialog properly.
test('Ensure a part can be created and then deleted @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); + const uniquePartData = createPartData(); await partView.navigate(); - await partView.fillPartFields(partData); + await partView.fillPartFields(uniquePartData); await partView.save(); await expect(managerPage.locator('text=Part has been created')).toBeVisible(); + // Register dialog handler before clicking delete + managerPage.on('dialog', async dialog => await dialog.accept()); await managerPage.click('text=Delete'); - managerPage.on('dialog', async dialog => await dialog.accept()); await expect(managerPage.locator('text=Part has been deleted')).toBeVisible(); });
🧹 Nitpick comments (2)
tests/playwright/e2e/manager/part-create.spec.ts (2)
2-2: Standardize the import style.The import style is inconsistent: one import uses curly braces and one doesn't.
-import { test } from "@hipanel-core/fixtures"; -import {expect} from "@playwright/test"; +import { test } from "@hipanel-core/fixtures"; +import { expect } from "@playwright/test";
21-22: Consider using a more maintainable approach for repeated assertions.The test has several repetitive assertions checking the item count. Consider using a helper function to make this more maintainable.
test('Ensure part management buttons work @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); await partView.navigate(); + // Helper function to assert item count + async function assertItemCount(expected: number) { + expect(await managerPage.locator('div.item').count()).toBe(expected); + } + let n = await managerPage.locator('div.item').count(); - expect(n).toBe(1); + await assertItemCount(1); await partView.addPart(); - expect(await managerPage.locator('div.item').count()).toBe(++n); + await assertItemCount(++n); await partView.addPart(); - expect(await managerPage.locator('div.item').count()).toBe(++n); + await assertItemCount(++n); await partView.copyPart(); - expect(await managerPage.locator('div.item').count()).toBe(++n); + await assertItemCount(++n); await partView.removePart(); - expect(await managerPage.locator('div.item').count()).toBe(--n); + await assertItemCount(--n); await partView.removePart(); - expect(await managerPage.locator('div.item').count()).toBe(--n); + await assertItemCount(--n); await partView.removePart(); - expect(await managerPage.locator('div.item').count()).toBe(--n); + await assertItemCount(--n); });Also applies to: 25-26, 28-29, 31-32, 34-35, 37-38, 40-41
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/playwright/e2e/manager/part-create.spec.ts(1 hunks)tests/playwright/page/PartCreateView.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/playwright/page/PartCreateView.ts
🔇 Additional comments (3)
tests/playwright/e2e/manager/part-create.spec.ts (3)
16-41: Excellent test coverage for part management UI operations!This test validates all the basic UI operations (add, copy, remove parts) and verifies the item count after each action, providing solid coverage of the basic functionality.
43-61: Comprehensive validation of error messages - good practice!This test effectively verifies that all required fields show appropriate error messages when attempting to save without data. The loop approach for checking error messages is clean and maintainable.
12-12:❓ Verification inconclusive
Verify currency format consistency across the application.
The currency is defined as '$' here, but error messages check for "Currency cannot be blank." Verify that '$' is considered a valid currency value in the application.
🏁 Script executed:
#!/bin/bash # Check if there are any validation functions for currency in the codebase echo "Searching for currency validation functions..." rg -A 10 -B 10 "currency.*valid|valid.*currency" --glob "*.{php,js,ts}" echo "Checking if '$' is used as a valid currency value elsewhere..." rg "currency.*['\"]\\$['\"]|['\"]\\$['\"].*currency" --glob "*.{php,js,ts}"Length of output: 434
Below is a revised verification step to help confirm whether using
'$'as the currency value is properly supported across the application. It appears that the initial shell commands ran into quoting issues. Please run the following revised shell script to re-check for any currency validation functions and usages:#!/bin/bash # Revised script to search for currency validation functions and currency usages with proper quoting echo "Searching for currency validation functions..." rg -A 10 -B 10 "currency.*valid|valid.*currency" --glob "*.{php,js,ts}" echo "Checking if '\$' is used as a valid currency value elsewhere..." rg -A 10 -B 10 "currency.*['\"]\\\$['\"]" --glob "*.{php,js,ts}"Action: Verify Currency Validation Consistency
- In tests/playwright/e2e/manager/part-create.spec.ts at line 12 the code specifies
currency: '$',.- Please confirm that using
'$'meets the intended validation rules—and that it doesn’t incorrectly trigger the "Currency cannot be blank." error elsewhere in the application.Once the revised script’s output is reviewed and validated, please ensure the currency format consistency is maintained.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
tests/playwright/page/PartCreateView.ts (1)
16-27: Consider creating a typed interface for part data.The
fillPartFieldsmethod accepts ananytype forpartData, which reduces type safety. Creating a specific interface would make the code more self-documenting and help catch errors at compile time.+interface PartData { + partno: string; + src_id: string; + dst_id: string; + serials: string; + move_descr: string; + price: number; + currency: string; + company_id: string; +} export default class PartCreateView { private page: Page; public constructor(page: Page) { this.page = page; } public async navigate() { await this.page.goto('/stock/part/create'); } - public async fillPartFields(partData: any) { + public async fillPartFields(partData: PartData) { await Select2.field(this.page, `select[id$=partno]`).setValue(partData.partno); await Select2.field(this.page, `select[id$=src_id]`).setValue(partData.src_id); await Select2.field(this.page, `select[id$=dst_ids]`).setValue(partData.dst_id); await this.page.fill('input[id$=serials]', partData.serials); await this.page.fill('input[id$=move_descr]', partData.move_descr); await PriceWithCurrency.field(this.page, "part", 0).setSumAndCurrency(partData.price, partData.currency); await this.page.selectOption('select[id$=company_id]', partData.company_id); }tests/playwright/e2e/manager/part/part-replace.spec.ts (1)
4-19: Consider using test.beforeEach for test data setup.The test data is defined at the top level, which is fine for a single test but would be better organized within a
test.beforeEachblock if more tests are added later, especially if they need to use fresh data for each run.import { test } from "@hipanel-core/fixtures"; import PartIndexView from "@hipanel-module-stock/page/PartIndexView"; -const data = { - filters: [ - { - name: "move_descr_ilike", - value: "test description" - }, - { - name: "model_types", - value: "cpu" - }, - ], - replaceData: [ - { serialno: `test${Date.now()}` }, - { serialno: `test${Date.now()}` } - ], -}; test.describe("Part Replacement", () => { + let data: { + filters: Array<{name: string, value: string}>; + replaceData: Array<{serialno: string}>; + }; + + test.beforeEach(() => { + data = { + filters: [ + { + name: "move_descr_ilike", + value: "test description" + }, + { + name: "model_types", + value: "cpu" + }, + ], + replaceData: [ + { serialno: `test${Date.now()}` }, + { serialno: `test${Date.now()}` } + ], + }; + }); + test("Ensure parts can be replaced @hipanel-module-stock @manager", async ({ managerPage }) => {tests/playwright/page/PartIndexView.ts (1)
31-35: Consider making the fillReplaceForm method more robust.The current implementation uses a complex CSS selector that might be brittle if the table structure changes. Consider using data attributes or more reliable selectors if possible.
public async fillReplaceForm(replaceData: { serialno: string }[], rowNumbers: number[]) { for (const [index, data] of replaceData.entries()) { - await this.page.fill(`table tbody tr:nth-child(${rowNumbers[index]}) input[name='serialno']`, data.serialno); + // Assuming rows have a data attribute or can be identified more reliably + await this.page.fill(`table tbody tr[data-key="${rowNumbers[index]}"] input[name='serialno']`, data.serialno); + // Or use locator for more flexibility + const row = this.page.locator(`table tbody tr`).nth(rowNumbers[index] - 1); + await row.locator(`input[name='serialno']`).fill(data.serialno); } }If the table structure is fixed and well-defined, you might want to add a comment documenting the selector structure for future maintainers.
tests/playwright/e2e/manager/part/part-create.spec.ts (4)
1-14: Good test data setup with dynamic values for uniqueness.Using a timestamp in the serial number is a good approach to prevent test data collisions. The test data is well-structured with all necessary fields for part creation.
Consider extracting test data to a separate fixture or helper function to allow reuse across different test files:
- const partData = { - partno: 'CHASSIS EPYC 7402P', - src_id: 'TEST-DS-01', - dst_id: 'TEST-DS-02', - serials: `MG_TEST_PART${Date.now()}`, - move_descr: 'MG TEST MOVE', - price: 200, - currency: '$', - company_id: 'Other', - }; + import { generatePartData } from '@hipanel-module-stock/fixtures/part-data'; + + const partData = generatePartData();
16-41: Testing UI interactions and element counts is a good approach.This test effectively verifies that the part management buttons work correctly by checking the count of parts after each operation.
Consider adding descriptive comments about the expected behavior for better maintainability:
test('Ensure part management buttons work @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); await partView.navigate(); let n = await managerPage.locator('div.item').count(); expect(n).toBe(1); + // Adding parts should increase the count await partView.addPart(); expect(await managerPage.locator('div.item').count()).toBe(++n); await partView.addPart(); expect(await managerPage.locator('div.item').count()).toBe(++n); + // Copying a part should also increase the count await partView.copyPart(); expect(await managerPage.locator('div.item').count()).toBe(++n); + // Removing parts should decrease the count await partView.removePart(); expect(await managerPage.locator('div.item').count()).toBe(--n); await partView.removePart(); expect(await managerPage.locator('div.item').count()).toBe(--n); await partView.removePart(); expect(await managerPage.locator('div.item').count()).toBe(--n); });
63-69: Simple and effective test for part creation.The test confirms that a part can be successfully created with the provided data.
Consider extending the test to verify more than just the success message:
test('Ensure a part can be created @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); await partView.navigate(); await partView.fillPartFields(partData); await partView.save(); await expect(managerPage.locator('text=Part has been created')).toBeVisible(); + + // Verify we're redirected to the index page + await expect(managerPage).toHaveURL(/index/); + + // Verify the created part appears in the list + await expect(managerPage.locator(`text=${partData.partno}`)).toBeVisible(); + await expect(managerPage.locator(`text=${partData.serials}`)).toBeVisible(); });
1-91: Consider adding tests for edge cases and error scenarios.The test suite covers the basic functionality well but lacks tests for edge cases and error scenarios.
Consider adding tests for:
- Creating a part with an existing serial number (expecting an error)
- Creating a part with invalid price values
- Testing sorting and filtering in the parts list
- Testing the cancel button functionality
Example test for duplicate serial:
test('Should show error when creating part with existing serial @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); // Create first part await partView.navigate(); const testData = { ...partData, serials: `DUPLICATE_SERIAL_${Date.now()}` }; await partView.fillPartFields(testData); await partView.save(); await expect(managerPage.locator('text=Part has been created')).toBeVisible(); // Try to create second part with same serial await partView.navigate(); await partView.fillPartFields(testData); await partView.save(); await expect(managerPage.locator('text=Serial number has already been taken')).toBeVisible(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/playwright/e2e/manager/part/part-create.spec.ts(1 hunks)tests/playwright/e2e/manager/part/part-replace.spec.ts(1 hunks)tests/playwright/input/PriceWithCurrency.ts(1 hunks)tests/playwright/page/PartCreateView.ts(1 hunks)tests/playwright/page/PartIndexView.ts(1 hunks)
🔇 Additional comments (4)
tests/playwright/input/PriceWithCurrency.ts (1)
1-8: Class implementation looks clean and focused.The
PriceWithCurrencyclass provides a nice abstraction for price input fields with currency support. The static factory method makes it easy to create instances with the correct identifier pattern.tests/playwright/page/PartCreateView.ts (2)
24-24: Use string interpolation safety for PriceWithCurrency.Similar to the previous review comment on currency selectors, ensure that string values are properly escaped when passed to methods to prevent potential injection issues.
Let's check the implementation of the
setSumAndCurrencymethod to ensure it uses proper string escaping:#!/bin/bash # Check the implementation of setSumAndCurrency in InputWithCurrency fd -t file "InputWithCurrency.(ts|js)" | xargs cat | grep -A 15 "setSumAndCurrency"
3-3:❓ Verification inconclusive
Verify import path consistency.
The import path
@hipanel-module-stock/input/PriceWithCurrencyseems inconsistent with the file location we're reviewing (tests/playwright/input/PriceWithCurrency.ts). Please ensure this import resolves correctly in your project setup.
🏁 Script executed:
#!/bin/bash # Check if there's another PriceWithCurrency implementation in the codebase fd -t file "PriceWithCurrency.(ts|js)" --exclude "tests/playwright/input/PriceWithCurrency.ts" # Check if there's a path mapping in tsconfig.json for @hipanel-module-stock fd -t file "tsconfig.json" | xargs cat | grep -A 10 "paths"Length of output: 152
Confirm Import Alias Configuration
The import statement
import PriceWithCurrency from "@hipanel-module-stock/input/PriceWithCurrency";references an alias that appears inconsistent with the actual file location at
tests/playwright/input/PriceWithCurrency.ts. Our initial automated checks (search for alternative implementations and tsconfig path mappings) did not yield evidence of an alias configuration that would resolve@hipanel-module-stockto thetests/playwright/inputdirectory.Please verify the following manually:
- Confirm that the alias
@hipanel-module-stockis correctly configured (e.g., intsconfig.json, Webpack, or another bundler configuration) to point to the intended directory.- Ensure that this import resolves as expected in your project’s setup across different environments (development, testing, production).
- If the alias mapping is not present or is misconfigured, either update the configuration or adjust the import path to match the file structure.
tests/playwright/e2e/manager/part/part-create.spec.ts (1)
43-61:❓ Verification inconclusive
Good validation testing with comprehensive error message checks.
This test effectively validates that form submission is blocked when required fields are missing and that appropriate error messages are displayed.
Ensure that these error messages are consistent with the actual application. If translations are used, the messages might change:
🏁 Script executed:
#!/bin/bash # Check for error messages in the project's language files grep -r "Part No. cannot be blank" --include="*.php" --include="*.json" . grep -r "Source cannot be blank" --include="*.php" --include="*.json" . grep -r "Destination cannot be blank" --include="*.php" --include="*.json" . # Add more relevant searches as neededLength of output: 228
Action Required: Manual Verification of Error Message Translations
The test case effectively ensures that form submission is blocked when required fields are missing and checks for multiple error messages. However, our automated search did not locate these messages in the language files. Please manually verify that the error messages (e.g., "Part No. cannot be blank.", "Source cannot be blank.", etc.) match the actual text displayed by the application—especially if translations or dynamic localization is used.
- Review Points:
- Ensure that the messages tested in the spec are consistent with those defined in your language/translation files.
- Confirm that any translation updates or alternate language usage has been reflected in both the tests and the application's UI.
| await partIndexView.selectPartsToReplace(1, data.replaceData.length); | ||
| // await partIndexView.fillReplaceForm(data.replaceData, data.rowNumbers); | ||
| //await partIndexView.confirmReplacement(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug pause and uncomment implementation.
There's a pause() call in the test, which is typically used for debugging and should be removed before merging. Additionally, there are commented-out lines suggesting the test implementation is incomplete.
await partIndexView.navigate();
await partIndexView.applyFilters(data.filters);
await partIndexView.selectPartsToReplace(1, data.replaceData.length);
-await partIndexView.pause();
-// await partIndexView.fillReplaceForm(data.replaceData, data.rowNumbers);
-//await partIndexView.confirmReplacement();
+await partIndexView.fillReplaceForm(data.replaceData, [1, 2]); // Assuming row numbers correspond to the selected range
+await partIndexView.confirmReplacement();Committable suggestion skipped: line range outside the PR's diff.
| } | ||
|
|
||
| public async confirmReplacement() { | ||
| await this.index.clickBulkButton('Peplace'); |
There was a problem hiding this comment.
Fix typo in button label.
There's a typo in the button label: 'Peplace' instead of 'Replace'. This could cause the test to fail if it's looking for the correct button.
public async confirmReplacement() {
- await this.index.clickBulkButton('Peplace');
+ await this.index.clickBulkButton('Replace');
await this.index.hasNotification('Parts have been replaced');
}📝 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.index.clickBulkButton('Peplace'); | |
| public async confirmReplacement() { | |
| - await this.index.clickBulkButton('Peplace'); | |
| + await this.index.clickBulkButton('Replace'); | |
| await this.index.hasNotification('Parts have been replaced'); | |
| } |
|
|
||
| public async selectPartsToReplace(start: number, end: number) { | ||
| await this.index.chooseRangeOfRowsOnTable(start, end); | ||
| await this.page.pause(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug pause from selectPartsToReplace method.
The pause() method is typically used for debugging and should be removed before merging to production code.
public async selectPartsToReplace(start: number, end: number) {
await this.index.chooseRangeOfRowsOnTable(start, end);
- await this.page.pause();
await this.index.clickBulkButton('Replace');
}📝 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.pause(); | |
| public async selectPartsToReplace(start: number, end: number) { | |
| await this.index.chooseRangeOfRowsOnTable(start, end); | |
| await this.index.clickBulkButton('Replace'); | |
| } |
| test('Ensure a part can be created and then deleted @hipanel-module-stock @manager', async ({ managerPage }) => { | ||
| const partView = new PartCreateView(managerPage); | ||
| await partView.navigate(); | ||
| await partView.fillPartFields(partData); | ||
| await partView.save(); | ||
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | ||
|
|
||
| await managerPage.click('text=Delete'); | ||
| managerPage.on('dialog', async dialog => await dialog.accept()); | ||
| await expect(managerPage.locator('text=Part has been deleted')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
The dialog handling setup should be before clicking the Delete button.
The current implementation sets up the dialog handler after clicking the delete button, which might lead to race conditions where the dialog appears before the handler is set up.
Move the dialog handler setup before clicking the delete button:
test('Ensure a part can be created and then deleted @hipanel-module-stock @manager', async ({ managerPage }) => {
const partView = new PartCreateView(managerPage);
await partView.navigate();
await partView.fillPartFields(partData);
await partView.save();
await expect(managerPage.locator('text=Part has been created')).toBeVisible();
+ managerPage.on('dialog', async dialog => await dialog.accept());
await managerPage.click('text=Delete');
- managerPage.on('dialog', async dialog => await dialog.accept());
await expect(managerPage.locator('text=Part has been deleted')).toBeVisible();
});📝 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.
| test('Ensure a part can be created and then deleted @hipanel-module-stock @manager', async ({ managerPage }) => { | |
| const partView = new PartCreateView(managerPage); | |
| await partView.navigate(); | |
| await partView.fillPartFields(partData); | |
| await partView.save(); | |
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | |
| await managerPage.click('text=Delete'); | |
| managerPage.on('dialog', async dialog => await dialog.accept()); | |
| await expect(managerPage.locator('text=Part has been deleted')).toBeVisible(); | |
| }); | |
| test('Ensure a part can be created and then deleted @hipanel-module-stock @manager', async ({ managerPage }) => { | |
| const partView = new PartCreateView(managerPage); | |
| await partView.navigate(); | |
| await partView.fillPartFields(partData); | |
| await partView.save(); | |
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | |
| managerPage.on('dialog', async dialog => await dialog.accept()); | |
| await managerPage.click('text=Delete'); | |
| await expect(managerPage.locator('text=Part has been deleted')).toBeVisible(); | |
| }); |
| test('Ensure multiple parts can be created @hipanel-module-stock @manager', async ({ managerPage }) => { | ||
| const partView = new PartCreateView(managerPage); | ||
| await partView.navigate(); | ||
| await partView.fillPartFields(partData); | ||
| await partView.addPart(); | ||
| await partView.save(); | ||
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Multiple parts creation test needs verification of both parts.
The test adds a second part form but doesn't verify that both parts are actually created.
Enhance the test to verify that both parts are created correctly:
test('Ensure multiple parts can be created @hipanel-module-stock @manager', async ({ managerPage }) => {
const partView = new PartCreateView(managerPage);
await partView.navigate();
+
+ // Create unique serial numbers for each part to verify later
+ const serial1 = `MG_TEST_PART1_${Date.now()}`;
+ const serial2 = `MG_TEST_PART2_${Date.now()}`;
+
+ // Fill the first part
+ const firstPartData = { ...partData, serials: serial1 };
+ await partView.fillPartFields(firstPartData);
- await partView.fillPartFields(partData);
await partView.addPart();
+
+ // Fill the second part
+ const secondPartData = { ...partData, serials: serial2 };
+ await partView.fillPartFields(secondPartData, 1); // Assuming second form is at index 1
+
await partView.save();
await expect(managerPage.locator('text=Part has been created')).toBeVisible();
+
+ // Verify both parts appear in the list
+ await expect(managerPage.locator(`text=${serial1}`)).toBeVisible();
+ await expect(managerPage.locator(`text=${serial2}`)).toBeVisible();
});📝 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.
| test('Ensure multiple parts can be created @hipanel-module-stock @manager', async ({ managerPage }) => { | |
| const partView = new PartCreateView(managerPage); | |
| await partView.navigate(); | |
| await partView.fillPartFields(partData); | |
| await partView.addPart(); | |
| await partView.save(); | |
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | |
| }); | |
| test('Ensure multiple parts can be created @hipanel-module-stock @manager', async ({ managerPage }) => { | |
| const partView = new PartCreateView(managerPage); | |
| await partView.navigate(); | |
| // Create unique serial numbers for each part to verify later | |
| const serial1 = `MG_TEST_PART1_${Date.now()}`; | |
| const serial2 = `MG_TEST_PART2_${Date.now()}`; | |
| // Fill the first part | |
| const firstPartData = { ...partData, serials: serial1 }; | |
| await partView.fillPartFields(firstPartData); | |
| await partView.addPart(); | |
| // Fill the second part | |
| const secondPartData = { ...partData, serials: serial2 }; | |
| await partView.fillPartFields(secondPartData, 1); // Assuming second form is at index 1 | |
| await partView.save(); | |
| await expect(managerPage.locator('text=Part has been created')).toBeVisible(); | |
| // Verify both parts appear in the list | |
| await expect(managerPage.locator(`text=${serial1}`)).toBeVisible(); | |
| await expect(managerPage.locator(`text=${serial2}`)).toBeVisible(); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/playwright/e2e/manager/part/part-create.spec.ts (2)
72-80: 🛠️ Refactor suggestionEnhance multiple parts creation test to verify both parts individually.
The current test doesn't verify that both parts are actually created with the expected data.
test('Ensure multiple parts can be created @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); await partView.navigate(); - await partView.fillPartFields(partData); + + // Create unique serial numbers for each part + const serial1 = UniqueId.generate(`MG_TEST_PART1`); + const serial2 = UniqueId.generate(`MG_TEST_PART2`); + + // Fill the first part + const firstPartData = createPartData({ serials: serial1 }); + await partView.fillPartFields(firstPartData); await partView.addPart(); - await partView.fillPartFields(partData, 1); + + // Fill the second part with different data + const secondPartData = createPartData({ serials: serial2 }); + await partView.fillPartFields(secondPartData, 1); + await partView.save(); await expect(managerPage.locator('text=Part has been created')).toBeVisible(); + + // Navigate to part list view to verify both parts exist + await managerPage.click('text=Parts'); + await expect(managerPage.locator(`text=${serial1}`)).toBeVisible(); + await expect(managerPage.locator(`text=${serial2}`)).toBeVisible(); });
89-90:⚠️ Potential issueFix the race condition in dialog handling.
The dialog handler is set up AFTER clicking the delete button, which could lead to a race condition where the dialog appears before the handler is set up, causing the test to hang.
await expect(managerPage.locator('text=Part has been created')).toBeVisible(); +managerPage.on('dialog', async dialog => await dialog.accept()); await managerPage.click('text=Delete'); -managerPage.on('dialog', async dialog => await dialog.accept()); await expect(managerPage.locator('text=Part has been deleted')).toBeVisible();
🧹 Nitpick comments (10)
tests/playwright/helper/PartForm.ts (3)
12-23: Consider using a typed interface instead ofanyfor partData.Using the
anytype bypasses TypeScript's type checking benefits. Consider defining a specific interface for the expected data structure.-public async fillPartFields(partData: any, index: number = 0) { +interface PartData { + partno: string; + src_id: string; + dst_id: string; + serials: string; + move_descr: string; + price: number | string; + currency: string; + company_id: string; +} + +public async fillPartFields(partData: PartData, index: number = 0) {
29-31: Clarify the fillSerials method signature.The method name suggests it accepts multiple serials, but it takes a single string parameter. Consider renaming or modifying the method to accept an array.
-public async fillSerials(serial: string, index: number = 0) { +public async fillSerial(serial: string, index: number = 0) { await this.page.fill(this.selector('input', 'serials', index), serial); } +public async fillSerials(serials: string[], index: number = 0) { + await this.page.fill(this.selector('input', 'serials', index), serials.join('\n')); +}
40-42: Consider using a more specific selector for the save button.The current text-based selector could be fragile if button text changes or if multiple buttons with the same text exist. Consider using a more specific selector.
public async save() { - await this.page.click('button:has-text("Save")'); + await this.page.click('button.btn-success:has-text("Save"), button[type="submit"]:has-text("Save")'); }tests/playwright/page/PartCreateView.ts (3)
13-15: Consider making the path configurable or using a relative URL.Hardcoded paths might break if the application is deployed to a different base URL. Consider making it configurable or using a relative URL.
public async navigate() { - await this.page.goto('/stock/part/create'); + await this.page.goto('./stock/part/create'); }
17-19: Consider propagating a typed interface instead ofany.This method inherits the
anytype issue from the PartForm class. Consider using a typed interface for better type safety.
25-35: Consider more robust selectors for UI interactions.The current selectors rely on specific HTML structure, particularly the "last-child" selector. This could be fragile if the HTML structure changes. Consider using more specific selectors or data attributes.
public async addPart() { - await this.page.click("div.item:last-child button.add-item"); + await this.page.click("div.item:not(.hidden):last-child button.add-item"); } public async removePart() { - await this.page.click("div.item:last-child button.remove-item"); + await this.page.click("div.item:not(.hidden):last-child button.remove-item"); } public async copyPart() { - await this.page.click("div.item:last-child button.copy"); + await this.page.click("div.item:not(.hidden):last-child button.copy"); }tests/playwright/e2e/manager/part/part-replace.spec.ts (1)
6-21: Consider moving test data to a separate factory or fixture.Extracting test data to a separate fixture or factory would improve maintainability and allow for easier reuse across tests.
+import { createPartReplaceTestData } from '@hipanel-module-stock/fixtures/partReplaceFixtures'; -const data = { - filters: [ - { - name: "move_descr_ilike", - value: "test description" - }, - { - name: "model_types", - value: "cpu" - }, - ], - replaceData: [ - { serialno: UniqueId.generate(`test`) }, - { serialno: UniqueId.generate(`test`) } - ], -}; +const data = createPartReplaceTestData();tests/playwright/e2e/manager/part/part-create.spec.ts (3)
6-15: Consider creating a test data factory for better test isolation.The current approach using a shared partData object could lead to test interference if multiple tests run in parallel or if tests modify the data object. A factory function would provide fresh data for each test.
-const partData = { - partno: 'CHASSIS EPYC 7402P', - src_id: 'TEST-DS-01', - dst_id: 'TEST-DS-02', - serials: UniqueId.generate(`MG_TEST_PART`), - move_descr: 'MG TEST MOVE', - price: 200, - currency: '$', - company_id: 'Other', -}; +function createPartData(customData = {}) { + return { + partno: 'CHASSIS EPYC 7402P', + src_id: 'TEST-DS-01', + dst_id: 'TEST-DS-02', + serials: UniqueId.generate(`MG_TEST_PART`), + move_descr: 'MG TEST MOVE', + price: 200, + currency: '$', + company_id: 'Other', + ...customData + }; +}
64-70: Consider enhancing verification of part creation.This test confirms the success message appears, but doesn't verify the part was created with the correct data. Add navigation to part details or list view to confirm the part exists with expected properties.
test('Ensure a part can be created @hipanel-module-stock @manager', async ({ managerPage }) => { const partView = new PartCreateView(managerPage); await partView.navigate(); - await partView.fillPartFields(partData); + const testData = createPartData(); + await partView.fillPartFields(testData); await partView.save(); await expect(managerPage.locator('text=Part has been created')).toBeVisible(); + + // Navigate to part list view to verify the part exists + await managerPage.click('text=Parts'); + await expect(managerPage.locator(`text=${testData.serials}`)).toBeVisible(); + await expect(managerPage.locator(`text=${testData.partno}`)).toBeVisible(); });
82-92: Consider adding cleanup for test data.This is the only test that cleans up after itself by deleting the created part. Consider adding similar cleanup steps to other tests to prevent test data accumulation.
For other tests, you could add an
afterEachhook that navigates to the parts list and deletes any parts created during tests:test.afterEach(async ({ managerPage }) => { // Navigate to parts list await managerPage.click('text=Parts'); // Look for parts with the test prefix in serials const testParts = await managerPage.locator('//tr[contains(., "MG_TEST_PART")]'); const count = await testParts.count(); if (count > 0) { // Select all test parts for (let i = 0; i < count; i++) { await testParts.nth(i).locator('input[type="checkbox"]').check(); } // Delete selected parts await managerPage.click('text=Delete'); managerPage.on('dialog', async dialog => await dialog.accept()); await expect(managerPage.locator('text=Part has been deleted')).toBeVisible(); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/playwright/e2e/manager/part/part-create.spec.ts(1 hunks)tests/playwright/e2e/manager/part/part-replace.spec.ts(1 hunks)tests/playwright/helper/PartForm.ts(1 hunks)tests/playwright/page/PartCreateView.ts(1 hunks)tests/playwright/page/PartIndexView.ts(1 hunks)tests/playwright/page/PartReplaceView.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/playwright/page/PartIndexView.ts
🔇 Additional comments (9)
tests/playwright/page/PartReplaceView.ts (3)
1-11: Clean class initialization and dependency management.The code properly imports the required dependencies and initializes the class with a Page object, following a good pattern of dependency injection and encapsulation.
13-19: Well-structured form filling logic with proper iteration.The method uses a clean approach to iterate through replace data and increment keys. This is a good pattern for handling form fields with dynamic indices.
21-23: Good delegation pattern for form submission.The class appropriately delegates the save functionality to the underlying PartForm component, promoting code reuse and maintainability.
tests/playwright/helper/PartForm.ts (2)
25-27: Well-defined reusable selector pattern.Good approach for creating consistent selectors based on element type, name, and index. This promotes maintainability and readability.
33-38: Good implementation with clear documentation.The comment clearly explains the inconsistency in field naming between different pages, which is valuable information for maintainers.
tests/playwright/page/PartCreateView.ts (2)
1-11: Clean class initialization and dependency management.The class properly imports dependencies and initializes with a Page object, following good patterns of dependency injection.
21-23: Good delegation pattern for form submission.The class appropriately delegates the save functionality to the underlying PartForm component, promoting code reuse and maintainability.
tests/playwright/e2e/manager/part/part-create.spec.ts (2)
18-42: LGTM! Effective testing of part management interface.The test thoroughly verifies UI controls for part management by checking the count of items after each operation, ensuring proper interaction with the form elements.
44-62: LGTM! Well-structured validation test.Good approach using a list of expected error messages and verifying each one. This makes the test maintainable as error messages can be easily updated.
| test.describe("Part Replacement", () => { | ||
| test("Ensure parts can be replaced @hipanel-module-stock @manager", async ({ managerPage }) => { | ||
| const partIndexView = new PartIndexView(managerPage); | ||
| const partReplaceView = new PartReplaceView(managerPage); | ||
|
|
||
| await partIndexView.navigate(); | ||
| await partIndexView.applyFilters(data.filters); | ||
| await partIndexView.selectPartsToReplace(1, data.replaceData.length); | ||
|
|
||
| await partReplaceView.fillReplaceForm(data.replaceData); | ||
| await partReplaceView.save(); | ||
|
|
||
| await partIndexView.confirmReplacement(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to verify the replacement succeeded.
The test confirms that the form can be submitted, but doesn't verify that the replacement was actually successful. Consider adding assertions to check the results.
await partReplaceView.save();
await partIndexView.confirmReplacement();
+
+// Add assertions to verify successful replacement
+await partIndexView.navigate();
+await partIndexView.applyFilters([
+ {
+ name: "serial_like",
+ value: data.replaceData[0].serialno
+ }
+]);
+await partIndexView.expectToSeePartWithSerial(data.replaceData[0].serialno);📝 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.
| test.describe("Part Replacement", () => { | |
| test("Ensure parts can be replaced @hipanel-module-stock @manager", async ({ managerPage }) => { | |
| const partIndexView = new PartIndexView(managerPage); | |
| const partReplaceView = new PartReplaceView(managerPage); | |
| await partIndexView.navigate(); | |
| await partIndexView.applyFilters(data.filters); | |
| await partIndexView.selectPartsToReplace(1, data.replaceData.length); | |
| await partReplaceView.fillReplaceForm(data.replaceData); | |
| await partReplaceView.save(); | |
| await partIndexView.confirmReplacement(); | |
| }); | |
| test.describe("Part Replacement", () => { | |
| test("Ensure parts can be replaced @hipanel-module-stock @manager", async ({ managerPage }) => { | |
| const partIndexView = new PartIndexView(managerPage); | |
| const partReplaceView = new PartReplaceView(managerPage); | |
| await partIndexView.navigate(); | |
| await partIndexView.applyFilters(data.filters); | |
| await partIndexView.selectPartsToReplace(1, data.replaceData.length); | |
| await partReplaceView.fillReplaceForm(data.replaceData); | |
| await partReplaceView.save(); | |
| await partIndexView.confirmReplacement(); | |
| // Add assertions to verify successful replacement | |
| await partIndexView.navigate(); | |
| await partIndexView.applyFilters([ | |
| { | |
| name: "serial_like", | |
| value: data.replaceData[0].serialno | |
| } | |
| ]); | |
| await partIndexView.expectToSeePartWithSerial(data.replaceData[0].serialno); | |
| }); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/playwright/page/PartIndexView.ts (3)
13-15: Consider using a configuration-based URL approach.The hardcoded URL path could be challenging to maintain if the application structure changes. Consider extracting this to a configuration variable or a constants file.
async navigate() { - await this.page.goto("/stock/part/index?representation=common"); + await this.page.goto(URLS.PART_INDEX_PAGE); }Where
URLSwould be imported from a constants file containing all application URLs.
43-45: Consider making this method private or enhancing its functionality.This method is a simple passthrough to the index object's method with the same name. Consider either making it private if it's only used internally, or enhancing it with additional functionality if it needs to be public.
47-60: Add flexibility to seePartWasCreated method.The method currently uses a hardcoded row number (1) which assumes the created part will always be the first row. This might be brittle if sorting changes or multiple parts are created.
-public async seePartWasCreated() { - const rowNumber = 1; +public async seePartWasCreated(rowNumber = 1) { await this.index.hasNotification('Part has been created'); await this.index.closeNotification(); // Ensure the current URL matches expected Move index URL await expect(this.page).toHaveURL(/\/stock\/move\/index\?MoveSearch%5Bid%5D=/); // Get first row move ID from the index table const moveId = await this.index.getRowDataKeyByNumber(rowNumber); expect(moveId).not.toBeNull(); await this.index.clickColumnOnTable('Parts', rowNumber); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/playwright/e2e/manager/part/part-create.spec.ts(1 hunks)tests/playwright/page/PartIndexView.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/playwright/e2e/manager/part/part-create.spec.ts
🔇 Additional comments (3)
tests/playwright/page/PartIndexView.ts (3)
1-11: Good implementation of the page object pattern.The class is well-structured with proper private properties and a constructor that accepts a Playwright Page object. This follows best practices for Page Object Model design.
17-23: Well-structured filter application method.This method elegantly handles applying multiple filters and submitting the search form.
25-28: Using dropdown bulk button is the correct approach.The method correctly implements selecting parts and triggering the replace action through the dropdown bulk button. The previous issue with the debug pause has been addressed.
| public async deleteItemOnTable(number: number) { | ||
| await this.chooseNumberRowOnTable(number); | ||
|
|
||
| await this.page.getByRole('button', { name: 'Delete' }).click(); | ||
|
|
||
| this.page.on('dialog', async dialog => await dialog.accept()); | ||
| await this.index.hasNotification('Part has been deleted'); | ||
| } |
There was a problem hiding this comment.
Fix event handler registration timing in deleteItemOnTable method.
The dialog event handler is registered after clicking the delete button, which could lead to race conditions. The event handler should be registered before triggering the action.
public async deleteItemOnTable(number: number) {
await this.chooseNumberRowOnTable(number);
+ this.page.on('dialog', async dialog => await dialog.accept());
await this.page.getByRole('button', { name: 'Delete' }).click();
-
- this.page.on('dialog', async dialog => await dialog.accept());
await this.index.hasNotification('Part has been deleted');
}📝 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.
| public async deleteItemOnTable(number: number) { | |
| await this.chooseNumberRowOnTable(number); | |
| await this.page.getByRole('button', { name: 'Delete' }).click(); | |
| this.page.on('dialog', async dialog => await dialog.accept()); | |
| await this.index.hasNotification('Part has been deleted'); | |
| } | |
| public async deleteItemOnTable(number: number) { | |
| await this.chooseNumberRowOnTable(number); | |
| this.page.on('dialog', async dialog => await dialog.accept()); | |
| await this.page.getByRole('button', { name: 'Delete' }).click(); | |
| await this.index.hasNotification('Part has been deleted'); | |
| } |
| public async confirmReplacement() { | ||
| await this.index.hasNotification('Part has been replaced'); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider handling single vs. multiple part replacements.
The notification message only checks for a singular form ("Part has been replaced"). If multiple parts are replaced, the message might use a plural form.
🏁 Script executed:
#!/bin/bash
# Description: Check if there are different notification messages for single vs multiple part replacements
# Look for notification strings in the codebase
echo "Searching for part replacement notification strings in the codebase..."
rg -i "part(s?)( have| has) been replaced" --type php --type js --type tsLength of output: 642
Action Required: Update Notification Handling for Multiple Part Replacements
After searching the codebase, it’s confirmed that all part replacement notifications currently use the singular message ("Part has been replaced")—across controllers, messages, and tests. If the application supports replacing multiple parts at once, please consider implementing conditional logic (or pluralized messages) in both the notification generation and corresponding tests. If single replacements are the only supported scenario, adding a clarifying comment regarding this design decision would be beneficial.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/playwright/page/PartIndexView.ts (1)
34-41:⚠️ Potential issueFix event handler registration timing in deleteItemOnTable method
The dialog event handler is registered after clicking the delete button, which leads to race conditions. The event handler should be registered before triggering the action.
public async deleteItemOnTable(number: number) { await this.chooseNumberRowOnTable(number); + this.page.on('dialog', async dialog => await dialog.accept()); await this.page.getByRole('button', { name: 'Delete' }).click(); - - this.page.on('dialog', async dialog => await dialog.accept()); await this.index.hasNotification('Part has been deleted'); }
🧹 Nitpick comments (7)
tests/playwright/page/PartIndexView.ts (4)
17-23: Consider adding error handling for filter applicationThe method handles filter application, but doesn't include any error handling if a filter fails to apply or if the search submission fails.
async applyFilters(filters: Array<{ name: string; value: string }>) { try { for (const filter of filters) { await this.index.setFilter(filter.name, filter.value); } await this.index.submitSearchButton(); + // Optional: wait for results to load + await this.page.waitForLoadState('networkidle'); + } catch (error) { + console.error(`Failed to apply filters: ${error}`); + throw new Error(`Failed to apply filters: ${error}`); + } }
43-45: Consider making the wrapper method private or adding functionalityThe
chooseNumberRowOnTablemethod is just a wrapper around the same method in the index class. Consider either:
- Making it private if it's only used internally
- Adding additional functionality to justify it being a public method
- Removing it and directly using index.chooseNumberRowOnTable where needed
47-61: Improve the seePartWasCreated method with better comments and assertionsThis method combines several actions (checking notifications, URL validation, extracting IDs, and navigation). Consider breaking it down and improving comments to better explain the workflow.
public async seePartWasCreated() { const rowNumber = 1; await this.index.hasNotification('Part has been created'); await this.index.closeNotification(); - // Ensure the current URL matches expected Move index URL + // After part creation, we're redirected to Move index page with the created move ID await expect(this.page).toHaveURL(/\/stock\/move\/index\?MoveSearch%5Bid%5D=/); - // Get first row move ID from the index table + // Extract the move ID from the first row to verify it exists const moveId = await this.index.getRowDataKeyByNumber(rowNumber); expect(moveId).not.toBeNull(); - // Wait /stock/part/view page to load + // Navigate to the Part details page by clicking the Parts column in the first row await this.index.clickColumnOnTable('Parts', rowNumber); + // Wait for the part view page to load completely + await this.page.waitForLoadState('networkidle'); }
1-62: Add consistent access modifiers and method return typesSome methods have explicit 'public' modifiers while others don't. Some methods like navigate() and applyFilters() should be made consistently public or private based on their usage patterns.
-async navigate() { +public async navigate(): Promise<void> { await this.page.goto("/stock/part/index?representation=common"); } -async applyFilters(filters: Array<{ name: string; value: string }>) { +public async applyFilters(filters: Array<{ name: string; value: string }>): Promise<void> { for (const filter of filters) { await this.index.setFilter(filter.name, filter.value); } await this.index.submitSearchButton(); }tests/playwright/page/PartView.ts (3)
17-20: Consider more flexible dialog handling.Currently, the dialog handler automatically accepts all dialogs. This might not be the desired behavior in all cases, especially for confirmation dialogs where users might want to cancel in some test scenarios.
Consider implementing a more flexible approach:
- private registerAcceptDeleteDialogHandler() { - // By default, dialogs are auto-dismissed by Playwright, so you don't have to handle them - this.page.on('dialog', async dialog => await dialog.accept()); - } + private registerDialogHandler(shouldAccept: boolean = true) { + // Configure dialog handling behavior + this.page.on('dialog', async dialog => { + shouldAccept ? await dialog.accept() : await dialog.dismiss(); + }); + }Then update the constructor to use this method:
public constructor(page: Page) { this.page = page; this.notification = new Notification(page); this.detailMenu = new DetailMenu(page); - this.registerAcceptDeleteDialogHandler(); + this.registerDialogHandler(true); // Default to accepting dialogs }
22-26: Add error handling and consistent formatting.The
deletePartmethod lacks error handling and has inconsistent formatting with the opening brace on a new line (unlike other methods).- public async deletePart() - { - await this.detailMenu.clickDetailMenuItem("Delete"); - await this.notification.hasNotification('Part has been deleted'); - } + public async deletePart(): Promise<boolean> { + try { + await this.detailMenu.clickDetailMenuItem("Delete"); + await this.notification.hasNotification('Part has been deleted'); + return true; + } catch (error) { + console.error('Failed to delete part:', error); + return false; + } + }This change adds:
- Consistent brace positioning
- Error handling with try/catch
- Return value to indicate success/failure
- Return type annotation for better type safety
1-27: Add JSDoc comments for better documentation.Consider adding JSDoc comments to improve code documentation and maintainability.
import {Page} from "@playwright/test"; import Notification from "@hipanel-core/helper/Notification"; import DetailMenu from "@hipanel-core/helper/DetailMenu"; +/** + * Page object representing the Part view page. + * Provides methods to interact with a part's detail view. + */ export default class PartView { private page: Page; private notification: Notification; private detailMenu: DetailMenu; + /** + * Creates a new PartView instance. + * @param page - The Playwright Page object + */ public constructor(page: Page) { this.page = page; this.notification = new Notification(page); this.detailMenu = new DetailMenu(page); this.registerAcceptDeleteDialogHandler(); } + /** + * Registers a handler to automatically accept all dialogs. + * @private + */ private registerAcceptDeleteDialogHandler() { // By default, dialogs are auto-dismissed by Playwright, so you don't have to handle them this.page.on('dialog', async dialog => await dialog.accept()); } + /** + * Deletes the currently displayed part. + * @returns Promise that resolves when the part is deleted and notification appears + */ public async deletePart() { await this.detailMenu.clickDetailMenuItem("Delete"); await this.notification.hasNotification('Part has been deleted'); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/views/part/view.php(1 hunks)tests/playwright/e2e/manager/part/part-create.spec.ts(1 hunks)tests/playwright/page/PartIndexView.ts(1 hunks)tests/playwright/page/PartView.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/views/part/view.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/playwright/e2e/manager/part/part-create.spec.ts
🔇 Additional comments (4)
tests/playwright/page/PartIndexView.ts (3)
1-15: Good implementation of the Page Object ModelThe class is well-structured and follows the Page Object Model design pattern, effectively encapsulating page interactions for the parts index view. The constructor properly initializes dependencies, and the navigate method includes a specific representation parameter.
25-28: LGTM on selectPartsToReplace implementationThe method correctly uses the index helper to select a range of rows and then clicks the appropriate bulk action button. Previous debug pause has been removed as suggested.
30-32: Update notification check to handle multiple part replacementsThe notification message only checks for a singular form ("Part has been replaced"). If multiple parts are replaced, the message might use a plural form.
#!/bin/bash # Search for plural notification messages for part replacements in the application echo "Searching for part replacement notification strings..." rg -i "parts? (has|have) been replaced" --type php --type tstests/playwright/page/PartView.ts (1)
5-16: LGTM! Well-structured class implementation.The class structure is clean with proper encapsulation and clear property initialization in the constructor.
| import {Page} from "@playwright/test"; | ||
| import Notification from "@hipanel-core/helper/Notification"; | ||
| import DetailMenu from "@hipanel-core/helper/DetailMenu"; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify imports consistency with other page objects.
Ensure that the import style is consistent with other page objects in the codebase.
🏁 Script executed:
#!/bin/bash
# Check imports in other page objects for consistency
echo "Checking import styles in other page object files:"
fd -e ts -p "tests/playwright/page/.*View\.ts" --exec head -n 5 | grep -v "PartView.ts"Length of output: 876
Action: Unify Import Formatting Across Page Objects
The file tests/playwright/page/PartView.ts currently uses the following imports:
import {Page} from "@playwright/test";
import Notification from "@hipanel-core/helper/Notification";
import DetailMenu from "@hipanel-core/helper/DetailMenu";A quick survey of similar page object files shows variations in style:
- PartIndexView.ts:
import {expect, Page} from "@playwright/test"; import Index from "@hipanel-core/page/Index";
- PartReplaceView.ts:
import { Page } from "@playwright/test"; import PartForm from "@hipanel-module-stock/helper/PartForm";
- PartCreateView.ts:
import {Page}from "@playwright/test"; import PartForm from "@hipanel-module-stock/helper/PartForm";
While the imports in PartView.ts are functionally correct, there is inconsistency in spacing (inside curly braces and before the from keyword) across the page objects. Please update these statements to a unified, project-wide format to maintain consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/playwright/page/PartIndexView.ts (1)
42-49:⚠️ Potential issueFix event handler registration timing in deleteItemOnTable method.
The dialog event handler is registered after clicking the delete button, which could lead to race conditions. The event handler should be registered before triggering the action.
public async deleteItemOnTable(number: number) { await this.chooseNumberRowOnTable(number); + this.page.on('dialog', async dialog => await dialog.accept()); await this.page.getByRole('button', { name: 'Delete' }).click(); - this.page.on('dialog', async dialog => await dialog.accept()); await this.index.hasNotification('Part has been deleted'); }
🧹 Nitpick comments (11)
tests/playwright/page/PartIndexView.ts (8)
13-15: Consider adding a return type to the navigate method.While TypeScript can infer the return type, it's good practice to explicitly define return types for public methods to improve code clarity and maintainability.
- public async navigate() { + public async navigate(): Promise<void> { await this.page.goto("/stock/part/index?representation=common"); }
17-23: Consider adding explicit return type and improving error handling.Adding return types enhances code clarity, and error handling would make this method more robust.
- public async applyFilters(filters: Array<{ name: string; value: string }>) { + public async applyFilters(filters: Array<{ name: string; value: string }>): Promise<void> { for (const filter of filters) { + try { await this.index.setFilter(filter.name, filter.value); + } catch (error) { + console.error(`Failed to set filter ${filter.name}:`, error); + throw error; + } } await this.index.submitSearchButton(); }
25-27: Add explicit return type to filterBySerial method.For consistency and better type safety.
- public async filterBySerial(serial: string) { + public async filterBySerial(serial: string): Promise<void> { await this.index.applyFilter('serial_ilike', serial); }
29-32: Update method parameter description and add explicit return type.The method name 'selectPartsToReplace' suggests selecting specific parts, but the implementation selects a range from 1 to count. Consider more descriptive parameter naming or documentation.
- public async selectPartsToReplace(count: number) { + public async selectPartsToReplace(count: number): Promise<void> { + // Selects the first 'count' rows in the table for replacement await this.selectRows(count); await this.index.clickDropdownBulkButton('Bulk actions', 'Replace'); }
34-36: Add explicit return type to selectRows method.For consistency with other methods.
- public async selectRows(count: number) { + public async selectRows(count: number): Promise<void> { await this.index.chooseRangeOfRowsOnTable(1, count); }
51-53: Add explicit return type to chooseNumberRowOnTable method.For consistency and better type safety.
- public async chooseNumberRowOnTable(number: number) { + public async chooseNumberRowOnTable(number: number): Promise<void> { await this.index.chooseNumberRowOnTable(number); }
55-69: Consider decomposing seePartWasCreated into smaller methods and add return type.This method performs multiple actions and could be broken down for better maintainability. Also, adding return types improves type safety.
- public async seePartWasCreated() { + public async seePartWasCreated(): Promise<void> { const rowNumber = 1; await this.index.hasNotification('Part has been created'); await this.index.closeNotification(); // Ensure the current URL matches expected Move index URL await expect(this.page).toHaveURL(/\/stock\/move\/index\?MoveSearch%5Bid%5D=/); // Get first row move ID from the index table const moveId = await this.index.getRowDataKeyByNumber(rowNumber); expect(moveId).not.toBeNull(); // Wait /stock/part/view page to load await this.index.clickColumnOnTable('Parts', rowNumber); }Consider refactoring this into smaller methods like:
verifyPartCreationNotification()verifyMoveIndexUrl()verifyMoveIdAndNavigateToPartView(rowNumber)
71-73: Add explicit return type to openSellModal method.For consistency and better type safety.
- public async openSellModal() { + public async openSellModal(): Promise<void> { await this.index.clickDropdownBulkButton('Sell parts', 'Sell parts'); }tests/playwright/page/BillIndexView.ts (3)
13-15: Add explicit return type to navigate method.For consistency and better type safety.
- public async navigate() { + public async navigate(): Promise<void> { await this.page.goto('/finance/bill/index'); }
17-28: Add explicit return type and consider adding error handling and timeout.For a more robust implementation, consider adding error handling and possibly a timeout for the page loading.
- public async ensureSellingBillWasCreated(sellData: { descr: string; prices: number[] }) { + public async ensureSellingBillWasCreated(sellData: { descr: string; prices: number[] }): Promise<void> { + try { await this.navigate(); // Apply filter for the description await this.index.applyFilter('descr', sellData.descr); // Open the row menu of the first bill and select "View" await this.index.clickPopoverMenu(1, 'View'); // Ensure the correct number of parts were sold await expect(this.page.locator('tr table tr[data-key]')).toHaveCount(sellData.prices.length); + } catch (error) { + console.error('Failed to verify selling bill creation:', error); + throw error; + } }
17-28: Consider adding a more descriptive comment for the billing verification.The current comment "Ensure the correct number of parts were sold" could be enhanced to better explain the assertion logic.
public async ensureSellingBillWasCreated(sellData: { descr: string; prices: number[] }) { await this.navigate(); // Apply filter for the description await this.index.applyFilter('descr', sellData.descr); // Open the row menu of the first bill and select "View" await this.index.clickPopoverMenu(1, 'View'); - // Ensure the correct number of parts were sold + // Verify that the number of rows in the bill details table matches the number of prices in the sell data, + // confirming that all parts were properly included in the bill await expect(this.page.locator('tr table tr[data-key]')).toHaveCount(sellData.prices.length); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/playwright/e2e/manager/part/part-sell.spec.ts(1 hunks)tests/playwright/page/BillIndexView.ts(1 hunks)tests/playwright/page/PartIndexView.ts(1 hunks)tests/playwright/page/SellModal.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/playwright/e2e/manager/part/part-sell.spec.ts
- tests/playwright/page/SellModal.ts
🔇 Additional comments (3)
tests/playwright/page/PartIndexView.ts (2)
4-11: Class structure looks good!The class is well-structured with proper private fields and a clean constructor. Good practice using TypeScript's access modifiers.
38-40: Update notification message to handle plural form.The notification message only checks for a singular form ("Part has been replaced"). If multiple parts can be replaced, consider handling both singular and plural forms.
#!/bin/bash # Check for different notification messages for part replacements echo "Searching for part replacement notification message in the codebase..." rg -i "part(s?)( have| has) been replaced" --type php --type ts --type jstests/playwright/page/BillIndexView.ts (1)
4-11: Class structure looks good!The class is well-structured with proper private fields and a clean constructor. Good practice using TypeScript's access modifiers.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/playwright/page/SellModal.ts (2)
34-36: Consider adding a wait for navigation or state change after confirming the saleAfter clicking the sell button, the page likely changes state or navigates. It's a good practice to wait for this state change to avoid potential race conditions in tests.
public async confirmSale() { await this.modalLocator().locator(`button:text('Sell')`).click(); + // Wait for the modal to close or for navigation/state change + await this.page.waitForSelector("div.modal-body[data-action-url$=sell]", { state: "hidden" }); }
1-41: Add JSDoc comments to improve code documentationAdding JSDoc comments to methods will improve code documentation and provide better IDE hints for users of this class.
Here's an example for the
fillSellWindowFieldsmethod:+ /** + * Fills in the sell modal form with the provided data + * @param sellData - The data to fill in the form + * @param sellData.contact_id - The contact ID to select + * @param sellData.time - The time value to input + * @param sellData.descr - The description text + * @param sellData.currency - The currency option to select + * @param sellData.prices - Array of price values to input in respective fields + */ public async fillSellWindowFields(sellData: SellData) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/playwright/page/SellModal.ts(1 hunks)
🔇 Additional comments (5)
tests/playwright/page/SellModal.ts (5)
1-4: Appropriate imports for the Playwright page objectThe imports look good, properly bringing in Playwright components and custom helper classes.
5-12: Well-structured class with appropriate constructor initializationThe class follows good OOP principles with proper encapsulation and initialization.
30-32: Good encapsulation of the modal selectorExtracting the modal selector into a private method is a good practice for maintainability.
38-40: Good validation of successful operationChecking for a success notification is a good way to verify the operation completed successfully.
1-41:✅ Verification successful
Verify integration with the test that uses this class
Since this is a new page object, it would be good to verify that it's used correctly in the test and that all the selectors work as expected.
🏁 Script executed:
#!/bin/bash # Find and examine the tests that use SellModal echo "Searching for tests that import SellModal..." rg -l "import.*SellModal" --type ts tests/playwright # Look for the part-sell.spec.ts file mentioned in the AI summary echo "Looking for part-sell.spec.ts..." if [ -f "tests/playwright/e2e/manager/part/part-sell.spec.ts" ]; then echo "Found part-sell.spec.ts" cat "tests/playwright/e2e/manager/part/part-sell.spec.ts" else echo "part-sell.spec.ts not found" fiLength of output: 1710
Integration Verified: SellModal is Correctly Utilized in Tests
The integration with the test (specifically in
tests/playwright/e2e/manager/part/part-sell.spec.ts) has been verified. TheSellModalclass is being imported and used as expected, and all selectors (such as those infillSellWindowFieldsandmodalLocator) align correctly with the test's workflow.No changes are required.
tests/playwright/page/SellModal.ts
Outdated
| public async fillSellWindowFields(sellData) { | ||
| const modal = this.modalLocator(); | ||
|
|
||
| await Select2.field(this.page, 'select[id$=contact_id]').setValue(sellData.contact_id); | ||
|
|
||
| await modal.locator("input[name*=time]").fill(sellData.time); | ||
| await modal.locator("textarea[id$=description]").fill(sellData.descr); | ||
| await modal.locator("select[id$=currency]").selectOption(sellData.currency); | ||
|
|
||
| const priceFields = await modal.locator("div[class$=sell] input[type=text][id^=partsell]") | ||
| .all(); | ||
| for (let i = 0; i < sellData.prices.length; i++) { | ||
| await priceFields[i].fill(sellData.prices[i].toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript interface for the sellData parameter
The sellData parameter lacks type information, which reduces type safety and makes it harder for other developers to understand what's expected.
+ interface SellData {
+ contact_id: string;
+ time: string;
+ descr: string;
+ currency: string;
+ prices: number[];
+ }
- public async fillSellWindowFields(sellData) {
+ public async fillSellWindowFields(sellData: SellData) {Additionally, consider adding error handling to check if there are enough price fields for the provided prices array:
const priceFields = await modal.locator("div[class$=sell] input[type=text][id^=partsell]")
.all();
+ if (priceFields.length < sellData.prices.length) {
+ throw new Error(`Expected at least ${sellData.prices.length} price fields but found ${priceFields.length}`);
+ }
for (let i = 0; i < sellData.prices.length; i++) {
await priceFields[i].fill(sellData.prices[i].toString());
}📝 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.
| public async fillSellWindowFields(sellData) { | |
| const modal = this.modalLocator(); | |
| await Select2.field(this.page, 'select[id$=contact_id]').setValue(sellData.contact_id); | |
| await modal.locator("input[name*=time]").fill(sellData.time); | |
| await modal.locator("textarea[id$=description]").fill(sellData.descr); | |
| await modal.locator("select[id$=currency]").selectOption(sellData.currency); | |
| const priceFields = await modal.locator("div[class$=sell] input[type=text][id^=partsell]") | |
| .all(); | |
| for (let i = 0; i < sellData.prices.length; i++) { | |
| await priceFields[i].fill(sellData.prices[i].toString()); | |
| } | |
| } | |
| interface SellData { | |
| contact_id: string; | |
| time: string; | |
| descr: string; | |
| currency: string; | |
| prices: number[]; | |
| } | |
| public async fillSellWindowFields(sellData: SellData) { | |
| const modal = this.modalLocator(); | |
| await Select2.field(this.page, 'select[id$=contact_id]').setValue(sellData.contact_id); | |
| await modal.locator("input[name*=time]").fill(sellData.time); | |
| await modal.locator("textarea[id$=description]").fill(sellData.descr); | |
| await modal.locator("select[id$=currency]").selectOption(sellData.currency); | |
| const priceFields = await modal.locator("div[class$=sell] input[type=text][id^=partsell]") | |
| .all(); | |
| if (priceFields.length < sellData.prices.length) { | |
| throw new Error(`Expected at least ${sellData.prices.length} price fields but found ${priceFields.length}`); | |
| } | |
| for (let i = 0; i < sellData.prices.length; i++) { | |
| await priceFields[i].fill(sellData.prices[i].toString()); | |
| } | |
| } |
- PartCreateCest.php - PartReplaceCest.php - PartSellCest.php
Summary by CodeRabbit
New Features
Bug Fixes
Tests