test(calling): add call history e2e flow#4994
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bf0733e67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const callerRecord = await waitForCallHistoryRecord( | ||
| callerPage, | ||
| (record) => | ||
| recordMatchesCallCase(record, { | ||
| counterpartNumber: calleeNumber, | ||
| direction: 'OUTGOING', | ||
| startedAt, | ||
| dispositions: UNANSWERED_CALLER_DISPOSITIONS, | ||
| }), |
There was a problem hiding this comment.
Because this serial suite runs the missed-call test immediately before this rejected-call test, these predicates can return the previous missed call: isRecentRecord() accepts rows up to 120 seconds before startedAt, the counterpart/direction are identical, and the callee-side rejected dispositions include MISSED. If the rejected history row is not created yet, waitForCallHistoryRecord() can pass using the earlier missed-call records, so the test no longer verifies that rejecting the new call wrote history. Snapshot existing record keys before placing this call or use the waitForNewHistoryRecord() helper here as the journey test does.
Useful? React with 👍 / 👎.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9babdedee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const callerCanceledRecord = await waitForCallHistoryRecord( | ||
| callerPage, | ||
| (record) => | ||
| recordMatchesCallCase(record, { | ||
| counterpartNumber: calleeNumber, | ||
| direction: 'OUTGOING', | ||
| startedAt, | ||
| dispositions: UNANSWERED_CALLER_DISPOSITIONS, | ||
| }), |
There was a problem hiding this comment.
In this serial suite, the answered-call test runs immediately before this missed-call test, and the caller-side matcher here accepts INITIATED for unanswered calls while the answered caller record can also be INITIATED (ANSWERED_DISPOSITIONS includes it). Because isRecentRecord() allows records up to 120 seconds before this call's startedAt, waitForCallHistoryRecord() can pass by returning the previous answered outgoing row before the new missed-call row exists, so the test may not verify caller history for this missed call. Snapshot existing keys before placing the call, or use the new-record helper as the journey test does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 239129e5fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const callerRecord = await waitForCallHistoryRecord( | ||
| callerPage, | ||
| (record) => | ||
| recordMatchesCallCase(record, { | ||
| counterpartNumber: calleeNumber, | ||
| direction: 'OUTGOING', | ||
| startedAt, | ||
| dispositions: ANSWERED_DISPOSITIONS, | ||
| }), | ||
| 'caller outgoing answered call' | ||
| ); |
There was a problem hiding this comment.
In the SET_2USER suite, callHistoryTests() runs immediately after callLifecycleTests()/callLifecycleMediaTests() (see set-2user.spec.ts), which already create answered calls between the same two accounts. Since isRecentRecord() accepts records up to 120 seconds before this test's startedAt and these predicates only match counterpart/direction/answered disposition, this can return a lifecycle record before the new answered call is written, so the test may pass without verifying this call's history. Snapshot existing history keys before placing the call or use the new-record helper used by the journey test.
Useful? React with 👍 / 👎.
| backup: {region: string; uris: string[]}; | ||
| }; | ||
|
|
||
| const CALLING_UMD_BUNDLE_PATH = path.resolve(__dirname, '../../../webex/umd/calling.min.js'); |
There was a problem hiding this comment.
Is this required to run call history? Does the default sample app not support CallHistory?
There was a problem hiding this comment.
This is not required. The default sample app already supports CallHistory through callHistory: true and exposes window.callHistory. I removed the custom UMD bundle routing so the tests use the default sample app bundle.
| await page.route('**/samples/calling.min.js', (route) => | ||
| route.fulfill({ | ||
| path: CALLING_UMD_BUNDLE_PATH, | ||
| contentType: 'application/javascript', | ||
| }) | ||
| ); | ||
| await page.route('**/samples/calling.min.js.map', (route) => | ||
| route.fulfill({ | ||
| path: CALLING_UMD_BUNDLE_MAP_PATH, | ||
| contentType: 'application/json', | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Addressed.The tests now use the default sample app bundle directly, since the sample app already supports CallHistory.
| 'Run three User 1 to User 2 outgoing call attempts: missed, rejected, and answered.', | ||
| 'Run three User 2 to User 1 outgoing call attempts: missed, rejected, and answered.', | ||
| 'Open the sample app Call History list for both users.', | ||
| ], | ||
| expected: [ | ||
| 'User 1 history contains the three recent outgoing User 1 to User 2 records.', | ||
| 'User 2 history contains the three recent outgoing User 2 to User 1 records.', | ||
| 'The table header renders the Call History columns and all expected rows are visible.', | ||
| ], | ||
| }, | ||
| { | ||
| id: 'CH-CALL-001', | ||
| title: 'Answered call creates exact per-user history records', | ||
| preconditions: ['Caller and callee are registered and can place/answer calls.'], | ||
| steps: [ | ||
| 'Caller dials callee.', | ||
| 'Callee answers and the call reaches established state on both pages.', | ||
| 'Caller ends the call.', | ||
| 'Poll Call History for caller and callee.', | ||
| 'Render each returned record through the Call History UI table.', | ||
| ], | ||
| expected: [ | ||
| 'Caller history contains a recent OUTGOING record for the callee.', | ||
| 'Callee history contains a recent INCOMING record for the caller.', | ||
| 'Each UI row shows the exact Janus direction, disposition, start time, end time, and session type.', | ||
| 'Call timing is valid: start/end are ISO dates, end is not before start, and duration is plausible.', | ||
| ], | ||
| }, | ||
| { | ||
| id: 'CH-CALL-002', | ||
| title: 'Missed call creates exact callee MISSED history', | ||
| preconditions: ['Caller and callee are registered; callee does not answer the incoming call.'], | ||
| steps: [ | ||
| 'Caller dials callee.', | ||
| 'Callee rings and intentionally does not answer.', | ||
| 'Caller ends the ringing call.', | ||
| 'Poll Call History for caller and callee.', | ||
| 'Render the returned records through the Call History UI table.', | ||
| ], | ||
| expected: [ | ||
| 'Callee history contains a recent INCOMING MISSED record for the caller.', | ||
| 'Caller history contains a recent OUTGOING unanswered record for the callee.', | ||
| 'The UI reflects the exact backend status and call timing for each user.', | ||
| ], | ||
| }, | ||
| { | ||
| id: 'CH-CALL-003', | ||
| title: 'Rejected call creates exact per-user history records', | ||
| preconditions: ['Caller and callee are registered; callee can reject the incoming call.'], | ||
| steps: [ | ||
| 'Caller dials callee.', | ||
| 'Callee rejects the ringing call from the UI.', | ||
| 'Wait until both users have no active calls.', | ||
| 'Poll Call History for caller and callee.', | ||
| 'Render the returned records through the Call History UI table.', | ||
| ], | ||
| expected: [ | ||
| 'Caller and callee both receive recent history rows for the rejected call.', | ||
| 'The UI renders the exact Janus direction, disposition, start time, end time, and session type.', | ||
| ], | ||
| }, | ||
| { | ||
| id: 'CH-ALL-001', | ||
| title: 'User 1 login shows missed, rejected, and answered outgoing call history', | ||
| preconditions: ['Both users stay initialized, registered, and media-ready in one browser run.'], | ||
| steps: [ | ||
| 'User 1 calls User 2 and User 2 does not answer.', | ||
| 'User 1 calls User 2 again and User 2 rejects.', | ||
| 'User 1 calls User 2 again and User 2 answers.', | ||
| 'After both users complete their outgoing call journeys, open Call History for User 1.', | ||
| ], | ||
| expected: [ | ||
| 'User 1 shows three recent OUTGOING records in one login.', | ||
| 'The missed, rejected, and answered attempts show the backend dispositions returned by Call History.', | ||
| 'Every record has valid start and end timestamps.', | ||
| 'The answered call includes valid non-negative duration derived from the timestamps.', | ||
| 'The sample app Call History table renders all three outgoing records for User 1.', | ||
| ], | ||
| }, | ||
| { | ||
| id: 'CH-ALL-002', | ||
| title: 'User 2 login shows missed, rejected, and answered outgoing call history', | ||
| preconditions: ['Both users stay initialized, registered, and media-ready in one browser run.'], | ||
| steps: [ | ||
| 'User 2 calls User 1 and User 1 does not answer.', | ||
| 'User 2 calls User 1 again and User 1 rejects.', | ||
| 'User 2 calls User 1 again and User 1 answers.', | ||
| 'After both users complete their outgoing call journeys, open Call History for User 2.', | ||
| ], | ||
| expected: [ | ||
| 'User 2 shows three recent OUTGOING records in one login.', | ||
| 'The missed, rejected, and answered attempts show the backend dispositions returned by Call History.', | ||
| 'Every record has valid start and end timestamps.', | ||
| 'The answered call includes valid non-negative duration derived from the timestamps.', | ||
| 'The sample app Call History table renders all three outgoing records for User 2.', | ||
| ], | ||
| }, | ||
| ] as const; |
There was a problem hiding this comment.
The specs shouldn't be in the code.
There was a problem hiding this comment.
Fixed. I removed the specs metadata from the test group and replaced it with explicit Playwright test cases. Since GitHub marks this thread as outdated, this should be covered by the latest changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9faf23f07e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const REJECTED_CALLEE_DISPOSITIONS = ['REJECTED', 'MISSED', 'CANCELED']; | ||
| const HISTORY_TIME_LOOKBACK_MS = 5000; | ||
| const RECENT_RECORD_TOLERANCE_MS = 120000; | ||
| const COUNTERPART_MATCH_MIN_DIGITS = 4; |
There was a problem hiding this comment.
Using only the last 4 digits to identify the counterpart can match unrelated call-history rows whenever two destinations share a suffix (common with enterprise extensions/DIDs), so waitForCallHistoryRecord() can resolve the wrong record and let assertions pass without validating the intended call. The helper default is already 7 digits, but this suite overrides it to 4, which materially weakens record identity in serial flows with many recent calls.
Useful? React with 👍 / 👎.
| record.other?.callbackAddress, | ||
| record.other?.primaryDisplayString, | ||
| record.other?.secondaryDisplayString, | ||
| record.other?.name, |
There was a problem hiding this comment.
Including record.other?.name in phoneMatchesRecord() can produce false matches because this field is a display label, not a canonical phone/address field, and it may contain digits (e.g., extensions/team labels). Since matching is suffix-based, a numeric name can satisfy the predicate even when the real callback/phone fields belong to a different call, causing history polling to select the wrong row.
Useful? React with 👍 / 👎.
| // Account roles resolved from testInfo.project.name → USER_SETS. | ||
| callLifecycleTests(); | ||
| callLifecycleMediaTests(); | ||
| callHistoryTests(); |
There was a problem hiding this comment.
We can use a different suite for this. The call history tests can be run in parallel
There was a problem hiding this comment.
I moved Call History into its own suite: set-2user-call-history.spec.ts, and added separate SET_2USER_CALL_HISTORY Playwright projects so it can run independently/parallel where account dependencies allow.
| .locator(CALLING_SELECTORS.DESTINATION_INPUT) | ||
| .fill(destination, {timeout: AWAIT_TIMEOUT}); | ||
| await page.locator(CALLING_SELECTORS.MAKE_CALL_BTN).click({timeout: AWAIT_TIMEOUT}); | ||
| await expect(page.locator(CALLING_SELECTORS.MAKE_CALL_BTN)).toBeDisabled({ |
There was a problem hiding this comment.
Why did we need these changes ? How are these related to call history tests ?
There was a problem hiding this comment.
These were not directly related to Call History. I removed the unrelated call.ts changes and kept the Call History-specific waits/assertions inside the Call History utilities/tests.
| * Navigate to the calling sample app | ||
| */ | ||
| export const navigateToCallingApp = async (page: Page): Promise<void> => { | ||
| await page.route('**/samples/calling.min.js', (route) => |
There was a problem hiding this comment.
Same comment here. What is this change about ?
There was a problem hiding this comment.
This override is not needed because the default sample app already supports CallHistory. I removed the page.route bundle override from navigateToCallingApp.
| } from '../utils/call-history'; | ||
| import {CALLING_SELECTORS} from '../constants'; | ||
|
|
||
| const CALL_HISTORY_AI_SPECS = [ |
There was a problem hiding this comment.
Why do we have these specs added in the testgroup ? Shouldn't this have testcases ?
There was a problem hiding this comment.
Those spec objects were redundant metadata and did not belong in the test group. I removed them and changed the file to use explicit Playwright test cases with the test IDs in the test names.
| }, | ||
| ] as const; | ||
|
|
||
| const ANSWERED_DISPOSITIONS = ['ANSWERED', 'INITIATED']; |
There was a problem hiding this comment.
These constants should be in constants file
There was a problem hiding this comment.
Done. I moved the Call History disposition/timing constants into playwright/constants/call-history.ts and export them through playwright/constants/index.ts.
|
|
||
| type UserLabel = HistoryDebugRecord['user']; | ||
|
|
||
| type CallJourneyLeg = { |
There was a problem hiding this comment.
These types also shouldn't be pasrt of test group. If we these types for testcases, then move to a separate file and import from there
There was a problem hiding this comment.
Done. I moved the Call History journey/matcher/debug types into playwright/utils/call-history-types.ts and import them where needed.
| return matchingRecord as CallHistoryRecord; | ||
| }; | ||
|
|
||
| const clearCallHistoryTable = async (page: Page): Promise<void> => { |
There was a problem hiding this comment.
Why do we have so many util methods created ? I see lot of them here as well as in test-group itself
There was a problem hiding this comment.
I cleaned this up. The test group now mostly contains the actual test cases. Shared low-level API/UI helpers remain in utils/call-history.ts, and journey-specific orchestration is isolated in utils/call-history-journey.ts so the test cases stay readable and we avoid duplicating polling, matching, and UI verification logic.
| @@ -0,0 +1,4 @@ | |||
| import {callHistoryTests} from '../test-groups/call-history'; | |||
There was a problem hiding this comment.
name doesnt have to set say set-2. If we have single file for call history tests then stating call-history-spec.ts in the name should be enough
There was a problem hiding this comment.
Done, renamed the suite file to call-history.spec.ts and updated the USER_SETS mapping accordingly.
| @@ -0,0 +1,45 @@ | |||
| import type {Page} from '@playwright/test'; | |||
There was a problem hiding this comment.
nitpick: are the types in these files used in multiple other files? im seeing type definitions in packages/calling/playwright/utils/call-history.ts as well? either those can be moved here or these definitions can be moved to the file they are used it.
There was a problem hiding this comment.
Done. I moved shared call-history types there, so types are not split between two files.
| const HISTORY_URL_PATTERN = '**/history/userSessions**'; | ||
| const HISTORY_EVENTUAL_CONSISTENCY_TIMEOUT = 150000; | ||
| const HISTORY_POLL_INTERVALS = [5000, 5000, 10000, 10000, 15000]; | ||
| const TIMING_TOLERANCE_MS = 120000; | ||
| const DURATION_TOLERANCE_SECONDS = 120; |
There was a problem hiding this comment.
move these to constants as well
There was a problem hiding this comment.
Done. I moved these into packages/calling/playwright/constants/call-history.ts:
CALL_HISTORY_URL_PATTERN, timeout, poll intervals, timing tolerance, duration tolerance.
|
|
||
| // Call History uses USER_1+USER_2 after their single-user suites complete, | ||
| // so it can run alongside the SET_2USER call lifecycle suite in PROD. | ||
| SET_2USER_CALL_HISTORY: { |
There was a problem hiding this comment.
you can call it SET_CALL_HISTORY since we are dropping the 2USER/3USER naming convention
There was a problem hiding this comment.
Done. Renamed SET_2USER_CALL_HISTORY to SET_CALL_HISTORY.
| // Call History has its own suite and can run in parallel with SET_2USER - PROD | ||
| // because it uses USER_1+USER_2 after those single-user suites complete. | ||
| { | ||
| name: 'SET_2USER_CALL_HISTORY - PROD', |
There was a problem hiding this comment.
same here.
you can call it
SET_CALL_HISTORYsince we are dropping the 2USER/3USER naming convention
There was a problem hiding this comment.
Done. Renamed SET_2USER_CALL_HISTORY - PROD to SET_CALL_HISTORY - PROD.
| }, | ||
| // INT aliases overlap between USER_1/2 and USER_4/5, so keep INT ordered. | ||
| { | ||
| name: 'SET_2USER_CALL_HISTORY - INT', |
There was a problem hiding this comment.
Done. Renamed SET_2USER_CALL_HISTORY - INT to SET_CALL_HISTORY - INT.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 702337cec6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .map(({expectedDisposition, record}) => ({ | ||
| ...record, | ||
| disposition: expectedDisposition, |
There was a problem hiding this comment.
In the bidirectional journey, this helper rewrites each API record's disposition before passing it to expectUiShowsHistoryRecords(), which then stubs the Janus response consumed by the sample app. Since the sample app renders the raw disposition column directly, any allowed backend value like INITIATED for an answered caller or CANCELED for a rejected caller gets replaced with ANSWERED/REJECTED, so the UI assertion can pass with a value the real UI would not have shown for that call. Use the actual record disposition here, or assert the normalization in code that actually performs it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. getDisplayHistoryRecords now preserves the actual backend record disposition instead of replacing it with expectedDisposition before the UI check. This keeps the stubbed Janus response aligned with what the sample app would really render.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a31052a43f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // because it uses USER_1+USER_2 after those single-user suites complete. | ||
| { | ||
| name: 'SET_CALL_HISTORY - PROD', | ||
| dependencies: ['SET_1 - PROD', 'SET_2 - PROD'], |
There was a problem hiding this comment.
This dependency list names projects that are not defined anywhere in this Playwright config: the registration projects above are generated as SET_REGISTRATION_1 - PROD and SET_REGISTRATION_2 - PROD, and the INT block below similarly depends on a nonexistent SET_2USER - INT (I checked the config with rg "SET_1|SET_2|SET_2USER|SET_REGISTRATION"). Playwright resolves dependencies by project name, so the new call-history projects make the config invalid before the suite can run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. Updated SET_CALL_HISTORY dependencies to use existing Playwright project names: SET_REGISTRATION_1 - PROD and SET_REGISTRATION_2 - PROD for PROD, and SET_CALL - INT for INT.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ad2eae054
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // because it uses USER_1+USER_2 after those single-user suites complete. | ||
| { | ||
| name: 'SET_CALL_HISTORY - PROD', | ||
| dependencies: ['SET_REGISTRATION_1 - PROD', 'SET_REGISTRATION_2 - PROD'], |
There was a problem hiding this comment.
In PROD this project can start as soon as the USER_1/USER_2 registration suites finish, while SET_CALL_TRANSFER_CONSULT - PROD still uses USER_1/USER_2/USER_3 and only depends on SET_CALL - PROD. With workers: 10, Playwright can therefore run the new call-history contexts for USER_1/USER_2 at the same time as the transfer suite after SET_CALL completes, violating the account constraint in test-data.ts and causing registration/call collisions. Please add explicit ordering between SET_CALL_HISTORY and SET_CALL_TRANSFER_CONSULT and mirror the same serialization for INT.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed. SET_CALL_TRANSFER_CONSULT now explicitly depends on SET_CALL_HISTORY for both PROD and INT, so the transfer suite cannot run concurrently with call-history while both use USER_1/USER_2.
| await expectUiShowsHistoryRecord(calleePage, calleeRecord); | ||
| }); | ||
|
|
||
| test('CH-LIST-001 / CH-ALL-001 / CH-ALL-002: Bidirectional journey renders in Call History UI', async ({}, testInfo) => { |
There was a problem hiding this comment.
Why does the index have to be CH-LIST-001 / CH-ALL-001 / CH-ALL-002 ?
There was a problem hiding this comment.
The test title should map to one clear test case ID. I renamed it from CH-LIST-001 / CH-ALL-001 / CH-ALL-002 to only CH-LIST-001.
| @@ -0,0 +1,97 @@ | |||
| import type {Page} from '@playwright/test'; | |||
|
|
|||
| export type SortOrder = 'ASC' | 'DESC'; | |||
There was a problem hiding this comment.
These types can directly be mentioned in the CallHistoryQuery, we don't need necessarily need a separate type for them
There was a problem hiding this comment.
Updated. Removed SortOrder and SortBy; the unions are now inline in CallHistoryQuery.
|
|
||
| export type CallJourneyOutcome = 'ANSWERED' | 'REJECTED' | 'MISSED'; | ||
| export type CallHistoryDisposition = CallJourneyOutcome | 'CANCELED'; | ||
| export type UserLabel = 'user1' | 'user2'; |
There was a problem hiding this comment.
This could just be a string type wherever being used, don't need a type for this
There was a problem hiding this comment.
This was not adding much value as a separate type, so I changed those fields to use string directly.
| forwardedBy: string; | ||
| }; | ||
|
|
||
| export type CallJourneyOutcome = 'ANSWERED' | 'REJECTED' | 'MISSED'; |
There was a problem hiding this comment.
This and below can be single type. Anyway the values are in 'OR' operation
There was a problem hiding this comment.
These were small union types used together, so I removed the extra aliases and kept the needed union values inline where they are used.
| records.forEach((record) => seenKeys.add(getHistoryRecordKey(record))); | ||
| }; | ||
|
|
||
| export const endCallerIfStillActive = async (page: Page): Promise<void> => { |
There was a problem hiding this comment.
This method seems like it's being used for ending the call so how is this related to history. Check if we already have some existing method in call.ts to do so and if not move this method there and import from there wherever needed.
There was a problem hiding this comment.
This helper is about ending/cleaning up an active call, not call history. I moved it from call-history-journey.ts to utils/call.ts and imported it from there.
| UserLabel, | ||
| } from './call-history-types'; | ||
|
|
||
| export const attachCallHistorySummary = async ( |
There was a problem hiding this comment.
Move waitForCallHistoryCase, recordMatchesCallCase, expectDisposition, attachCallHistorySummary, getDisplayHistoryRecords to call-history.ts as the methods are single leg operations anyway. If we already have one files where we are placing methods to perform certain actions so that tests can be run smoothly then another file created like call-history-journey should have a particular purpose and should be only used for that. From what i understand that there are certain operations which require multi-leg orhcestration so aprticularyly those methods can be kept here
There was a problem hiding this comment.
I moved waitForCallHistoryCase, recordMatchesCallCase, attachCallHistorySummary, and getDisplayHistoryRecords to call-history.ts. Now call-history-journey.ts is focused only on multi-leg journey orchestration.
| export const normalizeDisposition = (disposition?: string): string => | ||
| (disposition ?? '').toUpperCase(); | ||
|
|
||
| export const normalizeDirection = (direction?: string): string => (direction ?? '').toUpperCase(); |
There was a problem hiding this comment.
This method is only used once and it's a single line method so the logic can be directly pasted there, don't think we need separate function for that. Do we have other usecases of this ,method ?
There was a problem hiding this comment.
These were thin one-line helpers and not needed as exported functions. I removed them and used the uppercase logic directly at the matching point.
| ): Promise<CallHistoryRecord> => | ||
| waitForCallHistoryRecord(page, (record) => recordMatchesCallCase(record, options), description); | ||
|
|
||
| export const expectDisposition = (record: CallHistoryRecord, expected: string[]): void => { |
There was a problem hiding this comment.
Same comment here. Why is this method needed. if we already have a method to normalize disposition then just for a single expect statement why is another method needed
There was a problem hiding this comment.
This wrapper was unnecessary because waitForCallHistoryCase already filters records by expected dispositions. I removed the extra expectDisposition call and helper.
| }); | ||
|
|
||
| /* eslint-disable no-empty-pattern */ | ||
| test('CH-CALL-001: Answered call creates exact per-user history records', async ({}, testInfo) => { |
There was a problem hiding this comment.
Do we have a test which atleast checks sort/limit/sortBy query parameters to justify their existence ?
There was a problem hiding this comment.
I added CH-QUERY-001, which verifies that days, limit, sort, and sortBy are passed correctly to getCallHistoryData.
|
|
||
| const normalizePhoneNumber = (value?: string): string => (value ?? '').replace(/\D/g, ''); | ||
|
|
||
| export const phoneMatchesRecord = ( |
There was a problem hiding this comment.
Would be good to add JSDOC comments for these functions for readability perspective
There was a problem hiding this comment.
I added short JSDoc comments for the exported call-history helper functions to make their purpose easier to understand.
4 participants
COMPLETES #https://jira-eng-sjc12.cisco.com/jira/browse/CAI-7876
This pull request addresses
Adds automated Playwright e2e coverage for the Webex Calling Call History flow in the two-user calling suite. The tests validate that call history records are created and rendered correctly for answered, missed, and rejected call scenarios.
by making the following changes
SET_2USERsuite.Change Type
The following scenarios were tested
SET_2USER.The GAI Coding Policy And Copyright Annotation Best Practices
I certified that