Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Departments to typescript conversion and e2e tests #27792

Closed
wants to merge 19 commits into from

Conversation

FabioCavaleti
Copy link
Contributor

@FabioCavaleti FabioCavaleti commented Jan 19, 2023

In this PR I converted the omnichannel department files from javascript to typescript and create some tests for the add and remove agents functionalities.

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

OC-608

@CLAassistant
Copy link

CLAassistant commented Jan 19, 2023

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Merging #27792 (0f9150c) into develop (7532102) will increase coverage by 4.66%.
The diff coverage is 100.00%.

❗ Current head 0f9150c differs from pull request most recent head 118c17b. Consider uploading reports for the commit 118c17b to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #27792      +/-   ##
===========================================
+ Coverage    40.15%   44.82%   +4.66%     
===========================================
  Files          737      762      +25     
  Lines        14463    14800     +337     
  Branches      2033     2092      +59     
===========================================
+ Hits          5808     6634     +826     
+ Misses        8380     7870     -510     
- Partials       275      296      +21     
Flag Coverage Δ
e2e 44.78% <100.00%> (+4.65%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@FabioCavaleti FabioCavaleti marked this pull request as ready for review January 25, 2023 12:39
@FabioCavaleti FabioCavaleti requested review from a team as code owners January 25, 2023 12:39
const newAgent = { ...user, agentId: user?._id } as unknown as ILivechatDepartmentAgents;
setAgentList([newAgent, ...agentList]);
setUserId('');
setAgentsAdded((agents) => [...agents, { agentId: user?._id }]); // Search the real type of setAgentsAdded and type the function on the root props
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setAgentsAdded((agents) => [...agents, { agentId: user?._id }]); // Search the real type of setAgentsAdded and type the function on the root props
setAgentsAdded((agents) => [...agents, { agentId: user?._id }]);

Is this comment still necessary?

Comment on lines 12 to 16
agentId: Key;
username: string;
name?: string;
avatarETag?: string;
mediaQuery: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
agentId: Key;
username: string;
name?: string;
avatarETag?: string;
mediaQuery: boolean;
agentId: ILivechatDepartmentAgents['_id'];
username: ILivechatDepartmentAgents['username'];
name?: ILivechatDepartmentAgents['name'];
avatarETag?: ILivechatDepartmentAgents['avatarETag'];
mediaQuery: ILivechatDepartmentAgents['mediaQuery'];

You can use the ILivechatDepartmentAgents type here, but you must also have to update the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, the properties: name, avatarETag and mediaQuery is not part of ILivechatDepartmentsAgents type, what should I do with them?

@@ -68,6 +68,10 @@ export class OmnichannelDepartments {
return this.page.locator('table tr:first-child td:first-child');
}

get btnDeleteFirstRowInTable() {
return this.page.locator('table tr:first-child td:nth-child(6) button');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return this.page.locator('table tr:first-child td:nth-child(6) button');
return this.page.locator('button[title="Remove"]');

does it work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this conflict in the bad case there's other "remove" button around? 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was added but this isn't needed because of the new changes hehe, so I will delete this function

@MartinSchoeler MartinSchoeler changed the title Chore: Departments to typescript conversion and e2e tests Refactor: Departments to typescript conversion and e2e tests Mar 14, 2023
@MartinSchoeler MartinSchoeler changed the title Refactor: Departments to typescript conversion and e2e tests refactor: Departments to typescript conversion and e2e tests Mar 14, 2023
@aleksandernsilva
Copy link
Contributor

Due to conflicts and also convenience, changes proposed on this PR were moved to #28948.
This PR will closed in favor of the new one.

@aleksandernsilva aleksandernsilva deleted the chore/departments-to-ts branch April 18, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants