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

Regression: Visitor being overwritten on call end #26756

Merged
merged 6 commits into from
Sep 1, 2022

Conversation

aleksandernsilva
Copy link
Contributor

Proposed changes (including videos or screenshots)

This PR adds a check to the createRoom method, responsible for creating VoIP rooms. It checks whether the visitor already exists before creating a new one, if one is found it uses it instead of overwriting existing visitors.

Issue(s)

Steps to test or reproduce

Further comments

const { contact } = await getContactBy({ phone });

if (contact) {
return contact as unknown as ILivechatVisitor;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type returned by useEndpoint is not compatible with the same type on core-typings. If any of you know how to solve this, please let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is because of Types Serialization, when we send some data over HTTP requests, the objects and types are converted to strings, in this case Date type is converted to string type which creates this problem.

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #26756 (0c0b252) into develop (717dc66) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #26756      +/-   ##
===========================================
+ Coverage    40.58%   40.61%   +0.03%     
===========================================
  Files          799      799              
  Lines        18309    18309              
  Branches      1957     1957              
===========================================
+ Hits          7431     7437       +6     
+ Misses       10579    10575       -4     
+ Partials       299      297       -2     
Flag Coverage Δ
e2e 40.61% <ø> (+0.03%) ⬆️

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

@aleksandernsilva aleksandernsilva marked this pull request as ready for review August 31, 2022 02:58
@aleksandernsilva aleksandernsilva requested a review from a team as a code owner August 31, 2022 02:58
KevLehman
KevLehman previously approved these changes Aug 31, 2022
Copy link
Contributor

@KevLehman KevLehman left a comment

Choose a reason for hiding this comment

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

Approving since it works and we need it on QA 😬
Pls FE folks, do your magic here.

@KevLehman KevLehman added this to the 5.1.0 milestone Aug 31, 2022
@aleksandernsilva aleksandernsilva modified the milestone: 5.1.0 Aug 31, 2022
MartinSchoeler
MartinSchoeler previously approved these changes Aug 31, 2022
const { contact } = await getContactBy({ phone });

if (contact) {
return contact as unknown as ILivechatVisitor;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is something that is not exclusive to this PR, I seen this happen in other places, we need to investigate if this is an inconsistency issue between our types.

This is a work that will go over the scope of this PR so we can do this separately

@ggazzo ggazzo added the stat: ready to merge PR tested and approved waiting for merge label Aug 31, 2022
@ggazzo ggazzo merged commit 4d340d9 into develop Sep 1, 2022
@ggazzo ggazzo deleted the regression/visitor-name-overwrite branch September 1, 2022 17:11
@murtaza98 murtaza98 mentioned this pull request Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants