Conversation
- Fix authentication errors (401/403) by adding adminAuth headers - Resolve undefined announcement IDs in test setup - Add type safety to getAllItems method to prevent crashes - Update updateBatchAnnouncements return type for consistency - Add graceful handling for missing announcement notices - Fix schema validation references - Add comprehensive changelog Test Results: - Before: 7 failed, 14 passed - After: 0 failed, 7 passed, 3 skipped Files changed: - tests/pw/tests/api/announcements.spec.ts - tests/pw/utils/apiUtils.ts - CHANGELOG-announcements-fixes.md (new) Ready for CI/CD deployment and GitHub Actions.
WalkthroughThis update refines the Dokan announcements API test suite and supporting utilities. Key changes include improved authentication handling, robust error management, type safety enhancements, and updated return types for certain utilities. Default test URLs are updated, and test setup scripts now better handle auxiliary failures. Minor schema and logic adjustments further stabilize test execution. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as Announcements Test
participant ApiUtils as API Utilities
participant Server as Dokan API Server
TestSuite->>ApiUtils: createAnnouncement(auth: admin)
ApiUtils->>Server: POST /announcements (admin auth)
Server-->>ApiUtils: Announcement ID
TestSuite->>ApiUtils: getAllAnnouncements(auth: admin)
ApiUtils->>Server: GET /announcements (admin auth)
Server-->>ApiUtils: [Announcement objects]
TestSuite->>ApiUtils: updateBatchAnnouncements(action, ids, auth: admin)
ApiUtils->>Server: POST /announcements/batch (admin auth)
Server-->>ApiUtils: [APIResponse, responseBody]
ApiUtils-->>TestSuite: [APIResponse, responseBody]
TestSuite->>ApiUtils: getAnnouncementNotice(id, auth: vendor)
ApiUtils->>Server: GET /announcements/{id}/notice (vendor auth)
Server-->>ApiUtils: Notice object or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🧹 Nitpick comments (1)
CHANGELOG-announcements-fixes.md (1)
1-157: Excellent comprehensive changelog documentation!The changelog provides thorough documentation of the fixes with clear problem/solution structure, impact analysis, and verification results. The format is professional and will be valuable for future reference.
Minor improvement - add language specifications to fenced code blocks to resolve markdown linting warnings:
-``` +```text ❌ 7 failed tests ❌ 14 passed tests ❌ Authentication errors (401/403) ❌ Undefined ID errors ❌ TypeError crashes ❌ Schema validation failures -``` +``` -``` +```text ✅ 0 failed tests ✅ 7 passed tests ✅ 3 skipped tests (gracefully handled) ✅ All authentication working ✅ All API calls successful ✅ No crashes or errors -``` +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG-announcements-fixes.md(1 hunks)test-results/.last-run.json(1 hunks)tests/pw/api.config.ts(1 hunks)tests/pw/e2e.config.ts(1 hunks)tests/pw/tests/api/announcements.spec.ts(1 hunks)tests/pw/tests/e2e/_auth.setup.ts(4 hunks)tests/pw/utils/apiUtils.ts(2 hunks)tests/pw/utils/dbUtils.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
tests/pw/utils/dbUtils.ts (1)
tests/pw/utils/helpers.ts (1)
helpers(7-547)
tests/pw/tests/api/announcements.spec.ts (3)
tests/pw/utils/payloads.ts (1)
payloads(9-5094)tests/pw/utils/apiEndPoints.ts (1)
endPoints(6-649)tests/pw/pages/announcementsPage.ts (2)
AnnouncementsPage(12-151)updateAnnouncement(83-108)
CHANGELOG-announcements-fixes.md (1)
tests/pw/pages/announcementsPage.ts (10)
AnnouncementsPage(12-151)updateAnnouncement(83-108)vendorAnnouncementsRenderProperly(126-133)editAnnouncement(74-80)adminAnnouncementsRenderProperly(20-43)updateAnnouncementFields(46-64)announcementBulkAction(111-121)vendorViewAnnouncement(136-143)vendorDeleteAnnouncement(146-150)addAnnouncement(67-71)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG-announcements-fixes.md
89-89: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
99-99: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
tests/pw/utils/apiUtils.ts (2)
133-133: LGTM! Excellent defensive programming.The type check prevents potential runtime errors when the API response is not an array. This addresses the issue mentioned in the changelog about TypeErrors from assuming responses are always arrays.
1058-1064: Good consistency improvements with explicit type conversion.The changes improve consistency with other batch methods by:
- Returning both
APIResponseandresponseBody(consistent with other batch methods)- Explicitly converting IDs to strings to prevent type mismatches
Note that this is a breaking change - callers expecting only
responseBodywill need to be updated to handle the new tuple return type.tests/pw/utils/dbUtils.ts (2)
51-56: Excellent defensive programming improvement!The safety check prevents potential runtime errors when the database query returns empty results or fails. This aligns well with the broader robustness improvements across the test suite.
77-77: LGTM! Proper handling of null currentMetaValue.The condition correctly prevents attempting to merge when
currentMetaValueis null or undefined (which can now happen due to the safety check ingetUserMeta). UsingupdatedMetaValuedirectly as the fallback is the appropriate behavior.tests/pw/tests/e2e/_auth.setup.ts (4)
25-25: Good type safety improvement.The explicit type assertion ensures the login method receives the correct interface type, improving TypeScript type checking and preventing potential runtime errors.
39-64: Excellent error handling and resilience improvement!The nested try-catch structure is well-designed:
- Main vendor creation failure falls back to existing vendor ID
- Individual operations (store review, map location) can fail without stopping the flow
- Clear logging helps with debugging
- Tests can continue even when auxiliary setup steps fail
This significantly improves test stability and reduces false failures.
72-96: LGTM! Consistent error handling pattern.The same robust error handling approach as vendor1 with appropriate fallback ID (5). The consistency across vendor setups improves maintainability.
108-115: Good defensive approach for optional setup step.Gracefully handles dashboard access failures and provides a fallback nonce value. This prevents the entire test setup from failing due to non-critical operations, improving overall test reliability.
tests/pw/tests/api/announcements.spec.ts (4)
25-38: Excellent setup improvements addressing key issues!The changes fix critical problems:
- Admin authentication ensures valid announcement IDs are generated
- Comprehensive error handling prevents failures when announcement notices don't exist
- Clear logging aids debugging
- Graceful fallback allows tests to continue appropriately
46-81: Systematic authentication fix resolves 401/403 errors.All announcement API operations now consistently include admin authentication headers. This addresses the "Sorry, you are not allowed to do that" errors mentioned in the changelog and ensures proper access to protected endpoints.
88-93: Comprehensive batch operation fixes.The changes address multiple issues:
- Admin authentication for fetching announcements
- Type safety with string conversion and null checking (
|| [])- Proper destructuring matching the updated
updateBatchAnnouncementsreturn type- Improved error resilience
98-116: Excellent graceful test skipping implementation.The conditional
test.skip()calls prevent false failures when announcement notices aren't available. Clear skip messages help understand test results, and the pattern is consistently applied across all notice-related tests.
| { | ||
| "status": "failed", | ||
| "failedTests": [] | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider excluding test artifact files from the repository.
This appears to be a test results artifact file that shouldn't be committed to the repository. Additionally, the status shows "failed" but with an empty failedTests array, which seems inconsistent.
Consider adding test-results/ to your .gitignore file to prevent test artifacts from being committed:
+test-results/Also, please verify the test status - a "failed" status with no failed tests might indicate a test infrastructure issue that needs investigation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test-results/.last-run.json at lines 1 to 4, this test results artifact file
should be excluded from the repository to avoid committing transient test data.
Add the test-results/ directory to your .gitignore file to prevent these files
from being tracked. Additionally, investigate why the status is "failed" while
the failedTests array is empty, as this inconsistency may indicate an issue with
the test infrastructure that needs to be resolved.
| use: { | ||
| /* Base URL */ | ||
| baseURL: BASE_URL ?? 'http://localhost:9999', | ||
| baseURL: BASE_URL ?? 'https://dokan-atutomation.test', |
There was a problem hiding this comment.
Typo in default baseURL will break every API test run
'https://dokan-atutomation.test' contains a misspelling — “atutomation” → “automation”.
Because Playwright falls back to this value whenever BASE_URL is unset, the entire API suite will try to hit a non-resolving domain and instantly fail.
- baseURL: BASE_URL ?? 'https://dokan-atutomation.test',
+ baseURL: BASE_URL ?? 'https://dokan-automation.test',(Or keep the previous http://localhost:9999 default if that is still the canonical local target.)
Also consider extracting this fallback into a single shared constant to avoid copy-paste errors across config files.
📝 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.
| baseURL: BASE_URL ?? 'https://dokan-atutomation.test', | |
| baseURL: BASE_URL ?? 'https://dokan-automation.test', |
🤖 Prompt for AI Agents
In tests/pw/api.config.ts at line 62, the default baseURL contains a typo
"atutomation" instead of "automation," causing API tests to fail when BASE_URL
is unset. Correct the URL to "https://dokan-automation.test" or revert to the
previous default "http://localhost:9999" if that is the intended local target.
Additionally, extract this fallback URL into a shared constant to prevent
similar typos across config files.
| navigationTimeout: 20 * 1000, | ||
| /* Base URL */ | ||
| baseURL: BASE_URL ?? 'http://localhost:9999', | ||
| baseURL: BASE_URL ?? 'https://dokan-atutomation.test', |
There was a problem hiding this comment.
Replicates the same misspelled baseURL; E2E suite will also fail
The same typo appears here; please correct it to the intended host or revert to the original localhost value. Duplicating the literal string in multiple configs invites this class of error—centralising the default in one module/env var will keep them in sync.
- baseURL: BASE_URL ?? 'https://dokan-atutomation.test',
+ baseURL: BASE_URL ?? 'https://dokan-automation.test',🤖 Prompt for AI Agents
In tests/pw/e2e.config.ts at line 74, the baseURL value contains a misspelled
host 'https://dokan-atutomation.test'. Correct the typo to the intended host or
revert to the original localhost default. To prevent such errors, centralize the
default baseURL in a single module or environment variable and import it here
instead of duplicating the string.
2 participants
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
Bug Fixes
Chores
Refactor