Update RFQ tests for new changes #2394
Conversation
WalkthroughThe changes in this pull request involve significant modifications across multiple classes and test files. Key updates include enhancements to navigation methods in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (16)
tests/pw/pages/privacyPolicyPage.ts (1)
32-32: Approve refactoring with a minor suggestion.The refactoring of
goToPrivacyPolicymethod improves code readability and maintainability by introducing theforceLinkToSameTabmethod. This change aligns well with the DRY principle and encapsulates the functionality of ensuring the link opens in the same tab.Consider adding a brief comment explaining why forcing the link to open in the same tab is necessary for this specific use case. This would provide context for future developers who might work on this code.
async goToPrivacyPolicy(storeName: string) { await this.gotoSingleStore(storeName); + // Force the privacy policy link to open in the same tab to ensure consistent navigation in tests await this.forceLinkToSameTab(singleStoreCustomer.storeContactForm.privacyPolicyLink); await this.clickAndWaitForUrl(helpers.stringToRegex('privacy-policy'), singleStoreCustomer.storeContactForm.privacyPolicyLink); }tests/pw/tests/e2e/requestForQuoteRules.spec.ts (4)
19-19: Create an issue to track the pending API update.The TODO comment indicates that there's a pending fix related to the API update for the
createQuoteRulemethod. To ensure this doesn't get overlooked, it would be beneficial to create an issue to track this task.Would you like me to create a GitHub issue to track this TODO item?
36-37: Approve the enhancement with a minor suggestion.Great improvement! Including a product in the quote rule makes the test more realistic and comprehensive.
To further enhance readability and maintainability, consider extracting the product creation logic into a separate helper method within the
ApiUtilsclass or a dedicated setup function.Here's a suggested refactor:
// In ApiUtils or a new helper file async function createProductForTest() { const [, , productName] = await apiUtils.createProduct(payloads.createProduct(), payloads.vendorAuth); return productName; } // In the test const productName = await createProductForTest(); await admin.addQuoteRule({ ...data.requestForQuotation.quoteRule(), includeProducts: productName });
41-41: Approve the change and suggest an additional test case.Good addition! Including the
specificProductsflag in the edit operation makes the test more comprehensive.To ensure full coverage of this feature, consider adding another test case where
specificProductsis set totrue. This will validate both scenarios of the flag.Here's a suggested additional test case:
test('admin can edit quote rule with specific products', { tag: ['@pro', '@admin'] }, async () => { await admin.editQuoteRule({ ...data.requestForQuotation.quoteRule(), title: quoteRuleTitle, specificProducts: true }); });
Line range hint
45-46: Approve the changes and suggest a potential optimization.Excellent improvements! Creating a new quote rule for each test ensures proper isolation and reduces inter-test dependencies. This approach makes the tests more robust and reliable.
To potentially optimize these tests and reduce duplication, consider creating a helper function that encapsulates the quote rule creation and action execution. This could make the tests more concise and easier to maintain.
Here's a suggested helper function:
async function testQuoteRuleAction(action: 'trash' | 'restore' | 'permanently-delete', initialStatus: 'publish' | 'trash' = 'publish') { const [, , quoteRuleTitle] = await apiUtils.createQuoteRule({ ...payloads.createQuoteRule(), status: initialStatus }, payloads.adminAuth); await admin.updateQuoteRule(quoteRuleTitle, action); } // Usage in tests: test('admin can trash quote rule', { tag: ['@pro', '@admin'] }, async () => { await testQuoteRuleAction('trash'); }); test('admin can restore quote rule', { tag: ['@pro', '@admin'] }, async () => { await testQuoteRuleAction('restore', 'trash'); }); test('admin can permanently delete quote rule', { tag: ['@pro', '@admin'] }, async () => { await testQuoteRuleAction('permanently-delete', 'trash'); });This approach would make the tests more DRY and easier to maintain.
Also applies to: 50-51, 55-56
tests/pw/tests/api/quoteRequests.spec.ts (1)
Skipped Test Reduces Coverage for Batch Update Quotes
The test for updating batch request quotes has been skipped without a clear justification. This reduces test coverage for a critical functionality and may lead to potential regression issues.
- Actions to Consider:
- Re-enable the Test: If the test was skipped accidentally or the underlying issue has been resolved, consider re-enabling it to ensure continued coverage.
- Document the Reason: If skipping the test is necessary (e.g., due to known issues), please add a comment explaining the reason and track it with a corresponding issue or ticket.
- Alternative Solutions: If the test is problematic, explore alternatives such as optimizing test performance or restructuring the test to avoid current issues.
🔗 Analysis chain
Line range hint
81-87: Clarify the reason for skipping the batch update testThe test for updating batch request quotes has been skipped. This reduces test coverage for an important functionality and may lead to potential regression issues.
Could you please explain why this test is being skipped? Is it due to a failing test, ongoing refactoring, or performance issues?
If the test is problematic, consider these alternatives:
- If it's failing, fix the underlying issue rather than skipping the test.
- If it's a performance concern, consider moving it to a separate, less frequently run test suite.
- If the functionality is changing, update the test to match the new expected behavior.
If skipping is temporary, please add a comment explaining the reason and create a ticket to track its re-enablement.
To help understand the context, let's check for any related changes or issues:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for recent changes to the batch update functionality rg -i "batch.*update.*quote" --type ts # Look for any TODO or FIXME comments related to quote requests rg -i "TODO|FIXME" --type ts | rg -i "quote" # Check for any open issues related to quote requests gh issue list --label "quote-requests" --state openLength of output: 1200
tests/pw/pages/singleStorePage.ts (1)
98-100: Approved: Good refactoring of link handling logicThe changes to the
storeSharemethod improve code readability and potentially reusability by introducing theforceLinkToSameTabmethod. This aligns well with the DRY (Don't Repeat Yourself) principle mentioned in the PR objectives.Consider adding a brief comment explaining the purpose of
forceLinkToSameTabfor better documentation:// Ensure the share link opens in the same tab await this.forceLinkToSameTab(singleStoreCustomer.sharePlatForms[site as keyof typeof singleStoreCustomer.sharePlatForms]);tests/pw/tests/e2e/catalogmode.spec.ts (1)
Line range hint
1-124: Overall assessment of changesThe modifications in this file are focused on improving the 'vendor can enable RFQ in catalog mode' test case. The changes simplify product creation and add a workaround for an API limitation in quote rule creation. While these changes enhance the test's functionality and readability, they also highlight an underlying issue that needs to be addressed.
Recommendations:
- Create a tracking issue for the API fix mentioned in the TODO comment.
- Once the API is fixed, remove the workaround and update this test accordingly.
- Review other test files to ensure consistency in product creation and quote rule handling across the test suite.
- Consider adding a comment explaining why the quote rule content needs to be updated after creation, to provide context for future maintainers.
To improve the overall architecture and maintainability of the test suite, consider the following:
- If the
product_switchsetting is consistently required, update thecreateQuoteRulepayload to include this by default.- Evaluate if a helper function for creating and setting up quote rules would be beneficial across multiple test files.
- Document the expected behavior of the API regarding quote rule creation and updates in a central location for all developers to reference.
tests/pw/tests/e2e/requestForQuotes.spec.ts (1)
Line range hint
1-189: Overall improvements with some areas needing attention.The changes in this file consistently replace
quoteTitlewithquoteId, which improves clarity and accuracy throughout the tests. This is a positive change that aligns with best practices for handling unique identifiers.However, there are a few areas that require attention:
Two tests have been skipped (lines 44 and 165). The reasons for skipping these tests should be documented and addressed in future updates.
New quote rule functionality has been added (lines 141-142), which includes direct database manipulation. While this expands test coverage, it introduces a dependency on
dbUtilsthat may need to be removed once the API is fixed.Consider the following recommendations:
- Document the reasons for skipped tests in code comments or link to relevant issues.
- Create a tracking issue for removing the direct database manipulation once the API is fixed.
- Review the overall test structure to ensure it remains maintainable with these new additions.
tests/pw/pages/basePage.ts (2)
Line range hint
94-107: LGTM! Consider adding error handling.The changes to the
goIfNotTheremethod look good. The addition of theforceparameter provides more flexibility in navigation scenarios.Consider adding error handling for the navigation and reload operations. For example:
async goIfNotThere(subPath: string, waitUntil: 'load' | 'domcontentloaded' | 'networkidle' | 'commit' = 'domcontentloaded', force = false): Promise<void> { const alreadyThere = this.isCurrentUrl(subPath); if (!alreadyThere) { const url = this.createUrl(subPath); await this.toPass(async () => { - await this.goto(url, { waitUntil: waitUntil }); + try { + await this.goto(url, { waitUntil: waitUntil }); + } catch (error) { + console.error(`Navigation failed: ${error}`); + throw error; + } const currentUrl = this.getCurrentUrl(); expect(currentUrl).toMatch(subPath); }); } if (force) { - await this.reload(); + try { + await this.reload(); + } catch (error) { + console.error(`Force reload failed: ${error}`); + throw error; + } } }
1621-1626: LGTM! Consider adding error handling.The new
forceLinkToSameTabmethod is a good addition to theBasePageclass. It effectively ensures that a link opens in the same tab instead of a new one.Consider adding error handling to make the method more robust. For example:
async forceLinkToSameTab(selector: string): Promise<void> { - // ensure link suppose to open on new tab - await this.toHaveAttribute(selector, 'target', '_blank'); - // force link to open on the same tab - await this.setAttributeValue(selector, 'target', '_self'); + try { + // ensure link is supposed to open in a new tab + await this.toHaveAttribute(selector, 'target', '_blank'); + // force link to open on the same tab + await this.setAttributeValue(selector, 'target', '_self'); + } catch (error) { + console.error(`Failed to force link to same tab: ${error}`); + throw error; + } }This change will help catch and report any errors that might occur during the attribute checks or modifications.
tests/pw/utils/payloads.ts (1)
1696-1696: LGTM! Update documentation if necessary.The change of the billing email domain from yopmail.com to email.com in the
createOrderCodpayload is consistent with the previous modification. This systematic update improves the consistency of test data across different order types.Consider updating any relevant documentation or comments that may reference the old email domain to reflect this change.
tests/pw/pages/requestForQuotationsPage.ts (2)
111-115: Address the TODO: Implement 'expireLimit' functionalityThe comment
// todo: skipped until api is fixedindicates that theexpireLimitfeature is currently not implemented due to an API issue. This may leave the quote rule functionality incomplete.Would you like assistance in implementing the
expireLimitfeature or opening a GitHub issue to track this task?
262-262: Address the TODO: Implement direct navigation to quotesThe comment
// todo: add goto quote via direct urlindicates the need to navigate directly to a quote using its ID.Would you like assistance in implementing direct URL navigation to the quote, enhancing the efficiency of the
editQuotemethod?tests/pw/utils/interfaces.ts (2)
1292-1292: Add missing line number annotation for 'hidePriceText'The
hidePriceTextproperty on line 1293 is missing a line number annotation. Ensure that all changed lines are properly annotated with line numbers and tildes for clarity and consistency.
1375-1383: Ensure consistent naming conventions in 'address' interfaceIn the
guestaddress interface, some property names could be standardized:
countrySelectValueandstateSelectValue: If these represent country and state codes, consider renaming them tocountryCodeandstateCodefor clarity and consistency.
postCodeis used here, whilezipCodeis used elsewhere in the codebase. To maintain consistency, it's advisable to use one term throughout. Choose eitherpostCodeorzipCodebased on the project's standard.Apply the following changes for consistency:
- countrySelectValue: string; + countryCode: string; - stateSelectValue: string; + stateCode: string; - postCode: string; + zipCode: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- tests/pw/pages/basePage.ts (3 hunks)
- tests/pw/pages/privacyPolicyPage.ts (1 hunks)
- tests/pw/pages/requestForQuotationsPage.ts (11 hunks)
- tests/pw/pages/selectors.ts (10 hunks)
- tests/pw/pages/singleStorePage.ts (1 hunks)
- tests/pw/pages/vendorPage.ts (1 hunks)
- tests/pw/pages/vendorVerificationsPage.ts (2 hunks)
- tests/pw/tests/api/quoteRequests.spec.ts (1 hunks)
- tests/pw/tests/e2e/catalogmode.spec.ts (1 hunks)
- tests/pw/tests/e2e/requestForQuoteRules.spec.ts (2 hunks)
- tests/pw/tests/e2e/requestForQuotes.spec.ts (9 hunks)
- tests/pw/tests/e2e/vendorStaff.spec.ts (2 hunks)
- tests/pw/utils/dbUtils.ts (1 hunks)
- tests/pw/utils/interfaces.ts (3 hunks)
- tests/pw/utils/payloads.ts (18 hunks)
- tests/pw/utils/schemas.ts (1 hunks)
- tests/pw/utils/testData.ts (5 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/pw/pages/vendorPage.ts
- tests/pw/pages/vendorVerificationsPage.ts
🧰 Additional context used
🔇 Additional comments (23)
tests/pw/tests/e2e/requestForQuoteRules.spec.ts (1)
Line range hint
1-68: Overall assessment: Improvements align well with PR objectives.The changes in this file significantly enhance the test suite for Request for Quotation Rules. They improve test isolation, increase coverage of new features (like
specificProducts), and make the tests more robust by ensuring each test operates on a valid quote rule instance.These modifications align well with the PR objectives of updating RFQ tests for new changes. They also adhere to good testing practices, making the test suite more reliable and maintainable.
A few suggestions have been made to further improve code organization and test coverage, but overall, these changes represent a solid improvement to the test suite.
tests/pw/tests/e2e/vendorStaff.spec.ts (2)
7-7: Improved test suite description clarity.The updated test suite description 'Vendor staff test (vendor)' provides better context by explicitly stating that these tests are from the vendor's perspective. This change enhances the readability and understanding of the test suite's purpose, which is beneficial for maintainability.
48-48: Significant improvement in test suite description accuracy.The updated test suite description 'Vendor staff test (vendorStaff)' now accurately reflects the content of the tests, which focus on vendor staff functionality. This change greatly enhances the clarity and maintainability of the test suite.
However, given the significant shift in perspective from the previous 'Wholesale test (customer)' description, it's important to verify that no customer-specific tests were inadvertently removed or altered during this update.
Could you please confirm that:
- No customer-specific tests were removed during this update?
- If any tests were removed, were they replaced with equivalent vendor staff tests?
- Does this change align with any recent updates to the RFQ functionality that might have shifted focus from customer to vendor staff perspectives?
To assist in verification, you can run the following script to check for any removed test cases:
✅ Verification successful
No customer-specific tests were removed.
The update to the test suite description has been verified, ensuring that no customer-specific tests were inadvertently removed or altered during this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for removed test cases related to customer perspective # Test: Search for removed test cases mentioning 'customer' git diff HEAD~1 tests/pw/tests/e2e/vendorStaff.spec.ts | grep -E '^\-.*test\(.*customer' || echo "No customer-specific tests were removed."Length of output: 181
tests/pw/pages/singleStorePage.ts (1)
98-100: Verify the implementation offorceLinkToSameTabThe
forceLinkToSameTabmethod is used here but not defined in this file. To ensure the correct functionality:
- Verify that
forceLinkToSameTabis properly implemented in the parentCustomerPageclass or a relevant utility file.- Confirm that it correctly sets the link's target attribute to '_self' or uses an equivalent method to ensure the link opens in the same tab.
✅ Verification successful
forceLinkToSameTabis properly implemented inbasePage.ts.
- Implementation Location:
tests/pw/pages/basePage.ts- Functionality:
- Sets the
targetattribute of links to_selfto ensure they open in the same tab.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of forceLinkToSameTab rg --type typescript -A 5 'forceLinkToSameTab'Length of output: 81
Script:
#!/bin/bash # Search for the implementation of forceLinkToSameTab with corrected file type rg --type ts -A 5 'forceLinkToSameTab'Length of output: 4199
Script:
#!/bin/bash # Alternative search using ast-grep for more precise results ast-grep --lang typescript --pattern 'function forceLinkToSameTab($_)'Length of output: 72
tests/pw/tests/e2e/requestForQuotes.spec.ts (9)
6-6: LGTM: New import for database utilities.The addition of the
dbUtilsimport is appropriate as it's used later in the file for updating quote rule content.
15-15: LGTM: Variable renamed for consistency.Changing
quoteTitletoquoteIdis a good practice as it more accurately represents the unique identifier for quotes. This change improves clarity and consistency throughout the tests.
24-24: LGTM: API call updated to use quote ID.The quote creation API call has been correctly updated to use
quoteId, maintaining consistency with the earlier variable renaming. This change ensures that the unique identifier for the quote is properly captured and used in subsequent tests.
44-45: Verify the reason for skipping the 'admin can edit quote' test.The test for editing a quote has been skipped. Please provide the reason for skipping this test and ensure it's addressed in future updates.
Can you explain why this test has been skipped? Is there an ongoing issue or planned changes that necessitate this?
49-50: LGTM: Consistent use ofquoteIdin admin tests.The changes in these lines consistently use
quoteIdinstead ofquoteTitlein API calls and method arguments. This improves the overall consistency and clarity of the tests.Also applies to: 54-55, 59-60, 64-65, 69-70
93-93: LGTM: Consistent use ofquoteIdin vendor tests.The vendor test suite has been updated to use
quoteIdinstead ofquoteTitle, maintaining consistency with the changes made in the admin suite. This improves the clarity and accuracy of the tests.Also applies to: 106-106
141-142: Verify the need for direct database manipulation.A new quote rule is being created and its content updated using
dbUtils. The comment suggests this is a temporary solution until an API fix is implemented.Is there an open issue or planned work to address the API limitation that necessitates this direct database manipulation? Please ensure this is tracked and removed once the API is fixed.
143-143: LGTM: Consistent use ofquoteIdin customer tests.The quote request creation now correctly uses
quoteId, maintaining consistency with the changes made in other test suites.
165-165: Verify the reason for skipping the 'customer can update quote request' test.The test for updating a quote request has been skipped. This could indicate ongoing changes or issues with this functionality.
Can you provide the reason for skipping this test? Is there an open issue or planned work to address any problems with the customer's ability to update quote requests?
tests/pw/utils/testData.ts (4)
1597-1597: Improved email address configurationThe changes to the email addresses in the
customerobject are beneficial:
- They now use the
CUSTOMERvariable, making them consistent with other parts of the codebase.- This approach allows for easier configuration and maintenance of email addresses.
Also applies to: 1601-1601
1805-1813: Added guest address informationA new
addressobject has been added to theguestfunction:
- It includes fields for
country,countrySelectValue,stateSelectValue,city,postCode,addressLine1, andaddressLine2.- This addition allows for more detailed address information collection for guest users.
This change enhances the ability to gather and manage guest user data, potentially improving shipping calculations and user experience for guest checkouts. Ensure that any forms or processes handling guest information are updated to accommodate these new fields.
To ensure proper implementation, please run the following script to check for any guest-related forms or processes that might need updating:
#!/bin/bash # Search for guest-related forms or processes rg --type typescript "guest.*address|country|city|postCode" -g '!tests/pw/utils/testData.ts'
1760-1772: Expanded quote informationThe
quotefunction has been updated with additional properties:
- New
id,title,user,fullName,vendor,shippingCost, andofferProductQuantityfields have been added.- The
CUSTOMERvariable, consistent with earlier changes.- The
productandquantityfields have been retained.These changes provide a more comprehensive structure for quote information, allowing for better tracking and management of quotes. Ensure that all systems interacting with this quote object are updated to handle these new properties.
To ensure compatibility, please run the following script to check for any usage of the old
quotestructure:#!/bin/bash # Search for old quote structure usage rg --type typescript "quote.*email|product|quantity" -g '!tests/pw/utils/testData.ts'
1736-1749: Enhanced quote rule configurationThe
quoteRulefunction has been significantly refactored:
- New properties like
applyOnAllProducts,specificProducts,includeProducts,specificCategories,categories,specificVendors,includeVendors, andexcludeVendorshave been added.- The
hideAddToCartButtonproperty has been retained.- A new
hidePriceTextproperty has been introduced.These changes provide more fine-grained control over quote rules, allowing for better customization of product visibility and pricing display. However, ensure that all parts of the system using this object are updated to handle these new properties correctly.
To ensure compatibility, please run the following script to check for any usage of the old
quoteRulestructure:tests/pw/utils/payloads.ts (2)
1646-1646: LGTM! Consider checking for email domain consistency.The change from a yopmail.com domain to an email.com domain for the billing email is appropriate. This update likely improves the consistency and reliability of test data.
Let's verify if this email domain change has been applied consistently across other payloads:
#!/bin/bash # Search for email addresses still using yopmail.com grep -n "yopmail.com" tests/pw/utils/payloads.ts # Search for email addresses using the new email.com domain grep -n "email.com" tests/pw/utils/payloads.ts
7-7: LGTM! Verify VENDOR_ID usage in payloads.The addition of
VENDOR_IDto the destructuredprocess.envobject is appropriate. This change suggests that vendor-specific operations are being introduced or expanded in the test scenarios.To ensure this change is properly utilized, let's verify the usage of
VENDOR_IDin the payloads:tests/pw/pages/requestForQuotationsPage.ts (3)
371-375: Ensure correct parameter type for vendor quote updatesIn the
vendorUpdateQuoteRequestmethod, the parameterquotehas been updated torequestForQuotation['vendorQuote']. Ensure that thevendorQuoteinterface includes all necessary fields used within this method.Confirm that all properties accessed on
quoteare defined inrequestForQuotation['vendorQuote']and update the interface if necessary.
515-517: Confirm fields for 'expectedDelivery' and 'additionalMessage'The code inputs
expectedDeliveryandadditionalMessage. Verify that these fields are always present on the form and that their inclusion is necessary for the test scenarios.Check if these fields are conditional and whether they need additional checks before interaction.
221-223: Verify the format of user selection inputsThe code is constructing the selector using
${quote.user}( ${quote.email} ). Ensure that this format matches the options presented in the dropdown for user selection.Run the following script to confirm that the user selection format aligns with the expected pattern:
tests/pw/utils/interfaces.ts (1)
1294-1295: Verify consistency of optional boolean propertiesIn the
quoteRuleinterface:
hideAddToCartButton?: boolean;is optional.keepBothCartQuoteButton?: boolean;is optional.These properties are related to button display logic. Verify that both properties are intended to be optional and that their defaults are handled correctly in the application logic to prevent unexpected behavior.
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Chores